fix(agent): list available tools for unknown calls#4360
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughAdds a ChangesUnknown Tool Error Enrichment
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant ToolPolicyMiddleware
participant ToolOutputMiddleware
Agent->>ToolPolicyMiddleware: call unregistered/hidden tool
ToolPolicyMiddleware->>ToolPolicyMiddleware: check visibility_filter_active
ToolPolicyMiddleware->>ToolPolicyMiddleware: build available_tools_hint()
ToolPolicyMiddleware-->>ToolOutputMiddleware: "not available" error + hint
ToolOutputMiddleware->>ToolOutputMiddleware: detect is_recovery_guidance, skip truncation
ToolOutputMiddleware-->>Agent: return full guidance message
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
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