feat(skills): publish workflow skills to skills.sh#1357
Conversation
📝 WalkthroughWalkthroughAdds skills.sh-compatible OpenSpec skills, a generator with filesystem safeguards, workflow template updates, line-ending normalization, onboarding documentation, and parity tests for committed generated files. ChangesSkills.sh publication
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Commit the 12 OpenSpec workflow skills as static skills/<name>/SKILL.md so `npx skills add Fission-AI/OpenSpec` can install them (skills.sh reads static files from the repo; OpenSpec otherwise only generates skills at init time). Files are generated from the existing templates via `pnpm generate:skills`, not hand-copied, and skillssh-parity.test.ts fails CI if a template changes without regenerating. The volatile generatedBy frontmatter line is stripped so the committed copies stay byte-stable across releases. Closes Fission-AI#1258 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2c0ebb7 to
5115467
Compare
The skills.sh distribution files are generated LF-only and compared byte-for-byte by skillssh-parity.test.ts. Windows autocrlf checked them out as CRLF, failing the parity assertion. A scoped .gitattributes pins them to LF on checkout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
skills/openspec-explore/SKILL.md (1)
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to fenced blocks.
markdownlintreports MD040 for these fences. Use identifiers such astext,bash, ormarkdownconsistently for the displayed examples.Also applies to: 150-150, 173-173, 204-204, 221-221, 262-262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/openspec-explore/SKILL.md` at line 56, Add explicit language identifiers to every fenced code block in SKILL.md, including the blocks around the referenced examples. Use appropriate identifiers such as text, bash, or markdown consistently so all fences satisfy markdownlint MD040.Source: Linters/SAST tools
skills/openspec-onboard/SKILL.md (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to fenced blocks.
markdownlintreports MD040 for these fences. Add identifiers such astext,bash,powershell, ormarkdownto the examples.Also applies to: 85-85, 118-118, 139-139, 148-148, 169-169, 183-183, 193-193, 203-203, 213-213, 252-252, 263-263, 279-279, 308-308, 318-318, 353-353, 363-363, 391-391, 409-409, 425-425, 439-439, 449-449, 502-502, 518-518
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/openspec-onboard/SKILL.md` at line 40, Add language identifiers to every fenced code block in SKILL.md, including the listed locations, choosing appropriate tags such as text, bash, powershell, or markdown based on each block’s contents. Preserve all example content while ensuring no unlabeled fences remain.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/openspec-apply-change/SKILL.md`:
- Line 14: Executable command examples omit the selected store flag, allowing
operations to target the wrong repository. Update the command examples in
skills/openspec-apply-change/SKILL.md:14,
skills/openspec-continue-change/SKILL.md:14,
skills/openspec-ff-change/SKILL.md:14, skills/openspec-new-change/SKILL.md:14,
and skills/openspec-propose/SKILL.md:23 to include --store <id> on every listed
command that reads or writes specs or changes, while leaving commands that do
not support the flag unchanged.
In `@skills/openspec-archive-change/SKILL.md`:
- Line 107: Update the final success summary in the archive workflow so it
reflects the actual completion status tracked by the logic in Lines 38–52.
Report incomplete artifacts or tasks when archiving unfinished work, while
preserving the existing “All artifacts complete. All tasks complete.” message
only when both are actually complete.
- Around line 65-69: Update the archive decision flow in the prompt options so
selecting “Cancel” immediately aborts the operation and does not archive the
change. Only proceed to archive after the user selects a sync or archive action,
while preserving the existing sync behavior for sync choices.
- Line 61: Make main-spec resolution store-aware in both instruction sites: in
skills/openspec-archive-change/SKILL.md:61, compare each delta spec with the
selected store’s resolved main-spec path instead of hardcoding openspec/specs;
in skills/openspec-sync-specs/SKILL.md:55, read and write the main spec using
the status/store context and its resolved path rather than the current
repository path.
- Line 99: Replace each remaining unlabeled Markdown code fence with an
appropriate language-identified fence: update
skills/openspec-archive-change/SKILL.md at 99-99;
skills/openspec-bulk-archive-change/SKILL.md at 57-57, 88-88, 98-98, 104-104,
145-145, 162-162, 170-170, 185-185, 202-202, 216-216, and 231-231; and
skills/openspec-sync-specs/SKILL.md at 90-90. Preserve the existing fenced
content and choose the identifier matching each block’s syntax.
In `@skills/openspec-explore/SKILL.md`:
- Around line 84-110: Propagate the selected store through every OpenSpec
workflow command by adding --store <selected store> to list and status in
skills/openspec-explore/SKILL.md lines 84-110, and to new change (177-180),
proposal instructions (246-250), specs instructions (271-275), design
instructions (344-347), task instructions (382-385), and archive (433-436) in
skills/openspec-onboard/SKILL.md; preserve each command’s existing arguments and
workflow.
In `@skills/openspec-update-change/SKILL.md`:
- Around line 20-22: Propagate the selected store ID through every documented
command sequence: update `skills/openspec-update-change/SKILL.md` ranges 20-22,
34-46, and 59-65 to include the store flag in `openspec list`, `openspec
status`, and `openspec instructions`; update
`skills/openspec-verify-change/SKILL.md` ranges 20-22, 30-37, and 39-45
similarly for `openspec list`, `openspec status`, and `openspec instructions
apply`. Preserve the existing command flow while ensuring each command targets
the selected OpenSpec store.
In `@skills/openspec-verify-change/SKILL.md`:
- Around line 116-125: Update the fenced code block in the Verification Report
template to declare the markdown language using a markdown-labeled fence, while
preserving the report example content unchanged.
---
Nitpick comments:
In `@skills/openspec-explore/SKILL.md`:
- Line 56: Add explicit language identifiers to every fenced code block in
SKILL.md, including the blocks around the referenced examples. Use appropriate
identifiers such as text, bash, or markdown consistently so all fences satisfy
markdownlint MD040.
In `@skills/openspec-onboard/SKILL.md`:
- Line 40: Add language identifiers to every fenced code block in SKILL.md,
including the listed locations, choosing appropriate tags such as text, bash,
powershell, or markdown based on each block’s contents. Preserve all example
content while ensuring no unlabeled fences remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7a2c8ea-0cb7-40c2-9029-77392cccece6
📒 Files selected for processing (18)
.gitattributespackage.jsonscripts/generate-skillssh.mjsscripts/skillssh-shared.mjsskills/README.mdskills/openspec-apply-change/SKILL.mdskills/openspec-archive-change/SKILL.mdskills/openspec-bulk-archive-change/SKILL.mdskills/openspec-continue-change/SKILL.mdskills/openspec-explore/SKILL.mdskills/openspec-ff-change/SKILL.mdskills/openspec-new-change/SKILL.mdskills/openspec-onboard/SKILL.mdskills/openspec-propose/SKILL.mdskills/openspec-sync-specs/SKILL.mdskills/openspec-update-change/SKILL.mdskills/openspec-verify-change/SKILL.mdtest/core/templates/skillssh-parity.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The current parity test only iterates expected templates, so an extra or renamed skills directory remains publishable with green CI, and the generator follows a pre-existing symlinked skill directory when writing. Please assert the exact owned directory and file set and reject symlinked or non-contained outputs before cleanup and writes, with focused tests.
Review feedback (alfred): the parity test only visited expected templates, so an extra or renamed skills/ directory shipped with green CI, and the generator would write through a pre-existing symlinked skill directory to anywhere on disk. - generator: refuse to run if skills/ contains any symlink (checked before any deletion, so a bad tree is left intact), validate dirNames against a path-segment allowlist, and lstat the target before writing. - parity test: assert skills/ holds exactly README.md plus one real directory per template, each containing a single real SKILL.md. - focused tests cover symlink refusal (no partial deletion), traversal names, and stale-directory cleanup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CodeRabbit review on Fission-AI#1357, fixed at the template source and regenerated: - archive-change: choosing "Cancel" at the sync prompt now stops the flow instead of archiving anyway (skill + command templates). - archive-change skill: the success output no longer hardcodes "All artifacts complete. All tasks complete." when archiving incomplete work. - archive/bulk-archive/sync-specs/verify-change: language identifiers on previously plain code fences (MD040), skill and command twins alike. Golden hashes in skill-templates-parity.test.ts recomputed from dist/; skills/ regenerated via pnpm generate:skills. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@alfred-openspec Both points addressed in af6eded: Exact owned file set. Symlink / containment guards in the generator. The cleanup + write logic moved into shared helpers (
Focused tests in |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The symlink, containment, and exact-set guards resolve my original review, but the committed skills still contain the store-unaware archive and sync paths fixed by open PR #1360. Please merge #1360 first, then rebase and regenerate this distribution, or incorporate that fix here, so the two PRs cannot leave main with stale published skills and a failing parity contract.
|
Still blocked on the same head ( |
Regenerate the parity hashes and the committed skills/ distribution from the merged templates, picking up the store-aware archive/sync paths (Fission-AI#1360) and the re-read-dependencies guidance (Fission-AI#1368) alfred flagged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Fixed up per the review (097425e): merged |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/openspec-ff-change/SKILL.md (1)
4-4: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the allowlists with the workflow requirements. Both skills require
AskUserQuestionandTodoWrite, butallowed-toolsonly includesBash(openspec:*); add the missing tools or remove those requirements.
skills/openspec-ff-change/SKILL.md#L4skills/openspec-propose/SKILL.md#L4🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/openspec-ff-change/SKILL.md` at line 4, Update the allowed-tools declarations in skills/openspec-ff-change/SKILL.md at line 4 and skills/openspec-propose/SKILL.md at line 4 to include AskUserQuestion and TodoWrite alongside Bash(openspec:*), keeping both skill allowlists aligned with their workflow requirements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@skills/openspec-ff-change/SKILL.md`:
- Line 4: Update the allowed-tools declarations in
skills/openspec-ff-change/SKILL.md at line 4 and
skills/openspec-propose/SKILL.md at line 4 to include AskUserQuestion and
TodoWrite alongside Bash(openspec:*), keeping both skill allowlists aligned with
their workflow requirements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dce530f9-1f68-46f5-a36d-00788703c816
📒 Files selected for processing (8)
skills/openspec-archive-change/SKILL.mdskills/openspec-continue-change/SKILL.mdskills/openspec-ff-change/SKILL.mdskills/openspec-propose/SKILL.mdskills/openspec-sync-specs/SKILL.mdsrc/core/templates/workflows/archive-change.tssrc/core/templates/workflows/sync-specs.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/core/templates/workflows/sync-specs.ts
- src/core/templates/workflows/archive-change.ts
- skills/openspec-continue-change/SKILL.md
- skills/openspec-archive-change/SKILL.md
- skills/openspec-sync-specs/SKILL.md
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Verified the rebase/regeneration resolves both prior blockers: the static distribution now includes #1360’s store-aware archive/sync paths and #1368’s fresh disk-read guidance, while the exact-set, symlink, and containment guards remain intact with focused coverage. Parity, Windows, Nix, and the full CI gate are green, and the PR is mergeable.
2 participants
What was missing: Multiple requests (#1258) to install OpenSpec skills via skills.sh, which reads static
SKILL.mdfiles from a repo — but OpenSpec generates its skills atopenspec inittime, so none existed in the tree.What it does: Commits the 12 workflow skills under
skills/sonpx skills add Fission-AI/OpenSpecworks. The files are generated from the existing skill templates (pnpm generate:skills), not hand-copied, and a parity test keeps them in sync. The volatilegeneratedByfrontmatter line is stripped so the committed copies stay byte-stable across releases (no per-release regen churn).Proof it works:
skillssh-parity.test.ts+ existing golden-hash tests (skill-templates-parity,skill-generation) pass — 34 tests.… is stale — run pnpm generate:skills.zsh-installer(oh-my-zsh) failures; 1859 others pass.Notes / scope:
openspecCLI and assume anopenspec/project, soopenspec initremains the fuller setup —skills/README.mdstates this.skills/is not in the npmfileslist, so it doesn't bloat the published package (skills.sh reads from GitHub, not npm).Closes #1258
🤖 Generated with Claude Code
Summary by CodeRabbit
generate:skillsregeneration script and normalized skills line endings to LF for consistent output.