diff --git a/src/cli/args.js b/src/cli/args.js index fbbc8709..8e0a337a 100644 --- a/src/cli/args.js +++ b/src/cli/args.js @@ -390,6 +390,7 @@ function parseAgentsArgs(rawArgs) { dryRun: false, reviewIntervalSeconds: 30, cleanupIntervalSeconds: 60, + cleanupOnly: false, idleMinutes: DEFAULT_SHADOW_CLEANUP_IDLE_MINUTES, staleAgeMinutes: 24 * 60, pid: null, @@ -551,6 +552,10 @@ function parseAgentsArgs(rawArgs) { index += 1; continue; } + if (arg === '--cleanup-only') { + options.cleanupOnly = true; + continue; + } if (arg === '--cleanup-interval') { const next = rest[index + 1]; if (!next) { @@ -1029,7 +1034,11 @@ function parseCleanupArgs(rawArgs) { keepRemote: false, keepCleanWorktrees: true, includeCleanLinkedWorktrees: false, - includePrMerged: false, + // Squash/rebase merges leave the branch tip off the base's history, so + // ancestry alone reports a merged PR's branch as unmerged and cleanup + // never reclaims it. Consult merged PRs by default; `gh` missing or failing + // disables the lookup (fail-closed: nothing is treated as merged). + includePrMerged: true, idleMinutes: 0, watch: false, intervalSeconds: 60, @@ -1094,6 +1103,10 @@ function parseCleanupArgs(rawArgs) { options.includePrMerged = true; continue; } + if (arg === '--no-include-pr-merged') { + options.includePrMerged = false; + continue; + } if (arg === '--idle-minutes') { const next = rawArgs[index + 1]; if (!next) { diff --git a/src/cli/commands/agents.js b/src/cli/commands/agents.js index 9934866a..08733b73 100644 --- a/src/cli/commands/agents.js +++ b/src/cli/commands/agents.js @@ -254,7 +254,15 @@ function agents(rawArgs) { const reviewRunning = processAlive(existingReviewPid); const cleanupRunning = processAlive(existingCleanupPid); - if (reviewRunning && cleanupRunning) { + // --cleanup-only: run just the cleanup bot. The review bot spends model + // tokens on every PR; reclaiming merged lanes should not require it. + const reviewWanted = !options.cleanupOnly; + if ((reviewRunning || !reviewWanted) && cleanupRunning) { + if (!reviewWanted) { + console.log(`[${TOOL_NAME}] Cleanup bot already running (pid=${existingCleanupPid}).`); + process.exitCode = 0; + return; + } console.log( `[${TOOL_NAME}] Repo agents already running (review pid=${existingReviewPid}, cleanup pid=${existingCleanupPid}).`, ); @@ -271,7 +279,7 @@ function agents(rawArgs) { let reusedAny = false; const mainScriptPath = require.resolve('../main.js'); - if (!reviewRunning) { + if (reviewWanted && !reviewRunning) { reviewPid = spawnDetachedAgentProcess({ command: process.execPath, args: [ @@ -288,7 +296,7 @@ function agents(rawArgs) { logPath: reviewLogPath, }); startedAny = true; - } else { + } else if (reviewRunning) { reusedAny = true; } @@ -333,7 +341,7 @@ function agents(rawArgs) { repoRoot, startedAt: new Date().toISOString(), review: { - pid: reviewPid, + pid: reviewWanted || reviewRunning ? reviewPid : null, intervalSeconds: reviewIntervalSeconds, script: mainScriptPath, logPath: reviewLogPath, @@ -348,12 +356,18 @@ function agents(rawArgs) { }); console.log( - `[${TOOL_NAME}] Started repo agents in ${repoRoot} (review pid=${reviewPid}, cleanup pid=${cleanupPid}).`, + reviewWanted + ? `[${TOOL_NAME}] Started repo agents in ${repoRoot} (review pid=${reviewPid}, cleanup pid=${cleanupPid}).` + : reviewRunning + ? `[${TOOL_NAME}] Started the cleanup bot in ${repoRoot} (cleanup pid=${cleanupPid}); the review bot already running (pid=${existingReviewPid}) is left as is.` + : `[${TOOL_NAME}] Started the cleanup bot in ${repoRoot} (cleanup pid=${cleanupPid}; review bot not started: --cleanup-only).`, ); if (reusedAny && startedAny) { console.log(`[${TOOL_NAME}] Reused healthy bot process(es) and started only missing ones.`); } - console.log(`[${TOOL_NAME}] Logs: ${reviewLogPath}, ${cleanupLogPath}`); + console.log( + `[${TOOL_NAME}] Logs: ${reviewWanted || reviewRunning ? `${reviewLogPath}, ` : ''}${cleanupLogPath}`, + ); process.exitCode = 0; return; } diff --git a/src/context.js b/src/context.js index 2b2ac8be..d88d68f8 100644 --- a/src/context.js +++ b/src/context.js @@ -674,7 +674,7 @@ const CLI_COMMAND_HELP = { summary: 'Repo-scoped review and cleanup bots, plus the agent inbox and lane inspection.', subcommands: [ ['status', 'Show the running bots and the agent lanes (the default with no subcommand)'], - ['start', 'Start the review + cleanup bots for this repo'], + ['start', 'Start the review + cleanup bots for this repo (--cleanup-only: just the cleanup bot)'], ['stop', 'Stop them'], ['finish', 'Finish an agent lane by session or branch'], ['files', 'Show which files each lane has touched'], @@ -693,8 +693,9 @@ const CLI_COMMAND_HELP = { ['--session ', 'Address one agent session'], ['--branch ', 'Address one agent branch'], ['--dry-run', 'Print actions without applying them'], + ['--cleanup-only', 'start: run only the cleanup bot (no token-spending review bot)'], ], - examples: ['gx agents status --json', 'gx agents files --branch agent/claude/my-lane'], + examples: ['gx agents status --json', 'gx agents start --cleanup-only', 'gx agents files --branch agent/claude/my-lane'], }, 'pr-review': { diff --git a/src/finish/index.js b/src/finish/index.js index 552145f4..11c89151 100644 --- a/src/finish/index.js +++ b/src/finish/index.js @@ -235,7 +235,8 @@ OPTIONS --keep-clean-worktrees Preserve clean, unmerged agent worktrees (default) --include-clean-linked-worktrees Also prune clean linked worktrees outside managed agent directories - --include-pr-merged Treat branches from merged PRs as merged + --include-pr-merged Treat branches from merged PRs as merged (default; needs gh) + --no-include-pr-merged Judge merged by git ancestry only (misses squash merges) --idle-minutes Only consider worktrees idle for at least n minutes --watch Repeat cleanup cycles (defaults idle threshold to 60) --interval Watch interval in seconds (minimum: 5; default: 60) diff --git a/templates/scripts/agent-worktree-prune.sh b/templates/scripts/agent-worktree-prune.sh index 91cafed2..7b6bf33f 100644 --- a/templates/scripts/agent-worktree-prune.sh +++ b/templates/scripts/agent-worktree-prune.sh @@ -228,14 +228,18 @@ load_merged_pr_branches() { return 1 fi + # Name -> space-separated merged PR head SHAs. The SHA is what makes the + # name match safe: a reused branch name, or commits pushed after the PR + # merged, must not read as "merged" (see branch_has_merged_pr). local merged_branches="" merged_branches="$( - "$GH_BIN" pr list --state merged --base "$BASE_BRANCH" --limit 200 --json headRefName --jq '.[].headRefName' 2>/dev/null || true + "$GH_BIN" pr list --state merged --base "$BASE_BRANCH" --limit 200 --json headRefName,headRefOid --jq '.[] | "\(.headRefName) \(.headRefOid)"' 2>/dev/null || true )" if [[ -n "$merged_branches" ]]; then - while IFS= read -r merged_branch; do - [[ -z "$merged_branch" ]] && continue - MERGED_PR_BRANCHES["$merged_branch"]=1 + local merged_branch merged_oid + while IFS=' ' read -r merged_branch merged_oid; do + [[ -z "$merged_branch" || -z "$merged_oid" ]] && continue + MERGED_PR_BRANCHES["$merged_branch"]="${MERGED_PR_BRANCHES[$merged_branch]:-} ${merged_oid}" done <<< "$merged_branches" fi PR_MERGED_LOOKUP_LOADED=1 @@ -288,7 +292,31 @@ branch_has_merged_pr() { return 1 fi load_merged_pr_branches || return 1 - [[ -n "${MERGED_PR_BRANCHES[$branch]:-}" ]] + local merged_oids="${MERGED_PR_BRANCHES[$branch]:-}" + [[ -n "$merged_oids" ]] || return 1 + # Merged only if every tip we can see — the local branch AND origin's — + # IS a merged PR's head or is contained in one. Anything newer (commits + # pushed after the merge, locally or to origin; a reused name) is unmerged + # work — fail closed. A PR head we do not have locally cannot prove + # containment, so only the exact-SHA match applies then. + local tips=() ref resolved + for ref in "refs/heads/${branch}" "refs/remotes/origin/${branch}"; do + resolved="$(git -C "$repo_root" rev-parse --verify --quiet "${ref}^{commit}" 2>/dev/null || true)" + [[ -n "$resolved" ]] && tips+=("$resolved") + done + [[ ${#tips[@]} -gt 0 ]] || return 1 + local tip oid contained + for tip in "${tips[@]}"; do + contained=0 + for oid in $merged_oids; do + if [[ "$tip" == "$oid" ]] || git -C "$repo_root" merge-base --is-ancestor "$tip" "$oid" >/dev/null 2>&1; then + contained=1 + break + fi + done + [[ "$contained" -eq 1 ]] || return 1 + done + return 0 } load_lane_pr_metadata() { diff --git a/test/agents.test.js b/test/agents.test.js index cb24f970..6cd797eb 100644 --- a/test/agents.test.js +++ b/test/agents.test.js @@ -275,6 +275,160 @@ test('agents command starts review+cleanup bots for the target repo and stops th }); +test('agents start --cleanup-only starts only the cleanup bot', () => { + const repoDir = initRepo(); + seedCommit(repoDir); + const scriptsDir = path.join(repoDir, 'scripts'); + fs.mkdirSync(scriptsDir, { recursive: true }); + const reviewScriptPath = path.join(scriptsDir, 'review-bot-watch.sh'); + fs.writeFileSync(reviewScriptPath, fakeReviewBotDaemonScript(), 'utf8'); + fs.chmodSync(reviewScriptPath, 0o755); + const pruneScriptPath = path.join(scriptsDir, 'agent-worktree-prune.sh'); + fs.writeFileSync( + pruneScriptPath, + '#!/usr/bin/env bash\n' + + 'set -euo pipefail\n' + + 'exit 0\n', + 'utf8', + ); + fs.chmodSync(pruneScriptPath, 0o755); + + let result = runNode( + ['agents', 'start', '--target', repoDir, '--cleanup-only', '--cleanup-interval', '47', '--idle-minutes', '12'], + repoDir, + ); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.match(result.stdout, /Started the cleanup bot/); + assert.match(result.stdout, /review bot not started: --cleanup-only/); + + const statePath = path.join(repoDir, '.omx', 'state', 'agents-bots.json'); + const state = JSON.parse(fs.readFileSync(statePath, 'utf8')); + assert.equal(state.review.pid, null, 'no review bot under --cleanup-only'); + assert.equal(isPidAlive(state.cleanup.pid), true, 'cleanup bot pid should be alive after start'); + assert.equal( + fs.existsSync(path.join(repoDir, '.omx', 'logs', 'agent-review.log')), + false, + 'no review log under --cleanup-only', + ); + + // A second start is a no-op while the cleanup bot runs. + result = runNode(['agents', 'start', '--target', repoDir, '--cleanup-only'], repoDir); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.match(result.stdout, /Cleanup bot already running/); + + result = runNode(['agents', 'stop', '--target', repoDir], repoDir); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.equal(waitForPidExit(state.cleanup.pid), true, 'cleanup bot pid should exit after stop'); +}); + +test('gx cleanup consults merged PRs by default and skips them under --no-include-pr-merged', () => { + const repoDir = initRepo(); + seedCommit(repoDir); + const execGit = (dir, args) => { + const out = runCmd('git', args, dir); + assert.equal(out.status, 0, out.stderr || out.stdout); + return String(out.stdout || '').trim(); + }; + const base = execGit(repoDir, ['rev-parse', '--abbrev-ref', 'HEAD']); + // An agent lane whose tip is NOT an ancestor of the base — what a squash + // merge leaves behind — so only the PR lookup can call it merged. + execGit(repoDir, ['checkout', '-q', '-b', 'agent/claude/squashed-lane']); + fs.writeFileSync(path.join(repoDir, 'lane.txt'), 'lane\n', 'utf8'); + execGit(repoDir, ['add', 'lane.txt']); + execGit(repoDir, ['commit', '-q', '-m', 'lane work']); + execGit(repoDir, ['checkout', '-q', base]); + + const ghLog = path.join(repoDir, '.gh-calls.log'); + const fakeGh = path.join(repoDir, 'fake-gh.sh'); + fs.writeFileSync(fakeGh, `#!/usr/bin/env bash\necho "$*" >> ${JSON.stringify(ghLog)}\nexit 0\n`, 'utf8'); + fs.chmodSync(fakeGh, 0o755); + + let result = runNodeWithEnv( + ['cleanup', '--target', repoDir, '--base', base, '--dry-run'], + repoDir, + { GUARDEX_GH_BIN: fakeGh }, + ); + assert.equal(result.status, 0, result.stderr || result.stdout); + const calls = fs.existsSync(ghLog) ? fs.readFileSync(ghLog, 'utf8') : ''; + assert.match(calls, /pr list --state merged/, 'default cleanup must ask for merged PRs'); + + fs.rmSync(ghLog, { force: true }); + result = runNodeWithEnv( + ['cleanup', '--target', repoDir, '--base', base, '--dry-run', '--no-include-pr-merged'], + repoDir, + { GUARDEX_GH_BIN: fakeGh }, + ); + assert.equal(result.status, 0, result.stderr || result.stdout); + const optOut = fs.existsSync(ghLog) ? fs.readFileSync(ghLog, 'utf8') : ''; + assert.doesNotMatch(optOut, /--state merged/, 'opt-out must not query merged PRs'); +}); + +test('gx cleanup treats a lane as PR-merged only when its tip matches the merged PR head', () => { + const repoDir = initRepo(); + seedCommit(repoDir); + const execGit = (dir, args) => { + const out = runCmd('git', args, dir); + assert.equal(out.status, 0, out.stderr || out.stdout); + return String(out.stdout || '').trim(); + }; + const base = execGit(repoDir, ['rev-parse', '--abbrev-ref', 'HEAD']); + const lane = 'agent/claude/squashed-lane'; + execGit(repoDir, ['checkout', '-q', '-b', lane]); + fs.writeFileSync(path.join(repoDir, 'lane.txt'), 'lane\n', 'utf8'); + execGit(repoDir, ['add', 'lane.txt']); + execGit(repoDir, ['commit', '-q', '-m', 'lane work']); + const prHead = execGit(repoDir, ['rev-parse', 'HEAD']); + execGit(repoDir, ['checkout', '-q', base]); + + const fakeGhFor = (oid) => { + const fakeGh = path.join(repoDir, 'fake-gh.sh'); + fs.writeFileSync( + fakeGh, + '#!/usr/bin/env bash\n' + + `if [[ "$*" == *"--state merged"* ]]; then echo "${lane} ${oid}"; fi\n` + + 'exit 0\n', + 'utf8', + ); + fs.chmodSync(fakeGh, 0o755); + return fakeGh; + }; + const dryRun = (fakeGh) => + runNodeWithEnv(['cleanup', '--target', repoDir, '--base', base, '--dry-run'], repoDir, { + GUARDEX_GH_BIN: fakeGh, + }); + + // Tip == merged PR head: the squash-merged lane is reclaimed. + let result = dryRun(fakeGhFor(prHead)); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.match(`${result.stdout}${result.stderr}`, new RegExp(`merged PR branch: ${lane}`)); + + // Work pushed after the PR merged: the tip is past the PR head — keep it. + execGit(repoDir, ['checkout', '-q', lane]); + fs.writeFileSync(path.join(repoDir, 'later.txt'), 'later\n', 'utf8'); + execGit(repoDir, ['add', 'later.txt']); + execGit(repoDir, ['commit', '-q', '-m', 'work after merge']); + execGit(repoDir, ['checkout', '-q', base]); + result = dryRun(fakeGhFor(prHead)); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.doesNotMatch(`${result.stdout}${result.stderr}`, new RegExp(`merged PR branch: ${lane}`)); + + // Local branch at the PR head, but origin moved past it after the merge: + // the remote carries unmerged work — keep it. + execGit(repoDir, ['branch', '-f', lane, prHead]); + const laterOnOrigin = execGit(repoDir, ['rev-parse', `${lane}@{1}`]); + execGit(repoDir, ['update-ref', `refs/remotes/origin/${lane}`, laterOnOrigin]); + result = dryRun(fakeGhFor(prHead)); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.doesNotMatch(`${result.stdout}${result.stderr}`, new RegExp(`merged PR branch: ${lane}`)); + execGit(repoDir, ['update-ref', '-d', `refs/remotes/origin/${lane}`]); + execGit(repoDir, ['branch', '-f', lane, laterOnOrigin]); + + // A reused name whose merged PR head is unrelated to this tip — keep it. + result = dryRun(fakeGhFor('0123456789abcdef0123456789abcdef01234567')); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.doesNotMatch(`${result.stdout}${result.stderr}`, new RegExp(`merged PR branch: ${lane}`)); +}); + test('agents start reuses running review bot when only cleanup bot is missing', () => { const repoDir = initRepo(); seedCommit(repoDir); diff --git a/test/cli-args-dispatch.test.js b/test/cli-args-dispatch.test.js index c4d286c0..015ed3d0 100644 --- a/test/cli-args-dispatch.test.js +++ b/test/cli-args-dispatch.test.js @@ -136,6 +136,7 @@ test('parseAgentsArgs applies interval overrides and validates the subcommand', dryRun: false, reviewIntervalSeconds: 15, cleanupIntervalSeconds: 45, + cleanupOnly: false, idleMinutes: 12, staleAgeMinutes: 1440, pid: null, @@ -328,6 +329,18 @@ test('parseCleanupArgs defaults idle minutes when watch mode is enabled', () => assert.equal(options.idleMinutes, DEFAULT_SHADOW_CLEANUP_IDLE_MINUTES); }); +test('parseCleanupArgs consults merged PRs by default so squash-merged lanes are reclaimed', () => { + assert.equal(parseCleanupArgs([]).includePrMerged, true); + assert.equal(parseCleanupArgs(['--include-pr-merged']).includePrMerged, true); + assert.equal(parseCleanupArgs(['--no-include-pr-merged']).includePrMerged, false); + assert.equal(parseCleanupArgs(['--watch']).includePrMerged, true); +}); + +test('parseAgentsArgs accepts --cleanup-only for a cleanup bot without the review bot', () => { + assert.equal(parseAgentsArgs(['start']).cleanupOnly, false); + assert.equal(parseAgentsArgs(['start', '--cleanup-only']).cleanupOnly, true); +}); + test('parseCleanupArgs preserves clean worktrees unless pruning is explicitly requested', () => { assert.equal(parseCleanupArgs([]).keepCleanWorktrees, true); assert.equal(parseCleanupArgs(['--prune-clean-worktrees']).keepCleanWorktrees, false); diff --git a/test/finish.test.js b/test/finish.test.js index 807901d7..7bfdee1f 100644 --- a/test/finish.test.js +++ b/test/finish.test.js @@ -1834,9 +1834,11 @@ test('cleanup command can remove squash-merged agent branches via merged PR dete result = runCmd('git', ['-C', worktreePath, 'commit', '-m', 'feature commit'], repoDir); assert.equal(result.status, 0, result.stderr || result.stdout); + // A squash-merged PR's head is the lane tip; gh reports name + head SHA. + const laneTip = runCmd('git', ['-C', worktreePath, 'rev-parse', 'HEAD'], repoDir).stdout.trim(); const { fakePath: fakeGhPath } = createFakeGhScript( 'if [[ "$1" == "pr" && "$2" == "list" ]]; then\n' + - ' printf \'%s\\n\' "agent/test-cleanup-pr-merged"\n' + + ` printf '%s %s\\n' "agent/test-cleanup-pr-merged" "${laneTip}"\n` + ' exit 0\n' + 'fi\n' + 'exit 1',