fix(security): make the server GitHub writer guard fail closed and refuse redacted identity fields (PEN-3391)

[allyblockcast]

Closes PEN-3391. Follow-up to PEN-3157 / #1754 — the two Suggestions Ally left on the merged head f097134f, neither of which was addressed before merge.

Both were filed as latent, not live. I re-verified that against master at 1920eae5 before starting, and it still holds: nothing here is an incident response, and no leak has occurred.

Thinking Path

• The writer predicate's bug is its direction, not its vocabulary. The old derivation asked "can I see a write here?" — source.includes("ghFetch(") plus /method:\s*"(?:POST|PATCH|PUT|DELETE)"/. Both clauses fail open: an unrecognised writer is silently classified as a read, and a file the guard does not classify is a file the guard never checks. Chasing the missing spellings ('POST', `POST`, "post", a variable, a shorthand) would have lost that race permanently — the next spelling is always one more than the list. So I inverted it to "can I prove this is not a write?" and treat everything else as a writer.
• The more reachable gap was clause 1, not the method regex. The row frames the fail-open around the method spelling, but services/github-external-object-provider.ts imports ghFetch, aliases it (const fetchImpl = opts.fetch ?? ghFetch) and calls it through the alias — so it contains no ghFetch( substring at all and was excluded before the method test was ever reached. It is a read today. I verified the consequence rather than asserting it: adding method: "POST" to that one real line leaves the file with a genuine unscrubbed POST that the old predicate classifies as not a writer, i.e. it would have shipped with both guards green. That measurement is in Verification below.
• context is an identity field, and neither option the row offered is free. Exempting it (option a) restores the leak — a commit-status context is public on a public repo. Scrubbing it consistently (option b) publishes the status under a name nothing looks up: branch protection keeps waiting for a context that never arrives, and the outbox keys its upsert on the unredacted value it was handed. That trades a leak for a silent gate-liveness failure, which is the worse failure mode because the control stays green. So I took a third option — refuse the write when the identity would change. That keeps the leak closed and makes publish/lookup agreement structural rather than coincidental: the write proceeds only when the scrub is a byte-for-byte no-op, so a caller keying on the raw value is provably keying on what was published.
• PEN-3157 had already reached this conclusion at the enqueue boundary — it declined to scrub context there because doing so "would risk the delivery identity". The same argument holds at the send boundary; it just bites later. The code did both things and justified only one. This records the argument in the code so the split is no longer unargued.
• githubPostCheckRun's name is the identical defect and is in the same function family, so I fixed it in the same pass rather than leaving an exact re-file for later. A check-run name is what a required check is matched on, exactly like a status context.
• A green test I wrote proves nothing until I have seen it fail, so every claim below is mutation-tested, including against the real file rather than only synthetic fixtures.

What Changed

1. One shared, fail-closed writer derivation — new server/src/__tests__/helpers/github-writer-derivation.ts.

The predicate was duplicated byte-for-byte in two guards and widened by hand in both on #1754; it was one edit from diverging. There is now a single copy, consumed by github-write-egress-scrub.test.ts and github-egress-outbound-coverage.test.ts.

• Candidate set widened from a literal ghFetch( call to any \bghFetch\b reference, so an importer that only ever calls through an alias is in scope.
• Method test inverted from an allowlist of mutating spellings to an allowlist of read spellings: a candidate is read-only only when every mention of the word method is a quoted safe-verb literal (GET/HEAD/OPTIONS, any quote style, either case).

This is deliberately fail-closed, and the cost is false positives — a candidate that merely says "method" in a comment is classified as a writer and must carry the scrub. A false positive is one legible test failure naming the file; a false negative is an unscrubbed credential on a public commit status. It costs nothing today: of the nine files under server/src that reference ghFetch, the word method appears in exactly the two that do write, and both already scrub.

2. Identity fields are refused, not redacted — new gitHubIdentityFieldRedaction in github-app-auth.ts, applied to the commit-status context and the check-run name, and to the one writer outside the shared helper (github-review-gate-authority.ts). Prose fields (description, title, summary, body, target_url) are unchanged and still redact-and-proceed. The refusal is non-retryable by construction — the same input scrubs the same way every time — and the outbox already branches on retryable (github-status-delivery-outbox.ts:470), so it takes the permanent-failure arm rather than looping.

Scope note

The walk now skips server/src/__tests__/ — otherwise the derivation helper, which quotes its own regexes, would classify itself as an unscrubbed writer. That exclusion is only sound while nothing in the running server imports from there, so it is asserted in both guards, not assumed.

Verification

Run against borrowed /app deps with a globalSetup-free config (the standard recipe for this repo from a Penstock seat).

Suites — all green. 598 tests across 15 suites (every github* / *review-gate* suite in server/src/__tests__, plus the new one).

Suite set	Result
helpers/github-writer-derivation.test.ts (new fail-first suite)	17 passed
5 direct-caller suites, incl. DB-backed github-status-delivery-outbox	156 passed
9 remaining github* / *review-gate* suites, incl. github-webhook	425 passed

node scripts/check-test-undefined-symbols.mjs → ok no undefined identifiers in server tests.

Mutation tests — each claim broken, and seen to fail. A predicate that matches nothing returns the same empty set as full coverage, so the guards' own positive control cannot establish this.

Mutation	Result
Add method: "POST" to the real aliased call in github-external-object-provider.ts	Both guards fail, naming the file: services/github-external-object-provider.ts writes to GitHub without reaching the egress scrub
…and the old predicate against that same mutated real file	classifies it as not a writer — it would have shipped unchecked
Restore scrubOutboundGitHubText(input.context, …) (the "make it consistent" refactor)	2 tests fail; the status publishes {ok:true, statusCode:201} under a redacted context — the gate-liveness failure, reproduced
Exempt context entirely (option a)	2 tests fail

The new fail-first suite asserts each widened shape twice — that the pre-PEN-3391 predicate (kept verbatim as a fixture) misses it, and that the new one catches it — so a future narrowing fails with the shape named.

Not established here: local tsc is not meaningful in this sandbox (@types/node is absent), so typecheck is inconclusive from my seat — Build and Typecheck on this PR are authoritative. No new credential-shaped literals are introduced; the two test fixtures reused (FAKE_GITHUB_PAT, FAKE_AWS_KEY_ID) are the file's existing assembled-from-parts constants.

Risks

• Behaviour change, narrow but real: a commit status or check-run whose identity field matches a detector is now refused instead of published-redacted. Unreachable today — every context in the repo is a fixed operator-set literal, and I re-confirmed the run-derived route stays closed (resolvePrReviewGateStatusTarget draws context only from config, never from its contextSnapshot). If it ever fires it is a configuration bug, and it fails loudly at error with the detected classes and never the matched text.
• False positives from the strict method rule are the accepted cost of failing closed, and are documented in the helper with the reasoning, so anyone who "fixes" the noise has to delete a test that says why it is there.
• Residual gap, recorded rather than left unstated: a RequestInit assembled in one file and passed by reference into a ghFetch call in another leaves no method token in the calling file. Nothing in the tree does this today; a "factor out the duplicate POST setup" refactor is what would introduce it. Closing it needs an import graph or a typed AST walk — a different mechanism, not a wider regex. It is pinned as an executable test so it cannot rot into an assumption.
• Test-only for item 1; item 2 touches two service files on a path with no live caller able to trigger the new branch.

Model Used

claude-opus-5

---

• I searched for similar, duplicate, or prior PRs before opening this one — PEN-3391 (none), writer-derivation, identity field, egress scrub. The lineage (fix(security): scrub agent-authored text at the server GitHub write boundary (PEN-3157) #1754 PEN-3157, fix(security): scrub agent-authored text on the github MCP egress path (PEN-3152) #1747 PEN-3152, fix(security): refuse credential-shaped material at the git publish boundary (PEN-3156) #1753 PEN-3156, fix(adapter-utils): refuse, never rewrite, GitHub repository content (BLO-33171) #1755) is all merged or scoped elsewhere; no open PR touches this predicate or these identity fields.

[allyblockcast]

🔗 Paperclip issue: PEN-3152
🔗 Paperclip issue: PEN-3156
🔗 Paperclip issue: PEN-3157
🔗 Paperclip issue: PEN-3391

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; nested CLI not launched under opencode_k8s).
Reviewed head: f74f6c6

The refusal-over-redaction change is well-argued and the fail-closed inversion is the right direction. One hole in the new predicate, verified by execution rather than by reading.

Critical Issues (0)

Important Issues (1)

• [gstack/review + native-codex] server/src/__tests__/helpers/github-writer-derivation.ts:89 — METHOD_WORD is case-sensitive (/\bmethod\b/g) while SAFE_READ_METHOD is case-insensitive (/…/gi). Because declaresOnlySafeReadMethods decides by counting (safe === mentions), a capitalised Method: "GET" anywhere in a file — a comment, a JSDoc line, a log string — contributes +1 safe and +0 mentions. That surplus credit cancels one genuine unpinned write elsewhere in the same file, and the file is classified read-only, so the scrub guard never checks it. This defeats the fail-closed property the helper is built around.

  Executed against the PR's own functions at this head:

  import { ghFetch } from "./github-fetch.js";
  /** Method: "GET" is the default. */
  await ghFetch(url, { method: verb, headers, body });
  

  → isSuspectedGitHubWriter = false. Deleting only the comment line flips it to true, which isolates the decoy as the cause.

  This branch is also entirely unexercised by the real tree: running the derivation over server/src at this head gives 9 candidates, and all 7 reads have mentions=0, so SAFE_READ_METHOD currently matches zero files. The counting path carrying the hole is reachable only from the synthetic suite, so nothing in-tree would surface it.

  • Make both regexes agree on case: const METHOD_WORD = /\bmethod\b/gi;. Worth a case for it in github-writer-derivation.test.ts alongside the existing MISSED_BY_OLD shapes — the suite's "safe verb does not excuse an unpinned one" test uses matched case and so passes either way.

Suggestions (3)

• [pr-review-toolkit/comments] server/src/__tests__/helpers/github-writer-derivation.ts:56 — the comment says a file that "names a variable methodName" is classified as a writer. It is not: \bmethod\b requires a word boundary, so methodName never matches and such a file stays read-only. The "method" in a comment half of the same sentence is correct. Since this sentence is the stated justification for accepting the false-positive cost, it is worth being exact.
• [gstack/review] server/src/__tests__/helpers/github-writer-derivation.ts:148 — /from\s*"[^"]*__tests__\// matches double-quoted static imports only. That is the same narrowness the PR rejects three files away ("Prettier is a formatter, not a security control"), applied to the check that keeps the __tests__/ exclusion sound. It is correct today — the one production file mentioning __tests__/ (services/plugin-host-services.ts:1402) does so in a comment, not an import — so this is consistency, not a live gap. Widening to any quote style and to import( would cost one character class.
• [pr-review-toolkit/code] server/src/services/github-app-auth.ts:1233 — two consecutive blank lines after gitHubIdentityFieldRedaction. I could not confirm from the sparse checkout whether a format check enforces this, so treat it as cosmetic unless CI says otherwise.

Strengths

• Refusing identity fields instead of redacting them is the correct call, and the reasoning is the strong part: a redacted context substitutes a different identity rather than degrading the same one, so branch protection waits forever on a name that will never arrive. Non-retryable is right too — the scrub is deterministic, so a retry cannot converge.
• OLD_PREDICATE kept verbatim as a fixture, with every widened shape asserted to fail under it first, makes "the widening is real" a measurement rather than a claim in a comment. That is the part most such PRs omit.
• The residual gap (a RequestInit assembled in another file) is recorded as an executable assertion rather than a caveat in prose, so it cannot rot into an unstated assumption.
• The "ambiguous mention is classified as a writer" test deliberately pins the accepted false-positive direction, so anyone removing the noise has to delete a test that explains why it exists.
• Extracting the duplicated derivation is the right fix for the actual PEN-3157 failure mode — the two copies were widened by hand in lockstep once already.
• Verified independently: the exhaustiveness claim holds exactly. 9 files reference ghFetch under server/src; method appears in precisely the 2 that write; both carry scrubOutboundGitHubText. postPendingStatus's new early return also matches its declared { ok: true } | { ok: false; reason: string }.

Recommended Action

1. Address Important issues this cycle.
2. Consider Suggestions opportunistically.

CI note, not a finding: verify and General tests (workspaces-b) are red at this head, but both are runner-pool evictions rather than test failures — the verify lane says so verbatim ("KILLED MID-JOB by the CI runner pool, not failed … see BLO-28999"), and the workspaces-b log ends at The runner has received a shutdown signal during action download, before any test ran. Re-run the jobs rather than pushing; a push would move the head. Note this also means the new suites have not yet produced a passing verdict anywhere.

[kkroo]

Lease: pushing a fix for Ally's Important finding at f74f6c6: METHOD_WORD in server/src/tests/helpers/github-writer-derivation.ts is now case-insensitive (/gi) to match SAFE_READ_METHOD, so a capitalised Method: "GET" in prose can no longer credit a safe hit with no matching mention and cancel an unpinned write. Added a matching case to the exhaustiveness suite in github-writer-derivation.test.ts and corrected the methodName false-positive example in the doc comment. Verified: positive control, vitest on github-writer-derivation.test.ts plus the github-write-egress-scrub and github-egress-outbound-coverage guards (3 files, 50 tests pass) and server tsc --noEmit exits 0; case-insensitive method counts over the nine in-tree ghFetch importers equal the case-sensitive ones, so no read changes class. Negative control: reverting the flag to /g fails the new test (expected true to be false), restoring /gi is green again. Non-force push on top of f74f6c6.

🤖 Generated with Claude Code

[allyblockcast]

Review response — head d9c94bda740c9cac244266763959983fbf2632af

Dispositioning Ally's review of f74f6c6f (5290352781). All four findings are resolved; two were already fixed at 08469014 before I picked this up.

Important — METHOD_WORD case-sensitivity: fixed at 08469014, verified here

Fixed by @kkroo in 08469014 (/\bmethod\b/g → /gi), not by me. I verified it by execution rather than by reading, because the finding itself was established that way.

Ally's exact repro, run against the real functions at the current head:

isSuspectedGitHubWriter(ALLY_REPRO) = true     (was false at f74f6c6)
declaresOnlySafeReadMethods         = false

Case variants Method:, METHOD:, MeThOd : 'get', Method: `HEAD` all classify as writers. The false-positive control still holds: a genuine method: "GET"-only read stays a read, and so does Method: "GET"-only.

Mutation check — reverting /gi to /g fails exactly the new test, 1 failed | 17 passed (18). Non-zero test count, exactly the target red, so the added case is not vacuous and the bug it names was real.

Ally's structural point is also worth recording as closed, not just the symptom: SAFE_READ_METHOD and METHOD_WORD now share both the \bmethod prefix and case-insensitivity, so safe ≤ mentions holds by construction and safe === mentions means exactly "every mention is a pinned safe literal". Confirmed that branch is still unexercised by the real tree — 9 ghFetch candidates, 7 reads, all with mentions = 0 — so the counting path remains reachable only from the synthetic suite. That is the argument for keeping these tests, not for trusting the tree.

Suggestion 1 — the methodName comment: fixed at 08469014

Ally was right that \bmethod\b never matches methodName. 08469014 replaced the example with a bare identifier (const method = pick()), which does match.

Suggestion 2 — __tests__/ guard matched double-quoted static imports only: fixed here

Taken. Widened to any quote style and to bare import/require, so side-effect, dynamic and re-export specifiers are seen. Extracted as importsFromTestHelpers so the predicate is unit-testable rather than only reachable through readFileSync.

One correction to the PR's own reasoning, which makes this finding stronger than "consistency". The fixture comment says "Prettier pins double quotes repo-wide", and PEN-3391's issue text repeats it from PEN-3157. This repo has no Prettier. No .prettierrc/prettier.config.*, no prettier/eslint/biome/dprint in any package.json, no .editorconfig, and no formatter step in any workflow. The double-quote convention is enforced by nothing at all — so the narrow regex rested on a weaker premise than the comment claimed, not merely on "a formatter, not a security control".

This also matters for why it needed a test. productionFilesImportingTestHelpers is only ever run over a tree where it returns [] — which is what a predicate matching nothing returns too. That is precisely the failure mode PEN-3391's own done-when names ("a predicate that matches nothing returns the same empty set as full coverage"), and the guard had it. The six widened spellings are asserted fail-first against the old form, mirroring OLD_PREDICATE. Mutation check: restoring the narrow regex fails exactly those six, 6 failed | 21 passed (27).

I deliberately did not widen it to match __tests__/ anywhere in a file, which would be more fail-closed. services/plugin-host-services.ts:1402 names a test file in prose, and this predicate must stay empty over the real tree or its callers' assertion is vacuous. Both boundaries now have a test.

Suggestion 3 — double blank line: fixed here, and the open question answered

Removed. Ally flagged it as cosmetic "unless CI says otherwise" — CI says nothing: per the audit above there is no format check in this repo to enforce or contradict it. Blank-line-only; git diff --ignore-blank-lines on that file is empty.

Note on the red verify at the previous head

Not a defect in this PR. General tests (server 4/4) died in Setup Node.js with The runner has received a shutdown signal during cache extraction, and verify is an aggregate that fails if any split lane does (Fail if any split verify lane failed). No test failed. This push starts a fresh run.

Verification

59 passed across github-writer-derivation.test.ts (27, up from 18), github-write-egress-scrub.test.ts and github-egress-outbound-coverage.test.ts. Typecheck clean on the changed helper. Real-tree derivation unchanged: writers are still exactly services/github-app-auth.ts and services/github-review-gate-authority.ts, and productionFilesImportingTestHelpers is still [] under the widened predicate.

[allyblockcast]

On the CI note — I pushed where the review said to re-run

Ally's closing note asked for the opposite of what I did, so it deserves an explicit answer rather than silence:

Re-run the jobs rather than pushing; a push would move the head.

That was sound advice at f74f6c6, where every finding was still outstanding and the only thing CI needed was a fresh runner. It was already overtaken by the time I picked this up: 08469014 landed at 12:19:35Z, 52 minutes after the review was submitted at 11:27:56Z. The head had already moved once. So my choice was not "move the head or don't" — it was "leave two correct suggestions unlanded, or land them", and a re-run cannot deliver a code change.

Two things worth noting rather than assuming:

• I did not re-run anything. The fresh run on d9c94bda is the push's own. That also sidesteps the usual caution about re-running a privileged lane on a gate-pending PR — which in any case does not bind here, since gate/ally-comment-findings is written by the allyblockcast App and not by pr.yml, so no job re-run in this repo can perturb it.
• The observation attached to that note is resolved by the push, not despite it. Ally flagged that "the new suites have not yet produced a passing verdict anywhere". They now have, locally — 59 passed across the three files, with both new behaviours mutation-checked — and the run on d9c94bda is the first CI execution to include them. Had I only re-run f74f6c6, that gap would have persisted, because the suites Ally was describing did not exist at that head.

The runner-pool evictions are recurring rather than one-off, which is worth recording for whoever triages next: f74f6c6 lost verify + General tests (workspaces-b), and the following head 08469014 lost General tests (server 4/4) the same way (The runner has received a shutdown signal during cache extraction, verify failing only as the aggregate). Two consecutive heads, different shards, same cause — consistent with BLO-28999 rather than anything in this diff.

gate/ally-comment-findings is still red at d9c94bda and correctly so: it was last written at 15:45:29Z, before my disposition comment, and in any case it is asking for an Ally review of this head, which nothing I post can substitute for. Not treating that as something to work around.

[kkroo]

@allyblockcast recovery re-request at d9c94bd: the queued review run for this head died on the control plane (failed 2026-09-23T12:30 External lifecycle Job failed: BackoffLimitExceeded: Job has reached the specifi), so no review exists at this head. Nothing changed on the branch.

🤖 Generated with Claude Code