diff --git a/.changeset/curious-newts-sprint.md b/.changeset/curious-newts-sprint.md new file mode 100644 index 000000000..4f8275043 --- /dev/null +++ b/.changeset/curious-newts-sprint.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2894 +--- +**Releases no longer fail their own emitted-parity gate** — cutting any release ran the differential attribution check against a baseline built at a different version, so the install-time hook version stamp made all 364 emitted hook paths look like unexplained drift and every `finalize`/`rc` run hard-failed before tagging or publishing. (#2891) diff --git a/tests/emitted-attribution.test.cjs b/tests/emitted-attribution.test.cjs index 89349fb3c..f05ccff72 100644 --- a/tests/emitted-attribution.test.cjs +++ b/tests/emitted-attribution.test.cjs @@ -34,11 +34,12 @@ const test = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); +const crypto = require('node:crypto'); const { execFileSync } = require('node:child_process'); const fc = require('fast-check'); const { cleanup, createTempDir } = require('./helpers.cjs'); -const { BUILD_SCRIPT } = require('./helpers/install-shared.cjs'); +const { BUILD_SCRIPT, buildParityManifest, buildInstallTree, PKG_VERSION } = require('./helpers/install-shared.cjs'); const { resolveChangedPaths, resolveBase, @@ -57,6 +58,7 @@ const { touchesRuntimeRegistry, reconcileFamilies, safeDirArgs, + measuredPackageVersion, } = require('./helpers/emitted-runtime.cjs'); const { EXPECTED_MANIFEST_COUNT, loadManifests } = require('./helpers/emitted-provenance.cjs'); @@ -2448,3 +2450,303 @@ test('differential attribution over the real tree', { timeout: 900_000 }, async `emitted-attribution failed against ${base}@${baseSha.slice(0, 12)}:\n\n${formatReport(result)}`, ); }); + +// ─── Cross-tree version normalization (#2891) ────────────────────────────────── +// +// #2767's `currentManifests({ repoRoot })` spawns a DIFFERENT checkout's installer +// but, before this fix, still normalized the emitted content against THIS checkout's +// PKG_VERSION — so a version-bumped current tree compared against an older-version +// baseline worktree never collapsed the baseline's `// gsd-hook-version: ` stamp +// to '', every one of that baseline's emitted files spuriously "differed", +// and the differential attribution gate above (the real-tree test) failed with all +// 364 emitted hook paths unattributed. These tests pin the mechanism directly against +// `buildParityManifest`'s `pkgVersion` option and `measuredPackageVersion`, the two +// pieces `currentManifests` composes to fix it, rather than only against the +// expensive real-tree gate. + +function makeVersionStampedTree(hookVersion) { + const root = createTempDir('gsd-test-ppm-version-'); + const configDir = path.join(root, 'cfg'); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, 'hook.js'), + `// gsd-hook-version: ${hookVersion}\nconsole.log('hook body unchanged across versions');\n`, + ); + return { root, configDir }; +} + +test('buildParityManifest: same content at two different pkgVersions hashes identically when each is normalized against its OWN version (#2891)', () => { + const a = makeVersionStampedTree('1.8.0'); + const b = makeVersionStampedTree('1.9.0'); + try { + const manifestA = buildParityManifest(a.configDir, a.root, { pkgVersion: '1.8.0' }); + const manifestB = buildParityManifest(b.configDir, b.root, { pkgVersion: '1.9.0' }); + assert.equal( + manifestA['hook.js'], + manifestB['hook.js'], + 'byte-identical-apart-from-version-stamp files must hash identically once each side ' + + 'is normalized against the version that actually produced it' + ); + } finally { + cleanup(a.root); + cleanup(b.root); + } +}); + +test('buildParityManifest: hash of a measured tree does not depend on the MEASURING repo\'s own version (#2891)', () => { + // Reproduces the real cross-tree shape: content stamped with version X, normalized + // with the EXPLICIT pkgVersion of the tree that produced it (X) — never with this + // checkout's own PKG_VERSION (Y), which is what the pre-fix bug silently defaulted to. + const measuredVersion = '7.7.7'; + assert.notEqual( + measuredVersion, + PKG_VERSION, + 'test fixture must use a version distinct from this checkout\'s own PKG_VERSION for the assertion below to be meaningful' + ); + const tree = makeVersionStampedTree(measuredVersion); + try { + const manifest = buildParityManifest(tree.configDir, tree.root, { pkgVersion: measuredVersion }); + // The stamp must have collapsed to '' — if it hadn't (e.g. because the + // measuring repo's own PKG_VERSION had been used instead), the raw '7.7.7' would + // still be present pre-hash and this hash would differ from a control manifest + // built directly against the sentinel-substituted content. + const controlContent = `// gsd-hook-version: \nconsole.log('hook body unchanged across versions');\n`; + const controlHash = crypto.createHash('sha256').update(controlContent).digest('hex').slice(0, 16); + assert.equal( + manifest['hook.js'], + controlHash, + 'hash must reflect the version-stamp collapsing to using the MEASURED tree\'s ' + + 'own version, independent of whatever PKG_VERSION the measuring repo happens to be at' + ); + } finally { + cleanup(tree.root); + } +}); + +test('buildParityManifest: pkgVersion guard rejects empty/undefined/null/non-string/non-semver-shaped and never corrupts the manifest (#2891)', () => { + const tree = makeVersionStampedTree('1.8.0'); + try { + // Omitting pkgVersion entirely is legitimate (defaults to this checkout's own + // PKG_VERSION) and must NOT throw. + assert.doesNotThrow(() => buildParityManifest(tree.configDir, tree.root)); + + // Explicitly passing a bad value must throw — including an EXPLICIT `undefined`, + // which is deliberately NOT treated the same as omitting the key (see the `in` + // guard in install-shared.cjs: a caller-side bug that resolves a version to + // `undefined` must fail loudly, never silently fall back to this checkout's own + // version). '1' and '12' are shape failures (#2891 review FINDING 2): a + // non-semver-shaped string like '1' must be rejected, not silently accepted and + // later matched as a substring of unrelated numeric content (e.g. 'v 1.8.0 x'). + for (const bad of ['', undefined, null, 42, '1', '12']) { + assert.throws( + () => buildParityManifest(tree.configDir, tree.root, { pkgVersion: bad }), + /pkgVersion must be a non-empty semver-ish string/, + `expected pkgVersion=${JSON.stringify(bad)} to throw` + ); + } + + // Corruption check: an empty pkgVersion, if it ever reached blind substring + // replacement, would corrupt content that merely contains matching characters. + // Confirm the guard fires BEFORE that — a file whose entire content is 'abc' must + // never make it into a manifest via a '' pkgVersion. + const corruptibleRoot = createTempDir('gsd-test-ppm-corrupt-'); + const corruptibleDir = path.join(corruptibleRoot, 'cfg'); + fs.mkdirSync(corruptibleDir, { recursive: true }); + fs.writeFileSync(path.join(corruptibleDir, 'f.txt'), 'abc'); + try { + assert.throws( + () => buildParityManifest(corruptibleDir, corruptibleRoot, { pkgVersion: '' }), + /pkgVersion must be a non-empty semver-ish string/ + ); + } finally { + cleanup(corruptibleRoot); + } + } finally { + cleanup(tree.root); + } +}); + +test('buildParityManifest: pkgVersion shape guard boundary — limit-1/limit/limit+1 by length (#2891 review FINDING 2)', () => { + const tree = makeVersionStampedTree('1.8.0'); + try { + // '0.0.0' is the shortest string SEMVER_ISH_RE accepts (5 chars: MAJOR.MINOR.PATCH, + // all single-digit, no prerelease/build) — the "limit" case. + assert.doesNotThrow( + () => buildParityManifest(tree.configDir, tree.root, { pkgVersion: '0.0.0' }), + 'a minimal valid MAJOR.MINOR.PATCH string must be accepted (limit)' + ); + // '12' (limit+1 relative to the 1-char failure below, and still nowhere near + // semver-shaped) must still be rejected. + assert.throws( + () => buildParityManifest(tree.configDir, tree.root, { pkgVersion: '12' }), + /pkgVersion must be a non-empty semver-ish string/, + 'a 2-char non-semver-shaped string must be rejected (limit+1 by length from \'1\')' + ); + // '1' (limit-1 relative to '12') must be rejected — the concrete regression this + // guard exists to close: {pkgVersion:'1'} was previously ACCEPTED and rewrote + // 'v 1.8.0 x' to 'v .8.0 x' via a bare-substring match. + assert.throws( + () => buildParityManifest(tree.configDir, tree.root, { pkgVersion: '1' }), + /pkgVersion must be a non-empty semver-ish string/, + 'a 1-char string must be rejected (limit-1)' + ); + } finally { + cleanup(tree.root); + } +}); + +test('buildParityManifest: pkgVersion is honored when reachable via the PROTOTYPE CHAIN, not only as an own key (#2891 review FINDING 4)', () => { + const tree = makeVersionStampedTree('7.7.7'); + try { + // Object.create({pkgVersion:'7.7.7'}) has NO own 'pkgVersion' key, but the key IS + // reachable via `in` — before the fix this silently fell through to this + // checkout's own PKG_VERSION (the exact silent-fallback the guard exists to + // prevent), reached via a different vector than an explicit own-key bad value. + const opts = Object.create({ pkgVersion: '7.7.7' }); + const manifest = buildParityManifest(tree.configDir, tree.root, opts); + const controlContent = `// gsd-hook-version: \nconsole.log('hook body unchanged across versions');\n`; + const controlHash = crypto.createHash('sha256').update(controlContent).digest('hex').slice(0, 16); + assert.equal( + manifest['hook.js'], + controlHash, + 'an inherited pkgVersion must be read and normalized against, not silently ignored in favor of this checkout\'s own PKG_VERSION' + ); + } finally { + cleanup(tree.root); + } +}); + +test('buildParityManifest: opts guard rejects null/non-object (#2891 review FINDING 5)', () => { + const tree = makeVersionStampedTree('1.8.0'); + try { + for (const bad of [null, 'x', 42, true, []]) { + assert.throws( + () => buildParityManifest(tree.configDir, tree.root, bad), + /opts must be a plain object or omitted/, + `expected opts=${JSON.stringify(bad)} to throw a clear message, not a raw TypeError` + ); + } + } finally { + cleanup(tree.root); + } +}); + +test('buildInstallTree: no longer accepts/forwards an opts argument, so a bad third argument is silently ignored rather than reaching buildParityManifest\'s guard (#2891 review FINDINGS 5+6)', () => { + // FINDING 6 removed buildInstallTree's dead opts-forwarding parameter (pkgVersion + // never affects the emitted FILE SET, only content hashes, and no caller ever passed + // a third argument). A consequence: buildInstallTree(cd, root, null) — the exact + // FINDING 5 repro against the OLD forwarding code — no longer reaches + // buildParityManifest's opts guard at all; the extra argument is simply unused, + // consistent with ordinary JS call semantics, and buildParityManifest gets its + // default `{}`. This must NOT throw. + const tree = makeVersionStampedTree('1.8.0'); + try { + assert.doesNotThrow(() => buildInstallTree(tree.configDir, tree.root, null)); + assert.deepEqual( + buildInstallTree(tree.configDir, tree.root, null), + buildInstallTree(tree.configDir, tree.root), + 'a discarded third argument must not change the result' + ); + } finally { + cleanup(tree.root); + } +}); + +test('measuredPackageVersion: resolves this checkout\'s version with no repoRoot, the measured tree\'s version with one, and fails closed (#2891)', () => { + // No repoRoot at all (key genuinely absent): this checkout's own PKG_VERSION, no + // filesystem I/O. + assert.equal(measuredPackageVersion(), PKG_VERSION); + // Explicit `undefined` is the ONLY falsy value treated as "this checkout" — every + // OTHER falsy value ('', 0, false) is a caller-side bug and must fail closed rather + // than silently defaulting, consistent with `currentManifests`' installScript gate + // and with `buildParityManifest`'s pkgVersion guard (#2891 review FINDING 7). + assert.equal(measuredPackageVersion(undefined), PKG_VERSION); + for (const bad of ['', 0, false]) { + assert.throws( + () => measuredPackageVersion(bad), + /repoRoot must be a non-empty path or omitted entirely/, + `expected repoRoot=${JSON.stringify(bad)} to throw` + ); + } + + // A different tree's package.json: its OWN version, not this checkout's. + const measuredRoot = createTempDir('gsd-test-mpv-ok-'); + try { + fs.writeFileSync( + path.join(measuredRoot, 'package.json'), + JSON.stringify({ name: 'measured-tree', version: '9.9.9' }), + ); + assert.equal(measuredPackageVersion(measuredRoot), '9.9.9'); + } finally { + cleanup(measuredRoot); + } + + // Missing package.json: fails closed, never falls back to this checkout's version. + const missingRoot = createTempDir('gsd-test-mpv-missing-'); + try { + assert.throws(() => measuredPackageVersion(missingRoot), /cannot read/); + } finally { + cleanup(missingRoot); + } + + // Unparseable package.json. + const badJsonRoot = createTempDir('gsd-test-mpv-badjson-'); + try { + fs.writeFileSync(path.join(badJsonRoot, 'package.json'), '{not json'); + assert.throws(() => measuredPackageVersion(badJsonRoot), /not valid JSON/); + } finally { + cleanup(badJsonRoot); + } + + // Version-less package.json (key ABSENT entirely) — the only branch the pre-review + // test suite drove. + const noVersionRoot = createTempDir('gsd-test-mpv-noversion-'); + try { + fs.writeFileSync(path.join(noVersionRoot, 'package.json'), JSON.stringify({ name: 'no-version' })); + assert.throws(() => measuredPackageVersion(noVersionRoot), /no non-empty string "version" field/); + } finally { + cleanup(noVersionRoot); + } + + // "version" key PRESENT but an empty string — a distinct branch from "absent" + // (`typeof '' === 'string'` but `''.length === 0`); the pre-review suite never drove + // it and both surviving mutants collapse this into the absent-key case (#2891 review + // FINDING 3). + const emptyVersionRoot = createTempDir('gsd-test-mpv-emptyversion-'); + try { + fs.writeFileSync(path.join(emptyVersionRoot, 'package.json'), JSON.stringify({ version: '' })); + assert.throws(() => measuredPackageVersion(emptyVersionRoot), /no non-empty string "version" field/); + } finally { + cleanup(emptyVersionRoot); + } + + // "version" key PRESENT but non-string (e.g. a bare JSON number) — the other branch + // `typeof version !== 'string'` guards, distinct from both "absent" and "empty + // string" (#2891 review FINDING 3). + const numericVersionRoot = createTempDir('gsd-test-mpv-numericversion-'); + try { + fs.writeFileSync(path.join(numericVersionRoot, 'package.json'), JSON.stringify({ version: 123 })); + assert.throws(() => measuredPackageVersion(numericVersionRoot), /no non-empty string "version" field/); + } finally { + cleanup(numericVersionRoot); + } + + // Unreadable package.json: monkeypatch fs.readFileSync (NEVER chmod 0o000 — root + // bypasses mode bits and the test would silently pass with zero coverage in root + // Docker/CI). Save original, override to throw, assert.throws, restore in `finally`. + const unreadableRoot = createTempDir('gsd-test-mpv-unreadable-'); + try { + fs.writeFileSync(path.join(unreadableRoot, 'package.json'), JSON.stringify({ version: '1.0.0' })); + const orig = fs.readFileSync; + try { + fs.readFileSync = () => { throw new Error('injected package.json read failure'); }; + assert.throws(() => measuredPackageVersion(unreadableRoot), /injected package\.json read failure/); + } finally { + fs.readFileSync = orig; + } + // Restoration is real, not assumed. + assert.equal(measuredPackageVersion(unreadableRoot), '1.0.0'); + } finally { + cleanup(unreadableRoot); + } +}); diff --git a/tests/helpers/emitted-runtime.cjs b/tests/helpers/emitted-runtime.cjs index 5819fff5d..be542e698 100644 --- a/tests/helpers/emitted-runtime.cjs +++ b/tests/helpers/emitted-runtime.cjs @@ -42,6 +42,7 @@ const { MINIMUM_MANIFEST_FAMILIES, runMinimalInstall, buildParityManifest, + PKG_VERSION, } = require('./install-shared.cjs'); const REPO_ROOT = path.join(__dirname, '..', '..'); @@ -601,6 +602,73 @@ function baselineSizesAtRef(base = 'origin/next') { return found === 0 ? null : sizes; } +/** + * The package version of the tree whose emitted output is about to be measured. + * + * `bin/install.js` bakes `{{GSD_VERSION}}` -> `pkg.version` into every emitted hook, + * and `buildParityManifest`'s `pkgVersion` normalization exists to collapse that stamp + * back to '' so a version bump alone never moves a hash. That normalization is + * only correct when `pkgVersion` matches the version of the tree that PRODUCED the + * content being hashed — for `repoRoot`-driven cross-tree measurement (#2767, #2891) + * that is NOT necessarily this checkout's own `PKG_VERSION`. This helper resolves the + * right version for whichever tree is actually being measured. + * + * Fails closed: a missing, unreadable, unparseable, or version-less `package.json` at + * `repoRoot` throws rather than silently falling back to this checkout's own version — + * a silent fallback here is exactly the cross-tree mis-attribution bug #2891 fixes + * (every emitted hook path would spuriously differ, and the differential attribution + * gate would blame nothing for it). + * + * @param {string} [repoRoot] - absolute path to the tree being measured. Only an + * OMITTED (or explicit `undefined`) repoRoot means "this checkout" and returns this + * module's own PKG_VERSION with no I/O. Any OTHER falsy value (`''`, `0`, `false`) is + * NOT treated as "this checkout" — it is a caller-side bug (e.g. an unresolved path + * variable) and must fail closed rather than silently measuring the wrong tree, + * consistent with `currentManifests`' `installScript` gate below and with how + * `buildParityManifest`'s `pkgVersion` treats an explicit-but-empty value as a caller + * error rather than a fallback trigger (#2891 review FINDING 7). + * @returns {string} non-empty package version string. + */ +function measuredPackageVersion(repoRoot) { + if (repoRoot === undefined) return PKG_VERSION; + if (!repoRoot) { + throw new Error( + `measuredPackageVersion: repoRoot must be a non-empty path or omitted entirely, got ${JSON.stringify(repoRoot)}. ` + + 'An omitted/undefined repoRoot means "this checkout"; any other falsy value is treated as a caller error.' + ); + } + + const pkgPath = path.join(repoRoot, 'package.json'); + let raw; + try { + raw = fs.readFileSync(pkgPath, 'utf8'); + } catch (err) { + throw new Error( + `measuredPackageVersion: cannot read ${pkgPath} (${err.message}); cannot normalize ` + + 'emitted version for the measured tree; a wrong version silently mis-attributes every hook.' + ); + } + + let parsed; + try { + parsed = JSON.parse(raw); + } catch (err) { + throw new Error( + `measuredPackageVersion: ${pkgPath} is not valid JSON (${err.message}); cannot normalize ` + + 'emitted version for the measured tree; a wrong version silently mis-attributes every hook.' + ); + } + + const version = parsed && parsed.version; + if (typeof version !== 'string' || version.length === 0) { + throw new Error( + `measuredPackageVersion: ${pkgPath} has no non-empty string "version" field; cannot ` + + 'normalize emitted version for the measured tree; a wrong version silently mis-attributes every hook.' + ); + } + return version; +} + /** * Build the CURRENT emitted manifest set for real — one installer spawn per runtime. * This is the expensive, honest half: it reflects what the tree actually emits now, @@ -616,14 +684,25 @@ function baselineSizesAtRef(base = 'origin/next') { * base-ref worktree) so the two sides stay comparable even if the definition itself * evolves — running the OTHER tree's own (older, or absent) copy of this function would * defeat that. + * + * The version normalized into '' inside each manifest entry is resolved via + * `measuredPackageVersion(repoRoot)` — i.e. `/package.json`'s own version, + * NOT this checkout's — since `/bin/install.js` is what stamped the emitted + * content in the first place (#2891). */ function currentManifests({ repoRoot } = {}) { - const installScript = repoRoot ? path.join(repoRoot, 'bin', 'install.js') : undefined; + // Gated the same way `measuredPackageVersion` gates its own repoRoot below: only an + // omitted/`undefined` repoRoot means "this checkout" (installScript stays the + // default). Any other falsy value falls through to `measuredPackageVersion`, which + // throws — this line never gets a chance to diverge from that same rule (#2891 + // review FINDING 7). + const installScript = repoRoot !== undefined ? path.join(repoRoot, 'bin', 'install.js') : undefined; + const pkgVersion = measuredPackageVersion(repoRoot); const manifests = {}; for (const { name, runtime, scope } of MANIFEST_FAMILIES) { const { configDir, root } = runMinimalInstall({ runtime, scope, installScript }); try { - manifests[name] = buildParityManifest(configDir, root); + manifests[name] = buildParityManifest(configDir, root, { pkgVersion }); } finally { cleanup(root); } @@ -695,6 +774,7 @@ module.exports = { baselineManifestsAtRef, baselineSizesAtRef, buildBaselineAtRef, + measuredPackageVersion, currentManifests, currentSizes, readAckFile, diff --git a/tests/helpers/install-shared.cjs b/tests/helpers/install-shared.cjs index 3ab130a06..d684e4cbb 100644 --- a/tests/helpers/install-shared.cjs +++ b/tests/helpers/install-shared.cjs @@ -124,9 +124,16 @@ const SKILL_RUNTIMES = [ // (the generator's copy was missing the realpath normalization below) and // shipped broken fixtures three times (#2086, #2095, #2100). -// The installed package version, normalized to '' in hash computation so +// This checkout's own package version — the DEFAULT normalized in hash computation so // the golden is stable across version bumps (the rc step runs `npm version X.Y.Z-rc.N` // before tests, which rebakes the version into hook files and gsd-core/VERSION). +// +// This is only a default. buildParityManifest's `pkgVersion` option exists precisely +// because the tree being MEASURED is not always this checkout (#2767's `repoRoot` +// installer-spawn path measures a DIFFERENT tree's `bin/install.js` output). The +// version that must be normalized is always the version of the tree that PRODUCED +// the emitted bytes, not the version of whichever checkout happens to be running this +// test file — see the pkgVersion JSDoc on buildParityManifest below. const PKG_VERSION = require('../../package.json').version; // Volatile metadata files always excluded from the parity manifest. @@ -193,10 +200,73 @@ const EXCLUDED_PREFIXES = ['gsd-core/bin/lib/']; // ─── Helper functions ───────────────────────────────────────────────────────── function stripAnsi(str) { - + return str.replace(/\x1b\[[0-9;]*m/g, ''); } +// A version string can itself contain regex metacharacters (`.`, and — via +// prerelease/build metadata — `-`/`+`), so it must be escaped before being spliced +// into a RegExp source, or e.g. the `.` in "1.9.0" would match ANY character. +function escapeRegExp(str) { + return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +// Loosely semver-shaped: leading `MAJOR.MINOR.PATCH`, optional `-prerelease` and/or +// `+build` metadata (e.g. `1.9.0`, `1.9.0-rc.1`, `1.9.0+abc`). Deliberately loose +// (not the full semver grammar) — this only needs to reject obviously-malformed +// values like `'1'` (FINDING 2, #2891 review) before they reach regex construction, +// not to be a semver validator. +const SEMVER_ISH_RE = /^\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?(\+[0-9A-Za-z.-]+)?$/; + +/** + * Anchored, narrow version-stamp normalization — the FINDING 1 (#2891 review) fix for + * the prior `.split(pkgVersion).join('')`, which blind-replaced EVERY + * occurrence of the version string anywhere in emitted content. That was unsound in + * both directions once each side of a diff normalizes against a DIFFERENT pkgVersion + * (baseline vs current tree, #2891): a bare semver literal that genuinely changed + * between the two versions collapses to '' on both sides and goes invisible + * (false negative), while an UNCHANGED file that happens to contain a literal equal to + * only the CURRENT version collapses on one side only and reports as spurious drift + * (false positive). Emitted sources really do carry bare semver literals that are NOT + * install-time stamps — e.g. gsd-core/workflows/update.md's `1.4.0`/`1.3.1` examples, + * agents/gsd-project-researcher.md's `1.2.3`, gsd-core/workflows/help/modes/full.md's + * `1.0.0` — and those must stay VISIBLE to the parity gate if they ever change. + * + * So instead of replacing the version everywhere, this only normalizes it at the + * specific places `bin/install.js` actually stamps `pkg.version` into emitted content + * as a version FIELD/marker (not prose): + * - `// gsd-hook-version: ` / `# gsd-hook-version: ` — the + * `{{GSD_VERSION}}` substitution done for every emitted hook file. + * - `"version": ""` — JSON manifests (plugin/extension/capability-style) + * embedding the package version as a string field. + * - `version: ""` / `version: ` — YAML frontmatter version fields (e.g. + * skill frontmatter's `yamlQuote(pkg.version)`, Hermes' category + * `DESCRIPTION.md`). + * - `@opengsd/gsd-core@` — pinned package-spec references. + * - a file whose ENTIRE trimmed content IS the version (`gsd-core/VERSION`). + * Each pattern only matches when the version in the content EQUALS the supplied + * `pkgVersion` — this is deliberately narrow, at the cost of needing a new pattern any + * time the installer grows a new stamp site (see the empirical repro-harness check + * this fix was verified against, #2891 review FINDING 1). + * + * @param {string} content + * @param {string} pkgVersion - already validated non-empty semver-ish string. + * @returns {string} + */ +function normalizeVersionStamps(content, pkgVersion) { + const v = escapeRegExp(pkgVersion); + // `(?![\w.+-])` after a bare (unquoted/uncaptured-suffix) match stops a version + // from matching as a PREFIX of a longer version-shaped string it is not equal to + // (e.g. pkgVersion '1.9.0' must not match inside '1.9.0-rc.1' or '1.9.0.1'). + return content + .replace(new RegExp(`((?:\\/\\/|#)\\s*gsd-hook-version:\\s*)${v}(?![\\w.+-])`, 'g'), '$1') + .replace(new RegExp(`("version"\\s*:\\s*")${v}(")`, 'g'), '$1$2') + .replace(new RegExp(`(version:\\s*")${v}(")`, 'g'), '$1$2') + .replace(new RegExp(`(version:\\s*)${v}(?![\\w.+-])`, 'g'), '$1') + .replace(new RegExp(`(@opengsd/gsd-core@)${v}(?![\\w.+-])`, 'g'), '$1') + .replace(new RegExp(`^(\\s*)${v}(\\s*)$`), '$1$2'); +} + function walk(dir) { const results = []; for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { @@ -218,9 +288,68 @@ function walk(dir) { * * @param {string} configDir - absolute path to the installed runtime config dir * @param {string} root - temp root path to replace with '' + * @param {object} [opts] + * @param {string} [opts.pkgVersion] - the version string to normalize to ''. + * MUST be the version of the tree that PRODUCED the emitted content at `configDir` + * — NOT necessarily this checkout's own version. OMITTING this option (or the whole + * `opts` argument) defaults to this checkout's own PKG_VERSION, which is only + * correct when `configDir` was emitted by THIS checkout's installer. A caller + * measuring a DIFFERENT tree's installer output (#2767's `repoRoot`-driven spawns) + * must pass that other tree's own version explicitly, or the normalization silently + * compares apples to oranges: baseline hooks baked with version X never collapse to + * '' when normalized against version Y, so every emitted file looks changed + * even when byte-identical apart from the version stamp. Must be a non-empty + * semver-ish string (`MAJOR.MINOR.PATCH` with optional prerelease/build metadata) + * when the key is REACHABLE at all — see the guard below for why, and why an + * explicit `{ pkgVersion: undefined }` is treated as a caller error rather than + * silently falling back to the default (that fallback is the exact bug being + * fixed). `opts` itself must be a plain object (or omitted/undefined) — `null` or a + * non-object throws rather than reaching `Object`'s coercion of `null`/`undefined` + * (#2891 review FINDING 5). * @returns {{ [rel: string]: string }} */ -function buildParityManifest(configDir, root) { +function buildParityManifest(configDir, root, opts = {}) { + // `opts = {}` only substitutes for an OMITTED (or explicit `undefined`) third + // argument — `null` and other non-object values sail past a default parameter and + // would otherwise reach the `in` check below and throw a raw, unhelpful + // `TypeError: Cannot convert undefined or null to object` (#2891 review FINDING 5). + // Fail with a clear, attributable message instead. + if (opts === null || typeof opts !== 'object' || Array.isArray(opts)) { + throw new Error( + `buildParityManifest: opts must be a plain object or omitted, got ${JSON.stringify(opts)}.` + ); + } + + // Distinguish "the caller didn't pass pkgVersion at all" (legitimate — use this + // checkout's own PKG_VERSION) from "the caller passed pkgVersion explicitly, and it + // happens to be undefined/null/empty/non-string" (a caller error that must throw, + // never silently fall back). A plain default-parameter (`{ pkgVersion = PKG_VERSION + // } = {}`) cannot make this distinction — JS treats an explicit `undefined` value + // identically to an absent key, which would let a caller-side bug (e.g. a repoRoot + // version lookup that resolved to undefined) silently normalize against THIS + // checkout's version instead of throwing — exactly the cross-tree mis-attribution + // bug #2891 fixes. Use the `in` operator (not `hasOwnProperty`) so the key is honored + // whether it is OWN or INHERITED — `Object.create({ pkgVersion: 'x' })` reaches this + // function with the key reachable via the prototype chain, and a caller-error value + // sitting there must still be validated (and rejected) rather than silently ignored + // in favor of the default; only a key ABSENT from the whole chain means "caller + // didn't specify one, use this checkout's own version" (#2891 review FINDING 4). + const pkgVersion = 'pkgVersion' in opts ? opts.pkgVersion : PKG_VERSION; + + // GUARD: an empty/falsy/non-string/non-semver-shaped pkgVersion must never reach the + // normalization below. A careless caller passing e.g. '1' would silently match and + // corrupt content that merely contains that digit as a substring of an unrelated + // number (#2891 review FINDING 2) — and, pre-FINDING-1, an empty string reaching + // `.split('')` would have exploded manifest content into individual characters. Fail + // closed instead of falling back to this checkout's PKG_VERSION: a silent fallback is + // exactly the cross-tree mis-attribution bug this option exists to fix (#2891). + if (typeof pkgVersion !== 'string' || pkgVersion.length === 0 || !SEMVER_ISH_RE.test(pkgVersion)) { + throw new Error( + `buildParityManifest: pkgVersion must be a non-empty semver-ish string ` + + `(MAJOR.MINOR.PATCH, optional -prerelease/+build), got ${JSON.stringify(pkgVersion)}. ` + + 'Pass the version of the tree that produced the emitted content at configDir.' + ); + } const allFiles = walk(configDir); const unsorted = {}; @@ -246,12 +375,21 @@ function buildParityManifest(configDir, root) { const content = fs.readFileSync(full); // Normalize every occurrence of the temp root so hashes are stable across runs. - // Also normalize the package version so the golden survives `npm version` bumps - // (the rc release step bakes the new version into hook files before running tests). - const normalized = content.toString('utf8') - .split(realRoot).join('') - .split(root).join('') - .split(PKG_VERSION).join(''); + // Also normalize the PRODUCING tree's package version at its known stamp sites + // (pkgVersion, defaulted to this checkout's own) — via the ANCHORED + // normalizeVersionStamps, not a blind substring replace — so the golden survives + // `npm version` bumps (the rc release step bakes the new version into hook files + // before running tests) and, for a cross-tree caller, so the version stamp baked + // in by a DIFFERENT tree's installer doesn't masquerade as a real content diff, + // WITHOUT also masking a genuine content change to an unrelated bare semver + // literal that happens to equal pkgVersion (#2891; see normalizeVersionStamps' + // own doc comment for the blind-replace failure modes this replaced). + const normalized = normalizeVersionStamps( + content.toString('utf8') + .split(realRoot).join('') + .split(root).join(''), + pkgVersion, + ); const hash = crypto.createHash('sha256').update(normalized).digest('hex').slice(0, 16); unsorted[rel] = hash; } @@ -266,7 +404,14 @@ function buildParityManifest(configDir, root) { /** Sorted list of emitted relative paths for a runtime install (file-set snapshot, * #2267). Reuses buildParityManifest's exact exclusion set so the tree and the - * content manifest never diverge on which files they cover. */ + * content manifest never diverge on which files they cover. Deliberately does NOT + * take (or forward) a `pkgVersion`/`opts` parameter: the emitted FILE SET is + * version-independent — `pkgVersion` only ever changes which bytes a file's HASH + * normalizes to, never which paths buildParityManifest walks or excludes — so there + * is nothing for a caller to pass here, and forwarding one through would only let a + * bad version value make a pure file-set query throw for no file-set-shaped reason + * (#2891 review FINDING 6; verified no caller passes a third argument — + * tests/golden-install-tree.test.cjs, scripts/gen-install-tree-fixtures.cjs). */ function buildInstallTree(configDir, root) { return Object.keys(buildParityManifest(configDir, root)).sort(); }