joshtronic/igor
joshtronic/igor
Watch
2
Code Issues 3 Pull requests 1 Activity Actions

feat: agentic read-only code review -- give the reviewer its own worktree + read/grep tools #241

New issue
Closed
opened 2026-06-24 23:56:57 +00:00 by igor · 2 comments
igor commented 2026-06-24 23:56:57 +00:00
Collaborator
Copy link

Why

The code review runs as a one-shot claude_call (tick.sh ~L2440): the model
gets the PR diff as a TEXT blob and nothing else -- no worktree, no Read/Grep. So
it can't verify any claim that spans beyond the diff (leftover refs after a
rename, a caller it can't see, a constant defined elsewhere) and honestly punts
those to the human as COMMENT-instead-of-APPROVE. A human reviewer doesn't defer
that legwork to the dev -- they grep the codebase themselves. The reviewer should
too. (Deferring verification to the AUTHOR was considered and REJECTED -- that's
self-review, the exact boundary the binding flow protects.)

Make the review agentic + read-only: its own checkout of the PR's head + read/
grep tools, so it verifies whole-tree facts itself. Independence is preserved
(non-author AGENT_MODEL_REVIEW tier, fresh context, adversarial directive); it
just gains a human reviewer's verification power.

The change (in do_review_tick)

  1. Swap claude_call -> claude_run_with_cost -- the agentic primitive
    issue-work already uses (tick.sh ~L3466). Reuse that invocation shape:
    cd into the worktree, then claude_run_with_cost "review" "$log" "$timeout" --model "$AGENT_MODEL_REVIEW" --settings <review-profile> --append-system-prompt "$directive" --max-turns N --print "$user_msg".
  2. Only after the patch-id dedup decides to review, check out the PR's head
    into a worktree off origin/<pr-head> (fetched fresh), so the reviewer can
    Read/Grep the tree AT THE PR'S STATE. (Keep the dedup cheap -- no worktree for
    skipped reviews.) Remove the worktree on every exit path (trap, like the
    maintenance worktree).
  3. Run under a RESTRICTED read-only settings profile -- the safety-critical
    part. The reviewer must be STRUCTURALLY unable to mutate what it reviews:
    • ALLOW: Read, Grep, Glob, and a TIGHT read-only Bash allow-list
      (git log/show/diff/status, grep, find, cat/ls/head/tail/wc).
    • DO NOT allow: Edit, Write, MultiEdit, NotebookEdit, git add/commit/push,
      anything that writes.
    • Make it a SEPARATE file agent-review-settings.json -- NOT a mutation of
      agent-settings.json (operator-coordinated). It's strictly MORE restrictive
      than the worker profile (read-only), so it tightens, never loosens.
  4. Verdict via a file: the reviewer writes .agent/REVIEW.md with the existing
    contract (a VERDICT: line + ===BODY=== + the markdown review). The harness
    reads + parses it with the existing review_parse_response. (An agentic run
    doesn't hand back a clean string like claude_call does -- a file is the robust
    hand-off, same pattern as PR_BODY.md.) Normalize dashes on it like the other
    .agent/ outputs.
  5. Update bin/lib/review-directive.md: tell the reviewer it has the PR's tree
    checked out READ-ONLY and should Grep/Read to confirm anything it can't verify
    from the diff alone (leftover refs, callers, definitions elsewhere) BEFORE
    settling on a can't-confirm COMMENT. It still primarily reviews the DIFF; the
    tree is for verification, not a re-review of the whole repo.

Keep UNCHANGED

  • Verdict ROUTING (3a/3b: APPROVE/COMMENT -> request human; REQUEST_CHANGES ->
    rework loop + 3-round cap). This ticket changes how the verdict is PRODUCED,
    not what happens to it.
  • patch-id dedup, the per-sha comment marker / crash-safety net, and
    AGENT_MODEL_REVIEW (independence stays).

Watch out for (safety-critical -- I will scrutinize these hardest on review)

  • Read-only must be STRUCTURAL, not prompted. The settings profile is the
    enforcement. Backstop: after the run, assert the worktree has NO new commits and
    NO dirty tracked files; if it does, DISCARD the verdict + log loudly (the
    reviewer broke read-only -- a bug, never trusted).
  • The reviewer must NEVER push/commit/edit -- the thing under review cannot be
    mutated by its reviewer.
  • Cost: agentic + a worktree per review is materially pricier than one cheap
    completion, on every un-reviewed bot-PR head. Bound it: a review timeout and a
    sane --max-turns. (Operator has accepted the cost: verification > spend.)
  • agent-review-settings.json is a permission artifact -- get the allow-list
    exactly right; the operator reviews it on the PR.

Definition of done

  • The review runs agentically with Read/Grep at the PR's head; it CAN grep the
    whole tree (e.g. on a rename PR it confirms "no leftover refs" itself and reaches
    APPROVE instead of punting).
  • It produces APPROVE/COMMENT/REQUEST_CHANGES via .agent/REVIEW.md, parsed
    unchanged; the binding routing is untouched.
  • The reviewer is STRUCTURALLY read-only (the profile denies writes; the post-run
    worktree is provably unmutated).
  • bash -n bin/tick.sh clean; bash bin/check-sync.sh passes.
<!-- agent:enqueue --> ## Why The code review runs as a one-shot `claude_call` (`tick.sh` ~L2440): the model gets the PR diff as a TEXT blob and nothing else -- no worktree, no Read/Grep. So it can't verify any claim that spans beyond the diff (leftover refs after a rename, a caller it can't see, a constant defined elsewhere) and honestly punts those to the human as COMMENT-instead-of-APPROVE. A human reviewer doesn't defer that legwork to the dev -- they grep the codebase themselves. The reviewer should too. (Deferring verification to the AUTHOR was considered and REJECTED -- that's self-review, the exact boundary the binding flow protects.) Make the review **agentic + read-only**: its own checkout of the PR's head + read/ grep tools, so it verifies whole-tree facts itself. Independence is preserved (non-author `AGENT_MODEL_REVIEW` tier, fresh context, adversarial directive); it just gains a human reviewer's verification power. ## The change (in `do_review_tick`) 1. **Swap `claude_call` -> `claude_run_with_cost`** -- the agentic primitive issue-work already uses (`tick.sh` ~L3466). Reuse that invocation shape: `cd` into the worktree, then `claude_run_with_cost "review" "$log" "$timeout" --model "$AGENT_MODEL_REVIEW" --settings <review-profile> --append-system-prompt "$directive" --max-turns N --print "$user_msg"`. 2. **Only after the patch-id dedup decides to review**, check out the PR's head into a worktree off `origin/<pr-head>` (fetched fresh), so the reviewer can Read/Grep the tree AT THE PR'S STATE. (Keep the dedup cheap -- no worktree for skipped reviews.) Remove the worktree on every exit path (trap, like the maintenance worktree). 3. **Run under a RESTRICTED read-only settings profile** -- the safety-critical part. The reviewer must be STRUCTURALLY unable to mutate what it reviews: - ALLOW: Read, Grep, Glob, and a TIGHT read-only Bash allow-list (`git log/show/diff/status`, `grep`, `find`, `cat/ls/head/tail/wc`). - DO NOT allow: Edit, Write, MultiEdit, NotebookEdit, `git add/commit/push`, anything that writes. - Make it a SEPARATE file `agent-review-settings.json` -- NOT a mutation of `agent-settings.json` (operator-coordinated). It's strictly MORE restrictive than the worker profile (read-only), so it tightens, never loosens. 4. **Verdict via a file:** the reviewer writes `.agent/REVIEW.md` with the existing contract (a `VERDICT:` line + `===BODY===` + the markdown review). The harness reads + parses it with the existing `review_parse_response`. (An agentic run doesn't hand back a clean string like `claude_call` does -- a file is the robust hand-off, same pattern as `PR_BODY.md`.) Normalize dashes on it like the other `.agent/` outputs. 5. **Update `bin/lib/review-directive.md`:** tell the reviewer it has the PR's tree checked out READ-ONLY and should Grep/Read to confirm anything it can't verify from the diff alone (leftover refs, callers, definitions elsewhere) BEFORE settling on a can't-confirm COMMENT. It still primarily reviews the DIFF; the tree is for verification, not a re-review of the whole repo. ## Keep UNCHANGED - Verdict ROUTING (3a/3b: APPROVE/COMMENT -> request human; REQUEST_CHANGES -> rework loop + 3-round cap). This ticket changes how the verdict is PRODUCED, not what happens to it. - patch-id dedup, the per-sha comment marker / crash-safety net, and `AGENT_MODEL_REVIEW` (independence stays). ## Watch out for (safety-critical -- I will scrutinize these hardest on review) - **Read-only must be STRUCTURAL, not prompted.** The settings profile is the enforcement. Backstop: after the run, assert the worktree has NO new commits and NO dirty tracked files; if it does, DISCARD the verdict + log loudly (the reviewer broke read-only -- a bug, never trusted). - The reviewer must NEVER push/commit/edit -- the thing under review cannot be mutated by its reviewer. - **Cost:** agentic + a worktree per review is materially pricier than one cheap completion, on every un-reviewed bot-PR head. Bound it: a review timeout and a sane `--max-turns`. (Operator has accepted the cost: verification > spend.) - `agent-review-settings.json` is a permission artifact -- get the allow-list exactly right; the operator reviews it on the PR. ## Definition of done - The review runs agentically with Read/Grep at the PR's head; it CAN grep the whole tree (e.g. on a rename PR it confirms "no leftover refs" itself and reaches APPROVE instead of punting). - It produces APPROVE/COMMENT/REQUEST_CHANGES via `.agent/REVIEW.md`, parsed unchanged; the binding routing is untouched. - The reviewer is STRUCTURALLY read-only (the profile denies writes; the post-run worktree is provably unmutated). - `bash -n bin/tick.sh` clean; `bash bin/check-sync.sh` passes.
igor self-assigned this 2026-06-24 23:59:00 +00:00
igor commented 2026-06-25 00:16:28 +00:00
Author
Collaborator
Copy link

