From 666193454f9b541d782d4e6073d73f16e264dbe8 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Tue, 28 Jul 2026 18:44:06 -0400 Subject: [PATCH] feat: a home on the board for the one human-only act in the model (SYD-290) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Declared issue<->PR links had no UI. Seeing one, confirming an agent's declaration, or revoking a wrong one all required a terminal, a checkout and a token — so the act the design most wants a person to make deliberately was the least reachable one, while every act it wants gated was a click away. Adds a PR links panel to the issue page and the review pane: - lists live links with role, provenance (who declared, who confirmed), and what GitHub was last seen doing to the PR; - Confirm on an unconfirmed `delivers` link, human-only and HIDDEN rather than left to 400 — an agent should never be shown an act it can never perform; - Revoke with its required reason; - Declare, defaulting to the PR numbers the issue's own timeline already names (design §8's "did you mean?"); - `references` suggestions rendered subordinate, with promotion instead of Confirm — confirming a suggestion is refused server-side, because it would prove nothing. The panel shows BOTH halves of the join, which is the part that isn't cosmetic. `pr_state` only started covering every PR in a bound repo at SYD-287, so PRs merged before that (this repo has a run of them, #121-#155) have no observation at all. Declaring and confirming a link to one is a perfectly valid statement that still leaves `done_without_merged_pr` lit. Showing only the declaration would put "confirmed ✓" beside a lit warning and read as a bug in the banner rather than the missing half it actually is — so `listLiveLinkViews` returns `observed` and a `provesLanded` verdict, and the panel says plainly when a click cannot help and points at "Mark resolved…". Confirmation is deliberately NOT folded into Approve. The review pane shows the panel next to the verdict buttons so the reviewer is already looking at it, but vouching stays its own click: making it a side effect of a different verdict would mean a human vouches for a link without ever choosing to. Adds a "🔗 unlinked" done-column filter, because clearing the ~46 flagged issues is per-issue by design and finding them first is what makes that a queue rather than an archaeology exercise. Server side is additive: `GET /api/issues/:ref` returns the link view instead of the bare rows. No schema, no new endpoints. §13.2 (should confirmation carry the head SHA it saw) is deliberately left to SYD-282, which re-derives authorization on top of SYD-280/281 and is where the CAS belongs. --- src/rest/api-routes.ts | 14 +- src/services/pr-links.ts | 103 +++++++++- tests/services/pr-links.test.ts | 136 ++++++++++++ ui/src/App.tsx | 4 +- ui/src/IssuePopover.test.tsx | 2 + ui/src/PrLinks.test.tsx | 236 +++++++++++++++++++++ ui/src/PrLinks.tsx | 329 ++++++++++++++++++++++++++++++ ui/src/api.ts | 37 +++- ui/src/styles.css | 94 +++++++++ ui/src/types.ts | 60 +++++- ui/src/views/Board.test.tsx | 1 + ui/src/views/Board.tsx | 20 +- ui/src/views/IssueDetail.test.tsx | 8 +- ui/src/views/IssueDetail.tsx | 15 +- ui/src/views/NewIssue.test.tsx | 1 + ui/src/views/Review.test.tsx | 12 +- ui/src/views/Review.tsx | 23 ++- ui/src/views/Search.test.tsx | 1 + ui/src/views/Triage.test.tsx | 1 + 19 files changed, 1076 insertions(+), 21 deletions(-) create mode 100644 ui/src/PrLinks.test.tsx create mode 100644 ui/src/PrLinks.tsx diff --git a/src/rest/api-routes.ts b/src/rest/api-routes.ts index 7e86760..7e51adf 100644 --- a/src/rest/api-routes.ts +++ b/src/rest/api-routes.ts @@ -37,7 +37,12 @@ import { } from "../services/dependencies.js"; import { addComment, getActivity } from "../services/comments.js"; import { recordDeliveryEvent } from "../services/delivery-events.js"; -import { declarePrLink, confirmPrLink, revokePrLink, listLiveLinks } from "../services/pr-links.js"; +import { + declarePrLink, + confirmPrLink, + revokePrLink, + listLiveLinkViews, +} from "../services/pr-links.js"; import { startAgentSession, endAgentSession, @@ -282,7 +287,12 @@ export function buildApiRoutes(db: Db, attachmentsDir: string = defaultAttachmen // SYD-280: additive. openPr/deliveryPin keep their exact shape, so the UI // is unaffected; this exposes the attribution behind them so a human can // see what is linked and confirm or revoke it. - prLinks: listLiveLinks(db, issue.id), + // + // SYD-290 widened it from the bare rows to the view: the panel that acts + // on these links needs actor NAMES and, crucially, whether anything has + // observed the PR — a confirmed link to an unobserved PR is a valid + // statement that still proves nothing, and the panel has to say so. + prLinks: listLiveLinkViews(db, issue.id), activity: getActivity(db, ref), dependencies: listDependencies(db, ref), attachments: listAttachments(db, ref), diff --git a/src/services/pr-links.ts b/src/services/pr-links.ts index d13e8a0..72f16d6 100644 --- a/src/services/pr-links.ts +++ b/src/services/pr-links.ts @@ -22,8 +22,9 @@ // authority infra already holds, not a widening of it. import { and, eq, isNull, sql } from "drizzle-orm"; +import { alias } from "drizzle-orm/sqlite-core"; import type { Db, DbOrTx } from "../db/index.js"; -import { prLinks, type PrLinkRole } from "../db/schema.js"; +import { actors, prLinks, prState, type PrLinkRole } from "../db/schema.js"; import type { Actor } from "./actors.js"; import type { Attribution } from "./attribution.js"; import { SwitchyardError } from "./errors.js"; @@ -48,6 +49,106 @@ export function listLiveLinks(db: DbOrTx, issueId: number): PrLink[] { .all(); } +/** What GitHub was last seen doing to a linked PR — the pr_state half of the join. */ +export type PrObservationView = { + status: "open" | "merged" | "closed"; + url: string | null; + ghUpdatedAt: number | null; +}; + +export type PrLinkView = PrLink & { + declaredByName: string; + confirmedByName: string | null; + /** Whether a HUMAN confirmed — the §5a exception turns on this, not on merely being confirmed. */ + confirmedByHuman: boolean; + /** null when nothing has ever observed this PR, which is not the same as "not merged". */ + observed: PrObservationView | null; + /** True when this link on its own would let a reader conclude the work landed. */ + provesLanded: boolean; +}; + +/** + * Does this link, joined to its observation, satisfy the proof-bearing + * predicate the readers use? The TS mirror of the SQL in attention.ts's + * unresolvedDoneWithoutMerge — kept as one exported function because the UI + * panel and the attention banner sit on the same screen, and a panel that says + * "confirmed ✓" beside a lit "done without a merged PR" warning is a second + * contradictory signal rather than an explanation (SYD-290). + * + * The three conjuncts, per design §5/§5a and the pr_links schema comment: + * role is `delivers` (a suggestion proves nothing), someone accountable + * confirmed it, and — unless that confirmer was a human — the observation must + * postdate the declaration, so a stale merge can't be retro-claimed. + */ +export function provesLanded( + link: Pick, + confirmedByHuman: boolean, + observed: PrObservationView | null, +): boolean { + if (link.role !== "delivers" || link.confirmedBy === null) return false; + if (observed?.status !== "merged") return false; + if (confirmedByHuman) return true; + return observed.ghUpdatedAt !== null && observed.ghUpdatedAt >= link.declaredAt; +} + +/** + * Every live link on an issue with the two things a human needs in order to + * act on it: WHO said what (declarer/confirmer names, so the panel can show + * "declared by claude/dev, unconfirmed" rather than an actor id), and WHETHER + * ANYTHING OBSERVED THE PR. + * + * The observation half matters more than it looks. `pr_state` only started + * covering every PR in a bound repo at SYD-287; PRs merged before that — this + * repo has a run of them, #121-#155 — have no row at all. Declaring and + * confirming a link to one of those is a perfectly valid statement that still + * leaves `done_without_merged_pr` lit, because the join has no observation + * half. Surfacing `observed: null` is what stops the panel from implying a + * click will clear a flag that it cannot. + */ +export function listLiveLinkViews(db: DbOrTx, issueId: number): PrLinkView[] { + const declarer = alias(actors, "declarer"); + const confirmer = alias(actors, "confirmer"); + const rows = db + .select({ + link: prLinks, + declaredByName: declarer.name, + confirmedByName: confirmer.name, + confirmerType: confirmer.type, + status: prState.status, + url: prState.url, + ghUpdatedAt: prState.ghUpdatedAt, + }) + .from(prLinks) + .innerJoin(declarer, eq(declarer.id, prLinks.declaredBy)) + .leftJoin(confirmer, eq(confirmer.id, prLinks.confirmedBy)) + // pr_state is keyed (repo, prNumber) with repo stored as written, so match + // case-insensitively the way every other reader of this join does. + .leftJoin( + prState, + and( + sql`lower(${prState.repo}) = lower(${prLinks.repo})`, + eq(prState.prNumber, prLinks.prNumber), + ), + ) + .where(and(eq(prLinks.issueId, issueId), isNull(prLinks.revokedAt))) + .orderBy(sql`${prLinks.declaredAt} DESC, ${prLinks.id} DESC`) + .all(); + + return rows.map((r) => { + const observed: PrObservationView | null = + r.status === null ? null : { status: r.status, url: r.url, ghUpdatedAt: r.ghUpdatedAt }; + const confirmedByHuman = r.confirmerType === "human"; + return { + ...r.link, + declaredByName: r.declaredByName, + confirmedByName: r.confirmedByName, + confirmedByHuman, + observed, + provesLanded: provesLanded(r.link, confirmedByHuman, observed), + }; + }); +} + /** * The issues holding a live `delivers` link to a PR — the (repo, prNumber) * direction of the same predicate pr-status.ts reads issue-first as diff --git a/tests/services/pr-links.test.ts b/tests/services/pr-links.test.ts index b0f28eb..b531882 100644 --- a/tests/services/pr-links.test.ts +++ b/tests/services/pr-links.test.ts @@ -23,9 +23,12 @@ import { confirmPrLink, revokePrLink, listLiveLinks, + listLiveLinkViews, recordIngestedPrLink, backfillPrLinksFromPrState, } from "../../src/services/pr-links.js"; +import { getAttention } from "../../src/services/attention.js"; +import type { Actor } from "../../src/services/actors.js"; const REPO = "acme/widgets"; const OTHER_REPO = "acme/unrelated"; @@ -447,3 +450,136 @@ describe("the DoS the previous design died on", () => { expect(kinds).toContain("gh_pr_opened"); }); }); + +// SYD-290. The links panel and the attention banner render on the same screen, +// so the view backing the panel has to reach the same verdict the banner's SQL +// does. Every test here pins one of the two against the other rather than +// asserting the view's output in isolation. +describe("listLiveLinkViews — what the panel shows a human", () => { + /** Drives the real webhook so the observation comes from ingestion, not an insert. */ + function observeMerge(db: ReturnType, prNumber: number, updatedAt: string) { + handleGithubWebhook(db, "pull_request", { + action: "closed", + repository: { full_name: REPO }, + pull_request: { + number: prNumber, + merged: true, + merge_commit_sha: "d".repeat(40), + html_url: `https://github.com/${REPO}/pull/${prNumber}`, + head: { ref: "feat/some-topic", sha: "c".repeat(40) }, + title: "some interactive work", + updated_at: updatedAt, + }, + }); + } + + /** Walks SYD-1 to `done` with no PR of any kind, arming done_without_merged_pr. */ + function stampDone(db: ReturnType, human: Actor, agent: Actor) { + claimIssue(db, agent, "SYD-1"); + updateIssue(db, human, "SYD-1", { status: "in_review" }); + updateIssue(db, human, "SYD-1", { status: "done" }); + } + + it("names the declarer and confirmer instead of leaving the panel with actor ids", () => { + const { db, human, agent } = setup(); + const lease = claim(db, agent); + declarePrLink(db, agent, "SYD-1", { repo: REPO, prNumber: 7 }, lease); + + const [before] = listLiveLinkViews(db, 1); + expect(before.declaredByName).toBe("claude/worker"); + expect(before.confirmedByName).toBeNull(); + expect(before.confirmedByHuman).toBe(false); + + confirmPrLink(db, human, "SYD-1", { repo: REPO, prNumber: 7 }); + const [after] = listLiveLinkViews(db, 1); + expect(after.confirmedByName).toBe("sean"); + expect(after.confirmedByHuman).toBe(true); + }); + + // The trap this field exists to close. A PR merged before SYD-287 widened + // ingestion has no pr_state row, so declaring AND confirming a link to it is + // a valid statement that still proves nothing — the join has no observation + // half. Without `observed`, the panel would show "confirmed ✓" beside a lit + // warning and look like a bug in the banner. + it("reports observed: null for a PR nothing ever observed, and agrees the flag stays lit", () => { + const { db, human, agent } = setup(); + stampDone(db, human, agent); + expect(getAttention(db, 1)?.reason).toBe("done_without_merged_pr"); + + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 128 }); + + const [view] = listLiveLinkViews(db, 1); + expect(view.role).toBe("delivers"); + expect(view.confirmedByName).toBe("sean"); // a human declaration auto-confirms + expect(view.observed).toBeNull(); + expect(view.provesLanded).toBe(false); + // The banner reaches the same conclusion — the panel is explaining it, not + // contradicting it. + expect(getAttention(db, 1)?.reason).toBe("done_without_merged_pr"); + }); + + it("reports the merge and proves landing once both halves exist, clearing the flag", () => { + const { db, human, agent } = setup(); + stampDone(db, human, agent); + observeMerge(db, 42, "2026-07-12T11:00:00Z"); + expect(getAttention(db, 1)?.reason).toBe("done_without_merged_pr"); + + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 42 }); + + const [view] = listLiveLinkViews(db, 1); + expect(view.observed?.status).toBe("merged"); + expect(view.provesLanded).toBe(true); + expect(getAttention(db, 1)).toBeNull(); + }); + + // §5a. A non-human confirmer buys no exemption from recency, so a merge that + // predates the declaration can't be retro-claimed by infra. + it("withholds proof from a service-confirmed link whose merge predates the declaration", () => { + const { db, human, agent, infra } = setup(); + stampDone(db, human, agent); + observeMerge(db, 43, "2026-07-12T11:00:00Z"); + + declarePrLink(db, infra, "SYD-1", { repo: REPO, prNumber: 43 }); + + const [view] = listLiveLinkViews(db, 1); + expect(view.confirmedByName).toBe("deliver"); + expect(view.confirmedByHuman).toBe(false); + expect(view.observed?.status).toBe("merged"); + expect(view.provesLanded).toBe(false); + expect(getAttention(db, 1)?.reason).toBe("done_without_merged_pr"); + }); + + // A suggestion from PR prose is not a claim about what carries the work, so + // it can never prove landing however merged the PR is. + it("never proves landing from a references suggestion", () => { + const { db, human, agent } = setup(); + stampDone(db, human, agent); + handleGithubWebhook(db, "pull_request", { + action: "closed", + repository: { full_name: REPO }, + pull_request: { + number: 44, + merged: true, + merge_commit_sha: "e".repeat(40), + html_url: `https://github.com/${REPO}/pull/44`, + head: { ref: "attacker/whatever" }, + title: "unrelated work that mentions SYD-1", + updated_at: "2026-07-12T11:00:00Z", + }, + }); + + const [view] = listLiveLinkViews(db, 1); + expect(view.role).toBe("references"); + expect(view.observed?.status).toBe("merged"); + expect(view.provesLanded).toBe(false); + expect(getAttention(db, 1)?.reason).toBe("done_without_merged_pr"); + }); + + it("drops a revoked link from the panel entirely", () => { + const { db, human } = setup(); + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 7 }); + expect(listLiveLinkViews(db, 1)).toHaveLength(1); + revokePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 7, reason: "wrong PR" }); + expect(listLiveLinkViews(db, 1)).toHaveLength(0); + }); +}); diff --git a/ui/src/App.tsx b/ui/src/App.tsx index 3542caf..d169a4a 100644 --- a/ui/src/App.tsx +++ b/ui/src/App.tsx @@ -103,9 +103,9 @@ function ShellRouter({ me }: { me: Actor }) { {route.view === "triage" && } {route.view === "board" && } - {route.view === "issue" && } + {route.view === "issue" && } {route.view === "review" && ( - + )} {route.view === "new-issue" && } {route.view === "search" && ( diff --git a/ui/src/IssuePopover.test.tsx b/ui/src/IssuePopover.test.tsx index d712b5d..7f528d4 100644 --- a/ui/src/IssuePopover.test.tsx +++ b/ui/src/IssuePopover.test.tsx @@ -22,6 +22,7 @@ import type { IssueDetail } from "./types"; const ISSUE: IssueDetail = { id: 1, + projectId: 1, ref: "SYD-83", title: "Rich internal refs", description: "", @@ -50,6 +51,7 @@ const ISSUE: IssueDetail = { parentRef: null, attachments: [], deliveryPin: null, + prLinks: [], }; async function renderMarkdown(text: string): Promise { diff --git a/ui/src/PrLinks.test.tsx b/ui/src/PrLinks.test.tsx new file mode 100644 index 0000000..c5c3a6c --- /dev/null +++ b/ui/src/PrLinks.test.tsx @@ -0,0 +1,236 @@ +// @vitest-environment jsdom +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { act } from "react"; +import { createRoot } from "react-dom/client"; + +vi.mock("./api", () => ({ + listGithubRepos: vi.fn(() => Promise.resolve([])), + declarePrLink: vi.fn(() => Promise.resolve({})), + confirmPrLink: vi.fn(() => Promise.resolve({})), + revokePrLink: vi.fn(() => Promise.resolve({})), +})); + +import { confirmPrLink, declarePrLink, listGithubRepos } from "./api"; +import PrLinks, { linkState, prUrl, repoOptions, suggestedPrNumbers } from "./PrLinks"; +import type { Activity, Actor, GithubRepoView, PrLinkView } from "./types"; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +const HUMAN: Actor = { id: 1, name: "sean", type: "human" }; +const AGENT: Actor = { id: 2, name: "claude/dev", type: "agent" }; + +function link(o: Partial = {}): PrLinkView { + return { + id: 1, + issueId: 1, + repo: "mobilitylabs/switchyard", + prNumber: 226, + role: "delivers", + declaredBy: 2, + declaredByName: "claude/dev", + declaredAt: 1_785_000_000, + confirmedBy: null, + confirmedByName: null, + confirmedByHuman: false, + confirmedAt: null, + revokedAt: null, + observed: { status: "merged", url: null, ghUpdatedAt: 1_785_000_100 }, + provesLanded: false, + ...o, + }; +} + +function ev(type: string, prNumber: number, createdAt = 0): Activity { + return { type, actorName: "github", viaAgentName: null, payload: { prNumber }, createdAt }; +} + +async function render(props: { + links: PrLinkView[]; + me: Actor; + activity?: Activity[]; +}): Promise { + const container = document.createElement("div"); + document.body.appendChild(container); + const root = createRoot(container); + await act(async () => { + root.render( + {}} + />, + ); + }); + await act(async () => {}); // flush the listGithubRepos poll + return container; +} + +function buttonByText(root: HTMLElement, text: string): HTMLButtonElement | undefined { + return [...root.querySelectorAll("button")].find((b) => b.textContent?.trim() === text) as + HTMLButtonElement | undefined; +} + +beforeEach(() => { + vi.mocked(listGithubRepos).mockResolvedValue([ + { id: 1, fullName: "mobilitylabs/switchyard", projectId: 1, createdAt: 0, hasSecret: true }, + ] as GithubRepoView[]); + vi.mocked(declarePrLink).mockClear(); + vi.mocked(confirmPrLink).mockClear(); +}); + +describe("linkState — the two halves read as one status", () => { + // The distinction the whole panel exists for. "Nothing ever observed this + // PR" is not "this PR did not merge", and only the second is something a + // human can fix by clicking Confirm. + it("separates never-observed from merged-but-unproven", () => { + expect(linkState(link({ observed: null })).label).toBe("⚪ never observed"); + expect(linkState(link({ provesLanded: false })).label).toBe("🔒 merged, unproven"); + expect(linkState(link({ provesLanded: true })).label).toBe("✅ merged"); + }); + + it("reports what GitHub last saw for a PR that never merged", () => { + expect( + linkState(link({ observed: { status: "open", url: null, ghUpdatedAt: null } })).label, + ).toBe("🔀 open"); + expect( + linkState(link({ observed: { status: "closed", url: null, ghUpdatedAt: null } })).label, + ).toBe("🚫 closed"); + }); + + // Three different reasons a merged PR can fail to prove landing, and the + // human needs to know which one they are looking at. + it("explains WHY a merged PR is unproven", () => { + expect(linkState(link({ role: "references" })).title).toMatch(/never proves/i); + expect(linkState(link({ confirmedBy: null })).title).toMatch(/nobody has confirmed/i); + expect(linkState(link({ confirmedBy: 3, confirmedByName: "deliver" })).title).toMatch( + /recency binding/i, + ); + }); +}); + +describe("suggestedPrNumbers — the 'did you mean?' affordance", () => { + it("offers PR numbers from the issue's own timeline, newest first", () => { + const activity = [ev("gh_pr_opened", 100, 1), ev("gh_pr_merged", 226, 2)]; + expect(suggestedPrNumbers(activity, [])).toEqual([226, 100]); + }); + + it("omits PRs already linked, so the form never suggests a no-op", () => { + const activity = [ev("gh_pr_merged", 226, 1), ev("gh_pr_opened", 100, 2)]; + expect(suggestedPrNumbers(activity, [link({ prNumber: 226 })])).toEqual([100]); + }); + + it("ignores events that name no PR", () => { + const activity: Activity[] = [ + { type: "comment", actorName: "sean", viaAgentName: null, payload: {}, createdAt: 1 }, + ev("gh_pushed", 5, 2), + ]; + expect(suggestedPrNumbers(activity, [])).toEqual([]); + }); +}); + +describe("repoOptions", () => { + it("keeps repos bound to this project and unscoped ones, drops other projects'", () => { + const repos = [ + { id: 1, fullName: "a/mine", projectId: 1, createdAt: 0, hasSecret: false }, + { id: 2, fullName: "a/theirs", projectId: 2, createdAt: 0, hasSecret: false }, + { id: 3, fullName: "a/global", projectId: null, createdAt: 0, hasSecret: false }, + ] as GithubRepoView[]; + expect(repoOptions(repos, 1)).toEqual(["a/mine", "a/global"]); + }); +}); + +describe("prUrl", () => { + it("prefers the observed URL and falls back to a constructed one", () => { + expect( + prUrl(link({ observed: { status: "open", url: "https://gh/x", ghUpdatedAt: null } })), + ).toBe("https://gh/x"); + expect(prUrl(link({ observed: null }))).toBe( + "https://github.com/mobilitylabs/switchyard/pull/226", + ); + }); +}); + +describe("who may act", () => { + // Hidden, not disabled-and-400: an agent should never be shown the one act + // the model reserves for a person. + it("hides Confirm and the declare form from a non-human", async () => { + const root = await render({ links: [link()], me: AGENT }); + expect(buttonByText(root, "Confirm")).toBeUndefined(); + expect(root.querySelector(".pr-link-declare")).toBeNull(); + }); + + it("offers Confirm to a human on an unconfirmed delivers link", async () => { + const root = await render({ links: [link()], me: HUMAN }); + const button = buttonByText(root, "Confirm"); + expect(button).toBeDefined(); + await act(async () => button!.click()); + expect(confirmPrLink).toHaveBeenCalledWith("SYD-290", { + repo: "mobilitylabs/switchyard", + prNumber: 226, + }); + }); + + it("offers no Confirm once the link is confirmed", async () => { + const root = await render({ + links: [link({ confirmedBy: 1, confirmedByName: "sean", confirmedByHuman: true })], + me: HUMAN, + }); + expect(buttonByText(root, "Confirm")).toBeUndefined(); + }); + + // confirmPrLink refuses a `references` link server-side (confirming a + // suggestion would prove nothing), so the panel must offer the verb that + // works — declaring, which supersedes the suggestion and confirms in one go. + it("offers promotion, not Confirm, on a references suggestion", async () => { + const root = await render({ links: [link({ role: "references" })], me: HUMAN }); + expect(buttonByText(root, "Confirm")).toBeUndefined(); + const promote = buttonByText(root, "This one carries the work"); + expect(promote).toBeDefined(); + await act(async () => promote!.click()); + expect(declarePrLink).toHaveBeenCalledWith("SYD-290", { + repo: "mobilitylabs/switchyard", + prNumber: 226, + role: "delivers", + }); + }); +}); + +describe("what the panel says when it cannot help", () => { + // The trap: declaring and confirming a link to a PR nothing ever observed is + // a valid statement that leaves done_without_merged_pr lit. Saying so here is + // what stops the panel contradicting the banner beside it. + it("warns that an unobserved PR can't prove landing however it is confirmed", async () => { + const root = await render({ + links: [link({ observed: null, confirmedBy: 1, confirmedByName: "sean" })], + me: HUMAN, + }); + const note = root.querySelector(".pr-link-note"); + expect(note?.textContent).toMatch(/can't prove the work landed/i); + expect(note?.textContent).toMatch(/Mark resolved/i); + }); + + it("says nothing is declared rather than showing an empty list", async () => { + const root = await render({ links: [], me: HUMAN }); + expect(root.querySelector(".empty")?.textContent).toMatch(/No PR is declared/i); + }); + + it("refuses to declare when no repo is bound, instead of posting a bad request", async () => { + vi.mocked(listGithubRepos).mockResolvedValue([]); + const root = await render({ links: [], me: HUMAN }); + const input = root.querySelector(".pr-link-input") as HTMLInputElement; + await act(async () => { + const setter = Object.getOwnPropertyDescriptor( + window.HTMLInputElement.prototype, + "value", + )!.set!; + setter.call(input, "42"); + input.dispatchEvent(new Event("input", { bubbles: true })); + }); + await act(async () => buttonByText(root, "Declare")!.click()); + expect(declarePrLink).not.toHaveBeenCalled(); + expect(root.querySelector(".error-bar")?.textContent).toMatch(/No GitHub repo is bound/i); + }); +}); diff --git a/ui/src/PrLinks.tsx b/ui/src/PrLinks.tsx new file mode 100644 index 0000000..32da588 --- /dev/null +++ b/ui/src/PrLinks.tsx @@ -0,0 +1,329 @@ +import { useMemo, useState } from "react"; +import { confirmPrLink, declarePrLink, listGithubRepos, revokePrLink } from "./api"; +import { usePoll } from "./usePoll"; +import { PromptModal } from "./Modal"; +import { safeHref } from "./safeHref"; +import type { Activity, Actor, GithubRepoView, PrLinkView } from "./types"; + +// The one human-only act in the attribution model, given a home on the board +// (SYD-290). Before this, seeing/confirming/revoking a declared issue<->PR link +// needed a terminal, a checkout and a token — so the act the design most wants a +// person to make deliberately was the least reachable one, while every act it +// wants gated was a click away. +// +// The panel deliberately shows BOTH halves of the join per link: +// declaration — who said this PR carries the work, and who vouched for it +// observation — what GitHub was last seen doing to that PR +// because a confirmed link whose PR nothing ever observed is a valid statement +// that still proves nothing. Showing only the declaration would put a +// "confirmed ✓" beside a lit "done without a merged PR" banner and read as a +// bug in the banner rather than the missing half it actually is. + +function when(seconds: number): string { + return new Date(seconds * 1000).toLocaleDateString(undefined, { + year: "numeric", + month: "short", + day: "numeric", + }); +} + +export function prUrl(link: PrLinkView): string { + return link.observed?.url ?? `https://github.com/${link.repo}/pull/${link.prNumber}`; +} + +/** PR numbers this issue's own timeline names, newest first, minus the ones already linked. + * + * Design §8's "did you mean?": the issues that need a link most are done ones + * whose merge is already sitting in their activity feed, so the declare form + * should offer that number rather than making a human go find it on GitHub. */ +export function suggestedPrNumbers(activity: Activity[], links: PrLinkView[]): number[] { + const linked = new Set(links.map((l) => l.prNumber)); + const seen = new Set(); + const out: number[] = []; + for (let i = activity.length - 1; i >= 0; i--) { + const ev = activity[i]; + if ( + ev.type !== "gh_pr_merged" && + ev.type !== "gh_pr_opened" && + ev.type !== "gh_pr_reopened" && + ev.type !== "gh_pr_closed" && + ev.type !== "pr_opened" && + ev.type !== "delivered" + ) { + continue; + } + const n = Number(ev.payload.prNumber); + if (!Number.isInteger(n) || n <= 0 || seen.has(n) || linked.has(n)) continue; + seen.add(n); + out.push(n); + } + return out; +} + +/** The repos a link on this issue may name — bound to its project, or unscoped. */ +export function repoOptions(repos: GithubRepoView[], projectId: number): string[] { + return repos + .filter((r) => r.projectId === projectId || r.projectId === null) + .map((r) => r.fullName); +} + +type LinkStateLabel = { label: string; className: string; title: string }; + +/** How a link's two halves read as one status chip. */ +export function linkState(link: PrLinkView): LinkStateLabel { + if (link.observed === null) { + return { + label: "⚪ never observed", + className: "badge pr-link-unobserved", + title: + "Nothing has ever observed this PR, so no declaration about it can prove the work landed. " + + "PRs merged before ingestion was widened have no record at all.", + }; + } + if (link.observed.status === "merged") { + return link.provesLanded + ? { + label: "✅ merged", + className: "badge pr-link-merged", + title: "Merged, and this link proves it landed.", + } + : { + label: "🔒 merged, unproven", + className: "badge warn", + title: + link.role === "references" + ? "This PR merged, but a `references` suggestion never proves what carried the work." + : link.confirmedBy === null + ? "This PR merged, but nobody has confirmed the link yet." + : "This PR merged before the link was declared, and no human confirmed it — so recency binding still applies.", + }; + } + return link.observed.status === "open" + ? { label: "🔀 open", className: "badge pr-link-open", title: "GitHub last saw this PR open." } + : { + label: "🚫 closed", + className: "badge pr-link-closed", + title: "GitHub last saw this PR closed unmerged.", + }; +} + +function Provenance({ link }: { link: PrLinkView }) { + return ( + + declared by {link.declaredByName} · {when(link.declaredAt)} + {link.confirmedByName === null ? ( + <> + {" · "} + unconfirmed + + ) : ( + <> + {" · confirmed by "} + {link.confirmedByName} + {link.confirmedAt !== null && ` · ${when(link.confirmedAt)}`} + {!link.confirmedByHuman && " (not a human — recency still binds)"} + + )} + + ); +} + +export default function PrLinks({ + refId, + projectId, + links, + activity, + me, + onChanged, + compact = false, +}: { + refId: string; + projectId: number; + links: PrLinkView[]; + activity: Activity[]; + me: Actor; + onChanged: () => void; + compact?: boolean; +}) { + const [error, setError] = useState(null); + const [revoking, setRevoking] = useState(null); + const [prInput, setPrInput] = useState(""); + const [repo, setRepo] = useState(null); + const repos = usePoll(listGithubRepos, [], 60000); + + const isHuman = me.type === "human"; + const options = useMemo(() => repoOptions(repos.data ?? [], projectId), [repos.data, projectId]); + const suggestions = useMemo(() => suggestedPrNumbers(activity, links), [activity, links]); + // One bound repo is the overwhelmingly common case, so don't make a human + // pick from a list of one before they can declare anything. + const targetRepo = repo ?? options[0] ?? null; + + const delivers = links.filter((l) => l.role === "delivers"); + const references = links.filter((l) => l.role === "references"); + + const act = (fn: () => Promise) => + fn().then( + () => { + setError(null); + onChanged(); + }, + (e) => setError(e.message), + ); + + const declare = (prNumber: number, role?: "delivers") => { + if (!targetRepo) { + setError( + "No GitHub repo is bound to this project — bind one in Settings → Integrations before declaring a link.", + ); + return; + } + act(() => + declarePrLink(refId, { repo: targetRepo, prNumber, role }).then(() => setPrInput("")), + ); + }; + + const row = (link: PrLinkView) => { + const state = linkState(link); + return ( +
  • + + #{link.prNumber} + {" "} + + {state.label} + {" "} + + + {/* Human-only, and hidden rather than left to 400: an agent viewing + the board should not be shown an act it can never perform. A + `references` link is excluded because confirming one is refused + server-side — promoting it is the correct verb, offered below. */} + {isHuman && link.role === "delivers" && link.confirmedBy === null && ( + + )} + {isHuman && link.role === "references" && ( + + )} + + + {link.observed === null && link.role === "delivers" && ( +

    + Nothing has observed {link.repo}#{link.prNumber}, so this link can't + prove the work landed however it's confirmed. If it did land, clear the flag with + “Mark resolved…” instead. +

    + )} +
    • + ); + }; + + return ( +
    +

    PR links

    + {error && ( +

    + {error} +

    + )} + + {delivers.length > 0 ? ( +
      {delivers.map(row)}
    + ) : ( +

    + No PR is declared as carrying this work. Nothing infers it from a branch name or a mention + — someone has to say so. +

    + )} + + {references.length > 0 && ( + <> +

    + Possibly related{" "} + + — suggestions from PR text + +

    +
      {references.map(row)}
    + + )} + + {isHuman && ( +
    + Declare a PR + {options.length > 1 && ( + + )} + setPrInput(e.target.value.replace(/[^0-9]/g, ""))} + onKeyDown={(e) => { + if (e.key !== "Enter") return; + e.preventDefault(); + if (prInput) declare(Number(prInput)); + }} + /> + + {suggestions.length > 0 && ( + + from this issue's timeline:{" "} + {suggestions.slice(0, 3).map((n) => ( + + ))} + + )} +
    + )} + + {revoking && ( + setRevoking(null)} + onSubmit={(reason) => { + const target = revoking; + setRevoking(null); + act(() => + revokePrLink(refId, { + repo: target.repo, + prNumber: target.prNumber, + reason, + }), + ); + }} + /> + )} +
    + ); +} diff --git a/ui/src/api.ts b/ui/src/api.ts index 9d57395..f76ef4c 100644 --- a/ui/src/api.ts +++ b/ui/src/api.ts @@ -1,6 +1,7 @@ import type { Actor, ActorWithStatus, + AttentionReason, AgentSession, Attachment, Issue, @@ -9,6 +10,8 @@ import type { PendingActionStatus, Priority, Project, + PrLinkRole, + PrLinkView, Status, WebhookView, GithubRepoView, @@ -75,7 +78,10 @@ export const listIssues = ( text?: string; needsInput?: boolean; excludeSnoozed?: boolean; - attention?: "delivery_failed"; + // Any flag, not just delivery_failed: searchIssues has always accepted the + // full AttentionFlag["reason"] union, and clearing the done_without_merged_pr + // backlog (SYD-290) needs to ask for exactly that one. + attention?: AttentionReason; openPr?: boolean; } = {}, ) => { @@ -151,6 +157,35 @@ export const resolveDeviation = (ref: string, reason: string, note?: string) => method: "POST", body: JSON.stringify({ reason, note }), }); +// Declared issue<->PR attribution (SYD-280), reachable from the board (SYD-290). +// +// A human declaring is auto-confirmed server-side, so `declarePrLink` is the +// one-step path that both records the attribution and vouches for it — and the +// only way to PROMOTE a free-text `references` suggestion, which `confirm` +// deliberately refuses (confirming a suggestion would prove nothing, because +// every reader of proof requires role 'delivers'). +export const declarePrLink = ( + ref: string, + input: { repo: string; prNumber: number; role?: PrLinkRole }, +) => + api(`/api/issues/${ref}/pr-links`, { + method: "POST", + body: JSON.stringify(input), + }); +export const confirmPrLink = (ref: string, input: { repo: string; prNumber: number }) => + api(`/api/issues/${ref}/pr-links/confirm`, { + method: "POST", + body: JSON.stringify(input), + }); +export const revokePrLink = ( + ref: string, + input: { repo: string; prNumber: number; reason: string }, +) => + api<{ ok: true }>(`/api/issues/${ref}/pr-links/revoke`, { + method: "POST", + body: JSON.stringify(input), + }); + export const addDependency = (blockerRef: string, blockedRef: string) => api<{ ok: true }>("/api/dependencies", { method: "POST", diff --git a/ui/src/styles.css b/ui/src/styles.css index 494d3e5..ec0d8be 100644 --- a/ui/src/styles.css +++ b/ui/src/styles.css @@ -1525,3 +1525,97 @@ kbd { text-decoration: underline; cursor: pointer; } + +/* Declared issue<->PR links (SYD-290). Each row carries two independent + states — the declaration's provenance and the PR's observed status — so the + layout keeps them on one line and lets the explanatory note wrap below. */ +.pr-links { + margin: 12px 0; +} +.pr-links h3 { + margin: 0 0 8px; +} +.pr-links.compact { + margin: 8px 0; +} +.pr-link-list { + list-style: none; + margin: 0; + padding: 0; + display: flex; + flex-direction: column; + gap: 8px; +} +.pr-link-row { + display: flex; + align-items: center; + flex-wrap: wrap; + gap: 6px; + font-size: 14px; +} +.pr-link-number { + font-weight: 600; +} +.pr-link-provenance { + color: var(--muted); + font-size: 13px; +} +.pr-link-unconfirmed { + color: var(--warn); + font-style: normal; +} +.pr-link-actions { + display: flex; + gap: 6px; + margin-left: auto; +} +.pr-link-merged { + color: var(--ok); + border-color: var(--ok); +} +.pr-link-open { + color: var(--accent-2); + border-color: var(--accent-2); +} +.pr-link-closed, +.pr-link-unobserved { + color: var(--muted); +} +/* The note explaining why a confirmed link still proves nothing takes the full + row width — it is the panel's whole reason for showing the observation half. */ +.pr-link-note { + flex-basis: 100%; + margin: 2px 0 0; + font-size: 13px; + color: var(--warn); +} +.pr-link-references { + opacity: 0.8; + margin-top: 4px; +} +.pr-link-suggestions-head { + margin: 12px 0 4px; + font-size: 13px; + font-weight: 600; +} +.pr-link-hint { + font-weight: 400; + color: var(--muted); +} +.pr-link-declare { + display: flex; + align-items: center; + flex-wrap: wrap; + gap: 8px; + margin-top: 12px; + font-size: 13px; + color: var(--muted); +} +.pr-link-input { + width: 90px; +} +.pr-link-suggests { + display: flex; + align-items: center; + gap: 6px; +} diff --git a/ui/src/types.ts b/ui/src/types.ts index 154b00b..7cc870b 100644 --- a/ui/src/types.ts +++ b/ui/src/types.ts @@ -24,8 +24,21 @@ export const SUMMARY_MAX_LENGTH = 280; export const INTERACTIVE_PREFERENCE = "interactive"; /** Options for the "preferred worker" dropdown: an engine name, or the interactive sentinel. */ export const WORKER_PREFERENCES = ["claude", "codex", "gemini", INTERACTIVE_PREFERENCE] as const; +/** Every reason `GET /api/issues?attention=` accepts — mirrors AttentionFlag["reason"] server-side. */ +export const ATTENTION_REASONS = [ + "delivery_failed", + "merged_pr_not_done", + "open_pr_not_in_review", + "stale_claim", + "done_without_merged_pr", + "done_pr_not_delivered", +] as const; +export type AttentionReason = (typeof ATTENTION_REASONS)[number]; + export type Issue = { id: number; + /** Needed to scope a declared PR link to the repos bound to this issue's project. */ + projectId: number; ref: string; title: string; description: string; @@ -45,16 +58,45 @@ export type Issue = { snoozedUntil: number | null; createdAt: number; updatedAt: number; - attention: - | { reason: "delivery_failed"; message: string } - | { reason: "merged_pr_not_done"; message: string } - | { reason: "open_pr_not_in_review"; message: string } - | { reason: "stale_claim"; message: string } - | { reason: "done_without_merged_pr"; message: string } - | { reason: "done_pr_not_delivered"; message: string } - | null; + attention: { reason: AttentionReason; message: string } | null; openPr: { prNumber: number; url: string; repo: string; headSha: string | null } | null; }; +export const PR_LINK_ROLES = ["delivers", "references"] as const; +export type PrLinkRole = (typeof PR_LINK_ROLES)[number]; + +/** + * A declared issue↔PR link as the issue page shows it (SYD-280/SYD-290) — the + * shape `listLiveLinkViews` returns, which is the DECLARATION (who said this PR + * carries the work, and who vouched for it) joined to the OBSERVATION (what + * GitHub was last seen doing to that PR). + * + * `observed: null` is not "not merged" — it means nothing has ever observed + * this PR, which is the state every PR merged before SYD-287 is in. The panel + * has to distinguish the two, because only the first is fixable by clicking. + */ +export type PrLinkView = { + id: number; + issueId: number; + repo: string; + prNumber: number; + role: PrLinkRole; + declaredBy: number; + declaredByName: string; + declaredAt: number; + confirmedBy: number | null; + confirmedByName: string | null; + confirmedByHuman: boolean; + confirmedAt: number | null; + revokedAt: number | null; + observed: { + status: "open" | "merged" | "closed"; + url: string | null; + ghUpdatedAt: number | null; + } | null; + /** Whether this link alone lets a reader conclude the work landed — the server's verdict, not the UI's. */ + provesLanded: boolean; +}; + export type Activity = { type: string; actorName: string; @@ -87,6 +129,8 @@ export type IssueDetail = Issue & { headSha: string | null; status: "open" | "merged" | "closed"; } | null; + /** Live declared PR links — read these directly, never inferred from openPr/deliveryPin. */ + prLinks: PrLinkView[]; }; export type AgentSession = { id: number; diff --git a/ui/src/views/Board.test.tsx b/ui/src/views/Board.test.tsx index 92f5938..0ba0d19 100644 --- a/ui/src/views/Board.test.tsx +++ b/ui/src/views/Board.test.tsx @@ -24,6 +24,7 @@ vi.mock("../api", () => ({ function issue(o: Partial = {}): Issue { return { id: 1, + projectId: 1, ref: "SYD-1", title: "Ship it", description: "", diff --git a/ui/src/views/Board.tsx b/ui/src/views/Board.tsx index 1655de8..28d890d 100644 --- a/ui/src/views/Board.tsx +++ b/ui/src/views/Board.tsx @@ -21,7 +21,13 @@ const LABELS: Record = { // SYD-175: the actionable view is the DEFAULT (during a queue recovery the // actionable set was 9 of 150+ cards); the full history sits behind an // explicit "all" pill, and an explicit choice persists per browser. -type DoneFilter = "errors" | "not_merged"; +// SYD-290: "unlinked" is the third, and the one with a backlog behind it — +// ~46 done issues carry an unresolved done_without_merged_pr and none of them +// had a live PR link, because declaring and confirming were CLI-only. Clearing +// them is per-issue by design (Sean's call: click through them, no bulk +// script), so the filter that finds them is what makes that a queue rather than +// an archaeology exercise. +type DoneFilter = "errors" | "not_merged" | "unlinked"; const DONE_FILTERS_STORAGE_KEY = "switchyard:done-filters"; const DEFAULT_DONE_FILTERS: DoneFilter[] = ["errors", "not_merged"]; @@ -98,7 +104,8 @@ export default function Board({ project }: { project: string }) { cards = cards.filter( (i) => (doneFilters.has("errors") && i.attention?.reason === "delivery_failed") || - (doneFilters.has("not_merged") && i.openPr != null), + (doneFilters.has("not_merged") && i.openPr != null) || + (doneFilters.has("unlinked") && i.attention?.reason === "done_without_merged_pr"), ); } const badgeText = @@ -141,6 +148,15 @@ export default function Board({ project }: { project: string }) { > 🔀 not merged +