Re: [RFC] devtools: add tool calling support to review-patch.py

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue,  9 Jun 2026 14:26:47 -0400
Aaron Conole <aconole@redhat.com> 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 <aconole@redhat.com>
> ---

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.