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
Original file line numberDiff line numberDiff line change
@@ -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
41 changes: 41 additions & 0 deletions codev/state/bugfix-901_thread.md
Original file line numberDiff line numberDiff line change
@@ -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.
32 changes: 32 additions & 0 deletions packages/codev/src/agent-farm/__tests__/overview.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 () => {
Expand Down
26 changes: 25 additions & 1 deletion packages/codev/src/agent-farm/servers/overview.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 };
Expand DownExpand Up@@ -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<string>();
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;
}
Expand Down
38 changes: 37 additions & 1 deletion packages/dashboard/__tests__/NeedsAttentionList.test.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = [
Expand All@@ -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();
Expand All@@ -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.
Expand Down
27 changes: 24 additions & 3 deletions packages/dashboard/src/components/NeedsAttentionList.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand DownExpand Up@@ -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`
Expand DownExpand Up@@ -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}`,
Expand DownExpand Up@@ -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 <p className="work-empty">Nothing needs attention</p>;
Expand Down
1 change: 1 addition & 0 deletions packages/dashboard/src/components/WorkView.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -121,6 +121,7 @@ export function WorkView({ state, onRefresh, onSelectTab }: WorkViewProps) {
<NeedsAttentionList
prs={overview?.pendingPRs ?? []}
builders={overview?.builders ?? []}
recentlyMergedIssueIds={overview?.recentlyMergedIssueIds ?? []}
/>
)}
</section>
Expand Down
10 changes: 10 additions & 0 deletions packages/types/src/api.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 };
Expand Down
Loading