Skip to content

fix(skills): make evolved skill text optimizer-visible and validate the full skill file - #184

Open
brianbaldock wants to merge 1 commit into
NousResearch:mainfrom
brianbaldock:fix/skill-evolution-writeback
Open

brianbaldock wants to merge 1 commit into
NousResearch:mainfrom
brianbaldock:fix/skill-evolution-writeback

Conversation

@brianbaldock

Copy link
Copy Markdown

What does this PR do?

Phase 1 skill evolution could never emit a genuinely evolved skill. Two independent defects sat on the same path, and either one alone is sufficient to make the pipeline a no-op:

  1. The optimizer could not see the skill text. SkillModule stored the skill body in a plain Python attribute (self.skill_text) and passed it to the signature as an InputField. DSPy optimizers rewrite signature instructions and demos — not arbitrary instance attributes — so GEPA and MIPROv2 had nothing to mutate. evolve_skill.py then read that same untouched attribute back as the "evolved" body, so the file written to output/<skill>/evolved_skill.md was byte-identical to the baseline.

  2. Every evolved candidate failed structural validation. The validator was handed skill["body"], but _check_skill_structure requires YAML frontmatter — which load_skill has already stripped out of the body. The evolved candidate was therefore rejected before the holdout comparison ever ran.

Why this approach: rather than patching the two call sites, the skill text is moved into the state the optimizer actually owns (signature instructions, read back through a property), and structural validation is given an API that makes the mistake unrepresentable. ConstraintValidator.validate_skill() takes frontmatter and body separately and reassembles internally, so a bare body can no longer reach the frontmatter check by mistake. A no-op guard then aborts the run when the optimizer returns unchanged text, so the pipeline cannot report a delta that is measuring noise instead of evolution.

That last guard matters beyond these two bugs: with defect 1 present, any reported improvement was measuring the DSPy wrapper, not the artifact being shipped. Failing loudly is better than a plausible number.

Related Issue

Fixes #141
Fixes #11

Also reported as: #34, #74, #93, #110, #169, #171 (validator target) and #38, #87, #119, #172, #175 (optimizer no-op).

Prior art — please read before reviewing this one. Per CONTRIBUTING.md's "Search First" step, I searched before opening this. Both bugs are long-standing and several contributors got here first. Overlapping open PRs include #137, #140, #153, #161, #168, #174, #177, #178 and #183. I am not claiming precedence and this should not jump the queue#97 and #140 predate it substantially. If a maintainer prefers any of those, close this one; I am happy to review or rebase onto whichever lands first. I opened it because it adds two things I did not find elsewhere: an API shape that prevents defect 2 from recurring, and the no-op guard that makes defect 1 fail loudly instead of silently producing a number.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • evolution/skills/skill_module.pySkillModule now builds its signature with .with_instructions(skill_text) and exposes skill_text as a read-only property backed by predictor.predict.signature.instructions. Removed the skill_instructions InputField; the skill is instructions, not runtime input.
  • evolution/core/constraints.py — added ConstraintValidator.validate_skill(frontmatter, body, baseline_body=None), which applies size/growth/non-empty to the body and the structural check to the reassembled file. validate_all is unchanged for other artifact types.
  • evolution/skills/evolve_skill.py — both baseline and evolved validation now call validate_skill; added the unchanged-text guard between extraction and validation.
  • tests/skills/test_evolution_writeback.py — new; pins both defects.
  • tests/skills/test_evolution_e2e.py — new; drives the full extract → guard → validate → reassemble chain with a stub optimizer (no network, no API key).

How to Test

  1. Confirm the defects on main (both are offline checks):

    python -c "from evolution.skills.skill_module import SkillModule; m=SkillModule('x'); print([n for n,_ in m.named_parameters()])"
    

    On main this prints ['predictor.predict']skill_text is absent, so no optimizer can reach it.

  2. Reproduce defect 2 against any real skill:

    python -c "
    from pathlib import Path
    from evolution.skills.skill_module import load_skill
    from evolution.core.constraints import ConstraintValidator
    from evolution.core.config import EvolutionConfig
    p = next(Path('<hermes-agent>/skills').rglob('SKILL.md'))
    s = load_skill(p)
    v = ConstraintValidator(EvolutionConfig(hermes_agent_path=Path('<hermes-agent>')))
    print(v._check_skill_structure(s['body']).message)"
    

    On main: Skill missing: YAML frontmatter (---), name field, description field.

  3. Run the new tests, which fail on main and pass here:

    pytest tests/skills/test_evolution_writeback.py tests/skills/test_evolution_e2e.py -q
    
  4. Run the full suite: pytest tests/ -q154 passed.

Checklist

Code

  • I've read the Contributing Guide — this repo has none, so I followed the parent hermes-agent guide and its PR template
  • My commit messages follow Conventional Commits (fix(skills):)
  • I searched for existing PRs to make sure this isn't a duplicate — it overlaps several; see Related Issue above, where I name them and defer to the older ones
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass (154 passed)
  • I've added tests for my changes
  • I've tested on my platform: Debian GNU/Linux 13 (trixie), Python 3.13.5, dspy 3.3.1, pytest 9.1.1

Documentation & Housekeeping

  • I've updated relevant documentation (docstrings on both changed classes explain why the old shape was wrong) — README/docs/ N/A, no user-facing interface changed
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A, no config keys touched
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A, neither file exists in this repo
  • I've considered cross-platform impact — N/A, pure in-memory string handling and existing pathlib usage; no file I/O, process management, or path semantics changed
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A, no Hermes tools involved

Not claimed: I have not run a real GEPA optimization end-to-end against a live API, so I am not asserting that evolution now produces better skills — only that the optimizer's output is no longer discarded and candidates are no longer rejected by a false-positive gate. Whether the current fitness function (bag-of-words overlap, fitness.py:129-134) can produce a meaningful improvement signal is a separate question, tracked in #12 and #33.

Screenshots / Logs

Full suite on this branch:

$ python -m pytest tests/ -q
........................................................................ [ 46%]
........................................................................ [ 93%]
..........                                                               [100%]
154 passed in 2.27s

Sabotage probe confirming the no-op guard discriminates (reverting the fix in-memory and re-running):

FIXED   : body changed=True   guard_aborts=False
SABOTAGE: body changed=False  guard_aborts=True

PROOF OK: guard is silent on a real rewrite and ABORTS on the original bug.

…he full file

Phase 1 could never emit a genuinely evolved skill. Two defects:

1. SkillModule held the skill text in a plain Python attribute and passed it
   to the signature as an InputField, so no DSPy optimizer could rewrite it.
   evolve_skill then read that same untouched attribute back as the "evolved"
   body, making the written file byte-identical to the baseline.

2. The constraint validator received skill["body"], but _check_skill_structure
   requires YAML frontmatter that load_skill has already stripped out of the
   body. Every evolved candidate failed structural validation and was rejected
   before the holdout comparison ran.

The skill text now lives in the predictor's signature instructions (the state
GEPA and MIPROv2 actually rewrite) and is read back through a property.
A new ConstraintValidator.validate_skill takes frontmatter and body separately
and reassembles before the structural check, so a bare body can no longer reach
the frontmatter validator by mistake. A no-op guard aborts the run when the
optimizer returns unchanged text, rather than reporting a delta that would be
measuring noise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant