fix(agent-runner): round-2 review-agent fixes — never discard buffered findings, symlink-safe read_file, tunable budget (#137)#170
Conversation
…d findings, symlink-safe read_file, tunable budget (#137) Five round-2 fixes to the native review agent (all in services/agent-runner): 1. read_file symlink escape (SECURITY-HIGH): resolve_in_root was purely lexical and followed symlinks — a planted in-repo symlink to /etc/passwd or the SA token could be read. Now canonicalize both the checkout root and the resolved path and verify the target stays inside the root; reject otherwise. Non-existent path → clean "not found". New unit test. 2. read_file truncated line-count: a sliced read of an over-cap file reported a total line count computed from truncated content. Now the message says the file was truncated at the byte cap instead of quoting a bogus total. 3. MAX_TURNS → config: max_turns is now a ReviewConfig knob (review.max_turns / LLM_MAX_TURNS), DEFAULT_MAX_TURNS = 40 (was a hardcoded 16, far too tight on the ~6s/turn deepseek model). ai-helm chart wiring handled separately. 4. Never discard buffered work, never end silently: run_native_agent returns a ReviewOutcome { Finished, Exhausted, Aborted(reason) } instead of bailing on exhaustion. main.rs finalizes on Finished AND Exhausted AND Aborted, with a truncation note on Exhausted and an abort note on Aborted. A real prod run lost 5 findings when exhaustion used to drop the buffer at turn 16. Only a true transport Err posts nothing. Light one-time nudge to call finish once a finding is recorded. 5. Failure detail on the FINAL status: the review-failure/exhaustion/abort detail is carried to the terminal succeeded/failed report instead of a mid-run running report (the control plane clears error_detail on running). Verified: cargo fmt --check, clippy -D warnings, cargo test (45+8 pass). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
✅ AI Governance check passedThis PR declares AI usage, references a source of truth, and provides verification evidence. Thank you. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an operator-tunable turn budget (max_turns) for the review agent, defaulting generously to 40 turns. It refactors the agent loop to return a structured ReviewOutcome (Finished, Exhausted, or Aborted) rather than treating non-fatal outcomes as errors, which ensures that buffered findings are finalized and posted instead of being discarded. Additionally, it resolves a security vulnerability in the read_file tool by canonicalizing paths to prevent symlink escapes outside the repository checkout root. There are no review comments provided, so I have no feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
1. Summary
This PR changes (round-2 fixes to the native review agent, all in
services/agent-runner):read_filesymlink escape (SECURITY-HIGH):resolve_in_rootwas purely lexical (rejects../absolute) but followed symlinks — a malicious PR could plant an in-repo symlink to/etc/passwdor the SA token and have the model read it. It now canonicalizes both the checkout root and the resolved path and verifies the real target stays inside the real root, rejecting otherwise.read_filetruncated line-count: a sliced read of an over-64 KiB file reported a total line count computed from the truncated content, so "(of {total})" lied. The message now says the file was truncated at the byte cap instead of quoting a bogus total.MAX_TURNS→ config-driven: the hardcodedconst MAX_TURNS = 16becomes a tunablereview.max_turnsknob (DEFAULT_MAX_TURNS = 40). 16 was far too tight: turns are ~6s on the deepseek model and the agent records roughly one finding per turn.run_native_agentnow returns aReviewOutcome { Finished, Exhausted, Aborted(reason) }instead of bailing on exhaustion.main.rsfinalizes on Finished AND Exhausted AND Aborted so buffered findings are always posted (with a truncation note on Exhausted, an abort note on Aborted). Only a true transport error posts nothing.succeeded/failedreport instead of a mid-runrunningreport (which the control plane clears on everyrunningtransition).It solves:
add_review_comment, then hit the turn-16 ceiling without callingfinish— the loop returnedErr,main.rstreated it as non-fatal, and all 5 findings were silently discarded with nothing posted. The symlink-escape risk was flagged by Gemini on the feat(agent-runner): per-turn logging + read_file tool, report review failure detail (#137) #167 follow-up.2. Intent
The intent of this PR is:
3. Scope
In Scope
services/agent-runner/src/review/native/tools.rs— symlink-saferead_file(canonicalize + containment check) + honest truncation message + a symlink-escape unit test.services/agent-runner/src/review/native/agent.rs—ReviewOutcomeenum, config-drivenmax_turns, finish-nudge, no longer bails on exhaustion/abort.services/agent-runner/src/bootstrap/config.rs—max_turnsonReviewFile/ReviewConfig+DEFAULT_MAX_TURNS.services/agent-runner/src/main.rs— finalize on all three outcomes, set truncation/abort summaries, carry detail to the terminal status.services/agent-runner/src/review/mod.rs— re-exportReviewOutcome.Out of Scope
max_turnsknob (handled separately; this is the code side only).error_detailonrunningtransitions (separate PR; this PR adapts to it by reporting detail on the terminal status).4. Verification
I verified this change by:
Commands run:
cargo fmt -p agent-runner -- --check cargo clippy -p agent-runner --all-targets -- -D warnings cargo test -p agent-runnerResults:
5. Screenshots / Evidence
add_review_commentthen hit the turn-16 ceiling withoutfinish→ loop returnedErr→main.rsnon-fatal path → all 5 findings discarded, nothing posted. This PR's fix [Ticket]: Implement real authN — better-auth sessions + Rust user store #4 makes that run finalize the buffer with a truncation note instead.resolve_in_rootfollows symlinks (a planted in-repo symlink could read/etc/passwdor the SA token); fix Scaffold monorepo foundation: AI governance, docs/ADRs, Rust + Next.js skeletons #1 + the newread_file_rejects_symlink_escapetest close it.6. Risk Assessment
Risk level:
Potential risks:
Exhausted/Abortedposts a review that previously posted nothing — a behavioural change, but the whole point (and gated by the existing finalize empty-run backstop).canonicalizerequires the path to exist; a non-existent file now maps to "not found" (already the prior behaviour for a missing file).Mitigation:
max_turnsdefaults generously (40) and is config-gated, so behaviour is unchanged where unset except the higher ceiling.7. AI Usage Declaration
AI was used for:
Human verification:
8. Reviewer Focus
Please focus your review on:
🤖 Generated with Claude Code