Skip to content

Reviewer is described as read-only, but still receives write and execute tools #1894

Description

@nothariharan

Summary

The reviewer graph is positioned as read-only, which makes sense. It should inspect a PR, record findings, publish a review, and not mutate the checkout underneath it.

Right now that read-only guarantee is mostly prompt text rather than a hard tool boundary.

What's going on

In get_reviewer_agent, we still pass a sandbox backend into create_deep_agent, and there is no hard exclusion of the mutating filesystem / shell tools that Deep Agents injects (write_file, edit_file, execute, and similar). The model is told not to touch the tree, but the tools to touch the tree are still available. If the model or a task subagent uses them, nothing in the tool layer stops it.

The chat agent already handles this properly, which is why this stood out:

  • agent/chat.py excludes execute, write_file, edit_file, and delete
  • The chat general-purpose subagent gets an allowlisted FilesystemMiddleware (read_file, ls, glob, grep only)

The reviewer prompt even says "Read-only. Do not commit, push..." in agent/reviewer.py, but the assembly path does not match that claim the way chat does.

Why it matters

This is less about the reviewer casually rewriting branches today, and more about the safety boundary being soft. Prompt-only read-only is a weak guarantee for an agent that consumes untrusted PR and issue text.

There is a real gap between the product language and what the agent can actually do.

Proposed approach

Mirror what chat already does on the reviewer path:

  1. Exclude mutating tools on the main reviewer agent (execute, write_file, edit_file, delete, or whatever the current Deep Agents set is).
  2. Give the reviewer general-purpose / task subagent an allowlisted read-only filesystem middleware instead of inheriting full write + shell access.
  3. Keep the curated review tools as they are (fetch_review_diff, findings tools, publish_review, and related).
  4. If the reviewer still needs execute for a specific checkout or gh recovery path, call that out explicitly instead of describing the whole agent as read-only.

Happy to open a PR once maintainers are aligned on the shape. If you prefer a different approach (sandbox FS permissions, a separate read-only backend wrapper, etc.), say the word and I will follow that.

Scope

Not asking to redesign the reviewer. This is a small consistency fix. Chat already has the pattern. Mainly looking for a thumbs-up or a preferred direction before implementing.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions