[Spec 927] Needs Attention: surface PRs via the universal pr gate; delete gateless builder-derived fallbacks - #928
Merged
Merged
Conversation
Needs Attention: surface PRs via the universal pr gate; delete gateless builder-derived fallbacks. Grounds the issue's direction in the current overview.ts / NeedsAttentionList code and surfaces the verify-approval, recentlyMergedIssueIds, and derivePrReady-form decisions.
3-way: Gemini APPROVE, Claude APPROVE, Codex REQUEST_CHANGES (both items resolved). Pinned the dashboard-local scope of the no-builder-stand-in rule (keep pr in shared GATE_LABELS), the requested_at-aware gate-pending invariant, the verify-approval='verify review' label contract, and the detectBlockedSince sync point. Corrected two over-broad reviewer suggestions: gate-pending check must include requested_at, and fetchRecentMergedPRs is retained (second consumer).
3 phases: (1) server-derivation — gate-authoritative derivePrReady + verify-approval label + unify detectBlockedSince [shared infra / VSCode blast radius]; (2) dashboard-surfacing — PR-rows-only buildItems + verify styling; (3) remove-dead-projection — delete recentlyMergedIssueIds end-to-end, retain fetchRecentMergedPRs. VSCode blast radius is an explicit cross-cutting item with test coverage + an Amr review flag.
3-way: Gemini APPROVE, Claude APPROVE, Codex REQUEST_CHANGES (resolved). Phase 1 now OWNS the VSCode blast-radius validation (api.ts prReady doc-comment rewrite + vscode builders.test.ts + VSCode acceptance criteria/tests; pr regression-guarded). Resolved Codex's 'discussed not owned' + '+types omitted' points and Claude's test-scope ambiguity. Added dev-review gateKindClass gap as a flagged optional drive-by (default out-of-scope). Teed up VSCode UX + drive-by decisions for plan-approval / Amr.
…al + verify-approval label - derivePrReady: reduce to the requested_at-aware pr-gate-pending check; drop the pr_ready_for_human field dependency and the bugfix && verified fallback. The pending pr gate is the universal post-CMAP signal. - GATE_LABELS: add verify-approval -> 'verify review' (pr stays mapped so VSCode is unaffected; dashboard excludes pr locally in a later phase). - detectBlockedSince: iterate Object.keys(GATE_LABELS) instead of a separate hardcoded array (removes the silent drift hazard; verify-approval now carries blockedSince). - api.ts + overview.ts: rewrite prReady doc comments to the gate-authoritative contract. - Tests: rewrite derivePrReady block (universal-gate true; requested_at guard; ignores pr_ready_for_human; no bugfix-verified fallback); add verify-approval cases to detectBlocked/detectBlockedSince; add VSCode blast-radius suite in builders.test.ts (verify-review surfaces as blocked; pr unchanged). porch build + tests green; vscode tests type-check (Electron run is area/vscode CI).
…tion + verify styling - buildItems: delete the builder-emit branch and its bookkeeping (emittedPrReadyIssueIds, mergedIssueIdSet, recentlyMergedIssueIds param). PR-ready builders now surface ONLY as PR rows; the builder loop early-outs on if (b.prReady) continue, so a cache-missed pr-gate builder emits nothing and the pr gate never becomes a gate row. - Gate-row loop now naturally covers spec/plan/dev/verify-approval. - gateKindClass: add 'verify review' -> attention-kind--verify; index.css: add the matching rule (review-style accent). - WorkView: stop passing recentlyMergedIssueIds; NeedsAttentionList prop dropped. - Tests: invert the two 'missing PR still surfaces a builder' tests (now assert no row), replace the merged-suppression test with a 'missing PR => no row' case (recentlyMergedIssueIds removed), add a verify-approval gate-row test. Dashboard tests 14/14; porch build + tests green.
…ses gate timestamp exclusively (Codex CMAP) Codex REQUEST_CHANGES (Phase 2 iter-1): the affiliated PR path could still fall back to pr.createdAt, and two tests encoded the now-impossible gateless BUGFIX shape (prReady=true with no gate/no blockedSince). - buildItems: waitingSince = prReady ? (readySince ?? pr.createdAt) : pr.createdAt. Affiliated PRs use the builder's gate-requested time (blockedSince); the ?? is an unreachable type guard, not the old gateless fallback. createdAt is now reserved for unaffiliated/human PRs. - tests: reframe the BUGFIX test to the post-#887 gate shape (asserts waitingSince === blockedSince); remove the impossible 'no blockedSince' fallback test; assert unaffiliated PR uses createdAt. 13/13 pass. porch build + tests green.
…rgedIssueIds end-to-end The merged-suppression projection (#901/#902) is dead once the dashboard builder-emit branch is gone (Phase 2). Remove it: - OverviewData (api.ts + overview.ts mirror): drop the recentlyMergedIssueIds field. - overview.ts: delete the merged-issue computation block and its inclusion in the returned result. - overview.test.ts: remove the two recentlyMergedIssueIds tests + the null-mergedPRs empty-array assertion. RETAINED: fetchRecentMergedPRs, the mergedPRs fetch, and the issueToPrUrl map — still needed for recentlyClosed PR-link enrichment (positive prUrl test at L1853 still passes, proving the fetch is wired). porch build + tests green; dashboard 13/13.
Net -45 LOC across packages/. Documents the universal-pr-gate Needs Attention contract, the dashboard-local no-builder-stand-in rule, the verify-approval label/styling (render-verified in headless chromium), and reconciles #919/#902. Captures the consultation history (Codex was the consistent skeptic; all blocks legitimate and addressed) and MAINTAIN-candidate lessons.
…R CMAP) Codex PR-level REQUEST_CHANGES (C1): approved spec/plan artifacts must carry YAML frontmatter (approved + validated) per the SPIR final-approval steps; they were left as Status: draft. Add frontmatter (approved: 2026-05-29, validated: [gemini, codex, claude]) and flip Status -> approved on both. C2 (dev-review gateKindClass styling) rebutted: explicitly ruled out of scope by the architect at plan-approval; tracked as a separate follow-up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #927.
What
Reworks the dashboard's Needs Attention surface so PR-readiness keys on the universal
prgate instead of fragile builder-state derivations. Net −45 LOC (3 SPIR phases) — mostly deletion.Needs Attention = (A) ∪ (B):
prgate → PR rows only (never a builder standing in for a PR; a cache-missed/merged PR yields no row).REVIEW_REQUIRED) fallback.How (by phase)
packages/codev,packages/types):derivePrReadyreduced to therequested_at-awarepr-gate-pending check (drops thebugfix && verifiedfallback and thepr_ready_for_humanfield dependency — kills the Terminal state 'verified' over-promises: split back into 'complete' (phases done) vs 'verified' (verify-approval passed) #919 sticky-field hazard).verify-approval → "verify review"added toGATE_LABELS;detectBlockedSinceunified onObject.keys(GATE_LABELS).packages/dashboard): deleted the builder-emit branch;if (b.prReady) continue;makes PR-ready builders surface only as PR rows; affiliated PRs use the gate timestamp (createdAtonly for unaffiliated);verifystyling (gateKindClass+.attention-kind--verify).packages/codev,packages/types): removedrecentlyMergedIssueIdsend-to-end. RetainedfetchRecentMergedPRs(still feeds the recentlyClosed PR-link map).Shared-infra / VSCode blast radius (cc area/vscode — @amr)
prstays in the sharedGATE_LABELS/detectBlocked*so VSCode (tree/toast/status-bar) is unchanged; the "no builder stand-in" rule is dashboard-local. New:verify-approvalnow surfaces as a blocked builder in VSCode via the gate-toast generic fallback (added abuilders.test.tsregression-guard). Open question for Amr: is that the desired VSCode UX, or do you want a dedicatedGATE_ACTIONS"Verify" action? (additive follow-up — not a blocker.)Verification
build+testsgreen at every phase; dashboardNeedsAttentionList13/13; VSCodebuilders.test.tstype-checks (Electron run = area CI).--verifystyling in headless chromium against the built CSS:.attention-kind--verify→rgb(234,179,8)(--status-waiting, == PR row), distinct from spec and from an unstyled control.Relationships
derivePrReadyparts — recommend descoping Terminal state 'verified' over-promises: split back into 'complete' (phases done) vs 'verified' (verify-approval passed) #919 to just theverified → completerename.recentlyMergedIssueIdsretired.See
codev/reviews/927-needs-attention-surface-prs-vi.mdfor the full review, consultation history, and lessons.