Skip to content

fix(agent): instruct background review fork to call skill_view before patching skills (#62397) - #62428

Closed
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/62397-review-skill-view-prompt
Closed

fix(agent): instruct background review fork to call skill_view before patching skills (#62397)#62428
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/62397-review-skill-view-prompt

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Summary

The background self-improvement review fork is instructed to patch skills, but the prompt never tells the model to call skill_view(name) first. The guard _background_review_write_guard in tools/skill_manager_tool.py enforces a read-before-write requirement and refuses the patch because the current SKILL.md hasn't been loaded. Net effect: corrections are silently dropped.

Change

Updated _SKILL_REVIEW_PROMPT and _COMBINED_REVIEW_PROMPT in agent/background_review.py with a "Read-before-write requirement" instruction that tells the model to:

  1. Always call skill_view(name) BEFORE any skill_manage write operation
  2. If the tool returns _read_before_write_required: true, read first then retry

Also improved the deny message format to tell the model which tools are available, reducing trial-and-error.

Verification

76 background review tests pass.

@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 tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jul 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #60331 (earliest open, 2026-07-07) — same file (agent/background_review.py), same mechanism: adds a read-before-write instruction to _SKILL_REVIEW_PROMPT / _COMBINED_REVIEW_PROMPT so the review fork calls skill_view(name) before skill_manage writes, aligning with the shipped guard #55906. Sibling #62414 is likewise a dup of #60331. Closes #62397 (the spec/discussion). related_to #62414, #62397.

@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 background-review guard to the prompts; the current-main mismatch is real.

Problems

  • The new wording says skill_view(name) before all listed writes, but that is insufficient for existing supporting files. tools/skill_manager_tool.py:1167-1172 and :1228-1232 require the exact file path to have been read; tests/tools/test_skill_manager_tool.py:1330-1348 verifies that reading SKILL.md does not authorize overwriting references/workflow.md.
  • The PR changes both prompt constants without extending the existing behavior-level prompt suite in tests/run_agent/test_review_prompt_class_first.py.

Suggested changes

  • Teach the retry path to use skill_view(name, file_path=...) when the rejected mutation targets an existing supporting file, while retaining skill_view(name) for SKILL.md.
  • Add focused assertions for both review prompts covering that distinction and the _read_before_write_required retry instruction.

Automated hermes-sweeper review.

"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"
"Read-before-write requirement (MANDATORY — the tool enforces this): "

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 must distinguish SKILL.md from supporting files. The guard requires the exact target to be read, so skill_view(name) will still reject an overwrite/remove/patch of an existing references/... file; instruct skill_view(name, file_path=...) for that case.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 11, 2026
@webtecnica

Copy link
Copy Markdown
Contributor Author

Closing as duplicate — another PR already addressed this. Appreciate the contribution!

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 duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants