Skip to content

fix(code): route orchestrated tool calls through the confirmation gate - #2853

Merged
kovtcharov-amd merged 2 commits into
mainfrom
security/codeagent-tool-confirmation
Aug 7, 2026
Merged

fix(code): route orchestrated tool calls through the confirmation gate#2853
kovtcharov-amd merged 2 commits into
mainfrom
security/codeagent-tool-confirmation

Conversation

@kovtcharov-amd

Copy link
Copy Markdown
Collaborator

CodeAgent's orchestrator ran tools by pulling the callable straight out of _TOOL_REGISTRY, skipping Agent._execute_tool and with it the user-confirmation guardrail — so run_shell_command, write_file and any MCP tool registered by a co-resident agent in the same process all executed with no prompt during orchestrated runs. Orchestrated calls now take the same path as the agent loop's, so the same tools prompt whether they are invoked directly or through a checklist.

Same vulnerability class as #2846, which fixes it for MCP tools in the base agent; this is the CodeAgent half. Reported via responsible disclosure.

A denial is treated as a user decision rather than a transient fault: it is never retried, it stops the checklist, and the orchestrator stops replanning instead of queuing another prompt for the same work. Result parsing now reads the base agent's status field alongside the legacy success key, so a denied or errored call can no longer be mistaken for success by tools that return a bare payload dict.

Also fixes warnings being dropped when a checklist exits early — a stopped run now still reports what the completed items produced.

Test plan

  • python -m pytest hub/agents/code/python/tests/test_tool_executor_confirmation.py -q — 16 tests, 19 subtests
  • Confirm a denied gated tool does not execute (asserted via side-effect, not just the return value)
  • Confirm a denied item stops the checklist and is not retried by the error handler
  • Confirm the orchestrator halts replanning after a denial rather than re-prompting
  • Run a normal gaia-code generation task through the orchestrator and confirm non-gated tools are unaffected
  • python util/lint.py --all

CodeAgent's orchestrator ran tools by pulling the callable straight out of
_TOOL_REGISTRY, bypassing Agent._execute_tool entirely. That skipped the
user-confirmation guardrail for every gated tool the orchestrator touched,
including run_shell_command and write_file, and any MCP tool registered by a
co-resident agent in the same process. The executor now delegates to
_execute_tool, so orchestrated calls take the same path as the agent loop's:
the gate, name resolution, bounded execution and error formatting.

A denial is a user decision, not a transient fault, so it is threaded through
as its own outcome rather than a generic failure. ItemExecutionResult carries
a denied flag, the error handler never retries a denied item, and the
orchestrator stops replanning instead of queuing another prompt for the same
work. Result parsing now reads the base agent's status field alongside the
legacy success key, so a denied or errored call is no longer mistaken for
success by tools that return a bare payload dict.

Also fixes warnings being dropped when a checklist exits early — they are now
collected before the exit checks, so a stopped run still reports what the
completed items produced.
@github-actions github-actions Bot added documentation Documentation changes agent::code Code agent changes labels Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions — safe to merge; one gap worth closing in this PR.

This PR fixes a real guardrail bypass: CodeAgent's orchestrator was calling the tool registry directly, so shell/file-mutating tools (run_cli_command, write_file, …) ran during an orchestration without ever hitting the user-confirmation gate. Routing the executor through Agent._execute_tool closes that, and denials now stop the checklist cleanly instead of silently succeeding or looping back to re-prompt. The timeout overrides are the necessary companion — the base path now wraps every tool in a 180s bound, which would have killed long npm/prisma installs. The fix is correct and the test suite pinning it is genuinely good.

The one thing to fix: the new regression test that pins this guardrail (test_tool_executor_confirmation.py) isn't run by any CI workflow, so it can't actually prevent this bypass from coming back. Add it to test_code_agent.yml — it's fast and needs no LLM. Details and a one-click suggestion below.

Real-world evidence

evidence-bundle.md is present and substantive (no-inference ubuntu-latest lane; real-world strix-halo tier deferred). Relaying what it captured:

  • The new gate test file was run for real, not trusted from the diff:
    $ python -m pytest hub/agents/code/python/tests/test_tool_executor_confirmation.py -v
    16 passed, 19 subtests passed in 1.03s
    
    Includes the filesystem-canary assertions proving a denied tool's body never runs (plus a positive control that the same command does create the canary when approved).
  • Timeout overrides resolved live on a real CodeAgent, matching the diff exactly: run_cli_command→1800, fix_code→1900, setup_nextjs_testing→1500, manage_data_model→3900, generate_style_tests→900.
  • gaia-code --help / --list-tools run clean; the touched tools are all registered.
  • Spot regression: sibling orchestration/tool suites show the same pass/fail split as base a905b057 (pre-existing failures reproduced verbatim on the base commit), so nothing here regressed.
  • N/A for HTTP/MCP surfaces — this PR touches no route or MCP tool. UI confirmation-dialog pixels deferred to the strix-halo lane.

The evidence supports the verdict: the guardrail demonstrably holds and the timeout budgets are real. It also independently surfaced the CI-wiring gap below.

🔍 Technical details

🟡 Important

New guardrail regression test never runs in CI (.github/workflows/test_code_agent.yml)

The whole point of this PR is a durable security guardrail, and test_tool_executor_confirmation.py is what protects it from regressing. But the workflow enumerates test files explicitly (lines 80, 102, 108–109, 122, 138) and this new file is in none of them — so a future refactor that reintroduces the direct-registry bypass keeps CI green. This is exactly the "test exists but doesn't run the surface" gap CLAUDE.md warns about. The file is fast (~1s) and needs no LLM, so it belongs in the unit-test step:

          # Validators and write-guardrail tests (moved here from tests/unit and
          # tests/ root during the hub migration; keep them gated).
          python -m pytest \
            hub/agents/code/python/tests/test_code_validators.py \
            hub/agents/code/python/tests/test_file_io_guardrails.py \
            hub/agents/code/python/tests/test_tool_executor_confirmation.py \
            -v --tb=short

Notes (non-blocking)

  • 🟢 Timeout backstops are well-reasoned. The run_cli_command outer bound (1800s) stays safely above the largest inner budget callers pass (1200s for create-next-app / prisma init / db-sync), so the thread guard won't abandon a legitimately long install. cli_tools.py:236-239.
  • 🟢 test_every_checklist_tool_has_a_deliberate_timeout is a nice inversion guard — it forces a decision on any new slow checklist tool rather than re-listing the ones already fixed (which is how fix_code was originally missed). test_tool_executor_confirmation.py:551.

Strengths

  • Correct fix at the right seam. Delegating to _execute_tool reuses name resolution, the confirmation gate, bounded execution, and error formatting instead of duplicating them; the dead _TOOL_REGISTRY import is cleanly removed (agent.py).
  • Denial modelled end-to-end, not just at one layer. denied propagates ItemExecutionResult → ChecklistExecutionResult.denied → orchestrator break, with error_recoverable=False so a denial is never retried and never skipped by stop_on_error. The parsing correctly treats _execute_tool's status ∈ {denied, error} shapes as failure while a bare payload dict still counts as success (checklist_executor.py:1735-1760).
  • Tests target behaviour, not mocks. Filesystem canaries prove the body didn't run; the TestRealGateReachesChecklist suite pins the real _execute_tool denial key against the parser so the two halves can't drift green while the bypass returns. Docs updated too (docs/guides/code.mdx).

… lane

The test pinning the orchestrator to the confirmation gate was not in any
workflow, so it guarded nothing — the bypass could return without CI
noticing. It is fast and needs no LLM, so it joins the existing gated
validator/guardrail set.
@kovtcharov-amd

Copy link
Copy Markdown
Collaborator Author

Fixed in 9b579b4 — good catch. A regression test no workflow runs guards nothing, which defeats the point of adding it.

test_tool_executor_confirmation.py now runs in test_code_agent.yml alongside the existing validator and write-guardrail set. Verified the three files pass together as CI will run them: 93 passed, 19 subtests in ~98s, no LLM required.

@kovtcharov-amd
kovtcharov-amd merged commit c3d9ec1 into main Aug 7, 2026
7 checks passed
@kovtcharov-amd
kovtcharov-amd deleted the security/codeagent-tool-confirmation branch August 7, 2026 06:16
@itomek itomek mentioned this pull request Aug 10, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent::code Code agent changes documentation Documentation changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant