[kyle-sexton]

No related issue: review-input hardening from #9875 review (F1, F5)

Summary

Two findings from the review of claude-code-plugins#9875 about what the review model reads.

• F5 (fixed). The full-diff step ran git diff in the head checkout. A head that adds a .gitattributes marking a path -diff or binary, or that puts a NUL byte in a text file, turned that file into Binary files ... differ, so the reviewer never saw its text.
• F1 (not reproduced, no change). The concern was that a head-side CLAUDE.md, AGENTS.md, .claude/** or .mcp.json loads into the reviewer's context. run-claude-cli already starts the CLI with --restricted --strict-mcp-config (lane.cjs cliArgs). A credential-free probe on CLI 2.1.310 showed none of those files reach the model, in the main session or in a subagent. Details below.

Fix

• Both model jobs now write the diff with git diff --no-color --no-ext-diff --no-textconv --text "$base" "$HEAD_SHA".
  • --text shows every changed file as text, whatever the attributes or content say.
  • --no-textconv keeps the text raw. --no-ext-diff was already there.
• --attr-source=<base> alone would not be enough: a NUL byte makes git treat a file as binary with no attributes involved. The new test covers that case.

Metric, baseline, expected change

• Metric: files whose text is missing from .claude-lane/pr.diff.
• Baseline: any file the head marks -diff or binary, or that holds a NUL byte, appears only as Binary files ... differ.
• Expected change: every changed file appears as text.
• Trade-off: base-marked -diff files, such as package-lock.json in these repositories, now appear in full. A genuine binary, such as a PNG, now writes its raw bytes into the diff file. That makes the file larger for PRs that change lockfiles or images. Nothing bounds the diff file's size today.

Verification

• New test in pr-review-cli.test.cjs, for both the key job and the subscription job:
  • It runs the full-diff step's script against a temporary repository whose head hides one file each way: -diff, binary, and a NUL byte.
  • It requires each file's +echo ... line in the written diff.
  • It fails on main: the first assertion fails and the diff shows Binary files /dev/null and b/hidden.sh differ. It passes with the fix.
• node --test over .github/scripts/**/*.test.cjs and .github/actions/**/*.test.cjs: 852 pass, 0 fail.
• biome ci .github is clean.
• zizmor --offline on the workflow: no findings.
• actionlint on the workflow: the same 18 findings as main (local 1.7.12 does not know cache-mode or $/ action refs), none new.
• standards naming-lint (enforcing): 0 blocking.

F1 evidence

• The probe. CLI 2.1.310 ran with an empty HOME, a cleared environment and a dummy API key, pointed at a local HTTP stand-in that recorded each Messages request. The fixture workspace held:
  • a root CLAUDE.md or a root AGENTS.md
  • sub/AGENTS.md and sub2/CLAUDE.md
  • .claude/rules/r.md
  • .claude/agents/head-agent.md
  • .claude/skills/headskill/SKILL.md
  • .mcp.json
• What was sent.
  • Without --restricted, the requests carried every marker: root files at launch, nested ones after a Read in that directory, and the subagent's requests too.
  • With --restricted --strict-mcp-config (what lane.cjs passes), no request carried any marker: main session, subagent, at launch or after the Reads. The init message listed no head agent, no head skill and no MCP server.
  • The debug log reads project memory is off and project settings are not read.
• The docs. The CLI reference --restricted row says it "loads only managed settings and --settings". It does not name CLAUDE.md, so this is observed behaviour, not a documented guarantee.
  • Recheck trigger: a Claude Code release note that changes --restricted or how instruction files load. The lane runs cli-version: latest unless it is pinned.
• lane.test.cjs already asserts that the launcher passes --restricted.
• CLAUDE_CODE_DISABLE_CLAUDE_MDS=1 was tested and is not needed: under --restricted it adds nothing, and without --restricted it still lets project agents and skills load.

Related

• Incremental reviews read GitHub's compare patches. The API returned a patch for a -diff package-lock.json (claude-code-plugins commit 941f96a), so the attribute does not hide text there. A file GitHub treats as binary gets the "read the file at the head commit" placeholder, and the full diff now shows its text.
• Release, the standards runner-policy entry and the re-pins in claude-code-plugins and pr-pipeline-sandbox follow this merge.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-24T21:38:24.871393Z	b27e98e	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 3m 53s —— View job

---

Claude's review — pr-review-cli: full-diff --text/--no-textconv fix

• Gather context (diff, CLAUDE.md/REVIEW.md, .claude/rules/pr-body-contract.md, docs/standards/ index)
• Invoke /review:code-review and follow it for this PR
• Post findings as inline comments (none met the bar)
• Post summary comment

Scope notes

