Skip to content

[Bugfix #901] Skip stale post-merge prReady rows in NeedsAttentionList#902

Open
waleedkadous wants to merge 7 commits into
mainfrom
builder/bugfix-901
Open

[Bugfix #901] Skip stale post-merge prReady rows in NeedsAttentionList#902
waleedkadous wants to merge 7 commits into
mainfrom
builder/bugfix-901

Conversation

@waleedkadous
Copy link
Copy Markdown
Contributor

@waleedkadous waleedkadous commented May 28, 2026

Summary

After a PR merges, NeedsAttentionList still surfaced the corresponding builder with a "PR review" badge. Cross-reference the defensive prReady builder-fallback against the workspace's recently-merged PR set so post-merge stale rows are suppressed.

Fixes #901

Root Cause

Two interacting problems documented in the issue:

  1. Data hazard (already fixed by Fix #887: BUGFIX add pr gate (close v3.1.4 pr_ready timing gap) #888): v3.1.4 advanceProtocolPhase line 453 re-set pr_ready_for_human = true on the terminal pr → verified advance. In-flight builders that crossed the v3.1.4 → v3.1.5 boundary keep the stale true in status.yaml.
  2. Consumer-side gap (this PR): NeedsAttentionList.buildItems iter-2 defensive fallback (PR [Bugfix #872] Fix: emit canonical pr_ready_for_human signal across all protocols #874) emits a row for any b.prReady === true builder whose PR isn't in pendingPRs[]. Intent was cache-miss defense, but the same path fires when the PR has been merged — merged PRs are correctly absent from pendingPRs (open-only), and surfacing the row is wrong.

The data needed was already fetched: overview.ts:911 calls fetchMergedPRsCached, but the result was only used internally to enrich recentlyClosed[] with prUrl.

Fix

  • OverviewData.recentlyMergedIssueIds: string[] — new field on both the wire type (packages/types/src/api.ts) and the internal overview type (packages/codev/src/agent-farm/servers/overview.ts).
  • Overview server projects mergedPRs → linked-issue IDs via parseLinkedIssue, deduplicated, attached to every response (empty array when forge fails).
  • NeedsAttentionList.buildItems accepts the merged-issue list and skips the defensive emit when b.issueId matches a recently-merged PR's linked issue.
  • WorkView passes overview.recentlyMergedIssueIds through.

The PR-loop is untouched — it iterates open PRs only, so already correct. No one-shot migration needed for stale pr_ready_for_human: true rows: this filter masks them in the UI, and they self-correct as new builders use #888's code path.

Test Plan

  • Regression test added: builder with prReady: true + issueId in recentlyMergedIssueIds → no row emitted.
  • Defensive test updated: explicit empty recentlyMergedIssueIds argument, comment clarifies that the test models the cache-miss scenario (NOT merged).
  • Overview-server test added: recentlyMergedIssueIds populated from linked-issue parsing of fetchMergedPRsCached; deduplicated; empty [] when the call returns null.
  • Build passes (porch check: ✓ build 4.1s, ✓ tests 20.1s).
  • All NeedsAttentionList tests (13/13) and overview tests (149/149) green.

Scope

  • In scope: data-flow + consumer fix; no behavior change for cache-miss scenarios (defensive emit still fires when the missing PR is NOT in the merged set).
  • Out of scope: one-shot migration to clean up stale pr_ready_for_human: true in already-verified status.yaml files (per the issue — self-corrects, and this fix masks them anyway).

CMAP Review (3-way, iter 1)

Model Verdict Confidence Key issues
Gemini APPROVE HIGH None
Codex APPROVE HIGH None
Claude APPROVE HIGH None (one non-blocking style note on test-call self-documentation; not addressed — see below)

Claude's non-blocking observation: a couple of existing buildItems(prs, builders) test invocations rely on the new recentlyMergedIssueIds = [] default rather than passing [] explicitly. Functionally identical to the updated test at line 208 that passes [] with a clarifying comment — left as-is to keep the diff minimal.

Full per-reviewer outputs auto-persisted at codev/projects/bugfix-901-needs-attention-surfaces-build/bugfix-901-pr-iter1-{gemini,codex,claude}.txt.

Cross-reference the defensive prReady builder-fallback against the
workspace's recently-merged PR set. After a PR merges, pendingPRs (open
PRs only) correctly omits it, but in-flight builders that crossed the
v3.1.4 → v3.1.5 line-453 setter boundary keep stale pr_ready_for_human
in status.yaml. The iter-2 cache-miss defense (PR #874) couldn't
distinguish "PR missing because cache miss" from "PR missing because
merged" and surfaced both — emitting a stale "PR review" row for work
already shipped.

Adds OverviewData.recentlyMergedIssueIds (derived from the already-
fetched mergedPRs list via parseLinkedIssue) and plumbs it into
NeedsAttentionList.buildItems. The defensive emit now skips when the
builder's issueId is in the merged set.

Fixes #901
Adds an overview-server regression test that exercises the dedup `seen`
set in `recentlyMergedIssueIds`: when the same issue has two merged PRs
in the 24h window (e.g. a primary fix and a follow-up), the consumer
must see one entry, not two.

Thread note documents the dual `OverviewData` interfaces (shared in
packages/types/src/api.ts plus a local copy in
packages/codev/src/agent-farm/servers/overview.ts) — both had to be
updated to satisfy the local tsc check.
@waleedkadous
Copy link
Copy Markdown
Contributor Author

Builder note — PR ready for architect review

CMAP-3 (gemini/codex/claude, --protocol bugfix --type pr) all returned:

  • APPROVE / HIGH confidence / KEY_ISSUES: None

Iter-1 outputs auto-persisted at codev/projects/bugfix-901-needs-attention-surfaces-build/bugfix-901-pr-iter1-{gemini,codex,claude}.txt and the verdict table is appended to the PR description.

Running porch done bugfix-901 to request the pr gate. Tried afx send architect first — Tower's terminal registry isn't resolving the architect target from this worktree right now (and afx send --all returns NOT_FOUND for unrelated names), so leaving this PR comment as the notification surface and relying on the gate-requested state on pr for architect-side visibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Needs Attention surfaces builders whose PR is already merged (stale prReady)

1 participant