* fix(#3025): refuse cross-runtime skill sync in sync-skills 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 grok/gemini resolve to ANOTHER runtime's skills root. A verbatim cross-runtime cp -r therefore produces content the installer would never have written for the destination, and can damage a runtime the user never named. #3024 (closed) un-masked this, making the corruption live. Fix (option b, user decision): add a functional Step 1 guard that refuses any --to != --from with an actionable installer pointer, before any resolution or copy. Identity sync (--from == --to) remains a no-op. The non-functional Step 5 comment is replaced; Arguments/Limitations updated. Regression: tests/sync-skills-cross-runtime-refuse.test.cjs (source-text- is-the-product) asserts the guard exits non-zero for cross-runtime, points at the installer, precedes the cp -r copy, and preserves identity. * docs(#3025): backfill changeset PR number (#3404) --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/sharp-wolves-wander.md
Normal file
5
.changeset/sharp-wolves-wander.md
Normal file
@@ -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)
|
||||
@@ -11,7 +11,7 @@ Sync managed `gsd-*` skill directories from one canonical runtime's skills root
|
||||
| Flag | Required | Default | Description |
|
||||
|------|----------|---------|-------------|
|
||||
| `--from <runtime>` | Yes | *(none)* | Source runtime — the canonical runtime to copy from |
|
||||
| `--to <runtime\|all>` | Yes | *(none)* | Destination runtime or `all` supported runtimes |
|
||||
| `--to <runtime\|all>` | 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 <<EOF
|
||||
error: cross-runtime skill sync is not supported (--from $FROM_RUNTIME --to $DEST).
|
||||
Skill content and directory layout are runtime-specific: the installer applies
|
||||
per-runtime converters, adapter headers, brand swaps, and layout rules that a
|
||||
verbatim copy cannot reproduce, and some runtimes share another runtime's skills
|
||||
root — so a cross-runtime sync can damage a runtime you did not name.
|
||||
To install correctly-converted skills for the '$DEST' runtime, run the GSD
|
||||
installer for that runtime (not sync):
|
||||
npx -y @opengsd/gsd-core@latest --global --<runtime>
|
||||
(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 --<dest-runtime> --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: <N> skills synced to <M> 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 --<runtime>`).
|
||||
- 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`.
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
{
|
||||
"maxFiles": 297,
|
||||
"maxFiles": 298,
|
||||
"grace": 3
|
||||
}
|
||||
|
||||
@@ -1,8 +0,0 @@
|
||||
{
|
||||
"version": 1,
|
||||
"paths": {
|
||||
"sync-skills.md": {
|
||||
"reason": "#3024: Step 2 now resolves skills roots via `gsd_run query skills-root <runtime>` 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)."
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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<<EOF heredoc error block, the new validation bullets, the rewritten Step 5 comment (the prior non-functional #3025 comment is replaced), and the strengthened Limitations bullet. No code module or path-resolution changes (out of scope per triage)."
|
||||
}
|
||||
}
|
||||
}
|
||||
94
tests/sync-skills-cross-runtime-refuse.test.cjs
Normal file
94
tests/sync-skills-cross-runtime-refuse.test.cjs
Normal file
@@ -0,0 +1,94 @@
|
||||
// allow-test-rule: source-text-is-the-product (see #3025)
|
||||
// sync-skills.md is a shipped workflow whose deployed text IS what the runtime
|
||||
// loads — asserting its content tests the deployed contract (per CONTRIBUTING's
|
||||
// source-text-is-the-product exemption; not a compiled-.cjs source-grep).
|
||||
|
||||
/**
|
||||
* #3025 — sync-skills must refuse cross-runtime sync.
|
||||
*
|
||||
* Skill content/layout is runtime-specific (the installer applies per-runtime
|
||||
* converters, adapter headers, brand swaps, layout rules), and `grok`/`gemini`
|
||||
* resolve to ANOTHER runtime's skills root. A verbatim `cp -r` from one runtime
|
||||
* corrupts every other destination and can damage a runtime the user never named.
|
||||
*
|
||||
* Chosen fix (user decision, 2026-08-13): option (b) — refuse unsafe (cross-
|
||||
* runtime) destinations with an actionable installer pointer; keep identity sync
|
||||
* as a no-op. See gsd-core/workflows/sync-skills.md Step 1 guard.
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
|
||||
const WORKFLOW = path.join(__dirname, '..', 'gsd-core', 'workflows', 'sync-skills.md');
|
||||
|
||||
function readWorkflow() {
|
||||
return fs.readFileSync(WORKFLOW, 'utf8');
|
||||
}
|
||||
|
||||
describe('#3025: sync-skills refuses cross-runtime skill sync', () => {
|
||||
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 (`--<runtime>`, 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 --<runtime>/, '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');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user