• pr-review-security.yml exists in this repo, so per REVIEW.md security findings are out of scope for this lane; there's no security-relevant surface in this diff anyway (test fixtures + a git diff flag change).
• docs/standards/README.md has no paved-path row, so no paved-path criteria apply.
• No PR CONTEXT: file was supplied and the PR body opens with No related issue: ..., so there's no closing issue to check acceptance criteria against.
• .claude/rules/pr-body-contract.md carries no paths: scoping; the PR body itself complies (opens with No related issue: <reason>, has ## Summary/## Fix/## Verification/## Related, no stray committer-leading line).

What I checked

• The --no-color --no-ext-diff --no-textconv --text flag addition against git's documented -a/--text semantics (forces text treatment regardless of -diff/binary attributes or the NUL-byte binary heuristic) and --no-textconv (keeps raw bytes, bypassing any textconv filter) — matches the stated fix.
• The checkout step (actions/checkout with ref: head.sha) confirms the working tree at diff time is the head commit, so attributes are read from the head's .gitattributes without needing --attr-source — consistent with the new test's fixture (clone checked out at head).
• The new test (pr-review-cli.test.cjs) reuses this file's established for (const job of [key, subscription]) / step() / makeTemporaryDirectory() patterns already used 6 other times in the suite; its three fixtures (-diff, binary, NUL byte) match the three failure modes named in the PR description and would plausibly fail on main as claimed (pre-fix, each shows as Binary files ... differ with no +echo ... line).
• The identical command/comment added in both the review-key and review-subscription jobs mirrors this workflow's documented, deliberate design (each model job "repeats every hardening step" in full isolation, per the file's header comment) — not a new duplication this PR introduces, so not flagged under reference-dont-duplicate.md.
• Nothing else in the workflow reads .claude-lane/pr.diff's bytes besides the model itself, so the acknowledged trade-off (raw bytes for genuine binaries, unbounded file size) has no other downstream consumer to break.

Not independently run: I could not execute node --test in this sandbox (command execution needs approval unavailable to this lane), so the "852 pass" and biome/zizmor/actionlint results in the PR body are author-claimed, not verified by me here.

No findings to report — I didn't find a correctness or convention issue meeting the high-signal bar.

claude-lane-summary: lane=claude-review sha=b27e98ed51d1df0a97a6fe7a954b6ce6522e946d run=40135033058

[claude]

Claude finished @kyle-sexton's task in 1m 59s —— View job

---

Security review complete

• Gather context (PR body, diff, changed files)
• Review pr-review-cli.yml diff change (--text/--no-textconv git diff flags)
• Review new test file changes
• Check for trusted-path / manifest implications
• Report findings

Scope: .github/workflows/pr-review-cli.yml (both the key-job and subscription-job full-diff steps) and .github/scripts/pr-review-cli.test.cjs.

No security issues found.

Analysis:

• The change only adds --no-textconv --text to the existing git diff --no-color --no-ext-diff "$base" "$HEAD_SHA" invocation (pr-review-cli.yml#L807, mirrored at #L1894). This forces files git would otherwise treat as binary (via -diff/binary attributes or a NUL byte) to be diffed as text, which is the intended fix — it closes an evasion path where an attacker-controlled head commit could hide a change from the reviewer behind a Binary files ... differ placeholder.
• $base is validated against ^[0-9a-f]{40}$ before use (#L782, #L793); both SHAs are passed as discrete git argv entries, not shell-interpolated, so there's no command-injection surface — unchanged by this diff.
• The diff output is redirected straight to $DIFF_FILE (> "$DIFF_FILE"), not threaded through a GITHUB_OUTPUT/GITHUB_ENV heredoc, so raw bytes now present in the output (including NUL bytes or content that happens to match a heredoc delimiter) can't break out and inject into subsequent step outputs or env vars.
• The diff file is read by the model as FULL DIFF context only; it's never echoed into Action logs or GITHUB_STEP_SUMMARY, and the artifact upload steps in this workflow (#L1373, #L2460, #L2696) upload unrelated handoff/review artifacts, not .claude-lane/pr.diff — so this doesn't create a new log-injection or artifact-leak surface.
• Showing full binary bytes in-context is a disclosed trade-off in the PR description (diff-file size growth for lockfile/image-heavy PRs), not a new vulnerability class — PR content already flows into the model as untrusted data under the existing framing contract.
• Test-only change (pr-review-cli.test.cjs) exercises the fix with a real temporary git repo and asserts the previously-hidden payloads now appear; no production code path, no secrets, nothing security-relevant.
• Neither file matches a TRUSTED PATHS glob (.github/standards/trusted-actors/**, .github/actions/check-trusted-trigger/**, **/CODEOWNERS, **/.gitmodules), no plugin manifest was touched, and no standing instruction/guardrail was removed or narrowed.

GitHub Actions hardening (pinning, permissions, triggers) is unchanged in this diff and is zizmor's lane, not re-reported here.

critical-findings: 0

claude-lane-summary: lane=claude-security-review sha=b27e98ed51d1df0a97a6fe7a954b6ce6522e946d run=40135033058

[github-actions]

claude-security-review has reviewed this pull request through b27e98e; a later push is reviewed from there.

[github-actions]

claude-review has reviewed this pull request through b27e98e; a later push is reviewed from there.