Skip to content

feat: trigger-wording check — detect skills that should have loaded but didn't #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hashbender wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: trigger-wording check — detect skills that should have loaded but didn't #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
273 changes: 82 additions & 191 deletions agent/background_review.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,141 +18,15 @@

from __future__ import annotations

import contextlib
import json
import logging
import os
from typing import Any, Dict, List, Optional

from agent.thread_scoped_output import thread_scoped_silence

logger = logging.getLogger(__name__)


# ---------------------------------------------------------------------------
# Background-review aux-model selector + routed digest.
#
# The review fork runs on the MAIN model by default ("auto"), replaying the
# full conversation — already warm in the prompt cache, so cheap cache reads.
# Optimal and unchanged. A user can route the review to a different, cheaper
# model via auxiliary.background_review.{provider,model}. A different model
# cannot reuse the parent's cache (different key), so the fork is cold
# regardless — replaying the full transcript would just cold-write it. So when
# (and only when) routed to a different model, we replay a compact DIGEST to
# minimise cold-written tokens. Same model -> full replay; different model ->
# digest. That's the whole policy.
# ---------------------------------------------------------------------------


def _resolve_review_runtime(agent: Any) -> Dict[str, Any]:
"""Resolve provider/model/credentials for the review fork.

Default (auto / unset / same as parent): inherit the parent's live runtime
(with codex_app_server -> codex_responses downgrade). ``routed`` is False —
the fork uses the main model and the warm cache, exactly as before. When
``auxiliary.background_review.{provider,model}`` names a concrete model
different from the parent's, resolve that runtime and set ``routed=True``.
"""
parent_runtime = agent._current_main_runtime()
parent_api_mode = parent_runtime.get("api_mode") or None
if parent_api_mode == "codex_app_server":
parent_api_mode = "codex_responses"
parent = {
"provider": agent.provider,
"model": agent.model,
"api_key": parent_runtime.get("api_key") or None,
"base_url": parent_runtime.get("base_url") or None,
"api_mode": parent_api_mode,
"routed": False,
}
try:
from hermes_cli.config import load_config
cfg = load_config()
except Exception:
return parent
aux = cfg.get("auxiliary", {}) if isinstance(cfg.get("auxiliary"), dict) else {}
task = aux.get("background_review", {}) if isinstance(aux.get("background_review"), dict) else {}
task_provider = (str(task.get("provider", "")).strip() or None)
task_model = (str(task.get("model", "")).strip() or None)
task_base_url = (str(task.get("base_url", "")).strip() or None)
task_api_key = (str(task.get("api_key", "")).strip() or None)
if not (task_provider and task_provider != "auto" and task_model):
return parent
if task_provider == (agent.provider or "") and task_model == (agent.model or ""):
return parent # same model/provider as parent -> not routed
try:
from hermes_cli.runtime_provider import resolve_runtime_provider
rp = resolve_runtime_provider(
requested=task_provider,
target_model=task_model,
explicit_api_key=task_api_key,
explicit_base_url=task_base_url,
)
return {
"provider": rp.get("provider") or task_provider,
"model": task_model,
"api_key": rp.get("api_key"),
"base_url": rp.get("base_url"),
"api_mode": rp.get("api_mode"),
"routed": True,
}
except Exception as e:
logger.debug("background-review aux routing failed (%s); using main model", e)
return parent


def _msg_text(m: Dict) -> str:
c = m.get("content")
if isinstance(c, str):
return c.strip()
if isinstance(c, list):
return " ".join(b.get("text", "") for b in c if isinstance(b, dict)).strip()
return ""


def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict]:
"""Compact replay for the routed (different-model) path only.

Keeps the recent ``tail`` messages verbatim, collapses older turns into one
synthetic user-role digest, preserving role alternation. Used ONLY when
routed to a different model (cache cold regardless, so fewer cold-written
tokens is a pure win). Never on the main-model path (full replay stays warm).
"""
msgs = list(messages_snapshot or [])
if len(msgs) <= tail:
return msgs
keep = msgs[-tail:]
while keep and isinstance(keep[0], dict) and keep[0].get("role") == "tool":
tail += 1
if len(msgs) <= tail:
return msgs
keep = msgs[-tail:]
old = msgs[:-len(keep)]
lines: List[str] = []
for m in old:
if not isinstance(m, dict):
continue
role = m.get("role")
text = _msg_text(m).replace("\n", " ")
if role == "user" and text:
lines.append(f"USER: {text[:300]}")
elif role == "assistant":
tcs = m.get("tool_calls") or []
if tcs:
names = [(tc.get("function") or {}).get("name", "?") for tc in tcs if isinstance(tc, dict)]
lines.append(f"ASSISTANT[tools: {', '.join(names)}]")
if text:
lines.append(f"ASSISTANT: {text[:200]}")
digest = {
"role": "user",
"content": (
"[Earlier conversation digest — older turns summarised to bound the "
"review's cold-write cost on the routed aux model. Recent turns "
"follow verbatim below.]\n" + "\n".join(lines)
),
}
return [digest] + keep


