Repository navigation
[codex] Add toolset governance - #92
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPipeline tool handling was refactored around a new Toolset: pipeline stages now create/use ChangesTool Governance & Pipeline Integration
Sequence DiagramsequenceDiagram
participant Pipeline as Pipeline (stage_run_model)
participant Toolset as Toolset
participant Runtime as HookRuntime
participant Executor as DirectiveExecutor
Pipeline->>Toolset: ensure/create toolset & collect_schemas()
Pipeline->>Runtime: receive streamed model tool-call dicts
loop per tool call
Pipeline->>Pipeline: dict → ToolCallRequest
Pipeline->>Toolset: validate_tool_call(request)
alt validation fails
Pipeline->>Pipeline: append tool_result (validation error) + emit ToolResultEvent(is_error)
else
Pipeline->>Toolset: approve_tool_call(request)
Toolset->>Runtime: call approve_tool_call hooks
Runtime-->>Toolset: Directive / decision
alt directive present
Toolset->>Executor: execute directive (if executor provided)
Executor-->>Toolset: approval outcome
end
alt approval denied
Pipeline->>Pipeline: append tool_result (approval error) + emit ToolResultEvent(is_error)
else
Pipeline->>Toolset: execute_tools([requests], options)
alt batch hook used
Toolset->>Runtime: execute_tools_batch(payload)
else
loop per call
Toolset->>Runtime: execute_tool(name, args)
end
end
Runtime-->>Toolset: execution results
Toolset-->>Pipeline: ToolExecutionResult[]
Pipeline->>Pipeline: normalize & append tool_result entries + emit ToolResultEvent
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/agentkit/tools/toolset.py`:
- Around line 278-289: The conversion in _envelope_raw_result currently treats
any None as "missing", which misclassifies valid None results; change the
unhandled-case signal to an explicit sentinel or dedicated exception (e.g.,
UNHANDLED = object() or UnhandledToolResult) and update the logic in
_envelope_raw_result to set ToolExecutionResult.missing=True only when result is
that sentinel/exception, leaving actual None return values to be returned as
successful results in ToolExecutionResult.result; refer to ToolExecutionResult
and the _envelope_raw_result function (and ensure callers that previously used
None for unhandled now return the chosen sentinel/exception).
- Around line 214-223: The batch path currently calls
self._runtime.call_first("execute_tools_batch", ...) and awaits it via
self._await_if_needed without converting exceptions/timeouts into
ToolExecutionResult, so wrap the call/await to execute_tools_batch in a
try/except that mirrors _execute_one's error handling: catch exceptions and
timeouts from self._await_if_needed and construct a list of
ToolExecutionResult(error=...) entries (one per call) instead of letting the
exception bubble, ensuring batch_results is always a list of ToolExecutionResult
objects; reference the symbols execute_tools_batch, _runtime.call_first,
_await_if_needed, _execute_one, and ToolExecutionResult when implementing this
conversion and also handle unexpected non-list/None responses by mapping them to
error ToolExecutionResult instances.
- Around line 178-193: The approve_tool_call result is not awaited before
type-checking, so an async hook returned by
self._runtime.call_first("approve_tool_call", ...) will be treated as
non-Directive and fail-closed; update the code around self._runtime.call_first
to await the result (e.g., assign directive = await
self._runtime.call_first(...)) before checking isinstance(directive, Directive)
and before returning ToolApprovalResult instances, ensuring the async hook path
is handled the same as sync hooks and preserving existing checks involving
Directive and self._directive_executor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ebaed29-e0a4-4534-9d5a-9d4ac6d75178
📒 Files selected for processing (6)
coding-agent/src/agentkit/runtime/pipeline.pycoding-agent/src/agentkit/tools/__init__.pycoding-agent/src/agentkit/tools/toolset.pycoding-agent/tests/agentkit/runtime/test_pipeline.pycoding-agent/tests/agentkit/tools/test_toolset.pycoding-agent/tests/integration/test_pipeline_e2e.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/agentkit/tools/toolset.py`:
- Around line 348-350: The current early-return treats any directive as approved
when self._directive_executor is None, which bypasses Reject/AskUser directives;
change the logic in the approval branch that references
self._directive_executor, directive, and ToolApprovalResult so that if
_directive_executor is None you fail-closed instead of approving—return a
ToolApprovalResult with approved=False (and include the directive and an
explanatory reason/message) or raise a clear exception indicating the missing
DirectiveExecutor, and otherwise continue to call await
self._directive_executor.execute(directive).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 651872a1-a716-4697-8164-1ddf3870916a
📒 Files selected for processing (2)
coding-agent/src/agentkit/tools/toolset.pycoding-agent/tests/agentkit/tools/test_toolset.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
coding-agent/src/agentkit/tools/toolset.py (1)
418-422:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn error envelopes instead of raising on batch result-count mismatch.
On Line 419, this
ValueErrorescapesexecute_tools()and aborts the round, while other batch failures are already normalized via_error_results(...). Keep failure handling consistent and per-call.Suggested fix
if len(raw_results) != len(calls): - raise ValueError( - "execute_tools_batch returned " - f"{len(raw_results)} results for {len(calls)} tool calls" - ) + return self._error_results( + calls, + ValueError( + "execute_tools_batch returned " + f"{len(raw_results)} results for {len(calls)} tool calls" + ), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/agentkit/tools/toolset.py` around lines 418 - 422, The current length-check in execute_tools_batch raises a ValueError when len(raw_results) != len(calls), which aborts the round; instead build and return per-call error envelopes using the existing _error_results helper so failures remain normalized. Replace the raise with code that constructs a clear error message referencing raw_results and calls, then return _error_results(calls, error_message) (or otherwise produce one error envelope per call) so execute_tools sees consistent per-call results rather than an exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/agentkit/tools/toolset.py`:
- Around line 397-407: The batch execution currently calls each batch hook only
once and returns on first failure; update the loop that invokes
hook(tool_calls=raw_payload, ctx=ctx) to honor ToolExecutionOptions.max_retries
by retrying the hook invocation up to options.max_retries times (matching the
retry behavior used by _execute_one): on exception catch and retry (optionally
with the same delay/backoff mechanism used by single-call paths if such
utilities exist), only call return self._error_results(calls, exc) after all
retry attempts are exhausted, and keep using self._await_if_needed(raw_results,
timeout_seconds=options.timeout_seconds) for each attempt; reference symbols to
change: raw_payload, hooks loop, hook(...) call, self._await_if_needed,
options.timeout_seconds, options.max_retries, and self._error_results(calls,
exc).
---
Duplicate comments:
In `@coding-agent/src/agentkit/tools/toolset.py`:
- Around line 418-422: The current length-check in execute_tools_batch raises a
ValueError when len(raw_results) != len(calls), which aborts the round; instead
build and return per-call error envelopes using the existing _error_results
helper so failures remain normalized. Replace the raise with code that
constructs a clear error message referencing raw_results and calls, then return
_error_results(calls, error_message) (or otherwise produce one error envelope
per call) so execute_tools sees consistent per-call results rather than an
exception.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fb3546c-75f8-42be-80bc-33392398c3b8
📒 Files selected for processing (2)
coding-agent/src/agentkit/tools/toolset.pycoding-agent/tests/agentkit/tools/test_toolset.py
🚧 Files skipped from review as they are similar to previous changes (1)
- coding-agent/tests/agentkit/tools/test_toolset.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/agentkit/tools/toolset.py`:
- Around line 329-360: Wrap all calls to approval hooks (the call to hook(...)
and the subsequent await via _await_if_needed) and the call to
self._directive_executor.execute(directive) with try/except so any exception is
caught locally; on exception return ToolApprovalResult(approved=False,
reason="policy") (or use str(getattr(directive,"reason","policy")) if the
exception occurs during directive execution and a directive exists).
Specifically, protect the invocation site of
self._runtime.get_hooks("approve_tool_call")/hook and the await of directive in
the loop, and protect the await of self._directive_executor.execute(directive);
log a warning including the exception type/name and tool name where helpful, but
ensure the function always returns a fail-closed ToolApprovalResult rather than
letting exceptions propagate. Ensure existing checks for isinstance(directive,
Directive) and missing self._directive_executor remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c9b62ee-772a-42bd-b22e-82cc85bfa867
📒 Files selected for processing (2)
coding-agent/src/agentkit/tools/toolset.pycoding-agent/tests/agentkit/tools/test_toolset.py
Approval hook and directive executor exceptions should deny the tool call locally instead of aborting the turn.
Scope single-tool retries to the provider hook that failed, initialize Toolset once per run context, and document the governance semantics surfaced by PR review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
uv run pytest tests/agentkit/tools/test_toolset.py::test_toolset_retries_only_the_failing_tool_provider tests/agentkit/runtime/test_pipeline.py::TestPipeline::test_mount_initializes_toolset_once -quv run pytest tests/agentkit/tools/test_toolset.py tests/agentkit/runtime/test_pipeline.py::TestPipeline::test_mount_initializes_toolset_once tests/integration/test_pipeline_e2e.py::TestPipelineE2E::test_large_tool_result_truncated -quv run pytest tests/agentkit/ -quv run pytest tests/coding_agent/test_pipeline_adapter.py tests/coding_agent/plugins/test_approval.py tests/integration/test_pipeline_e2e.py::TestPipelineE2E::test_large_tool_result_truncated tests/integration/test_pipeline_e2e.py::TestPipelineE2E::test_parallel_tools_execute_concurrently -quv run python -m py_compile src/agentkit/tools/toolset.py src/agentkit/runtime/pipeline.py tests/agentkit/tools/test_toolset.py tests/agentkit/runtime/test_pipeline.py tests/integration/test_pipeline_e2e.pygit diff --checkKnown Existing Failure
uv run pytest tests/coding_agent/test_pipeline_adapter.py tests/coding_agent/plugins/test_approval.py tests/integration/test_pipeline_e2e.py -qstill fails ontest_subagent_turn_persisted_tape_flows_into_eval_adapterbecausecoding-agent/data/eval/golden/parent-child-subagent-001.yamlis absent in this worktree. This PR does not touch that fixture path.