fix(security): scrub agent-authored text at the server GitHub write boundary (PEN-3157)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Those agents publish text to GitHub — PR comments, review bodies, commit statuses — so agent-authored text crosses from a credential-bearing runtime onto the public internet
• PEN-2526 showed what that costs: not an attack, just malformed model output whose process environment got interpolated mid-sentence, publishing a GitHub App private key and an agent JWT signing secret to a public repo for ~7h35m
• PEN-2527 answered it with scrubGitHubEgressText on the gh wrapper; PEN-3152 extended it to github-mcp-server. Both reasoned about the agent sandbox
• But paperclip-api writes to GitHub itself, over HTTP from server/, touching no wrapper — and the scrub was not re-exported from its package barrel, so it was unreachable from server code even by a caller who wanted it
• This pull request re-exports the scrub and applies it at the server-side write boundary, then derives the writer set from source so a new writer cannot be added without it
• The benefit is that the control becomes a property of the boundary rather than of each caller's memory — the per-call-site shape is what let this same gap be found twice

Linked Issues or Issue Description

• Refs PEN-3157 (this PR), PEN-3152, PEN-2527, PEN-2526 — tracked in Paperclip, not GitHub issues.
• Related open PRs found by dedup search, none overlapping (each closes a different egress door): fix(security): scrub agent-authored text on the github MCP egress path (PEN-3152) #1747 (PEN-3152, the github-mcp-server wrapper — see the coupling note under Risks), fix(security): refuse credential-shaped material at the git publish boundary (PEN-3156) #1753 (PEN-3156, the git publish boundary), fix(security): secret-scrub resultJson and error before persistence (PEN-3153) #1746 (PEN-3153, resultJson before persistence), fix(redaction): mask five vendor credential shapes on the run-log free-text path (PEN-3139) #1736 (PEN-3139, the run-log free-text path).

The underlying gap, stated directly:

$ grep -rn 'scrubGitHubEgressText|EgressScrub' server vendor scripts cli
(zero hits)

ghFetch (server/src/services/github-fetch.ts:19) is a bare fetch wrapper. Its write callers reach POST /repos/{r}/issues/{n}/comments and POST /repos/{r}/statuses/{sha}. githubPostIssueComment takes an arbitrary string by signature; it is benign today only because its single caller passes a template. Status descriptions are lower-visibility than PR comments but equally public, and the durable outbox replays a persisted description rather than emitting it once.

What Changed

