Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion services/api/src/github/issue-revision.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
65 changes: 55 additions & 10 deletions services/api/test/builder-plan-policy.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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;
Expand All @@ -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();
Expand Down Expand Up @@ -1109,7 +1154,7 @@ describe("builder plan policy integration", async () => {
source: "integration_test",
freshnessEvidence: {
baseSha,
issueRevisionSha256,
issueRevisionSha256: liveIssueRevisionSha256,
checkedAt: new Date().toISOString(),
},
},
Expand Down
48 changes: 48 additions & 0 deletions services/api/test/github-issue-revision.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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<typeof issueWithBody>;

/** 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));
});
});