Skip to content

fix: load_skill handles dir paths; validate evolved_full not body#51

Open
shilei103-afk wants to merge 1 commit into
NousResearch:mainfrom
shilei103-afk:fix/load-skill-validate-full
Open

fix: load_skill handles dir paths; validate evolved_full not body#51
shilei103-afk wants to merge 1 commit into
NousResearch:mainfrom
shilei103-afk:fix/load-skill-validate-full

Conversation

@shilei103-afk
Copy link
Copy Markdown

Summary

Two bugs prevented the self-evolution pipeline from completing:

Bug 1: load_skill() receives directory path, reads empty content

In evolution/skills/skill_module.py, find_skill() returns a directory path (e.g. /skills/github/github-code-review/). load_skill() tried to read this path as a file directly, got empty content, and all constraint checks failed silently.

Fix: detect when path is a directory and append /SKILL.md.

Bug 2: validate_all() called on evolved_body instead of evolved_full

In evolution/skills/evolve_skill.py, constraint validation was called on evolved_body (markdown body only, no frontmatter), so skill_structure constraint always failed because it checks for YAML frontmatter --- and name: field.

Fix: validate evolved_full (frontmatter + body) instead.

Bonus: holdout eval crash now caught

DSPy JSONAdapter crashes when the skill outputs bash commands. This is now wrapped in try/except so constraints-passed skills are always saved.

Testing

  • Dry run passes: ✓ skill_structure: Skill has valid frontmatter (name + description)
  • Optimization completes with all constraints passing
  • Output saved to output/github-code-review/<timestamp>/

Two bugs prevented the self-evolution pipeline from completing:

1. skill_module.py load_skill(): when find_skill() returns a directory
   path (e.g. /skills/github/github-code-review/), load_skill() tried to
   read it as a file, got empty/None content, and all constraint checks
   failed. Fixed by appending /SKILL.md when path is a directory.

2. evolve_skill.py: validate_all() was called on evolved_body (markdown
   only, no frontmatter) instead of evolved_full (frontmatter + body),
   so skill_structure constraint always failed. Also, holdout evaluation
   crashes (DSPy JSONAdapter on bash output) now caught so constraints-
   passed skill is always saved.
@steezkelly
Copy link
Copy Markdown

I did a quick pass over this because it overlaps with the local/fork fixes from #38/#49.

Useful parts:

One thing to double-check before merge:

  • On current upstream main, find_skill() appears to already return the concrete SKILL.md path, not the skill directory. So the load_skill() directory fallback is harmless defensive hardening, but I could not reproduce the claim that upstream find_skill() itself returns a directory path.

Potential concern:

  • The broad except Exception around holdout evaluation may make the pipeline report/save a candidate with 0.0/0.0 scores if the real evaluator is broken. That is useful for avoiding artifact loss, but it weakens the deploy/quality signal. I would consider splitting that into a separate PR or recording holdout_evaluation_failed: true in metrics so downstream automation does not treat the candidate as cleanly evaluated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants