feat: install a base skill set by default - #135
Conversation
Rename the `mandatory` frontmatter flag to `base` and turn it on for the
ten skills every metamask-mobile engineer needs, so a fresh clone lands
exactly those from `yarn install` with no flags and nothing to read first.
The flag was already implemented end to end but no skill had ever set it,
and the default domain filter was permissive enough that it never applied.
- `SKILLS_DOMAINS` gains `none`/`all` sentinels; the zero-config default is
now base-only rather than every domain
- `SKILLS_AUTO_UPDATE` defaults on, and the cache refresh moved behind that
gate so opted-out engineers no longer pay a network fetch per install
- suppress git credential prompts and bound every spawn, so enabling the
above cannot hang `yarn install` on a private-source fetch
- reject `base: true` alongside `maturity: experimental`, and warn when a
base description is too thin to be selected
- rewrite six base descriptions that could never have triggered
("General coding guidelines" was 25 characters)
Base set: mobile-testing, coding-guidelines, controller-integration,
ui-development, component-scaffold, performance, pr-guidelines, create-pr,
pr-description, pr-codeowners.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`yarn skills` reads as "give me the skills", so having it install 10 instead of 36 was a surprising regression for a command an engineer typed on purpose. Split the two paths: an automatic install nobody asked for gets the minimum that works, an explicit run still gets every domain. `postinstall` now exports SKILLS_DEFAULT_SCOPE=base when it delegates to sync, and sync branches its *fallback* on that. Everything above the fallback in the resolution order is untouched — a --domain flag, SKILLS_DOMAINS, the picker and a saved selection all still win. That last part is the point: passing `--domain none` from postinstall would have outranked a saved selection, so an engineer who opted into perps would have silently lost it on their next `yarn install`. | path | domains | installed | | --- | --- | --- | | `yarn skills`, no config | all | 36 | | `yarn install`, no config | none | 10 | | `yarn install`, SKILLS_DOMAINS=perps | perps | 12 | | `yarn skills --domain none` | none | 10 | Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NicolasMassart
left a comment
There was a problem hiding this comment.
Blocking on the delegated postinstall path still being able to wait indefinitely.
| const result = spawnSync(bash, delegatedArgs, { | ||
| stdio: options.stdio ?? 'inherit', | ||
| env, | ||
| env: { ...env, ...options.env }, |
There was a problem hiding this comment.
issue (blocking): the delegated sync still has no timeout
run() now bounds its child processes, but delegate() uses spawnSync() directly without a timeout.
This is the subprocess that launches tools/sync, including its git update work, from the automatic postinstall path. So a hung delegated script can still hang yarn install indefinitely despite the PR's “every spawn is capped” guarantee.
Could we apply the same default timeout here as well, while still allowing an explicit override?
There was a problem hiding this comment.
Good catch — fixed in 548bff1.
delegate() now passes a timeout and reports it, with three details worth flagging:
DELEGATE_TIMEOUT_MSis doubleSPAWN_TIMEOUT_MS, not equal.delegate()wraps a script that itself makes several already-capped calls, so with the same budget the outer timer fires first — after the wrapper's own startup cost — and kills a legitimately slow clone from the outside. That drops the engineer to the stale bundled snapshot, which is the exact outcome the existing comment says is worse than waiting.killSignal: 'SIGKILL'rather than the default SIGTERM: the child is Bash, which doesn't reliably forward a term signal to an in-flightgit, so SIGTERM can leave an orphan holding the index lock and break the next install too.- Explicit
ETIMEDOUThandling.spawnSyncreturnsstatus === nullon timeout, soresult.status ?? 1was returning a bare1— indistinguishable from an ordinary failure. It now says what happened and that the bundled snapshot is still in place, which is what makes it reportable rather than merely bounded.
...options is spread after timeout, matching run(), so an explicit override still wins.
Verified: lint clean, 65/65 tests pass, and confirmed empirically that spawnSync with timeout + SIGKILL yields error.code === 'ETIMEDOUT' / status === null, so the new branch is the one that actually fires.
run() bounds its own spawns, but delegate() called spawnSync directly with no timeout. That is the subprocess launching tools/sync — including its git work — from the automatic postinstall path, so a stalled child could still hang `yarn install` indefinitely despite the "every spawn is capped" guarantee. Three details beyond adding the flag: - DELEGATE_TIMEOUT_MS is double SPAWN_TIMEOUT_MS, not equal to it. delegate() wraps a script that itself makes several already-capped calls; with the same budget the outer timer would fire first, after the wrapper's own startup cost, and kill a legitimately slow clone from the outside. That drops the engineer to the stale bundled snapshot — the exact outcome the existing cap is written to avoid. - killSignal is SIGKILL rather than the default SIGTERM. The child is Bash, which does not reliably forward a term signal to an in-flight git, so SIGTERM can leave an orphan holding the index lock and break the next install too. - spawnSync reports status === null on timeout, so `result.status ?? 1` returned a bare 1, indistinguishable from an ordinary failure. It now detects ETIMEDOUT explicitly and says what happened and what state the engineer is left in, which is what makes the failure reportable rather than merely bounded. `...options` is spread after `timeout`, matching run(), so an explicit override still wins. Verified: node --check passes, yarn lint clean, 65/65 tests pass. Confirmed empirically that spawnSync with timeout + SIGKILL yields error.code ETIMEDOUT and status null, so the new branch is the one that fires. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NicolasMassart
left a comment
There was a problem hiding this comment.
Looks good to me.
|
Reviewed in depth — the diagnosis in the PR body is genuinely good (three gaps, one problem), and every number in it checks out against a clone at Blocker: legacy saved "all" selections are silently dropped to base-only. This breaks the PR's own "a saved selection still wins" guarantee — and since pruning is off by default, the other 26 skills stay on disk and silently go stale rather than visibly disappearing. Small fix in load_saved() {
[[ -f "$CONFIG_FILE" ]] || return 0
grep -q '^SKILLS_DOMAINS=' "$CONFIG_FILE" || return 0
local v; v=$(awk -F= '/^SKILLS_DOMAINS=/{sub(/^SKILLS_DOMAINS=/,""); print; exit}' "$CONFIG_FILE")
echo "${v:-all}"
}SSH prompts can still hang Smaller items:
|
What
Turns on a base skill set that installs for every engineer on a fresh clone, with no flags and nothing to read first. Mobile gets 10 skills; everything else stays opt-in.
The
mandatoryfrontmatter flag is renamed tobaseand actually used. It was already implemented end to end —tools/install,tools/sync, the schema, the CLI, even a★marker in the interactive picker — but no skill had ever set it, and the default domain filter was permissive enough that the bypass had nothing to bypass.Why
Three things had to be true together for "base skills by default" to work. Only the flag existed.
SKILLS_DOMAINSmeant all domains, sobasenever appliednone/allsentinelsSKILLS_AUTO_UPDATEwas off by default, sopostinstallrefreshed the cache and installed nothingcoding-guidelineswas literally"General coding guidelines"— 25 charactersThat last one is the subtle one: a skill's description is its entire trigger surface. A 25-character description never fires on the work it governs, while appearing to be covered.
Automatic vs explicit
yarn skillsreads as "give me the skills", so having it install 10 instead of 36 would be a surprising regression for a command someone typed on purpose. The two paths differ:yarn skills, no configyarn install→postinstall, no configyarn installwithSKILLS_DOMAINS=perpsyarn skills --domain nonepostinstallexportsSKILLS_DEFAULT_SCOPE=baseandtools/syncbranches its fallback on that. Everything above the fallback in the resolution order is untouched — a--domainflag,SKILLS_DOMAINS, the picker, and a saved selection all still win.That last property is the point of doing it this way. Passing
--domain nonefrompostinstallwould have outranked a saved selection, so an engineer who opted intoperpswould have silently lost it on their nextyarn install.The base set (10)
mobile-testing·coding-guidelines·controller-integration·ui-development·component-scaffold·performance·pr-guidelines·create-pr·pr-description·pr-codeownerspr-descriptionandpr-codeownersare included becausecreate-prdelegates its main output to them with "if available" phrasing — without them it silently runs in a reduced mode.Always-on cost: ~1,130 tokens per session. The set is deliberately small: when the agent's skill listing overflows its budget, descriptions get truncated starting with the least-invoked skills, so a bloated base set makes every skill harder to trigger.
Safety
Enabling auto-update puts git operations on every engineer's install path, so this also:
GIT_TERMINAL_PROMPT=0, emptyGIT_ASKPASS) in bothrun()and the delegated environment —tools/syncpulls the private Consensys overlay, which could otherwise block on stdin duringyarn installwith no outputbash --versionprobesGuardrails added
base: true+maturity: experimentalis now a lint error — contradictory intent; installing unfinished guidance for everyone is the bad outcomeVerified
SKILLS_DEFAULT_SCOPE=base).claude/skillsand.agents/skillsyarn skillsSKILLS_DOMAINS=perpsSKILLS_AUTO_UPDATE=0/CI=1postinstall→sync→installcoding-guidelines71-byte source stub → 3,990 bytes installednode --testlint-skill-entry.mjsMigration note
Existing engineers are not dropped to 10. Their current installs stay, because pruning is off by default and their next
yarn installonly adds the base set.yarn skills --prune-staleonce gives a clean state.This does reach external contributors: the skills package is a devDependency of public repos, so a drive-by contributor's
yarn installnow clones this repo into.skills-cache/and writes 10 gitignored skill files. Public content, gitignored output, one clone — flagging it rather than letting it be discovered later.Related
Known gaps documented separately: no standardized PR-review skill, platform APIs at 2 of ~11, and five deprecated testing skills that still compete with
mobile-testing.🤖 Generated with Claude Code