docs(specs): a task remembers every PR it has been associated with - #839
Conversation
wojciechszyjka
added
review
|
🤖
Update — 🤖
|
📸
|
| Image | Screen | Role |
|---|---|---|
current-01-tasks-overview.png |
Tasks overview (Ref column) + sidebar | Current — one chip, #831, the last declaration. #812 and #824 from the same task are unreachable from anywhere in the cockpit. |
current-02-task-thread-header.png |
Task-thread header meta line | Current — the same single #831 chip beside the branch and issue chips. |
mockup-01-task-thread-header.png |
Task-thread header | Proposed — primary chip + inline siblings + +N, with the popover listing every PR, its state and how it was associated. |
mockup-02-sidebar-and-overview.png |
Sidebar row + overview Ref column | Proposed — the 264 px sidebar keeps one chip with a count suffix; the table gains a two-character +N that opens the same popover. |
The mockups are illustrative statics (.html sources committed beside the spec under .ai/specs/assets/task-pr-reference-list/), not app code — they exist to communicate layout and priority, not pixels.
|
🤖 This spec had open questions and the run is autonomous, so I applied conservative
None carries a ⚠ NEEDS HUMAN CONFIRMATION marker: every default is additive, display-tier only, and revertible without touching persisted state — which is why the PR is open ready for review rather than held as a draft. To change any answer: reply here (or edit the spec's "Resolved assumptions" section) and re-run |
🤖
|
📦 npm preview published —
|
|
🤖 |
🔍 Specification review: a task remembers every PR it has been associated withVerdict: 🔴 Changes requested — 2 blockers, 2 majors, 4 minors, 2 nits. This is a strong, unusually well-grounded spec: the tier separation (created vs display) is respected, 💥 What can go wrong🔴 Blocker —
|
| Severity | Count | Findings |
|---|---|---|
| 🔴 Blocker | 2 | prNumber projection contradicted by three unlisted writers; legacy seed promotes a regex/namer guess above a declaration |
| 🟠 Major | 2 | §8 marker-semantics change declared as "unchanged"; Phase 2 demotion diverges from the persisted prNumber and the task title |
| 🔵 Minor | 4 | wrong file for the draft-PR path; v1-parity.test.ts does not exist; chip aria-label has no number; @patzick's stacked-PR ask unaccounted for |
| ⚪ Nit | 2 | skeleton drift (assumptions placement, no Research/Testability); store.ts:779 → :783 |
The two blockers share one root: the spec treats prNumber as if the list owned it, while the codebase has three other writers and one documented precedence rule (marker beats regex/namer) that the primary rule silently inverts. Resolving them is a Data Model edit plus three rows in the Architecture table — no rethink of the design.
wojciechszyjka
added
changes-requested
- rank the primary by provenance (created > marker > legacy > derived) so a CEZ:PR= declaration still outranks a number the regex or the namer guessed, preserving the rule at store.ts:848-852 - enumerate all six prNumber writers, including workflows/run.ts:769, :1835 and :3179, and route them through the store helper — without them 'prNumber is a projection' is not true and the field silently drifts out of the list - declare the CEZ:PR= display narrowing under BACKWARD_COMPATIBILITY.md §8 and schedule the matching handoff.ts instruction amendment, instead of claiming the agent protocol is unchanged - state the deliberate Phase 2 divergence: closed-PR demotion is display-only, prNumber and the task title stay on the offline primary - point the created-tier work at server.ts:4118-4127 (server/pr.ts is a re-export barrel), name versioned-surface.test.ts instead of a non-existent v1-parity.test.ts, fix store.ts:779 -> :783 - require per-chip accessible names (today's aria-label carries no number, so sibling chips read identically), and account for the stacked-PR ask from #779 - move Resolved assumptions ahead of the design, add a Testability section
🔍 Re-review: a task remembers every PR it has been associated withVerdict: 🟢 Approvable — all 2 blockers, 2 majors and 4 minors from the review above are resolved in Findings — how each one ended
VerificationRe-checked every claim the review had verified, against the pushed head: the six writer references ( Validation: unchanged from the review — the repository's five gate commands ( The spec is ready to implement. |
wojciechszyjka
added
merge-queue
|
🤖 Specification review found 2 blockers, 2 majors, 4 minors and 2 nits; the autofix loop (this PR is the automation's own) resolved every actionable one in the spec document across two commits — Labels:
|
|
🤖 All checks green on head
Head is |
|
🤖 Note on the claim: the previous This run exists to supply the one thing that PR could not: a formal approving-or-rejecting review from a different account, since GitHub refuses self-review. The prior verdict is treated as inherited input to be verified independently, not as a conclusion to rubber-stamp. |
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Specification review: a task remembers every PR it has been associated with
Verdict: 🔴 Changes requested — 2 majors, 1 minor, 1 nit. No blockers.
First formal review on this PR: the two earlier passes were posted as comments by the PR's own author account, which GitHub bars from reviewing itself. I re-derived everything from the code rather than trusting those conclusions.
The grounding here is genuinely excellent, and I checked it rather than assuming it. Of ~20 code references, 19 are exact: store.ts:862 (run.prNumber = refs.pr), :296 (MAX_PR_CANDIDATES = 8), the :848-852 precedence comment, run.ts:769 / :1835 / :3179 / :3226 / :3233, server.ts:4118-4127, pr.ts (a 7-line barrel, exactly as claimed), forge/github.ts:1401, contract/runs.ts:206, tasks-table.ts:135/:171, task-quick-list.tsx:364, run-header.tsx:504, tasks-overview.tsx:632/:884, handoff.ts:152, and auto-name.ts:198. Every Phase 2 primitive it leans on is real (/github/checks at server.ts:4702, fetchGithubChecks, resolveRepoHandle, and all three named suites route-parity / versioned-surface / bc-route-inventory). The §8 reading — that the section protects what a marker does, not just its spelling — is correct and well handled.
Both majors are the same root cause, and it is the one thing the previous pass's fix introduced rather than removed.
💥 What can go wrong
🟠 Major 1 — the created tier does not write prNumber today, so making it the top-ranked provenance silently changes a persisted value
.ai/specs/2026-08-10-task-pr-reference-list.md:47 says "Every writer that today assigns run.prNumber … is not three call sites but six", and the Data Model invariant at :130 concludes prNumber's "meaning to an external reader [is] unchanged".
Two of those six do not assign prNumber at all today:
- the janitor writes
this.updateRun(runId, { pullRequestUrl: created })(store.ts:759) - the draft-PR route writes
pullRequestUrl: outcome.url(server.ts:4127)
Because created is also the highest provenance rank, routing them through recordPrRef gives the created PR the primary slot — and therefore prNumber — where today it has no claim on that field at all.
Concretely, using the spec's own regression scenario (:222): task text "port the fix from pr 441" → today prNumber stays 441 for the life of the run even after the task opens PR #900. After Phase 1 it becomes 900. The spec asserts exactly this outcome as the expected test result, so the behavior is intended — it is the declaration that is missing.
That matters because prNumber is a §3-protected runs.json field, and §3's own precedent requires value changes to be written down: the diffStat.repointed bullet spells out "What did change for those runs is the values". Plan step 10 (:228) commits only to a §3 bullet for prRefs plus the downgrade note. Downstream readers that move: the CEZ:TITLE= prefix (run.ts:3233 reads current?.prNumber), the run DTO, and any script reading runs.json.
Not a blocker, because no rendered surface regresses — taskReference already prefers the URL-derived number (tasks-table.ts:174-175), so the chip shows #900 today too. The divergence is confined to the persisted field and the title prefix.
To resolve: keep the behavior, and declare it — a §3 bullet stating that prNumber now tracks the created PR where it previously kept a number scraped from the task text, plus a line in the Data Model replacing the "meaning unchanged" claim. Worth a CHANGELOG line for the same reason the diffStat change got one. Alternatively, if the title prefix must stay stable, rank created below marker/derived for the projection while keeping it primary for display — but that reintroduces exactly the drift Phase 1 exists to kill, so declaring is the better trade.
🟠 Major 2 — referencedPullRequestUrl is called a projection of the list, but the plan leaves it on last-declaration-wins
The TLDR (:5) and the Proposed Solution (:44) both say prNumber / pullRequestUrl / referencedPullRequestUrl become "derived projections" of prRefs. The plan does not make two of them projections:
pullRequestUrl— explicitly and correctly not one (:65, first-wins, action gates read it directly).referencedPullRequestUrl— plan step 2 (:220) says its "re-resolution stay[s] exactly as [it is]", i.e. still driven bymarkerRefs.pr, the latest declaration (store.ts:866-871).
So after CEZ:PR=A then CEZ:PR=B on a task with no created PR: the primary prRef is #A (earliest marker) and prNumber is A, while referencedPullRequestUrl still resolves to B's URL. Two persisted fields describing the same association disagree, and referencedPullRequestUrl is the one taskPrUrl falls back to (tasks-table.ts:139) — so unless taskPrRefs overrides it, the chip shows #B while prNumber says A, contradicting the spec's stated primary rule.
To resolve: pick one and say so — either recompute referencedPullRequestUrl from the primary too (and note the §3 value change alongside Major 1), or keep it last-declaration and stop calling it a projection, documenting the divergence the way :131 already does for the Phase 2 case. The second is likely right, since the field's name means "the PR most recently declared".
📋 What's missing
🔵 Minor — the six-writer enumeration is the reason Major 1 went unnoticed
:47 and the Risks bullet at :201 ("the five other places that also write that field") both assert a fact the code contradicts, and the spec leans on that enumeration as its proof that the projection holds. The Architecture table (:69-:71) describes each writer's actual job correctly, so an implementer following the table will build the right thing — but the argument above it should say "every writer that must now record an association" rather than "every writer that assigns prNumber", and distinguish the four that write the field today from the two that write pullRequestUrl.
📝 Improvements
⚪ Nit — one line reference is off by two
:74 and :164 cite reference-chip.tsx:50 for the accessible name; the aria-label is at :48. The observation itself — that today's accessible name carries no number, so sibling chips would read identically — is correct and is one of the sharpest points in the spec.
✅ Checked and found sound
- 🔁 Backward compatibility: the §8 narrowing of
CEZ:PR='s effect is declared rather than asserted away, with the matchinghandoff.ts:152amendment as Phase 1 work — that is the right call, and §8's "changing what an emitted marker does" clause confirms it was required.runs.jsonadditivity,.catch([])degradation and the lossy-but-safe downgrade round-trip are all correct;.catch([]).optional()is the right operator order to keep absent records absent. - 🎯 Simplicity: the five rejected alternatives are argued on evidence, not taste — in particular the refusal to merge
referencedPrCandidatesinto a rendered list, which would rebuild #526. - Inherited feedback: the four earlier findings (blockers on the projection and the legacy seed, the §8 semantics, the Phase 2 title divergence) are genuinely resolved in
5d333e4a+6015a950— I verified each against the spec text, not just the claim. The one declined nit (a Research section) I concur with: the Alternatives analysis covers that ground.
🧪 Validation gate
Docs-only diff (one markdown file + 4 assets + 2 HTML mockups), so every configured command is non-applicable — the repo defines no markdown linter or link checker:
| Command | Result |
|---|---|
npm run typecheck |
⏭️ skipped — no TypeScript in the diff |
npm test / npm run test:unit |
⏭️ skipped — no code under test |
npm run build |
⏭️ skipped — nothing to compile |
npm run test:package |
⏭️ skipped — package surface untouched |
Evidence instead: CI ran the full gate green on head 6015a950 (Unit/build/E2E/package, npm snapshot, license/cla all ✅), and no check is pending.
🤖 This PR is autofix-eligible on this run, so I am fixing these findings in the spec document myself rather than handing them back — no action needed from @wojciechszyjka. A re-review follows on the updated head.
pat-lewczuk
added
changes-requested
…RequestUrl split Review of #839 found two claims the codebase does not support, both stemming from the same imprecision in the writer enumeration. The created tier (janitor `store.ts:758`, draft-PR route `server.ts:4127`) writes `pullRequestUrl`, never `prNumber`. Enrolling it as the top-ranked `created` provenance therefore makes `prNumber` start naming a created PR where it previously kept a number scraped from the task text — a real value change to a BC §3-protected field that the spec asserted was "unchanged". Declared now under Data Model, Compatibility with runs.json, Risks, the Edge Cases table and plan steps 3 and 10, following the precedent §3 set for the `diffStat.repointed` value shift. The TLDR and Proposed Solution also called all three scalars projections of the list, while plan step 2 leaves `referencedPullRequestUrl` on last-declaration-wins. Only `prNumber` becomes a projection; the other two are preserved deliberately so a rollback still finds today's answer. The resulting divergence is now stated, with tests pinning it in steps 2 and 6. Also corrects `reference-chip.tsx:50` to `:48`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Re-review: a task remembers every PR it has been associated with
Verdict: 🟢 Approved — all 4 findings from the review above are resolved in 2f657000.
This is the formal approving review the PR has been waiting for since 2026-08-11. The two earlier passes were posted as comments because GitHub bars an author from reviewing their own PR; reviewing from a different account is the only thing that was actually missing, and the merge block should now clear.
Findings — how each one ended
| # | Finding | Outcome |
|---|---|---|
| 🟠 1 | prNumber's persisted value changes for created-PR runs, undeclared under §3 |
Fixed. The behaviour is kept — it is the more truthful answer — and now declared in five places: a new Data Model invariant with the 441 → 900 worked example, a Compatibility with runs.json bullet citing the diffStat.repointed precedent and stating that historical records are not backfilled, a Risks bullet tying it to the drift fix it comes from, an Edge Cases row, and plan steps 3 and 10 (§3 bullet + a CHANGELOG line naming the task-title prefix as the user-visible consequence). |
| 🟠 2 | referencedPullRequestUrl called a projection while step 2 leaves it last-declaration-wins |
Fixed, resolving toward not a projection — the right call, since the field's name means "most recently declared" and keeping its rule is what makes rollback free. The TLDR and Proposed Solution no longer claim all three scalars are projections; a new bullet states which read moves (the chip's fallback, now a list read) and which do not. The divergence is pinned by tests in steps 2 and 6 rather than left to be discovered. |
| 🔵 3 | The six-writer enumeration asserted all six write prNumber |
Fixed. Split into "four already assign run.prNumber" and "two assign pullRequestUrl and never touch prNumber", with the note that the second group is the behavioural change. The Risks bullet is reworded to "record an association". This was the root cause of finding 1, so correcting it is what made the omission visible. |
| ⚪ 4 | reference-chip.tsx:50 → :48 |
Fixed in both occurrences. |
🔁 Correction to my own review
I wrote the janitor's write as store.ts:759. It is at :758 — the spec had it right, and a fix I briefly applied has been reverted. The substance is unaffected: that line is this.updateRun(runId, { pullRequestUrl: created }), which writes pullRequestUrl and not prNumber, which is what finding 1 rests on.
🧪 Validation
Docs-only diff, so the same four commands remain non-applicable (no markdown linter or link checker is configured) — unchanged from the review above. Line references in the amended text were re-verified against the worktree: store.ts:758 / :848-852 / :854 / :862 / :296, server.ts:4118-4127, run.ts:769 / :1835 / :3179 / :3233, tasks-table.ts:139 / :174-175, reference-chip.tsx:48. The new tasks-table.ts:139 and :174-175 citations I added are correct.
CI on the previous head was fully green; the new head carries a markdown-only change, and its run is reported below.
Merge readiness: approved and in merge-queue. skip-qa is correct — there is no runtime behaviour to exercise. Note this branch is ~52 commits behind main, though MERGEABLE with no conflicts; a squash merge will be clean.
🤖 om-auto-review-pr — review + autofix, 1 fix iteration.
pat-lewczuk
added
merge-queue
|
🏷️
|
|
🤖 Specification review found 2 majors, 1 minor and 1 nit; the autofix loop resolved all four in the spec document in one iteration ( What this run added that the earlier passes could not: a formal approving review. The two reviews on 2026-08-11 were posted as comments because GitHub refuses to let an author review their own PR, so The findings were not a re-run of the earlier ones. Enrolling the created tier as the top-ranked provenance — the fix for the earlier pass's first blocker — is what makes Labels: |
|
🤖 All checks green on head
|
2 participants
Refs #779
Source doc:
.ai/specs/2026-08-10-task-pr-reference-list.mdStatus: complete
🎯 Goal
📐 Design in one line
RunRecordgains one additive optionalprRefslist (append-only, deduped, capped at 8);prNumber/pullRequestUrl/referencedPullRequestUrlbecome derived projections of that list, so every existing reader, script and hand-editedruns.jsonkeeps working and pre-spec records need no migration — they derive a one-entry view at read time.What Changed
.ai/specs/2026-08-10-task-pr-reference-list.md.ai/specs/assets/task-pr-reference-list/(also attached below as inline evidence)💥 Breaking Changes
runs.json(§3) and the run DTO, no route change in Phase 1, and a documented (non-destructive) downgrade behaviour when an older cezar rewrites the file.