Stephen Hemminger <[email protected]> writes: > On Tue, 9 Jun 2026 14:26:47 -0400 > Aaron Conole <[email protected]> wrote: > >> Add an iterative tool-use loop to review-patch.py for the Anthropic >> and OpenAI providers. The reviewer can now look up additional context >> from the DPDK source tree when the patch alone is insufficient, >> rather than having to guess at surrounding code, API contracts, or >> function signatures. >> >> Tool calling is enabled by default with a limit of 10 rounds. Pass >> '--tool-rounds 0' to disable it and restore the previous single-shot >> behavior. The round limit prevents runaway cost on large patches >> that when reached will force the model to deliver a final judgement. >> >> Initial tool set: >> - grep Searches for regex across the file system with >> optional path restrictions and case-insensitive >> matches. >> - file_read Line range read of a specific path. >> >> Both tools are limited to the repository root to prevent path >> traversal. Path outputs are relative to the repo root. >> >> The system prompt is extended when tool calling is active to >> encourage the model to use tools only when genuinely needed, >> keeping unnecessary round trips and token costs under control >> and to a minimum. >> >> Internally, _common.py gains send_request_raw() (returning the >> raw response dict) so the tool-calling loops can inspect >> stop_reason / finish_reason before extracting text. >> >> Signed-off-by: Aaron Conole <[email protected]> >> --- > > Well AI saw the security bypass here... > > Error > ===== > > devtools/ai/review-patch.py: path containment check is bypassable > (_tool_grep and _tool_file_read) > > repo_resolved = Path(repo_root).resolve() > search_path = (repo_resolved / rel_path).resolve() > if not str(search_path).startswith(str(repo_resolved)): > return "Error: path is outside the repository" > > str.startswith() on the resolved path string is a prefix match, not a > path-component match. If repo_root resolves to /home/me/dpdk, then > /home/me/dpdk-private/secrets passes the check, since the string starts > with "/home/me/dpdk". The commit message states both tools are "limited > to the repository root to prevent path traversal" -- that property does > not hold for sibling directories sharing the name prefix. Same bug in > both tool helpers. > > # is_relative_to (3.9+), raises nothing, no string games > if not search_path.is_relative_to(repo_resolved): > return "Error: path is outside the repository" > > (Absolute rel_path and ../ escapes are already caught by resolve() + > this fix; only the prefix case is currently open.) > > > Warning > ======= > > devtools/ai/_common.py: unrelated reformatting mixed into a feature > patch. The docstring re-wrapping in print_token_summary(), > _extract_text(), _print_verbose_usage(), and the classify_review() > regex reflow in review-patch.py are cosmetic churn unrelated to tool > calling. Drop them or split into a separate cleanup; they inflate the > diff and obscure the actual change. > > devtools/ai/review-patch.py: grep --include list excludes the files the > tool prompt points the model at. _tool_grep restricts to > *.[ch]/*.py/*.rst/*.ini, but TOOL_PROMPT_EXTENSION explicitly directs > the model to check ".ci/" scripts, meson.build, and MAINTAINERS. Those > are extensionless or shell, so grep silently returns "No matches found" > rather than surfacing the omission. Either widen the include set or > note the restriction in the tool description so the model doesn't draw > false conclusions from empty results. > > > Info > ==== > > xai is OpenAI wire-compatible and already shares build_openai_request() > elsewhere, but tool calling is gated to provider == "openai" in > call_api() and the verbose banner. xai (and any future > OpenAI-compatible provider) falls back to single-shot with no notice. > Reusing call_api_with_tools_openai() for xai looks free. > > send_request_raw() bypasses _extract_text(), which is where the > "error" in result check lived. The tool loops read stop_reason / > choices directly, so a provider error returned with HTTP 200 yields an > empty string and a misleading "clean" review instead of a hard failure. > Worth a guard on api_result.get("error") at the top of each loop body. > > Default-on at 10 rounds is a behavior and cost change for every existing > caller, and grants repo-wide read to the model by default. Reasonable > for an RFC to propose, but worth calling out explicitly for the list -- > some CI users may want opt-in rather than opt-out.
I think I've addressed many of the above issues. Posting the 'PATCH' version.
