fix(agent): list available tools for unknown calls#4360
Conversation
|
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:
📝 WalkthroughWalkthroughThe change propagates tool visibility state and sorted available tool names through tinyagents tool-policy and unknown-tool handling. It also derives visible names from tool specifications and updates CI allowlists and link-check exclusions. ChangesTool Visibility Propagation
CI Workflow Configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AgentSession
participant ChatTurnGraph
participant TinyagentsIntegration
participant ToolPolicyMiddleware
participant UnknownToolAdapter
AgentSession->>ChatTurnGraph: derive visible tool names from visible tool specs
ChatTurnGraph->>TinyagentsIntegration: pass visibility filter state
TinyagentsIntegration->>ToolPolicyMiddleware: pass visibility_filter_active
TinyagentsIntegration->>UnknownToolAdapter: pass sorted available tool names
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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
added
agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 257a77d5fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/agent/harness/subagent_runner/ops/tool_source.rs (1)
98-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
format_available_tools_hinthere for consistency.This branch still builds the hint inline via
available.join(", "), bypassing the centralized formatter this PR introduces. Whenavailableis empty (e.g., emptyallowed_namesand no lazy resolver) the message degrades to a trailingAvailable tools:instead of the helper'sNo tools are available in this turn.fallback.♻️ Proposed alignment
let available = self.available_tool_names(); let text = format!( - "Error: tool '{}' is not available to the {} sub-agent. Available tools: {}", - call.name, - self.agent_id, - available.join(", ") + "Error: tool '{}' is not available to the {} sub-agent. {}", + call.name, + self.agent_id, + crate::openhuman::agent::harness::engine::tools::format_available_tools_hint(&available) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent/harness/subagent_runner/ops/tool_source.rs` around lines 98 - 104, The unavailable-tool error message is still formatting the hint inline instead of using the centralized formatter. Update the `ToolSource::get_tool` branch to call `format_available_tools_hint` with the `available_tool_names()` result so the message stays consistent. This ensures the fallback text is used when there are no tools available, instead of leaving a blank `Available tools:` suffix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/agent/harness/subagent_runner/ops/tool_source.rs`:
- Around line 98-104: The unavailable-tool error message is still formatting the
hint inline instead of using the centralized formatter. Update the
`ToolSource::get_tool` branch to call `format_available_tools_hint` with the
`available_tool_names()` result so the message stays consistent. This ensures
the fallback text is used when there are no tools available, instead of leaving
a blank `Available tools:` suffix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8fa0f09-e7ef-4279-a987-c0ae2610213e
📒 Files selected for processing (5)
src/openhuman/agent/harness/engine/tool_source.rssrc/openhuman/agent/harness/engine/tools.rssrc/openhuman/agent/harness/session/agent_tool_exec.rssrc/openhuman/agent/harness/subagent_runner/ops/tool_source.rssrc/openhuman/agent/harness/test_support_tests.rs
coderabbitai
Bot
removed
agent
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/agent_session_turn_raw_coverage_e2e.rs (1)
824-862: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting the “Available tools” hint for the
hidden_toolerror.The ordered tool-call assertion and the allowed-tools boundary check look correct and match the scripted provider order and policy rendering logic. However, since this test specifically exercises the unknown-tool path (
hidden_tool), it would be a good opportunity to also assert that the tool_result content includes the newAvailable tools: ...hint introduced by this cohort, rather than only checking the error status tag.♻️ Suggested addition
assert!(joined.contains("<tool_result name=\"hidden_tool\" status=\"error\">")); + assert!( + joined.contains("Unknown tool: hidden_tool") && joined.contains("Available tools:"), + "expected unknown-tool error to include available-tools hint: {joined}" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agent_session_turn_raw_coverage_e2e.rs` around lines 824 - 862, The unknown-tool path in this test already checks the hidden_tool error tag, but it should also verify the new “Available tools” hint appears in the tool_result content. Update the assertions around the provider output in the existing test to check the joined message text includes the hint for hidden_tool, using the same provider.requests()/joined content and the hidden_tool tool_result as the anchor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/agent_session_turn_raw_coverage_e2e.rs`:
- Around line 824-862: The unknown-tool path in this test already checks the
hidden_tool error tag, but it should also verify the new “Available tools” hint
appears in the tool_result content. Update the assertions around the provider
output in the existing test to check the joined message text includes the hint
for hidden_tool, using the same provider.requests()/joined content and the
hidden_tool tool_result as the anchor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afeb34ad-4719-43ac-86fc-3c27926a7cfe
📒 Files selected for processing (1)
tests/agent_session_turn_raw_coverage_e2e.rs
coderabbitai
Bot
added
agent
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Review — #4360 fix(agent): list available tools for unknown calls
Walkthrough. The change enriches the "unknown tool" / "not available" recovery messages so a hallucinated tool call gets an Available tools: … hint the model can self-correct against, and exempts that guidance from the byte-cap truncation so it survives a small budget. The hint is correctly derived from the visible + policy-filtered set (not the full registry), so the "does it leak the curated tool set / blow up tokens" concern is largely a non-issue — it lists tool names only, bounded by the agent's advertised surface (see caveat in Major #2). The core wiring — visibility_filter_active threaded separately from the policy-filtered allowed set, and turn/core.rs switching visible_tool_names to the policy-filtered visible_tool_specs — is sound and matches the existing ParentExecutionContext precedent in turn/tools.rs:125-131. My main concerns are maintainability/dead-path and cross-PR collision, not correctness. No blockers.
Changed files
| File | What | Assessment |
|---|---|---|
tinyagents/mod.rs |
visibility_filter_active field + available_tool_names (allowed-filtered), wired into ToolPolicyMiddleware/UnknownToolAdapter |
✅ correct |
tinyagents/middleware.rs |
visibility_filter_active param, available_tools_hint(), is_recovery_guidance() truncation exemption |
✅ correct, one caveat |
tinyagents/tools.rs |
UnknownToolAdapter carries available_tool_names; new pub(crate) format_available_tools_hint |
✅ correct |
agent/harness/session/turn/core.rs |
visible names from visible_tool_specs; visibility_filter_active from raw scope |
✅ correct (see Minor #3) |
agent/harness/session/agent_tool_exec.rs |
available_tool_names_for_ctx + hints on 2 error branches + 2 tests |
|
tinyagents/tests.rs, tests/agent_session_turn_raw_coverage_e2e.rs |
regression coverage | ✅ good, one gap |
Major
1. agent_tool_exec.rs changes target a code path with no production callers (test-only).
run_agent_tool_call is reached only via execute_tool_call → execute_tools, and execute_tools has zero non-test callers anywhere in the repo (grep: every other reference is a doc comment). turn/core.rs:695 confirms "the legacy run_turn_engine has been removed" and every chat/sub-agent turn now routes through run_turn_via_tinyagents_shared. So the ~140 added lines here (available_tool_names_for_ctx + the two Available tools: branches + session_tool_executor_unknown_tool_* tests) modify and test a path that no longer executes in production. The real production behavior lives entirely in middleware.rs / tools.rs. This isn't a bug, but the two new tests read as covering "the session unknown-tool path" when they actually exercise dead code — false coverage confidence. Recommend either (a) dropping the agent_tool_exec.rs hunk and relying on tinyagents/tests.rs + the e2e test, or (b) adding a one-line comment noting the path is retained for parity only. (If I've missed a dynamic/trait caller, disregard — but I checked the whole tree.)
2. Direct collision + intent overlap with open M3gA-Mind PRs on the same functions.
- #4390 (
fix(agent-harness): structured policy-block messages) edits bothagent_tool_exec.rsandtinyagents/middleware.rs, restructuring the exact "not available to this agent" / policy-deny message surface that this PR appends hints to. High textual-conflict risk and semantic duplication risk — #4390's "workaround" text may duplicate #4360'sAvailable tools:hint. These two are solving adjacent halves of the same problem and should be sequenced/coordinated, not merged blind. - #4389 (
change strategy on repeated unproductive tool results) editstinyagents/middleware.rsandtinyagents/mod.rs, both adding middleware registration insiderun_turn_via_tinyagents_shared. Merge-order conflict is near-certain; also functionally adjacent (the richer-but-still-stable unknown-tool message keepsRepeatedToolFailureMiddleware/no-progress detection working, which is good — just call it out). - #4386 / #4387 both touch
turn/core.rs; low-to-moderate textual conflict with this PR's lines 928-949. - #4419 (merged, observability timeline name) is orthogonal and complementary — it fixes the attempted name shown in the timeline; #4360 fixes the result content. No file overlap. But note this branch has not rebased on #4419 (its
observability.rsis still at the #4249 base commit), so the sentinel timeline-name fix only applies once #4360 lands on top of current main — verify at merge. No action needed beyond a rebase.
Minor
3. Recovery-guidance truncation exemption removes all byte-capping. is_recovery_guidance() skips the budget backstop entirely for these messages. For a wildcard-visibility agent with a very large advertised tool set (e.g. a Composio-heavy agent), available_tool_names is the whole callable set and the Available tools: … line is now injected uncapped on every hallucinated call. It's names-only and bounded by the advertised surface, so no leak, but consider capping the hint (e.g. first N names + … (+K more)) so the exemption can't produce a multi-KB message. The model-controlled requested tool name also rides into this un-truncated message — again bounded in practice, but worth a length guard.
4. Duplicated format_available_tools_hint. agent_tool_exec.rs defines its own private copy identical to the new pub(crate) one in tinyagents/tools.rs. If #1 keeps the agent_tool_exec.rs code, reuse the tinyagents one instead of duplicating the fallback string ("No tools are currently available.").
5. Semantic downgrade in the wildcard-scope + channel-readonly case. Because allowed is now the policy-filtered set, a channel-denied tool (e.g. a write tool on a readonly channel) is no longer in valid_tools, so a call to it is rewritten onto the sentinel and — since visibility_filter_active is false in the wildcard case — falls through to UnknownToolAdapter producing Unknown tool: <write_tool> rather than a "denied by policy / not available on this channel" message. The mod.rs doc comment says this is intentional (the model never saw the tool advertised, so "unknown" is defensible), and the e2e test confirms denied by policy 'round17-deny' still fires for named-rule denials. Just confirm the "unknown" wording for channel-permission-denied tools is the desired model-facing UX.
Nitpicks / questions
- PR description is stale. It claims hints are applied to "sub-agent tool sources … allowlists/lazy resolver slugs", but the final diff has no
subagent_runner/ops/tool_source.rsorengine/tools.rschanges (those existed at commit257a77dthat CodeRabbit/Codex reviewed, then were dropped when the branch merged main). Sub-agents now get the hint solely viaUnknownToolAdapter(subagent=true). Please update the description so reviewers aren't hunting for reverted code — and confirm the drop of thesubagent_runnerpath was intentional (that path's un-hinted error was the subject of the outstanding Codex nitpick, which is now moot). - The e2e test asserts the ordered tool-call names and the allowed-tools boundary but not the
Available tools:hint on thehidden_toolresult (CodeRabbit flagged this). One extraassert!(joined.contains("Available tools:"))would close the loop. available_tools_hint()in the middleware sortsvisible_tool_names(alreadyallowed) on every call — fine given it only fires on the error path, but it could be precomputed alongsideavailable_tool_namesfor symmetry withUnknownToolAdapter.
Looks good
- Threading
visibility_filter_activeseparately from the (policy-filtered)allowedset is the right call — it preserves "genuinely unknown, no scope → generic Unknown tool" vs "scoped, hidden → not available to this agent". Themod.rsdoc comment explaining why is excellent. - Hint is built from
valid_toolsminus the sentinel / from the policy-filtered visible set, so hidden and policy-denied tools are not advertised — the leak concern is properly handled, andtinyagents/tests.rs::unknown_tool_hint_uses_the_policy_filtered_available_setlocks it in end-to-end. - Preserving the
Unknown tool: <name>substring keeps backward-compat with anything grepping the legacy wording.
Net: no blockers; correctness is solid. Please (a) resolve the dead-path question for the agent_tool_exec.rs hunk, (b) coordinate merge order with #4390/#4389, and (c) refresh the PR description.
# Conflicts: # src/openhuman/agent/harness/session/agent_tool_exec.rs # src/openhuman/tinyagents/middleware.rs # src/openhuman/tinyagents/mod.rs # src/openhuman/tinyagents/tools.rs # tests/raw_coverage/agent_session_turn_raw_coverage_e2e.rs
coderabbitai
Bot
added
the
rust-core
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci-lite.yml (1)
863-866: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPrevent potential shell injection by using environment variables.
Using expression syntax (
${{ }}) directly within a bash script can lead to shell injection or syntax errors if the variable contains unexpected characters. Static analysis tools often flag this pattern because it circumvents the runner's safe environment variable assignment. It is best practice to pass context variables safely via theenvblock.
.github/workflows/ci-lite.yml#L863-L866: Assign the${{ needs.changes.outputs.* }}variables toenvblock variables (e.g.,COVERAGE,FRONTEND_NEEDED) and use those inside the script..github/workflows/ci-lite.yml#L906-L913: Assign${{ github.base_ref }}and${{ github.event.before }}to environment variables (e.g.,BASE_REFandEVENT_BEFORE) and use those inside the script..github/workflows/ci-lite.yml#L948-L952: Assign${{ steps.coverage-compare.outputs.ref }}to an environment variable (e.g.,COMPARE_REF) and use it in thediff-covercommand instead of interpolating it directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-lite.yml around lines 863 - 866, Prevent shell injection in the workflow by moving the interpolated GitHub Actions expressions at .github/workflows/ci-lite.yml lines 863-866, 906-913, and 948-952 into step-level environment variables, then reference those variables within the bash script and diff-cover command; preserve the existing variable meanings and behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci-lite.yml:
- Around line 863-866: Prevent shell injection in the workflow by moving the
interpolated GitHub Actions expressions at .github/workflows/ci-lite.yml lines
863-866, 906-913, and 948-952 into step-level environment variables, then
reference those variables within the bash script and diff-cover command;
preserve the existing variable meanings and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcadb589-78ac-4158-a141-c7792c2f1efd
📒 Files selected for processing (2)
.github/workflows/ci-lite.yml.github/workflows/pr-quality.yml
2 participants
Summary
Problem
search_files.Unknown tool: <name>, which gave the model no concrete recovery path.Solution
run_one_tool.Unknown tool: ...substrings for compatibility while addingAvailable tools: ...context.Submission Checklist
cargo-llvm-covenforces changed-line coverage.## Related— N/A: no coverage-matrix feature ID applies.docs/RELEASE-MANUAL-SMOKE.md) — N/A: no release-cut/manual smoke surface touched.Closes #NNNin the## RelatedsectionImpact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/GH-4118-unknown-tool-list257a77d5fe24e3eb46d3109abe870e411ec810aaValidation Run
pnpm --filter openhuman-app format:check— N/A: Rust-only change; frontend app not touched.pnpm typecheck— N/A: Rust-only change; TypeScript not touched.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib unknown_tool -- --nocapture;GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib visible_tool_names_whitelist_rejects_filtered_out_tools -- --nocapturecargo fmt --all -- --check;GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml;git diff --checkValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
Unknown tool: <name>prefix remains in tool results.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Chores