docs(specs): Linked-PR chips on the GitHub Issues list - #816
Conversation
|
|
|
🤖
Current Proposed — green = open, purple = merged, red = closed; multiple PRs render up to 3 chips then Mockup is an illustrative static HTML render ( |
|
🤖 This spec had open questions and the run is autonomous, so I applied conservative, low-blast-radius defaults to keep moving. Please review and override before merge if any is wrong. (The load-bearing product decisions — source = GitHub linked PRs, marker = clickable chip, states = open + merged/closed, fetch = per-issue
No assumption needs human confirmation before merge (all are display-only and reversible). To change any: reply here or edit the spec's "Resolved assumptions" section, then implement with the corrected spec via |
|
🤖
|
🤖
|
|
🤖 |
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Specification review: Linked-PR chips on the GitHub Issues list
🎯 Summary
This PR is design-only: it adds .ai/specs/2026-08-09-issue-linked-pr-chip.md, the brainstorm brief it came from, and three mockup assets (a current-state screenshot, a proposed-state screenshot, and the static HTML that produced it). No code, config, or CI file is touched, so this ran as a specification review rather than a code review — the five design lenses below, with findings on the usual blocker/major/minor/nit scale.
The spec itself is genuinely good work and I want to say that before the findings. It picks the right architecture: an additive, windowed, lazily-hydrated sibling of the /api/v1/github/checks endpoint (#664) rather than fattening the one-shot list fetch, which is exactly the discipline #664 established and which BACKWARD_COMPATIBILITY.md §2 protects. It carries its rejected alternatives with measured evidence (the "invert closingIssuesReferences" path really does miss ~60% of the signal here, and gh pr list --json closingIssuesReferences really does error), it names the degradation contract the GitHub family requires ({ available: false, reason }, never a 5xx — AGENTS.md), and it resolves its own open questions with reversible defaults instead of leaving them dangling. The layer-by-layer architecture table and the step-by-step implementation plan are the right shape for handing to an implementing agent.
What holds it back is grounding. I checked every file and symbol the spec names against the actual tree at a4927c8, and the client layer is pointed at the wrong package — a load-bearing error that an implementer would inherit — plus four design/coverage gaps that would each cost a review round after implementation. All of them are cheap to fix in the document.
Verdict
❌ request changes — one blocker (the client layer is specified against packages/api-client, which contains no per-endpoint fetchers at all; the sibling this slice mirrors lives in packages/web/src/api/client.ts) and four majors (an off-by-design GraphQL fetch that can silently miss every linked PR on a busy issue, a chip DOM structure that nests an <a> inside the row <a>, no CEZ_DRY_RUN=1 mock path in a codebase where every forge fetcher has one and the whole e2e suite boots on it, and a test plan that names contract tests which do not exist while omitting the parity guard that does). None of these change the design's direction — the shape of the slice is right and I would approve it once the document says what the codebase actually is.
🧪 Validation Gate
The configured gate is aimed at code, and this diff contains only Markdown, PNG, and a standalone HTML mockup. There is no linter, no Markdown check, and no link checker in this repository — a fact the repo asserts about itself in packages/cezar/src/server/bc-route-inventory.test.ts's header comment — so there is no docs-applicable command to run. Every configured command was skipped, deliberately and for the same reason:
| Command | Status | Notes |
|---|---|---|
npm run typecheck |
⏭️ SKIPPED | Docs-only diff; no TypeScript changed, so the type gate has nothing to measure. |
npm test |
⏭️ SKIPPED | Docs-only diff; no code path is exercised by a Markdown or PNG file. |
npm run test:unit |
⏭️ SKIPPED | Docs-only diff; same reason. |
npm run build |
⏭️ SKIPPED | Docs-only diff; nothing under packages/ changed, so the build output is byte-identical. |
npm run test:package |
⏭️ SKIPPED | Docs-only diff; the packaging surface is untouched. |
In place of the gate, the spec was grounded against the codebase: every file, symbol, endpoint, test, and convention it names was checked against the worktree at the PR head. That grounding pass is what produced the blocker and majors below.
Merge state: the head merges cleanly into main (mergeable: MERGEABLE) — there is no conflict to resolve. mergeStateStatus reads BLOCKED only because a review was outstanding and the CLA check has not settled.
Findings
⛔ Blocker
.ai/specs/2026-08-09-issue-linked-pr-chip.md:34 and :159 — the client layer is specified against the wrong package, and the type-export step is a no-op.
The architecture table's Client row and implementation-plan step 4 both say to add getGithubIssuePrs(numbers) to packages/api-client/src/client.ts and export the new types from that package's index.ts. Neither instruction matches the repository:
packages/api-client/src/client.tsis 108 lines containing exactly one thing —createCezarClient<T>(), the generic Honohcfactory. It has zero per-endpoint functions; a grep forgithubin the wholepackages/api-client/srctree returns one comment. The package's own doc comment explains why: it "must stay installable, and usable at runtime, without the server package present," so endpoint helpers deliberately do not live there.- The function this slice mirrors,
getGithubChecks, is atpackages/web/src/api/client.ts:667. That is where the cockpit's fetchers live, that is whatpackages/web/src/api/queries.ts:20imports, and that is whatpackages/web/src/api/client.test.ts:165covers. packages/api-client/src/index.ts:27already doesexport * from '@open-mercato/cezar-contract', so the momentlinkedPrSchema/githubIssuePrsDataSchemaland inpackages/contract/src/github.tstheir types are re-exported automatically. There is nothing to add toindex.ts.
An implementing agent following this spec literally would add an endpoint-specific helper to a package that deliberately has none, break that package's no-server-dependency property, and still leave the cockpit without a fetcher. Because the spec is the input to om-auto-implement-spec, this error is inherited by the implementation rather than caught by it — which is what makes it a blocker rather than a nit.
Fix: change the Client row and step 4 to packages/web/src/api/client.ts (add getGithubIssuePrs(numbers, opts) next to getGithubChecks, going through cez.api.v1.p[':projectId']['issue-prs'].$get with unwrap(...) exactly as the sibling does), drop the index.ts export step, and note that the contract types reach the web app for free through @open-mercato/cezar-api-client's export *.
⚠️ Major
.ai/specs/2026-08-09-issue-linked-pr-chip.md:81, :92, :135 — timelineItems(first: 30, …) can silently return zero PRs on exactly the issues that need the chip most.
first: 30 takes the oldest thirty items of the filtered connection, and that connection contains CROSS_REFERENCED_EVENTs from issues as well as pull requests. The design drops non-PullRequest subjects/sources after the fetch (line 92), so issue→issue cross-references consume the 30-item budget before the filter ever runs. On a long-lived, heavily-referenced issue — the kind most likely to already have a PR — the first thirty timeline events can be entirely issue mentions, and the row renders no chip at all while a merged PR sits right there on GitHub. This is a false negative on the one question the feature exists to answer, and it fails silently: nothing distinguishes "no linked PR" from "the linked PR fell outside the window."
Q5's rationale (line 135, "an issue with >30 linked PRs is not a real case") measures the wrong quantity. The bound that matters is total connected + cross-referenced events, not linked PRs, and thirty of those is very reachable.
Fix: switch to last: 30 so the window holds the most recent references (a newly opened PR is the case that matters most), and restate Q5's rationale in terms of the real bound — total timeline events of those two types, with the shared budget called out explicitly. If you want to be thorough, note in Edge Cases that the result is best-effort under that cap rather than exhaustive.
.ai/specs/2026-08-09-issue-linked-pr-chip.md:100 — the chip as specified nests an anchor inside an anchor.
GithubRow renders the entire row — icon, title, and the whole pl-[22px] meta line — inside a single react-router <Link> (packages/web/src/routes/github/github.tsx:513-556). The spec puts the chip on that meta line as "a real link nested in the row Link". An <a> inside an <a> is invalid HTML: the a element's content model excludes interactive content, and nested interactive controls are a WCAG 4.1.2 problem — screen-reader and keyboard behaviour for the inner control is undefined, which undercuts the careful aria-label work in the same bullet. The stopPropagation the spec calls for solves the router double-navigation, which is a different problem; it does not make the nesting valid. The app is a Vite SPA with no SSR, so React's DOM API will happily produce the nested pair rather than the HTML parser rejecting it — meaning this ships broken-but-working and nobody notices until an accessibility pass.
There is no precedent for this in the repo: the closest thing (hand-to-agent.tsx:485) is a <button> inside a cmdk CommandItem div, not inside an anchor.
Fix: pick a structure that keeps one interactive element per region. The cleanest is to make the <li> the positioning context, render the row <Link> as a stretched-link overlay (absolute inset-0) behind the content, and let the chips be ordinary sibling links above it — that also removes the need for stopPropagation entirely. The lighter alternative is to move the chip out of the <Link> into a sibling flex cell on the meta row. Either way, say which one the spec chooses.
.ai/specs/2026-08-09-issue-linked-pr-chip.md:32 and :157 — no CEZ_DRY_RUN=1 mock path, in a codebase where every forge fetcher has one.
packages/cezar/src/server/forge/github.ts contains fourteen CEZ_DRY_RUN references, and every route-facing fetcher opens with the same guard — fetchGithubChecks begins if (process.env.CEZ_DRY_RUN === '1') return mockGithubChecks(numbers) at line 1087, with mockGithubChecks reading straight from the mock catalog so the offline demo shows glyphs. This is not decoration: AGENTS.md treats dry-run as a standing requirement for the runner surface, CEZ_DRY_RUN=1 npm run dev is the documented offline manual-verification path, and the entire e2e suite boots under it (packages/web/e2e/agent-browser.ts:54 sets CEZ_DRY_RUN: '1' for every browser fixture, and packages/web/e2e/github.e2e.ts exercises the GitHub tab through it).
Without a mockGithubIssuePrs, the new endpoint shells out to real gh under dry-run, the chip never appears in the offline demo, and — the part that actually bites — the feature cannot be given an e2e test at all, because there is no way to make a chip render in the e2e environment.
Fix: add the dry-run branch to the forge row of the architecture table and to implementation-plan step 2, mirroring mockGithubChecks: synthesise links for a couple of mock issues from the existing mock catalog so the offline demo and the e2e fixture both show chips. Then step 6's test note can reasonably gain an e2e assertion alongside the component test.
.ai/specs/2026-08-09-issue-linked-pr-chip.md:156 and :158 — the test plan names contract tests that do not exist and omits the parity guard that does.
Step 1 says to add "a github.contract.test case parsing a happy and a degraded payload (mirror the existing github contract tests)." There are no tests in packages/contract at all — find packages/contract -name '*.test.*' is empty — so there is nothing to mirror and no file by that name to extend. The only place githubChecksDataSchema is exercised is packages/cezar/src/server/contract-parity.github.test.ts, a compile-time guard that asserts each contract schema is mutually assignable with the route's InferResponseType — "no wider, no narrower," in its own words, enforced by npm run typecheck.
That guard is precisely what a new route+schema pair must be added to, and the spec never mentions it. Step 3 names route-parity and bc-route-inventory (both correct and both real) but stops short of the one guard that would catch the schema and the handler drifting apart.
Fix: replace step 1's test note with "extend packages/cezar/src/server/contract-parity.github.test.ts with a GithubIssuePrs200 mutual-assignability assertion, alongside the existing GithubChecks200 one," and drop the reference to nonexistent contract tests. If you want runtime parse coverage of the happy and degraded payloads, put it in the route test that step 3 already calls for (github-checks-api.test.ts is the pattern to copy).
🔹 Minor
:78-90 — the GraphQL snippet is not a valid document. It shows a bare issue(number: N) selection, but a query with repeated issue fields at the same level is invalid GraphQL — two selections of the same field name need distinct response keys. The prose one line above says "via node aliases (batched like prChecksQuery)," which is right, and prChecksQuery (forge/github.ts:1000) shows the real shape (p${i}: pullRequest(number: ${n})). Write the snippet with the alias (i0: issue(number: N) { … }) so nobody copies an unrunnable document into the forge.
:32 and :116 — resetGithubIssuePrsCache() breaks the file's naming convention and cannot do what Edge Cases implies. The three existing cache hooks in forge/github.ts are __clearChecksCacheForTests(), __clearCommentsCacheForTests(), and __clearRepoHandleCacheForTests() — the __/ForTests shape is deliberate, marking them as test-only seams. More importantly, line 116 lists the reset alongside the header refresh as if it bounds staleness for users, but it cannot: /github/checks takes no refresh parameter (server.ts:4688 validates only prs), so the header refresh re-runs the client query and is handed the same ≤60 s server cache. For this feature that matters more than it does for checks glyphs — the flagship scenario is "an agent just opened a PR for this issue," and the chip will not appear for up to a minute even if the user hits refresh. Either rename to __clearIssuePrsCacheForTests() and state the ≤60 s delay plainly as accepted, or add a refresh=1 parameter the way /github/comments/:kind/:number does.
:92 — DISCONNECTED_EVENT is not considered, so an unlinked PR keeps its chip forever. Removing a linked PR from an issue appends a DisconnectedEvent; the original ConnectedEvent stays in the timeline. Since the design reads only CONNECTED_EVENT and CROSS_REFERENCED_EVENT, a deliberately unlinked PR renders indefinitely. Either fetch DISCONNECTED_EVENT too and subtract those PR numbers, or say in Edge Cases that this is accepted (it is arguably fine — the PR is still real and still related) so an implementer does not have to guess.
:51 — isDraft is in the contract but nothing consumes it. linkedPrSchema declares isDraft: z.boolean().optional(), and the UI/UX section (:96-107) never mentions draft state; the tint vocabulary is open/merged/closed only. Because contract-parity.github.test.ts compares schema and route in both directions, an optional field still has to be reconciled with what the handler actually sends. Either give it a rendering rule (a draft PR is a materially weaker "someone is on this" signal and arguably deserves a distinct tint) or drop it until Phase 2 needs it.
💅 Nit
Risk signal disagrees with itself across the three artifacts. The brief says "Risk signal: medium" (briefs/2026-08-07-issue-linked-pr-chip.md:6, citing the per-issue GraphQL hydration path and the §2 contract change); the spec's Risks & Impact Review argues additive/low; the label-rationale comment on this PR recommends risk-low. risk-low is the right call for this PR, which ships a document — but the implementation PR will add a route, a contract schema, and a GraphQL path, and risk-medium is the honest rating there. Say which one applies to which PR so the implementing run does not have to guess.
:32 — "reuses resolveRepoHandle" is a deviation from the sibling, not a reuse of it. fetchGithubChecks does not use resolveRepoHandle; it inlines gh repo view --json nameWithOwner (forge/github.ts:1099). Using the cached helper instead is better — it saves a subprocess per window — but the spec presents it as mirroring the sibling when it actually diverges. Worth one clause so the difference is intentional rather than accidental.
💥 Breaking Changes
This PR changes no contract surface — it adds five files under .ai/specs/. The checklist below is therefore about what the design commits to, which is what a specification review can meaningfully gate:
- No exported/public symbol removed or renamed without a deprecation path — the design is purely additive (
linkedPrSchema,githubIssuePrsDataSchema,fetchGithubIssuePrs,getGithubIssuePrs,useGithubIssuePrsare all new). - No function signature changed in a breaking way — no existing signature is touched.
- No required type field removed or narrowed —
githubItemSchema, the §2-protected list shape, is explicitly left alone (:31,:121). - No HTTP route URL removed or renamed; no method changed —
GET /api/v1/github/issue-prsis a new path in an existing family. - No field removed or retyped in an existing response shape — the one-shot list payload is a stated non-goal (
:145). - No event or message name renamed or removed — none involved.
- No CLI command or flag renamed or removed — none involved.
- No database table or column renamed or removed — the design has no persistence (
:41). - No config key renamed and no default changed silently — none involved.
- Where a contract had to change, the old surface keeps working — the spec correctly identifies the one hard requirement, registering the new route in
BACKWARD_COMPATIBILITY.md§2 sobc-route-inventory.test.tsandroute-parity.test.tsstay green (:121,:158). I verified both tests exist and that §2 line 53 is the inventory line the new route belongs on. This part is right.
The gap worth naming is not a break but an omission: contract-parity.github.test.ts is the third guard in that family and the spec does not mention it (see the fourth major above). Nothing breaks if it is skipped, but the new schema then has no guard keeping it exact against its handler, which is the whole reason that file exists.
🧪 Test Coverage
There is nothing to test in this diff — it is a document plus assets. What I can review is the spec's own test plan, and it is above average: every one of the six implementation steps carries a *Test* line, they name real sibling patterns to mirror (fetchGithubChecks's injected-runGraphql unit tests, github.test.tsx component tests), and step 6's assertions are specific enough to be actionable (chip text, tint, href/route target, stopPropagation, and the empty/unavailable cases).
Three gaps, all already stated as findings above and repeated here so the coverage picture is in one place:
- Step 1 targets a test file that does not exist, and the real guard —
contract-parity.github.test.ts— is never named. This is the fourth major. - No e2e coverage is planned, and as specified none is possible, because the e2e suite runs under
CEZ_DRY_RUN=1and the design has no dry-run mock. Fixing the dry-run major unblocks an e2e assertion inpackages/web/e2e/github.e2e.ts, which is where a user-visible row affordance like this really wants a test. - The window-boundary case is untested. The
first: 30problem in the second major is exactly the kind of thing a unit test with an injectedrunGraphqlcatches for free: feed it a timeline whose first thirty items are issue cross-references followed by a PR, and assert the PR is still found. Worth adding to step 2's test note once thelast: 30fix lands.
📋 Inherited reviewer feedback
None. The five existing comments on this PR are the CLA-assistant bot's status badge and four om-auto-write-spec run artifacts (visual evidence, the open-questions table, the label-rationale request, and the run summary) — this collection's own markers, filtered out per the inherited-feedback rules. There were no review bodies and no inline diff comments from any other actor, so there is no outstanding ask from a previous reviewer to carry forward or account for.
⏱️ CI status
One required check was still pending at the time of this review, and it is not waiting on code. license/cla is PENDING because the CLA-assistant has not recorded a signature for @sheeerth on this PR — it settles when the CLA is signed, not when a build finishes. main is not branch-protected, so per the tracker contract every reported check counts as required; this one is the only check the PR has. Nothing was failing at review time, so no CI failure contributed to this verdict. @sheeerth, please sign via the link in the CLA-assistant comment above — it is a hard gate on merge regardless of how the code review lands.
🔁 What happens next
Everything above is a document edit; none of it requires implementation work or changes the design's direction. Once the blocker's file paths are corrected and the four majors are addressed in the spec text, this is an approve — the slice is well-chosen, the alternatives analysis is honest and evidence-backed, and the compatibility reasoning is sound.
pat-lewczuk
added
changes-requested
|
🤖 The specification review requested changes, so the pipeline label moves out of the review state and the supporting labels are set to match what this PR actually is: a design document with no runtime surface.
The earlier |
|
Thanks @sheeerth — review found actionable items, so I'm handing this PR back to you for the next pass. When the updates are pushed, re-request review and the automation can pick it up from the latest head. |
|
🤖 This ran as a specification review (every changed file is a spec, brief, or mockup asset under autofix: skipped (not my PR — re-run with CI: the only check on this PR is |
…ted anchor, dry-run mock, contract-parity test Addresses pat-lewczuk's specification review on open-mercato#816: - blocker: client fetcher moves to packages/web/src/api/client.ts (api-client holds no per-endpoint fetchers; contract types re-export for free) - major: timelineItems last:30 (+DisconnectedEvent) so a fresh PR is never outside the window - major: stretched-link overlay instead of an <a> nested in the row <a> (WCAG 4.1.2) - major: CEZ_DRY_RUN=1 mockGithubIssuePrs so the offline demo + e2e show chips - major: extend contract-parity.github.test.ts (GithubIssuePrs200); drop nonexistent github.contract.test - minors/nits: aliased GraphQL doc, __clearIssuePrsCacheForTests + refresh=1 param, DISCONNECTED handling, isDraft rendering rule, per-artifact risk rating, resolveRepoHandle divergence noted
…ted anchor, dry-run mock, contract-parity test Addresses pat-lewczuk's specification review on open-mercato#816: - blocker: client fetcher moves to packages/web/src/api/client.ts (api-client holds no per-endpoint fetchers; contract types re-export for free) - major: timelineItems last:30 (+DisconnectedEvent) so a fresh PR is never outside the window - major: stretched-link overlay instead of an <a> nested in the row <a> (WCAG 4.1.2) - major: CEZ_DRY_RUN=1 mockGithubIssuePrs so the offline demo + e2e show chips - major: extend contract-parity.github.test.ts (GithubIssuePrs200); drop nonexistent github.contract.test - minors/nits: aliased GraphQL doc, __clearIssuePrsCacheForTests + refresh=1 param, DISCONNECTED handling, isDraft rendering rule, per-artifact risk rating, resolveRepoHandle divergence noted
|
🤖 Thanks @pat-lewczuk — grounded, correct findings. All are document fixes; pushed in ⛔ Blocker — client layer package
🔹 Minors / 💅 nits
Re-requesting review. Two gates remain outside this automation's reach on a fork: the CLA ( |
ccb70a9 to
b7768c2
Compare
…n-mercato#816) Steps 1.1 and 1.2 of the spec's implementation plan. - `linkedPrSchema` + `githubIssuePrsDataSchema` in the contract, with the `GithubIssuePrs200` mutual-assignability assertion in contract-parity. - `fetchGithubIssuePrs` / `fetchIssuePrLinks` in the forge: one aliased GraphQL query per window over `timelineItems(last: 30)`, deduped by PR number, disconnections subtracted, ordered open -> merged -> closed; a bounded 60s per-issue cache with a `refresh` bypass; a CEZ_DRY_RUN mock so the offline demo and the e2e suite paint chips.
…ato#816) Step 1.3. A chained `.get` link in the githubRoutes family (a loose `app.get` would vanish from AppType), same CSV validator shape and 400 wording as /github/checks, plus the optional refresh=1 flag. Inventoried in BACKWARD_COMPATIBILITY.md §2 and covered by a route test mirroring github-checks-api.test.ts.
…rs (open-mercato#816) Steps 1.4 and 1.5. `getGithubIssuePrs` beside `getGithubChecks` (a `refresh: false` sends nothing, as everywhere else on this client), plus `queryKeys.githubIssuePrs` and `useGithubIssuePrs` mirroring `useGithubChecks`. packages/api-client is untouched — its `export * from '@open-mercato/cezar-contract'` already re-exports the new types.
…rcato#816) Step 1.6. Derives the on-screen issue window (pinning the URL-selected issue), wires useGithubIssuePrs for the Issues view, and paints a state-tinted 'PR #n' chip per link with a muted draft variant, an in-cockpit target when the PR is in the loaded open set, and a +N overflow past three. The row is restructured to a stretched link to make that possible: an <a> inside the row <Link> is invalid HTML and a WCAG 4.1.2 failure that this SSR-less SPA would ship silently, so the <li> becomes the positioning context, the row link an absolute inset-0 overlay carrying the navigation, drag payload and accessible name, and the chips are its siblings — no nested anchors, no stopPropagation. The header refresh re-fetches the window with refresh=1 so a just-opened PR is not hidden by the 60s cache.
… complete (open-mercato#816) Steps 1.7 and 1.8. The e2e case drives the real cockpit under CEZ_DRY_RUN=1 — which the step-2 mock catalog now serves links for — and screenshots the chipped row. Full gate green: typecheck, npm test (6091), test:unit (36), build + check:pack, test:package (15).
…-mercato#816) Review follow-up. The old comment justified first-writer-wins by where `last:` puts the newest events, which reads backwards. The real reason order does not matter is that GraphQL resolves state/isDraft off the pull request now, not as of the event, so every occurrence carries identical fields.
|
🤖 The spec this PR landed is now implemented on its own PR, keeping this one design-only as intended: #885 — feat(github): linked-PR chips on the Issues list. That PR carries |
…n-mercato#816) Steps 1.1 and 1.2 of the spec's implementation plan. - `linkedPrSchema` + `githubIssuePrsDataSchema` in the contract, with the `GithubIssuePrs200` mutual-assignability assertion in contract-parity. - `fetchGithubIssuePrs` / `fetchIssuePrLinks` in the forge: one aliased GraphQL query per window over `timelineItems(last: 30)`, deduped by PR number, disconnections subtracted, ordered open -> merged -> closed; a bounded 60s per-issue cache with a `refresh` bypass; a CEZ_DRY_RUN mock so the offline demo and the e2e suite paint chips.
…ato#816) Step 1.3. A chained `.get` link in the githubRoutes family (a loose `app.get` would vanish from AppType), same CSV validator shape and 400 wording as /github/checks, plus the optional refresh=1 flag. Inventoried in BACKWARD_COMPATIBILITY.md §2 and covered by a route test mirroring github-checks-api.test.ts.
…rs (open-mercato#816) Steps 1.4 and 1.5. `getGithubIssuePrs` beside `getGithubChecks` (a `refresh: false` sends nothing, as everywhere else on this client), plus `queryKeys.githubIssuePrs` and `useGithubIssuePrs` mirroring `useGithubChecks`. packages/api-client is untouched — its `export * from '@open-mercato/cezar-contract'` already re-exports the new types.
…rcato#816) Step 1.6. Derives the on-screen issue window (pinning the URL-selected issue), wires useGithubIssuePrs for the Issues view, and paints a state-tinted 'PR #n' chip per link with a muted draft variant, an in-cockpit target when the PR is in the loaded open set, and a +N overflow past three. The row is restructured to a stretched link to make that possible: an <a> inside the row <Link> is invalid HTML and a WCAG 4.1.2 failure that this SSR-less SPA would ship silently, so the <li> becomes the positioning context, the row link an absolute inset-0 overlay carrying the navigation, drag payload and accessible name, and the chips are its siblings — no nested anchors, no stopPropagation. The header refresh re-fetches the window with refresh=1 so a just-opened PR is not hidden by the 60s cache.
… complete (open-mercato#816) Steps 1.7 and 1.8. The e2e case drives the real cockpit under CEZ_DRY_RUN=1 — which the step-2 mock catalog now serves links for — and screenshots the chipped row. Full gate green: typecheck, npm test (6091), test:unit (36), build + check:pack, test:package (15).
…-mercato#816) Review follow-up. The old comment justified first-writer-wins by where `last:` puts the newest events, which reads backwards. The real reason order does not matter is that GraphQL resolves state/isDraft off the pull request now, not as of the event, so every occurrence carries identical fields.
open-mercato#816) The authorship rewrite (wrong author email — the commits were being attributed to another GitHub account, which is what held license/cla at not-signed) changed every sha on this branch, so the plan's Progress references pointed at commits that no longer exist. Also adds the `PR: open-mercato#885` back-reference om-auto-continue-pr resumes from.
…mercato#816) Two correctness defects found by a second review pass, both confirmed against live GitHub rather than reasoned about. 1. `issue(number:)` does not answer null for a number that is not an issue — GitHub returns a NOT_FOUND entry in an `errors` array, and `gh api graphql` exits non-zero the moment that array is present, even when `data` holds a good answer for every other alias. So one bad number in the window discarded the whole batch and every row lost its chips. Reachable in one click: the window pinned the URL-selected number, so `/github/issues/<a PR number>` poisoned the list. This is the trap `refStatusQuery` already documents; take the same cure — `issueOrPullRequest(number:)` with the timeline selected `... on Issue`, so a PR number resolves to a bare PullRequest and is skipped. The window also no longer pins a `:n` the list does not hold, which removes the nonexistent-number vector at source and hydrates nothing that has no row anyway. 2. A failed query was reported as success. `fetchIssuePrLinks` swallowed chunk failures, so `fetchGithubIssuePrs` could not tell "we could not ask" from "there is nothing here" and answered `{available: true, links: {}}` — the shape that asserts the forge was asked and had nothing — then cached that emptiness for 60s. It now returns the failed numbers and a reason (mirroring `fetchRefStatuses`), caches nothing for them, and answers `{available: false, reason}` when the window could not be answered at all. Also drops an unreachable `/ENOENT/` branch whose helpful "install gh" hint could never fire, because `resolveRepoHandle` swallows the cause first; the handle failure now names the possibilities instead. Tests: the failure semantics of the real transport were never exercised — every case injected `runGraphql`, whose failure modes are not `gh`'s. Adds the PR-number skip, the union assertion, failed-chunk reporting, available:false on total failure, no-cache-on-failure, and the pin guard. All verified red without the fix (git stash push -- <source>), green with.
3 participants
Source doc: .ai/specs/2026-08-09-issue-linked-pr-chip.md
Status: complete
🎯 Goal
↗ PR #123chip on each row of the GitHub Issues list when a pull request is linked to that issue, tinted by PR state (open / merged / closed), so a triager can see an issue already has a PR before dispatching an agent.What Changed
.ai/specs/2026-08-09-issue-linked-pr-chip.md.ai/specs/briefs/2026-08-07-issue-linked-pr-chip.md📐 Design in one line
GET /api/v1/github/issue-prshydrates issue→linked-PR data for on-screen rows only via GitHubtimelineItems(CONNECTED + CROSS_REFERENCED); the fast one-shot list fetch and every §2-protected shape are untouched.💥 Breaking Changes