diff --git a/.changeset/sharp-wolves-wander.md b/.changeset/sharp-wolves-wander.md new file mode 100644 index 000000000..791663368 --- /dev/null +++ b/.changeset/sharp-wolves-wander.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3404 +--- +**`/gsd-sync-skills` now refuses cross-runtime skill sync** — skill content and directory layout are runtime-specific (the installer applies per-runtime converters/adapter headers/brand swaps/layout rules), and two runtimes alias another runtime's skills root, so a verbatim cross-runtime copy silently corrupted destination skills and could overwrite a runtime the user never named. sync now refuses any `--to` that differs from `--from` and points at the installer, keeping identity sync (`--from` == `--to`) as a no-op. (#3025) diff --git a/gsd-core/workflows/sync-skills.md b/gsd-core/workflows/sync-skills.md index 0f9fff602..ab6c0da07 100644 --- a/gsd-core/workflows/sync-skills.md +++ b/gsd-core/workflows/sync-skills.md @@ -11,7 +11,7 @@ Sync managed `gsd-*` skill directories from one canonical runtime's skills root | Flag | Required | Default | Description | |------|----------|---------|-------------| | `--from ` | Yes | *(none)* | Source runtime — the canonical runtime to copy from | -| `--to ` | Yes | *(none)* | Destination runtime or `all` supported runtimes | +| `--to ` | Yes | *(none)* | Destination runtime or `all` supported runtimes. **Must equal `--from`** — cross-runtime sync is refused (#3025: skill content/layout is runtime-specific and produced by the installer's per-runtime converters; use the installer for a different runtime). | | `--dry-run` | No | *on by default* | Preview changes without writing anything | | `--apply` | No | *off* | Execute the diff (overrides dry-run) | @@ -50,6 +50,61 @@ fi - If `--from` is missing or unrecognized: print error and exit - If `--to` is missing or unrecognized: print error and exit - If `--from` == `--to` (single destination): print `[no-op: source and destination are the same runtime]` and exit +- If any `--to` destination differs from `--from` (cross-runtime): REFUSE with the installer pointer below and exit. sync only supports identity sync — see the guard. +- If `--from` or any `--to` value is not a runtime-id shape (`^[a-z0-9][a-z0-9-]*$`): REFUSE and exit — runtime ids are lowercase alphanumeric (+ hyphen); this rejects shell metacharacters before any interpolation (see security guard). + +**#3025 — Runtime-id shape validation (security: run BEFORE any interpolation):** + +`--from`/`--to` are interpolated into later `echo`/heredoc/`[[ ]]` contexts. Reject any value that is not a runtime-id shape BEFORE it reaches them, so a hostile value (e.g. `--to '$(cmd)'`, captured wholesale by the parser) cannot execute via command substitution in an error message. + +```bash +# #3025 (security): runtime ids are lowercase alphanumeric (+ hyphen). Reject +# anything else BEFORE any echo/heredoc/[[ ]] so a hostile --from/--to value +# cannot execute via command substitution in a later error message. +is_runtime_id() { [[ "$1" =~ ^[a-z0-9][a-z0-9-]*$ ]]; } +if ! is_runtime_id "$FROM_RUNTIME"; then + echo "error: invalid --from runtime id (not lowercase alphanumeric): '$FROM_RUNTIME'" >&2 + exit 1 +fi +for DEST in "${TO_RUNTIMES[@]}"; do + if ! is_runtime_id "$DEST"; then + echo "error: invalid --to runtime id (not lowercase alphanumeric): '$DEST'" >&2 + exit 1 + fi +done +``` + +**#3025 — Cross-runtime refuse guard (run BEFORE Step 2 resolution / Step 5 copy):** + +Skill content and directory layout are runtime-specific. The installer applies per-runtime +converters, adapter headers, brand swaps, and layout rules at install time, and two runtimes +(`grok`, `gemini`) resolve to ANOTHER runtime's skills root. A verbatim copy from one runtime's +skills root therefore produces content the installer would never have written for the destination, +and can damage a runtime the user never named. Every cross-runtime pair is unsafe (content and/or +layout and/or aliasing); only identity (`--from` == `--to`) is safe. Refuse cross-runtime and point +the user at the installer — the only path that produces correctly converted skills. + +```bash +# #3025: refuse cross-runtime skill sync before any resolution or copy. +for DEST in "${TO_RUNTIMES[@]}"; do + if [[ "$DEST" != "$FROM_RUNTIME" ]]; then + cat >&2 < + (grok and gemini have no dedicated installer flag — they alias the codex and + claude skills roots respectively, which is itself why sync refuses them.) + sync only supports identity sync, where --from and --to are the same runtime. +EOF + exit 1 + fi +done +``` --- @@ -175,12 +230,12 @@ DEST_ROOT=$(gsd_run query skills-root "$DEST_RUNTIME" --raw) mkdir -p "$DEST_ROOT" -# #3025: verbatim cp -r copies the SOURCE runtime's converted skill form, which -# corrupts skills for destinations that need a different conversion (e.g. Claude -# SKILL.md → Codex TOML agent). Until the installer exposes a per-skill conversion -# CLI, sync is limited to runtime pairs that share the same skill format. -# Run `gsd install -- --local` to get correctly converted skills -# for a destination that uses a different format. +# #3025: cross-runtime sync is refused in Step 1's guard (skill content/layout is +# runtime-specific; a verbatim copy corrupts destinations and can alias another +# runtime's root). This loop is therefore reached only for IDENTITY sync, where +# every skill is SKIP (source == destination) and the create/update lists are +# empty. If per-runtime conversion is ever wired in, this is where it would go; +# until then the cp -r must never run for a destination != source. for SKILL in $CREATE_LIST $UPDATE_LIST; do rm -rf "$DEST_ROOT/$SKILL" @@ -215,6 +270,6 @@ Sync complete: skills synced to runtime(s). ## Limitations -- Sync copies files verbatim and does not apply runtime-specific content transformations. Use the GSD installer directly for runtimes that require format conversion. +- Sync copies files verbatim and does not apply runtime-specific content transformations. **Cross-runtime sync is refused** (#3025): skill content and layout are runtime-specific, and some runtimes alias another runtime's skills root, so a verbatim cross-runtime copy corrupts the destination (and can damage a runtime you did not name). Only identity sync (`--from` == `--to`) is supported. To install skills for a different runtime, run the GSD installer for that runtime (`npx -y @opengsd/gsd-core@latest --global --`). - Cross-project skills (`.agents/skills/`) are out of scope — this command only touches global runtime skills roots. - Bidirectional sync is not supported. Choose one canonical source with `--from`. diff --git a/scripts/lint-allow-test-rule-refs.ceiling.json b/scripts/lint-allow-test-rule-refs.ceiling.json index cb1d9734a..9e97dd204 100644 --- a/scripts/lint-allow-test-rule-refs.ceiling.json +++ b/scripts/lint-allow-test-rule-refs.ceiling.json @@ -1,4 +1,4 @@ { - "maxFiles": 297, + "maxFiles": 298, "grace": 3 } diff --git a/tests/emitted-drift-acks/3024-sync-skills-runtime-launcher-preamble.json b/tests/emitted-drift-acks/3024-sync-skills-runtime-launcher-preamble.json deleted file mode 100644 index 0c2ab1bab..000000000 --- a/tests/emitted-drift-acks/3024-sync-skills-runtime-launcher-preamble.json +++ /dev/null @@ -1,8 +0,0 @@ -{ - "version": 1, - "paths": { - "sync-skills.md": { - "reason": "#3024: Step 2 now resolves skills roots via `gsd_run query skills-root ` instead of shelling out to the unshipped `install.js --skills-root`. Because the workflow now calls `gsd_run`, it must carry the canonical runtime-launcher preamble (`node scripts/sync-runtime-launcher.cjs`, per runtime-launcher-parity.test.cjs) so `gsd_run` resolves on every non-Claude runtime, not just Claude Code — without it the fix would be dead exactly where the original #3024 bug bit. The single-line preamble (~4.4KB, one bash line with a resolver arm per supported runtime home) accounts for essentially all of the growth (6,125 -> 10,928 bytes); the remainder is the reworded Step 2 lead-in and error-guidance text that no longer references the unshipped install.js entry point (defect C). Follow-up hardening (still #3024): neither `gsd_run query skills-root` call in Step 2 checked its exit status or for an empty result, so an unregistered/invalid runtime id silently produced an empty `$DEST_ROOT`, which Step 5 then fed into `rm -rf \"$DEST_ROOT/$SKILL\"` (an absolute-root deletion). Step 2's bash now checks exit status + non-empty for both the source and every destination resolution and aborts with a named error; a matching prose guard covers destination-resolution failure alongside the pre-existing source-not-found guard; Step 5 adds a one-line non-empty/absolute check on both `$SRC_SKILLS_ROOT` and `$DEST_ROOT` before any `rm -rf`/`cp -r`. (Separately noted but NOT fixed here, as it requires restructuring Steps 3-5 rather than a Step 2/Step 5 guard: `DEST_SKILLS_ROOTS` is populated as a keyed map but is never read back into `$DEST_ROOT` anywhere in the file, and on bash 3.2 — the macOS system `/bin/bash` — assigning to it without `declare -A` silently collapses every destination's resolved root into index `[0]`; `declare -A` itself is bash4+-only and errors on 3.2, so this is not a one-line fix.) This accounts for the additional growth (10,928 -> 12,166 bytes). Follow-up (review BLOCKER on this fix): the \"Supported runtime names\" prose list and the `--to all` TO_RUNTIMES expansion both hand-copied a runtime-id list that had drifted from the capability registry — `grok` and `gemini` were listed but are not registered runtime ids, and this branch's own-property validation gate in `routeSkillsRoot` now correctly rejects them, so `--to all` (and any explicit `--to grok`/`--to gemini`) aborted. Both lists are corrected to the registry's 19 runtime ids minus `vscode` (deliberately excluded and named in prose: `installSurface: 'none'`, `getGlobalSkillsBase('vscode')` returns `null`, so a sync to/from it cannot succeed) — 18 ids. A parity test (`tests/install.test.cjs`) now fails if the documented list and the registry disagree in either direction. This accounts for the growth (12,166 -> 12,510 bytes). Follow-up (this fix, the previously-deferred `DEST_SKILLS_ROOTS` defect noted above): that associative-array assignment is dropped entirely — it was never read back anywhere in the file and, on bash 3.2, silently collapsed every destination's resolved root into index `[0]`. Step 2's TO_RUNTIMES loop is kept for its eager per-destination validation (a bad runtime id anywhere in a multi-destination `--to` still aborts before any destination is touched) but no longer stores the resolved value. Steps 3 and 5 each now bind `DEST_ROOT` themselves at the top of their own bash block via `DEST_ROOT=$(gsd_run query skills-root \"$DEST_RUNTIME\" --raw)`, so every use of `$DEST_ROOT` is unambiguously scoped to the destination currently being processed in that construct, with no shared array and no bash4+ syntax. This accounts for the final growth (12,510 -> 13,480 bytes). Follow-up (delta-review BLOCKER: the previous fix's `grok`/`gemini` removal was itself wrong for `grok`): `grok` has a live, dedicated `~/.agents`-layout resolution branch in `getGlobalConfigDir` (`src/runtime-homes.cts`'s `LEGACY_NON_REGISTRY_RUNTIME_IDS`) predating the capability registry, and was documented pre-fix — removing it silently broke working, documented `--skills-root`/`sync-skills` support. `grok` is restored to both the \"Supported runtime names\" prose list and the `--to all` TO_RUNTIMES expansion, with a clause explaining why it is included despite being absent from the registry; `gemini` stays excluded (it has no dedicated branch and falls through to claude's skills root — the wrong-runtime bug this PR exists to fix). Separately, Step 3 (`ls`-based diff computation) re-resolved `DEST_ROOT` with no exit-status/empty guard, contradicting the file's own stated guarantee (\"never proceed to Step 3 or Step 5 with an empty or unresolved root\") even though Step 5 already carried that guard; Step 3 now carries the identical exit-status + non-empty check immediately after its `DEST_ROOT` resolution. This accounts for the final growth (13,480 -> 13,842 bytes)." - } - } -} diff --git a/tests/emitted-drift-acks/3025-sync-skills-refuse-cross-runtime.json b/tests/emitted-drift-acks/3025-sync-skills-refuse-cross-runtime.json new file mode 100644 index 000000000..4bf231c9e --- /dev/null +++ b/tests/emitted-drift-acks/3025-sync-skills-refuse-cross-runtime.json @@ -0,0 +1,8 @@ +{ + "version": 1, + "paths": { + "sync-skills.md": { + "reason": "#3025 (option b, user decision): sync-skills refused cross-runtime sync. Skill content/layout is runtime-specific (the installer applies per-runtime converters/adapter headers/brand swaps/layout rules) and grok/gemini alias another runtime's skills root, so a verbatim cross-runtime cp -r corrupted destination skills and could damage a runtime the user never named; #3024 (closed) un-masked it. This PR adds: (1) a Step 1 runtime-id SHAPE validation guard (^[a-z0-9][a-z0-9-]*$) that rejects shell-metachar --from/--to values before any echo/heredoc/[[ ]] interpolation, hardening a command-substitution injection vector in the new (and pre-existing) error messages; (2) a FUNCTIONAL Step 1 cross-runtime refuse guard (a bash loop over TO_RUNTIMES that exits 1 with an installer pointer when any destination != FROM_RUNTIME), placed before Step 2 resolution and Step 5's rm -rf/cp -r so cross-runtime can never reach the copy; identity sync (--from == --to) stays a no-op. Growth is: the shape-validation guard (is_runtime_id + loop), the cross-runtime refuse guard loop + cat< { + const text = readWorkflow(); + + test('Step 1 has a functional guard that exits non-zero for any cross-runtime destination', () => { + // The guard must compare each destination to the source runtime and exit 1 on mismatch. + assert.match(text, /!=\s+"\$FROM_RUNTIME"/, 'guard must compare each destination != FROM_RUNTIME'); + // The exit 1 must be INSIDE the guard loop, not one of the unrelated exit 1s in + // Steps 2/3/5 — otherwise a mutant that drops only the guard's exit survives. + const loopStart = text.indexOf('for DEST in "${TO_RUNTIMES[@]}"'); + const loopEnd = text.indexOf('done', loopStart); + assert.notEqual(loopStart, -1, 'guard loop must exist'); + assert.ok(loopEnd > loopStart, 'guard loop must close'); + assert.ok( + /exit 1/.test(text.slice(loopStart, loopEnd)), + 'the guard loop itself must exit non-zero (not an unrelated exit 1 elsewhere)', + ); + }); + + test('the refusal points the user at the installer (actionable, not a bare rejection)', () => { + // Hyrum's Law: the narrowed vocabulary is a visible contract change; the error must + // hand the user a command that produces correctly converted skills. The pointer is + // generic (`--`, not `--$DEST`) because grok/gemini have no dedicated flag. + assert.match(text, /cross-runtime skill sync is not supported/, 'names the unsupported operation'); + assert.match(text, /npx -y @opengsd\/gsd-core@latest --global --/, 'prints the installer command'); + assert.match(text, /\$DEST/, 'names the refused destination runtime'); + assert.match(text, /grok and gemini have no dedicated installer flag/, 'accurately notes grok/gemini aliasing rather than printing a wrong --grok/--gemini flag'); + }); + + test('the guard runs BEFORE Step 5\'s verbatim cp -r copy (cross-runtime can never reach the copy)', () => { + const guardIdx = text.indexOf('!= "$FROM_RUNTIME"'); + const copyIdx = text.indexOf('cp -r "$SRC_SKILLS_ROOT/$SKILL"'); + assert.notEqual(guardIdx, -1, 'guard must exist'); + assert.notEqual(copyIdx, -1, 'Step 5 copy loop must exist'); + assert.ok( + guardIdx < copyIdx, + `guard (idx ${guardIdx}) must precede the cp -r copy (idx ${copyIdx}) so a cross-runtime destination is refused before any filesystem write`, + ); + }); + + test('--to all is cross-runtime by definition when --from is set, so it is refused too', () => { + // The guard iterates TO_RUNTIMES; `all` expands to runtimes that include some != FROM_RUNTIME. + assert.match(text, /for DEST in "\$\{TO_RUNTIMES\[@\]\}"/, 'guard iterates the destination set'); + assert.match(text, /--to all/, 'the all expansion is still part of the interface'); + }); + + test('identity sync (--from == --to) remains a supported no-op and is NOT refused', () => { + // The guard condition is `!=`, so identity passes; the pre-existing no-op contract stays. + const noOpIdx = text.indexOf('[no-op: source and destination are the same runtime]'); + assert.notEqual(noOpIdx, -1, 'identity no-op message must remain'); + assert.match(text, /If .--from. == .--to./, 'identity handling retained in validation'); + }); + + test('#3025 security: --from/--to are shape-validated before any interpolation (no command-substitution injection)', () => { + // A hostile value like --to '$(cmd)' must be rejected as not-a-runtime-id before it + // reaches an echo/heredoc/[[ ]], where unquoted interpolation would execute it. + assert.match(text, /is_runtime_id\(\)/, 'a runtime-id shape predicate must exist'); + assert.match(text, /\^\[a-z0-9\]\[a-z0-9-\]\*\$/, 'predicate must require lowercase-alphanumeric shape'); + const shapeIdx = text.indexOf('is_runtime_id()'); + // Shape validation must run BEFORE the cross-runtime refuse guard and before Step 2. + const xruntimeIdx = text.indexOf('#3025: refuse cross-runtime skill sync'); + assert.ok(shapeIdx !== -1 && xruntimeIdx !== -1 && shapeIdx < xruntimeIdx, 'shape validation must precede the cross-runtime guard'); + }); +});