diff --git a/codev/projects/bugfix-901-needs-attention-surfaces-build/status.yaml b/codev/projects/bugfix-901-needs-attention-surfaces-build/status.yaml new file mode 100644 index 00000000..c2137bb9 --- /dev/null +++ b/codev/projects/bugfix-901-needs-attention-surfaces-build/status.yaml @@ -0,0 +1,17 @@ +id: bugfix-901 +title: needs-attention-surfaces-build +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-05-28T02:13:21.360Z' + approved_at: '2026-05-28T18:59:18.179Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-05-28T01:48:37.300Z' +updated_at: '2026-05-28T18:59:18.179Z' +pr_ready_for_human: false diff --git a/codev/state/bugfix-901_thread.md b/codev/state/bugfix-901_thread.md new file mode 100644 index 00000000..17af917f --- /dev/null +++ b/codev/state/bugfix-901_thread.md @@ -0,0 +1,41 @@ +# bugfix-901 — Needs Attention surfaces post-merge builders + +## Investigation + +Issue #901: After PR merge, builder still surfaces under Needs Attention with stale `prReady: true`. Two interacting problems documented in the issue: + +1. **Data hazard** (already fixed by #888): v3.1.4 re-set `pr_ready_for_human=true` on terminal `pr→verified` advance. In-flight builders that crossed that boundary still carry the stale `true` in status.yaml. Self-corrects for new builders. +2. **Consumer-side gap** (this fix): `NeedsAttentionList.buildItems` (packages/dashboard/src/components/NeedsAttentionList.tsx:108) defensive fallback emits a row for any `b.prReady === true` builder whose PR isn't in `prs[]`. The intent was cache-miss defense (PR #874 iter-2). But merged PRs are also absent from `prs[]` (which lists open PRs only) — the same code path fires wrongly. + +The data we need is already fetched: `overview.ts:911` calls `fetchMergedPRsCached(workspaceRoot)` and uses the result internally to enrich `recentlyClosed[]` with `prUrl`. It just isn't exposed on `OverviewData` for `NeedsAttentionList` to cross-reference. + +## Approach + +Minimal fix: + +1. Add `recentlyMergedIssueIds: string[]` to `OverviewData` (`packages/types/src/api.ts`). Just the issue-ID set — that's all the consumer needs. +2. Populate it in `overview.ts` from the already-fetched `mergedPRs` via `parseLinkedIssue`. +3. Plumb through `NeedsAttentionList`'s `buildItems` signature and use it to gate the defensive fallback emit. +4. Update tests: regression test + clarify the defensive test's setup. + +PR-loop is untouched — it iterates open PRs only, so already correct. + +## Implementation notes + +- The codev package defines its OWN `OverviewData` interface (`packages/codev/src/agent-farm/servers/overview.ts:156`) in addition to the shared one in `packages/types/src/api.ts`. Both needed updating — otherwise the local tsc check fails with `Object literal may only specify known properties` even though the wire type is correct. Worth noting if anyone wonders why the change touches two type declarations. +- The merged-PR projection runs unconditionally even when `closed === null` (the existing recentlyClosed block early-outs). This is intentional — the consumer needs the merged-issue set independent of whether the closed-issue enrichment succeeded. +- `recentlyMergedIssueIds: readonly string[]` in the React prop with a default of `[]` so the change is backwards-compatible for any tests/callers that don't (yet) pass it. + +## PR + +PR #902 opened. CMAP-3 (gemini/codex/claude) ran in parallel via consult. + +## CMAP-3 result + +All three: **APPROVE / HIGH / no key issues**. Iter-1 outputs persisted at +`codev/projects/bugfix-901-needs-attention-surfaces-build/bugfix-901-pr-iter1-{gemini,codex,claude}.txt`. +Claude flagged a non-blocking style nit (two existing `buildItems(prs, builders)` +test calls rely on the new default param instead of passing `[]` explicitly) — +left as-is. PR body updated with the verdict table. + +Notifying architect + running `porch done bugfix-901` to request the pr gate. diff --git a/packages/codev/src/agent-farm/__tests__/overview.test.ts b/packages/codev/src/agent-farm/__tests__/overview.test.ts index 4280be5a..277a202e 100644 --- a/packages/codev/src/agent-farm/__tests__/overview.test.ts +++ b/packages/codev/src/agent-farm/__tests__/overview.test.ts @@ -1878,6 +1878,38 @@ describe('overview', () => { expect(data.recentlyClosed).toHaveLength(1); expect(data.recentlyClosed[0].prUrl).toBeUndefined(); + // Issue #901: null merged-PR fetch should still yield an empty array, + // not undefined, so NeedsAttentionList's `?? []` doesn't have to absorb + // an inconsistent shape. + expect(data.recentlyMergedIssueIds).toEqual([]); + }); + + it('exposes recentlyMergedIssueIds for merged-PR consumers (Issue #901)', async () => { + mockFetchMergedPRs.mockResolvedValue([ + { number: 150, title: '[Bugfix #100] Fix the bug', url: 'https://github.com/org/repo/pull/150', body: 'Fixes #100', createdAt: '2026-01-02T00:00:00Z', mergedAt: new Date().toISOString() }, + { number: 151, title: '[Spec 200] Add feature', url: 'https://github.com/org/repo/pull/151', body: 'Closes #200', createdAt: '2026-01-02T00:00:00Z', mergedAt: new Date().toISOString() }, + { number: 152, title: 'No linked issue', url: 'https://github.com/org/repo/pull/152', body: 'Cleanup', createdAt: '2026-01-02T00:00:00Z', mergedAt: new Date().toISOString() }, + ]); + + const cache = new OverviewCache(); + const data = await cache.getOverview(tmpDir); + + expect(new Set(data.recentlyMergedIssueIds)).toEqual(new Set(['100', '200'])); + }); + + it('deduplicates recentlyMergedIssueIds when the same issue has multiple merged PRs', async () => { + // A follow-up PR can land while the original sits in the 24h merged + // window — both PRs reference the same `Fixes #N`. The consumer wants + // a SET of issue IDs, not a multi-bag, so the dedup is load-bearing. + mockFetchMergedPRs.mockResolvedValue([ + { number: 150, title: '[Bugfix #100] First attempt', url: 'https://github.com/org/repo/pull/150', body: 'Fixes #100', createdAt: '2026-01-02T00:00:00Z', mergedAt: new Date().toISOString() }, + { number: 151, title: '[Bugfix #100] Follow-up', url: 'https://github.com/org/repo/pull/151', body: 'Fixes #100', createdAt: '2026-01-02T01:00:00Z', mergedAt: new Date().toISOString() }, + ]); + + const cache = new OverviewCache(); + const data = await cache.getOverview(tmpDir); + + expect(data.recentlyMergedIssueIds).toEqual(['100']); }); it('enriches issueId from DB issue_number for unknown protocols (#664)', async () => { diff --git a/packages/codev/src/agent-farm/servers/overview.ts b/packages/codev/src/agent-farm/servers/overview.ts index 249e0e8d..c645fdce 100644 --- a/packages/codev/src/agent-farm/servers/overview.ts +++ b/packages/codev/src/agent-farm/servers/overview.ts @@ -158,6 +158,13 @@ export interface OverviewData { pendingPRs: PROverview[]; backlog: BacklogItem[]; recentlyClosed: RecentlyClosedItem[]; + /** + * Issue IDs of PRs merged in the recent window (24h, matching + * `fetchRecentMergedPRs`). Consumers cross-reference this against a + * builder's `issueId` to skip stale post-merge `prReady` rows (Issue #901). + * Empty array when the forge call fails or yields no merge-linked rows. + */ + recentlyMergedIssueIds: string[]; /** Auto-detected forge login of the current user (via the user-identity concept). */ currentUser?: string; errors?: { prs?: string; issues?: string }; @@ -996,7 +1003,24 @@ export class OverviewCache { }); } - const result: OverviewData = { builders, pendingPRs, backlog, recentlyClosed }; + // 6. Project merged-PR linked-issue IDs for consumers (Issue #901). + // NeedsAttentionList uses this set to skip the cache-miss defensive + // emit for prReady builders whose PR is correctly absent from + // pendingPRs because the PR has been merged. Empty when the forge + // call fails (mergedPRs === null) or returned no merge-linked rows. + const recentlyMergedIssueIds: string[] = []; + if (mergedPRs) { + const seen = new Set(); + for (const pr of mergedPRs) { + const linkedIssue = parseLinkedIssue(pr.body || '', pr.title); + if (linkedIssue !== null && !seen.has(linkedIssue)) { + seen.add(linkedIssue); + recentlyMergedIssueIds.push(linkedIssue); + } + } + } + + const result: OverviewData = { builders, pendingPRs, backlog, recentlyClosed, recentlyMergedIssueIds }; if (currentUser) { result.currentUser = currentUser; } diff --git a/packages/dashboard/__tests__/NeedsAttentionList.test.tsx b/packages/dashboard/__tests__/NeedsAttentionList.test.tsx index 129db5df..fbbd79c8 100644 --- a/packages/dashboard/__tests__/NeedsAttentionList.test.tsx +++ b/packages/dashboard/__tests__/NeedsAttentionList.test.tsx @@ -189,6 +189,11 @@ describe('NeedsAttentionList buildItems — PR gating (issue #844)', () => { // the realistic shape, the original gate-loop early-out filtered the // builder before the prReady check could fire. Architect-side CMAP // caught this; the loop now checks prReady BEFORE the !b.blocked skip. + // + // Issue #901: the defensive emit is only correct when the PR is missing + // for a "still open" reason (cache / pagination / transient failure). + // recentlyMergedIssueIds is the merged-PR-issue set; this test must NOT + // include the builder's issueId there, or the row is correctly skipped. const prs: OverviewPR[] = []; const startedAt = new Date('2026-01-05T12:00:00Z').toISOString(); const builders = [ @@ -205,7 +210,7 @@ describe('NeedsAttentionList buildItems — PR gating (issue #844)', () => { }), ]; - const items = buildItems(prs, builders); + const items = buildItems(prs, builders, []); const row = items.find(i => i.key === 'gate-bugfix-42'); expect(row).toBeDefined(); @@ -214,6 +219,37 @@ describe('NeedsAttentionList buildItems — PR gating (issue #844)', () => { expect(row!.waitingSince).toBe(startedAt); }); + it('does NOT surface a prReady builder whose PR has been merged (Issue #901)', () => { + // After PR merge, the PR is correctly absent from `prs` (open-only) and + // the builder may still carry stale `prReady: true` in status.yaml — the + // v3.1.4 line-453 setter wrote it on terminal `pr → verified` advance; + // PR #888 fixed that but in-flight builders that crossed the v3.1.4 → + // v3.1.5 boundary keep the stale value. Cross-referencing the builder's + // issueId against recentlyMergedIssueIds suppresses the stale row + // without a one-shot migration. + const prs: OverviewPR[] = []; + const builders = [ + makeBuilder({ + id: 'spir-1842', + issueId: '1842', + protocol: 'spir', + phase: 'verified', + blocked: null, + blockedGate: null, + blockedSince: null, + startedAt: new Date('2026-01-05T12:00:00Z').toISOString(), + prReady: true, + }), + ]; + + // Merged-PR set includes 1842 — its PR is gone for a reason (merged), + // not for a transient failure. + const items = buildItems(prs, builders, ['1842']); + + expect(items.find(i => i.key === 'gate-spir-1842')).toBeUndefined(); + expect(items).toHaveLength(0); + }); + it('still surfaces a prReady gated builder (AIR/SPIR shape) when its PR is missing from prs', () => { // Gate-bearing variant — preserves the iter-1 coverage but with the // expectation explicitly tied to the gate-blocked shape. diff --git a/packages/dashboard/src/components/NeedsAttentionList.tsx b/packages/dashboard/src/components/NeedsAttentionList.tsx index caa5c5e9..bd35bb8f 100644 --- a/packages/dashboard/src/components/NeedsAttentionList.tsx +++ b/packages/dashboard/src/components/NeedsAttentionList.tsx @@ -3,6 +3,14 @@ import type { OverviewPR, OverviewBuilder } from '../lib/api.js'; interface NeedsAttentionListProps { prs: OverviewPR[]; builders: OverviewBuilder[]; + /** + * Issue IDs of recently-merged PRs (Issue #901). Used to distinguish + * "PR missing from `prs` because of cache miss / pagination" (surface + * the defensive row) from "PR missing because it has been merged" + * (skip — there is nothing left for the human to do). Defaults to an + * empty array for callers that haven't been wired up yet. + */ + recentlyMergedIssueIds?: readonly string[]; } interface AttentionItem { @@ -40,8 +48,13 @@ function timeAgo(dateStr: string): string { return `${days}d`; } -export function buildItems(prs: OverviewPR[], builders: OverviewBuilder[]): AttentionItem[] { +export function buildItems( + prs: OverviewPR[], + builders: OverviewBuilder[], + recentlyMergedIssueIds: readonly string[] = [], +): AttentionItem[] { const items: AttentionItem[] = []; + const mergedIssueIdSet = new Set(recentlyMergedIssueIds); // A PR is genuinely waiting on a human only after the builder finishes CMAP // for the PR-creating phase. Issue #872 made this a canonical `prReady` @@ -105,7 +118,15 @@ export function buildItems(prs: OverviewPR[], builders: OverviewBuilder[]): Atte // transient API failure). Surface anyway so we don't silently drop a real // human-action signal. blockedSince may be absent for gateless protocols // (BUGFIX) — fall back to startedAt so the waiting chip still renders. + // + // Skip when the builder's PR has been MERGED (Issue #901): pendingPRs + // lists open PRs only, so a merged PR is correctly absent — emitting the + // defensive row would surface a stale "PR review" item for work that's + // already shipped. The data hazard from v3.1.4 → v3.1.5 left some + // in-flight builders with stale `pr_ready_for_human: true` in + // status.yaml; this filter masks them without a one-shot migration. if (b.prReady) { + if (b.issueId && mergedIssueIdSet.has(b.issueId)) continue; const label = b.issueId ? `#${b.issueId}` : b.id; items.push({ key: `gate-${b.id}`, @@ -139,8 +160,8 @@ export function buildItems(prs: OverviewPR[], builders: OverviewBuilder[]): Atte return items; } -export function NeedsAttentionList({ prs, builders }: NeedsAttentionListProps) { - const items = buildItems(prs, builders); +export function NeedsAttentionList({ prs, builders, recentlyMergedIssueIds }: NeedsAttentionListProps) { + const items = buildItems(prs, builders, recentlyMergedIssueIds); if (items.length === 0) { return

Nothing needs attention

; diff --git a/packages/dashboard/src/components/WorkView.tsx b/packages/dashboard/src/components/WorkView.tsx index 4b4072c4..6f672312 100644 --- a/packages/dashboard/src/components/WorkView.tsx +++ b/packages/dashboard/src/components/WorkView.tsx @@ -121,6 +121,7 @@ export function WorkView({ state, onRefresh, onSelectTab }: WorkViewProps) { )} diff --git a/packages/types/src/api.ts b/packages/types/src/api.ts index 6efd9540..14f5889f 100644 --- a/packages/types/src/api.ts +++ b/packages/types/src/api.ts @@ -247,6 +247,16 @@ export interface OverviewData { pendingPRs: OverviewPR[]; backlog: OverviewBacklogItem[]; recentlyClosed: OverviewRecentlyClosed[]; + /** + * Issue IDs of PRs merged in the recent window (matches the + * `fetchRecentMergedPRs` 24h window in the overview server). Consumers + * cross-reference this against a builder's `issueId` to detect post-merge + * builders whose status.yaml may still carry stale `prReady: true` — used + * by `NeedsAttentionList` to skip the cache-miss defensive emit when the + * PR is correctly absent from `pendingPRs` because it has been merged + * (Issue #901). Empty array when the forge call fails or returns no rows. + */ + recentlyMergedIssueIds: string[]; /** Auto-detected GitHub login of the current user (via the user-identity forge concept). */ currentUser?: string; errors?: { prs?: string; issues?: string };