-
Notifications
You must be signed in to change notification settings - Fork 4.3k
chore(scripts): add a parity-hash regeneration helper #1416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+466
−0
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string>, | ||
| * 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); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)), | ||
| ]) | ||
| ); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| 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'); | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the configured American-English spelling.
Replace
afterwardswithafterward.🧰 Tools
🪛 LanguageTool
[locale-violation] ~54-~54: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...duce a wrong value. Always run the test afterwards; it, not this script, is the authority....
(AFTERWARDS_US)
🤖 Prompt for AI Agents
Source: Linters/SAST tools