diff --git a/codev/projects/bugfix-1637-afx-spawn-open-pr-collision-gu/status.yaml b/codev/projects/bugfix-1637-afx-spawn-open-pr-collision-gu/status.yaml new file mode 100644 index 000000000..393027d57 --- /dev/null +++ b/codev/projects/bugfix-1637-afx-spawn-open-pr-collision-gu/status.yaml @@ -0,0 +1,22 @@ +id: bugfix-1637 +title: afx-spawn-open-pr-collision-gu +protocol: bugfix +phase: verified +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-09-10T02:46:18.290Z' + approved_at: '2026-09-11T10:21:55.540Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-09-10T02:24:06.981Z' +updated_at: '2026-09-11T10:22:00.571Z' +pr_history: + - phase: pr + pr_number: 1673 + branch: builder/bugfix-1637 + created_at: '2026-09-10T02:40:32.738Z' +pr_ready_for_human: false diff --git a/codev/state/bugfix-1637_thread.md b/codev/state/bugfix-1637_thread.md new file mode 100644 index 000000000..328c3fab7 --- /dev/null +++ b/codev/state/bugfix-1637_thread.md @@ -0,0 +1,120 @@ +# bugfix-1637 — spawn open-PR collision guard counts merged PRs + +Issue #1637. Strict BUGFIX. Worktree: .builders/bugfix-1637. + +## Investigate (complete) + +**Symptom**: `afx spawn` refuses with "Found N open PR(s) referencing issue #X" when the +referenced PRs are actually *merged*, forcing `--force`. + +**Root cause** — a #1619 (3.3.3) side effect: +- Guard: `checkBugfixCollisions` in `packages/codev/src/agent-farm/commands/spawn-worktree.ts` + lines 590-607. It calls the `pr-search` forge concept and treats **every** result as an + open PR (`openPRs.length` → fatal). +- `pr-search` was changed to `--state all` (github) / `--all` (gitlab) by #1619 so + `consult --type pr` finds merged PRs (#759). Correct for consult; the guard inherited + merged/closed PRs into its "open" count. +- Compounding: `scripts/forge/github/pr-search.sh` outputs only + `number,headRefName,baseRefName` — **no `state`** — so the guard can't filter client-side. + +**Two consumers, opposite needs** (pinned by a paired regression test): +- consult `findPRForIssue` (consult/index.ts:2055) — wants `--state all`; takes first result. +- spawn guard — must ignore non-OPEN. + +**Fix plan** (scope ~small, fits BUGFIX): +1. `scripts/forge/github/pr-search.sh`: add `state` to `--json` fields (gh → OPEN/MERGED/CLOSED). +2. `scripts/forge/gitlab/pr-search.sh`: add `state`, normalize glab's lowercase + `opened`→`OPEN` via jq to match github convention. +3. `PrSearchItem` contract (forge-contracts.ts:149): add optional `state?: string`. +4. Guard: filter to items that are OPEN **or missing state** (conservative — legacy overrides + without `state` keep pre-fix behavior; existing test at spawn-worktree.test.ts:642 passes an + item with no `state` and expects a fatal, so "missing = assume open" is required for + backward-compat). Keep `--state all` in scripts — consult needs it, the guard filters. +5. Regression test pairing both consumers: guard ignores MERGED/CLOSED, consult still sees them. + +Scripts ship via `packages/codev/package.json` `files: [scripts/forge]`; no codev-skeleton copy. + +## Fix (complete) + +Implemented exactly the 5-point plan: +- github/pr-search.sh: `--json …,state` (gh → OPEN/MERGED/CLOSED). +- gitlab/pr-search.sh: jq normalizes glab's lowercase `opened`→`OPEN` (missing → ""). +- forge-contracts.ts: `PrSearchItem.state?: string`, documented as optional/normalized. +- spawn-worktree.ts guard: `filter(pr => pr.state === undefined || pr.state === 'OPEN')`; + missing state kept as "assume open" (conservative — existing test passes a stateless item + and expects a fatal). +- Tests: 4 new guard cases in spawn-worktree.test.ts (merged ignored, open fatals, mixed + counts only open, missing-state still fatals) + 2 script-contract cases in + bugfix-759-pr-search-state-all.test.ts (github emits state; gitlab maps to OPEN). + +**Verification**: +- Regression proven: temporarily reverted the filter → the merged-ignored and mixed-count + tests FAIL; restored → all 4 PASS. +- tsc --noEmit clean. Full `pnpm build` clean (skeleton + dashboard copy). +- Full non-e2e suite from packages/codev: 6059 passed, 0 failed. Remaining failures are all + `.e2e.test.ts`/tower-shellper integration needing live services — pre-existing, environmental, + none touch forge/pr-search/spawn. + +NOTE on running tests: run vitest FROM packages/codev (the vitest-setup.ts sandbox-pin path is +cwd-relative); a root-level `npx vitest packages/codev` spuriously fails the isolation/metrics +tests because the setup file doesn't resolve. + +## PR (complete) + +PR #1673 opened (`Fixes #1637`), recorded with porch. Thread committed. + +CMAP (all scoped with `--project-id bugfix-1637`; bare invocation hit "Multiple +projects found"): +- gemini = APPROVE (HIGH, no key issues) +- codex = APPROVE (HIGH, no key issues) +- claude = APPROVE (HIGH) — 2 non-blocking nits addressed in commit a3b072ab8: + (1) gitlab/pr-search.sh now captures glab output before piping to jq (#1645 + convention) so a glab failure surfaces; (2) guard predicate broadened to + `!pr.state` so gitlab's empty-string state fallback is treated as open + (conservative), with a blank-state regression test. + +All three APPROVE. Handing off at the pr gate — awaiting architect approval +before merge (CMAP APPROVE is not merge authorization). + +## CI fix (post-gate-request) + +Architect flagged real CI red on 83756b9c5: `forge.test.ts` #1645 check +'no built-in provider resolves a concept to a shell builtin'. Cause: my CMAP +follow-up (a3b072ab8) reshaped gitlab/pr-search.sh to capture-then-pipe, which +hid `glab` from extractExecutable's first-line heuristic (it skips the `out=` +assignment, lands on the `printf` builtin). Fix (a834106b1): added explicit +`# forge-executable: glab` declaration, matching github/pr-list.sh. + +Lesson: after changing a forge SCRIPT's shape, re-run forge.test.ts — I only +re-ran guard+script tests on the CMAP follow-up. Full non-e2e suite now 6060 +passing; forge.test.ts 80 passing. Awaiting CI 7/7 on a834106b1 before +re-requesting the gate. + +## Provider audit + locked-state safety (owner-requested) + +Audited all four forge providers' pr-search resolution (verified empirically via +resolveAllConcepts): +- github -> github/pr-search.sh (gh: OPEN/CLOSED/MERGED) ✅ +- gitlab -> gitlab/pr-search.sh (jq normalization) ✅ +- gitea -> DISABLED in preset; guard never calls pr-search, whole check skipped — no regression +- linear -> falls through to github/pr-search.sh (Linear PRs live on GitHub) ✅ + +Owner asked to lock in the gitlab `locked` safety change: `locked` (transient +merging state) now maps to OPEN alongside `opened`, so the guard errs toward the +recoverable --force prompt instead of silently skipping a live collision +(commit 48df843c3). + +KNOWN pre-existing gap (NOT introduced here, flag for separate issue): gitlab's +`glab mr list --output json` returns GitLab-shaped objects (iid/source_branch), +not the contract's number/headRefName — so the guard's message reads +`PR #undefined` on gitlab. forge.ts documents non-github presets as best-effort +/ may-not-conform. State filtering works; field mapping is the older gap. + +## gitlab field-mapping fix (owner-requested "fix it now") + +Grounded the gitlab shape against the repo's own authority (gitlab/pr-list.sh +comment + pr-exists.sh): glab emits iid/source_branch/target_branch, not +number/headRefName/baseRefName. Fixed pr-search.sh to map all three in the jq +pass (commit fced6d1a1), so the guard shows the real MR number and consult's +findPRForIssue can resolve the base branch. Marked UNVERIFIED per #920 (no live +glab in authoring/CI). CI baseline before this: 7/7 green on e292b1889. diff --git a/packages/codev/scripts/forge/github/pr-search.sh b/packages/codev/scripts/forge/github/pr-search.sh index edc2736d9..211a7f0bc 100755 --- a/packages/codev/scripts/forge/github/pr-search.sh +++ b/packages/codev/scripts/forge/github/pr-search.sh @@ -1,7 +1,9 @@ #!/bin/sh # Forge concept: pr-search (GitHub via gh CLI) # Input: CODEV_SEARCH_QUERY -# Output: JSON [{number, headRefName, baseRefName}] +# Output: JSON [{number, headRefName, baseRefName, state}] # --state all is required so the search includes merged/closed PRs; without it # `gh pr list` defaults to --state open and post-merge lookups return nothing (#759). -exec gh pr list --state all --search "$CODEV_SEARCH_QUERY" --json number,headRefName,baseRefName +# `state` (OPEN/MERGED/CLOSED) lets consumers that only want open PRs filter +# client-side — the spawn collision guard must ignore merged/closed ones (#1637). +exec gh pr list --state all --search "$CODEV_SEARCH_QUERY" --json number,headRefName,baseRefName,state diff --git a/packages/codev/scripts/forge/gitlab/pr-search.sh b/packages/codev/scripts/forge/gitlab/pr-search.sh index 8da4306c9..60c6db0c8 100755 --- a/packages/codev/scripts/forge/gitlab/pr-search.sh +++ b/packages/codev/scripts/forge/gitlab/pr-search.sh @@ -1,5 +1,38 @@ #!/bin/sh -# Forge concept: pr-search (GitLab via glab CLI) +# Forge concept: pr-search (GitLab via glab CLI — merge requests) +# Output: JSON [{number, headRefName, baseRefName, state, ...glab fields}] +# +# ⚠️ UNVERIFIED — `glab` is not available in the authoring environment (#920); +# written against glab's documented `mr list --output json` shape (confirmed +# against the field mappings already documented in gitlab/pr-list.sh and +# gitlab/pr-exists.sh). Smoke-test before relying on it. +# # --all is required so the search includes merged/closed MRs; without it # `glab mr list` defaults to opened only and post-merge lookups return nothing (#759). -exec glab mr list --all --search "$CODEV_SEARCH_QUERY" --output json +# +# Maps glab's GitLab-shaped fields into the forge-neutral PrSearchItem contract +# (forge-contracts.ts); `. + {…}` is non-destructive for keys we don't touch: +# iid -> number (the MR number glab mr view/merge expects) +# source_branch -> headRefName +# target_branch -> baseRefName +# Without this the spawn guard's message reads `PR #undefined` and consult's +# findPRForIssue can't resolve the MR's base branch on GitLab. +# +# glab reports state lowercase (opened/merged/closed/locked); normalize to the +# GitHub convention (OPEN/MERGED/CLOSED) so consumers filter with one comparison. +# The spawn collision guard keys off state === "OPEN" to ignore merged MRs (#1637). +# `locked` (a transient merging state) maps to OPEN too: the guard should err +# toward the recoverable --force prompt, never silently skip a live collision. +# +# Not `glab … | jq`: POSIX sh has no pipefail, so the caller would see jq's exit +# status and a failed glab would look like a successful empty list (#1645). +# The capture-then-pipe shape hides `glab` from extractExecutable's first-line +# heuristic (it lands on the `printf` builtin), so declare the backend explicitly +# — same as github/pr-list.sh (#1645). +# forge-executable: glab +out="$(glab mr list --all --search "$CODEV_SEARCH_QUERY" --output json)" || exit 1 +printf '%s' "$out" | jq 'map(. + + {number: .iid} + + {headRefName: .source_branch} + + {baseRefName: .target_branch} + + {state: ((.state // "") | ascii_downcase as $s | if $s == "opened" or $s == "locked" then "OPEN" else ($s | ascii_upcase) end)})' diff --git a/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts b/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts index 7424b8675..0bc8e5066 100644 --- a/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts @@ -650,6 +650,66 @@ describe('spawn-worktree', () => { ); }); + it('ignores merged PRs referencing the issue (#1637)', async () => { + // pr-search runs --state all (#759), so merged PRs come back too. The guard + // must not count them as open, else it blocks spawns on already-resolved work. + const { existsSync } = await import('node:fs'); + vi.mocked(existsSync).mockReturnValueOnce(false); + executeForgeCommandMock.mockResolvedValueOnce([ + { number: 4747, headRefName: 'fix-a', state: 'MERGED' }, + { number: 4766, headRefName: 'fix-b', state: 'CLOSED' }, + ]); + const { fatal } = await import('../utils/logger.js'); + await expect( + checkBugfixCollisions(4750, '/tmp/wt', baseIssue, false), + ).resolves.toBeUndefined(); + expect(fatal).not.toHaveBeenCalled(); + }); + + it('still fatals for an open PR referencing the issue (#1637)', async () => { + const { existsSync } = await import('node:fs'); + vi.mocked(existsSync).mockReturnValueOnce(false); + executeForgeCommandMock.mockResolvedValueOnce([{ number: 99, headRefName: 'fix-42', state: 'OPEN' }]); + const { fatal } = await import('../utils/logger.js'); + await checkBugfixCollisions(42, '/tmp/wt', baseIssue, false); + expect(fatal).toHaveBeenCalledWith(expect.stringContaining('open PR')); + }); + + it('counts only the open PRs when results mix open and merged (#1637)', async () => { + const { existsSync } = await import('node:fs'); + vi.mocked(existsSync).mockReturnValueOnce(false); + executeForgeCommandMock.mockResolvedValueOnce([ + { number: 10, headRefName: 'merged', state: 'MERGED' }, + { number: 11, headRefName: 'open', state: 'OPEN' }, + ]); + const { fatal } = await import('../utils/logger.js'); + await checkBugfixCollisions(42, '/tmp/wt', baseIssue, false); + expect(fatal).toHaveBeenCalledWith(expect.stringContaining('Found 1 open PR(s)')); + }); + + it('treats a missing state as open for stale pr-search overrides (#1637)', async () => { + // A project-local pr-search.sh predating the state field returns no state; + // the guard keeps the conservative pre-fix behavior rather than silently + // dropping the collision check. + const { existsSync } = await import('node:fs'); + vi.mocked(existsSync).mockReturnValueOnce(false); + executeForgeCommandMock.mockResolvedValueOnce([{ number: 99, headRefName: 'fix-42' }]); + const { fatal } = await import('../utils/logger.js'); + await checkBugfixCollisions(42, '/tmp/wt', baseIssue, false); + expect(fatal).toHaveBeenCalledWith(expect.stringContaining('open PR')); + }); + + it('treats a blank state as open (gitlab pr-search leaves it empty) (#1637)', async () => { + // gitlab/pr-search.sh maps a missing glab state to "" via jq; the guard must + // treat that the same as absent — conservatively counting it as open. + const { existsSync } = await import('node:fs'); + vi.mocked(existsSync).mockReturnValueOnce(false); + executeForgeCommandMock.mockResolvedValueOnce([{ number: 99, headRefName: 'fix-42', state: '' }]); + const { fatal } = await import('../utils/logger.js'); + await checkBugfixCollisions(42, '/tmp/wt', baseIssue, false); + expect(fatal).toHaveBeenCalledWith(expect.stringContaining('open PR')); + }); + it('warns when issue is already closed', async () => { const { existsSync } = await import('node:fs'); vi.mocked(existsSync).mockReturnValueOnce(false); diff --git a/packages/codev/src/agent-farm/commands/spawn-worktree.ts b/packages/codev/src/agent-farm/commands/spawn-worktree.ts index 471cd4bc8..b6bce2b4f 100644 --- a/packages/codev/src/agent-farm/commands/spawn-worktree.ts +++ b/packages/codev/src/agent-farm/commands/spawn-worktree.ts @@ -587,20 +587,29 @@ export async function checkBugfixCollisions( } } - // 3. Check for open PRs referencing this issue via pr-search concept + // 3. Check for open PRs referencing this issue via pr-search concept. + // pr-search runs `--state all` (#759) so consult can find merged PRs; this guard + // must ignore merged/closed ones, else it blocks spawns on already-resolved work + // (#1637). Filter to state === "OPEN"; treat an absent/empty state (a stale + // pr-search.sh override predating the field, or a forge script that leaves it + // blank) as unknown → keep counting it, the conservative pre-#1637 behavior, + // with --force still available. try { const result = await executeForgeCommand('pr-search', { CODEV_SEARCH_QUERY: `in:body #${issueNumber}`, }, { forgeConfig }); - if (result && Array.isArray(result) && result.length > 0) { - const openPRs = result as Array<{ number: number; title?: string; headRefName?: string }>; - if (!force) { - const prList = openPRs.slice(0, 5).map((pr) => - ` - PR #${pr.number}${pr.title ? `: ${pr.title}` : ''}`, - ).join('\n'); - fatal(`Found ${openPRs.length} open PR(s) referencing issue #${issueNumber}:\n${prList}\nUse --force to proceed anyway.`); + if (result && Array.isArray(result)) { + const prs = result as Array<{ number: number; title?: string; headRefName?: string; state?: string }>; + const openPRs = prs.filter((pr) => !pr.state || pr.state === 'OPEN'); + if (openPRs.length > 0) { + if (!force) { + const prList = openPRs.slice(0, 5).map((pr) => + ` - PR #${pr.number}${pr.title ? `: ${pr.title}` : ''}`, + ).join('\n'); + fatal(`Found ${openPRs.length} open PR(s) referencing issue #${issueNumber}:\n${prList}\nUse --force to proceed anyway.`); + } + logger.warn(`Warning: Found ${openPRs.length} open PR(s) referencing issue - proceeding with --force`); } - logger.warn(`Warning: Found ${openPRs.length} open PR(s) referencing issue - proceeding with --force`); } } catch { // Non-fatal: continue if PR search concept unavailable diff --git a/packages/codev/src/commands/porch/__tests__/bugfix-759-pr-search-state-all.test.ts b/packages/codev/src/commands/porch/__tests__/bugfix-759-pr-search-state-all.test.ts index bcbf11e32..56086e72b 100644 --- a/packages/codev/src/commands/porch/__tests__/bugfix-759-pr-search-state-all.test.ts +++ b/packages/codev/src/commands/porch/__tests__/bugfix-759-pr-search-state-all.test.ts @@ -32,6 +32,14 @@ describe('pr-search forge scripts', () => { const content = fs.readFileSync(scriptPath, 'utf-8'); expect(content).toContain('--search "$CODEV_SEARCH_QUERY"'); }); + + it('emits state so the spawn guard can filter merged PRs (#1637)', () => { + // Paired-consumer contract: --state all keeps merged PRs visible for consult + // (#759), while the `state` field lets the spawn collision guard ignore them. + const content = fs.readFileSync(scriptPath, 'utf-8'); + expect(content).toContain('--state all'); + expect(content).toMatch(/--json\s+\S*\bstate\b/); + }); }); describe('gitlab/pr-search.sh', () => { @@ -50,5 +58,26 @@ describe('pr-search forge scripts', () => { const content = fs.readFileSync(scriptPath, 'utf-8'); expect(content).toContain('--search "$CODEV_SEARCH_QUERY"'); }); + + it('normalizes opened/locked MRs to the OPEN state so the guard can filter (#1637)', () => { + // glab reports state lowercase; the script maps opened -> OPEN so the spawn + // collision guard's state === "OPEN" comparison is forge-agnostic. `locked` + // (a transient merging state) also maps to OPEN so the guard errs toward the + // recoverable --force prompt rather than silently skipping a live collision. + const content = fs.readFileSync(scriptPath, 'utf-8'); + expect(content).toContain('--all'); + expect(content).toContain('"OPEN"'); + expect(content).toMatch(/"opened"\s+or\s+\$s\s*==\s*"locked"/); + }); + + it('maps glab fields to the PrSearchItem contract (#1637)', () => { + // glab emits iid/source_branch/target_branch, not number/headRefName/ + // baseRefName. Without the mapping the spawn guard prints `PR #undefined` + // and consult can't resolve the MR base branch. + const content = fs.readFileSync(scriptPath, 'utf-8'); + expect(content).toContain('number: .iid'); + expect(content).toContain('headRefName: .source_branch'); + expect(content).toContain('baseRefName: .target_branch'); + }); }); }); diff --git a/packages/codev/src/lib/forge-contracts.ts b/packages/codev/src/lib/forge-contracts.ts index 079c4be05..8ba1b600a 100644 --- a/packages/codev/src/lib/forge-contracts.ts +++ b/packages/codev/src/lib/forge-contracts.ts @@ -149,6 +149,16 @@ export type RecentlyMergedResult = MergedPrItem[]; export interface PrSearchItem { number: number; headRefName: string; + /** + * PR/MR state, normalized to the GitHub convention: `OPEN` | `MERGED` | + * `CLOSED`. Because `pr-search` runs `--state all` (#759), consumers that + * only want open PRs must filter on this — the spawn collision guard ignores + * non-open results (#1637). Optional: a stale project-local override of + * `pr-search.sh` may predate this field (and the gitlab script leaves it + * blank when glab omits it), so consumers treat an absent or empty value as + * "unknown" rather than assuming a state. + */ + state?: string; } /** Output of the `pr-search` concept command. */