feat: trigger-wording check — detect skills that should have loaded but didn't - #68
feat: trigger-wording check — detect skills that should have loaded but didn't#68hashbender wants to merge 1 commit into
Conversation
|
Review Complete Files Reviewed: 2 By Severity:
This PR modifies background_review.py (removing _resolve_review_runtime, _digest_history, auxiliary model routing, and stdout/stderr redirection) but introduces regressions: a process-global stdout/stderr redirect silences all other threads, memory toolset is unconditionally whitelisted bypassing the config flag, and the associated test file is orphaned. Files Reviewed (2 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (72/100) — 1 high finding, 2 medium · 318 LOC across 2 files
Summary
PR #68 refactors agent/background_review.py to simplify the background review agent fork by removing _resolve_review_runtime(), _digest_history(), and auxiliary model routing configuration. The changes target reducing complexity in the review agent's runtime setup but introduce several regressions.
Key Findings
High Severity — Orphaned Test (finding-001)
The test file tests/run_agent/test_background_review_cost_controls.py calls the removed functions _resolve_review_runtime and _digest_history with no corresponding test update in this PR. The renamed test_review_prompt_class_first.py does not cover these call sites.
High Severity — Thread-Safety Regression (finding-002)
At agent/background_review.py:522, the diff introduces sys.stdout = devnull; sys.stderr = devnull — a process-global redirect. This silences output from ALL threads including gateway event loops, the scheduler, and any concurrent agent sessions. Use thread-local redirection or per-fork stream capture instead.
Medium Severity — Tool Whitelist Bypass (finding-003)
At agent/background_review.py:648, the review fork unconditionally enables the memory toolset regardless of the user's memory_enabled config flag. This bypasses the explicit opt-out mechanism and may trigger unexpected memory provider operations during background reviews.
Medium Severity — Config Path Severed (finding-004)
At agent/background_review.py:563, the auxiliary_model_config parameter is removed from the AIAgent constructor call. This severs the auxiliary.review config path that previously allowed users to override the model used for background reviews, removing a documented customization point.
| with open(os.devnull, "w", encoding="utf-8") as _devnull, \ | ||
| contextlib.redirect_stdout(_devnull), \ | ||
| contextlib.redirect_stderr(_devnull): |
There was a problem hiding this comment.
🟠 Process-global stdout/stderr redirect silences all other threads during background review (bug)
The PR replaces thread_scoped_silence() (from agent/thread_scoped_output.py, which routes only the calling thread's writes to /dev/null) with contextlib.redirect_stdout(devnull) + contextlib.redirect_stderr(devnull) at two sites: the main review body (lines 522-524) and the finally-block safety-net cleanup (lines 728-730). redirect_stdout/redirect_stderr operate process-globally — they reassign sys.stdout/sys.stderr for every thread in the process. The background review runs as a daemon thread sharing a process with the gateway's asyncio event-loop thread (Telegram/Discord long-polls, cron scheduler). During the review (tens of seconds, fired every ~10 conversation turns), ALL output from those other threads — user-facing status messages, error logs, platform diagnostics — is silently written to /dev/null and lost. This is a regression of bugs previously fixed as NousResearch#55769 / NousResearch#55925, which the removed code comment explicitly warned about: 'A process-global contextlib.redirect_stdout(devnull) here would also blank sys.stdout/sys.stderr for every other thread — including a gateway event-loop thread driving a Telegram long-poll.' The agent/thread_scoped_output.py module is still present and importable.
💡 Suggestion: Restore from agent.thread_scoped_output import thread_scoped_silence and replace both with open(os.devnull, ...) as _devnull, contextlib.redirect_stdout(_devnull), contextlib.redirect_stderr(_devnull): blocks with with thread_scoped_silence():. The two sites are: (1) lines 522-524 in the main review body, and (2) lines 728-730 in the finally block. thread_scoped_silence() installs a per-thread routing proxy on sys.stdout/sys.stderr once and only silences the calling thread — all other threads retain normal output.
📋 Prompt for AI Agents
In agent/background_review.py: (1) Restore from agent.thread_scoped_output import thread_scoped_silence in the imports (after the from __future__ block). (2) At lines 522-524, replace with open(os.devnull, "w", encoding="utf-8") as _devnull, \ / contextlib.redirect_stdout(_devnull), \ / contextlib.redirect_stderr(_devnull): with a single with thread_scoped_silence():. (3) At lines 728-730, replace the identical pattern with with thread_scoped_silence():. (4) Remove import contextlib at line 21 if no other usage remains. The agent/thread_scoped_output.py module already exists and provides the correct per-thread isolation.
| t["function"]["name"] | ||
| for t in get_tool_definitions( | ||
| enabled_toolsets=review_toolsets, | ||
| enabled_toolsets=["memory", "skills"], |
There was a problem hiding this comment.
🟡 Memory toolset unconditionally whitelisted in review fork, bypassing memory_enabled flag (bug)
At line 648 in agent/background_review.py, the background review agent's tool whitelist is built via enabled_toolsets=["memory", "skills"] unconditionally. The old code (visible in the patch diff) constructed the toolsets list based on review_agent._memory_enabled and review_agent._user_profile_enabled flags, starting with ["skills"] and only prepending "memory" when either flag was truthy. The removed comment cited bug NousResearch#54937 layer 2: 'Hardcoding ["memory", "skills"] granted the review LLM the MEMORY.md read/write tool even when a profile set memory_enabled: false, contaminating a memory-disabled profile.' The review agent correctly inherits _memory_enabled (line 588) and _user_profile_enabled (line 589) from the parent, but these flags are no longer consulted when building the whitelist. While the memory tool handler will still reject writes when _memory_store is None (for memory-disabled profiles), the LLM can still call the tool, wasting iterations. The existing test test_background_review_excludes_memory_when_disabled (test_background_review_toolset_restriction.py) will fail against the new unconditional list.
💡 Suggestion: Restore the conditional gating: build review_toolsets = ['skills'], then only prepend 'memory' when review_agent._memory_enabled or review_agent._user_profile_enabled is truthy. Pass review_toolsets to get_tool_definitions(enabled_toolsets=review_toolsets, ...) instead of the hardcoded ["memory", "skills"].
📋 Prompt for AI Agents
In agent/background_review.py, before line 645, add: review_toolsets = ['skills']; if review_agent._memory_enabled or review_agent._user_profile_enabled: review_toolsets.insert(0, 'memory'). Then change line 648 from enabled_toolsets=["memory", "skills"], to enabled_toolsets=review_toolsets,. This restores the gating logic that was removed from the old code (the flags on review_agent are already set at lines 588-589).
| review_agent = AIAgent( | ||
| model=_rt.get("model") or agent.model, | ||
| model=agent.model, | ||
| max_iterations=16, | ||
| quiet_mode=True, | ||
| platform=agent.platform, | ||
| provider=_rt.get("provider") or agent.provider, | ||
| api_mode=_rt.get("api_mode"), | ||
| base_url=_rt.get("base_url") or None, | ||
| api_key=_rt.get("api_key") or None, | ||
| provider=agent.provider, | ||
| api_mode=_parent_api_mode, | ||
| base_url=_parent_runtime.get("base_url") or None, | ||
| api_key=_parent_runtime.get("api_key") or None, |
There was a problem hiding this comment.
🟡 Auxiliary model routing for background reviews removed, config path severed (bug)
Lines 563-571 construct the review agent using model=agent.model and provider=agent.provider directly, always using the main model. The old code used _resolve_review_runtime(agent) (deleted function at old lines 35-89) which checked auxiliary.background_review.{provider,model} config — if a different model was configured, the review fork would route to that cheaper/specialized model. The _routed flag tracked whether the fork was on a different model, gating downstream behavior (digest history, cached system prompt). The new code removes all of this: the auxiliary routing config key is silently ignored, _routed is never computed, and the review always runs on the main model. The _resolve_review_runtime() function, _msg_text() helper, _digest_history() compaction, and the _routed flag are all completely removed from the module. Users who set auxiliary.background_review.provider and auxiliary.background_review.model expecting routed reviews will have their config silently ignored with no error or deprecation warning.
💡 Suggestion: Either restore the auxiliary routing logic (at minimum the auxiliary.background_review config check) or document the breaking change and add a deprecation warning when the config key is present. If the intent is permanent removal, emit a log warning when auxiliary.background_review is set in user config.
📋 Prompt for AI Agents
In agent/background_review.py, determine whether this feature removal is intentional. If intentional: add a config check that logs a deprecation warning when auxiliary.background_review is set in user config. If accidental: restore _resolve_review_runtime() from the previous version to re-enable the routing path. The function should be called before constructing AIAgent at line 563, and its return dict should provide model, provider, api_mode, base_url, api_key, and a 'routed' flag.
1 participant
Problem
Skills load on-demand via description match (the
Use when...trigger line). If a skill's description doesn't match the task phrasing, the skill never fires. The miss is invisible by definition — the self-repair loop only fires after a successful load, so a badly-worded trigger creates a permanent blind spot that never surfaces.This is the core weakness of lazy-loaded skills vs always-injected rules: guaranteed recall is traded for context efficiency, but the cost is silent misses on trigger-wording bugs.
Solution
The background review agent already replays every conversation to check if skills/memory should be updated. This PR adds a TRIGGER-WORDING CHECK to both
_SKILL_REVIEW_PROMPTand_COMBINED_REVIEW_PROMPT. After the existing review pass, the reviewer:skills_list)skill_view, patches its trigger description to match the actual task phrasingTRIGGER FIX: <skill-name> — missed for <task summary>This surfaces the previously-invisible miss and fixes it at the source (the description), not the content.
Why prompt-only (no new hooks/tools)
This fits cleanly into existing infrastructure — no new hooks, no new tools, no new env vars, no new config keys. The background review fork already runs after every turn with
memory+skillstoolsets. Adding a new review dimension to the prompt is the smallest-footprint change that closes the gap.Changes
agent/background_review.py— +21 lines to_SKILL_REVIEW_PROMPT, +21 lines to_COMBINED_REVIEW_PROMPT(identical block)tests/run_agent/test_review_prompt_class_first.py— +45 lines: 2 new behavior tests + shared assertion helperTests
All 43 existing background-review tests pass, plus 2 new tests:
Tests follow the existing behavior-assertion pattern (not snapshot tests):
skills_listis named as the mechanismdescriptionis identified as the fix targetTRIGGER FIXlog marker is presentMirror-of: NousResearch#55965
NousResearch#55965