# Review-prompt strings — used by ``spawn_background_review_thread`` to build
# the user-message that the forked review agent receives. AIAgent exposes
# them as class attributes (``_MEMORY_REVIEW_PROMPT`` etc.) for back-compat;
Expand Down Expand Up @@ -238,6 +112,27 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict]
"skill that governs that task needs to carry the lesson.\n\n"
"If you notice two existing skills that overlap, note it in your "
"reply — the background curator handles consolidation at scale.\n\n"
"TRIGGER-WORDING CHECK — skills can only fire if their description "
"matches the task. A skill that SHOULD have loaded but DIDN'T is a "
"trigger-wording bug, not a skill bug — and the miss is invisible "
"because the self-repair loop only fires AFTER a load. After every "
"review, run this check:\n"
" 1. List all skills (skills_list). Read the full set of "
"descriptions.\n"
" 2. Compare them against the conversation. Was there a task where "
"a skill's description COVERS the territory, but the skill was never "
"loaded via skill_view?\n"
" 3. If yes, the skill's description (the 'Use when...' trigger "
"line) is failing to match. Patch it — broaden the wording, add the "
"task phrasing that appeared in this conversation, or add synonyms "
"the user actually used. This is a description edit, not a content "
"edit.\n"
" 4. Log the miss: in your reply, note "
"'TRIGGER FIX: <skill-name> — missed for <task summary>'. This "
"surfaces the invisible miss so the pattern is visible over time.\n"
"This check matters MOST for skills that were never loaded this "
"session — their descriptions have never been validated against real "
"task phrasing.\n\n"
"Protected skills (DO NOT edit these):\n"
" • Bundled skills (shipped with Hermes, e.g. 'hermes-agent').\n"
" • Hub-installed skills (installed via 'hermes skills install').\n"
Expand Down Expand Up @@ -324,6 +219,27 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict]
"should carry user-preference lessons when relevant.\n\n"
"If you notice overlapping existing skills, mention it — the "
"background curator handles consolidation.\n\n"
"TRIGGER-WORDING CHECK — skills can only fire if their description "
"matches the task. A skill that SHOULD have loaded but DIDN'T is a "
"trigger-wording bug, not a skill bug — and the miss is invisible "
"because the self-repair loop only fires AFTER a load. After every "
"review, run this check:\n"
" 1. List all skills (skills_list). Read the full set of "
"descriptions.\n"
" 2. Compare them against the conversation. Was there a task where "
"a skill's description COVERS the territory, but the skill was never "
"loaded via skill_view?\n"
" 3. If yes, the skill's description (the 'Use when...' trigger "
"line) is failing to match. Patch it — broaden the wording, add the "
"task phrasing that appeared in this conversation, or add synonyms "
"the user actually used. This is a description edit, not a content "
"edit.\n"
" 4. Log the miss: in your reply, note "
"'TRIGGER FIX: <skill-name> — missed for <task summary>'. This "
"surfaces the invisible miss so the pattern is visible over time.\n"
"This check matters MOST for skills that were never loaded this "
"session — their descriptions have never been validated against real "
"task phrasing.\n\n"
"Protected skills (DO NOT edit these):\n"
" • Bundled skills (shipped with Hermes, e.g. 'hermes-agent').\n"
" • Hub-installed skills (installed via 'hermes skills install').\n"
Expand Down Expand Up @@ -603,15 +519,9 @@ def _bg_review_auto_deny(command, description, **kwargs):
review_agent = None
review_messages: List[Dict] = []
try:
# Silence stdout/stderr for THIS worker thread only. 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 — for the full
# duration of the review (tens of seconds), swallowing their console
# output (#55769 / #55925). ``thread_scoped_silence`` routes only this
# thread's writes to devnull and leaves all other threads on the real
# streams.
with thread_scoped_silence():
with open(os.devnull, "w", encoding="utf-8") as _devnull, \
contextlib.redirect_stdout(_devnull), \
contextlib.redirect_stderr(_devnull):
Comment on lines +522 to +524

@tenki-reviewer tenki-reviewer Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 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.

# Inherit the parent agent's live runtime (provider, model,
# base_url, api_key, api_mode) so the fork uses the exact
# same credentials the main turn is using. Without this,
Expand All @@ -620,13 +530,18 @@ def _bg_review_auto_deny(command, description, **kwargs):
# creds, or credential-pool setups where the resolver can't
# reconstruct auth from scratch -- producing the spurious
# "No LLM provider configured" warning at end of turn.
# _resolve_review_runtime() returns the parent's live runtime by
# default (routed=False; main model, warm cache), or — when the user
# set auxiliary.background_review.{provider,model} to a different
# model — that model's runtime (routed=True). The codex_app_server
# -> codex_responses downgrade is applied inside the resolver.
_rt = _resolve_review_runtime(agent)
_routed = bool(_rt.get("routed"))
_parent_runtime = agent._current_main_runtime()
_parent_api_mode = _parent_runtime.get("api_mode") or None
# The review fork needs to call agent-loop tools (memory,
# skill_manage). Those tools require Hermes' own dispatch,
# which the codex_app_server runtime bypasses entirely
# (it runs the turn inside codex's subprocess). So when
# the parent is on codex_app_server, downgrade the review
# fork to codex_responses — same auth/credentials, but
# talks to the OpenAI Responses API directly so Hermes
# owns the loop and the agent-loop tools dispatch.
if _parent_api_mode == "codex_app_server":
_parent_api_mode = "codex_responses"
# skip_memory=True keeps the review fork from
# touching external memory plugins (honcho, mem0,
# supermemory, etc.). Without it, the fork's
Expand All @@ -646,14 +561,14 @@ def _bg_review_auto_deny(command, description, **kwargs):
# in the request body — Anthropic's cache key includes it.
# (The runtime whitelist below still restricts dispatch.)
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,
Comment on lines 563 to +571

