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);