Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/cli/args.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down
26 changes: 20 additions & 6 deletions src/cli/commands/agents.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}).`,
);
Expand All @@ -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: [
Expand All @@ -288,7 +296,7 @@ function agents(rawArgs) {
logPath: reviewLogPath,
});
startedAny = true;
} else {
} else if (reviewRunning) {
reusedAny = true;
}

Expand Down Expand Up @@ -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,
Expand All @@ -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;
}
Expand Down
5 changes: 3 additions & 2 deletions src/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Expand All @@ -693,8 +693,9 @@ const CLI_COMMAND_HELP = {
['--session <id>', 'Address one agent session'],
['--branch <agent/...>', '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': {
Expand Down
3 changes: 2 additions & 1 deletion src/finish/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <n> Only consider worktrees idle for at least n minutes
--watch Repeat cleanup cycles (defaults idle threshold to 60)
--interval <seconds> Watch interval in seconds (minimum: 5; default: 60)
Expand Down
38 changes: 33 additions & 5 deletions templates/scripts/agent-worktree-prune.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down
154 changes: 154 additions & 0 deletions test/agents.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
13 changes: 13 additions & 0 deletions test/cli-args-dispatch.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion test/finish.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Loading