@tenki-reviewer tenki-reviewer Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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.

credential_pool=getattr(agent, "_credential_pool", None),
parent_session_id=agent.session_id,
enabled_toolsets=getattr(agent, "enabled_toolsets", None),
Expand Down Expand Up @@ -692,20 +607,15 @@ def _bg_review_auto_deny(command, description, **kwargs):
# issue #25322 and PR #17276 for the full analysis +
# measured impact (~26% end-to-end cost reduction on
# Sonnet 4.5).
# Share the parent's warm cached system prompt ONLY when the review
# runs on the SAME model (not routed). When routed to a different
# model the parent's cached prompt is for the wrong model/cache key
# and would miss anyway, so let the routed fork build its own.
if not _routed:
review_agent._cached_system_prompt = agent._cached_system_prompt
# Defensive: pin session_start + session_id to the
# parent's so any code path that re-renders parts of
# the system prompt (compression, plugin hooks) still
# produces byte-identical output. The cached-prompt
# assignment above already short-circuits the normal
# rebuild path, but these pins guarantee parity even
# if a future code path bypasses the cache.
review_agent.session_start = agent.session_start
review_agent._cached_system_prompt = agent._cached_system_prompt
# Defensive: pin session_start + session_id to the
# parent's so any code path that re-renders parts of
# the system prompt (compression, plugin hooks) still
# produces byte-identical output. The cached-prompt
# assignment above already short-circuits the normal
# rebuild path, but these pins guarantee parity even
# if a future code path bypasses the cache.
review_agent.session_start = agent.session_start
review_agent.session_id = agent.session_id
# The fork shares the parent's live session_id (pinned above for
# prefix-cache parity). It is single-lifecycle and calls close()
Expand All @@ -732,17 +642,10 @@ def _bg_review_auto_deny(command, description, **kwargs):
clear_thread_tool_whitelist,
)

# Gate the built-in memory tool on the profile's memory_enabled flag.
# 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 (#54937 layer 2).
review_toolsets = ["skills"]
if review_agent._memory_enabled or review_agent._user_profile_enabled:
review_toolsets.insert(0, "memory")
review_whitelist = {
t["function"]["name"]
for t in get_tool_definitions(
enabled_toolsets=review_toolsets,
enabled_toolsets=["memory", "skills"],

@tenki-reviewer tenki-reviewer Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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).

quiet_mode=True,
)
}
Expand All @@ -754,28 +657,14 @@ def _bg_review_auto_deny(command, description, **kwargs):
),
)
try:
from tools.skill_manager_tool import _reset_background_review_read_marks

_reset_background_review_read_marks()
except Exception:
pass

try:
# Routed to a different model -> replay a digest (cache is cold
# on that model anyway, so minimise cold-written tokens). Same
# model -> replay the full snapshot (warm cache reads).
_review_history = (
_digest_history(messages_snapshot) if _routed
else messages_snapshot
)
review_agent.run_conversation(
user_message=(
prompt
+ "\n\nYou can only call memory and skill "
"management tools. Other tools will be denied "
"at runtime — do not attempt them."
),
conversation_history=_review_history,
conversation_history=messages_snapshot,
)
finally:
clear_thread_tool_whitelist()
Expand Down Expand Up @@ -829,14 +718,16 @@ def _bg_review_auto_deny(command, description, **kwargs):
logger.warning("Background memory/skill review failed: %s", e)
agent._emit_auxiliary_failure("background review", e)
finally:
# Safety-net cleanup for the exception path. Normal completion already
# shut down inside the thread-scoped silence above. Re-enter the
# thread-scoped silence here so teardown output (Honcho flush, Hindsight
# sync, background thread joins) stays quiet even on the exception path,
# without blanking other threads' streams.
# Safety-net cleanup for the exception path. Normal
# completion already shut down inside redirect_stdout above.
# Re-open devnull here so any teardown output (Honcho flush,
# Hindsight sync, background thread joins) stays silent even
# on the exception path where redirect_stdout already exited.
if review_agent is not None:
try:
with thread_scoped_silence():
with open(os.devnull, "w", encoding="utf-8") as _fn, \
contextlib.redirect_stdout(_fn), \
contextlib.redirect_stderr(_fn):
try:
review_agent.shutdown_memory_provider()
except Exception:
Expand Down
Loading
Loading