feat: give skill pull explicit location flags and stricter parsing - #226
Merged
Conversation
`--dir` is an exact path (`<dir>/<name>`), while the interactive picker appends `.claude/skills`. The non-interactive path — the one `--dir` exists for — therefore had no way to reach the layout agents actually read, and `skill pull foo --dir .` landed in `./foo` with nothing saying so. Add `--here` / `--global`, which route through `skillsDirFor` and reproduce the picker's two choices. `--dir` keeps its exact-path meaning as the escape hatch; combining it with either, or `--here` with `--global`, is an error. The usage string moves to an exported `PULL_USAGE` — the dispatcher and the runner each had their own copy. `parsePullArgs` becomes a single pass with an allowlist: an unrecognized `-`-prefixed token is now `Unknown flag: --forse` rather than silently dropped, which used to look like a successful run that just didn't do what was asked. The `--dir` value is consumed by that same pass, so it can no longer be re-read as a flag or as the target. Drop the `-f` alias for `--force`, for the reason `restore --force` has none: `-f` on `login`/`upload`/`doctor` forces a fresh login and costs nothing, while here the identical keystroke is an `rm -rf` of a skill directory that may hold local edits. Finally, both skill apps now ask about the keyboard before prompting. The dispatcher's hand-rolled TTY checks are replaced by `skillPromptUsable()`, whose keyboard half is `rawModeSupported()` (lib/tty.ts states Ink's gate once, so the two can't drift) and whose `stdout.isTTY` half stays as the separate scripting heuristic it always was — dropping it would pipe a rendered Ink frame into `… | tee log`. `SkillPullApp`/`SkillPushApp` gate `useInput` on `useCanType()` and bail with an explanation if their input phase is ever reached without a keyboard: unlike an ungated `useInput` that would be a silent hang, and for push, silence must never be read as consent to upload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…detail The rationale for `restore --force` having no `-f` alias applies verbatim to `skill pull --force`, which rm -rf's a skill directory that may hold local edits. State it once as a rule about destructive flags, name the second instance, and point at the allowlist that enforces it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
skill pull's two entry points disagreed about layout. The interactive picker installs to<root>/.claude/skills/<name>;--dir Xinstalls toX/<name>verbatim. So the non-interactive path — the one--direxists for — had no way to reach the layout agents actually read, andskill pull foo --dir .quietly landed in./foowith neitherhelp.tsnor the usage string mentioning it.What changed
Location flags. New
--here/--globalroute throughskillsDirForand reproduce the picker's two choices:--dirkeeps its exact-path meaning as the escape hatch. Combining it with either, or--herewith--global, is a parse error. The usage string moved to an exportedPULL_USAGEsince the dispatcher and the runner each had their own copy.Stricter parsing.
parsePullArgsis now a single pass over an allowlist. An unrecognized--prefixed token returnsUnknown flag: --forseinstead of being dropped — silently ignoring it looked like a successful run that just didn't do what was asked. The--dirvalue is consumed by that same pass, so it can't be re-read as a flag or as the target; the old code scanned twice and could disagree with itself about which token was the value.No
-falias, for the reasonrestore --forcehas none:-fonlogin/upload/doctorforces a fresh login and costs nothing, while here the identical keystroke is anrm -rfof a skill directory that may hold local edits.--forceis unchanged.AGENTS.mdnow states this as a general rule about destructive flags rather than arestoredetail, and names both instances.Raw-mode gating. The dispatcher's two hand-rolled
Boolean(process.stdin.isTTY && process.stdout.isTTY)checks becomeskillPromptUsable(). It keeps both conditions but sources them separately, because they answer different questions:rawModeSupported()—lib/tty.tsstates Ink's gate once so the two can't drift;stdout.isTTYstays and is deliberately not folded intotty.ts. It's a scripting heuristic, not a raw-mode one — dropping it would sendskill pull … | tee loginto the Ink app and pipe a rendered ANSI frame into the file.SkillPullAppandSkillPushAppnow gateuseInputonuseCanType()and bail with an explanation naming the escape hatch (--here/--global/--dir,--json) if their input phase is reached without a keyboard. Note the failure mode differs from the onetty.tswas written for: withuseInputgated but no effect there's no throw, just a prompt that hangs forever. For push it refuses rather than proceeds — that confirm is the last thing between the user and an upload, so silence must not be read as consent.Testing
pnpm fix,pnpm typecheck,pnpm test(1262 passed),pnpm build+ smoke-run all pass.New coverage: both layouts end-to-end;
--globalasserted through the pure helpers so no test writes to a real$HOME; unknown-flag rejection; both conflict cases;--dirvalue consumption;-frejected with the pre-existing directory verified intact, not just a non-zero exit; and a no-raw-mode render of each app using the established flip-isTTY-and-rerender pattern fromLogin.test.tsx.One existing assertion (
/pass --dir/i) was updated to the new message rather than left matching a string the code no longer emits.Also verified against the built bundle under piped stdin —
skill pull,skill pushand the no-location case all degrade to their runners with a plain sentence, no React stack and no hang.Not included
skill pull foo@1.2).GET /api/v1/skills/<target>accepts a name or id only, so this needs server work first; today the@1.2is sent as part of the name and comes back asSkill "foo@1.2" not found or not public.🤖 Generated with Claude Code