From d8b24b986a528befb7b53c75ac4fa09ba262abe5 Mon Sep 17 00:00:00 2001 From: NagyVikt Date: Thu, 23 Apr 2026 17:59:05 +0200 Subject: [PATCH] Prevent gx doctor sandbox branches from lingering after merge Doctor previously ran agent-branch-finish from inside the sandbox worktree, so merge cleanup could not prune the active doctor worktree. The fix now lets branch-finish handle the merge without the broader prune side effects, then doctor explicitly verifies and removes its own local and remote sandbox refs. Added regression coverage for worktree removal after successful protected-main doctor auto-finish. Constraint: doctor must preserve later auto-finish sweep behavior for unrelated ready agent branches Rejected: Keep using --cleanup from the doctor finish call | repo-wide prune consumed unrelated ready worktrees before the sweep Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep doctor sandbox cleanup targeted to metadata.branch and metadata.worktreePath; do not reintroduce repo-wide prune here without proving it cannot consume unrelated ready worktrees Tested: node --test test/doctor.test.js Tested: openspec validate --specs Not-tested: Live GitHub PR merge against the real origin for this branch --- .../notes.md | 14 ++ src/doctor/index.js | 163 +++++++++++++++++- test/doctor.test.js | 19 ++ 3 files changed, 191 insertions(+), 5 deletions(-) create mode 100644 openspec/changes/agent-codex-fix-doctor-sandbox-cleanup-2026-04-23-17-49/notes.md diff --git a/openspec/changes/agent-codex-fix-doctor-sandbox-cleanup-2026-04-23-17-49/notes.md b/openspec/changes/agent-codex-fix-doctor-sandbox-cleanup-2026-04-23-17-49/notes.md new file mode 100644 index 00000000..f5b42fdd --- /dev/null +++ b/openspec/changes/agent-codex-fix-doctor-sandbox-cleanup-2026-04-23-17-49/notes.md @@ -0,0 +1,14 @@ +# agent-codex-fix-doctor-sandbox-cleanup-2026-04-23-17-49 (minimal / T1) + +Branch: `agent/codex/fix-doctor-sandbox-cleanup-2026-04-23-17-49` + +`gx doctor` already merges protected-branch sandbox repairs through `gx branch finish --cleanup`, but the doctor flow runs that finisher from inside the sandbox worktree. That leaves the just-merged doctor sandbox attached as the active cwd, so the finisher cannot prune the worktree even though the merge succeeded. + +Scope: +- Run the doctor sandbox finish flow from the protected repo root instead of the sandbox cwd. +- Verify successful doctor auto-finish leaves no surviving local sandbox branch/worktree. +- Add a focused regression that proves the sandbox worktree path disappears after merge cleanup. + +Verification: +- `node --test test/doctor.test.js --test-name-pattern "doctor on protected main auto-commits sandbox repairs and runs PR finish flow when gh is authenticated"` +- `node --test test/doctor.test.js --test-name-pattern "doctor on protected main fails when sandbox PR is not merged"` diff --git a/src/doctor/index.js b/src/doctor/index.js index 819c48b8..527b78d0 100644 --- a/src/doctor/index.js +++ b/src/doctor/index.js @@ -314,6 +314,118 @@ function doctorFinishFlowIsPending(output) { ); } +function verifyDoctorSandboxCleanup(repoRoot, metadata) { + const branchExists = Boolean(metadata.branch) && gitRefExists(repoRoot, `refs/heads/${metadata.branch}`); + const worktreeExists = Boolean(metadata.worktreePath) && fs.existsSync(metadata.worktreePath); + + if (!branchExists && !worktreeExists) { + return { + status: 'verified', + note: 'doctor sandbox cleanup verified', + }; + } + + const cleanup = cleanupProtectedBaseSandbox(repoRoot, metadata); + const branchStillExists = Boolean(metadata.branch) && gitRefExists(repoRoot, `refs/heads/${metadata.branch}`); + const worktreeStillExists = Boolean(metadata.worktreePath) && fs.existsSync(metadata.worktreePath); + if (branchStillExists || worktreeStillExists) { + return { + status: 'failed', + note: + 'doctor sandbox cleanup incomplete ' + + `(branch=${branchStillExists ? 'present' : 'missing'}, worktree=${worktreeStillExists ? 'present' : 'missing'})`, + cleanup, + }; + } + + return { + status: 'verified', + note: 'doctor sandbox cleanup verified', + cleanup, + }; +} + +function verifyDoctorSandboxRemoteCleanup(repoRoot, metadata) { + if (!metadata.branch || !hasOriginRemote(repoRoot)) { + return { + status: 'skipped', + note: 'doctor sandbox remote cleanup skipped', + }; + } + + const remoteBefore = run( + 'git', + ['-C', repoRoot, 'ls-remote', '--heads', 'origin', metadata.branch], + { timeout: 20_000 }, + ); + if (isSpawnFailure(remoteBefore)) { + throw remoteBefore.error; + } + if (remoteBefore.status !== 0) { + return { + status: 'failed', + note: 'doctor sandbox remote branch inspection failed', + stdout: remoteBefore.stdout || '', + stderr: remoteBefore.stderr || '', + }; + } + if (!String(remoteBefore.stdout || '').trim()) { + return { + status: 'verified', + note: 'doctor sandbox remote cleanup verified', + }; + } + + const deleteResult = run( + 'git', + ['-C', repoRoot, 'push', 'origin', '--delete', metadata.branch], + { timeout: 30_000 }, + ); + if (isSpawnFailure(deleteResult)) { + throw deleteResult.error; + } + if (deleteResult.status !== 0) { + return { + status: 'failed', + note: 'doctor sandbox remote branch cleanup failed', + stdout: deleteResult.stdout || '', + stderr: deleteResult.stderr || '', + }; + } + + const remoteAfter = run( + 'git', + ['-C', repoRoot, 'ls-remote', '--heads', 'origin', metadata.branch], + { timeout: 20_000 }, + ); + if (isSpawnFailure(remoteAfter)) { + throw remoteAfter.error; + } + if (remoteAfter.status !== 0) { + return { + status: 'failed', + note: 'doctor sandbox remote cleanup recheck failed', + stdout: remoteAfter.stdout || '', + stderr: remoteAfter.stderr || '', + }; + } + if (String(remoteAfter.stdout || '').trim()) { + return { + status: 'failed', + note: 'doctor sandbox remote branch still present after cleanup', + stdout: remoteAfter.stdout || '', + stderr: remoteAfter.stderr || '', + }; + } + + return { + status: 'verified', + note: 'doctor sandbox remote cleanup verified', + stdout: deleteResult.stdout || '', + stderr: deleteResult.stderr || '', + }; +} + function finishDoctorSandboxBranch(blocked, metadata, options = {}) { if (!hasOriginRemote(blocked.repoRoot)) { return { @@ -353,8 +465,8 @@ function finishDoctorSandboxBranch(blocked, metadata, options = {}) { const finishResult = runPackageAsset( 'branchFinish', - ['--branch', metadata.branch, '--base', blocked.branch, '--via-pr', waitForMergeArg, '--cleanup'], - { cwd: metadata.worktreePath, timeout: finishTimeoutMs }, + ['--branch', metadata.branch, '--base', blocked.branch, '--via-pr', waitForMergeArg, '--no-cleanup'], + { cwd: blocked.repoRoot, timeout: finishTimeoutMs }, ); if (isSpawnFailure(finishResult)) { return { @@ -384,11 +496,52 @@ function finishDoctorSandboxBranch(blocked, metadata, options = {}) { }; } + let cleanupVerification; + try { + cleanupVerification = verifyDoctorSandboxCleanup(blocked.repoRoot, metadata); + } catch (error) { + return { + status: 'failed', + note: `doctor sandbox cleanup verification failed: ${error.message}`, + stdout: finishResult.stdout || '', + stderr: finishResult.stderr || '', + }; + } + if (cleanupVerification.status === 'failed') { + return { + status: 'failed', + note: cleanupVerification.note, + stdout: finishResult.stdout || '', + stderr: finishResult.stderr || '', + }; + } + + let remoteCleanupVerification; + try { + remoteCleanupVerification = verifyDoctorSandboxRemoteCleanup(blocked.repoRoot, metadata); + } catch (error) { + return { + status: 'failed', + note: `doctor sandbox remote cleanup verification failed: ${error.message}`, + stdout: finishResult.stdout || '', + stderr: finishResult.stderr || '', + }; + } + if (remoteCleanupVerification.status === 'failed') { + return { + status: 'failed', + note: remoteCleanupVerification.note, + stdout: [finishResult.stdout || '', remoteCleanupVerification.stdout || ''].filter(Boolean).join('\n'), + stderr: [finishResult.stderr || '', remoteCleanupVerification.stderr || ''].filter(Boolean).join('\n'), + }; + } + return { status: 'completed', - note: 'doctor sandbox finish flow completed', - stdout: finishResult.stdout || '', - stderr: finishResult.stderr || '', + note: 'doctor sandbox finish flow completed and cleanup verified', + stdout: [finishResult.stdout || '', remoteCleanupVerification.stdout || ''].filter(Boolean).join('\n'), + stderr: [finishResult.stderr || '', remoteCleanupVerification.stderr || ''].filter(Boolean).join('\n'), + cleanup: cleanupVerification.cleanup, }; } diff --git a/test/doctor.test.js b/test/doctor.test.js index 44a071f2..e309df7a 100644 --- a/test/doctor.test.js +++ b/test/doctor.test.js @@ -353,6 +353,7 @@ exit 1 result = runNodeWithEnv(['doctor', '--target', repoDir], repoDir, { GUARDEX_GH_BIN: fakeGhPath }); assert.equal(result.status, 0, result.stderr || result.stdout); + const doctorOutput = `${result.stdout}\n${result.stderr}`; assert.match(result.stdout, /Auto-committed doctor repairs in sandbox branch/); assert.match(result.stdout, /Auto-finish flow completed for sandbox branch/); assert.equal( @@ -364,10 +365,28 @@ exit 1 assertZeroCopyManagedGitignore(repairedRootGitignore); const createdBranch = extractCreatedBranch(result.stdout); + const createdWorktree = extractCreatedWorktree(result.stdout); result = runCmd('git', ['show-ref', '--verify', '--quiet', `refs/heads/${createdBranch}`], repoDir); assert.notEqual(result.status, 0, 'doctor auto-finish should clean up the merged sandbox branch locally by default'); result = runCmd('git', ['ls-remote', '--heads', 'origin', createdBranch], repoDir); assert.equal(result.stdout.trim(), '', 'doctor auto-finish should clean up the merged sandbox branch remotely by default'); + assert.equal( + fs.existsSync(createdWorktree), + false, + 'doctor auto-finish should remove the sandbox worktree after merge cleanup succeeds', + ); + const worktreeList = runCmd('git', ['worktree', 'list', '--porcelain'], repoDir); + assert.equal(worktreeList.status, 0, worktreeList.stderr || worktreeList.stdout); + assert.doesNotMatch( + worktreeList.stdout, + new RegExp(`worktree ${escapeRegexLiteral(createdWorktree)}`), + 'doctor auto-finish should not leave the sandbox worktree registered after cleanup', + ); + assert.doesNotMatch( + doctorOutput, + /Current worktree '.*' still exists because it is the active shell cwd/, + 'doctor should not finish from inside the sandbox cwd anymore', + ); const rootStatus = runCmd('git', ['status', '--short', '--untracked-files=no'], repoDir); assert.equal(rootStatus.status, 0, rootStatus.stderr || rootStatus.stdout);