diff --git a/package.json b/package.json index 53c3399aae..65c7e56c80 100644 --- a/package.json +++ b/package.json @@ -43,6 +43,7 @@ "lint": "eslint src/", "build": "node build.js", "generate:skills": "node scripts/generate-skillssh.mjs", + "regen:parity-hashes": "node scripts/regen-parity-hashes.mjs", "dev": "tsc --watch", "dev:cli": "pnpm build && node bin/openspec.js", "test": "vitest run", diff --git a/scripts/README.md b/scripts/README.md index dcdc6e744a..199fc5c30f 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -28,6 +28,45 @@ git add flake.nix git commit -m "chore: update flake.nix dependency hash" ``` +## regen-parity-hashes.mjs + +Recomputes the golden hashes pinned in +`test/core/templates/skill-templates-parity.test.ts`. + +**When to use**: After any intended workflow-template change, and after +rebasing a branch that edits templates — two branches touching different +templates collide on the same hash map, and hand-editing 64-character hashes +during a conflict is where transcription mistakes happen. + +**Usage**: +```bash +pnpm build && pnpm regen:parity-hashes +pnpm vitest run test/core/templates/skill-templates-parity.test.ts +``` + +**What it does**: +1. Refuses to run if `dist/` is missing or older than `src/` — hashes come from + the build, while the parity test reads `src/`, so regenerating against a + stale build writes hashes the test then rejects +2. Recomputes every pinned hash from the built `dist/` +3. Rewrites the map in place and prints which entries moved +4. Exits non-zero, writing nothing, if it cannot account for every pinned hash: + a label with no matching export (a renamed or deleted template), or a hash + line these patterns do not recognise. Both would otherwise be left stale + while the run reported success, so `nothing to update` always means it. + +Line endings round-trip unchanged, so a CRLF checkout is safe — `test/**` has no +`text eol=lf` attribute, so the file arrives with CRLF on Windows. + +The parity test recomputes the same hashes independently, so this script cannot +silently produce a wrong value. Always run the test afterwards; it, not this +script, is the authority. + +The rewriting lives in `parity-hash-shared.mjs` so its guards can be exercised +against fabricated input — see `test/core/templates/parity-hash-shared.test.ts`. +A test that ran this script for real would rewrite the repository's own parity +test file mid-suite. + ## postinstall.js Post-installation script that runs after package installation. diff --git a/scripts/parity-hash-shared.mjs b/scripts/parity-hash-shared.mjs new file mode 100644 index 0000000000..71f6ddb806 --- /dev/null +++ b/scripts/parity-hash-shared.mjs @@ -0,0 +1,130 @@ +/** + * Shared helpers for the parity-hash regeneration script and its tests. + * + * The rewriting lives here, separate from `regen-parity-hashes.mjs`, so its + * guards can be exercised against fabricated input instead of the repository's + * own parity test file. A test that ran the script for real would rewrite + * `test/core/templates/skill-templates-parity.test.ts` on disk mid-suite. + */ + +/** + * Pinned-hash line patterns. + * + * The trailing comma is optional: the last entry of a map may legally omit it, + * and requiring it silently skipped such a pin. + * + * CRLF needs no special handling. `test/**` carries no `text eol=lf` attribute, + * so a Windows checkout delivers CRLF, but JavaScript treats `\r` as a line + * terminator under /m - `$` matches before it, so the carriage return is never + * consumed and survives the rewrite. (Python's `re.M` does not, which is worth + * knowing before porting these patterns anywhere.) + */ +const FUNCTION_PIN = /^(\s+)(get[A-Za-z0-9]+): '([0-9a-f]{64})'(,?)$/gm; +const CONTENT_PIN = /^(\s+)'(openspec-[a-z0-9-]+)': '([0-9a-f]{64})'(,?)$/gm; + +/** Any 64-hex literal, however it is written. */ +const HEX_LITERAL = /'[0-9a-f]{64}'/g; + +/** + * Rewrite every pinned hash in the parity test's source. + * + * `resolveFunctionHash(name)` and `resolveContentHash(dirName)` return the hash + * a pin should now hold, or `undefined` when the label no longer corresponds to + * anything - a renamed or deleted template, which is an error rather than a + * line to leave alone. + * + * Throws without returning a partial rewrite when the number of 64-hex literals + * found does not match the number rewritten. That count uses a deliberately + * broader pattern than the two above, so it is a real cross-check: were it + * derived from the same patterns, a line they miss would go missing from both + * sides and prove nothing. + * + * @param {string} source - contents of the parity test file + * @param {{ + * resolveFunctionHash: (name: string) => string | undefined, + * resolveContentHash: (dirName: string) => string | undefined, + * knownContentKeys?: Iterable, + * sourceLabel?: string, + * }} resolvers + * @returns {{ source: string, moved: string[] }} rewritten source and the + * labels whose hash changed + */ +export function rewriteParityHashes( + source, + { resolveFunctionHash, resolveContentHash, knownContentKeys = [], sourceLabel = 'the parity test' } +) { + const moved = []; + const seenContentKeys = new Set(); + let seen = 0; + + const totalHexLiterals = (source.match(HEX_LITERAL) ?? []).length; + + let rewritten = source.replace(FUNCTION_PIN, (_match, indent, name, previous, comma) => { + const next = resolveFunctionHash(name); + if (next === undefined) { + throw new Error(`${name} is pinned in the parity test but not exported from skill-templates.js`); + } + if (next !== previous) moved.push(name); + seen += 1; + return `${indent}${name}: '${next}'${comma}`; + }); + + rewritten = rewritten.replace(CONTENT_PIN, (_match, indent, dirName, previous, comma) => { + const next = resolveContentHash(dirName); + if (next === undefined) { + throw new Error(`'${dirName}' is pinned in the parity test but not returned by getSkillTemplates()`); + } + if (next !== previous) moved.push(dirName); + seenContentKeys.add(dirName); + seen += 1; + return `${indent}'${dirName}': '${next}'${comma}`; + }); + + if (seen !== totalHexLiterals) { + throw new Error( + `Rewrote ${seen} of ${totalHexLiterals} 64-hex literals in ${sourceLabel}.\n` + + 'Every one is assumed to be a pinned hash, so the two counts must agree. Either:\n' + + ' - a pinned hash is formatted in a way the patterns here do not match, and ' + + 'would have been left stale without warning: widen them; or\n' + + ' - the file gained a 64-hex literal that is not a pin: narrow the count above ' + + 'so it stops being mistaken for one.' + ); + } + + // The checks above only see pins that exist. A workflow added to the registry + // but never pinned is invisible to them AND to the parity test, which compares + // only the entries it already lists - so it would ship with no golden hash at + // all while this reported success. Compare the other direction too. + const unpinned = [...knownContentKeys].filter((key) => !seenContentKeys.has(key)); + if (unpinned.length > 0) { + throw new Error( + `getSkillTemplates() returns ${unpinned.length} skill(s) with no pinned hash in ${sourceLabel}:\n` + + unpinned.map((key) => ` ${key}`).join('\n') + + '\nAdd each to EXPECTED_GENERATED_SKILL_CONTENT_HASHES and GENERATED_SKILL_FACTORIES.\n' + + 'Until then the skill ships with no parity coverage, so this run would have ' + + 'reported success while leaving it unguarded.' + ); + } + + return { source: rewritten, moved }; +} + +/** + * Stable, key-sorted serialisation used to hash a template's payload. + * + * Must stay byte-compatible with the copy in + * `test/core/templates/skill-templates-parity.test.ts`. A divergence cannot pass + * unnoticed: that test recomputes the hashes independently and compares. + */ +export function stableStringify(value) { + if (Array.isArray(value)) { + return `[${value.map(stableStringify).join(',')}]`; + } + if (value && typeof value === 'object') { + const entries = Object.entries(value) + .sort(([left], [right]) => left.localeCompare(right)) + .map(([key, item]) => `${JSON.stringify(key)}:${stableStringify(item)}`); + return `{${entries.join(',')}}`; + } + return JSON.stringify(value); +} diff --git a/scripts/regen-parity-hashes.mjs b/scripts/regen-parity-hashes.mjs new file mode 100644 index 0000000000..917e20d2fb --- /dev/null +++ b/scripts/regen-parity-hashes.mjs @@ -0,0 +1,115 @@ +#!/usr/bin/env node + +/** + * Regenerate the golden hashes in `test/core/templates/skill-templates-parity.test.ts`. + * + * That test pins a SHA-256 per template so an unintended edit to any workflow + * template fails loudly. The flip side is that every *intended* edit leaves the + * pinned hashes stale, and two branches editing different templates collide on + * the same hash map — so a rebase means recomputing them by hand, which is + * where transcription mistakes creep in. + * + * This script recomputes every pinned hash from the built `dist/` and rewrites + * the map in place, reporting exactly which entries moved. + * + * Three things are hard errors rather than silent skips, because "nothing to + * update" has to mean it: + * - a `dist/` older than `src/`, which would pin hashes from a stale build + * that the parity test (which reads `src/`) then rejects + * - a pinned label with no matching export (a renamed or deleted template) + * - a pinned hash whose line the patterns do not recognise, which would + * otherwise be left stale while the run reported success + * + * The last two live in `parity-hash-shared.mjs` so they can be exercised against + * fabricated input; see `test/core/templates/parity-hash-shared.test.ts`. + * + * It cannot silently produce wrong hashes: the parity test recomputes them + * independently and compares. If `stableStringify` ever drifted from the test's + * copy, the test fails. Always run the test afterwards - that check, not this + * script, is the authority. + * + * Usage: + * pnpm build && pnpm regen:parity-hashes && pnpm vitest run test/core/templates/skill-templates-parity.test.ts + */ + +import { createHash } from 'node:crypto'; +import { readdirSync, readFileSync, statSync, writeFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; + +import { rewriteParityHashes, stableStringify } from './parity-hash-shared.mjs'; + +const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '..'); +const distUrl = (p) => pathToFileURL(join(repoRoot, 'dist', p)).href; + +/** Newest mtime under a directory, or -1 if it does not exist. */ +function newestMtime(dir) { + let newest = -1; + let entries; + try { + entries = readdirSync(dir, { withFileTypes: true }); + } catch { + return newest; + } + for (const entry of entries) { + if (entry.name === 'node_modules' || entry.name.startsWith('.')) continue; + const full = join(dir, entry.name); + const mtime = entry.isDirectory() ? newestMtime(full) : statSync(full).mtimeMs; + if (mtime > newest) newest = mtime; + } + return newest; +} + +// Hashes are computed from dist/, but the parity test recomputes them from +// src/. Regenerating against a stale build therefore writes hashes the test +// then rejects, after reporting "nothing to update" - a false all-clear on the +// most common mistake there is, forgetting to build. Refuse to guess. +const srcMtime = newestMtime(join(repoRoot, 'src')); +const distMtime = newestMtime(join(repoRoot, 'dist')); +if (distMtime < 0) { + throw new Error('dist/ is missing. Run `pnpm build` first - hashes are computed from the build.'); +} +if (srcMtime > distMtime) { + throw new Error( + 'dist/ is older than src/, so the hashes would be computed from a stale build\n' + + 'and the parity test - which reads src/ - would reject them. Run `pnpm build` first.' + ); +} + +const templates = await import(distUrl('core/templates/skill-templates.js')); +const { getSkillTemplates, generateSkillContent } = await import( + distUrl('core/shared/skill-generation.js') +); + +const TEST_FILE = join(repoRoot, 'test/core/templates/skill-templates-parity.test.ts'); + +const sha256 = (value) => createHash('sha256').update(value).digest('hex'); + +// The generated-content hashes are keyed by skill directory. Read that mapping +// from the same production helper the skills.sh generator uses, so a new +// workflow never needs a second list kept in sync here. +const PARITY_BASELINE = 'PARITY-BASELINE'; +const contentByDir = new Map( + getSkillTemplates().map(({ dirName, template }) => [ + dirName, + sha256(generateSkillContent(template, PARITY_BASELINE)), + ]) +); + +const { source, moved } = rewriteParityHashes(readFileSync(TEST_FILE, 'utf-8'), { + resolveFunctionHash: (name) => + typeof templates[name] === 'function' ? sha256(stableStringify(templates[name]())) : undefined, + resolveContentHash: (dirName) => contentByDir.get(dirName), + knownContentKeys: contentByDir.keys(), + sourceLabel: TEST_FILE, +}); + +writeFileSync(TEST_FILE, source); + +if (moved.length === 0) { + console.log('Parity hashes already match the build - nothing to update.'); +} else { + console.log(`Updated ${moved.length} parity hash(es):`); + for (const name of moved) console.log(` ${name}`); +} +console.log('\nNow run: pnpm vitest run test/core/templates/skill-templates-parity.test.ts'); diff --git a/test/core/templates/parity-hash-shared.test.ts b/test/core/templates/parity-hash-shared.test.ts new file mode 100644 index 0000000000..6a92b9b37d --- /dev/null +++ b/test/core/templates/parity-hash-shared.test.ts @@ -0,0 +1,171 @@ +import { describe, expect, it } from 'vitest'; + +// @ts-expect-error - plain ESM helper shared with the regeneration script +import { rewriteParityHashes } from '../../../scripts/parity-hash-shared.mjs'; + +// Guards for scripts/regen-parity-hashes.mjs. Every case here uses fabricated +// input: running the real script would rewrite the repository's own +// skill-templates-parity.test.ts on disk mid-suite, and a failure part-way +// through would leave those hashes committed by the next `git add -A`. +// +// Each case corresponds to a way an earlier revision of the script reported +// "nothing to update" while leaving a pin stale, or refused to run at all. + +const OLD = 'a'.repeat(64); +const NEW = 'b'.repeat(64); +const OTHER = 'c'.repeat(64); + +/** Resolvers that answer for exactly the labels a fixture pins. */ +function resolvers( + fns: Record = {}, + dirs: Record = {}, + knownContentKeys: string[] = Object.keys(dirs) +) { + return { + resolveFunctionHash: (name: string) => fns[name], + resolveContentHash: (dirName: string) => dirs[dirName], + knownContentKeys, + sourceLabel: 'fixture', + }; +} + +describe('parity hash rewriting', () => { + it('rewrites a stale function pin and reports it moved', () => { + const src = `const M = {\n getFooTemplate: '${OLD}',\n};\n`; + const result = rewriteParityHashes(src, resolvers({ getFooTemplate: NEW })); + + expect(result.source).toContain(`getFooTemplate: '${NEW}',`); + expect(result.moved).toEqual(['getFooTemplate']); + }); + + it('rewrites a stale generated-content pin and reports it moved', () => { + const src = `const M = {\n 'openspec-foo-bar': '${OLD}',\n};\n`; + const result = rewriteParityHashes(src, resolvers({}, { 'openspec-foo-bar': NEW })); + + expect(result.source).toContain(`'openspec-foo-bar': '${NEW}',`); + expect(result.moved).toEqual(['openspec-foo-bar']); + }); + + it('leaves an already-correct pin untouched and reports nothing moved', () => { + const src = `const M = {\n getFooTemplate: '${OLD}',\n};\n`; + const result = rewriteParityHashes(src, resolvers({ getFooTemplate: OLD })); + + expect(result.source).toBe(src); + expect(result.moved).toEqual([]); + }); + + // A map's last entry may legally omit its trailing comma, and a reformat + // produces exactly that. Requiring the comma silently skipped such a pin + // while the run reported success. + it('rewrites a pin with no trailing comma and keeps it comma-less', () => { + const src = `const M = {\n getFooTemplate: '${OLD}'\n};\n`; + const result = rewriteParityHashes(src, resolvers({ getFooTemplate: NEW })); + + expect(result.source).toContain(`getFooTemplate: '${NEW}'\n`); + expect(result.source).not.toContain(`'${NEW}',`); + expect(result.moved).toEqual(['getFooTemplate']); + }); + + it('preserves a trailing comma when the pin has one', () => { + const src = `const M = {\n getFooTemplate: '${OLD}',\n getBarTemplate: '${OTHER}',\n};\n`; + const result = rewriteParityHashes( + src, + resolvers({ getFooTemplate: NEW, getBarTemplate: OTHER }) + ); + + expect(result.source).toContain(`getFooTemplate: '${NEW}',\n`); + expect(result.moved).toEqual(['getFooTemplate']); + }); + + // test/** carries no `text eol=lf` attribute, so a Windows checkout delivers + // CRLF. Anchoring on $ alone matched nothing there and the run aborted. + it('round-trips CRLF line endings unchanged', () => { + const src = `const M = {\r\n getFooTemplate: '${OLD}',\r\n 'openspec-foo': '${OTHER}',\r\n};\r\n`; + const result = rewriteParityHashes( + src, + resolvers({ getFooTemplate: NEW }, { 'openspec-foo': OTHER }) + ); + + expect(result.source).toContain(`getFooTemplate: '${NEW}',\r\n`); + expect(result.source.split('\n').length).toBe(src.split('\n').length); + expect(result.source).not.toMatch(/[^\r]\n/); + }); + + it('throws when a function pin names something that no longer exists', () => { + const src = `const M = {\n getGoneTemplate: '${OLD}',\n};\n`; + + expect(() => rewriteParityHashes(src, resolvers())).toThrow(/getGoneTemplate is pinned/); + }); + + it('throws when a generated-content pin names a directory that no longer exists', () => { + const src = `const M = {\n 'openspec-gone': '${OLD}',\n};\n`; + + expect(() => rewriteParityHashes(src, resolvers())).toThrow(/'openspec-gone' is pinned/); + }); + + // The count is taken with a broader pattern than the rewriters on purpose: + // derived from the same patterns, a line they miss would vanish from both + // sides and prove nothing. + it('throws when a pin is formatted in a way the patterns do not match', () => { + const src = `const M = {\n 'getFooTemplate': '${OLD}',\n};\n`; + + expect(() => rewriteParityHashes(src, resolvers({ getFooTemplate: NEW }))).toThrow( + /Rewrote 0 of 1 64-hex literals in fixture/ + ); + }); + + it('throws when the file gains a 64-hex literal that is not a pin', () => { + const src = `const UNRELATED = '${OTHER}';\nconst M = {\n getFooTemplate: '${OLD}',\n};\n`; + + expect(() => rewriteParityHashes(src, resolvers({ getFooTemplate: NEW }))).toThrow( + /Rewrote 1 of 2 64-hex literals/ + ); + }); + + // A workflow added to getSkillTemplates() but never pinned is invisible to the + // checks above (they only see pins that exist) and to the parity test (it + // compares only the entries it lists), so it would ship with no golden hash + // while the run reported success. + it('throws when the registry deploys a skill that is not pinned', () => { + const src = `const M = {\n 'openspec-foo': '${OLD}',\n};\n`; + + expect(() => + rewriteParityHashes( + src, + resolvers({}, { 'openspec-foo': OLD, 'openspec-brand-new': NEW }, [ + 'openspec-foo', + 'openspec-brand-new', + ]) + ) + ).toThrow(/openspec-brand-new/); + }); + + it('names every unpinned skill, not just the first', () => { + const src = `const M = {\n 'openspec-foo': '${OLD}',\n};\n`; + + expect(() => + rewriteParityHashes( + src, + resolvers({}, { 'openspec-foo': OLD }, ['openspec-foo', 'openspec-aaa', 'openspec-bbb']) + ) + ).toThrow(/openspec-aaa[\s\S]*openspec-bbb/); + }); + + it('accepts a registry fully covered by pins', () => { + const src = `const M = {\n 'openspec-foo': '${OLD}',\n};\n`; + const result = rewriteParityHashes( + src, + resolvers({}, { 'openspec-foo': NEW }, ['openspec-foo']) + ); + + expect(result.moved).toEqual(['openspec-foo']); + }); + + it('names both causes when the counts disagree, since either is possible', () => { + const src = `const UNRELATED = '${OTHER}';\nconst M = {\n getFooTemplate: '${OLD}',\n};\n`; + + expect(() => rewriteParityHashes(src, resolvers({ getFooTemplate: NEW }))).toThrow( + /widen them[\s\S]*not a pin/ + ); + }); +}); diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 69fdbdc114..2d41198058 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -164,6 +164,16 @@ describe('skill templates split parity', () => { expect(actualHashes).toEqual(EXPECTED_GENERATED_SKILL_CONTENT_HASHES); }); + // The assertion above only compares the skills this file already lists, so a + // workflow added to getSkillTemplates() but never pinned here would ship with + // no golden hash and nothing would fail. Pin the registry itself. + it('pins every skill the production registry deploys', () => { + const pinned = GENERATED_SKILL_FACTORIES.map(([dirName]) => dirName).sort(); + const deployed = getSkillTemplates().map(({ dirName }) => dirName).sort(); + + expect(pinned, 'add the new skill to GENERATED_SKILL_FACTORIES and EXPECTED_GENERATED_SKILL_CONTENT_HASHES').toEqual(deployed); + }); + // Iterating the production registries (not a local list) means a newly // added workflow is covered automatically; the full-constant containment // check fails if any template's interpolation drifts.