Skip to content

[Bugfix #844] Gate NeedsAttention PR rows on porch pr-gate#845

Merged
waleedkadous merged 7 commits into
mainfrom
builder/bugfix-844
May 24, 2026
Merged

[Bugfix #844] Gate NeedsAttention PR rows on porch pr-gate#845
waleedkadous merged 7 commits into
mainfrom
builder/bugfix-844

Conversation

@waleedkadous
Copy link
Copy Markdown
Contributor

@waleedkadous waleedkadous commented May 24, 2026

Summary

Fixes #844

Root Cause

packages/dashboard/src/components/NeedsAttentionList.tsx:42-56 iterated every open PR from overview.pendingPRs and emitted an attention row unconditionally. There was no cross-reference against the builder's porch gate state, so a PR appeared in Needs Attention the moment it was created — before the automated CMAP reviews (Gemini / Codex / Claude) had run. Reviewers arrived ahead of the AI review comments.

Fix

buildItems now consults OverviewBuilder.blocked / blockedSince to decide whether a PR is genuinely waiting on a human:

  1. Build prGateSince = Map<issueId, blockedSince> for builders with blocked === 'PR review' (the human-readable label for the porch pr gate, mapped in overview.ts:GATE_LABELS).
  2. For each PR, include it iff EITHER:
    • its linkedIssue matches a pr-gate builder (prGateSince.has(...)), OR
    • it has no associated builder AND reviewStatus === 'REVIEW_REQUIRED' (the human-authored / externally opened case — no porch gate to wait on, so fall back to GitHub's reviewDecision).
  3. waitingSince uses the builder's blockedSince for affiliated PRs (the "human became bottleneck" moment), falling back to pr.createdAt for unaffiliated PRs.
  4. Track emittedPrGateIssueIds; the builder loop only dedupes pr-gate builders whose PR was actually emitted. This protects against a stuck builder disappearing if its PR is missing from prs (cache miss, pagination, transient API failure).
  5. gateKindClass gains a 'PR review' → 'attention-kind--pr' case so the builder-loop fallback row from (4) renders with the same PR styling.

The linkedIssue plumbing already existed on OverviewPR (populated by parseLinkedIssue in overview.ts); no API/types change was needed.

Note on issue suggestion

The issue suggested dropping the continue at line 62, but doing so would create duplicate rows (one from the PR loop, one from the builder loop) for the same builder. Kept the skip and conditioned it on the emitted-set so a missing PR doesn't silently hide the gate signal.

Test Plan

  • 9 unit tests covering acceptance criteria + waitingSince + missing-PR fallback + dedupe
  • Local tests pass: pnpm test NeedsAttentionList → 9/9 passed
  • porch check passes (build + tests)

Test scenarios covered

  • PR with builder still in CMAP (blocked === null) → excluded
  • PR with builder at pr gate → included
  • PR with builder at pr gate → emitted exactly once (dedupe holds) ✓
  • Human-authored PR (no builder) + REVIEW_REQUIREDincluded
  • Human-authored PR (no builder) + APPROVEDexcluded
  • Other gate-pending builders (spec/plan/dev review) still surface ✓
  • PR with stale linkedIssue (no matching builder) treated as unaffiliated ✓
  • Affiliated PR waitingSince === builder.blockedSince (not pr.createdAt) ✓
  • Builder at pr-gate with missing PR still surfaces via builder loop ✓
  • Dedupe still holds when both PR and builder are present ✓

CMAP Review (PR-845, 3-way)

Model Verdict Confidence Notes
Codex APPROVE HIGH "Focused, addresses root cause, solid regression coverage."
Claude APPROVE HIGH "Clean, well-scoped." Flagged waitingSince as a non-blocking observation.
Gemini REQUEST_CHANGES HIGH Three points: waitingSince semantics, invisible stuck builders, missing gateKindClass case.

All three of Gemini's points addressed in iter-2 commit 7ec0fc9c.

Before: every open PR appeared in Needs Attention the instant it was
created, even while CMAP reviews were still running — reviewers arrived
before the AI comments did.

After: a PR with a tracked builder is surfaced only once that builder
reaches the porch `pr` gate (blocked === 'PR review'). PRs without an
associated builder (human-authored, externally opened) fall back to the
GitHub reviewDecision and are surfaced only when reviewStatus is
REVIEW_REQUIRED.

Adds buildItems export and 6 regression tests covering the scenarios
called out in the issue.
Three small refinements after the iter-1 CMAP review:

1. waitingSince now uses the builder's blockedSince (not pr.createdAt)
   for affiliated PRs — the wait-time chip now measures "how long since
   the human became the bottleneck," not "how long since the PR was
   opened while CMAP was still running." Falls back to pr.createdAt for
   unaffiliated PRs.

2. Builders stuck at the pr gate whose PR is missing from `prs` (cache
   miss, pagination, transient API failure) now surface via the builder
   loop instead of disappearing silently. Tracks an
   emittedPrGateIssueIds set during PR iteration and only dedupes when
   the PR was actually emitted.

3. gateKindClass now handles 'PR review' → 'attention-kind--pr' so the
   builder-loop fallback row from (2) renders with the same PR styling
   as the PR-loop emit.

3 new regression tests for these behaviors. 9/9 tests pass.
@waleedkadous
Copy link
Copy Markdown
Contributor Author

Excellent work — thank you. Approving.

Two things I particularly liked:

  1. Pushback on dropping the continue — the issue suggested removing it, you kept it and made it conditional on emittedPrGateIssueIds.has(issueId). That's a strictly better solution than what I proposed: it preserves the dedupe and gracefully handles the cache-miss case (PR not in prs yet, builder still surfaces). Good judgment.

  2. waitingSince = blockedSince, not createdAt (Gemini's catch). This is the right semantics — the waiting-time chip now measures "human-as-bottleneck" duration, which is what the Needs Attention surface is for.

The regression test for the double-emission invariant is the kind of test I'd hope someone would write but rarely see. Nice.

Please merge.

@waleedkadous waleedkadous merged commit d91fbb0 into main May 24, 2026
6 checks passed
@waleedkadous waleedkadous deleted the builder/bugfix-844 branch May 24, 2026 17:22
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 PRs before CMAP reviews finish

1 participant