From 8db58076194e70c8587a96e3402d45bef5b51988 Mon Sep 17 00:00:00 2001 From: Sean Perkins <1733750+seanperkins@users.noreply.github.com> Date: Sun, 27 Sep 2026 22:32:21 -0400 Subject: [PATCH 1/8] fix: require trusted PR declarations and preserve revocations Addresses #247, #252, and #260. --- src/services/delivery-events.ts | 22 ++- src/services/dependencies.ts | 5 + src/services/github-webhook.ts | 11 +- src/services/pr-links.ts | 83 +++++------ src/services/pr-state.ts | 80 ++-------- tests/mcp/write-tools.test.ts | 20 ++- tests/rest/api-pr-state.test.ts | 2 +- tests/rest/api-service-actor.test.ts | 15 ++ tests/services/attention.test.ts | 3 + tests/services/delivery-attempts.test.ts | 1 + tests/services/delivery-events.test.ts | 4 +- tests/services/deviation.test.ts | 4 + tests/services/github-webhook.test.ts | 10 +- tests/services/ingestion-authority.test.ts | 166 +++++++++++++++++++++ tests/services/pr-links.test.ts | 9 +- tests/services/pr-observation.test.ts | 10 +- tests/services/pr-state-cutover.test.ts | 4 +- tests/services/pr-state.test.ts | 22 ++- tests/services/pr-status.test.ts | 115 +++++++++++--- tests/services/service-actor.test.ts | 13 +- 20 files changed, 428 insertions(+), 171 deletions(-) create mode 100644 tests/services/ingestion-authority.test.ts diff --git a/src/services/delivery-events.ts b/src/services/delivery-events.ts index 63ff0369..aa860beb 100644 --- a/src/services/delivery-events.ts +++ b/src/services/delivery-events.ts @@ -11,7 +11,8 @@ import { getIssue } from "./issues.js"; import { recordEvent } from "./events.js"; import { boundRepoFullNames, normalizeRepoFullName } from "./github-repos.js"; import { parseGhTimestamp } from "./github-webhook.js"; -import { upsertPrState, attributedRef } from "./pr-state.js"; +import { upsertPrState } from "./pr-state.js"; +import { recordIngestedPrLink } from "./pr-links.js"; export type DeployResult = { ran: false } | { ran: true; ok: boolean; tail: string }; @@ -62,7 +63,7 @@ export function recordDeliveryEvent( if (repo === null) { // SYD-205 deploy-skew rule: infer only when it's unambiguous. const bound = boundRepoFullNames(db, issue.projectId); - if (bound.length === 1) repo = bound[0]; + if (bound.length === 1) repo = normalizeRepoFullName(bound[0]); else if (bound.length > 1) { throw new SwitchyardError( "repo is ambiguous — the issue's project has multiple bound repos, so this delivery event must name its repo.", @@ -86,7 +87,22 @@ export function recordDeliveryEvent( // state, exactly as in webhook ingestion. if (repo !== null && (input.type === "pr_opened" || input.type === "delivered")) { const branch = `agent/${issue.ref}`; - if (attributedRef(db, repo, branch) === issue.ref) { + if ( + boundRepoFullNames(db, issue.projectId).some((bound) => normalizeRepoFullName(bound) === repo) + ) { + // This route is authenticated as human/service infrastructure, and its + // explicit issue ref is the worker host's declaration. GitHub webhooks + // and poller observations have no such authority. Publication alone may + // establish the link; a later delivery must not invent missing attribution. + if (input.type === "pr_opened") { + recordIngestedPrLink(db, { + issueId: issue.id, + repo, + prNumber: input.prNumber, + role: "delivers", + actorId: actor.id, + }); + } upsertPrState(db, actor, { repo, prNumber: input.prNumber, diff --git a/src/services/dependencies.ts b/src/services/dependencies.ts index 426df400..e23967a3 100644 --- a/src/services/dependencies.ts +++ b/src/services/dependencies.ts @@ -29,6 +29,11 @@ export function addDependency( blockedRef: string, attr: Attribution = {}, ): void { + if (actor.type === "service") { + throw new SwitchyardError( + "Service actors post events, read, and comment — they cannot add dependencies.", + ); + } db.transaction((tx) => { const blocker = getIssue(tx, blockerRef); const blocked = getIssue(tx, blockedRef); diff --git a/src/services/github-webhook.ts b/src/services/github-webhook.ts index be040d23..d65a0a38 100644 --- a/src/services/github-webhook.ts +++ b/src/services/github-webhook.ts @@ -10,9 +10,8 @@ // first, then a bare ref in free text (PR title/body, or commit messages for // push). This is display, and a string may decide it. // - IS THE PR ATTRIBUTED to an issue's work? DECLARED, never parsed: a live -// `delivers` row in pr_links (SYD-280). An `agent/` branch auto-declares -// its own link inside upsertPrState, which is why dispatched work needs no -// extra step; everyone else calls declare_pr_link. +// `delivers` row in pr_links (SYD-280). The authenticated worker host +// declares at publish; a branch name from GitHub only suggests a reference. // // pull_request ingestion answers NEITHER question before writing `pr_state`: // since SYD-287 every PR in a bound repo is observed, because pr_state records @@ -35,7 +34,7 @@ import { getOrCreateActor } from "./actors.js"; import { getIssue, issueRefById } from "./issues.js"; import { recordEvent } from "./events.js"; import { boundRepoFullNames, findGithubRepo, normalizeRepoFullName } from "./github-repos.js"; -import { upsertPrState, attributedRef, type PrObservation } from "./pr-state.js"; +import { upsertPrState, type PrObservation } from "./pr-state.js"; import { recordIngestedPrLink, deliversLinkIssueIds } from "./pr-links.js"; const GITHUB_ACTOR_NAME = "github"; @@ -307,8 +306,6 @@ function handlePullRequest(db: Db, rawPayload: unknown, repo: string | null): Gi // sole-bound-repo inference above. const linkedIssueIds = resolvedRepo === null ? [] : deliversLinkIssueIds(db, resolvedRepo, prNumber); - const branchAttributed = - resolvedRepo !== null && attributedRef(db, resolvedRepo, branch) !== null; // OBSERVATION (SYD-206, widened by SYD-287). Every PR in a bound repo, full // stop — no branch test, no link test. @@ -376,7 +373,7 @@ function handlePullRequest(db: Db, rawPayload: unknown, repo: string | null): Gi // text ref's issue already took upsertPrState's canonical co-write, which is // what keeps one transition from appearing twice. let displayOutcome: GithubWebhookOutcome | null = null; - if (issue !== undefined && !branchAttributed && !linkedIssueIds.includes(issue.id)) { + if (issue !== undefined && !linkedIssueIds.includes(issue.id)) { const textOnlyRef = issue.ref; const byPrNumber = { jsonPath: "$.prNumber", value: prNumber }; if (action === "opened") { diff --git a/src/services/pr-links.ts b/src/services/pr-links.ts index 72f16d67..3bfcb7a2 100644 --- a/src/services/pr-links.ts +++ b/src/services/pr-links.ts @@ -2,8 +2,8 @@ // 2026-07-27-declared-pr-attribution-design.md). // // This module is the ONLY writer of pr_links. It replaces three string -// inference sites — the strict agent/ branch match (pr-state.ts's -// attributedRef), the first free-text ref in a PR title/body +// inference sites — the former strict agent/ branch match, +// the first free-text ref in a PR title/body // (github-webhook.ts's resolveRef), and a branch name reconstructed from the // issue ref (delivery-events.ts) — with a statement made by someone the system // can hold accountable. @@ -198,8 +198,7 @@ function findLiveLink( } /** - * The repo must be bound to the issue's project — the same rule attributedRef - * enforces today (src/services/pr-state.ts). Without it any repo could claim + * The repo must be bound to the issue's project. Without it any repo could claim * any issue, which is the cross-project attribution hole SYD-206 closed. */ function assertRepoBound(db: DbOrTx, projectId: number, repo: string): void { @@ -315,42 +314,16 @@ export function declarePrLink( } /** - * Ingestion's declaration path — webhook/poller and the worker's publish. + * Records an inert reference suggestion from GitHub, or the explicit issue + * declaration made by authenticated worker publication. Only the delivery + * service may pass `delivers`; external observations always pass `references`. + * A signed webhook authenticates GitHub's observation, never a fork author's + * relationship to an issue. Runs inside the caller's transaction. * - * Separate from declarePrLink because ingestion is not an actor staking a - * claim: it is the system recording an attribution it observed, attributed to - * the synthetic `github` actor (which is type `agent`, - * src/services/github-webhook.ts:176, and so could never satisfy the - * claim+lease rules). Runs inside the caller's transaction. - * - * **Scope discipline — this is parity, not a widening.** Only the - * branch-attributed path (a strict agent/ match in a repo bound to that - * ref's project) may pass `role: "delivers"`, because that is precisely the - * signal that gates claims and proves landing *today*. Free-text ref matches - * must pass `role: "references"`, which gates nothing and proves nothing — a - * narrowing of today's behaviour, and the fix for the false-clear hole where - * an unrelated PR that merely mentions an issue silences its warning. - * - * Note the untrusted-ingress caveat: POST /api/github-events accepts any - * human/service token and is indistinguishable from an HMAC-verified delivery - * (design "Scope", analysis §3.7). That is true of today's attribution too, so - * this is not a regression — it is SYD-282's to fix, and this function must - * not be read as making ingested merges trustworthy. - * - * Idempotent: a redelivery finds the live link and returns it unchanged. The - * one exception is the upgrade below — a branch-attributed observation - * supersedes a `references` suggestion an earlier free-text match minted for - * the same (issue, repo, PR), because otherwise ingestion order would decide - * whether a PR gates claims. - * - * **Records no event, deliberately.** The row itself carries the full audit - * (declared_by, declared_at, role), and the observation that prompted it is - * already on the timeline as gh_pr_opened/gh_pr_merged. Emitting an event too - * would put a "pr_link_declared" line above every single PR in every activity - * feed — a signal that fires on ordinary success, which is the noise class the - * intent document's principle 5 warns about. Actor-initiated declarations - * (declarePrLink/confirmPrLink/revokePrLink) DO record events, because those - * are decisions someone made rather than bookkeeping the system did. + * Replays preserve a live link and never undo a revocation. A host publication + * may promote an existing references suggestion to delivers; an explicit + * declarePrLink call is required to override a revoked relationship. + * The host's publication event supplies the audit; suggestions add no event. */ export function recordIngestedPrLink( tx: DbOrTx, @@ -361,13 +334,13 @@ export function recordIngestedPrLink( role: PrLinkRole; actorId: number; }, -): PrLink { +): PrLink | null { const repo = normalizeRepoFullName(input.repo); const now = nowSeconds(); const existing = findLiveLink(tx, input.issueId, repo, input.prNumber); if (existing) { - // Upgrade only, never downgrade (SYD-287). The branch-attributed path is - // strictly more authoritative than the free-text one, so it supersedes a + // Upgrade only, never downgrade. Authenticated host publication + // is an explicit declaration, so it supersedes a // `references` suggestion the same way an actor's declaration does — // otherwise a PR that happened to be ingested by text first would keep a // suggestion where a claim-gating link belongs, and lose its co-written @@ -376,10 +349,28 @@ export function recordIngestedPrLink( tx.update(prLinks).set({ revokedAt: now }).where(eq(prLinks.id, existing.id)).run(); } - // A `delivers` link from the branch-attributed path is confirmed, matching - // the authority pr_state.issue_ref carries today — its confirmer is not a - // human, so §5a's recency binding still applies to it. A `references` link - // is never confirmed: it is a suggestion for a human to promote. + // An observation cannot undo an explicit revocation. Also suppress an + // inert references suggestion after a revoke, so the panel does not offer + // the same rejected relationship on every poll. A deliberate declarePrLink + // call may create a new live row and supersede this tombstone. + if (!existing) { + const revoked = tx + .select({ id: prLinks.id }) + .from(prLinks) + .where( + and( + eq(prLinks.issueId, input.issueId), + sql`lower(${prLinks.repo}) = lower(${repo})`, + eq(prLinks.prNumber, input.prNumber), + sql`${prLinks.revokedAt} IS NOT NULL`, + ), + ) + .get(); + if (revoked) return null; + } + + // Trusted host publication retains its existing confirmation semantics. + // External references suggestions are never confirmed. const confirmed = input.role === "delivers"; const row = tx .insert(prLinks) diff --git a/src/services/pr-state.ts b/src/services/pr-state.ts index f13359ed..2df272d0 100644 --- a/src/services/pr-state.ts +++ b/src/services/pr-state.ts @@ -23,13 +23,9 @@ // an observation; a PR missing from a poll window simply produces no call. // - This table holds no attribution. It is a pure observation of a PR, keyed // (repo, prNumber); which issue a PR belongs to lives in pr_links (SYD-280). -// `issueRef` survives only as the SYD-280 §10 step-3 dual-write, written -// from the branch and read by nothing — never from a link, which would -// re-couple the two facts this split exists to separate. Step 4 drops it. -// - A strict agent/ match on a repo bound to that ref's project is the -// AUTO-DECLARATION trigger, not a gate: it records the `delivers` link the -// worker would otherwise have to declare by hand. An agent/SYD-1 PR in some -// other project's repo declares nothing. +// `issueRef` retains legacy cutover data only. New observations never infer +// attribution from a branch, including agent/ in a linked repository. +// Worker publication declares through the authenticated delivery service. // - On a real transition it co-writes ONE canonical audit event per issue // holding a live `delivers` link (gh_pr_opened/gh_pr_merged/gh_pr_closed/ // gh_pr_reopened), deduped against history via findEventIdByPayload so a @@ -40,15 +36,13 @@ import { and, eq, sql } from "drizzle-orm"; import type { Db, DbOrTx } from "../db/index.js"; -import { prState, githubRepos, type EventKind } from "../db/schema.js"; +import { prState, type EventKind } from "../db/schema.js"; import type { Actor } from "./actors.js"; import { SwitchyardError } from "./errors.js"; -import { getIssue } from "./issues.js"; -import { getProjectByKey } from "./projects.js"; import { normalizeRepoFullName } from "./github-repos.js"; import { recordEvent, findEventIdByPayload } from "./events.js"; -import { parseGhTimestamp, refFromBranch } from "./github-webhook.js"; -import { recordIngestedPrLink, deliversLinkIssueIds } from "./pr-links.js"; +import { parseGhTimestamp } from "./github-webhook.js"; +import { deliversLinkIssueIds } from "./pr-links.js"; export type PrStateRow = typeof prState.$inferSelect; export type PrStatus = "open" | "merged" | "closed"; @@ -107,40 +101,6 @@ export function listPrState(db: Db, filter: { repo?: string; status?: string } = return (conditions.length > 0 ? query.where(and(...conditions)) : query).all(); } -/** issueRef for a PR, or null: strict agent/ branch match, repo bound to - * that ref's project, and the issue actually exists. */ -export function attributedRef( - db: DbOrTx, - repo: string, - branch: string | null | undefined, -): string | null { - const ref = refFromBranch(branch); - if (!ref) return null; - let projectId: number; - try { - projectId = getProjectByKey(db, ref.split("-")[0]).id; - } catch { - return null; - } - const bound = db - .select({ id: githubRepos.id }) - .from(githubRepos) - .where( - and( - sql`lower(${githubRepos.fullName}) = lower(${repo})`, - eq(githubRepos.projectId, projectId), - ), - ) - .get(); - if (!bound) return null; - try { - getIssue(db, ref); - } catch { - return null; - } - return ref; -} - const EVENT_KIND: Record = { opened: "gh_pr_opened", merged: "gh_pr_merged", @@ -200,31 +160,19 @@ export function upsertPrState(db: Db, actor: Actor, input: PrObservation): Upser const decision = decide(existing, o, ts); if (!decision.apply) return { applied: false, transition: null, reason: decision.reason }; - const issueRef = attributedRef(tx, o.repo, o.branch) ?? existing?.issueRef ?? null; - - // SYD-280: the branch-attributed path co-declares the issue<->PR link. - // Done here rather than at each caller because upsertPrState is the ONLY - // pr_state writer, so this one site covers webhook, poller, the worker's - // publish and the delivery merge. Idempotent, so refreshes and - // redeliveries are no-ops. - if (issueRef !== null) { - recordIngestedPrLink(tx, { - issueId: getIssue(tx, issueRef).id, - repo: o.repo, - prNumber: o.prNumber, - role: "delivers", - actorId: actor.id, - }); - } + // Observations never establish attribution: even a signed GitHub event + // can describe a fork whose author chose agent/. The authenticated + // worker host declares at publish; every other author declares explicitly. + // Retain old cutover data only. New observations must not seed a later + // backfill with a branch-derived assertion. + const issueRef = existing?.issueRef ?? null; let lastTransitionEventId = existing?.lastTransitionEventId ?? null; if (decision.transition !== null) { // SYD-287: the issues this transition is canonical for are the ones // holding a live `delivers` link — read from pr_links, not from the - // branch-derived issueRef above. For agent/ work that is the same - // single issue it always was (the auto-declaration a few lines up wrote - // its link), so the SYD-280 regression fence holds; for an interactive - // feat/ PR it is the only reason the merge reaches a timeline at all. + // legacy issueRef column. Authenticated publication and explicit + // declarations share this path for worker and interactive PRs. // A PR declared by more than one issue (design §3, SYD-274) gets the // event on each; the row's single lastTransitionEventId takes the // earliest declarer's, which deliversLinkIssueIds orders first. diff --git a/tests/mcp/write-tools.test.ts b/tests/mcp/write-tools.test.ts index f1a67884..f976a10a 100644 --- a/tests/mcp/write-tools.test.ts +++ b/tests/mcp/write-tools.test.ts @@ -7,7 +7,7 @@ import path from "node:path"; import { openDb, type Db } from "../../src/db/index.js"; import { createActor, type Actor } from "../../src/services/actors.js"; import { createProject } from "../../src/services/projects.js"; -import { getIssue, SUMMARY_MAX_LENGTH } from "../../src/services/issues.js"; +import { createIssue, getIssue, SUMMARY_MAX_LENGTH } from "../../src/services/issues.js"; import { snoozeIssue } from "../../src/services/triage-actions.js"; import { buildMcpServer } from "../../src/mcp/server.js"; import { getActivity } from "../../src/services/comments.js"; @@ -36,6 +36,24 @@ beforeEach(async () => { }); describe("MCP write tools", () => { + it("refuses service actors adding a dependency through MCP", async () => { + createIssue(db, human, { projectKey: "AIPI", title: "Blocker" }); + createIssue(db, human, { projectKey: "AIPI", title: "Target" }); + const service = createActor(db, { name: "poller", type: "service" }).actor; + const serviceClient = await connect(service); + try { + const result = await serviceClient.callTool({ + name: "add_dependency", + arguments: { blocker_ref: "AIPI-1", blocked_ref: "AIPI-2" }, + }); + expect(result.isError).toBe(true); + expect(text(result)).toMatch(/cannot add dependencies/); + expect(getActivity(db, "AIPI-2").map((e) => e.type)).toEqual(["created"]); + } finally { + await serviceClient.close(); + } + }); + it("file_issue's description tells agents to set a suggested priority (SYD-65)", async () => { const { tools } = await client.listTools(); const fileIssue = tools.find((t) => t.name === "file_issue")!; diff --git a/tests/rest/api-pr-state.test.ts b/tests/rest/api-pr-state.test.ts index 0dfc477a..ee43b0e8 100644 --- a/tests/rest/api-pr-state.test.ts +++ b/tests/rest/api-pr-state.test.ts @@ -51,7 +51,7 @@ describe("GET /pr-state", () => { expect(res.status).toBe(200); const rows = (await res.json()) as { prNumber: number; status: string; issueRef: string }[]; expect(rows).toHaveLength(1); - expect(rows[0]).toMatchObject({ prNumber: 7, status: "open", issueRef: "SYD-1" }); + expect(rows[0]).toMatchObject({ prNumber: 7, status: "open", issueRef: null }); }); it("returns all rows for a repo without a status filter", async () => { diff --git a/tests/rest/api-service-actor.test.ts b/tests/rest/api-service-actor.test.ts index 8ec3346f..a00a3861 100644 --- a/tests/rest/api-service-actor.test.ts +++ b/tests/rest/api-service-actor.test.ts @@ -3,6 +3,7 @@ import { openDb, type Db } from "../../src/db/index.js"; import { createActor, type Actor } from "../../src/services/actors.js"; import { createProject } from "../../src/services/projects.js"; import { createIssue } from "../../src/services/issues.js"; +import { listDependencies } from "../../src/services/dependencies.js"; import { buildApiRoutes } from "../../src/rest/api-routes.js"; // SYD-213: REST-layer authorization for a `service` token. The service-layer @@ -23,6 +24,20 @@ beforeEach(() => { const svc = () => ({ authorization: `Bearer ${serviceToken}`, "content-type": "application/json" }); describe("service token — REST-layer guards", () => { + it("CANNOT add a dependency", async () => { + createIssue(db, human, { projectKey: "SYD", title: "Blocked target" }); + const res = await app.request("/dependencies", { + method: "POST", + headers: svc(), + body: JSON.stringify({ blockerRef: "SYD-1", blockedRef: "SYD-2" }), + }); + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ + error: expect.stringMatching(/cannot add dependencies/), + }); + expect(listDependencies(db, "SYD-2").blockedBy).toEqual([]); + }); + it("CANNOT create an actor (requireHumanCaller)", async () => { const res = await app.request("/actors", { method: "POST", diff --git a/tests/services/attention.test.ts b/tests/services/attention.test.ts index 6864d1fa..c19a17bd 100644 --- a/tests/services/attention.test.ts +++ b/tests/services/attention.test.ts @@ -64,6 +64,7 @@ describe("getAttention", () => { // lands in pr_state with a co-written transition event newer than the // failure — that is what clears the flag now (the deleted SYD-94 // reconcile pass used to do this with per-ref gh lookups). + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 7 }); upsertPrState(db, human, { repo: REPO, prNumber: 7, @@ -197,6 +198,7 @@ describe("getAttention — done_without_merged_pr (SYD-204)", () => { updateIssue(db, human, "SYD-1", { status: "in_review" }); updateIssue(db, human, "SYD-1", { status: "done" }); expect(getAttention(db, getIssue(db, "SYD-1").id)?.reason).toBe("done_without_merged_pr"); + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 9 }); upsertPrState(db, human, { repo: REPO, prNumber: 9, @@ -466,6 +468,7 @@ describe("getAttention — done_without_merged_pr (SYD-204)", () => { const { db, human, agent } = setup(); updateIssue(db, human, "SYD-1", { status: "todo" }); claimIssue(db, agent, "SYD-1"); + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 41 }); recordDeliveryEvent(db, human, "SYD-1", { type: "delivered", prNumber: 41, diff --git a/tests/services/delivery-attempts.test.ts b/tests/services/delivery-attempts.test.ts index 2f95db89..60bf8aa2 100644 --- a/tests/services/delivery-attempts.test.ts +++ b/tests/services/delivery-attempts.test.ts @@ -492,6 +492,7 @@ describe("poller-down done-stamp then recovery (SYD-228)", () => { expect(listPendingDeliveryAuthorizations(db)).toEqual([]); // The poller recovers and observes the still-open PR. + declarePrLink(db, human, issue.ref, { repo: REPO, prNumber: 41 }); upsertPrState(db, human, { repo: REPO, prNumber: 41, diff --git a/tests/services/delivery-events.test.ts b/tests/services/delivery-events.test.ts index ac8ae7ee..9410c2a7 100644 --- a/tests/services/delivery-events.test.ts +++ b/tests/services/delivery-events.test.ts @@ -169,7 +169,7 @@ describe("recordDeliveryEvent / ingestion groundwork (SYD-205)", () => { const row = findPrState(db, "acme/bound", 12)!; expect(row).toMatchObject({ status: "open", - issueRef: "SYD-1", + issueRef: null, branch: "agent/SYD-1", headSha: "a".repeat(40), }); @@ -203,7 +203,7 @@ describe("recordDeliveryEvent / ingestion groundwork (SYD-205)", () => { repo: "Acme/Bound", }); expect(getActivity(db, "SYD-1")[1].payload).toMatchObject({ repo: "acme/bound" }); - expect(findPrState(db, "acme/bound", 12)!.issueRef).toBe("SYD-1"); + expect(findPrState(db, "acme/bound", 12)!.issueRef).toBeNull(); }); it("never writes pr_state when the event's repo is not bound to the issue's project", () => { diff --git a/tests/services/deviation.test.ts b/tests/services/deviation.test.ts index 5dc279f3..b6a5feaf 100644 --- a/tests/services/deviation.test.ts +++ b/tests/services/deviation.test.ts @@ -1,3 +1,4 @@ +import { declarePrLink } from "../../src/services/pr-links.js"; import { describe, it, expect } from "vitest"; import { eq } from "drizzle-orm"; import { openDb, type Db } from "../../src/db/index.js"; @@ -263,6 +264,7 @@ describe("emitProcessDeviations", () => { branch: "agent/SYD-1", url: `https://github.com/${REPO}/pull/41`, }; + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 41 }); upsertPrState(db, human, { ...observation, status: "open", @@ -343,6 +345,7 @@ describe("getDeviation — done_pr_not_delivered (SYD-261)", () => { updateIssue(db, human, "SYD-1", { status: "in_review" }); updateIssue(db, human, "SYD-1", { status: "done" }); // The PR registers AFTER the stamp — the ordering this issue is about. + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 304 }); upsertPrState(db, human, { repo: REPO, prNumber: 304, @@ -538,6 +541,7 @@ describe("updateIssue done transition — done_without_merged_pr (SYD-204)", () createIssue(db, human, { projectKey: "SYD", title: "Ship it" }); updateIssue(db, human, "SYD-1", { status: "todo" }); claimIssue(db, agent, "SYD-1"); + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 41 }); recordDeliveryEvent(db, human, "SYD-1", { type: "delivered", prNumber: 41, diff --git a/tests/services/github-webhook.test.ts b/tests/services/github-webhook.test.ts index 0c73dad9..753d2414 100644 --- a/tests/services/github-webhook.test.ts +++ b/tests/services/github-webhook.test.ts @@ -14,7 +14,7 @@ import { refsFromText, repositoryFullName, } from "../../src/services/github-webhook.js"; -import { listLiveLinks } from "../../src/services/pr-links.js"; +import { declarePrLink, listLiveLinks } from "../../src/services/pr-links.js"; function setup(boundRepos: string[] = []) { const db = openDb(":memory:"); @@ -567,10 +567,12 @@ describe("handleGithubWebhook / pr_state integration (SYD-206)", () => { it("writes an attributed pr_state row on opened, with exactly one gh_pr_opened event (co-write, no double record)", () => { const db = setup(["acme/bound"]); + const declarer = createActor(db, { name: "declarer", type: "human" }).actor; + declarePrLink(db, declarer, "SYD-1", { repo: "acme/bound", prNumber: 12 }); const outcome = handleGithubWebhook(db, "pull_request", opened("opened")); expect(outcome).toEqual({ handled: true, ref: "SYD-1", type: "gh_pr_opened" }); const row = findPrState(db, "acme/bound", 12)!; - expect(row).toMatchObject({ status: "open", issueRef: "SYD-1", headSha: "a".repeat(40) }); + expect(row).toMatchObject({ status: "open", issueRef: null, headSha: "a".repeat(40) }); expect(getActivity(db, "SYD-1").filter((a) => a.type === "gh_pr_opened")).toHaveLength(1); }); @@ -684,6 +686,8 @@ describe("handleGithubWebhook / pr_state integration (SYD-206)", () => { it("attributes and writes pr_state despite a casing mismatch between the linked repo and the payload's repository.full_name (SYD-212)", () => { // Repo linked with a hand-typed lowercase full name... const db = setup(["acme/bound"]); + const declarer = createActor(db, { name: "declarer", type: "human" }).actor; + declarePrLink(db, declarer, "SYD-1", { repo: "acme/bound", prNumber: 12 }); // ...but the real webhook delivery carries GitHub's canonical case. const outcome = handleGithubWebhook(db, "pull_request", { ...opened("opened"), @@ -691,7 +695,7 @@ describe("handleGithubWebhook / pr_state integration (SYD-206)", () => { }); expect(outcome).toEqual({ handled: true, ref: "SYD-1", type: "gh_pr_opened" }); const row = findPrState(db, "acme/bound", 12)!; - expect(row).toMatchObject({ status: "open", issueRef: "SYD-1", repo: "acme/bound" }); + expect(row).toMatchObject({ status: "open", issueRef: null, repo: "acme/bound" }); // The stored row itself is normalized, not left as the canonical-case // string the payload happened to carry. expect(findPrState(db, "Acme/Bound", 12)?.repo).toBe("acme/bound"); diff --git a/tests/services/ingestion-authority.test.ts b/tests/services/ingestion-authority.test.ts new file mode 100644 index 00000000..b96b7640 --- /dev/null +++ b/tests/services/ingestion-authority.test.ts @@ -0,0 +1,166 @@ +import { describe, expect, it } from "vitest"; +import { createHmac } from "node:crypto"; +import { githubRepos } from "../../src/db/schema.js"; +import { openDb } from "../../src/db/index.js"; +import { createActor } from "../../src/services/actors.js"; +import { createProject } from "../../src/services/projects.js"; +import { createIssue, claimIssue, updateIssue } from "../../src/services/issues.js"; +import { addGithubRepo } from "../../src/services/github-repos.js"; +import { handleGithubWebhook } from "../../src/services/github-webhook.js"; +import { recordDeliveryEvent } from "../../src/services/delivery-events.js"; +import { declarePrLink, listLiveLinkViews, revokePrLink } from "../../src/services/pr-links.js"; +import { getMergedPr, getOpenPr } from "../../src/services/pr-status.js"; +import { findPrState } from "../../src/services/pr-state.js"; +import { buildGithubWebhookRoutes } from "../../src/rest/github-routes.js"; +import { buildApiRoutes } from "../../src/rest/api-routes.js"; + +const repo = "acme/widgets"; +const head = "a".repeat(40); +function setup() { + const db = openDb(":memory:"); + const human = createActor(db, { name: "reviewer", type: "human" }).actor; + const agent = createActor(db, { name: "worker", type: "agent" }).actor; + const service = createActor(db, { name: "host", type: "service" }); + createProject(db, human, { key: "SYD", name: "Switchyard" }); + const issue = createIssue(db, human, { projectKey: "SYD", title: "Real work" }); + updateIssue(db, human, issue.ref, { status: "todo" }); + addGithubRepo(db, human, { fullName: repo, projectKey: "SYD" }); + return { db, human, agent, service, issue }; +} +function payload(action = "opened", timestamp = "2030-01-01T00:00:00Z") { + return { + action, + repository: { full_name: repo }, + pull_request: { + number: 7, + title: "Unrelated change", + head: { ref: "agent/SYD-1", sha: head, repo: { full_name: "attacker/fork" } }, + updated_at: timestamp, + merged: action === "closed", + merge_commit_sha: action === "closed" ? "b".repeat(40) : null, + html_url: `https://github.com/${repo}/pull/7`, + }, + }; +} +const published = { + type: "pr_opened" as const, + repo, + prNumber: 7, + headSha: head, + ghUpdatedAt: "2030-01-01T00:00:00Z", + url: `https://github.com/${repo}/pull/7`, +}; + +describe("GitHub observations cannot assert delivery attribution", () => { + it.each(["webhook", "poller"])( + "observes a fork through %s without trusting its branch", + async (channel) => { + const { db, issue, agent, service } = setup(); + for (const action of ["opened", "closed"]) { + const body = payload(action); + let response: Response; + if (channel === "webhook") { + const raw = JSON.stringify(body); + response = await buildGithubWebhookRoutes(db, "test-secret").request("/webhooks/github", { + method: "POST", + headers: { + "content-type": "application/json", + "x-github-event": "pull_request", + "x-hub-signature-256": + "sha256=" + createHmac("sha256", "test-secret").update(raw).digest("hex"), + }, + body: raw, + }); + } else { + // The current poller does not include head.repo at all. Its credential + // authenticates the observation, not the PR author's issue declaration. + const pollerHead = { ref: body.pull_request.head.ref, sha: body.pull_request.head.sha }; + response = await buildApiRoutes(db).request("/github-events", { + method: "POST", + headers: { + authorization: `Bearer ${service.token}`, + "content-type": "application/json", + }, + body: JSON.stringify({ + event: "pull_request", + repo, + payload: { ...body, pull_request: { ...body.pull_request, head: pollerHead } }, + }), + }); + } + expect(response.status).toBe(200); + expect(findPrState(db, repo, 7)?.status).toBe(action === "opened" ? "open" : "merged"); + expect(findPrState(db, repo, 7)?.issueRef).toBeNull(); + expect(getOpenPr(db, issue.id)).toBeNull(); + expect(getMergedPr(db, issue.id)).toBeNull(); + expect( + listLiveLinkViews(db, issue.id).every( + (link) => link.role === "references" && !link.provesLanded, + ), + ).toBe(true); + } + expect(claimIssue(db, agent, issue.ref).issue.assigneeId).toBe(agent.id); + }, + ); + + it.each([true, false])( + "publishes against a legacy mixed-case repository binding (explicit repo: %s)", + (explicit) => { + const { db, service, issue } = setup(); + // Legacy rows predate normalized writes; both binding and observation + // identity must still converge without requiring an operator migration. + db.update(githubRepos).set({ fullName: "Acme/Widgets" }).run(); + recordDeliveryEvent(db, service.actor, issue.ref, { + ...published, + repo: explicit ? repo : undefined, + }); + expect(getOpenPr(db, issue.id)?.repo).toBe(repo); + expect(listLiveLinkViews(db, issue.id)[0].role).toBe("delivers"); + }, + ); + + it("preserves authenticated host publication, including a webhook that arrived first", () => { + const { db, issue, agent, service } = setup(); + handleGithubWebhook(db, "pull_request", payload()); + recordDeliveryEvent(db, service.actor, issue.ref, published); + expect(getOpenPr(db, issue.id)?.prNumber).toBe(7); + expect(() => claimIssue(db, agent, issue.ref)).toThrow(/open PR/); + handleGithubWebhook(db, "pull_request", payload("closed", "2030-01-01T00:01:00Z")); + expect(getMergedPr(db, issue.id)?.prNumber).toBe(7); + expect(listLiveLinkViews(db, issue.id)[0].provesLanded).toBe(true); + }); +}); + +describe("revocation survives observations and host publication retries", () => { + it.each(["delivers", "references"] as const)( + "preserves a revoked %s link until an explicit re-declaration", + (role) => { + const { db, human, issue, service } = setup(); + if (role === "delivers") recordDeliveryEvent(db, service.actor, issue.ref, published); + else handleGithubWebhook(db, "pull_request", payload()); + revokePrLink(db, human, issue.ref, { repo, prNumber: 7, reason: "Unrelated work" }); + for (const observation of [ + payload(), + payload("synchronize", "2030-01-01T00:01:00Z"), + payload("closed", "2030-01-01T00:02:00Z"), + ]) { + handleGithubWebhook(db, "pull_request", observation); + expect(listLiveLinkViews(db, issue.id)).toEqual([]); + expect(getOpenPr(db, issue.id)).toBeNull(); + expect(getMergedPr(db, issue.id)).toBeNull(); + } + recordDeliveryEvent(db, service.actor, issue.ref, published); + recordDeliveryEvent(db, service.actor, issue.ref, { + type: "delivered", + repo, + prNumber: 7, + mergeSha: "b".repeat(40), + deploy: { ran: false }, + }); + expect(listLiveLinkViews(db, issue.id)).toEqual([]); + declarePrLink(db, human, issue.ref, { repo, prNumber: 7 }); + expect(getMergedPr(db, issue.id)?.prNumber).toBe(7); + expect(listLiveLinkViews(db, issue.id)[0].provesLanded).toBe(true); + }, + ); +}); diff --git a/tests/services/pr-links.test.ts b/tests/services/pr-links.test.ts index b5318826..6f68c107 100644 --- a/tests/services/pr-links.test.ts +++ b/tests/services/pr-links.test.ts @@ -411,7 +411,7 @@ describe("the DoS the previous design died on", () => { expect(links[0].confirmedBy).toBeNull(); }); - it("the agent/ branch path still declares delivers — parity with today", () => { + it("the agent/ branch convention alone only suggests a reference", () => { const { db } = setup(); handleGithubWebhook(db, "pull_request", { action: "opened", @@ -427,10 +427,9 @@ describe("the DoS the previous design died on", () => { }); const links = listLiveLinks(db, 1); expect(links).toHaveLength(1); - expect(links[0].role).toBe("delivers"); - // Confirmed, matching the authority pr_state.issue_ref carries today — but - // by a non-human, so §5a recency binding still applies at the read sites. - expect(links[0].confirmedBy).not.toBeNull(); + expect(links[0].role).toBe("references"); + expect(links[0].confirmedBy).toBeNull(); + expect(getOpenPr(db, 1)).toBeNull(); }); it("ingestion records no timeline event — the link row is the audit", () => { diff --git a/tests/services/pr-observation.test.ts b/tests/services/pr-observation.test.ts index e0d9478b..a0544707 100644 --- a/tests/services/pr-observation.test.ts +++ b/tests/services/pr-observation.test.ts @@ -295,11 +295,12 @@ describe("the SYD-280 regression fence still holds (SYD-287)", () => { }, }); - it("an agent/ PR behaves exactly as before — one delivers link, one event, one row", () => { - const { db } = setup(); + it("a declared agent/ PR retains its attribution — one delivers link, one event, one row", () => { + const { db, human } = setup(); + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 12 }); const outcome = handleGithubWebhook(db, "pull_request", agentPr("opened")); expect(outcome).toEqual({ handled: true, ref: "SYD-1", type: "gh_pr_opened" }); - expect(findPrState(db, REPO, 12)).toMatchObject({ status: "open", issueRef: "SYD-1" }); + expect(findPrState(db, REPO, 12)).toMatchObject({ status: "open", issueRef: null }); const links = listLiveLinks(db, 1); expect(links).toHaveLength(1); expect(links[0].role).toBe("delivers"); @@ -349,6 +350,7 @@ describe("the SYD-280 regression fence still holds (SYD-287)", () => { it("still queues an agent/ PR — the guard bounds the queue, it does not empty it", () => { const { db, human } = setup(); + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 12 }); handleGithubWebhook(db, "pull_request", { action: "opened", repository: { full_name: REPO }, @@ -365,7 +367,7 @@ describe("the SYD-280 regression fence still holds (SYD-287)", () => { expect(pending[0].pin).toMatchObject({ prNumber: 12, repo: REPO }); }); - it("never writes issueRef from a link — that column stays branch-derived until it is dropped", () => { + it("never writes issueRef from a link — that column retains legacy data only", () => { const { db, human } = setup(); declare(db, human); handleGithubWebhook(db, "pull_request", featPr("opened")); diff --git a/tests/services/pr-state-cutover.test.ts b/tests/services/pr-state-cutover.test.ts index 5234c9ed..da579b09 100644 --- a/tests/services/pr-state-cutover.test.ts +++ b/tests/services/pr-state-cutover.test.ts @@ -1,3 +1,4 @@ +import { declarePrLink } from "../../src/services/pr-links.js"; // SYD-207 cutover invariants (spec: docs/2026-07-12-sync-simplification- // assessment.md Step 6): the backfill rides POST /api/github-events → // handleGithubWebhook, so these tests drive that exact ingestion and assert @@ -56,7 +57,8 @@ function prPayload( describe("search-vs-claim-gate agreement (SYD-207)", () => { it("the ?openPr= filter and the claim gate answer from the same oracle", () => { - const { db, agent } = setup(); + const { db, human, agent } = setup(); + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 41 }); handleGithubWebhook( db, "pull_request", diff --git a/tests/services/pr-state.test.ts b/tests/services/pr-state.test.ts index 36bd8537..e4a263a7 100644 --- a/tests/services/pr-state.test.ts +++ b/tests/services/pr-state.test.ts @@ -8,6 +8,8 @@ import { describe, it, expect } from "vitest"; import { openDb, type Db } from "../../src/db/index.js"; import { createActor } from "../../src/services/actors.js"; import { createProject } from "../../src/services/projects.js"; +import { declarePrLink } from "../../src/services/pr-links.js"; +import { getOpenPr } from "../../src/services/pr-status.js"; import { createIssue } from "../../src/services/issues.js"; import { getActivity } from "../../src/services/comments.js"; import { addGithubRepo } from "../../src/services/github-repos.js"; @@ -24,13 +26,16 @@ const T1 = "2026-07-12T10:00:00Z"; const T2 = "2026-07-12T11:00:00Z"; const T3 = "2026-07-12T12:00:00Z"; -function setup(opts: { bindRepo?: boolean } = {}) { +function setup(opts: { bindRepo?: boolean; declare?: boolean } = {}) { const db = openDb(":memory:"); const human = createActor(db, { name: "sean", type: "human" }).actor; const github = createActor(db, { name: "github", type: "agent" }).actor; createProject(db, human, { key: "SYD", name: "Switchyard" }); createIssue(db, human, { projectKey: "SYD", title: "Ship v1" }); if (opts.bindRepo !== false) addGithubRepo(db, human, { fullName: REPO, projectKey: "SYD" }); + if (opts.bindRepo !== false && opts.declare !== false) { + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 12 }); + } return { db, human, github }; } @@ -48,7 +53,7 @@ function obs(o: Partial = {}): PrObservation { } describe("upsertPrState / insert + attribution", () => { - it("inserts an open row from a first observation, attributed via branch + repo binding, and co-writes gh_pr_opened", () => { + it("inserts an open row from a first observation, attributed via an explicit human declaration, and co-writes gh_pr_opened", () => { const { db, github } = setup(); const outcome = upsertPrState(db, github, obs()); expect(outcome).toMatchObject({ applied: true, transition: "opened" }); @@ -59,7 +64,7 @@ describe("upsertPrState / insert + attribution", () => { prNumber: 12, status: "open", branch: "agent/SYD-1", - issueRef: "SYD-1", + issueRef: null, headSha: "a".repeat(40), url: "https://github.com/acme/widgets/pull/12", }); @@ -79,6 +84,7 @@ describe("upsertPrState / insert + attribution", () => { it("normalizes repo casing on write and read: link lowercase, observe canonical case, binding + pr_state both resolve (SYD-212)", () => { const { db, human, github } = setup({ bindRepo: false }); addGithubRepo(db, human, { fullName: REPO.toLowerCase(), projectKey: "SYD" }); + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 12 }); const canonicalRepo = "Acme/Widgets"; const outcome = upsertPrState(db, github, obs({ repo: canonicalRepo })); @@ -86,7 +92,8 @@ describe("upsertPrState / insert + attribution", () => { // Attribution succeeded (bound repo matched despite differing casing)... const rowByLower = findPrState(db, REPO.toLowerCase(), 12)!; - expect(rowByLower.issueRef).toBe("SYD-1"); + expect(getOpenPr(db, 1)?.prNumber).toBe(12); + expect(rowByLower.issueRef).toBeNull(); // ...and the stored row itself converged on one (lowercase) casing rather // than splitting into a second row under the canonical-case key. expect(rowByLower.repo).toBe("acme/widgets"); @@ -96,7 +103,7 @@ describe("upsertPrState / insert + attribution", () => { }); it("refuses attribution for an agent/ PR in a repo not bound to that ref's project (cross-repo), writing a display-only row and no event", () => { - const { db, human, github } = setup(); + const { db, human, github } = setup({ declare: false }); createProject(db, human, { key: "OTH", name: "Other" }); addGithubRepo(db, human, { fullName: "acme/other", projectKey: "OTH" }); @@ -109,7 +116,7 @@ describe("upsertPrState / insert + attribution", () => { }); it("never attributes from a non-agent branch (free-text scanning stays display-only)", () => { - const { db, github } = setup(); + const { db, github } = setup({ declare: false }); upsertPrState(db, github, obs({ branch: "feat/manual-work" })); expect(findPrState(db, REPO, 12)!.issueRef).toBeNull(); }); @@ -118,7 +125,8 @@ describe("upsertPrState / insert + attribution", () => { const { db, github } = setup(); upsertPrState(db, github, obs()); upsertPrState(db, github, obs({ branch: null, ghUpdatedAt: T2 })); - expect(findPrState(db, REPO, 12)!.issueRef).toBe("SYD-1"); + expect(getOpenPr(db, 1)?.prNumber).toBe(12); + expect(findPrState(db, REPO, 12)!.issueRef).toBeNull(); }); it("heals a PR first observed already merged (never-saw-open), co-writing gh_pr_merged", () => { diff --git a/tests/services/pr-status.test.ts b/tests/services/pr-status.test.ts index 297ae3ed..d2ae41ec 100644 --- a/tests/services/pr-status.test.ts +++ b/tests/services/pr-status.test.ts @@ -6,6 +6,8 @@ import { createIssue, getIssue } from "../../src/services/issues.js"; import { addGithubRepo } from "../../src/services/github-repos.js"; import { recordDeliveryEvent } from "../../src/services/delivery-events.js"; import { recordEvent } from "../../src/services/events.js"; +import { declarePrLink, listLiveLinks } from "../../src/services/pr-links.js"; +import type { Actor } from "../../src/services/actors.js"; import { upsertPrState } from "../../src/services/pr-state.js"; import { getOpenPr, @@ -28,13 +30,19 @@ function setup() { /** An attributed observation for agent/ — the shape every pr_state * writer (webhook, poller, publish, delivery, backfill) converges on. */ -function observe( +function declaredObservation( + db: ReturnType, + human: Actor, ref: string, prNumber: number, status: "open" | "merged" | "closed", ghUpdatedAt: string, extra: { reopened?: boolean } = {}, ) { + const issue = getIssue(db, ref); + if (!listLiveLinks(db, issue.id).some((l) => l.repo === REPO && l.prNumber === prNumber)) { + declarePrLink(db, human, ref, { repo: REPO, prNumber }); + } return { repo: REPO, prNumber, @@ -54,7 +62,11 @@ describe("getOpenPr (pr_state-derived, SYD-207)", () => { it("flags an issue with an open attributed row", () => { const { db, human } = setup(); - upsertPrState(db, human, observe("SYD-1", 41, "open", "2026-07-13T10:00:00Z")); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 41, "open", "2026-07-13T10:00:00Z"), + ); expect(getOpenPr(db, getIssue(db, "SYD-1").id)).toEqual({ prNumber: 41, url: `https://github.com/${REPO}/pull/41`, @@ -66,7 +78,7 @@ describe("getOpenPr (pr_state-derived, SYD-207)", () => { it("carries repo and headSha (SYD-208)", () => { const { db, human } = setup(); upsertPrState(db, human, { - ...observe("SYD-1", 41, "open", "2026-07-13T10:00:00Z"), + ...declaredObservation(db, human, "SYD-1", 41, "open", "2026-07-13T10:00:00Z"), headSha: "abc123", }); expect(getOpenPr(db, getIssue(db, "SYD-1").id)).toEqual({ @@ -80,21 +92,39 @@ describe("getOpenPr (pr_state-derived, SYD-207)", () => { it("clears when the row goes merged or closed", () => { const { db, human } = setup(); const issueId = getIssue(db, "SYD-1").id; - upsertPrState(db, human, observe("SYD-1", 41, "open", "2026-07-13T10:00:00Z")); - upsertPrState(db, human, observe("SYD-1", 41, "merged", "2026-07-13T11:00:00Z")); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 41, "open", "2026-07-13T10:00:00Z"), + ); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 41, "merged", "2026-07-13T11:00:00Z"), + ); expect(getOpenPr(db, issueId)).toBeNull(); }); it("flags again after a genuine reopen", () => { const { db, human } = setup(); const issueId = getIssue(db, "SYD-1").id; - upsertPrState(db, human, observe("SYD-1", 41, "open", "2026-07-13T10:00:00Z")); - upsertPrState(db, human, observe("SYD-1", 41, "closed", "2026-07-13T11:00:00Z")); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 41, "open", "2026-07-13T10:00:00Z"), + ); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 41, "closed", "2026-07-13T11:00:00Z"), + ); expect(getOpenPr(db, issueId)).toBeNull(); upsertPrState( db, human, - observe("SYD-1", 41, "open", "2026-07-13T12:00:00Z", { reopened: true }), + declaredObservation(db, human, "SYD-1", 41, "open", "2026-07-13T12:00:00Z", { + reopened: true, + }), ); expect(getOpenPr(db, issueId)?.prNumber).toBe(41); }); @@ -131,9 +161,21 @@ describe("getOpenPr (pr_state-derived, SYD-207)", () => { it("a belated close for an old PR can't hide a newer still-open PR (SYD-125 shape)", () => { const { db, human } = setup(); const issueId = getIssue(db, "SYD-1").id; - upsertPrState(db, human, observe("SYD-1", 1, "open", "2026-07-13T09:00:00Z")); - upsertPrState(db, human, observe("SYD-1", 2, "open", "2026-07-13T10:00:00Z")); - upsertPrState(db, human, observe("SYD-1", 1, "closed", "2026-07-13T11:00:00Z")); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 1, "open", "2026-07-13T09:00:00Z"), + ); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 2, "open", "2026-07-13T10:00:00Z"), + ); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 1, "closed", "2026-07-13T11:00:00Z"), + ); expect(getOpenPr(db, issueId)).toEqual({ prNumber: 2, url: `https://github.com/${REPO}/pull/2`, @@ -168,7 +210,11 @@ describe("listOpenPrByIssueId (pr_state-derived, SYD-207)", () => { it("only includes issues with an open attributed row", () => { const { db, human } = setup(); createIssue(db, human, { projectKey: "SYD", title: "Also shipping" }); // SYD-2 - upsertPrState(db, human, observe("SYD-1", 41, "open", "2026-07-13T10:00:00Z")); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 41, "open", "2026-07-13T10:00:00Z"), + ); const open = getIssue(db, "SYD-1"); const clean = getIssue(db, "SYD-2"); @@ -184,8 +230,16 @@ describe("listOpenPrByIssueId (pr_state-derived, SYD-207)", () => { it("keeps the newest PR when an issue somehow has two open rows", () => { const { db, human } = setup(); - upsertPrState(db, human, observe("SYD-1", 41, "open", "2026-07-13T10:00:00Z")); - upsertPrState(db, human, observe("SYD-1", 55, "open", "2026-07-13T09:00:00Z")); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 41, "open", "2026-07-13T10:00:00Z"), + ); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 55, "open", "2026-07-13T09:00:00Z"), + ); expect(listOpenPrByIssueId(db).get(getIssue(db, "SYD-1").id)?.prNumber).toBe(55); }); }); @@ -199,9 +253,13 @@ describe("getMergedPr (pr_state-derived, SYD-207)", () => { it("returns prNumber + the co-written transition event id", () => { const { db, human } = setup(); const issueId = getIssue(db, "SYD-1").id; - upsertPrState(db, human, observe("SYD-1", 41, "open", "2026-07-13T10:00:00Z")); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 41, "open", "2026-07-13T10:00:00Z"), + ); upsertPrState(db, human, { - ...observe("SYD-1", 41, "merged", "2026-07-13T11:00:00Z"), + ...declaredObservation(db, human, "SYD-1", 41, "merged", "2026-07-13T11:00:00Z"), mergeSha: "abc123", }); const merged = getMergedPr(db, issueId); @@ -212,13 +270,22 @@ describe("getMergedPr (pr_state-derived, SYD-207)", () => { it("returns the most recently merged PR when several exist", () => { const { db, human } = setup(); const issueId = getIssue(db, "SYD-1").id; - upsertPrState(db, human, observe("SYD-1", 41, "merged", "2026-07-13T10:00:00Z")); - upsertPrState(db, human, observe("SYD-1", 42, "merged", "2026-07-13T12:00:00Z")); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 41, "merged", "2026-07-13T10:00:00Z"), + ); + upsertPrState( + db, + human, + declaredObservation(db, human, "SYD-1", 42, "merged", "2026-07-13T12:00:00Z"), + ); expect(getMergedPr(db, issueId)?.prNumber).toBe(42); }); it("the delivery worker's merge (recordDeliveryEvent delivered) is visible", () => { const { db, human } = setup(); + declarePrLink(db, human, "SYD-1", { repo: REPO, prNumber: 41 }); const issueId = getIssue(db, "SYD-1").id; recordDeliveryEvent(db, human, "SYD-1", { type: "delivered", @@ -241,15 +308,15 @@ describe("deliveryPinFor (SYD-208)", () => { const { db, human } = setup(); const issueId = getIssue(db, "SYD-1").id; upsertPrState(db, human, { - ...observe("SYD-1", 40, "closed", "2026-07-13T09:00:00Z"), + ...declaredObservation(db, human, "SYD-1", 40, "closed", "2026-07-13T09:00:00Z"), headSha: "sha-closed", }); upsertPrState(db, human, { - ...observe("SYD-1", 41, "merged", "2026-07-13T10:00:00Z"), + ...declaredObservation(db, human, "SYD-1", 41, "merged", "2026-07-13T10:00:00Z"), headSha: "sha-merged", }); upsertPrState(db, human, { - ...observe("SYD-1", 42, "open", "2026-07-13T08:00:00Z"), + ...declaredObservation(db, human, "SYD-1", 42, "open", "2026-07-13T08:00:00Z"), headSha: "sha-open", }); expect(deliveryPinFor(db, issueId)).toEqual({ @@ -264,11 +331,11 @@ describe("deliveryPinFor (SYD-208)", () => { const { db, human } = setup(); const issueId = getIssue(db, "SYD-1").id; upsertPrState(db, human, { - ...observe("SYD-1", 40, "closed", "2026-07-13T09:00:00Z"), + ...declaredObservation(db, human, "SYD-1", 40, "closed", "2026-07-13T09:00:00Z"), headSha: "sha-closed", }); upsertPrState(db, human, { - ...observe("SYD-1", 41, "merged", "2026-07-13T10:00:00Z"), + ...declaredObservation(db, human, "SYD-1", 41, "merged", "2026-07-13T10:00:00Z"), headSha: "sha-merged", }); expect(deliveryPinFor(db, issueId)).toEqual({ @@ -283,7 +350,7 @@ describe("deliveryPinFor (SYD-208)", () => { const { db, human } = setup(); const issueId = getIssue(db, "SYD-1").id; upsertPrState(db, human, { - ...observe("SYD-1", 40, "closed", "2026-07-13T09:00:00Z"), + ...declaredObservation(db, human, "SYD-1", 40, "closed", "2026-07-13T09:00:00Z"), headSha: "sha-closed", }); expect(deliveryPinFor(db, issueId)).toEqual({ diff --git a/tests/services/service-actor.test.ts b/tests/services/service-actor.test.ts index f9dfebff..3088e069 100644 --- a/tests/services/service-actor.test.ts +++ b/tests/services/service-actor.test.ts @@ -8,7 +8,11 @@ import { } from "../../src/services/actors.js"; import { createProject } from "../../src/services/projects.js"; import { createIssue, updateIssue } from "../../src/services/issues.js"; -import { addDependency, removeDependency } from "../../src/services/dependencies.js"; +import { + addDependency, + removeDependency, + listDependencies, +} from "../../src/services/dependencies.js"; import { addComment } from "../../src/services/comments.js"; import { recordDeliveryEvent } from "../../src/services/delivery-events.js"; import { @@ -146,6 +150,13 @@ describe("service actor — DENIED all issue create/modify (fail-closed)", () => }); describe("service actor — DENIED (config / dependencies / tokens)", () => { + it("cannot add a dependency or emit a blocking event", () => { + const before = listIssueEvents(db, getIssue(db, "AIPI-2").id).length; + expect(() => addDependency(db, service, "AIPI-1", "AIPI-2")).toThrow(/cannot add dependencies/); + expect(listDependencies(db, "AIPI-2").blockedBy).toEqual([]); + expect(listIssueEvents(db, getIssue(db, "AIPI-2").id)).toHaveLength(before); + }); + it("cannot remove a dependency", () => { addDependency(db, human, "AIPI-1", "AIPI-2"); expect(() => removeDependency(db, service, "AIPI-1", "AIPI-2")).toThrowError(/only humans/i); From 7b7f763111f335b88173450c27a1bd16e22a4975 Mon Sep 17 00:00:00 2001 From: Sean Perkins <1733750+seanperkins@users.noreply.github.com> Date: Sun, 27 Sep 2026 22:33:16 -0400 Subject: [PATCH 2/8] fix: bind review actions and retries to displayed issue state Addresses #250, #251, #258, #259. --- ui/src/Composer.tsx | 3 + ui/src/PrLinks.tsx | 10 +- ui/src/usePasteUpload.ts | 5 +- ui/src/views/IssueDetail.tsx | 9 +- ui/src/views/NewIssue.tsx | 123 ++++++--- ui/src/views/Review.tsx | 385 +++++++++++++++++------------ ui/src/views/reviewSafety.test.tsx | 380 ++++++++++++++++++++++++++++ 7 files changed, 705 insertions(+), 210 deletions(-) create mode 100644 ui/src/views/reviewSafety.test.tsx diff --git a/ui/src/Composer.tsx b/ui/src/Composer.tsx index 30a94600..f9aa7f82 100644 --- a/ui/src/Composer.tsx +++ b/ui/src/Composer.tsx @@ -11,12 +11,14 @@ export function Composer({ placeholder, paste, children, + disabled = false, }: { value: string; onChange: (value: string) => void; placeholder: string; paste: ReturnType | ReturnType; children?: ReactNode; + disabled?: boolean; }) { const { onPaste, uploading, uploadError, setUploadError, textareaRef } = paste; return ( @@ -28,6 +30,7 @@ export function Composer({ )}