Skip to content

Refactor skills installation/status around shared multi-agent catalog#549

Open
mariusvniekerk wants to merge 4 commits intomainfrom
t3code/refactor-skills-catalog
Open

Refactor skills installation/status around shared multi-agent catalog#549
mariusvniekerk wants to merge 4 commits intomainfrom
t3code/refactor-skills-catalog

Conversation

@mariusvniekerk
Copy link
Collaborator

@mariusvniekerk mariusvniekerk commented Mar 20, 2026

Summary

  • Replace per-agent install/update logic with a shared supportedAgents catalog and a unified installAgent flow.
  • Add reusable helpers for agent path resolution, embedded skill loading, installed-skill detection, and legacy-skill cleanup.
  • Update ListSkills to build metadata from a combined embedded catalog across agents instead of a Claude-only source.
  • Refactor Status and Update to use shared catalog-driven logic, including legacy-only installs via anyFileExists checks.
  • Rework skills tests to be agent-parameterized and dynamically derive expected skill names from ListSkills.

Testing

  • Not run (tests were not executed in this context).
  • Verified by inspection that refactored tests cover install, idempotency, legacy cleanup, and update behavior for both Claude and Codex paths.

@roborev-ci
Copy link

roborev-ci bot commented Mar 20, 2026

roborev: Combined Review (1b6d941)

Verdict: The ref
actor introduces a medium severity issue regarding skill installation detection brittleness that needs to be addressed.

Medium

  • Location: internal/skills/skills.go (around currentInstalledSkillFilePaths, installedSkillFilePaths, and Update)
  • Problem: Installation detection now depends on successfully reading
    every embedded SKILL.md for an agent. A single missing or unreadable embedded skill file will make IsInstalled return false and cause Update to fail for that entire agent, even though the local installation state could still be determined from the filesystem alone. The previous implementation only checked expected paths and was not blocked
    by embedded content reads.
  • Fix: Split "enumerate embedded skill directory names" from "read embedded skill contents/frontmatter". Use directory names only for IsInstalled/Update path checks, and reserve full file reads for install/list/status code paths that actually need the content.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

mariusvniekerk and others added 3 commits March 23, 2026 17:53
@mariusvniekerk @wesm
Refactor skills installer around shared multi-agent catalog
d61b918 
- Replace duplicated Claude/Codex install and status logic with shared agent specs
- Build skill metadata and installed-path checks from embedded skills dynamically
- Update tests to use agent table cases and runtime-derived expected skills
@wesm @claude
Decouple installation detection from embedded file reads
f420f48 
IsInstalled and Update only need directory names to check filesystem
paths, not full SKILL.md content. Add embeddedSkillDirNames() that
enumerates directories without reading files, so a missing or
unreadable embedded file no longer breaks installation detection.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@wesm @claude
Simplify ListSkills dedup and add regression tests
6531468 
Replace embeddedSkillCatalog's field-by-field merge with a simpler
seen-set dedup in ListSkills, making it explicit that the first
agent's metadata wins when frontmatter differs across agents.

Add tests covering: deduplication across agents, first-agent-wins
metadata selection, and a mock-FS regression test verifying that
installation detection (embeddedSkillDirNames/currentInstalledSkillFilePaths)
succeeds even when embedded SKILL.md files are absent.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@wesm wesm force-pushed the t3code/refactor-skills-catalog branch from 1b6d941 to 6531468 Compare March 23, 2026 23:53
@roborev-ci
Copy link

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (6531468)

Verdict: The refactor successfully consolidates skill installation logic, but introduces a medium
-severity issue regarding error isolation in status reporting.

Medium

  • Location: internal/skills/skills.go (around Status() / embeddedSkillsForAgent(spec) call)
  • Problem: Status() now aborts status collection for the entire agent if reading any single
    embedded SKILL.md fails. Before this refactor, a bad/missing embedded file only affected that one skill. This makes packaging mistakes much harder to diagnose because one broken skill now turns into an empty/partial status for the whole agent.
  • Fix: Keep the per-skill read/compare
    loop inside Status(), or change embeddedSkillsForAgent to return partial results plus per-skill errors so unreadable skills can be marked missing without dropping the rest.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
Fix lint: use slices.ContainsFunc and assert.Contains
932e21d 
Address modernize and testifylint findings from CI.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link

roborev-ci bot commented Mar 23, 2026

roborev: Combined Review (932e21d)

Verdict: All agents agree the code is clean; no medium, high, or critical issues were found during the review.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mariusvniekerk @wesm