From e51641d69ca269c664f218a59b57ffc31443a1a4 Mon Sep 17 00:00:00 2001 From: Paris <202901147+ArchitectOvPan@users.noreply.github.com> Date: Sat, 29 Aug 2026 17:31:36 -0700 Subject: [PATCH] fix(governance): stop an empty issue body stranding an approved plan githubRequestContext stores empty and whitespace-only issue text as null, while a live GET /issues/:number read returns "" for a body cleared after creation. The Builder plan gate digests one on each side, so a project on builderPlanPolicy "required" refused the dispatch as builder_plan_stale against a revision that never moved, and the denial audit blamed an issue edit that never happened. Canonicalize in githubIssueRevisionContext so both producers agree. Both paths already funnel through it, so this leaves the stored shape of run.trigger.request untouched and keeps the digest producer-independent. The existing tests could not catch this: each exercised one producer, and the integration fixture reused the recorded digest as its own observed freshness evidence, making the comparison true by construction. The fixture now derives each side the way production does. --- services/api/src/github/issue-revision.ts | 12 +++- .../builder-plan-policy.integration.test.ts | 65 ++++++++++++++++--- .../api/test/github-issue-revision.test.ts | 48 ++++++++++++++ 3 files changed, 114 insertions(+), 11 deletions(-) diff --git a/services/api/src/github/issue-revision.ts b/services/api/src/github/issue-revision.ts index 96596628..7d452f4f 100644 --- a/services/api/src/github/issue-revision.ts +++ b/services/api/src/github/issue-revision.ts @@ -110,8 +110,18 @@ function issueLabels(value: unknown) { ].sort(); } +/** + * Fold every representation of "no text" onto `null` before it reaches the + * digest. The producers disagree about which one they emit for the same issue: + * `githubRequestContext` stores empty or whitespace-only text as `null`, while + * a live `GET /issues/:number` read returns `""` for a body that was cleared + * after creation. A revision digest has to describe the issue, not the producer + * that observed it, so both spellings have to canonicalize the same way. + */ function normalizedText(value: unknown): string | null { - return typeof value === "string" ? value.replace(/\r\n?/g, "\n") : null; + if (typeof value !== "string") return null; + const normalized = value.replace(/\r\n?/g, "\n"); + return normalized.trim() ? normalized : null; } function stringValue(value: unknown) { diff --git a/services/api/test/builder-plan-policy.integration.test.ts b/services/api/test/builder-plan-policy.integration.test.ts index 223f3ea6..4a971bef 100644 --- a/services/api/test/builder-plan-policy.integration.test.ts +++ b/services/api/test/builder-plan-policy.integration.test.ts @@ -25,9 +25,12 @@ import { withBuilderPlanPreflight, } from "../src/builder-plan-policy.js"; import { ApiError } from "../src/errors.js"; -import { githubIssueRevisionSha256 } from "../src/github/issue-revision.js"; +import { + githubIssueRevisionContext, + githubIssueRevisionSha256, +} from "../src/github/issue-revision.js"; import { syncRepoFacilityConfig } from "../src/github/kickstart.js"; -import { routeTrigger, type TriggerPayload } from "../src/github/router.js"; +import { githubRequestContext, routeTrigger, type TriggerPayload } from "../src/github/router.js"; import { dispatchRun } from "../src/sandbox/orchestrator.js"; import type { AppConfig } from "../src/types.js"; @@ -134,6 +137,35 @@ describe("builder plan policy integration", async () => { ).rejects.toMatchObject({ code: "builder_plan_already_consumed" }); }); + it("admits a fresh plan whose issue body GitHub reports as empty rather than absent", async () => { + // An issue body cleared after creation reads back as `""` from + // `GET /issues/:number`, while `githubRequestContext` stored the same issue + // with `body: null`. Nothing about the issue changed between the Architect + // run and this dispatch, so the required gate has to admit it rather than + // report `builder_plan_stale` against a revision that never moved. + const fixture = await canonicalFixture({ issueBody: "" }); + await expect(assertBuilderPlanDispatch(db, fixture.dispatch)).resolves.toEqual({ + mode: "builder", + isBuilder: true, + }); + }); + + it("still denies an empty-bodied issue whose live revision moved", async () => { + const fixture = await canonicalFixture({ issueBody: "" }); + const before = await projectRuns(fixture.orgId, fixture.projectId); + await expect( + assertBuilderPlanDispatch(db, { + ...fixture.dispatch, + freshnessEvidence: { + ...fixture.dispatch.freshnessEvidence, + issueRevisionSha256: createHash("sha256").update("moved").digest("hex"), + }, + }), + ).rejects.toMatchObject({ code: "builder_plan_stale" }); + expect(await projectRuns(fixture.orgId, fixture.projectId)).toHaveLength(before.length); + await expect(lastDenialCode(fixture.orgId)).resolves.toBe("builder_plan_stale"); + }); + it("recognizes a Builder by agentDefId when the run mode is a surface alias", async () => { const fixture = await canonicalFixture(); const contractId = newId("item"); @@ -922,7 +954,9 @@ describe("builder plan policy integration", async () => { await expect(lastDenialCode(fixture.orgId)).resolves.toBe(expected); }); - async function canonicalFixture(options: { state?: string; expiresAt?: Date } = {}): Promise<{ + async function canonicalFixture( + options: { state?: string; expiresAt?: Date; issueBody?: string | null } = {}, + ): Promise<{ orgId: string; projectId: string; proposalId: string; @@ -941,17 +975,28 @@ describe("builder plan policy integration", async () => { const actionTypeId = `act_plan_${suffix}`; const baseSha = "a".repeat(40); const issueUrl = `https://github.test/facility-test/plan-${suffix}/issues/204`; - const issueRequest = { + // Derive each side of the freshness comparison the way production does + // rather than sharing one literal digest between them: `trigger.request` is + // whatever `githubRequestContext` stored at Architect dispatch, while the + // Builder gate re-derives its digest from a live `GET /issues/:number` read. + // Reusing a single value here would hide any disagreement between the two. + const liveIssue = { + number: 204, title: "Require a plan", - body: "Implement it", + body: options.issueBody === undefined ? "Implement it" : options.issueBody, state: "open", - author: "requester", - url: issueUrl, + user: { login: "requester" }, labels: [], - comments: [], + html_url: issueUrl, }; + const issueRequest = githubRequestContext({ issue: liveIssue }, []); const issueRevisionSha256 = githubIssueRevisionSha256(issueRequest); - if (!issueRevisionSha256) throw new Error("issue revision fixture missing"); + const liveIssueRevisionSha256 = githubIssueRevisionSha256( + githubIssueRevisionContext(liveIssue, []), + ); + if (!issueRevisionSha256 || !liveIssueRevisionSha256) { + throw new Error("issue revision fixture missing"); + } const plan = "Implement the reviewed change and run the named checks."; const planSha256 = createHash("sha256").update(plan).digest("hex"); const decidedAt = new Date(); @@ -1109,7 +1154,7 @@ describe("builder plan policy integration", async () => { source: "integration_test", freshnessEvidence: { baseSha, - issueRevisionSha256, + issueRevisionSha256: liveIssueRevisionSha256, checkedAt: new Date().toISOString(), }, }, diff --git a/services/api/test/github-issue-revision.test.ts b/services/api/test/github-issue-revision.test.ts index 4d3597d9..cb062262 100644 --- a/services/api/test/github-issue-revision.test.ts +++ b/services/api/test/github-issue-revision.test.ts @@ -3,6 +3,7 @@ import { githubIssueRevisionContext, githubIssueRevisionSha256, } from "../src/github/issue-revision.js"; +import { githubRequestContext } from "../src/github/router.js"; describe("GitHub issue revision", () => { const issue = { @@ -93,3 +94,50 @@ describe("GitHub issue revision", () => { ).not.toBe(baseline); }); }); + +describe("GitHub issue revision producers agree", () => { + // Two different producers feed the same digest. At Architect dispatch the + // router stores `githubRequestContext(...)` in `run.trigger.request`, and + // `ensureArchitectPlanAcceptance` seals its digest into the proposal payload. + // At Builder dispatch `resolveBuilderPlanFreshnessForProposal` re-derives the + // digest from a live `GET /issues/:number` read. A project on + // `builderPlanPolicy: "required"` compares the two, so they have to agree + // whenever the issue itself did not change. + const issueWithBody = (body: string | null) => ({ + number: 204, + title: "Keep sources consistent", + body, + state: "open", + user: { login: "requester" }, + labels: [{ name: "frontend" }, "delivery"], + html_url: "https://github.test/theam/aifindr-ui/issues/116", + }); + type LiveIssue = ReturnType; + + /** What Facility sealed into the proposal at Architect time. */ + const storedDigest = (issue: LiveIssue) => + githubIssueRevisionSha256(githubRequestContext({ issue }, [])); + + /** What the Builder gate observes from GitHub at dispatch time. */ + const liveDigest = (issue: LiveIssue) => + githubIssueRevisionSha256(githubIssueRevisionContext(issue, [])); + + it.each([ + ["absent since creation", null], + ["cleared after creation", ""], + ["spaces only", " "], + ["a newline only", "\n"], + ["CRLF only", "\r\n"], + ["ordinary prose", "Apply the same rule on every surface."], + ])("digests an unchanged issue whose body is %s identically on both sides", (_case, body) => { + const issue = issueWithBody(body); + expect(liveDigest(issue)).toBe(storedDigest(issue)); + }); + + it("still detects an issue that gained or lost material body text", () => { + const empty = issueWithBody(""); + const filled = issueWithBody("A changed scope."); + expect(liveDigest(filled)).not.toBe(storedDigest(empty)); + expect(liveDigest(empty)).not.toBe(storedDigest(filled)); + }); +});