ci: ship hooks/, and check a description names the installed command - #99
Open
MajorLift wants to merge 2 commits into
Open
ci: ship hooks/, and check a description names the installed command#99MajorLift wants to merge 2 commits into
hooks/, and check a description names the installed command#99MajorLift wants to merge 2 commits into
Conversation
`hooks/` was absent from the bundle list, so a skill shipping a hook delivered everything except the hook. This is the fourth instance of one root cause — a directory that exists in source and is not in `copy_bundle_dirs` — after `pr-validate`'s hooks, domain `knowledge/`, and `workflows/`. The `BUNDLE_DIRS` drift test covers it now, so a fifth cannot land silently. Separately, the installer prefixes every emitted skill, so a description advertising `/<name>` names a command no operator exposes. The description is the discovery surface, which makes a wrong trigger string a selection failure rather than a typo. The check requires the `mms-` form. Its first version flagged `@metamask/gator-cli` — a scoped package, not a slash command. A negative lookbehind now excludes `@scope/name` and path-like forms, and that case is a test rather than a note.
6 tasks
Stacked on #99, which is what makes `hooks/` reach a consumer at all. **Base is #99's branch, not `main`** — review that one first. ## The gap Copying a hook does not activate it. Claude Code runs one only once it is registered in `settings.json`, and the path to register is **absolute** — different per machine, per consumer repo, and per `mms-` prefixed skill directory. So the setup reference could only ever say: ``` python3 /absolute/path/to/evidence/hooks/pr-evidence-gate.py ``` and leave the reader to work out what that is. Meanwhile `evidence` cites the hook twice in its body as its enforcement mechanism, so someone installing it reasonably assumes the gate is live. It isn't. ## Two surfaces, one output **At install** — when any installed skill ships a hook, `tools/install` prints the registration with every path resolved against the actual install: ``` Note: 1 skill(s) ship a hook. Copying the file does not activate it — Claude Code runs a hook only once it is registered in settings.json. Add this to ~/.claude/settings.json (or <target>/.claude/settings.json): { "hooks": { "PreToolUse": [ { "matcher": "Bash", "hooks": [ { "type": "command", "command": "python3 <target>/.claude/skills/mms-gatekeeper/hooks/evidence-gate.py" } ] } ] } } ``` **On demand** — `metamask-skills hooks [--target <path>]` prints the same thing, for anyone who scrolled past it or is re-registering later. Says *"No installed skill ships a hook"* rather than printing empty JSON, and exits non-zero only when the target has no installed skills at all. ## What it deliberately does not do **Neither surface writes to `settings.json`.** Editing a user's operator config is a materially larger permission than "copy files into the repo you pointed me at", and it should be decided deliberately rather than inherited as a side effect of shipping one hook. This is also the only hook in the corpus — a sample of one is thin evidence for automating a write to `$HOME`. ## Test plan - [x] `yarn test` — 69 pass / 0 fail across three files - [x] Fixture skill with `hooks/evidence-gate.py`: file delivered, registration printed, path resolved - [x] Printed registration **parses as JSON** — asserted by `JSON.parse`, matcher and command checked - [x] `metamask-skills hooks` emits the same registration - [x] Target with skills but no hooks → message, exit 0 - [x] Target with no installed skills → warning, exit 1 ## Note An earlier version emitted a trailing comma and told the reader to delete it. Handing someone JSON that doesn't parse is worse than handing them none, so the entries are joined properly and the output is valid as printed.
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
1 participant
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.
Two follow-ups to #47, which merged before these landed.
hooks/never shippedcopy_bundle_dirscoveredreferences scripts assets adapters workflows— nothooks. A skill shipping a hook delivered everything except the hook.This is the fourth instance of one root cause: a directory that exists in source and is not in the copy list. The earlier three were
evidence'shooks/, domainknowledge/, andworkflows/— the last of which was 14 files referenced 22 times and never delivered. TheBUNDLE_DIRSdrift test from #47 now covershookstoo, so a fifth cannot land silently.Verified end to end: a fixture skill with
hooks/gate.pynow delivers it.A description that names a command nobody has
The installer prefixes every emitted skill, so a description advertising
/<name>names a command no operator exposes — the reader getsmms-<name>. Since the description is the discovery surface, a wrong trigger string is a selection failure rather than a typo.The check requires the prefixed form.
Its first version was wrong, and the fix is the interesting part: it flagged
@metamask/gator-cli, reading a scoped package name as a slash command. A negative lookbehind now excludes@scope/nameand path-like forms, and that exact case is a test rather than a comment.mainreports 0 errors across 47 skills with the rule active, so this gates new breakage rather than landing red.Test plan
yarn test— 64 pass / 0 fail across three filesnode .github/scripts/lint-skill-entry.mjs— 0 errors, 47 skillshooks/gate.py→ delivered tomms-h/hooks/gate.py/demoin a description → fails, naming both forms/mms-demo→ passes@metamask/gator-cliin a description → passes, not treated as a commandNote
Branched fresh from
mainrather than pushed onto #47's ref. That branch was deleted on merge, and my local copy predated the testing-domain restructure that merged alongside it — reusing it would have reverted that work.