fix(reborn): discourage disabled tool workarounds - #5307
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds visible capability-policy text, gateway suppression for unavailable tools, and E2E coverage for disabled echo requests. ChangesDisabled tool workaround path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/ironclaw_turns/src/run_profile/instruction_bundle.rs`:
- Around line 24-28: `CAPABILITY_SURFACE_USAGE_POLICY` is inline prompt text in
Rust and should be moved to a prompt file loaded with `include_str!` to match
the repo invariant for prompt templates in `*.rs` files. Update the
`instruction_bundle.rs` usage to read from a shared prompt asset under
`crates/ironclaw_turns/prompts/` (or the engine’s shared prompts directory),
then align the duplicated literals in `tests/mock_llm.py` and
`tests/agent_loop_host_contract.rs` to the same source text so the policy stays
synchronized across all three locations.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7376564-a169-4778-9d22-4a080f2ad3d7
📒 Files selected for processing (4)
crates/ironclaw_turns/src/run_profile/instruction_bundle.rscrates/ironclaw_turns/tests/agent_loop_host_contract.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
|
🚅 Deployed to the ironclaw-pr-5307 environment in ironclaw-ci-preview
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/mock_llm.py (1)
884-888: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRoute this case through
CANNED_RESPONSES.The new disabled-tool behavior was added as bespoke
match_response/match_tool_callbranches, which creates a second mock-dispatch path instead of extending the file’s canonical last-user-message matcher. Please fold this intoCANNED_RESPONSESand keep the policy check as data for that canned case.As per coding guidelines,
Add new canned LLM responses to CANNED_RESPONSES in mock_llm.py by pattern-matching against the last user message; unmatched messages return a default response.Also applies to: 1131-1138, 1333-1342
🤖 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/e2e/mock_llm.py` around lines 884 - 888, Route the new disabled-tool behavior through CANNED_RESPONSES instead of bespoke match_response/match_tool_call branches. Update mock_llm.py so the canonical last-user-message matcher handles this case, with _conversation_has_disabled_tool_workaround_policy used as the policy/data check for that canned response. Keep the existing match dispatch unified in CANNED_RESPONSES and let unmatched messages fall back to the default response.Source: Coding guidelines
🤖 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.
Inline comments:
In `@tests/e2e/scenarios/test_reborn_webui_v2_smoke.py`:
- Around line 316-326: The fixture disabled_echo_shell_ask_policy needs safer
cleanup around the permission mutations. Move the setup calls to
_set_tool_permission for builtin.echo and builtin.shell inside the try block so
cleanup always starts even if the second mutation fails. In the finally block,
reset builtin.echo and builtin.shell independently using separate cleanup
attempts so one restore failure does not prevent the other from running.
---
Outside diff comments:
In `@tests/e2e/mock_llm.py`:
- Around line 884-888: Route the new disabled-tool behavior through
CANNED_RESPONSES instead of bespoke match_response/match_tool_call branches.
Update mock_llm.py so the canonical last-user-message matcher handles this case,
with _conversation_has_disabled_tool_workaround_policy used as the policy/data
check for that canned response. Keep the existing match dispatch unified in
CANNED_RESPONSES and let unmatched messages fall back to the default response.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6aebbeaa-93c3-46a3-ade1-9bf1afdb45ae
📒 Files selected for processing (5)
crates/ironclaw_turns/prompts/capability_surface_usage_policy.mdcrates/ironclaw_turns/src/run_profile/instruction_bundle.rscrates/ironclaw_turns/tests/agent_loop_host_contract.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/e2e/mock_llm.py`:
- Around line 113-117: The premature fallback in match_tool_call is caused by
REQUESTED_UNAVAILABLE_TOOL_TRIGGER being too broad and matching generic “use
<dotted-token> to” phrases before the specific capability handlers run. Tighten
that regex in mock_llm.py to only catch the intended issue 5197 echo workaround
path, such as requiring the exact disabled echo workaround wording or
builtin.echo-style prompts, and keep the UNAVAILABLE_CAPABILITY_POLICY_TEXT
check aligned with that narrow case. If needed, reorder the match_tool_call
branches so gmail/pikastream and other specific handlers are evaluated before
this fallback returns the builtin_shell echo "disabled-test" response.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3e66fbbf-9dae-4e55-aae4-e0a61c17e3ab
📒 Files selected for processing (4)
crates/ironclaw_turns/prompts/capability_surface_usage_policy.mdcrates/ironclaw_turns/tests/agent_loop_host_contract.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/e2e/mock_llm.py`:
- Around line 113-116: The disabled-tool mock case is bypassing the standard
canned-response flow and should be moved into CANNED_RESPONSES. Add the
issue-5197 prompt match to the existing last-user-message pattern table in
mock_llm.py, then key the tool-call suppression from that matched canned
response instead of the standalone REQUESTED_UNAVAILABLE_TOOL_TRIGGER path. Keep
the existing match_response/match_tool_call behavior aligned with the
CANNED_RESPONSES entry so unmatched messages still fall through to the default
response.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2d7e10e7-044e-4f95-b511-4d6db46ae568
📒 Files selected for processing (1)
tests/e2e/mock_llm.py
940529d to
619f664
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@crates/ironclaw_reborn/src/model_gateway.rs`:
- Around line 1155-1168: The unavailable-capability guard in
model_gateway::unavailable_requested_capability_guard is still using visible
namespace membership as a proxy, so explicit requests like gmail.send can slip
through when only unrelated tools such as builtin.shell are visible. Update the
guard to detect explicit capability-request phrasing directly from
latest_user.content and suppress substitute tool calls without relying on
visible_namespaces, using visible_capability_ids and
extract_explicit_capability_ids as the key symbols to adjust. Add a regression
test at the gateway caller that exercises the full path through the model
gateway so the side effect is validated where it is triggered.
In `@tests/e2e/mock_llm.py`:
- Around line 1323-1330: The synthetic builtin_shell fallback in the mock LLM
should also honor the existing deduplication guard so it stays deterministic. In
the REQUESTED_UNAVAILABLE_TOOL_TRIGGER branch, check the same
recent_tool_results state used by the generic fallback before returning the
synthetic builtin_shell call, and suppress the repeat if that tool was already
emitted. Keep the fix within the mock LLM flow around
REQUESTED_UNAVAILABLE_TOOL_TRIGGER and recent_tool_results so the behavior
matches the rest of the tool-selection logic.
In `@tests/e2e/scenarios/test_reborn_webui_v2_smoke.py`:
- Around line 413-415: The test function
test_reborn_v2_disabled_tool_does_not_route_through_shell uses
disabled_echo_shell_ask_policy only for fixture side effects, so Ruff flags it
as an unused argument. Rename that parameter in the test signature to a
leading-underscore variant, or otherwise reference it inside the test, to
satisfy ARG001 while preserving the current behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 52a66966-2211-4021-906f-8a7b4e6e35d6
📒 Files selected for processing (4)
crates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/tests/llm_gateway.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/ironclaw_reborn/src/model_gateway.rs`:
- Around line 1156-1159: The explicit capability filtering in
model_gateway::extract_explicit_capability_request_ids currently keeps only the
first hidden ID, which causes UnavailableCapabilityGuard to block visible
fallback tool calls too. Update the gateway flow to preserve the full explicit
request set, or at minimum the explicitly requested visible capability IDs, and
make UnavailableCapabilityGuard suppress only substitute tools that were not
directly requested. Adjust the caller that constructs the guard so it passes the
richer request information through. Add a regression test at the real gateway
call site that verifies a prompt like “builtin.echo, or builtin.shell if echo is
unavailable” still allows the directly requested visible fallback.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d06718da-592e-4203-94c1-d8e6a68c619c
📒 Files selected for processing (4)
crates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/tests/llm_gateway.rstests/e2e/mock_llm.pytests/e2e/scenarios/test_reborn_webui_v2_smoke.py
|
I would make this fail-closed at the boundary between visible capability inventory and execution routing, not only in the prompt wording. If a requested capability is disabled or absent, the loop can explain that the capability is unavailable, but it should not satisfy the same intent through a broader executable capability unless a fresh visible-capability selection path authorizes that alternate route. The regression fixture I would keep is:
For the mock path, the unavailable-capability response should be accepted because it is the policy-compliant terminal outcome, not because the policy prose happened to appear in prior message history. Boundary: architecture and regression-test feedback only; no claim about running this branch, validating implementation behavior, merge readiness, security review, production readiness, partnership, or customer interest. |
Port from nearai/ironclaw#5307 ("discourage disabled tool workarounds"). When a user disables a tool via `hermes tools` (or runs a restricted-toolset session), the runtime already enforces that the tool can't be invoked — but the model can still route around it by using a general-purpose tool (e.g. shelling out via terminal) to do what the disabled dedicated tool would have done, or by treating a never-enabled capability as something to work around silently. Adds a short, universal DISABLED_TOOL_GUIDANCE block to the cached system prompt telling the model: if the user names a capability with no available tool, report it as unavailable/disabled rather than substituting another tool. General-purpose tools remain fine for their own legitimate tasks. Follows the existing universal-guidance pattern (TASK_COMPLETION_GUIDANCE, PARALLEL_TOOL_CALL_GUIDANCE): constant in prompt_builder, injected in system_prompt gated on agent.valid_tool_names + config flag agent.disabled_tool_guidance (default True), wired in agent_init and config DEFAULT_CONFIG. Costs ~80 tokens once in the cached prefix.
Summary
builtin.echo = disabled, guarding against fallbackbuiltin.shellworkaround requests.Linked Issue
Closes #5197
Validation
cargo fmt --package ironclaw_turnscargo test -p ironclaw_turns loop_prompt_port_materializes_memory_surface_and_safety_as_host_owned_refs --test agent_loop_host_contractcargo test -p ironclaw_turnsgit diff --check./.venv/bin/python -m py_compile mock_llm.py scenarios/test_reborn_webui_v2_smoke.py/v1/modelsreadiness before reaching the test assertion.Security Impact
Low. This tightens model-visible capability guidance so disabled or unavailable capabilities are less likely to be bypassed through broader tools such as shell. Runtime authorization remains the final enforcement layer.
Database Impact
No schema or migration changes.
Blast Radius
Limited to Reborn instruction bundle capability-surface prompt materialization and related Reborn WebUI v2/mock E2E regression coverage.
Rollback Plan
Revert this PR to remove the extra capability-surface policy and the associated regression tests.