The harness security review flagged a material issue in this change, so it was NOT pushed:

I reviewed this diff, which converts the PR reviewer from a pure text/diff call into an agentic reviewer that executes Bash and reads the filesystem while processing attacker-controlled PR content, then emits output (.agent/REVIEW.md) that is posted as the public PR review.

Findings:

  1. Secret exfiltration via untrusted-content prompt injection + over-broad read/cat allow (introduced by this diff).
    • agent-review-settings.json grants Read and Bash(cat:*) (plus find, grep, ls, head, tail) to an agent that now runs inside a worktree checked out at the untrusted PR head (do_review_tick in bin/tick.sh).
    • File-read protection is a denylist, which is inherently incomplete. The deny rules cover ~/.ssh, ~/.local/state/agent, ~/.config/agent, /etc/shadow, /etc/sudoers — but not common credential locations that the agent can freely read, e.g.:
      • cat ~/.config/gh/hosts.yml (GitHub CLI OAuth token — only ~/.config/agent is denied, not ~/.config/gh)
      • cat ~/.netrc, cat ~/.aws/credentials, cat ~/.gitconfig
      • cat /proc/self/environ (leaks the parent process environment, which for this tool plausibly includes ANTHROPIC_API_KEY / GitHub token)
    • This is the classic "lethal trifecta": untrusted input (PR files/diff) + tool access (read/cat) + an exfiltration channel (the review body that gets posted). A malicious PR can prompt-inject the reviewer into reading a credential file and embedding it in .agent/REVIEW.md, which is published.
    • The authors clearly intended to prevent secret exfiltration (hence the denylist), but the denylist has concrete gaps for credential files and /proc, making token/credential leakage realistically exploitable.

The worktree-mutation guard and the read-only checkout are reasonable, and the trap/path interpolation (repo names limited to safe chars) do not appear injectable. The material risk is the incomplete read denylist enabling credential exfiltration through the published review.

Recommendation: switch file access to an allowlist scoped to the worktree (or explicitly deny ~/.config/gh, ~/.netrc, ~/.aws, ~/.gitconfig, /proc), and ensure the reviewer subprocess runs with a scrubbed environment.

Address it, then remove Status/Blocked to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.)

