Skip to content

fix(claude-review): remove the agent's shell tools and redact credentials - #111

Closed
crowecawcaw wants to merge 1 commit into
aws-deadline:mainlinefrom
crowecawcaw:fix/claude-review-agent-tools
Closed

crowecawcaw wants to merge 1 commit into
aws-deadline:mainlinefrom
crowecawcaw:fix/claude-review-agent-tools

Conversation

@crowecawcaw

Copy link
Copy Markdown

What was the problem/requirement? (What/Why)

The threat model treats the review agent as attacker-controlled (prompt injection from PR content), so its tools are meant to be read-only. They weren't quite: git diff, git log, and git show accept --output=<file>, so the allowed Bash(git -C pr-head …) tools could write files on the runner, outside the agent's process. The agent step's own environment scrubbing doesn't cover that.

Separately, nothing stopped a credential-shaped string the model wrote from being posted to the public PR. GitHub masks secrets in logs, not in comments.

What was the solution? (How)

  • Remove Bash, Write, Edit, NotebookEdit, WebFetch, and WebSearch outright (--disallowedTools). The agent has no shell at all.
  • Scope the Read/Glob/Grep rules to pr-head/ and review-context/. Reads outside the workspace (the environment, /proc, $GITHUB_ENV) need an approval a headless run never gives.
  • Prepare writes the PR's commit messages to review-context/commits.txt, replacing git log.
  • The prompt says explicitly that line is the file's line number, not a position inside pr.diff. Without shell tools the model read pr.diff and used its line numbers, putting every comment 6 lines low.
  • The post job redacts anything shaped like a Bedrock API key, GitHub token, AWS access key ID, or JWT from all text it posts, including the fingerprint in the hidden marker, and redacts before truncating. This is a backstop for plain copies, not for encoded ones.

What is the impact of this change?

The agent can read the PR and the review context, and nothing else. Callers need no changes.

How was this change tested?

  • New unit tests: redaction (six credential formats, a false-positive guard, the fingerprint, and truncation order) and commit_log. Each failed before its fix. All 62 tests pass (python -m unittest discover -s test).
  • Local Claude Code 2.1.288, headless, with these exact tool flags: Bash/Write are not available at all, reads outside the cwd are refused, and reads under pr-head/ and review-context/ work.
  • End-to-end on the fork harness with Opus 5.5 (test: Claude review with scoped tools, line-number prompt fix crowecawcaw/deadline-cloud#30): 0 permission denials, all 5 planted bugs found and anchored on the correct lines. The run before the prompt fix (chore: use CodeArtifact for pip during prerelease hooks #29) anchored every finding 6 lines low, as described above.
  • zizmor 1.23.1 reports the same findings before and after.

Was this change documented?

Yes, in the workflow comment above the agent step and the defang docstring.

Is this a breaking change?

No.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ials

The agent's tools were not as read-only as intended: git diff, log, and show accept --output=, which writes a file. Remove Bash, Write, Edit, NotebookEdit, WebFetch, and WebSearch outright, scope the Read, Glob, and Grep rules to pr-head/ and review-context/ so reads outside the workspace need an approval a headless run never gives, and give the agent the PR's commit messages as review-context/commits.txt instead of git log.

As a backstop, the post job redacts anything shaped like a Bedrock API key, GitHub token, AWS access key ID, or JWT from text it posts, including the fingerprint in the marker, and redacts before truncating.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
@crowecawcaw

Copy link
Copy Markdown
Author

Folded into #112, together with a fix found in testing: the agent now gets diffs numbered with each line's anchor, since without shell tools it often anchored comments a few lines low.

@crowecawcaw crowecawcaw closed this Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant