fix(skill): skill-publishing — CLI multi-value flags take spaces, not commas - #33
fix(skill): skill-publishing — CLI multi-value flags take spaces, not commas#33wiiiimm (wiiiimm) wants to merge 2 commits into
Conversation
…ma lists, one of them silently Prompted by a sibling session bootstrapping a private skills repo, which hit `-a claude-code,codex` being rejected. Re-ran every invocation here against skills v1.5.23 rather than take the report as-is, which changed two of the conclusions. Verified: - `-s/--skill` and `-a/--agent` are variadic — space-separated or repeated. The report guessed `-s` shared the `-a` comma trap "presumably"; it does not have the same trap, it has a WORSE one. - `-a a,b` errors `Invalid agents: a,b` and then prints a valid-agent list containing both names you just passed — it reads like a tool bug, not a parse error, which is what makes it expensive. - `-s a,b` prints "No matching skills found", installs nothing, and EXITS 0. That is a fail-open: a scripted install with a typo'd separator reports success having done nothing. This is the finding worth having, and it wasn't in the report. - Agent names are `claude-code` (not `claude`) and `codex`. - `--all` = `--skill '*' --agent '*' -y`: ~19 agent dirs plus a NON-HIDDEN top-level `agent/` holding a second full copy of every skill — `git add -A` bait — and `remove --all` leaves the empty dir behind. - Project scope writes `skills-lock.json` (version 1, `computedHash`) at the repo root; global writes `~/.agents/.skill-lock.json` (version 3, `skillFolderHash`). Neither carries absolute paths, so the project lock is safe to commit — which is what makes gitignoring the agent dirs while tracking the lock work. - `experimental_install` / `experimental_sync` exist in `--help`; documented as unverified rather than claimed to work. None of these failure modes appear in `--help`, whose own `-a, --agent <agents>` wording is what invites the comma in the first place. Provenance note updated from "v1.5.x (2026-07)" to the exact version and date. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVBGrKyCoZtJStpVM1Li6L
|
Warning Review limit reachedNext included review available in 44 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 93 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50b3638a97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| Neither contains absolute paths, so the **project lock file is safe to commit** — | ||
| and committing it is how you keep the installed set reproducible while |
There was a problem hiding this comment.
When a project installs a skill from a private repository, committing this lock file to a public repository can disclose the private source identifier: the preceding table confirms that each entry includes source (and the global form includes sourceUrl). The absence of absolute paths therefore does not make the file unconditionally safe to commit; advise inspecting it or excluding/redacting private source metadata before publishing.
AGENTS.md reference: AGENTS.md:L59-L61
Useful? React with 👍 / 👎.
Greptile SummaryThe PR expands the skill-publishing documentation with verified multi-value flag behavior,
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking documentation inconsistency around the unverified lock-file restoration workflow. The changed skill provides concrete CLI guidance, but its claim that a committed lock keeps installations reproducible is not supported by a verified restoration command. Files Needing Attention: skills/skill-publishing/SKILL.md
|
| Filename | Overview |
|---|---|
| skills/skill-publishing/SKILL.md | The CLI guidance is substantially clarified, but the lock-file section promises reproducibility through restore commands whose behavior it simultaneously marks unverified. |
Reviews (1): Last reviewed commit: "fix(skill): skill-publishing — the CLI's..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Neither contains absolute paths, so the **project lock file is safe to commit** — | ||
| and committing it is how you keep the installed set reproducible while | ||
| `.gitignore`-ing the agent directories themselves, so vendored copies never enter | ||
| git. `experimental_install` ("Restore skills from `skills-lock.json`") and | ||
| `experimental_sync` exist for that flow; both are listed in `--help` but their |
There was a problem hiding this comment.
The text recommends committing skills-lock.json to keep installations reproducible, then states that the experimental commands needed to restore that installation are unverified. This leaves readers without a validated way to reconstruct the recorded skill contents after ignoring the installed agent directories.
…open was in my measurement The previous commit claimed `-s a,b` exits 0 — a silent fail-open. It does not. Measured directly, both comma forms exit 1 and fail cleanly: nothing installed, no lock file written. The 0 came from reading `$?` after a pipeline. `npx skills add … | grep …` returns grep's status, not npx's: measured npx=1, grep=0, pipeline $?=0, and `set -o pipefail` correctly yields 1. So I fell into the exact defect AGENTS.md documents — "a pipeline takes the last command's status unless pipefail is set" — while documenting a different tool's traps, and then shipped it as a finding about that tool. Caught by the sibling session that raised the original report, which measured it with a redirect instead of a pipe. Verified here before changing anything. The corrected fact generalises better than the wrong one did: the lesson is not "beware this CLI's exit code", it's "don't read any CLI's status through an unguarded pipe — verify what landed on disk". That now appears as a callout in the section, including how the bad claim got made, since the failure mode is more instructive than the flag syntax it was attached to. Everything else in the previous commit stands: variadic flags, `claude-code` vs `claude`, the `--all` top-level `agent/` copy, and the two lock-file shapes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVBGrKyCoZtJStpVM1Li6L
wiiiimm (wiiiimm)
changed the title
|
Correction to this PR's headline claim, pushed in I wrote that The So I hit the exact defect this repo's AGENTS.md documents — "a pipeline takes the last command's status unless The corrected fact is the more useful one anyway, so it's now a callout in the section rather than a buried line: don't read any CLI's status through an unguarded pipe — check Title updated ("one silently" → "take spaces, not commas"). Everything else stands and was re-verified: variadic Worth noting for the reviewer: this is the second time in this PR's short life that a confident claim about tooling came from a misread exit status rather than the tool. That's an argument for the "verify what landed on disk" rule being the durable takeaway. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f78de15bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| Neither contains absolute paths, so the **project lock file is safe to commit** — | ||
| and committing it is how you keep the installed set reproducible while |
There was a problem hiding this comment.
In a clean checkout where the agent directories are ignored, merely committing skills-lock.json does not establish that the installed set can be reproduced; the only restore mechanisms named here are experimental_install and experimental_sync, whose behavior the next sentence explicitly says is unverified. An agent following this guidance could omit the installed copies assuming the lock can rebuild them, only to have no tested recovery path. Run the exact restore invocation before asserting reproducibility, or describe the lock as recording the installed set instead.
AGENTS.md reference: AGENTS.md:L131-L133
Useful? React with 👍 / 👎.
1 participant
A sibling session bootstrapping a private skills repo hit
-a claude-code,codexbeing rejected and passed along four corrections. Per this repo's own rule — verify the reviewer's suggested mechanism, not just its concern — I re-ran every invocation againstskillsv1.5.23 instead of folding the report in as written. That changed two of the conclusions, including the one that matters most.What the CLI actually does
-s a b/-a claude-code codex-s a -s b/-a claude-code -a codex-a claude-code,codex■ Invalid agents: claude-code,codex— then prints a valid-agent list containing both names-s a,b■ No matching skills found, installs nothing, exits 0The
-scomma form is the finding worth having, and it wasn't in the report — which guessed-sshared the-atrap "presumably". It doesn't; it has a worse one.-aat least fails loudly.-sis a fail-open: a scripted or CI install with a typo'd separator does nothing and reports success. That's the exact defect class AGENTS.md names ("silence is not success"; "an exit code is a hint"), so it belongs in the skill in those terms.Also verified and now documented:
claude-code(notclaude) andcodex. The confusing part is that the rejection message for the comma form lists both names as valid, so it reads like a tool bug rather than a parsing problem.--allwrites more than you'd think — shorthand for--skill '*' --agent '*' -y, so ~19 agent directories, plus a non-hidden top-levelagent/holding a second full copy of every skill next to.agents/and.claude/. That'sgit add -Abait in a repo, andnpx skills remove --allleaves the empty directory behind.skills-lock.json(version 1,computedHash) at the repo root; global writes~/.agents/.skill-lock.json(version 3,skillFolderHash). Neither carries absolute paths, so the project lock is safe to commit — which is precisely what makes "gitignore the agent dirs, track the lock" a working pattern for a repo that wants installed skills usable but not vendored.experimental_install/experimental_syncexist in--help; documented as unverified rather than claimed to work.None of these failure modes are in
--help— whose own-a, --agent <agents>wording is what invites the comma in the first place. The provenance note goes from the vague "v1.5.x (2026-07)" to the exact version and date, since this is the second time that section has drifted.metadata.version1.1.0 → 1.2.0.🤖 Generated with Claude Code
https://claude.ai/code/session_01NVBGrKyCoZtJStpVM1Li6L