fix(isolation): complete reports false success when worktree remains on disk (fixes #964)#1034
Merged
Wirasm merged 3 commits intocoleam00:devfrom Apr 14, 2026
Merged
Conversation
📝 WalkthroughWalkthroughremoveEnvironment now returns a structured RemoveEnvironmentResult and callers handle skipped, partial, and success outcomes. Worktree provider runs git worktree prune and verifies registration post-removal, emitting warnings and marking worktreeRemoved false if still registered. CLI and cleanup callers record/report these outcomes and tests updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI (isolationCompleteCommand)
participant Service as CleanupService (removeEnvironment)
participant Provider as WorktreeProvider (destroy)
participant Git as Git (worktree cmd)
CLI->>Service: removeEnvironment(envId, options)
Service->>Provider: provider.destroy(...)
Provider->>Git: git worktree remove --force
Provider->>Git: git worktree prune
Provider->>Git: git worktree list --porcelain
alt worktree still registered
note right of Provider: append warning\nset worktreeRemoved = false
Provider-->>Service: { worktreeRemoved: false, branchDeleted: ..., warnings: [...] }
else removed successfully
Provider-->>Service: { worktreeRemoved: true, branchDeleted: ..., warnings: [...] }
end
Service-->>CLI: RemoveEnvironmentResult (may include skippedReason)
alt skippedReason present
CLI->>CLI: Log "Blocked: <branch> — <skippedReason>"
else worktreeRemoved === false
CLI->>CLI: Log "Partial: <branch> — worktree was not removed..." and log warnings
else success
CLI->>CLI: Log "Completed: <branch>"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/isolation.test.ts`:
- Around line 395-429: Add a regression test in isolation.test.ts that exercises
the "partial" path by mocking mockRemoveEnvironment (used by
isolationCompleteCommand) to return worktreeRemoved: false (and no
skippedReason) so the !result.worktreeRemoved branch in isolation.ts is
executed; call isolationCompleteCommand with that mock and assert the CLI output
reflects a failed/partial cleanup (check consoleErrorSpy/consoleLogSpy
counts/messages consistent with the implementation) to prevent regressions that
would incorrectly mark partial cleanups as Completed.
In `@packages/cli/src/commands/isolation.ts`:
- Around line 320-323: The partial-path error message incorrectly asserts
"branch/DB cleaned up" even when result.branchDeleted may be false/null; update
the branch to use result.branchDeleted to build the message (or remove the
assertion) so it only reports branch/DB cleanup when result.branchDeleted ===
true, e.g. change the console.error in the else-if that checks
result.worktreeRemoved to conditionally append "branch/DB cleaned up" only when
result.branchDeleted is truthy and otherwise state "branch/DB not removed" or
omit that clause; refer to result.worktreeRemoved and result.branchDeleted to
implement the correct conditional messaging in the function handling the result.
In `@packages/core/src/services/cleanup-service.ts`:
- Around line 210-214: removeEnvironment() now returns a result with
partial/skipped flags (worktreeRemoved, branchDeleted, warnings), but the batch
callers (runScheduledCleanup, cleanupStaleWorktrees, cleanupMergedWorktrees)
still treat any fulfilled promise as a successful removal; update those callers
to inspect the returned object from removeEnvironment() (and
isolationCompleteCommand() where applicable) and only add an entry to the
removed list when destroyResult.worktreeRemoved (or branchDeleted when that is
the metric you intend to record) is true; for false/skipped cases, record them
separately (e.g., skipped or warnings list) and propagate destroyResult.warnings
instead of treating them as successes.
In `@packages/isolation/src/providers/worktree.ts`:
- Around line 182-198: Move the git pruning and post-removal verification out
from behind the pathExists guard so prune (execFileAsync(... 'git worktree
prune')) and isWorktreeRegistered(repoPath, worktreePath) always run after
repoPath is resolved (even if the worktree directory is missing); if
verification fails mark result.worktreeRemoved = false, push a warning into
result.warnings (partial cleanup) and log the raw error with
getLog().debug/error. When catching git errors during prune/verification, pass
the caught error through classifyIsolationError() to produce a user-friendly
message to include in the result/warning, but always log the original error for
debugging; update the destroy() flow to use classifyIsolationError() for git
exec errors and ensure timeouts/permission/no-repo/no-space are mapped and
surfaced to the caller.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 777181f2-3c82-4626-aacf-a0f6cc886abe
📒 Files selected for processing (5)
packages/cli/src/commands/isolation.test.tspackages/cli/src/commands/isolation.tspackages/core/src/services/cleanup-service.test.tspackages/core/src/services/cleanup-service.tspackages/isolation/src/providers/worktree.ts
Comment on lines
+182
to
+198
| // Prune stale worktree references (git may keep refs to deleted paths) | ||
| try { | ||
| await execFileAsync('git', ['-C', repoPath, 'worktree', 'prune'], { timeout: 15000 }); | ||
| } catch (_error) { | ||
| // Best-effort — pruning failure is not critical | ||
| getLog().debug({ repoPath }, 'worktree_prune_failed'); | ||
| } | ||
|
|
||
| // Post-removal verification: confirm worktree is actually gone from git | ||
| if (result.worktreeRemoved) { | ||
| const stillRegistered = await this.isWorktreeRegistered(repoPath, worktreePath); | ||
| if (stillRegistered) { | ||
| result.worktreeRemoved = false; | ||
| const warning = `Worktree at ${worktreePath} was reported removed but is still registered in git`; | ||
| getLog().warn({ worktreePath, repoPath }, 'worktree_removal_verification_failed'); | ||
| result.warnings.push(warning); | ||
| } |
There was a problem hiding this comment.
Run prune/verification even when the worktree path is already missing.
This block still sits behind the pathExists guard from Line 144, so a ghost worktree whose directory was deleted manually never reaches git worktree prune or git worktree list --porcelain. In that case destroy() still returns worktreeRemoved: true, even though the entry can remain registered in git. Please move the prune/verification step after repoPath is resolved, and treat verification failures as partial cleanup instead of success.
Based on learnings: Use classifyIsolationError() from @archon/isolation to map git errors (permission denied, timeout, no space, not a git repo) to user-friendly messages; always log the raw error for debugging; send classified message to the user.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/isolation/src/providers/worktree.ts` around lines 182 - 198, Move
the git pruning and post-removal verification out from behind the pathExists
guard so prune (execFileAsync(... 'git worktree prune')) and
isWorktreeRegistered(repoPath, worktreePath) always run after repoPath is
resolved (even if the worktree directory is missing); if verification fails mark
result.worktreeRemoved = false, push a warning into result.warnings (partial
cleanup) and log the raw error with getLog().debug/error. When catching git
errors during prune/verification, pass the caught error through
classifyIsolationError() to produce a user-friendly message to include in the
result/warning, but always log the original error for debugging; update the
destroy() flow to use classifyIsolationError() for git exec errors and ensure
timeouts/permission/no-repo/no-space are mapped and surfaced to the caller.
Contributor
Author
|
Thanks @coderabbitai for the thorough review! Addressing each point: 1. Regression test for Partial branch ✅ Already covered — the test at line ~395 mocks 2. Branch cleanup message accuracy ✅ Already fixed — the Partial message conditionally shows 3. Batch cleanup callers honoring result — Good observation. The batch callers ( 4. Prune behind pathExists guard — Actually this is already handled correctly: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the code claims before responding. 🏁 Script executed: Length of output: 8259 Let me dig further to verify the Partial test and the conditional branch message logic. 🏁 Script executed: Length of output: 3042 One more check — need to verify the conditional 🏁 Script executed: Length of output: 1461
✅ Point 1 — Partial regression test: Confirmed.
` Partial: ${branch} — worktree was not removed from disk (branch/DB cleaned up)`There is no conditional check on A correct implementation would be: const cleanupDetail = result.branchDeleted
? '(branch/DB cleaned up)'
: '(DB marked destroyed)';
console.error(
` Partial: ${branch} — worktree was not removed from disk ${cleanupDetail}`
);✅ Point 3 — Batch cleanup callers: Understood — that's a reasonable scope boundary. Would you like me to open a follow-up GitHub issue to track adding per-result granularity to the batch callers ( ✅ Point 4 — Prune behind 🧠 Learnings used |
skundu42
pushed a commit
to skundu42/Archon
that referenced
this pull request
Apr 10, 2026
…eam00#1036) * fix(env): detect and refuse target-repo .env with sensitive keys (coleam00#1034) Bun auto-loads .env from subprocess CWD regardless of the clean env passed to Bun.spawn, silently overriding OAuth auth and billing the wrong API account. This adds a consent-based gate at registration time and a pre-spawn safety net in both Claude and Codex clients. Changes: - Add env-leak-scanner utility that checks 6 auto-loaded .env filenames for 7 sensitive keys (ANTHROPIC_API_KEY, OPENAI_API_KEY, etc.) - Add allow_env_keys boolean column to codebases table (migration 021) - Gate registerRepoAtPath to reject codebases with sensitive .env keys unless explicitly consented via allowEnvKeys flag - Add pre-spawn check in ClaudeClient and CodexClient sendQuery methods - Return 422 from POST /api/codebases on env leak detection - Surface env leak error in web UI with "Allow env keys" checkbox - Classify EnvLeakError as FATAL in workflow executor Fixes coleam00#1034 * fix: address review findings for env leak scanner PR - Fix FATAL_PATTERNS 'env leak' pattern that never matched EnvLeakError.message; now checks error.name === 'EnvLeakError' directly (immune to message rewording) - Fix pre-spawn consent lookup for worktree paths: add findCodebaseByPathPrefix() and use it as fallback when exact match returns null; prevents opt-in from being silently ineffective for workflow-based runs - Add allow_env_keys column to 000_combined.sql CREATE TABLE and idempotent ALTER section to fix fresh PostgreSQL installs - Remove non-existent --allow-env-keys CLI flag from error message; replace with web UI-only instruction - Narrow isEnvLeakError check from error.message.includes('env') to startsWith('Cannot add codebase') - Distinguish ENOENT (skip) from EACCES/other errors in scanner catch block; unreadable files now surface as findings to avoid silently bypassing the gate - Use cross-platform grep command instead of macOS-specific sed -i '' syntax - Add audit log (log.warn) when 422 EnvLeakError is returned from API - Add pre-spawn gate tests to claude.test.ts and codex.test.ts (4 tests each) - Add env leak gate tests to clone.test.ts (2 tests) - Add 422 and allowEnvKeys passthrough tests to api.codebases.test.ts * simplify: reduce complexity in changed files
Contributor
Author
|
Addressed all CodeRabbit review comments in d9877bb:
All three packages pass |
🧩 Analysis chainLet me verify the changes before triggering the re-review. 🏁 Script executed: Length of output: 40145
Triggering a full re-review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/cli/src/commands/isolation.test.ts (1)
395-429:⚠️ Potential issue | 🟡 MinorMissing regression test for the
Partialbranch (!result.worktreeRemovedwithoutskippedReason).The test at lines 395-411 tests
skippedReason, but the critical!result.worktreeRemovedbranch (Lines 320-330 in isolation.ts) is not exercised. A regression back to "Completed" on partial cleanup would still pass.🧪 Suggested test to add for the Partial path
it('counts as failed when worktree removal fails but no skippedReason (partial cleanup)', async () => { mockFindActiveByBranchName.mockResolvedValueOnce(mockEnv); mockRemoveEnvironment.mockResolvedValueOnce({ worktreeRemoved: false, branchDeleted: true, warnings: ['Worktree at /test/worktree was reported removed but is still registered in git'], }); await isolationCompleteCommand(['partial-branch'], { force: true, deleteRemote: true }); expect(consoleErrorSpy).toHaveBeenCalledWith( ' Partial: partial-branch — worktree was not removed from disk (branch deleted, DB updated)' ); expect(consoleErrorSpy).toHaveBeenCalledWith( ' ⚠ Worktree at /test/worktree was reported removed but is still registered in git' ); expect(consoleLogSpy).toHaveBeenCalledWith('\nComplete: 0 completed, 1 failed, 0 not found'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/isolation.test.ts` around lines 395 - 429, The test suite is missing a regression test for the Partial branch where result.worktreeRemoved is false but skippedReason is absent; add a new test that stubs mockFindActiveByBranchName to return mockEnv and mockRemoveEnvironment to return { worktreeRemoved: false, branchDeleted: true, warnings: [...] } then call isolationCompleteCommand(['partial-branch'], { force: true, deleteRemote: true }) and assert consoleErrorSpy is called with the "Partial: partial-branch — worktree was not removed from disk (branch deleted, DB updated)" message, that consoleErrorSpy logs the warning prefixed with "⚠", and that consoleLogSpy reports '\nComplete: 0 completed, 1 failed, 0 not found'; reference isolationCompleteCommand, mockRemoveEnvironment, mockFindActiveByBranchName, consoleErrorSpy and consoleLogSpy to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/isolation.ts`:
- Around line 309-329: The duplicate warning output comes from printing
result.warnings both unconditionally at the top and again inside the partial
cleanup branch; remove the initial unconditional loop over result.warnings and
instead print warnings only inside the relevant branches (when
result.skippedReason is set and when !result.worktreeRemoved). Concretely,
delete the top-level for (const warning of result.warnings) console.warn block
and add a single warning loop inside the skippedReason branch (alongside the
"Blocked" message) and reuse the existing loop inside the
!result.worktreeRemoved branch so each warning is emitted once and in the
appropriate format; keep references to result.warnings, result.skippedReason,
result.worktreeRemoved, branchDeleted and branch to preserve current messages
and formatting.
---
Duplicate comments:
In `@packages/cli/src/commands/isolation.test.ts`:
- Around line 395-429: The test suite is missing a regression test for the
Partial branch where result.worktreeRemoved is false but skippedReason is
absent; add a new test that stubs mockFindActiveByBranchName to return mockEnv
and mockRemoveEnvironment to return { worktreeRemoved: false, branchDeleted:
true, warnings: [...] } then call isolationCompleteCommand(['partial-branch'], {
force: true, deleteRemote: true }) and assert consoleErrorSpy is called with the
"Partial: partial-branch — worktree was not removed from disk (branch deleted,
DB updated)" message, that consoleErrorSpy logs the warning prefixed with "⚠",
and that consoleLogSpy reports '\nComplete: 0 completed, 1 failed, 0 not found';
reference isolationCompleteCommand, mockRemoveEnvironment,
mockFindActiveByBranchName, consoleErrorSpy and consoleLogSpy to locate the
code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b64ac9d-fb76-45f2-be00-b66260b456ac
📒 Files selected for processing (5)
packages/cli/src/commands/isolation.test.tspackages/cli/src/commands/isolation.tspackages/core/src/services/cleanup-service.test.tspackages/core/src/services/cleanup-service.tspackages/isolation/src/providers/worktree.ts
Comment on lines
+309
to
+329
| // Surface warnings from partial cleanup | ||
| for (const warning of result.warnings) { | ||
| console.warn(` Warning: ${warning}`); | ||
| } | ||
|
|
||
| if (result.skippedReason) { | ||
| console.error(` Blocked: ${branch} — ${result.skippedReason}`); | ||
| if (result.skippedReason === 'has uncommitted changes') { | ||
| console.error(' Use --force to override.'); | ||
| } | ||
| failed++; | ||
| } else if (!result.worktreeRemoved) { | ||
| const parts: string[] = []; | ||
| if (result.branchDeleted) parts.push('branch deleted'); | ||
| parts.push('DB updated'); | ||
| console.error( | ||
| ` Partial: ${branch} — worktree was not removed from disk (${parts.join(', ')})` | ||
| ); | ||
| for (const warning of result.warnings) { | ||
| console.error(` ⚠ ${warning}`); | ||
| } |
There was a problem hiding this comment.
Duplicate warning output in partial cleanup path.
When worktreeRemoved is false, warnings are printed twice:
- Lines 310-312:
console.warn(Warning: ${warning})for all warnings - Lines 327-329:
console.error(⚠ ${warning})for all warnings
This results in duplicate output for the same warnings with different formatting.
🔧 Suggested fix: Move the warning loop inside the else branches
try {
const result: RemoveEnvironmentResult = await removeEnvironment(env.id, {
force: options.force,
deleteRemoteBranch: options.deleteRemote ?? true,
});
- // Surface warnings from partial cleanup
- for (const warning of result.warnings) {
- console.warn(` Warning: ${warning}`);
- }
-
if (result.skippedReason) {
console.error(` Blocked: ${branch} — ${result.skippedReason}`);
if (result.skippedReason === 'has uncommitted changes') {
console.error(' Use --force to override.');
}
failed++;
} else if (!result.worktreeRemoved) {
const parts: string[] = [];
if (result.branchDeleted) parts.push('branch deleted');
parts.push('DB updated');
console.error(
` Partial: ${branch} — worktree was not removed from disk (${parts.join(', ')})`
);
for (const warning of result.warnings) {
console.error(` ⚠ ${warning}`);
}
failed++;
} else {
console.log(` Completed: ${branch}`);
+ // Surface warnings from partial cleanup (e.g., branch couldn't be deleted)
+ for (const warning of result.warnings) {
+ console.warn(` Warning: ${warning}`);
+ }
completed++;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/isolation.ts` around lines 309 - 329, The duplicate
warning output comes from printing result.warnings both unconditionally at the
top and again inside the partial cleanup branch; remove the initial
unconditional loop over result.warnings and instead print warnings only inside
the relevant branches (when result.skippedReason is set and when
!result.worktreeRemoved). Concretely, delete the top-level for (const warning of
result.warnings) console.warn block and add a single warning loop inside the
skippedReason branch (alongside the "Blocked" message) and reuse the existing
loop inside the !result.worktreeRemoved branch so each warning is emitted once
and in the appropriate format; keep references to result.warnings,
result.skippedReason, result.worktreeRemoved, branchDeleted and branch to
preserve current messages and formatting.
Contributor
Author
|
Added the missing regression test for the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/commands/isolation.test.ts (1)
413-429: Add the DB-only partial variant to fully lock conditional messaging.Line 425 validates the
branchDeleted: truemessage path. Please add a sibling case forworktreeRemoved: false+branchDeleted: falseto guard the alternate Partial text branch as well.Suggested test addition
+ it('counts as failed when removeEnvironment returns partial (worktree not removed, branch retained)', async () => { + mockFindActiveByBranchName.mockResolvedValueOnce(mockEnv); + mockRemoveEnvironment.mockResolvedValueOnce({ + worktreeRemoved: false, + branchDeleted: false, + warnings: ['Still registered in git worktree list'], + skippedReason: undefined, + }); + + await isolationCompleteCommand(['partial-db-only'], { force: true, deleteRemote: true }); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + ' Partial: partial-db-only — worktree was not removed from disk (DB marked destroyed only)' + ); + expect(consoleErrorSpy).toHaveBeenCalledWith(' ⚠ Still registered in git worktree list'); + expect(consoleLogSpy).toHaveBeenCalledWith('\nComplete: 0 completed, 1 failed, 0 not found'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/isolation.test.ts` around lines 413 - 429, Add a sibling test case that covers the "DB-only partial" variant by mocking mockFindActiveByBranchName to return mockEnv and mockRemoveEnvironment to resolve with worktreeRemoved: false, branchDeleted: false, warnings: ['Some warning'], skippedReason: undefined; then call isolationCompleteCommand(['partial-branch'], { force: true, deleteRemote: true }) and assert consoleErrorSpy was called with the alternate Partial message path (the message that indicates the worktree was not removed and only the DB was updated) and that the warning and summary log lines are emitted (use the same expectations as the existing test but adapted to the "(DB updated)" text), referencing the existing symbols mockFindActiveByBranchName, mockRemoveEnvironment, isolationCompleteCommand, consoleErrorSpy, and consoleLogSpy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli/src/commands/isolation.test.ts`:
- Around line 413-429: Add a sibling test case that covers the "DB-only partial"
variant by mocking mockFindActiveByBranchName to return mockEnv and
mockRemoveEnvironment to resolve with worktreeRemoved: false, branchDeleted:
false, warnings: ['Some warning'], skippedReason: undefined; then call
isolationCompleteCommand(['partial-branch'], { force: true, deleteRemote: true
}) and assert consoleErrorSpy was called with the alternate Partial message path
(the message that indicates the worktree was not removed and only the DB was
updated) and that the warning and summary log lines are emitted (use the same
expectations as the existing test but adapted to the "(DB updated)" text),
referencing the existing symbols mockFindActiveByBranchName,
mockRemoveEnvironment, isolationCompleteCommand, consoleErrorSpy, and
consoleLogSpy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e353292-d9b5-49ab-89fc-700b2716edc5
📒 Files selected for processing (1)
packages/cli/src/commands/isolation.test.ts
…on disk (fixes coleam00#964) Three changes to prevent ghost worktrees: 1. isolationCompleteCommand now checks result.worktreeRemoved — if the worktree was not actually removed (partial failure), it reports 'Partial' with warnings and counts as failed, not completed. Previously only skippedReason was checked; a destroy that returned successfully but with worktreeRemoved=false would still print 'Completed'. 2. WorktreeProvider.destroy() now runs 'git worktree prune' after removal to clean up stale worktree references that git may keep even after the directory is removed. 3. WorktreeProvider.destroy() adds post-removal verification: after git worktree remove, it checks 'git worktree list --porcelain' to confirm the worktree is actually unregistered. If still registered, worktreeRemoved is set back to false with a descriptive warning.
…p callers, accurate messages
Exercises the !result.worktreeRemoved path (without skippedReason) that was flagged as uncovered by CodeRabbit review.
kagura-agent
force-pushed
the
fix/worktree-ghost-cleanup
branch
from
7726105 to
548870f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/core/src/services/cleanup-service.ts (1)
300-305:⚠️ Potential issue | 🟠 MajorPartial removals are still counted as successful cleanup.
These call sites only check
skippedReason. A result like{ worktreeRemoved: false, warnings: [...] }still lands inremoved, so scheduled cleanup and the merged/stale batch helpers can keep reporting ghost worktrees as successfully cleaned. Only count a removal whenworktreeRemoved === true; route partials toskipped/errorswith the warning text instead.Suggested handling pattern
- if (removeResult.skippedReason) { - report.skipped.push({ id: env.id, reason: removeResult.skippedReason }); - } else { - report.removed.push(`${env.id} (path missing)`); - } + if (removeResult.skippedReason) { + report.skipped.push({ id: env.id, reason: removeResult.skippedReason }); + } else if (!removeResult.worktreeRemoved) { + report.skipped.push({ + id: env.id, + reason: removeResult.warnings[0] ?? 'worktree cleanup incomplete', + }); + } else { + report.removed.push(`${env.id} (path missing)`); + }Apply the same branch to the merged/stale callers below.
Also applies to: 334-342, 368-373, 534-539, 639-644
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/services/cleanup-service.ts` around lines 300 - 305, The cleanup currently treats any non-skipped removeEnvironment result as successful; change the handling to only count a removal as successful when removeEnvironment(...) returns worktreeRemoved === true and otherwise push the entry into report.skipped or report.errors including the warning text(s). Update the handling at the shown call site (and the similar blocks around the other listed ranges) to check result.worktreeRemoved first, and if false build a descriptive reason from result.warnings (or result.skippedReason if present) and add it to report.skipped/errors instead of report.removed; keep using the same id formatting (`env.id`) for entries.packages/cli/src/commands/isolation.ts (1)
309-329:⚠️ Potential issue | 🟡 MinorEmit cleanup warnings only once.
result.warningsis printed at Lines 309-312 and then again at Lines 327-329, so every partial cleanup shows duplicated warnings. Keep the top-level loop only for the completed path, or drop the inner loop.Suggested fix
- // Surface warnings from partial cleanup - for (const warning of result.warnings) { - console.warn(` Warning: ${warning}`); - } - if (result.skippedReason) { console.error(` Blocked: ${branch} — ${result.skippedReason}`); if (result.skippedReason === 'has uncommitted changes') { console.error(' Use --force to override.'); } failed++; } else if (!result.worktreeRemoved) { @@ for (const warning of result.warnings) { console.error(` ⚠ ${warning}`); } failed++; } else { console.log(` Completed: ${branch}`); + for (const warning of result.warnings) { + console.warn(` Warning: ${warning}`); + } completed++; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/isolation.ts` around lines 309 - 329, The code prints result.warnings twice (once at the top-level loop and again inside the partial-cleanup branch), causing duplicated warnings; remove the inner warnings loop (the for (const warning of result.warnings) that logs `⚠ ${warning}` in the block guarded by !result.worktreeRemoved) and rely on the existing top-level loop that iterates result.warnings, or alternatively move the single warnings-printing loop into the branches where you actually want warnings emitted; update any tests or call sites that expect the old duplicate output and ensure variables like result.warnings, result.worktreeRemoved, branch and failed remain unchanged.packages/isolation/src/providers/worktree.ts (1)
193-200:⚠️ Potential issue | 🟠 MajorDon't collapse verification failures into successful cleanup.
If
git worktree list --porcelainfails here,isWorktreeRegistered()returnsfalse, sodestroy()keepsworktreeRemoved = trueand the caller reports success even though registration was never verified. That reopens the false-positive path for repo/permission/list failures. Return an "unknown" state (or throw) and downgrade the result to partial with a warning instead of silently succeeding.Suggested fix
- private async isWorktreeRegistered(repoPath: string, worktreePath: string): Promise<boolean> { + private async isWorktreeRegistered( + repoPath: string, + worktreePath: string + ): Promise<boolean | null> { try { const { stdout } = await execFileAsync( 'git', ['-C', repoPath, 'worktree', 'list', '--porcelain'], { timeout: 15000 } @@ return stdout.split('\n').some(line => { if (!line.startsWith('worktree ')) return false; const listed = line.slice('worktree '.length).trim(); return resolve(listed) === normalizedTarget; }); - } catch (_error) { - // If we can't verify, assume it's gone (don't block on verification failure) - return false; + } catch (error) { + getLog().warn({ err: error, repoPath, worktreePath }, 'worktree_registration_check_failed'); + return null; } }- if (result.worktreeRemoved) { - const stillRegistered = await this.isWorktreeRegistered(repoPath, worktreePath); - if (stillRegistered) { + if (result.worktreeRemoved) { + const stillRegistered = await this.isWorktreeRegistered(repoPath, worktreePath); + if (stillRegistered !== false) { result.worktreeRemoved = false; - const warning = `Worktree at ${worktreePath} was reported removed but is still registered in git`; + const warning = + stillRegistered + ? `Worktree at ${worktreePath} was reported removed but is still registered in git` + : `Worktree at ${worktreePath} was removed, but git registration could not be verified`; getLog().warn({ worktreePath, repoPath }, 'worktree_removal_verification_failed'); result.warnings.push(warning); } }Based on learnings: "Let git handle what git does best ... surface git errors to users for actionable issues; handle expected failures gracefully."
Also applies to: 238-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/isolation/src/providers/worktree.ts` around lines 193 - 200, The current destroy() path treats any falsey return from isWorktreeRegistered(repoPath, worktreePath) as "not registered" and keeps result.worktreeRemoved = true, which hides git/list/permission errors; wrap the isWorktreeRegistered call in a try/catch and on error mark the cleanup as partial/unknown (e.g., set a result.partial or result.worktreeRemoved = 'unknown'), push a warning that includes the caught error and context (repoPath, worktreePath) via getLog().warn, and do not report success; only set result.worktreeRemoved = false when isWorktreeRegistered returns true. Apply the same pattern to the analogous block around lines 238-255.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/cli/src/commands/isolation.ts`:
- Around line 309-329: The code prints result.warnings twice (once at the
top-level loop and again inside the partial-cleanup branch), causing duplicated
warnings; remove the inner warnings loop (the for (const warning of
result.warnings) that logs `⚠ ${warning}` in the block guarded by
!result.worktreeRemoved) and rely on the existing top-level loop that iterates
result.warnings, or alternatively move the single warnings-printing loop into
the branches where you actually want warnings emitted; update any tests or call
sites that expect the old duplicate output and ensure variables like
result.warnings, result.worktreeRemoved, branch and failed remain unchanged.
In `@packages/core/src/services/cleanup-service.ts`:
- Around line 300-305: The cleanup currently treats any non-skipped
removeEnvironment result as successful; change the handling to only count a
removal as successful when removeEnvironment(...) returns worktreeRemoved ===
true and otherwise push the entry into report.skipped or report.errors including
the warning text(s). Update the handling at the shown call site (and the similar
blocks around the other listed ranges) to check result.worktreeRemoved first,
and if false build a descriptive reason from result.warnings (or
result.skippedReason if present) and add it to report.skipped/errors instead of
report.removed; keep using the same id formatting (`env.id`) for entries.
In `@packages/isolation/src/providers/worktree.ts`:
- Around line 193-200: The current destroy() path treats any falsey return from
isWorktreeRegistered(repoPath, worktreePath) as "not registered" and keeps
result.worktreeRemoved = true, which hides git/list/permission errors; wrap the
isWorktreeRegistered call in a try/catch and on error mark the cleanup as
partial/unknown (e.g., set a result.partial or result.worktreeRemoved =
'unknown'), push a warning that includes the caught error and context (repoPath,
worktreePath) via getLog().warn, and do not report success; only set
result.worktreeRemoved = false when isWorktreeRegistered returns true. Apply the
same pattern to the analogous block around lines 238-255.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5a8f557-e283-4e9f-b72e-dcd5af1dc637
📒 Files selected for processing (5)
packages/cli/src/commands/isolation.test.tspackages/cli/src/commands/isolation.tspackages/core/src/services/cleanup-service.test.tspackages/core/src/services/cleanup-service.tspackages/isolation/src/providers/worktree.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/commands/isolation.test.ts
Wirasm
added a commit
that referenced
this pull request
Apr 15, 2026
…g' test (#1230) After PR #1034 changed worktree existence checks from execFileAsync to fs/promises.access, the mockExecFileAsync rejections had no effect. removeEnvironment needs getById + getCodebase mocks to proceed past the early-return guard, otherwise envs route to report.skipped instead of report.removed. Replace the two stale mockExecFileAsync rejection calls with proper mockGetById and mockGetCodebase return values for both test environments. Fixes #1230
Wirasm
added a commit
that referenced
this pull request
Apr 15, 2026
…g' test (#1230) (#1232) After PR #1034 changed worktree existence checks from execFileAsync to fs/promises.access, the mockExecFileAsync rejections had no effect. removeEnvironment needs getById + getCodebase mocks to proceed past the early-return guard, otherwise envs route to report.skipped instead of report.removed. Replace the two stale mockExecFileAsync rejection calls with proper mockGetById and mockGetCodebase return values for both test environments. Fixes #1230
fcamblor
added a commit
to fcamblor/Archon
that referenced
this pull request
Apr 16, 2026
Problem: Web UI was blind to workflows in ~/.archon/.archon/workflows/
because GET /api/workflows called discoverWorkflowsWithConfig without
a third argument, so the global search path was never consulted.
Fix: pass { globalSearchPath: getArchonHome() } as third argument,
aligning with CLI (cli/workflow.ts:123) and orchestrator
(orchestrator-agent.ts:386).
Also fix missing `resolve` import in isolation/worktree.ts that caused
type-check and lint errors (introduced in coleam00#1034).
Ref: completes coleam00#1138 (PR coleam00#1136 only covered command-handler/webhook).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tyone88
pushed a commit
to Tyone88/Archon
that referenced
this pull request
Apr 16, 2026
…eam00#1036) * fix(env): detect and refuse target-repo .env with sensitive keys (coleam00#1034) Bun auto-loads .env from subprocess CWD regardless of the clean env passed to Bun.spawn, silently overriding OAuth auth and billing the wrong API account. This adds a consent-based gate at registration time and a pre-spawn safety net in both Claude and Codex clients. Changes: - Add env-leak-scanner utility that checks 6 auto-loaded .env filenames for 7 sensitive keys (ANTHROPIC_API_KEY, OPENAI_API_KEY, etc.) - Add allow_env_keys boolean column to codebases table (migration 021) - Gate registerRepoAtPath to reject codebases with sensitive .env keys unless explicitly consented via allowEnvKeys flag - Add pre-spawn check in ClaudeClient and CodexClient sendQuery methods - Return 422 from POST /api/codebases on env leak detection - Surface env leak error in web UI with "Allow env keys" checkbox - Classify EnvLeakError as FATAL in workflow executor Fixes coleam00#1034 * fix: address review findings for env leak scanner PR - Fix FATAL_PATTERNS 'env leak' pattern that never matched EnvLeakError.message; now checks error.name === 'EnvLeakError' directly (immune to message rewording) - Fix pre-spawn consent lookup for worktree paths: add findCodebaseByPathPrefix() and use it as fallback when exact match returns null; prevents opt-in from being silently ineffective for workflow-based runs - Add allow_env_keys column to 000_combined.sql CREATE TABLE and idempotent ALTER section to fix fresh PostgreSQL installs - Remove non-existent --allow-env-keys CLI flag from error message; replace with web UI-only instruction - Narrow isEnvLeakError check from error.message.includes('env') to startsWith('Cannot add codebase') - Distinguish ENOENT (skip) from EACCES/other errors in scanner catch block; unreadable files now surface as findings to avoid silently bypassing the gate - Use cross-platform grep command instead of macOS-specific sed -i '' syntax - Add audit log (log.warn) when 422 EnvLeakError is returned from API - Add pre-spawn gate tests to claude.test.ts and codex.test.ts (4 tests each) - Add env leak gate tests to clone.test.ts (2 tests) - Add 422 and allowEnvKeys passthrough tests to api.codebases.test.ts * simplify: reduce complexity in changed files
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #964 —
archon completereports "Completed" even when the worktree directory and git registration persist on disk, silently leaking ~789 MB per workflow.Root Cause
isolationCompleteCommandcheckedresult.skippedReasonbut notresult.worktreeRemoved. A destroy that returned successfully but withworktreeRemoved: false(partial failure) would still print "Completed" and increment the success counter.Changes
1. Honest success reporting (
packages/cli/src/commands/isolation.ts)skippedReasonis set (e.g., uncommitted changes without--force), now prints "Blocked" with actionable hint instead of generic "Failed"worktreeRemovedis false but noskippedReason, reports "Partial" with specific warnings and counts as faileddestroyResult.warningsare surfaced as warning lines2. Post-removal verification (
packages/isolation/src/providers/worktree.ts)After
git worktree remove, the provider now:git worktree pruneto clean up stale referencesgit worktree list --porcelainto verify the worktree is actually unregisteredworktreeRemoved = falsewith a descriptive warning3. Test coverage (
packages/cli/src/commands/isolation.test.ts)--forcehintworktreeRemoved: falsewithoutskippedReason)Testing
Summary by CodeRabbit
Bug Fixes
New Features
Tests