The harness security review flagged a material issue in this change, so it was NOT pushed: I reviewed this diff, which converts the PR reviewer from a pure text/diff call into an **agentic reviewer that executes Bash and reads the filesystem** while processing attacker-controlled PR content, then emits output (`.agent/REVIEW.md`) that is posted as the public PR review. Findings: 1. **Secret exfiltration via untrusted-content prompt injection + over-broad read/cat allow (introduced by this diff).** - `agent-review-settings.json` grants `Read` and `Bash(cat:*)` (plus `find`, `grep`, `ls`, `head`, `tail`) to an agent that now runs inside a worktree checked out at the **untrusted PR head** (`do_review_tick` in `bin/tick.sh`). - File-read protection is a **denylist**, which is inherently incomplete. The deny rules cover `~/.ssh`, `~/.local/state/agent`, `~/.config/agent`, `/etc/shadow`, `/etc/sudoers` — but **not** common credential locations that the agent can freely read, e.g.: - `cat ~/.config/gh/hosts.yml` (GitHub CLI OAuth token — only `~/.config/agent` is denied, not `~/.config/gh`) - `cat ~/.netrc`, `cat ~/.aws/credentials`, `cat ~/.gitconfig` - `cat /proc/self/environ` (leaks the parent process environment, which for this tool plausibly includes `ANTHROPIC_API_KEY` / GitHub token) - This is the classic "lethal trifecta": untrusted input (PR files/diff) + tool access (read/cat) + an exfiltration channel (the review body that gets posted). A malicious PR can prompt-inject the reviewer into reading a credential file and embedding it in `.agent/REVIEW.md`, which is published. - The authors clearly intended to prevent secret exfiltration (hence the denylist), but the denylist has concrete gaps for credential files and `/proc`, making token/credential leakage realistically exploitable. The worktree-mutation guard and the read-only checkout are reasonable, and the trap/path interpolation (repo names limited to safe chars) do not appear injectable. The material risk is the incomplete read denylist enabling credential exfiltration through the published review. Recommendation: switch file access to an allowlist scoped to the worktree (or explicitly deny `~/.config/gh`, `~/.netrc`, `~/.aws`, `~/.gitconfig`, `/proc`), and ensure the reviewer subprocess runs with a scrubbed environment. Address it, then remove `Status/Blocked` to re-queue. (If the note above says the gate could not complete, that's a transient error -- just re-queue.)
igor removed their assignment 2026-06-25 00:16:29 +00:00
igor commented 2026-06-25 00:32:42 +00:00
Author
Collaborator
Copy link

Closing per the security gate's finding + the board call.

Giving the review pass its own tools creates the lethal trifecta: untrusted PR content (the checked-out head) + read access (Read/cat/grep) + a published output channel (.agent/REVIEW.md is posted as the PR review). Confirmed exploitable — claude_run_with_cost strips only ANTHROPIC_API_KEY, so FORGEJO_TOKEN (+ SMTP2GO/GSC/marketstack) stay in the env, and a prompt-injected reviewer could cat /proc/self/environ straight into the public review. A denylist can't close this (inherently incomplete).

Decision: keep the pure, diff-only reviewer (no tools, no exfil surface). Its honest "can't verify beyond the diff" is closed cheaply by the human-side review, which already has full tools. If autonomous whole-tree verification is ever wanted, the safe path is harness-mediated deterministic checks (reviewer emits grep requests; the harness runs them) — not an agent with read + a publish channel. Not worth building now.

Nixed. The security gate did its job catching this.

Closing per the security gate's finding + the board call. Giving the review pass its own tools creates the lethal trifecta: untrusted PR content (the checked-out head) + read access (Read/`cat`/grep) + a **published** output channel (`.agent/REVIEW.md` is posted as the PR review). Confirmed exploitable — `claude_run_with_cost` strips only `ANTHROPIC_API_KEY`, so `FORGEJO_TOKEN` (+ SMTP2GO/GSC/marketstack) stay in the env, and a prompt-injected reviewer could `cat /proc/self/environ` straight into the public review. A denylist can't close this (inherently incomplete). Decision: keep the **pure, diff-only reviewer** (no tools, no exfil surface). Its honest "can't verify beyond the diff" is closed cheaply by the human-side review, which already has full tools. If autonomous whole-tree verification is ever wanted, the safe path is harness-mediated deterministic checks (reviewer emits grep *requests*; the harness runs them) — not an agent with read + a publish channel. Not worth building now. Nixed. The security gate did its job catching this.
igor closed this issue 2026-06-25 00:32:42 +00:00
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
master
agent/489-chore-delete-unconsumed-in-repo-prompt-copies
agent/439-event-driven-needs-you-notification-so-the
No results found.
Labels
Clear labels   
Agent

   
Compat/Breaking
Breaking change that won't be backward compatible

  
Kind/Bug
Something is not working

  
Kind/Documentation
Documentation changes

  
Kind/Enhancement
Improve existing functionality

  
Kind/Feature
New functionality

  
Kind/Security
This is security issue

  
Kind/Testing
Issue or pull request related to testing

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

No labels
Agent
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
joshtronic
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
joshtronic/igor#241
No description provided.