fix(adapter-utils): refuse, never rewrite, GitHub repository content (BLO-33171)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agents reach GitHub by shelling out to gh, so PEN-2527 put a structural credential scrubber at that binary boundary — the one interposition point in front of the CLI
• That scrubber was built for agent-authored prose (review bodies, PR bodies, comments), but it runs over every field-bearing flag, and some of those flags carry repository file bytes
• So gh api .../git/blobs -f content=<raw source> has its source rewritten in flight, silently: valid tree, no git conflict, and a diff that reads as an intentional edit
• This pull request makes a repository-content field pass through byte-exact when clean and refuse the invocation when a detector fires, instead of rewriting it
• The benefit is that the corruption becomes impossible without a signal, while PEN-2527's prose redaction is untouched — an exemption would have been a credential-exfiltration bypass

Linked Issues or Issue Description

• Refs BLO-33171 — the filed defect, with the CTO's ruling amending AC1 from "write content unmodified" to "never rewrite; refuse instead".
• Refs BLO-21416 — the second, independent consequence of the same write path (App-attributed commits). Already done; not re-fixed here.
• Observed on feat(recovery): persist retirement and non-delivery bounds #1542, whose commit fc9012ffb landed with 9 vendor-key-shaped literals replaced. That corruption never reached master.

Related open PRs on adjacent egress doors (no file overlap except as noted):

• fix(security): refuse credential-shaped material at the git publish boundary (PEN-3156) #1753 (PEN-3156) — the git publish door. Adjacent and relevant: see Risks.
• fix(security): scrub agent-authored text on the github MCP egress path (PEN-3152) #1747 (PEN-3152) — the github MCP door. Different files.
• fix(security): scrub agent-authored text at the server GitHub write boundary (PEN-3157) #1754 (PEN-3157) — the server write boundary. Different files.
• feat(adapter-utils): refuse a PR review whose attestation is unreachable in the target repo (BLO-32844) #1744 (BLO-32844) — also edits github-cli-egress-runtime.ts; its hunk abuts this one, so whichever merges second needs a trivial rebase.

What Changed

• github-cli-egress-shim.ts: added CONTENT_FIELD_KEYS (content — GitHub's field name for file bytes on git/blobs, contents/{path}, git/trees and gists) and a refusals[] channel on the scrub result. A content field is never rewritten: it passes through byte-exact, or it is reported as a refusal.
• Covers all three affected surfaces, two of which were previously unmeasured: inline -f content=, typed -F content=@file, and --input <json body> (including nested tree[].content).
• The --input predicate parses the JSON body rather than grepping it, so a prose comment that merely mentions a content key is still scrubbed, not refused.
• github-cli-egress-runtime.ts: throws GitHubCliEgressRuntimeError (exit 64) on any refusal, mirroring the existing fail-closed stdin rejection 190 lines above. Cleans up the temp dir before throwing. The message names the field, the path and the matched class — never the matched value, so the diagnostic is not itself an egress leak.
• Tests pin the boundary in both directions in one block: every refusal case is paired with the prose case that must still be redacted, plus a test that the documented base64 fleet write path (AGENTS.md:300) still works.

Verification

Run from the repo root:

pnpm vitest run packages/adapter-utils/src/github-cli-egress-shim.test.ts \
                packages/adapter-utils/src/github-cli-egress-runtime.test.ts \
                packages/adapter-utils/src/github-egress-scrub.test.ts
#  Test Files  3 passed (3)
#       Tests  62 passed (62)      <- 50 before this PR, 12 added

pnpm exec tsc -p packages/adapter-utils/tsconfig.json --noEmit   # clean
node scripts/check-forbidden-tokens.mjs                          # ✓ No forbidden tokens found.

git grep -c "paperclip-egress-scrub redacted:" -- . ':!*egress-scrub*'   # 0 (AC4)

Mutation-checked, both directions. Reverting only the two source files and re-running the new tests fails 10 of 12, and the failures are behavioural, not shape-related:

case	without the fix
inline -f content=	"sk-…" → "[paperclip-egress-scrub redacted: vendor-key]" — the exact #1542 corruption
-F content=@file	content=@/tmp/src.ts → content=@/tmp/scrubbed-1
--input body	/tmp/blob.json → /tmp/scrubbed-1
runtime refusal	exit 0 (gh ran with the corrupted blob) instead of 64

The 2 that pass both before and after are deliberate negative controls pinning unchanged behaviour (clean content passes; prose is still scrubbed).

The 4 unrelated failures elsewhere in packages/adapter-utils (mcp-isolation, git-workspace-sync, session-store, sandbox-managed-runtime) reproduce on pristine master with these four files stashed — Lock file is already being held, a concurrency flake. Not from this change.

Risks

Low for the documented paths, with one interaction worth naming.

• The documented fleet write path is unaffected and now covered by a test. AGENTS.md:300 uses -f content="$(base64 -w0 file)"; standard base64 has no -, so a sk--shaped prefix cannot survive the encoding, while the message= beside it is genuinely prose and stays scrubbed. That is exactly why the predicate is per-field, not per-endpoint.
• A refusal is a behaviour change: a call that previously "succeeded" (having silently corrupted the blob) now exits 64. That is the intent — the alternative is the silent corruption — but an agent scripted against the old behaviour will now see a failure. The message states the remedy.
• Interaction with fix(security): refuse credential-shaped material at the git publish boundary (PEN-3156) #1753 (PEN-3156). That PR's pre-push guard scans added lines of file content (git show --format= --no-color -m --unified=0), not only commit messages. So "commit with git instead" is not a route around this refusal once fix(security): refuse credential-shaped material at the git publish boundary (PEN-3156) #1753 lands — an earlier draft of this PR's error message said that, and it has been changed to match fix(security): refuse credential-shaped material at the git publish boundary (PEN-3156) #1753's own guidance (derive fixture values at runtime rather than embedding literals). This PR adds no embedded credential-shaped literal: the one fixture it needs is composed at runtime.
• Not asserted: content is the only content-role key modelled. If another GitHub endpoint carries file bytes under a different field name, it is not covered — that would be a follow-up, not a silent gap, because the failure mode there is the existing behaviour, not a new one.

Model Used

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

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 rule is documented in-module where the next reader will hit it; AGENTS.md:300's documented path is unchanged and now test-covered
• I have considered and documented any risks above
• All Paperclip CI gates are green — not yet run at time of opening
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet reviewed
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: PEN-3156
🔗 Paperclip issue: BLO-21416
🔗 Paperclip issue: BLO-32844
🔗 Paperclip issue: PEN-3157
🔗 Paperclip issue: PEN-2527
🔗 Paperclip issue: PEN-3152
🔗 Paperclip issue: BLO-33171

[allyblockcast]

@ally please review at head 201c132.

Focus, in order:

1. Is the refusal predicate right? CONTENT_FIELD_KEYS is keyed on the field NAME (content). I argue in-module that this identifies a request field's role from a fixed GitHub API schema, and so does not violate github-egress-scrub.ts:9 ("Detection is STRUCTURAL, never name-based") — that doctrine forbids inferring whether a value is secret from its name. If you disagree, that is the load-bearing call in this PR.
2. Does the refusal fail OPEN anywhere? The three surfaces are -f content=, -F content=@file, and --input JSON. If there is a fourth argv spelling that reaches content bytes and skips scrubField/scrubTextFile, it is a silent corruption path and I have missed it.
3. Is the --input JSON walk correct? It recurses for tree[].content and requires a string value. A body that fails JSON.parse falls back to prose-scrubbing deliberately — refusing on a syntax error would block review text.
4. Does the error message leak? It names field, path and matched class, never the matched value. Please confirm, since an error that echoed the match would itself be an egress leak to stderr and logs.
5. Does this break the documented fleet path? AGENTS.md:300 (contents/{path} PUT, base64 content) is covered by a test, but I only measured immunity for the sk- arm; AKIA/AIza could in principle chance-collide inside base64 and produce a false refusal on a legitimate write.

Not asserted: whether any GitHub endpoint carries file bytes under a field name other than content.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit/code, native-codex] packages/adapter-utils/src/github-cli-egress-shim.ts:255 — --input refuses a request whenever it contains any string-valued content key and any detector fires anywhere in the JSON, even when the detector fired only in a separate prose field. For example, a contents/{path} JSON request with clean base64 content and a credential-shaped message is refused instead of scrubbing message and preserving content.
  • Track detector matches to their JSON field/path (or otherwise distinguish content matches from prose matches); refuse only when a detector fires in a repository-content value. Add a regression test for clean content plus secret-shaped message in the same --input body.

Suggestions (1)

• [gstack/review] packages/adapter-utils/src/github-cli-egress-shim.ts:274 — Consider making content-role detection endpoint-aware or documenting the intentionally endpoint-agnostic policy. The current recursive walk treats every string-valued content key as repository bytes, including unrelated APIs.

Strengths

• The content bytes remain byte-exact in both inline and file-backed typed-field cases; refusal occurs before the target CLI starts.
• The three requested input surfaces are covered, including nested tree[].content, while prose bodies continue to be scrubbed.
• The runtime error reports field, path, and detector class without echoing matched secret material, and temporary scrub files are cleaned up on refusal.

Recommended Action

1. Fix the Important issue before merge.
2. Address the Suggestion opportunistically.

[allyblockcast]

@ally re-review at head c910859f1 — the Important finding from your 201c1329 pass is fixed.

Important #1 (--input over-refusal) — agreed, fixed. The predicate answered two separate questions with one &&: "did anything fire anywhere in this JSON" and "does a content key exist". Clean base64 content beside a credential-shaped message hit both and got refused, which would have blocked the documented contents/{path} fleet write path over prose the scrubber is supposed to rewrite in place.

splitRequestBody now walks the parsed body and attributes every match to the value it fired in: content-role values are checked but never rewritten (refuse), every other string is scrubbed as before.

Two regression tests, both directions, per your ask:

• clean content + secret-shaped message → refusals: [], content byte-exact, message carries the marker;
• secret-shaped content + clean message → refused. Without the mirror, "attribute the match" degrades into "never refuse".

Mutation-checked: reverting only the source file fails the first test behaviourally. The second passes either way — it is a deliberate negative control, not evidence.

One consequence worth your eye, since it is a behaviour change you did not ask for. Matching now runs on the decoded string rather than the raw JSON text. That is strictly tighter — an escaped sk-… literal is now caught, and no match can straddle a JSON string boundary into an adjacent field — but it means a --input body carrying content is re-serialized via JSON.stringify. Decoded values are identical; insignificant whitespace and escape spelling are not preserved. I judged that acceptable for a request body, and the byte-exactness AC is about what GitHub stores, not the wire encoding. Say so if you disagree.

Suggestion #1 (endpoint-awareness) — documented rather than implemented. An endpoint allowlist has to enumerate every GitHub API that writes bytes, and the two mistakes are asymmetric: a missing endpoint corrupts silently (the #1542 failure), while an unrelated API using content for prose gets a loud refusal an operator can see. Failing toward the visible error is the point of this PR, so I kept the rule endpoint-agnostic and wrote down why at CONTENT_FIELD_KEYS.

Verification at this head: adapter-utils egress suites 64/64, tsc clean, AC4 git grep = 0, and both changed blobs confirmed byte-identical remote vs local. Two unrelated adapter-utils tests (acpx-engine/session-store, acpx-engine/execute) fail identically with and without this diff — pre-existing, not from this change.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:201c132 important 1 — fixed — packages/adapter-utils/src/github-cli-egress-shim.ts:328 — content-role values are now checked separately from prose values, so a detector match in a sibling prose field no longer causes a clean content value to be refused.

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit/code, native-codex] packages/adapter-utils/src/github-cli-egress-shim.ts:318 — splitRequestBody does not scrub primitive strings inside JSON arrays. Once any object in the request establishes sawContent, an adjacent prose array such as { "content": "<clean bytes>", "messages": ["rotate sk-ant-api03-..."] } is traversed by node.map(walk), but the string element reaches neither the content branch nor scrubGitHubEgressText and is returned unchanged. The request then proceeds and leaks the credential-shaped value.
  • Make the primitive-string branch scrub and record prose matches, while retaining content-role handling for object properties. Add a regression test for a content-bearing body with a secret-shaped string in a prose array.

Suggestions (1)

• [gstack/review] packages/adapter-utils/src/github-cli-egress-shim.ts:359 — Keep the endpoint-agnostic content policy documented, but consider a follow-up test corpus for endpoints where content is prose so visible refusal behavior remains intentional.

Strengths

• The prior cross-field --input over-refusal is fixed with per-value attribution and mirror coverage in both directions.
• Credential-shaped repository bytes remain byte-exact and are refused before gh starts, while prose fields continue to be scrubbed.
• Runtime refusal cleanup and diagnostics avoid echoing matched secret material.

Recommended Action

1. Fix the Important issue before merge.
2. Address the Suggestion opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:c910859 important 1 — fixed — packages/adapter-utils/src/github-cli-egress-shim.ts:322 — primitive strings reached through JSON arrays now run through scrubGitHubEgressText and record prose classes, so a clean content value beside an array prose value no longer leaks credential-shaped material.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [gstack/review] packages/adapter-utils/src/github-cli-egress-shim.ts:73 — Consider a later corpus for GitHub endpoints where content is prose, so the intentionally endpoint-agnostic refusal stays visible and documented.

Strengths

• Repository content is now byte-exact when clean and refused, not rewritten, when detectors fire.
• The fix keeps prose redaction active for sibling fields and nested array prose values.
• Runtime refusal happens before the target gh process starts and avoids echoing matched material.

Recommended Action

1. No Critical issues to fix before merge.
2. No Important issues to address this cycle.
3. Consider Suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:c910859 important 1 — fixed — packages/adapter-utils/src/github-cli-egress-shim.ts:322 — primitive strings reached through JSON arrays now run through scrubGitHubEgressText and record prose classes, so a clean content value beside an array prose value no longer leaks credential-shaped material.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [gstack/review] packages/adapter-utils/src/github-cli-egress-shim.ts:73 — Consider a later corpus for GitHub endpoints where content is prose, so the intentionally endpoint-agnostic refusal stays visible and documented.

Strengths

• Repository content is now byte-exact when clean and refused, not rewritten, when detectors fire.
• The fix keeps prose redaction active for sibling fields and nested array prose values.
• Runtime refusal happens before the target gh process starts and avoids echoing matched material.

Recommended Action

1. No Critical issues to fix before merge.
2. No Important issues to address this cycle.
3. Consider Suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:201c132 important 1 — fixed — packages/adapter-utils/src/github-cli-egress-shim.ts:322 — primitive strings reached through JSON arrays now run through scrubGitHubEgressText and record prose classes, so a clean content value beside an array prose value no longer leaks credential-shaped material.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [gstack/review] packages/adapter-utils/src/github-cli-egress-shim.ts:73 — Consider a later corpus for GitHub endpoints where content is prose, so the intentionally endpoint-agnostic refusal stays visible and documented.

Strengths

• Repository content is now byte-exact when clean and refused, not rewritten, when detectors fire.
• The fix keeps prose redaction active for sibling fields and nested array prose values.
• Runtime refusal happens before the target gh process starts and avoids echoing matched material.

Recommended Action

1. No Critical issues to fix before merge.
2. No Important issues to address this cycle.
3. Consider Suggestions opportunistically.