fix(background-review): add relevance gate to skill review prompt - #66472
fix(background-review): add relevance gate to skill review prompt#66472Kailigithub wants to merge 1 commit into
Conversation
The per-session background review fork could write unrelated content into a skill via skill_manage(write_file) and skill_manage(patch) when the conversation produced research or notes that did not match the skill's documented purpose. The 'Be ACTIVE' pressure encouraged false positives: the fork saw research output and grafted it onto the nearest plausible skill. Closes NousResearch#66350
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused mitigation. Current main still triggers skill review from cadence plus tool availability (agent/turn_finalizer.py:493-499), while both prompts combine an active-update bias with instructions to add research/domain notes under existing skills (agent/background_review.py:181-235, 286-323). The existing read-before-write guard verifies only that the target was loaded, not that the content belongs there (tools/skill_manager_tool.py:382-409).
Problems
- The new gate says “before any” mutation but omits
skill_manage(action="edit"), which can fully rewriteSKILL.md(tools/skill_manager_tool.py:864-890). That leaves the same unrelated-content failure mode available through a sibling action. - No in-repo regression test accompanies the prompt change.
tests/run_agent/test_review_prompt_class_first.py:24-222is the existing behavior-test home for both prompt variants.
Suggested changes
- Cover all mutating actions, preferably by phrasing the gate as applying before any skill mutation; at minimum include
edit. - Add invariant-style tests for both prompts requiring purpose/scope relevance before the support-file guidance.
Automated hermes-sweeper review.
| "small. A pass that does nothing is a missed learning opportunity, " | ||
| "not a neutral outcome.\n\n" | ||
| "Relevance gate (mandatory): before any skill_manage(write_file), " | ||
| "skill_manage(patch), or skill_manage(create), verify that the " |
There was a problem hiding this comment.
This says the gate applies before “any” mutation, but skill_manage(action="edit") is also a supported full SKILL.md rewrite (tools/skill_manager_tool.py:864). Please cover edit too, preferably by phrasing this as every mutating skill action.
SummaryNine PRs address this background-review skill-mutation complex: #55657/#55906 add the read-before-write guard, #55665 adds a shrink heuristic, #60331/#62414/#62428/#62664 teach the prompt-side handshake, #66472 adds a topical-relevance gate, and #73975 repairs read-mark propagation across copied tool contexts. The diffs cover distinct layers of the reported failures, with substantial duplication within the guard and prompt-handshake groups. Related pull requests
Duplicates#55657 was cherry-picked into and superseded by #55906; #55665 targets the same #55647 failure with a weaker size heuristic. #62414 is byte-near-identical to #60331, while #62428 and #62664 are additional prompt-handshake variants for #62397, with #62664 providing broader visible test coverage. Suggested consolidationKeep #66472 open with a salvage path: apply its relevance invariant to every skill mutation, including edit, and add two-prompt regression tests, as required by the visible keep_open review. Close #62414 as duplicate of #60331 despite its keep_open review because the complete diff is byte-near-identical and retains the same new-file/delete scope errors; leave the already closed prompt variants as references, and keep #73975 open on the maintainer-bot's high-salvageability path while preserving f57cb2e. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I66350(["issue #66350 (open)"])
P66472["PR #66472 (open)"]
P66472 -->|best fix| I66350
class I66350 open
class P66472 open
class P66472 best
class P66472 target
click I66350 "https://github.com/NousResearch/hermes-agent/issues/66350"
click P66472 "https://github.com/NousResearch/hermes-agent/pull/66472"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 9 pull requests and 4 issues in this complex. Each diff was read against this issue; Assessment working set: 59 kB of PR diffs, 39 kB of issue/PR text, 29 kB of discussion (23 comments), 20 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Summary
The per-session background review fork could write unrelated content into a skill via
skill_manage(write_file)andskill_manage(patch)when the conversation produced research or notes that did not match the skill's documented purpose. The "Be ACTIVE" pressure in the prompt encouraged false positives: the fork saw research output and grafted it onto the nearest plausible skill.This PR adds a mandatory relevance gate sentence near the top of both
_SKILL_REVIEW_PROMPTand_COMBINED_REVIEW_PROMPT. The gate explicitly forbids writing when the proposed content is not directly about the skill's documented purpose, and frames "do not write" as a valid outcome rather than a missed learning opportunity.Diff
The gate is placed before the existing preference order (1–4) so it frames every action that follows, not just one path. The "Be ACTIVE" language is preserved — the gate does not contradict the activity pressure, it qualifies it with a relevance precondition.
Scope
This is the prompt-only half of the issue's proposal. The code-gate half (gating
_should_review_skillson a_skills_loaded_this_sessionflag tracked inturn_finalizer.py) is a larger change that touches the review trigger across multiple call sites and the session-finalizer state machine; it is intentionally out of scope for a 17-line prompt fix. A follow-up PR can land that change once we agree on the flag-tracking semantics.Verification
python3 -m py_compile agent/background_review.py→ OKpython3 -m pytest tests/run_agent/test_background_review*.py→ 38 passed/tmp/test_relevance_gate.py(4 assertions covering both prompts, gate positioning, and gate text). Confirmed it fails onmain(no relevance gate) and passes with this fix via the git-stash three-step dance.Closes #66350