Skip to content

fix(review): make skill curation precision-first - #64655

Open
MaximCrabbe wants to merge 1 commit into
NousResearch:mainfrom
MaximCrabbe:fix/background-skill-review-quality
Open

fix(review): make skill curation precision-first#64655
MaximCrabbe wants to merge 1 commit into
NousResearch:mainfrom
MaximCrabbe:fix/background-skill-review-quality

Conversation

@MaximCrabbe

Copy link
Copy Markdown
Contributor

Summary

  • replace the background skill review's activity bias with a shared precision-first quality gate
  • require every candidate to be novel, durable, evidence-backed, reusable, and well-scoped before writing
  • make Nothing to save. a normal, healthy outcome
  • route stable cross-task preferences to user memory; only task-specific learnings may update skills
  • keep the existing loaded-skill → umbrella → support-file → create ladder after the gate passes

Why

The current prompt says most sessions should produce a skill update and frames a no-op as a missed opportunity. That pressure encourages one-incident references, duplicated preferences, and speculative claims to become persistent instructions.

This change preserves self-improvement while optimizing for precision instead of write volume.

Partially addresses #57057. It complements, without duplicating, the background write-approval work in #63019 and read-before-write work in #60331.

Tests

  • uv run pytest tests/run_agent/test_background_review.py tests/run_agent/test_background_review_summary.py tests/run_agent/test_background_review_cost_controls.py tests/run_agent/test_background_review_cache_parity.py tests/run_agent/test_background_review_toolset_restriction.py tests/run_agent/test_review_prompt_class_first.py tests/test_background_review_list_shapes.py tests/test_background_review_session_isolation.py -q — 79 passed
  • uv run ruff check agent/background_review.py tests/run_agent/test_review_prompt_class_first.py
  • git diff --cached --check
  • read-only adversarial review of candidate tree f06118a... — approved
  • bounded micro-delta re-review of final tree f0a1c29... — approved

@alt-glitch alt-glitch added type/feature New feature or request comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have labels Jul 14, 2026

@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 tightening the review policy. The current prompts do contain the reported activity bias (agent/background_review.py:181-205 and :286-307), and the shared gate addresses it without changing the foreground session or tool schema.

Problems

  • The updated ladders still instruct the reviewer to use skills merely previously loaded in the conversation (agent/background_review.py:224-253, :323-348 in this PR). Background-review writes are rejected unless the exact target was read with skill_view in the current review turn (tools/skill_manager_tool.py:382-409; enforced for patch at :959-966). A quality-gated update can therefore still fail its first write attempt.

Suggested changes

  • Add the explicit fresh skill_view requirement for every existing target to both ladders, and cover it with a prompt-contract assertion.

Automated hermes-sweeper review.

"pick one when a signal above fired:\n"
"to be wrong, missing a step, or outdated. Treat the proven gap as a "
"candidate and patch only if every quality-gate criterion passes.\n\n"
"After the quality gate passes, use this preference order — prefer the "

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.

Add the current-review-turn read-before-write instruction before this ladder, and mirror it in the combined prompt. tools/skill_manager_tool.py:382-409 rejects background-review edits, patches, removals, and overwrites unless the exact target was loaded with skill_view in this review turn; looking back at a skill loaded in the inherited conversation does not satisfy that guard.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Eight PRs address or reference this skill-lifecycle complex. Their diffs separate into class-first curation (#16026), transient-failure filtering (#23004), write hygiene (#23288), memory routing (#24846), trigger repair (#55965), precision gating (#64655), and the duplicated read-before-write implementation in #59175/#75020.

Related pull requests

Duplicates

#59175 and #75020 implement materially the same #58475 fix; the explicit duplicate chain is #59175#75020, with #59175 already closed as superseded. #23288 and #64655 overlap on reducing low-quality writes but retain distinct hygiene and persistence-gate scopes.

Suggested consolidation

Keep #64655 open with a salvage path: retain its precision-first quality gate and memory/skill routing, but require a fresh skill_view for every existing write target and add the requested prompt-contract test, explicitly following its contributor keep_open review. Keep #75020 open as the focused current-main continuation of #59175 under its MAINTAINER-BOT keep_open verdict; leave #59175 closed as duplicate of #75020, and keep #23288, #24846, and #55965 only on their documented contributor-review salvage paths rather than combining their broader concerns into #64655.

Complex graph

flowchart 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
    I429(["issue #429 (open)"])
    I6051(["issue #6051 (closed)"])
    I58475(["issue #58475 (open)"])
    P64655["PR #64655 (open)"]
    P64655 -.->|partial| I429
    P64655 -.->|partial| I6051
    P64655 -.->|partial| I58475
    class I429 open
    class I6051 closed
    class I58475 open
    class P64655 open
    class P64655 target
    click I429 "https://github.com/NousResearch/hermes-agent/issues/429"
    click I6051 "https://github.com/NousResearch/hermes-agent/issues/6051"
    click I58475 "https://github.com/NousResearch/hermes-agent/issues/58475"
    click P64655 "https://github.com/NousResearch/hermes-agent/pull/64655"
Loading

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 8 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 100 kB of PR diffs, 37 kB of issue/PR text, 14 kB of discussion (14 comments), 15 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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 P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants