From c5f121e08aad87e5954f8e6314244f267548e467 Mon Sep 17 00:00:00 2001 From: Brian Baldock Date: Wed, 2 Sep 2026 06:56:10 +0000 Subject: [PATCH] fix(skills): make evolved skill text optimizer-visible and validate the 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. --- evolution/core/constraints.py | 25 +++++ evolution/skills/evolve_skill.py | 24 +++- evolution/skills/skill_module.py | 27 +++-- tests/skills/test_evolution_e2e.py | 79 +++++++++++++ tests/skills/test_evolution_writeback.py | 135 +++++++++++++++++++++++ 5 files changed, 275 insertions(+), 15 deletions(-) create mode 100644 tests/skills/test_evolution_e2e.py create mode 100644 tests/skills/test_evolution_writeback.py diff --git a/evolution/core/constraints.py b/evolution/core/constraints.py index d6b13459..5c33bb84 100644 --- a/evolution/core/constraints.py +++ b/evolution/core/constraints.py @@ -52,6 +52,31 @@ def validate_all( return results + def validate_skill( + self, + frontmatter: str, + body: str, + baseline_body: Optional[str] = None, + ) -> list[ConstraintResult]: + """Validate a skill from its parts, reassembling before structural checks. + + Structural validation needs the *whole file* (frontmatter + body), while + size and growth are most meaningful against the body — the only part + evolution actually rewrites. Callers pass the parts and this method + applies each constraint to the right text, so a bare body can no longer + be handed to the frontmatter check by mistake. + """ + from evolution.skills.skill_module import reassemble_skill + + results = [ + self._check_size(body, "skill"), + self._check_non_empty(body), + self._check_skill_structure(reassemble_skill(frontmatter, body)), + ] + if baseline_body is not None: + results.insert(1, self._check_growth(body, baseline_body, "skill")) + return results + def run_test_suite(self, hermes_repo: Path) -> ConstraintResult: """Run the full hermes-agent test suite. Must pass 100%.""" try: diff --git a/evolution/skills/evolve_skill.py b/evolution/skills/evolve_skill.py index 2a79a670..02a76d2b 100644 --- a/evolution/skills/evolve_skill.py +++ b/evolution/skills/evolve_skill.py @@ -118,7 +118,9 @@ def evolve( # ── 3. Validate constraints on baseline ───────────────────────────── console.print(f"\n[bold]Validating baseline constraints[/bold]") validator = ConstraintValidator(config) - baseline_constraints = validator.validate_all(skill["body"], "skill") + baseline_constraints = validator.validate_skill( + frontmatter=skill["frontmatter"], body=skill["body"] + ) all_pass = True for c in baseline_constraints: icon = "✓" if c.passed else "✗" @@ -179,13 +181,29 @@ def evolve( console.print(f"\n Optimization completed in {elapsed:.1f}s") # ── 6. Extract evolved skill text ─────────────────────────────────── - # The optimized module's instructions contain the evolved skill text + # The optimized module's signature instructions ARE the evolved skill text. evolved_body = optimized_module.skill_text evolved_full = reassemble_skill(skill["frontmatter"], evolved_body) + # Guard: if the optimizer produced no textual change, there is nothing to + # evaluate and any holdout delta would be measuring noise, not evolution. + if evolved_body.strip() == skill["body"].strip(): + console.print( + "\n[red]✗ Optimizer returned an unchanged skill body — aborting.[/red]" + ) + console.print( + " No evolution occurred, so no improvement can be reported.\n" + " Check that the optimizer ran and that skill text is optimizer-visible state." + ) + return + # ── 7. Validate evolved skill ─────────────────────────────────────── console.print(f"\n[bold]Validating evolved skill[/bold]") - evolved_constraints = validator.validate_all(evolved_body, "skill", baseline_text=skill["body"]) + evolved_constraints = validator.validate_skill( + frontmatter=skill["frontmatter"], + body=evolved_body, + baseline_body=skill["body"], + ) all_pass = True for c in evolved_constraints: icon = "✓" if c.passed else "✗" diff --git a/evolution/skills/skill_module.py b/evolution/skills/skill_module.py index 6d4d22ed..c77c4c32 100644 --- a/evolution/skills/skill_module.py +++ b/evolution/skills/skill_module.py @@ -84,11 +84,13 @@ def find_skill(skill_name: str, hermes_agent_path: Path) -> Optional[Path]: class SkillModule(dspy.Module): """A DSPy module that wraps a skill file for optimization. - The skill text (body) is the parameter that GEPA optimizes. - On each forward pass, the module: - 1. Uses the skill text as instructions - 2. Processes the task input - 3. Returns the agent's response + The skill text is held as the predictor's *signature instructions*, which is + the state DSPy optimizers (GEPA, MIPROv2) actually rewrite. Reading + `skill_text` back after `compile()` therefore returns the evolved text. + + Holding it in a plain Python attribute instead — or passing it as an + InputField — leaves it invisible to every optimizer, so the "evolved" skill + written to disk is byte-identical to the baseline. """ class TaskWithSkill(dspy.Signature): @@ -97,20 +99,21 @@ class TaskWithSkill(dspy.Signature): You are an AI agent following specific skill instructions to complete a task. Read the skill instructions carefully and follow the procedure described. """ - skill_instructions: str = dspy.InputField(desc="The skill instructions to follow") task_input: str = dspy.InputField(desc="The task to complete") output: str = dspy.OutputField(desc="Your response following the skill instructions") def __init__(self, skill_text: str): super().__init__() - self.skill_text = skill_text - self.predictor = dspy.ChainOfThought(self.TaskWithSkill) + signature = self.TaskWithSkill.with_instructions(skill_text) + self.predictor = dspy.ChainOfThought(signature) + + @property + def skill_text(self) -> str: + """The current skill text, read from optimizer-owned state.""" + return self.predictor.predict.signature.instructions def forward(self, task_input: str) -> dspy.Prediction: - result = self.predictor( - skill_instructions=self.skill_text, - task_input=task_input, - ) + result = self.predictor(task_input=task_input) return dspy.Prediction(output=result.output) diff --git a/tests/skills/test_evolution_e2e.py b/tests/skills/test_evolution_e2e.py new file mode 100644 index 00000000..416d33de --- /dev/null +++ b/tests/skills/test_evolution_e2e.py @@ -0,0 +1,79 @@ +"""End-to-end proof that the write-back path works, using a fake optimizer. + +No network, no API keys: a stub optimizer stands in for GEPA and rewrites the +signature instructions the way a real one does. This exercises the whole +extract -> guard -> validate -> reassemble chain. +""" + +import pytest + +from evolution.skills.skill_module import SkillModule, load_skill, reassemble_skill + + +SAMPLE_SKILL = """--- +name: demo-skill +description: Demo skill +--- + +# Demo + +## Procedure +1. Baseline step +""" + + +class FakeOptimizer: + """Stands in for dspy.GEPA: rewrites predictor signature instructions.""" + + def __init__(self, new_instructions: str): + self.new_instructions = new_instructions + + def compile(self, module, **_kwargs): + for _, predictor in module.named_predictors(): + predictor.signature = predictor.signature.with_instructions( + self.new_instructions + ) + return module + + +def test_optimizer_rewrite_survives_extraction_and_reassembly(tmp_path): + skill_file = tmp_path / "SKILL.md" + skill_file.write_text(SAMPLE_SKILL) + skill = load_skill(skill_file) + + module = SkillModule(skill["body"]) + evolved_text = "# Demo\n\n## Procedure\n1. Evolved step\n2. Extra verification step" + + optimized = FakeOptimizer(evolved_text).compile(module) + + evolved_body = optimized.skill_text + assert evolved_body != skill["body"], "optimizer rewrite was lost" + assert "Evolved step" in evolved_body + + evolved_full = reassemble_skill(skill["frontmatter"], evolved_body) + assert evolved_full.startswith("---") + assert "name: demo-skill" in evolved_full + assert "Evolved step" in evolved_full + + +def test_noop_optimizer_is_detectable(tmp_path): + """SABOTAGE CHECK: an optimizer that changes nothing must be detectable. + + This is the condition the pipeline's no-op guard aborts on. If this test + ever passes trivially, the guard is not measuring anything. + """ + skill_file = tmp_path / "SKILL.md" + skill_file.write_text(SAMPLE_SKILL) + skill = load_skill(skill_file) + + module = SkillModule(skill["body"]) + + class NoOpOptimizer: + def compile(self, module, **_kwargs): + return module # the original bug's behaviour + + optimized = NoOpOptimizer().compile(module) + + assert optimized.skill_text.strip() == skill["body"].strip(), ( + "no-op optimizer somehow changed the text; guard would not fire" + ) diff --git a/tests/skills/test_evolution_writeback.py b/tests/skills/test_evolution_writeback.py new file mode 100644 index 00000000..13620567 --- /dev/null +++ b/tests/skills/test_evolution_writeback.py @@ -0,0 +1,135 @@ +"""Regression tests for the Phase 1 skill-evolution write-back path. + +Two defects made `evolve_skill` unable to ever emit a genuinely evolved skill: + +1. `SkillModule` carried 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.py` then read that same untouched attribute back as the + "evolved" body. + +2. The constraint validator was handed `skill["body"]`, but `_check_skill_structure` + requires YAML frontmatter — which `load_skill` has already stripped out of the + body. Every evolved candidate therefore failed structural validation and was + rejected before the holdout comparison ran. + +These tests pin both behaviours so they cannot silently regress. +""" + +import pytest + +from evolution.core.config import EvolutionConfig +from evolution.core.constraints import ConstraintValidator +from evolution.skills.skill_module import ( + SkillModule, + load_skill, + reassemble_skill, +) + + +SAMPLE_SKILL = """--- +name: test-skill +description: A skill for testing things +version: 1.0.0 +--- + +# Test Skill — Testing Things + +## Procedure +1. First, do the thing +2. Then, verify it worked +""" + + +@pytest.fixture +def config(tmp_path): + return EvolutionConfig(hermes_agent_path=tmp_path) + + +class TestSkillTextIsOptimizable: + """Defect 1: the skill text must be optimizer-visible state, not a bare attribute.""" + + def test_skill_text_is_exposed_as_predictor_instructions(self): + module = SkillModule("# Baseline procedure\n1. Do the thing") + + instructions = [ + predictor.signature.instructions + for _, predictor in module.named_predictors() + ] + + assert instructions, "module exposes no predictors for the optimizer to touch" + assert any( + "Baseline procedure" in text for text in instructions + ), "skill text is not reachable through any predictor's signature instructions" + + def test_evolved_instructions_are_read_back(self): + """What the optimizer rewrites must be what we serialize.""" + module = SkillModule("# Baseline procedure") + + # Simulate what a DSPy optimizer does: rewrite signature instructions. + for _, predictor in module.named_predictors(): + predictor.signature = predictor.signature.with_instructions( + "# EVOLVED procedure\n1. Do the better thing" + ) + + assert "EVOLVED procedure" in module.skill_text + assert "Baseline procedure" not in module.skill_text + + +class TestStructureValidationTarget: + """Defect 2: structural validation must run against the reassembled file.""" + + def test_bare_body_has_no_frontmatter(self, tmp_path): + skill_file = tmp_path / "SKILL.md" + skill_file.write_text(SAMPLE_SKILL) + skill = load_skill(skill_file) + + # Precondition for the bug: load_skill strips frontmatter out of the body. + assert not skill["body"].lstrip().startswith("---") + + def test_bare_body_fails_structure_check(self, tmp_path, config): + """Pins WHY passing the body was wrong — it can never pass.""" + skill_file = tmp_path / "SKILL.md" + skill_file.write_text(SAMPLE_SKILL) + skill = load_skill(skill_file) + + result = ConstraintValidator(config)._check_skill_structure(skill["body"]) + assert not result.passed + + def test_reassembled_skill_passes_structure_check(self, tmp_path, config): + skill_file = tmp_path / "SKILL.md" + skill_file.write_text(SAMPLE_SKILL) + skill = load_skill(skill_file) + + evolved_full = reassemble_skill(skill["frontmatter"], skill["body"]) + result = ConstraintValidator(config)._check_skill_structure(evolved_full) + + assert result.passed, result.message + + def test_validate_skill_reassembles_internally(self, tmp_path, config): + """The call site passes frontmatter + body; the validator owns assembly. + + This is what makes the original defect unrepresentable: callers can no + longer hand a bare body to skill validation by mistake. + """ + skill_file = tmp_path / "SKILL.md" + skill_file.write_text(SAMPLE_SKILL) + skill = load_skill(skill_file) + + results = ConstraintValidator(config).validate_skill( + frontmatter=skill["frontmatter"], + body=skill["body"], + baseline_body=skill["body"], + ) + + failed = [r for r in results if not r.passed] + assert not failed, f"unexpected failures: {[(r.constraint_name, r.message) for r in failed]}" + + def test_validate_skill_still_catches_broken_frontmatter(self, tmp_path, config): + """Reassembly must not paper over genuinely malformed frontmatter.""" + results = ConstraintValidator(config).validate_skill( + frontmatter="version: 1.0.0", # no name, no description + body="# Body", + ) + + structure = [r for r in results if r.constraint_name == "skill_structure"] + assert structure and not structure[0].passed