chore(scripts): add a parity-hash regeneration helper - #1416
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Node.js script and npm command to regenerate deterministic template parity hashes, validate missing or renamed entries, rewrite the parity test, and document the workflow. ChangesParity hash regeneration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Script as regen-parity-hashes.mjs
participant Exports as dist skill template exports
participant Helpers as getSkillTemplates/generateSkillContent
participant Test as skill-templates-parity.test.ts
Script->>Exports: Import template factories
Script->>Helpers: Generate skill content
Helpers-->>Script: Return generated content
Script->>Test: Rewrite pinned hashes
Script-->>Test: Report changed or moved entries
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/README.md`:
- Around line 53-55: Update the parity test documentation text to use the
configured American-English spelling, replacing “afterwards” with “afterward”
while preserving the surrounding meaning.
In `@scripts/regen-parity-hashes.mjs`:
- Around line 62-67: Update the parity validation around contentByDir and moved
to compare getSkillTemplates() registry keys with the pinned keys in both
directions, detecting both missing registry entries and stale pinned entries.
Fail before writeFileSync whenever the sets differ, so the script cannot report
success without complete parity coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc59bc4f-9634-4c13-96e9-fc3bcedf27e8
📒 Files selected for processing (3)
package.jsonscripts/README.mdscripts/regen-parity-hashes.mjs
| 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the configured American-English spelling.
Replace afterwards with afterward.
🧰 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/README.md` around lines 53 - 55, Update the parity test documentation
text to use the configured American-English spelling, replacing “afterwards”
with “afterward” while preserving the surrounding meaning.
Source: Linters/SAST tools
21df69a to
d8951fc
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The helper still reports “nothing to update” when getSkillTemplates() contains a new directory absent from both EXPECTED_GENERATED_SKILL_CONTENT_HASHES and GENERATED_SKILL_FACTORIES; I reproduced that at this exact head by adding an unpinned registry entry to the built module. Please compare registry keys and pinned generated-content keys in both directions before writing, and add the missing-entry regression so success guarantees complete parity coverage.
skill-templates-parity.test.ts pins a SHA-256 per workflow template so an
unintended template edit fails loudly. The cost lands on every intended
edit: the pinned hashes go stale, and because all 37 live in two maps in
one file, two branches editing different templates collide there on rebase.
Resolving that means hand-editing 64-character hashes, which is where
transcription mistakes come from - and the test proves a hash matches its
source, never that the source is right, so a bad value regenerated over a
bad merge passes CI in silence.
Recompute every pinned hash from the built dist/ and rewrite the map in
place, reporting which entries moved. The skill-directory mapping comes
from getSkillTemplates(), the same helper the skills.sh generator uses, so
adding a workflow needs no second list here; function labels resolve
dynamically against the module exports, so there is no hard-coded list at
all.
"Nothing to update" has to mean it, so four things abort the run without
writing:
- dist/ missing or 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, from a renamed or deleted
template
- a pinned hash whose line the patterns do not recognise, counted by
comparing 64-hex literals found against literals rewritten; the count
uses a deliberately broader pattern so it is a real cross-check rather
than a restatement of the same patterns
- a skill the registry deploys that nothing pins, compared in the other
direction: pins-to-registry only sees pins that already exist
That last direction closes a hole that predates this script. A workflow
added to getSkillTemplates() but never pinned was invisible to the parity
test too, which compares only the entries it already lists - so it shipped
with no golden hash while everything reported success. skill-templates-
parity.test.ts now pins the registry itself, so CI catches it whether or
not anyone runs this script.
The rewriting lives in parity-hash-shared.mjs, following the split between
generate-skillssh.mjs and skillssh-shared.mjs, so those guards can be
exercised against fabricated input. Running the script for real from a test
would rewrite the repository's own parity test file mid-suite. Each case in
parity-hash-shared.test.ts was mutation-checked: removing the guard it
covers makes it fail.
The script cannot silently emit a wrong hash: the parity test recomputes
the same values independently and compares, so a drift between the two
copies of stableStringify fails the test. The test stays the authority.
Dev tooling only. scripts/ is not published (package.json files ships just
scripts/postinstall.js), no src/ is touched, and no runtime behaviour
changes - hence no changeset.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d8951fc to
c6044dc
Compare
|
Reproduced and fixed — thanks, this was a real hole and worse than the report suggests. At Fixed in both places: 1. The helper now compares both directions. Pins→registry already threw ( exit 2. 3. Three regression tests in Registry and pins are 12/12 with both difference sets empty today, so the bidirectional check adds no false positives. Full suite: 2163 passed (111 files). Head is now |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Approved at exact head c6044dc. The registry-to-pin guard now works in both directions and is pinned by a direct parity test; build, 26 focused tests, clean regeneration, the missing-entry mutation reproduction, and the full exact-head CI matrix all pass.
2 participants
Regenerating the golden template hashes by hand is error-prone. This adds one command that does it for you.
pnpm build && pnpm regen:parity-hashesWhy:
skill-templates-parity.test.tspins a SHA-256 per workflow template. Every intended template edit leaves those pins stale, and all 37 live in one file — so branches editing different templates collide there on rebase. Across the 15 PRs merged today, eight hit that collision, and fixing it meant retyping 64-character hashes by hand. The test only proves a hash matches its source, never that the source is right, so a mistyped value passes CI silently.What it does: recomputes every pinned hash from the build, rewrites them in place, and prints which ones changed.
nothing to updatealways means it — the run aborts, writing nothing, if:dist/is missing or older thansrc/That last one also closes a pre-existing gap: a new workflow added to
getSkillTemplates()but never pinned was invisible to the parity test too. It now fails in CI whether or not anyone runs this script.Safe to merge: dev tooling only. Nothing is published (
scripts/isn't inpackage.jsonfiles), nosrc/or runtime code is touched. Full suite: 2163 passed (111 files).Tested: 14 tests on the rewrite logic, all mutation-checked — removing any guard makes its test fail. They run against fabricated input, never the real parity file.
🤖 Generated with Claude Code