Skip to content

fix(agent): name substitute tools in background review deny message to prevent tool-denial storm (#61521) - #61618

Open
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/61521-background-review-deny-message
Open

fix(agent): name substitute tools in background review deny message to prevent tool-denial storm (#61521)#61618
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/61521-background-review-deny-message

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Summary

background_review fork advertises the parent's full toolset but only permits skills+memory. The model calls file tools (patch, read_file, write_file) which are denied with a generic message, then tries skill_manage which the read-before-write guard refuses because the model used read_file instead of skill_view. This creates a per-turn tool-denial storm that starves the self-improvement loop.

Change

Updated the deny message in agent/background_review.py to name the correct substitute tools when a non-whitelisted tool is denied:

{tool} is not available in review — use skill_view to read a skill, skill_manage to modify one, skills_list to discover skills, and memory for notes.

Also updated the proactive steering text with the same guidance.

Verification

41 background review tests pass, 0 regressions.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists labels Jul 9, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this addresses #61521 by rewording the background-review deny/steering text to name the correct substitute tools (skill_view/skill_manage/skills_list/memory). Competing/complementary with #61529 (by @liuhao1024), which instead structurally restricts the advertised enabled_toolsets at agent construction so denied tools are never advertised. Different mechanisms — a maintainer may prefer #61529's structural fix, this text mitigation, or both. Flagging the cluster (issue #61521, #61529, this PR).

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cache-preserving mitigation. Current main still has the mismatch reported in #61521: the fork receives the parent toolset at agent/background_review.py:694-695, while the runtime whitelist is built from skills plus conditional memory at agent/background_review.py:788-800.

Problems

  • agent/background_review.py:788-790 intentionally omits memory when both memory flags are disabled. The new unconditional memory for notes guidance would direct that review fork to an unavailable tool.
  • tests/run_agent/test_background_review_toolset_restriction.py:101-135 captures deny_msg_fmt but does not assert its contents, and no test covers the injected steering text.

Suggested changes

  • Generate the substitute-tool guidance from the actual whitelist, omitting memory when unavailable.
  • Add enabled/disabled-memory assertions for both the deny formatter and review prompt.

Automated hermes-sweeper review.

'"{tool_name}". Review mode only permits memory and '
"skill management tools. Use `skill_view` to read a "
"skill, `skill_manage` to modify one, `skills_list` "
"to discover skills, and `memory` for notes."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memory is absent from the whitelist when both memory flags are disabled (review_toolsets begins as only skills on current main). Please make this guidance conditional on memory being available, and apply the same condition to the injected review prompt.

…o prevent tool-denial storm

(cherry picked from commit 81e98b8c6766869489976960711285425619fe70)
@webtecnica
webtecnica force-pushed the fix/61521-background-review-deny-message branch from 81e98b8 to 4c2c742 Compare July 11, 2026 12:48
@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants