Skip to content

fix(governance): stop an empty issue body stranding an approved plan - #215

Open
ArchitectOvPan wants to merge 1 commit into
theam:mainfrom
ArchitectOvPan:fix/issue-revision-empty-body
Open

fix(governance): stop an empty issue body stranding an approved plan#215
ArchitectOvPan wants to merge 1 commit into
theam:mainfrom
ArchitectOvPan:fix/issue-revision-empty-body

Conversation

@ArchitectOvPan

@ArchitectOvPan ArchitectOvPan commented Aug 30, 2026

Copy link
Copy Markdown

What changes

githubIssueRevisionContext now folds empty and whitespace-only text onto
null before the issue-revision digest is taken, matching what
githubRequestContext already stores. Both producers of that digest now agree,
so a project on builderPlanPolicy: "required" stops reporting
builder_plan_stale for an issue that never changed.

Why

The Builder gate compares two digests that are produced by two different pieces
of code, and they canonicalize "this issue has no body" differently.

Write side — at Architect dispatch the router stores the issue snapshot in
run.trigger.request via githubRequestContext
(router.ts:733-734), which uses
nullableText (router.ts:781-783):

return typeof value === "string" && value.trim() ? value : null;

ensureArchitectPlanAcceptance seals the digest of that snapshot into the
proposal payload
(orchestrator.ts:948).

Read side — at Builder dispatch resolveBuilderPlanFreshnessForProposal
re-derives the digest from a live GET /issues/:number
(builder-plan-freshness.ts:110-112),
which goes through normalizedText
(issue-revision.ts:113-115):

return typeof value === "string" ? value.replace(/\r\n?/g, "\n") : null;

nullableText("") is null. normalizedText("") is "". JSON.stringify
sees two different objects, so the two digests differ for an issue nobody
touched, and the gate refuses the dispatch at
builder-plan-policy.ts:629-630:

if (currentBaseSha !== expectedBaseSha || currentIssueRevision !== expectedIssueRevision) {
  return invalid("builder_plan_stale", "base_or_issue_revision_changed");
}

The write side collapses null, undefined, "", " ", "\n", "\t" and
" \n " onto one digest. The read side gives each of them a different one.
null is the only representation that lands in the same place on both sides,
so an issue with no body at all is fine and an issue whose body is empty or
whitespace-only can never dispatch.

Reproduction

  1. Set a project to builderPlanPolicy: "required".
  2. Give the issue a body that is present but blank — a single space is enough
    (PATCH /issues/:n with body: " "). GitHub stores and returns it verbatim.
  3. Comment /architect. The run succeeds and publishes a plan.
  4. Approve it and comment /builder.

Expected: the Builder run starts. Actual: 409 builder_plan_stale, reason
base_or_issue_revision_changed, and a run.builder_plan_denied audit event —
against a revision that never moved. Re-running does not help; the stored
digest can never be reproduced from a live read, so the plan is stranded for the
life of the proposal. The audit trail attributes the refusal to a change in the
issue, which sends whoever investigates to look for an edit that did not happen.

The added integration test reproduces exactly this through
assertBuilderPlanDispatch; without the source change it fails with
{ statusCode: 409, code: 'builder_plan_stale', details: { reason: 'base_or_issue_revision_changed' } }.

Why the suite did not catch it

Both existing tests exercise one side each, and never compare them:

That last one is the reason this could not surface: the integration test proved
the gate compares two values, not that the two producers agree. This PR rewires
the fixture to derive each side the way production does — githubRequestContext
for the stored digest, githubIssueRevisionContext for the live one. The change
is digest-neutral for the existing cases, so every other assertion in that file
is unaffected.

The fix

Canonicalizing in githubIssueRevisionContext rather than changing
nullableText is deliberate:

  • Both paths already funnel through githubIssueRevisionContext
    (githubIssueRevisionSha256 re-applies it to whatever it is handed), so one
    change fixes both without touching the stored shape of run.trigger.request,
    which the runner also reads as the end-user request.
  • It keeps the digest producer-independent, which is the property the gate
    depends on. Any future producer gets the same canonical form for free.
  • It stays idempotent, which githubIssueRevisionSha256 relies on when it
    re-canonicalizes an already-canonical context.

The security property is unchanged: whitespace-only and empty bodies are
equivalent to each other, and any body that gains or loses material text still
moves the digest. There are explicit tests for both directions.

Effect on stored digests

Digests are recomputed on both sides at dispatch, so nothing persisted needs a
migration. The only proposals whose digest changes meaning are ones whose issue
body is empty or whitespace-only — exactly the ones that could never dispatch
before. A proposal sealed before this change against a whitespace-only body
keeps its stored digest and will now match, because the live read canonicalizes
to the same null.

Two related divergences I did not change

Both are unreachable through the GitHub API, so fixing them would be speculative:

  • A label name with surrounding whitespace canonicalizes as "bug" on the write
    side (router.ts:743 trims) and
    "bug " on the read side (issueLabels does not). GitHub trims label names on
    create and rename, so it cannot occur.
  • materialIssueComment
    (issue-revision.ts:95) reads
    comment.authorType.toLowerCase() and would throw on a comment without that
    field. FacilityGithubClient.listIssueComments
    (client.ts:875) defaults it to
    "User", so no live path can produce one.

Happy to fold either in if you would rather have the canonicalizer total.

Verification

  • pnpm verify passes locally
  • Behaviour verified beyond the test suite (see below)
  • Documentation updated, or no user-facing change — no user-facing surface changed

What I ran, and what it proved:

  • vitest run test/github-issue-revision.test.ts — 9 passed with the fix.
    Reverting only issue-revision.ts fails 4 of them, so the new cases are a real
    regression test and not a restatement of current behaviour.
  • vitest run test/builder-plan-policy.integration.test.ts against Postgres 16 —
    32 passed. Reverting only issue-revision.ts fails
    admits a fresh plan whose issue body GitHub reports as empty rather than absent
    with the exact production error.
  • Full @facility/api suite against Postgres — 591 passed. Three failures
    (api.test.ts audit-chain and readiness-doctor, github.test.ts review agent)
    reproduce identically on a clean checkout of main in this environment and
    are unrelated to this change; they need seeded registry and installation
    fixtures my local database does not have.
  • tsc --noEmit for @facility/api, biome check . over 422 files, and
    node guards/run.mjs all clean.
  • Differential harness over both producers across 30 issue shapes: 6 divergences
    before the change, 2 after — the two documented above as unreachable.

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.
@ArchitectOvPan
ArchitectOvPan force-pushed the fix/issue-revision-empty-body branch from 5b3513f to e51641d Compare August 30, 2026 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant