Skip to content

fix: address skills add review feedback#34

Merged
SoonIter merged 3 commits into
mainfrom
codex/address-pr33-comments
Jun 5, 2026
Merged

fix: address skills add review feedback#34
SoonIter merged 3 commits into
mainfrom
codex/address-pr33-comments

Conversation

@SoonIter

Copy link
Copy Markdown
Owner

Summary

This PR follows up on #33 review feedback by tightening add compatibility and safety: direct local:* add now requires an explicit skill name, --list stays read-only before write-context validation, ambiguous tree URLs require an explicit ref, and pinned full Git commits skip remote ref resolution. It also protects adopted local skills by clearing stale managed markers, removes the root skills-lock.yaml, updates the root manifest to github: pins, and fixes the called-out docs/homepage rendering issues.

Related Links

#33

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Tested with pnpm test, pnpm build, pnpm build:website, and pnpm check.

Copilot AI review requested due to automatic review settings May 21, 2026 04:27
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented May 21, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
skills-package-manager 25c467d Commit Preview URL

Branch Preview URL
May 21 2026, 07:37 AM

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR follows up on prior review feedback to tighten add/install safety and compatibility (especially for local:* and tree URL parsing), improve install fast paths for pinned git commits, and clean up docs/website rendering and example quoting while removing the root skills-lock.yaml.

Changes:

  • Hardened add parsing/behavior: local:* requires an explicit skill name, --list stays read-only before write-context validation, and ambiguous tree URLs require an explicit ref.
  • Improved install behavior: clear stale managed markers when adopting local skills, and short-circuit remote ref resolution for fully pinned git commits.
  • Repo/doc cleanup: remove root lockfile, update manifest pins to github: format, and fix docs/website rendering/examples.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
website/theme/components/HomePage/index.tsx Adds a “manifest” feature icon and minor JSX formatting cleanup.
website/docs/api/commands.mdx Quotes a specifier example to avoid shell parsing issues.
skills.json Updates schema version and switches GitHub specs to github: pins.
skills-lock.yaml Removes the root lockfile from the repo.
README.md Fixes inline code rendering in the HTML header block.
packages/skills-package-manager/test/install.test.ts Adds coverage for local marker adoption and pinned-commit fast path.
packages/skills-package-manager/test/add.test.ts Adds coverage for ambiguous tree URL rejection, list read-only behavior, and local:* requirements.
packages/skills-package-manager/src/specifiers/normalizeSpecifier.ts Threads skillName through normalization for local:*/root-path inference.
packages/skills-package-manager/src/resolvers/git.ts Skips remote ref resolution when the ref is already a full 40-char commit SHA.
packages/skills-package-manager/src/pipeline/types.ts Removes an unused type import.
packages/skills-package-manager/src/pipeline/index.ts Import ordering adjustments (no behavioral change indicated in diff).
packages/skills-package-manager/src/pipeline/fetchQueue.ts Clears stale managed markers when local skills are considered up-to-date.
packages/skills-package-manager/src/install/localSkills.ts Import ordering/type cleanup.
packages/skills-package-manager/src/index.ts Reorders/exports ResolvedSkillsPlan and installStageHooks.
packages/skills-package-manager/src/fetchers/local.ts Adds clearLocalSkillMarker and clears the marker during local fetch.
packages/skills-package-manager/src/config/resolveSkillsPlan.ts Formatting-only refactor of resolveSkillEntry callsite.
packages/skills-package-manager/src/commands/update.ts Import ordering change.
packages/skills-package-manager/src/commands/patchCommit.ts Collapses createBasePlan signature formatting.
packages/skills-package-manager/src/commands/patch.ts Collapses createBasePlan signature formatting.
packages/skills-package-manager/src/commands/add.ts Implements tightened --list flow, local:* explicit naming, and tree URL ambiguity handling.
packages/skills-package-manager/README.md Quotes specifier examples and updates local:* usage docs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/skills-package-manager/src/commands/add.ts Outdated
Comment thread packages/skills-package-manager/src/commands/add.ts Outdated
@SoonIter SoonIter merged commit 60d29ae into main Jun 5, 2026
4 checks passed
@SoonIter SoonIter deleted the codex/address-pr33-comments branch June 5, 2026 08:34
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