Skip to content

fix(agent): use restricted toolset for background review fork - #61529

Open
liuhao1024 wants to merge 3 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-61521-background-review-toolset
Open

fix(agent): use restricted toolset for background review fork#61529
liuhao1024 wants to merge 3 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-61521-background-review-toolset

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the background self-improvement review ("curator") by eliminating a toolset/schema mismatch. The review fork was constructed with the parent agent's full toolset (via enabled_toolsets=getattr(agent, "enabled_toolsets", None)), but the runtime dispatch whitelist only permitted {memory, skills}. This caused the model to see and call general-purpose file tools (read_file, write_file, patch) in the tools[] schema, then be denied at dispatch every turn with "denied non-whitelisted tool" errors. Net effect: the review loop ran continuously but almost never landed skill patches, breaking self-improvement.

Related Issue

Fixes #61521

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/background_review.py: Build review_toolsets (restricted to ["skills"] or ["memory", "skills"]) BEFORE constructing the review agent, and pass it as enabled_toolsets to AIAgent(). This eliminates the toolset/schema mismatch.
  • Removed duplicate review_toolsets construction that occurred after agent creation (it used review_agent._memory_enabled which is now unreachable since the agent is already constructed with the restricted toolset).
  • Updated comments to clarify the trade-off: tools[] no longer byte-matches the parent for prompt-cache prefix reuse (the original reason for inheriting the parent's full toolset). The functional correctness fix takes priority over cache parity.

How to Test

  1. Run existing background review tests to verify no regression:

    pytest tests/test_background_review_session_isolation.py -v
    pytest tests/test_background_review_list_shapes.py -v

    All tests pass.

  2. Verification (manual or automated):

    • The review fork's tools[] schema now only lists memory/skill tools (no read_file, write_file, patch).
    • No "denied non-whitelisted tool" errors appear in the review loop logs.
    • Skill patches from background review land correctly (the model no longer hits the denial storm).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.2

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

The background review fork was constructed with the parent agent's full
toolset (enabled_toolsets) while the runtime dispatch whitelist only
permitted {memory, skills}. This caused a storm of "denied non-whitelisted
tool" errors for file tools (read_file, write_file, patch), leading to
broken self-improvement (the model never landed skill patches).

Fix: build review_toolsets BEFORE constructing the review agent, and pass
it to AIAgent() as enabled_toolsets. This eliminates the toolset/schema
mismatch. Trade-off: tools[] no longer byte-matches the parent for
prompt-cache prefix reuse — acceptable cost for functional correctness.

Refs NousResearch#61521
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/skills Skills system (list, view, manage) tool/memory Memory tool and memory providers labels Jul 9, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: fixes #61521 (curator fork advertises the parent's full toolset but the runtime whitelist only permits skills+memory → per-turn tool-denial storm). This PR restricts enabled_toolsets at construction time for all providers, trading prompt-cache byte-parity for correct toolset semantics. That differs from the competing open PR #39997, which narrows the schema only for local endpoints (preserving full-toolset cache parity for remote cache-backed providers). Cross-linking so a maintainer can pick between the always-restrict and local-only approaches.

@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 tracing the real schema/whitelist mismatch; current main still has it at agent/background_review.py:694 and agent/background_review.py:791.

Problems

  • agent/background_review.py:708 narrows the advertised schema for every provider. That reverts the cache-parity behavior intentionally established by 5fe067226: its commit records that Anthropic’s cache key includes tools, and restoring the parent schema reduced measured end-to-end review cost by about 26%. Current main still documents and tests that parent-schema contract at agent/background_review.py:680-705 and tests/run_agent/test_background_review_cache_parity.py:191-238.

Suggested changes

  • Preserve parent-schema parity for remote cache-backed providers. If narrowing is selected, limit it to local/no-prefix-cache endpoints while retaining the existing whitelist; the linked #39997 takes that scoped approach.

Automated hermes-sweeper review.

credential_pool=getattr(agent, "_credential_pool", None),
parent_session_id=agent.session_id,
enabled_toolsets=getattr(agent, "enabled_toolsets", None),
enabled_toolsets=review_toolsets,

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.

This applies schema narrowing to remote cache-backed providers too. Commit 5fe0672 intentionally restored parent tools[] parity because Anthropic’s cache key includes tools and measured lower review cost; preserve the parent schema remotely and scope narrowing to endpoints without that cache benefit.

@teknium1 teknium1 added sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels 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-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) tool/memory Memory tool and memory providers tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

3 participants