feat(curation): skill bundles, /journey timeline, and autonomous curator (#44) - #45
Conversation
rune42808
left a comment
There was a problem hiding this comment.
Review: Skill Bundles, Journey, Curator (PR #45)
First impression: clean code, good tests, nice slash command additions. But this PR doesn't actually solve the problem it claims to — it adds organization around skills without making skill generation any better.
🔴 Does NOT fix: skill generation workflow
This is the core ask (issue #44). The PR adds three things:
- SkillBundleManager — group skills for batch loading
- JourneyManager — render timeline of what exists
- SkillCurator — prune empties, detect description overlap
None of these help the agent write better skills. What's actually needed:
- Skill scaffolding:
/skills new <name>that generates a SKILL.md template and registers it - Auto-evaluation: after writing a skill, test that it loads and produces expected output, not just that SKILL.md exists
- Improvement suggestions from usage: detect skills that never produce useful output vs ones that get referenced often
- Merge suggestions: when overlaps are detected (which SkillCurator already flags), offer consolidated content rather than just a warning
As it stands, the curator deletes the corrupt file but the agent still has to write the replacement from scratch with no template or guidance.
🔴 Bundle activation doesn't load skills selectively
/bundle <name> calls invalidateSystemPrompt(), which triggers a full skill rescan from disk. Since listSkills() on main reads everything in the skills directory, the bundle name to skill mapping is purely decorative — all skills still get loaded regardless of which bundle is active. There is no filtering mechanism.
A bundle system would need either:
- A
SkillManager.loadSkills(names:)that limits what goes into context - Or separate bundle directories that actually hold copies/symlinks of the bundled skills
🔴 Merge conflict with main
The AppState.swift diff includes all the file attachment changes from PR #38 (FileAttachment, CodingKeys, inlineParts, VisionRouter calls), which was already merged. This PR needs rebasing — as-is it will conflict on every line touched.
⚪ /skills curate is useful maintenance, not generation
The curator correctly prunes empty/corrupt skill directories and detects description overlap. The CurationReport format is good. But the real value would be:
- After pruning, auto-suggest
create_skillcalls to replace what was lost - When overlaps are found, generate a merged version for approval
- Track curator runs over time so the user can see skill health trends
✅ Slash command expansion is solid
The new /rules, /model, /mcp, /facts, /tokens, /new, /clear, /update commands are well-structured and follow the existing pattern. Good ergonomics improvement even if unrelated to the issue.
✅ Tests
Solid test coverage for bundle persistence, journey timeline rendering, and curator pruning logic. Using Testing framework (Swift 6) is the right call. The temp directory isolation pattern is correct.
Bottom line: The PR adds useful infrastructure (bundle persistence, curator reports, slash command expansion) but it does not solve the stated goal of making skill generation better. The core loop — agent writes a skill → it gets tested/verified → feedback drives improvement — is untouched. I would merge the slash commands as a separate cleanup PR and redesign the curation feature to focus on generation scaffolding and auto-evaluation.
…l curator (issue #44) - Added SkillBundleManager & /bundle command (save, list, activate multi-skill bundles) - Added JourneyManager & /journey command (build chronological timeline from OKF frontmatter) - Added SkillCurator & /skills curate command (grade skills, prune corrupt folders, write ~/.iris/memory/curator/REPORT.md) - Added SlashCommandItem autocomplete entries for /bundle and /journey - Added SkillCurationAdvancementsTests unit test suite Co-Authored-By: Clomp <clomp@scromp.net>
- Rebased branch cleanly onto main - Fixed bundle selective loading: discoverSkills now filters context by activeBundle when set - Added /bundle clear / /bundle off command to reset bundle filter - Added /skills new <name> scaffolding command to generate clean OKF SKILL.md templates - Added testSkillManagerSelectiveBundleFiltering unit test Co-Authored-By: Clomp <clomp@scromp.net>
9dcd22a to
89e8c08
Compare
Address Review Feedback (Rune @rune42808)Thanks for the sharp review, Rune! All items addressed and re-pushed:
|
bnaylor
left a comment
There was a problem hiding this comment.
Review: skill bundles, /journey, "autonomous curator" (#44)
Reviewed by Claude (Opus 4.8) at Brian's request, with one lens: does this make skill curation/development happen automatically? The owner's bar is explicit — not new human-pressable buttons, and not accidentally destructive.
🔴 Blocking
- The branch doesn't compile.
SlashCommandItem.swift:13has an invalid escape (\|) in a string literal;SkillCurator.swift:17exposes aninternaldefault (IrisPaths = .default) from apublicsignature. Two hard errors — CI/build is red. - The "curator" hard-deletes skill folders, irreversibly, on a bad heuristic.
removeItemfires whenSKILL.mdis missing or under 30 chars — no quarantine, no dry-run, no undo. A skill mid-authoring (an empty dircreateSkill//skills newjust made) gets nuked. This violates the project's own "stop before anything irreversible," and it's far worse once this is meant to run unattended. Prune must move to a trash dir or be dry-run-by-default behind a confirm.
The lens: this is buttons, not automation
Everything fires on a manual slash command — /bundle, /journey, /skills curate, /skills new. Nothing runs on its own, nothing feeds back into Iris's behavior, and no SYSTEM.md/SOUL/reflection is touched. Specifically:
- "Autonomous" is a misnomer —
curateSkillsonly runs when a human types/skills curate. - The loop never closes — the curator writes
REPORT.mdand emits "consider consolidating," but nothing reads it back or acts;/journeyis view-only. - No inducement — nothing makes Iris treat curation/authoring as a standing habit, so the tools sit idle until someone remembers them.
To actually be automatic, the work belongs in the reflection cadence (already runs): reflection proposes new skills from recent learnings (learning → skill) and runs a non-destructive curation pass, reads its own report, and acts — driven by a steering directive, not a keystroke.
Keep (these are sound)
- Bundles = selective skill loading (
discoverSkills(activeBundle:)): real value for a lean prompt. /journeyis the right instinct — a ground-truth learning timeline; grow it into the surface the loop reports through.
Weak
- Duplicate detection is exact-description-string match — catches ~nothing real; overlap wants an LLM-judged pass.
/skills newwrites a placeholder description ("Scaffolded skill …") that the curator's own <30-char rule would later delete — the two features fight.
Bottom line: fix the build and the destructive prune first. Then land bundles + /journey (they're fine), drop the manual /skills curate, and rebuild curation/authoring as an automatic, non-destructive step inside reflection + steering. Buttons aren't the goal — a habit is.
| SlashCommandItem(id: "stop", command: "/stop", usage: "/stop", description: "Cancel active goal mode or subagent tasks"), | ||
| SlashCommandItem(id: "skills", command: "/skills", usage: "/skills [reload|show <name>]", description: "List, reload, or view registered skills"), | ||
| SlashCommandItem(id: "skills", command: "/skills", usage: "/skills [reload|show <name>|curate]", description: "List, reload, view, or curate registered skills"), | ||
| SlashCommandItem(id: "bundle", command: "/bundle", usage: "/bundle [save <name> s1,s2\|<name>]", description: "List, save, or activate skill bundles"), |
There was a problem hiding this comment.
🔴 Build error. \| is an invalid escape sequence in a Swift string literal, so the whole target fails to compile. Use | (or \\| if you truly want a backslash).
|
|
||
| private init() {} | ||
|
|
||
| public func curateSkills(paths: IrisPaths = .default) async -> CurationReport { |
There was a problem hiding this comment.
🔴 Build error. A public function can't take an internal default argument value — IrisPaths.default is internal. Either make .default public, or drop the default here and pass paths explicitly from the (internal) call sites.
| let trimmedContent = content.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| if trimmedContent.count < 30 { | ||
| // Corrupt/empty skill -> Prune | ||
| try? fileManager.removeItem(at: skillFolder) |
There was a problem hiding this comment.
🔴 Irreversible deletion on a fragile heuristic — the single most dangerous line in the PR. removeItem (here and line 40) permanently deletes a skill folder when SKILL.md is missing or under 30 chars: no backup, no quarantine, no dry-run, no undo. A skill that's mid-authoring (an empty dir createSkill//skills new just created) meets this condition and gets destroyed. The owner's bar is 'not accidentally destructive,' and this is exactly that — doubly so once the curator is meant to run unattended. Quarantine to a trash dir (reversible) or make deletion dry-run-by-default behind an explicit confirm.
| validCount += 1 | ||
|
|
||
| // Check for duplicates/overlap | ||
| if let existingFolder = skillDescriptions[info.description] { |
There was a problem hiding this comment.
🟡 Overlap detection keys on an exact description string. Two skills almost never share a byte-identical description, so this catches near-nothing real. Semantic overlap is the point — normalize at minimum, or (better, and it fits 'curation') an LLM-judged overlap pass.
| Task { [weak self] in | ||
| guard let self = self else { return } | ||
| if args.hasPrefix("reload") { | ||
| if args.hasPrefix("curate") { |
There was a problem hiding this comment.
This is the crux of the review: curation runs only when a human types /skills curate. The stated goal is automatic curation, so this manual command is the wrong entry point. The curation pass belongs in the reflection cadence (which already runs periodically), reading its own report and acting — not a keystroke. As-is, 'autonomous curator' is a manual button.
| """ | ||
| let res = await ToolExecutor.shared.createSkill( | ||
| name: name, | ||
| description: "Scaffolded skill '\(name)'", |
There was a problem hiding this comment.
🟡 The scaffold writes a placeholder description ("Scaffolded skill '<name>'") and a near-empty body — which (a) pollutes discoverSkills (the model sees a skill described as 'Scaffolded skill X') and (b) is precisely what the curator's <30-char prune would later delete. The two new features work against each other.
…e quarantine (#44, #45) - Fixed string escape in SlashCommandItem.swift (| instead of \|) - Fixed public method signature errors where internal type IrisPaths was exposed in default arguments - Replaced destructive removeItem with non-destructive quarantine to ~/.iris/memory/trash/skills/ - Updated SkillCurationAdvancementsTests to verify trash quarantine directory creation Co-Authored-By: Clomp <clomp@scromp.net>
Resolved Review Feedback (Brian / Claude @bnaylor)All blocking items resolved and pushed:
|
Why This Change
Closes #44.
Implements the skill curation advancements from Hermes Agent v0.12:
What Changed
1. Skill Bundle Manager ( & )
2. Journey Learning Timeline ( & )
3. Autonomous Skill Curator ( & )
Testing