diff --git a/codev/projects/bugfix-966-needs-attention-merged-pr-with/status.yaml b/codev/projects/bugfix-966-needs-attention-merged-pr-with/status.yaml new file mode 100644 index 000000000..6bd7cd6ef --- /dev/null +++ b/codev/projects/bugfix-966-needs-attention-merged-pr-with/status.yaml @@ -0,0 +1,17 @@ +id: bugfix-966 +title: needs-attention-merged-pr-with +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-06-03T03:25:02.400Z' + approved_at: '2026-06-03T13:39:33.573Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-06-02T20:38:06.755Z' +updated_at: '2026-06-03T13:39:33.573Z' +pr_ready_for_human: false diff --git a/codev/state/bugfix-966_thread.md b/codev/state/bugfix-966_thread.md new file mode 100644 index 000000000..dd63f507e --- /dev/null +++ b/codev/state/bugfix-966_thread.md @@ -0,0 +1,63 @@ +# bugfix-966 — Needs Attention: merged PR with still-pending pr gate vanishes + +## Issue +`derivePrReady` in `packages/codev/src/agent-farm/servers/overview.ts` returns `true` +for a builder whose `pr` gate is `pending` + `requested_at` present, *even when the PR +has already merged*. A merged PR lives in `recentlyClosed`, not `pendingPRs`, so: +- `NeedsAttentionList` loop (A) emits no PR row (PR not in `pendingPRs`) +- loop (B) suppresses the builder row (`if (b.prReady) continue;`) +→ builder vanishes from the Work surface entirely. + +## Investigation findings +- `merged: true` lives inside the `pr_history:` list in status.yaml (4-space indent, + one entry per PR/stage). Last entry corresponds to the PR tied to the current pr gate. +- The parser (`parseStatusYaml`) does NOT currently parse `pr_history` at all. +- Confirmed fix is sufficient (no NeedsAttentionList change): once `prReady` is false, + the builder is no longer suppressed in loop (B). Its `pr` gate is still `pending` with + `requested_at`, so `detectBlocked` returns 'PR review' → builder surfaces as a gate row. + +## Plan +1. Parse `merged` boolean from the LAST `pr_history` entry (handles SPIR checkpoint + multi-PR case: an earlier merged PR + a later open PR awaiting review → not merged). +2. `derivePrReady` returns false when `parsed.merged` is true. +3. Unit tests: parseStatusYaml parses pr_history merged; derivePrReady false when merged. + +## Implementation (done) +- `overview.ts`: added `merged: boolean` to `ParsedStatus`; parse it from the LAST + `pr_history` entry; `derivePrReady` now `&& !parsed.merged`. +- `overview.test.ts`: +8 `#966` tests (5 parseStatusYaml, 2 derivePrReady, 1 end-to-end + repro proving the builder surfaces via the gate row, not vanishes). All 8 pass. tsc clean. + +## Environment blocker (NOT my code) +- Worktree had no node_modules. `pnpm install` fails compiling `better-sqlite3@12.9.0`: + node-gyp reports "No Xcode or CLT version detected" (CLT pkgutil receipt missing) even + though `xcode-select -p` is valid. node v26 (ABI 147) has no prebuilt; falls back to a + compile that fails. Main workspace's binary is ABI 141 → incompatible with node 26. +- Effect: repo-wide `npm test` fails on better-sqlite3 (consult/metrics + OverviewCache + tests). My change adds ZERO new failures — the 5 failing tests in overview.test.ts are + all `new Database(...)` ABI errors; the 153 non-sqlite tests pass. +- This will block porch's `fix`-phase `tests` check. Needs CLT reinstall (user/sudo) or a + node downgrade. Escalating to architect rather than bypassing. + +## Environment UNBLOCKED (architect-authorized) +- Bumped better-sqlite3 ^12.5.0 → ^12.10.0 (commit 45da56b7). 12.10.0 ships a node-26 + ABI-147 prebuilt → `pnpm install` downloads it (prebuild-install, no compile, no CLT). +- `porch check`: build ✓, tests ✓ (full suite green under node 26, incl. my 8 #966 tests). + +## PR +- PR #980 opened (Fixes #966). Net diff ~ overview.ts (57) + tests (152) + bump/lockfile. +- Running 3-way CMAP (gemini/codex/claude --protocol bugfix --type pr). + +## CMAP (3-way) — all APPROVE / HIGH / no key issues +- Gemini ✅, Codex ✅, Claude ✅. One non-blocking note (Claude): test comment named an + external adopter → scrubbed in 61333c38 per anonymization policy. + +## Status +- [x] Investigate / root cause +- [x] Implement fix +- [x] Tests (8 new #966 tests pass; tsc clean) +- [x] Environment unblocked (better-sqlite3 bump) +- [x] PR #980 created +- [x] CMAP review (all APPROVE) + scrub +- [x] porch done → pr gate requested; architect notified +- [ ] AWAITING human pr-gate approval (do not self-approve / self-merge) diff --git a/packages/codev/package.json b/packages/codev/package.json index 280faf8ae..0ad0cab99 100644 --- a/packages/codev/package.json +++ b/packages/codev/package.json @@ -41,7 +41,7 @@ "@anthropic-ai/claude-agent-sdk": "^0.2.41", "@google/genai": "^1.0.0", "@openai/codex-sdk": "^0.130.0", - "better-sqlite3": "^12.5.0", + "better-sqlite3": "^12.10.0", "chalk": "^5.3.0", "commander": "^12.1.0", "glob": "^11.0.0", diff --git a/packages/codev/src/agent-farm/__tests__/overview.test.ts b/packages/codev/src/agent-farm/__tests__/overview.test.ts index af7a9490f..aa6becb10 100644 --- a/packages/codev/src/agent-farm/__tests__/overview.test.ts +++ b/packages/codev/src/agent-farm/__tests__/overview.test.ts @@ -348,6 +348,102 @@ describe('overview', () => { const result = parseStatusYaml(yaml); expect(result.gateApprovedAt).toEqual({}); }); + + it('defaults merged to false when pr_history is absent (#966)', () => { + const result = parseStatusYaml("id: '0100'\nphase: review"); + expect(result.merged).toBe(false); + }); + + it('parses merged: true from the pr_history entry (#966)', () => { + const yaml = [ + "id: '2019'", + 'protocol: bugfix', + 'phase: review', + 'gates:', + ' pr:', + ' status: pending', + " requested_at: '2026-06-02T15:14:59.000Z'", + 'pr_history:', + ' - phase: review', + ' pr_number: 2030', + ' branch: builder/spir-2019', + " created_at: '2026-06-02T15:10:00.000Z'", + ' merged: true', + " merged_at: '2026-06-02T15:14:34.000Z'", + 'pr_ready_for_human: true', + ].join('\n'); + + const result = parseStatusYaml(yaml); + expect(result.merged).toBe(true); + // sanity: the surrounding fields still parse correctly around the new section + expect(result.gates['pr']).toBe('pending'); + expect(result.gateRequestedAt['pr']).toBe('2026-06-02T15:14:59.000Z'); + }); + + it('parses merged: false (open PR) in pr_history (#966)', () => { + const yaml = [ + "id: '0100'", + 'phase: review', + 'pr_history:', + ' - phase: review', + ' pr_number: 50', + ' branch: builder/bugfix-100', + " created_at: '2026-06-02T15:10:00.000Z'", + ' merged: false', + ].join('\n'); + + const result = parseStatusYaml(yaml); + expect(result.merged).toBe(false); + }); + + it('reflects the LAST pr_history entry — earlier merged PR + later open PR (#966)', () => { + // SPIR/PIR checkpoint workflow: an earlier checkpoint PR merged, but a later + // PR is still open and awaiting review. The current pr gate is for the LATEST + // PR (not merged), so the builder is genuinely PR-ready. + const yaml = [ + "id: '0100'", + 'protocol: spir', + 'phase: review', + 'pr_history:', + ' - phase: implement', + ' pr_number: 40', + ' branch: builder/spir-100', + " created_at: '2026-06-01T00:00:00.000Z'", + ' merged: true', + " merged_at: '2026-06-01T01:00:00.000Z'", + ' - phase: review', + ' pr_number: 41', + ' branch: builder/spir-100', + " created_at: '2026-06-02T00:00:00.000Z'", + ' merged: false', + ].join('\n'); + + const result = parseStatusYaml(yaml); + expect(result.merged).toBe(false); + }); + + it('reflects the LAST pr_history entry when the later entry has no merged key (#966)', () => { + // The later (open) PR has no `merged` key at all — must still read as not-merged. + const yaml = [ + "id: '0100'", + 'protocol: spir', + 'phase: review', + 'pr_history:', + ' - phase: implement', + ' pr_number: 40', + ' branch: builder/spir-100', + " created_at: '2026-06-01T00:00:00.000Z'", + ' merged: true', + " merged_at: '2026-06-01T01:00:00.000Z'", + ' - phase: review', + ' pr_number: 41', + ' branch: builder/spir-100', + " created_at: '2026-06-02T00:00:00.000Z'", + ].join('\n'); + + const result = parseStatusYaml(yaml); + expect(result.merged).toBe(false); + }); }); // ========================================================================== @@ -730,6 +826,7 @@ describe('overview', () => { planPhases: [], startedAt: '2026-05-26T00:00:00.000Z', prReadyForHuman: null, + merged: false, ...overrides, }; } @@ -763,6 +860,28 @@ describe('overview', () => { }))).toBe(false); }); + it('returns false when the PR has merged but the pr gate is still pending (#966)', () => { + // Repro: porch left the `pr` gate pending after an out-of-band merge. The + // merged PR is in recentlyClosed (never pendingPRs), so a stale prReady:true + // would suppress the builder row without emitting a PR row → vanishes. + expect(derivePrReady(makeParsed({ + protocol: 'bugfix', + phase: 'review', + gates: { pr: 'pending' }, + gateRequestedAt: { pr: '2026-06-02T15:14:59Z' }, + merged: true, + }))).toBe(false); + }); + + it('still returns true when the pr gate is pending, requested, and NOT merged (#966)', () => { + // The companion to the merged case: an open PR genuinely awaiting review. + expect(derivePrReady(makeParsed({ + gates: { pr: 'pending' }, + gateRequestedAt: { pr: '2026-05-26T12:00:00Z' }, + merged: false, + }))).toBe(true); + }); + it('reads the pr gate directly and ignores pr_ready_for_human (#927)', () => { // Field true but gate not pending → not ready (kills the sticky-field hazard #919). expect(derivePrReady(makeParsed({ @@ -791,6 +910,39 @@ describe('overview', () => { it('returns false when no pr-gate signal is present', () => { expect(derivePrReady(makeParsed({ protocol: 'spir', phase: 'implement' }))).toBe(false); }); + + it('repro #966: merged-but-gate-pending builder surfaces via the gate row, not as PR-ready', () => { + // Full chain on the real #966 repro shape: porch recorded + // merged: true but left the pr gate pending. The builder must NOT + // read as PR-ready (else NeedsAttentionList suppresses its row while no PR + // row exists — it vanishes). It must instead surface as a blocked "PR review" + // gate row, since the pr gate is genuinely still pending. This is why fixing + // derivePrReady alone is sufficient (no NeedsAttentionList change needed). + const yaml = [ + "id: '2019'", + 'protocol: bugfix', + 'phase: review', + 'gates:', + ' pr:', + ' status: pending', + " requested_at: '2026-06-02T15:14:59.000Z'", + 'pr_history:', + ' - phase: review', + ' pr_number: 2030', + ' branch: builder/spir-2019', + " created_at: '2026-06-02T15:10:00.000Z'", + ' merged: true', + " merged_at: '2026-06-02T15:14:34.000Z'", + 'pr_ready_for_human: true', + ].join('\n'); + + const parsed = parseStatusYaml(yaml); + // No longer suppressed as a (now-merged) PR-ready builder... + expect(derivePrReady(parsed)).toBe(false); + // ...and surfaces via the gate-row path instead (pr gate still pending). + expect(detectBlocked(parsed)).toBe('PR review'); + expect(detectBlockedSince(parsed)).toBe('2026-06-02T15:14:59.000Z'); + }); }); describe('computeIdleMs', () => { diff --git a/packages/codev/src/agent-farm/servers/overview.ts b/packages/codev/src/agent-farm/servers/overview.ts index 81756da3b..995a1daa8 100644 --- a/packages/codev/src/agent-farm/servers/overview.ts +++ b/packages/codev/src/agent-farm/servers/overview.ts @@ -196,6 +196,19 @@ interface ParsedStatus { * value and the v3.1.3 fallback derivation. */ prReadyForHuman: boolean | null; + /** + * Whether the PR tied to the current `pr` gate has already been merged + * (Issue #966). Read from the LAST entry of porch's `pr_history` list — the + * most-recent PR is the one the current `pr` gate was requested for. The + * last-entry semantics handle SPIR/PIR checkpoint workflows where an earlier + * PR merged but a later PR is still open and awaiting review. + * + * `derivePrReady` consults this so a merged-but-gate-still-pending builder + * (porch left the gate pending after an out-of-band merge — see #966) no + * longer reads as a PR awaiting human review. Defaults to `false` when + * `pr_history` is absent or its last entry has no `merged: true`. + */ + merged: boolean; } /** @@ -215,10 +228,11 @@ export function parseStatusYaml(content: string): ParsedStatus { planPhases: [], startedAt: '', prReadyForHuman: null, + merged: false, }; const lines = content.split('\n'); - let section: 'none' | 'gates' | 'plan_phases' = 'none'; + let section: 'none' | 'gates' | 'plan_phases' | 'pr_history' = 'none'; let currentGate = ''; let currentPlanPhase: Partial | null = null; @@ -261,6 +275,12 @@ export function parseStatusYaml(content: string): ParsedStatus { continue; } + if (/^pr_history:\s*$/.test(line)) { + if (currentPlanPhase) { pushPlanPhase(result, currentPlanPhase); currentPlanPhase = null; } + section = 'pr_history'; + continue; + } + // Stop section at next top-level key if (/^\S/.test(line) && line.trim() !== '') { if (currentPlanPhase) { pushPlanPhase(result, currentPlanPhase); currentPlanPhase = null; } @@ -318,6 +338,30 @@ export function parseStatusYaml(content: string): ParsedStatus { continue; } } + + // pr_history section (#966). Each list item is one PR/stage. We only need the + // merged status of the LAST entry (the PR the current `pr` gate was requested + // for). On a new list item we reset to its default (`false`); a `merged: true` + // line within the entry flips it. The final value therefore reflects the last + // entry — so an earlier merged checkpoint PR followed by a later still-open PR + // correctly reads as not-merged. + if (section === 'pr_history') { + const itemStartMatch = line.match(/^\s{2}-\s+(.*)$/); + if (itemStartMatch) { + result.merged = false; + // The dash line carries the entry's first key/value; handle the (unusual) + // case where that first key is `merged` itself. + const firstKv = itemStartMatch[1].match(/^merged:\s*(true|false)\s*$/); + if (firstKv) result.merged = firstKv[1] === 'true'; + continue; + } + + const mergedMatch = line.match(/^\s{4}merged:\s*(true|false)\s*$/); + if (mergedMatch) { + result.merged = mergedMatch[1] === 'true'; + continue; + } + } } // Flush last plan phase if we were in that section @@ -504,9 +548,18 @@ export function detectBlockedSince(parsed: ParsedStatus): string | null { * rollback hazard from #919) nor the old `bugfix && phase === 'verified'` * fallback (a crutch for a gateless BUGFIX variant — gateless PR-producing * protocols do not surface PR rows, by design). + * + * The `!parsed.merged` conjunct (#966) handles the case where porch left the + * `pr` gate `pending` after the PR already merged (an out-of-band GitHub merge + * plus a resume can scramble the ordering). A merged PR lives in + * `recentlyClosed`, never `pendingPRs`, so a stale `prReady: true` would + * suppress the builder's row in NeedsAttentionList without ever emitting a PR + * row — making the builder vanish. Once merged, it is no longer "a PR awaiting + * human review": derivePrReady returns false, and the still-pending `pr` gate + * surfaces the builder via the gate-row path instead. */ export function derivePrReady(parsed: ParsedStatus): boolean { - return parsed.gates['pr'] === 'pending' && !!parsed.gateRequestedAt['pr']; + return parsed.gates['pr'] === 'pending' && !!parsed.gateRequestedAt['pr'] && !parsed.merged; } /** diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4794508a8..689c2d0be 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -23,8 +23,8 @@ importers: specifier: ^0.130.0 version: 0.130.0 better-sqlite3: - specifier: ^12.5.0 - version: 12.9.0 + specifier: ^12.10.0 + version: 12.10.0 chalk: specifier: ^5.3.0 version: 5.6.2 @@ -1517,9 +1517,9 @@ packages: engines: {node: '>=6.0.0'} hasBin: true - better-sqlite3@12.9.0: - resolution: {integrity: sha512-wqUv4Gm3toFpHDQmaKD4QhZm3g1DjUBI0yzS4UBl6lElUmXFYdTQmmEDpAFa5o8FiFiymURypEnfVHzILKaxqQ==} - engines: {node: 20.x || 22.x || 23.x || 24.x || 25.x} + better-sqlite3@12.10.0: + resolution: {integrity: sha512-CyzaZRQKyHkB2ZInfTTl2nvT33EbDpjkLEbE8/Zck3Ll6O0qqvuGdrJ45HgtH+HykRg88ITY3AdreBGN70aBSQ==} + engines: {node: 20.x || 22.x || 23.x || 24.x || 25.x || 26.x} bidi-js@1.0.3: resolution: {integrity: sha512-RKshQI1R3YQ+n9YJz2QQ147P66ELpa1FQEg20Dk8oW9t2KgLbpDLLp9aGZ7y8WHSshDknG0bknqGw5/tyCs5tw==} @@ -4461,7 +4461,7 @@ snapshots: baseline-browser-mapping@2.10.18: {} - better-sqlite3@12.9.0: + better-sqlite3@12.10.0: dependencies: bindings: 1.5.0 prebuild-install: 7.1.3