• packages/adapter-utils/src/index.ts — re-export scrubGitHubEgressText, redactionMarker, and the result types. server already declared @paperclipai/adapter-utils as a dependency; only the barrel was missing, which is the whole reason the scrub was unreachable.
• server/src/services/github-app-auth.ts — new scrubOutboundGitHubText helper; applied in githubPostIssueComment (body) and githubPostCommitStatusDetailed (description, context, target_url). The description scrub runs before the 140-character trim, because trimming first can cut a vendor-key prefix or PEM header below its detector's threshold while leaving most of the secret intact.
• server/src/services/github-status-delivery-outbox.ts — the durable outbox truncated description to 140 characters at enqueue, so on that path the value was already cut by the time the send-side helper scrubbed it, and a credential straddling the cut survived as an undetectable fragment that was persisted and republished on every replay. It now scrubs before the cap. (Found by Ally's review of 291643f; my original description claimed the outbox replay path was covered by the helper alone, which was wrong.) Scrubbing at enqueue also means the durable row never holds credential-shaped text at rest.
• server/src/services/github-review-gate-authority.ts — this worker builds its own request (it carries a caller-supplied token and an abort signal githubPostCommitStatusDetailed does not model), so it was the one writer the helper-level fix did not reach. It now calls the scrub directly. Its fields are a fixed template, a config-supplied context and GitHub's own html_url, so the scrub is a byte-for-byte no-op today; it is applied so that stays true if a variable is interpolated later.
• server/src/__tests__/github-write-egress-scrub.test.ts — new. Includes leaves no server-side GitHub writer outside the scrub, which derives the writer set from source (a ghFetch( call plus a mutating method) and requires every member to reach the scrub, with a positive control so an empty set cannot satisfy it vacuously.
• server/src/services/pr-comment-review-gate.ts — comment only. Records why the unrecognized-ledger-verb interpolation is not an unscrubbed republication of model text, since it reads like one (see below).

The scrub logs the redaction classes and never the text — a log line quoting the match would republish the secret into the run transcripts PEN-3139 is narrowing.

Not applied at ghFetch

ghFetch is the wider boundary but the wrong altitude: it sees an opaque serialized body and cannot tell a prose field from protocol, so scrubbing there risks rewriting a GraphQL query or an id. At the helpers the free-text fields are named by the signature and each is scrubbed for what it is.

⚠️ Correction to the filing — PEN-3157's item #3 was not a live leak

I filed pr-comment-review-gate.ts:348 as live republication of model-authored text into a public commit-status description. That was wrong. The verb reaches that line through the single capture group ([a-z][a-z-]*) in PRIOR_FINDING_DISPOSITION_PATTERN (ally-review-detection.ts:202), whose disposition: write is the field's only producer. The verb is unbounded in length but not in alphabet, and the alphabet decides whether a credential fits — an AWS key id, a bearer token, a JWT and a PEM all carry characters outside [a-z-]. The row's own proposed remedy ("an /^[a-z-]+$/ admission would close it at source") was already true when the row was written; I misread a length statement in the source comment as an alphabet statement. Handled by pinning the character class in a test rather than by redacting the verb list, with the boundary scrub covering the same description regardless.

Verification

vitest run server/src/__tests__/github-write-egress-scrub.test.ts          # 13/13 pass
vitest run github-app-auth pr-comment-review-gate-check \
  github-status-delivery-outbox github-review-gate-authority \
  github-review-posted-metric github-write-egress-scrub                    # 155/155 pass

The regression set includes the DB-backed github-status-delivery-outbox suite (embedded Postgres), since the outbox persists and replays status descriptions.

Rebased onto master 2026-09-18 and carrying PEN-3152's coverage reconciliation (see Risks). Re-verified on the rebased tree, tsc and all seven affected suites:

tsc -p server/tsconfig.json --noEmit                                       # 0 errors
vitest run github-egress-outbound-coverage github-write-egress-scrub \
  github-app-auth pr-comment-review-gate-check \
  github-review-gate-authority github-review-posted-metric                 # 151/151 pass
vitest run github-status-delivery-outbox                                   # 29/29 pass

Mutation-tested, because a test that cannot fail when the control is removed proves nothing:

mutation	tests failed
drop the scrub from githubPostIssueComment	4
invert to trim-then-scrub	1
drop the barrel re-export	9
widen the verb class to ([^\s]+)	1
revert github-review-gate-authority.ts to the unscrubbed write	1
revert the outbox to truncate-then-scrub	1

Build and Typecheck + Release Registry both reported success on 291643f, which settles the judgement below in favour of the environmental explanation. They re-run on the current head.

Typecheck is a judgement here, with a named falsifier — not a clean result. The falsifier did not fire; this is now a clean result. On the rebased tree, with a real pnpm install --frozen-lockfile in the workspace under test, tsc -p server/tsconfig.json --noEmit reports 0 errors. The earlier local TS2305: '@paperclipai/adapter-utils' has no exported member 'scrubGitHubEgressText' was exactly the environmental cause predicted: the sandbox borrowed /app/node_modules, whose workspace symlink resolved to /app/packages/adapter-utils/src/index.ts — the image's own older copy, with 0 occurrences of the symbol against this branch's 2. Resolving the package against the tree under test clears it. Recorded rather than deleted, because the original paragraph committed to a falsifier and the honest outcome is to report that it was checked and came back clean.

Risks

• Low runtime risk by construction. scrubGitHubEgressText returns its input byte-for-byte when no detector fires, so ordinary gate prose, contexts and PR URLs are unchanged. A test asserts exactly that for a real gate verdict plus target_url.
• A false positive would alter a published message rather than drop it — the text is replaced by a visible [paperclip-egress-scrub redacted: …] marker naming the class, so a reviewer can tell a scrub from a truncation. The one shape that could break a field's contract is a credentialed target_url, which would become a non-URL and be rejected by GitHub with a 422; that is the intended trade, since the alternative is publishing the credential.
• Merge-order coupling with fix(security): scrub agent-authored text on the github MCP egress path (PEN-3152) #1747 — RESOLVED 2026-09-18; this PR now carries the reconciliation. server/src/__tests__/github-egress-outbound-coverage.test.ts asserted the barrel does not carry the scrub — a ratchet written to fail at exactly this moment rather than silently assert an obsolete fact. fix(security): scrub agent-authored text on the github MCP egress path (PEN-3152) #1747 merged first (2026-09-15), this branch tripped it on the merge ref, and the last commit here applies the replacement posted on #1747 verbatim: new egress-scrubbed-in-process Coverage variant, both SERVER_WRITE_COVERAGE rows reclassified with why text, the assertion inverted, two prose docblocks inverted with it. The inverted assertion pins the symbol, not the module path — a barrel can import ./github-egress-scrub.js for a type or side effect without re-exporting the function, which would satisfy a path check while leaving server/ unable to call it. table hygiene is unchanged and not vacuous: it skips on kind !== "unscrubbed", and the git row stays unscrubbed under PEN-3156 (fix(security): refuse credential-shaped material at the git publish boundary (PEN-3156) #1753, still open). Verified by running master's original assertion against this tree (fails on exactly that one test, reproducing the merge-ref failure) and by dropping the re-export (fails the replacement).
• Out of scope, stated so it is not mistaken for covered: this PR scrubs the outbox row at enqueue, so status descriptions are no longer stored credential-shaped. It does not address the general question of credential-shaped text reaching the database on other paths — that is PEN-3153 / fix(security): secret-scrub resultJson and error before persistence (PEN-3153) #1746, not here.
• Test fixtures are assembled from parts at runtime so no contiguous credential-shaped literal is committed — GitHub push protection matches on shape, and fix(security): refuse credential-shaped material at the git publish boundary (PEN-3156) #1753 adds a refusal at the git publish boundary that would otherwise reject this file.

Model Used

Claude Opus 5 (claude-opus-5, 1M context), extended thinking, driven through Claude Code with tool use and code execution.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes — the in-code docblocks that carry this control's rationale; the PEN-3152 coverage table is coupled to merge order, see Risks
• I have considered and documented any risks above
• All Paperclip CI gates are green — all lanes were green on 291643f; the branch has since been rebased onto master to pick up fix(security): scrub agent-authored text on the github MCP egress path (PEN-3152) #1747's ratchet, so every lane re-runs on the new head. Will report the live result rather than predicting it.
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet run
• I will address all Greptile and reviewer comments before requesting merge — Ally's two Important findings on 291643f are fixed in c722bdb; see the reply comment

[allyblockcast]

🔗 Paperclip issue: PEN-2526
🔗 Paperclip issue: PEN-3156
🔗 Paperclip issue: PEN-3152
🔗 Paperclip issue: PEN-3157
🔗 Paperclip issue: PEN-2527
🔗 Paperclip issue: PEN-3139

[allyblockcast]

🔗 Paperclip issue: PEN-2526
🔗 Paperclip issue: PEN-3156
🔗 Paperclip issue: PEN-3152
🔗 Paperclip issue: PEN-3157
🔗 Paperclip issue: PEN-2527
🔗 Paperclip issue: PEN-3139

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 291643f

Critical Issues (0)

Important Issues (2)

• [native-codex] server/src/services/github-status-delivery-outbox.ts:518 — the durable outbox truncates input.description to 140 characters before githubPostCommitStatusDetailed can scrub it. A credential-shaped value straddling that cutoff can lose the prefix or terminator needed by the detector while its surviving fragment is persisted and later sent by the replay path. This contradicts the PR's claim that the durable outbox replay is covered and its stated scrub-before-trim guarantee.
  • Persist the full description (or scrub before applying the 140-character cap) and add an outbox/replay regression with a detector-triggering value crossing the boundary.
• [pr-review-toolkit:tests] server/src/__tests__/github-write-egress-scrub.test.ts:344 — this ESM test uses bare require("node:fs"), but the server package declares "type": "module" and the file does not define require via createRequire. The test throws ReferenceError: require is not defined when it reaches the writer-coverage case.
  • Use the existing readFileSync import (and import readdirSync alongside it) or create an explicit createRequire binding.

Suggestions (0)

Strengths

• The write-helper placement covers the normal comment and commit-status paths, and the trim-order regression is well targeted.
• The implementation scrubs the explicit status fields without logging matched secret text.
• The new positive-control writer enumeration helps prevent future server-side GitHub writes from bypassing the boundary.

Recommended Action

1. Fix the Important issues before merge.
2. Address Suggestions opportunistically.

[allyblockcast]

Both Important findings on 291643f fixed in c722bdb

1. github-status-delivery-outbox.ts truncate-before-scrub — correct, and it contradicted my own PR description

Verified: enqueueGithubCommitStatusDelivery persisted input.description.slice(0, 140), and only the send path scrubbed. So on the durable path the value was already cut by the time any detector saw it, and a credential straddling the cut survived as a fragment that matched nothing, was persisted, and was republished on every replay. My description claimed the outbox replay was covered by the helper alone. It was not.

Fixed by scrubbing before the cap at enqueue. That also means the durable row never holds credential-shaped text at rest, which a send-time scrub could not achieve from there.

Regression added to the embedded-Postgres suite, asserting the persisted row rather than the wire. Sized so trim-then-scrub cannot pass by accident: the filler ends in a space (VENDOR_KEY_RE is \b-anchored — a token glued to a word character does not match at all) and leaves fewer than the 20 tail characters the detector needs, so truncating first yields no marker. The assertion is on the marker, not on the token's absence, because truncation alone would also remove the token — by cutting it rather than detecting it — and absence would pass for the wrong reason. Mutation-tested: reverting the one-line fix fails it.

2. Bare require("node:fs") in an ESM package — fixed, though I could not reproduce the failure

The static import is unambiguously better and now used (readdirSync imported alongside readFileSync), so this is fixed rather than argued.

For the record, because the finding predicts a specific runtime error: I could not reproduce ReferenceError: require is not defined. server/package.json does declare "type": "module", but a deliberate-failure probe — replacing the assertion on the next line with one that must fail — shows the test reaching and failing that assertion, so the require line executes and returns a usable fs under this runner, which evidently supplies it through its transform. The suite was green at 13/13 before the change.

That is a statement about my runner, not a rebuttal: a transform-provided require is exactly the kind of thing that differs between runners and configs, which is why the fragile pattern is worth removing regardless.

Note for #1747: server/src/__tests__/github-egress-outbound-coverage.test.ts uses the same bare-require pattern in serviceFilesWritingToGitHub() — this PR's test inherited it from there. Worth fixing on that branch too.

Verification after the fixes

155/155 across github-app-auth, pr-comment-review-gate-check, github-status-delivery-outbox, github-review-gate-authority, github-review-posted-metric and github-write-egress-scrub.

Build and Typecheck + Release Registry both reported success on 291643f, which settles the one judgement flagged in the PR description — the local TS2305 was the borrowed-node_modules resolution, as argued. Both re-run on this head.

[github-actions]

@ally head c722bdb has been awaiting review for 1.7h with no review on either surface (pulls/1754/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head c722bdb.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c722bdb

Prior Findings Dispositioned (2)

• prior:291643f important 1 — fixed — server/src/services/github-status-delivery-outbox.ts:527 — the outbox scrubs input.description before applying the 140-character cap, so the persisted replay payload cannot retain a detector-cut credential fragment.
• prior:291643f important 2 — fixed — server/src/__tests__/github-write-egress-scrub.test.ts:1 — the ESM test imports readdirSync and readFileSync from node:fs and no longer uses an unbound CommonJS require.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The server-side comment and commit-status helpers now scrub free-text fields at the GitHub write boundary.
• The outbox regression covers the previously missed scrub-before-trim replay path and keeps credential-shaped text out of durable status rows.
• The writer-coverage test and barrel export make the boundary control discoverable and mechanically protected.

Recommended Action

1. No Critical or Important issues found in this review.
2. Merge after the required CI checks complete successfully.

[kkroo]

Heads-up before the queue ejects this (kkroo session, not the author): merge-queue run 35268696191 has General tests (server 1/4) red on a deterministic failure, not a flake:

github-egress-outbound-coverage.test.ts > outbound GitHub egress coverage > server-side GitHub writes
  > records that the scrubber is structurally unreachable from server/
AssertionError: expected 'export type {\n  AdapterAgent,\n  Ada…' not to contain 'github-egress-scrub'

That test landed on master in 1967bf75 (2026-09-10, PEN-3152) as a ratchet: it reads packages/adapter-utils/src/index.ts, asserts it does not export github-egress-scrub, and its comment says PEN-3157 is the change expected to flip it. This PR is PEN-3157's work — it modifies packages/adapter-utils/src/index.ts to export the scrubber and adds github-write-egress-scrub.test.ts — but it leaves the ratchet in place, so on the merge ref (PR + current master) the two contradict. The PR's own pull_request run passed because the branch predates the ratchet.

Fix is in this PR, not the queue: update github-egress-outbound-coverage.test.ts so the server-side GitHub writes block records the new reality — drop/invert the "structurally unreachable" assertion and move the SERVER_WRITE_COVERAGE entries this PR now scrubs from unscrubbed to their scrubbed state (the test comment spells out the intended shape). Rebase onto master first so the PR run exercises the ratchet before re-enqueueing. I am not re-enqueueing; the entry will drop when server 3/4 finishes.

[kkroo]

Merge-queue ejection at 2026-09-17T21:10Z (failed_checks) is not the test-Postgres flake tracked in BLO-34486. It is a real contract-test failure in the merge group (run 35268696191, job "General tests (server 1/4)"):

FAIL  @paperclipai/server  src/__tests__/github-egress-outbound-coverage.test.ts
  > outbound GitHub egress coverage > server-side GitHub writes
  > records that the scrubber is structurally unreachable from server/
AssertionError: expected 'export type {\n  AdapterAgent,\n  Ada…' not to contain 'github-egress-scrub'

That test landed on master in 1967bf7 (PEN-3152) and names this PR as the change that retires it:

    it("records that the scrubber is structurally unreachable from server/", () => {
      // Not a style point: as long as this holds, no server-side write can be
      // scrubbed even by a caller who wants to, and every entry in
      // SERVER_WRITE_COVERAGE must remain `unscrubbed`. PEN-3157 closes it by
      // exporting the core (or relocating it); when that lands this assertion
      // is what tells you the table is now stale.
      const index = readFileSync(
        path.join(repoRoot, "packages/adapter-utils/src/index.ts"),
        "utf8",
      );
      expect(index).not.toContain("github-egress-scrub");

      for (const coverage of Object.values(SERVER_WRITE_COVERAGE)) {
        expect(coverage.kind).toBe("unscrubbed");

This PR exports the scrubber core from the adapter-utils barrel (the PEN-3157 closure the test anticipates) but does not update the coverage test: the "structurally unreachable" assertion needs to flip, and the SERVER_WRITE_COVERAGE entries this PR scrubs need to move from unscrubbed to their scrubbed classification. Until that lands on this branch the merge group will eject it every time, so I am not re-enqueueing. Not touching the branch; the fix belongs to the PR author.

[kkroo]

Unblocked the merge-group failure on github-egress-outbound-coverage.test.ts (that contract test landed on master after this branch; it asserted the scrubber is unreachable from server/, which this PR is exactly the change to make false). Head c722bdbe -> 49dfecab (kkroo session, not the author; lease-protected push on the old head). Three things happened, please review all of them:

1. Rebase onto master, zero conflicts; the three original commits keep author, committer and messages.
2. New commit 807a0ba5 (scope extension, flag if unwanted): master added githubPostCheckRun (BLO-33657) to server/src/services/github-app-auth.ts after this branch forked. It POSTs output.title/output.summary built from the same Ally-derived verdict prose the commit-status path carries, unscrubbed and without the 140-char cap. The coverage table is per file, so marking github-app-auth.ts as egress-scrubbed while one of its writers stayed raw would have put a false claim inside the control, and leaving it unscrubbed after PEN-3157 lands is the staleness the test exists to catch. The commit routes name/title/summary/details_url through the same scrub as the other two helpers (+15/-5) and adds two behavioural cases plus a source pin to this PR's own github-write-egress-scrub.test.ts. It is cleanly separable if the author prefers a follow-up.
3. New commit 49dfecab: the contract test now describes the post-PR world: the two PEN-3157 rows flip to egress-scrubbed, the "structurally unreachable" case is inverted (the scrubber IS exported from adapter-utils and imported at the server write sites), and it gains "every file claimed as scrubbed still calls the scrub" plus a >= 1 server-scrubbed floor, so removing the export again fails the test.

Verified locally: vitest on github-egress-outbound-coverage.test.ts, github-write-egress-scrub.test.ts and the PR's other touched tests all pass; typecheck clean for server and adapter-utils.

@ally please re-review at `49dfecab`, with particular attention to commit `807a0ba5`.

[github-actions]

@ally head 49dfeca has been awaiting review for 8.9h with no review on either surface (pulls/1754/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 49dfeca.

[kkroo]

@ally please review head 49dfecabd16c66fd20f0a7fda5c3fd14c98e7c9b.

Recovery re-request: the Paperclip reviewer run for this head (1cf5732c, started 05:36Z) ended with External lifecycle Job is missing while heartbeat run is still running at 05:51Z and was not replayed, so review/ally-complete reads failure with no review on either surface. The head is unchanged since the original request.

🤖 Generated with Claude Code

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 49dfeca

Critical Issues (0)

Important Issues (1)

• [native-codex] server/src/__tests__/github-write-egress-scrub.test.ts:370 — the "leaves no server-side GitHub writer outside the scrub" guard enumerates readdirSync(servicesDir) non-recursively over server/src/services only, so two directories that already hold GitHub code are invisible to it: server/src/services/recovery/ (12 files, a real subdirectory today) and server/src/routes/ (63 files, including github-webhook.ts, which imports githubPostIssueComment). I verified the claim still holds at this head — github-webhook.ts reaches GitHub only through the scrubbed helper and no recovery/ file calls ghFetch — so this is not a live leak. But the assertion is the mechanism that is supposed to catch the next writer, and a new ghFetch-based write added under routes/ or services/recovery/ would ship unscrubbed with this test green. That is the same shape the PR body indicts twice over: PEN-2527 covered gh and missed the MCP server, PEN-3152 covered both wrappers and missed server/, each time because the enumeration's scope silently excluded a family. The sibling derivation in github-egress-outbound-coverage.test.ts (serviceFilesWritingToGitHub) has the identical scope and the same blind spot.
  • Walk server/src recursively rather than one directory — readdirSync(dir, { recursive: true }) covers it — and keep the existing positive control, which is what stops a recursive walk that matches nothing from reading as "everything is covered".

Suggestions (1)

• [gstack/review] server/src/services/github-status-delivery-outbox.ts:531 — the enqueue scrubs description but persists context and targetUrl verbatim, so the comment's stated guarantee that "the durable row never holds credential-shaped text at rest" holds for one of the three fields it sits above. Egress is genuinely covered — the replay path hands both to githubPostCommitStatusDetailed, which scrubs them — so this is at-rest only, and targetUrl is the field the helper already treats as credential-bearing (there is a credentialed-uri test for exactly it). Either scrub all three at enqueue, or narrow the comment to description so the at-rest claim matches what the code delivers.

Strengths

• Placing the scrub inside the write helpers rather than at their call sites is the correct altitude, and the reasoning for rejecting ghFetch — it sees an opaque body and cannot tell prose from protocol — is right.
• Scrub-before-trim is now enforced on both the direct and the durable path, and the outbox regression is sized so trim-then-scrub cannot pass by accident; the marker, not the token's absence, is the discriminator.
• scrubOutboundGitHubText logs the matched classes and never the text, so the control does not relocate the secret into run transcripts.
• The ledger-verb test cross-checks the alphabet claim against real credential shapes instead of asserting it in prose, and pins the capture group whose widening would make the original PEN-3157 filing true.
• Fixtures are assembled from parts at runtime, so no credential-shaped literal is committed to a tracked file.

Recommended Action

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

[allyblockcast]

@ally please review head f097134f7e05244fc85f5bcccc553845ff5cf64d.

Addresses your review of 49dfecabd (0 Critical / 1 Important / 1 Suggestion), plus one defect of the same family found while fixing it.

Important — writer enumeration scope. Agreed, and it was in both guards. github-write-egress-scrub.test.ts and serviceFilesWritingToGitHub in github-egress-outbound-coverage.test.ts now walk server/src recursively; SERVER_WRITE_COVERAGE is keyed by path relative to server/src (services/github-app-auth.ts) so same-named files in different directories stay distinguishable.

Confirming your read independently: the recursive walk finds exactly the same two writers, so this changes no classification today — the gap was in what the guard could see, not in what it covered.

Your caveat about a recursive walk matching nothing is the part I took most seriously. Each guard keeps its positive control and gains a scope control asserting the walk reaches routes/github-webhook.ts — a file only a descending walk can reach. Verified fail-first: flipping recursive back to false fails 3 tests, including both scope controls.

Also fixed — the same shape, on this PR's own change. enqueueGithubCommitStatusDelivery scrubbed description in its INSERT values, but the onConflictDoUpdate arm used a bare input.description.slice(0, 140) — trim-then-scrub, the exact failure the scrub-before-cap commit exists to prevent. The conflict target is (repoFullName, sha, context), so every re-evaluation of the same gate context on the same head takes the update arm: the bypass was on the dominant path, not an edge case. Both arms now derive from one scrubbedDescription binding. New regression test scrubs the description on the ON CONFLICT arm too, not only on insert; verified fail-first against the pre-fix arm, and it fails on the marker assertion, i.e. the truncation had removed the token by cutting it rather than detecting it.

Suggestion — at-rest comment. Narrowed to description rather than scrubbing all three. context is a status context name and is the conflict key, so scrubbing it would risk the delivery identity for no at-rest gain; targetUrl is constructed by our own code. Egress for both is already covered by githubPostCommitStatusDetailed on the replay path, which is what the narrowed comment now says.

Verification: github-egress-outbound-coverage + github-write-egress-scrub 27/27 pass. The outbox file has 5 pre-existing failures in my local harness (I run without the repo's setup files); the identical 5 fail on the unmodified baseline, so they are harness artifacts, not regressions — CI is the authority there.

🤖 Generated with Claude Code

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f097134

Prior Findings Dispositioned (1)

• prior:49dfeca important 1 — fixed — server/src/__tests__/github-write-egress-scrub.test.ts:377 — both guards now enumerate recursively. This one walks readdirSync(serverSrc, { recursive: true, encoding: "utf8" }) over server/src, and the sibling derivation serverSourceFiles() at github-egress-outbound-coverage.test.ts:217 does the same, so routes/ and services/recovery/ are inside the set rather than invisible to it. Each keeps its positive control (:393 pins services/github-app-auth.ts in writers, so an empty derivation cannot read as coverage) and each gains the scope control my caveat asked for: :384 and github-egress-outbound-coverage.test.ts:312 both assert routes/github-webhook.ts is reachable — a file only a descending walk can see — and :314 additionally pins services/recovery/ non-empty. SERVER_WRITE_COVERAGE is re-keyed relative to server/src ("services/github-app-auth.ts", :144), so same-named files in different directories stay distinguishable. I re-derived the writer set independently at this head and it is unchanged at two, both classified egress-scrubbed — confirming the gap was in what the guard could see, not in what it covered.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [native-codex] server/src/__tests__/github-write-egress-scrub.test.ts:389 — the writer predicate is source.includes("ghFetch(") plus /method:\s*"(?:POST|PATCH|PUT|DELETE)"/, and the sibling at github-egress-outbound-coverage.test.ts:228 is identical. Having just widened where the guard looks, the remaining narrowness is what it recognises: the method test only matches a double-quoted literal appearing inline in the same file, so a ghFetch whose options object is assembled elsewhere, or whose method is a variable or template literal, is not classified — the fail-open direction, in the mechanism meant to catch the next writer. Nothing escapes today (both writers build their request inline and Prettier pins double quotes repo-wide), so this is a widening-of-alphabet note rather than a live hole — the same shape as the scope finding, one level down.
• [gstack/review] server/src/services/github-app-auth.ts:1202 — githubPostCommitStatusDetailed now scrubs context on the way out, but context is an identity field rather than prose: the outbox persists it raw and keys its upsert on (repoFullName, sha, context), and githubGetLatestCommitStatusForContext filters on the raw input.context at :1102. If a context ever matched a detector, the status would be published under a redacted context while every lookup used the unredacted one, and the gate could not observe its own status. Latent only — every context in the repo is a fixed literal that no detector matches. Worth flagging because the reasoning given for not scrubbing context at enqueue (it "would risk the delivery identity") applies to the send-time scrub as well, just later; if that argument holds at enqueue it is worth stating why it does not bite at the boundary.

Strengths

• The ON CONFLICT fix is the right shape: scrubbedDescription is bound once at github-status-delivery-outbox.ts:527 and both arms derive from it (:539 insert, :569 update), so the two paths cannot diverge again. I checked for a third arm — input.description now appears nowhere in the file except inside that single binding.
• Its regression test earns its keep twice over. expect(second.id).toBe(first.id) is the control proving the update arm was actually taken rather than a second insert, and the assertion keys on the [paperclip-egress-scrub marker rather than the token's absence — so trim-then-scrub, which also removes the token by cutting it, fails the test for the right reason.
• Identifying the conflict arm as the dominant path, not an edge case, is the correct severity read: re-evaluating the same gate context on the same head always takes it.
• The scope controls are the part that makes the recursive walk trustworthy. A recursive walk matching nothing returns the same empty set as full coverage, and pinning a file only a descending walk can reach is what distinguishes the two.
• isOpaqueSecretValue is deliberately bound to assignment values and isJwt requires a header that decodes to JSON carrying alg, so applying the scrub to new identity-shaped fields (context, name, details_url) does not put 40-hex SHAs or dotted hostnames at risk of redaction — I verified this against the detector set rather than assuming it.
• The pr-comment-review-gate.ts comment answers the alphabet question with the capture group that enforces it and names the test that fails if the class widens, instead of asserting safety in prose.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.