Repository navigation
chore: hold every declared package to the workflow steps that ship it - #838
Conversation
PNPM_PUBLISHED says which packages publish, and a test holds that to the release configs, but nothing held it to the workflows that act on it. So a package could be declared publishable and publish nowhere with CI green, which is what #706 was. The new release-wiring conformance check reads the roster and asserts, for each package, a semantic-release step in ci.yml's publish job, its coverage directory in Archive Coverage, a consumer-sbom matrix leg, and a place in verify-provenance's npm install. Each message names the file, the package and the step to add. The release step is matched on its working directory, not its name, since a copied step renamed but not retargeted still releases the package it was copied from. The root declares no YAML dependency, so the workflows are read by a small indentation reader in the file, for the reason parseWorkspacePackages already gives. A shape it cannot read comes back as missing, which fails the check rather than passing it. Refs #710
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe conformance runner adds a ChangesRelease Wiring Conformance
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Other · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Runner as Conformance runner
participant Check as checkReleaseWiring
participant Roster as PNPM_PUBLISHED
participant CI as ci.yml
participant SupplyChain as supply-chain.yml
participant Validator as releaseWiringViolations
Runner->>Check: Run release-wiring check
Check->>Roster: Read published packages
Check->>CI: Read CI workflow
Check->>SupplyChain: Read supply-chain workflow
Check->>Validator: Validate packages against workflow wiring
Validator-->>Runner: Return violations
Merge Risk: ⚪ Minimal · up to The change validates declared packages against release and supply-chain workflow rosters without changing publishing behavior. No merge-blocking issue remains; merge after normal checks pass. 🚥 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 |
Preview DeploymentPreview URL: https://8c0e4926.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | Archive Coverage is located with .find over every job's steps, so a second step by that name added earlier in the file would become the one validated while the real archive step goes unchecked. |
scripts/check-conformance.mjs:4446 |
| 2 | 🔵 Advisory | Robustness | npmInstallNames misses an npm install nested in an if/for/while compound, and skips the whole line when a trailing comment contains an apostrophe (tokenize throws). Both fail closed — verified empirically. |
scripts/check-conformance.mjs:4350 |
| 3 | 🔵 Advisory | API | The coverage assertion couples "declared in PNPM_PUBLISHED" to "produces a jest coverage directory". A future published package with no test suite would be told to add a path entry that never exists. |
scripts/check-conformance.mjs:4461 |
Overall: Sound, and unusually well evidenced for a check of this kind. The design decision that carries it — match the release step on working-directory rather than the step name, and let any shape the hand-rolled reader cannot parse read as missing — is the right polarity, and I confirmed it holds on the real files rather than only on fixtures. The riskiest part is the bespoke YAML reader, since it is a second parser of a format nothing else in scripts/ parses; I exercised it against both real workflows and it reproduced the publish job's step list, working directories, literal run: blocks, the with.path block scalar, and the strategy.matrix.include legs correctly. A human should focus on whether the four asserted surfaces are the right four — the PR body names three deliberate exclusions (the closure table, the SECURITY.md rosters, per-run success) and I verified the first: supply-chain.yml:86-89 genuinely has no eslint-plugin row, so asserting it today would red this PR.
Residual risk:
- Presence, not effect. An
if: false, acontinue-on-error: true, or a--dry-runon aSemantic Release (<pkg>)step satisfies the check. Verified: injectingif: falseinto the bulma-ui release step still returns 0 violations. The header comment states this contract explicitly ("It asserts that the wiring exists, not that it is correct"), so it is accepted rather than missed — but it is the way #706 could recur in a shape this check cannot see. - Word-match false pass on surface 1 (open). The release-step filter is a word match for
semantic-releaseover the comment-strippedrun:. A step withworking-directory: <pkg>whose script merely mentions the word outside a command position —echo "semantic-release will run"— would register that package as released. The symmetric case is already closed on surface 4 (there is a test that a name in anechois not an install); surface 1 has the plugin-path test (@semantic-release/npm) but no echo test. Contrived, and it needs a deliberate workflow edit, so not posted as a finding. - Reader shapes fail closed, confirmed. Folded (
>), flow ([a, b],{k: v}) and alias (*x) values returnnullfromyamlScalar, and a flow-style matrix produces the single "no legs could be read" violation rather than a pass — pinned by tests and re-checked by hand. - Shell-parse gaps fail closed, confirmed.
npm install a && npm install byields both names (tokenize splits&&into two operator words),if true; then npm install a; fiyields nothing, and an install line whose trailing comment holds an apostrophe yields nothing. Each miss produces a violation, never a silent pass. The spurious captures I could find (a--prefixvalue is read as a package name) can only mask a real gap if a package were literally named after a flag value. - Surfaces outside the diff. Asserting a fifth surface (the measured closure table) is blocked on a measurement that must land in a human-authored workflow file, and the roster prose in
SECURITY.mdhas no machine-readable shape yet. Both are named in the PR body and neither is regressed here. - Environment note, not a finding. Locally
version-regressionfails (shallow clone,--allow-untaggedterritory) and a block ofscripts/*.test.mjsassertions fail on an absentbulma-ui/dist. Both are pre-existing and unrelated; all 29release-wiringtests and every other conformance check pass on this branch.
🏄 Clean little set on this one — the reader paddles out, actually catches the real workflows instead of just the fixtures, and every wipeout it can have dumps it on the safe side of the break. Good to go.
…first The coverage step is found by name across every job, and taking the first match meant a second step by that name earlier in ci.yml was validated while the one that really archives coverage went unchecked. More than one now fails the check, naming the jobs they are in. The release steps have no such hole, since they are collected rather than looked up, so a duplicate adds a directory instead of displacing one. A test pins that. npmInstallNames also names its known misses, both of which read as not installed. Refs #710
|
On the three advisories:
|
|
deep-review: fresh. 11563d4 changes how the Archive Coverage step is found; review the check as it stands now. |
There was a problem hiding this comment.
Deep review (verify) — 0 resolved · 0 open
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| — | — | — | No open threads to settle — the prior deep review posted zero inline comments. | — |
Overall: This pass settled threads and reviewed no commits. The earlier deep review (marked claude-deep-review, submitted 2026-09-30T02:14Z) raised 0 blocking findings and 3 advisories, and advisories are summary rows by construction — so it left no inline threads behind, and a verify pass has nothing to re-examine. Nothing was resolved and nothing remains open.
For the maintainer — the steer asked for a run this one could not give. The deep-review: fresh comment was read (it arrived as this run's focus), but the run was still dispatched in verify mode, so 11563d4a has not been reviewed by anyone. Per CLAUDE.md a fresh steer stays selected once it is the newest triage+ deep-review: comment, so removing and re-adding the deep-review label should start a genuine fresh pass over the current code. Re-adding a label that is already present emits no labeled event — it has to come off first.
Residual risk: Not assessed — a verify pass raises no findings and re-reviews no code, so the residual-risk hunt from the prior review stands unchanged and un-rechecked against 11563d4a. The three advisories answered in the comment above (duplicate Archive Coverage step, npmInstallNames shell-parse gaps, coverage assumed for every published package) were not re-verified here; the first is the one 11563d4a claims to close, and confirming that claim is exactly what the fresh run is for.
🏄 Nothing to settle out here, dude — the last session left a totally clean lineup, no loose threads bobbing around. But that "fresh" call got routed to the verify break, so the new commit's wave is still unridden. Pop the label off and back on and let a real session paddle out.
Preview DeploymentPreview URL: https://fe275024.bestax.pages.dev |
|
deep-review: fresh 11563d4 changes how the Archive Coverage step is found, so review the check as it stands now. |
Preview DeploymentPreview URL: https://047c9d85.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 4 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | The ambiguity branch refuses to check coverage at all when two steps share the name — even when both list every directory. Verified: adding a second complete Archive Coverage step to the real ci.yml returns the ambiguity violation, not a pass. Deliberate ("will not guess"), and it fails loudly, but it means the only legal shape is exactly one such step. |
scripts/check-conformance.mjs:4461 |
| 2 | 🔵 Advisory | Robustness | Presence, not effect (the header comment's own contract): if: false, continue-on-error: true and --dry-run on a Semantic Release (<pkg>) step all satisfy surface 1. Verified empirically against the real ci.yml. |
scripts/check-conformance.mjs:4430 |
| 3 | 🔵 Advisory | API | The coverage assertion still couples "declared in PNPM_PUBLISHED" to "produces a jest coverage directory". A future published package with no test suite would be told to add a path entry for a directory that never exists. |
scripts/check-conformance.mjs:4478 |
| 4 | 🔵 Advisory | Correctness | Comment imprecision, safe direction: the new npmInstallNames "known misses" note says an install nested in if/for/while reads as not installed, but if true; then npm install a && npm install b; fi yields ["b"] — the miss is the first command after then/do, not every nested install. The note understates coverage, so nothing fails open. |
scripts/check-conformance.mjs:4346 |
Overall: Sound, and 11563d4a closes the hole the previous pass flagged cleanly — the coverage step is now collected across every job and more than one is a violation naming the jobs, which strictly dominates the old .find() (a decoy earlier in the file used to be validated while the real archive step went unchecked; it now reds). I re-ran this rather than reading it: all 32 tests in scripts/release-wiring.test.mjs pass, pnpm check:conformance is green on all 23 checks including release-wiring, and eslint/prettier are clean on both changed files. I also swept the rest of the check for the same class of defect the steer names — a lookup that silently validates the wrong thing — and the remaining by-key lookups (ciJobs.get('publish'), supplyJobs.get('consumer-sbom'), supplyJobs.get('verify-provenance'), and yamlMap's first-wins on duplicate keys) are all keyed on identifiers whose duplicates GitHub's own workflow parser rejects, so there is no second instance of the hole. The human should focus on advisory 1 — whether "exactly one Archive Coverage step, forever" is the constraint you want ci.yml held to.
Residual risk:
- Duplicate-name hole elsewhere — refuted. Release steps are collected into a
Setof working directories, so a duplicate adds a directory rather than displacing one; the new testtwo release steps for one directory do not stand in for a missing onepins it, and the sbom legs and the provenance install roster are collected the same way. The only remaining by-name lookup is the coverage step, which this commit converts from "take the first" to "report them all". - Word-match false pass on surface 1 (narrowed, still open). The filter is
/(^|\s)semantic-release(\s|$)/over the comment-strippedrun:. I retested the two shapes I named last time against the realpublishjob:echo "semantic-release will run"does not match (the opening quote is not whitespace), and a plain-scalar trailing comment is stripped byyamlScalar. What still passes is a trailing comment inside arun: |block (shellCodestrips only whole-line comments) and a bare unquotedecho semantic-release. Both need a deliberate, nonsensical edit to a human-authored workflow, so not posted. - Reader shapes fail closed — re-confirmed on the real file. A flow-sequence
path:produces five "has no<dir>/coverage" violations; a renamed coverage step produces the "update COVERAGE_STEP" message; folded/flow/alias values returnnullfromyamlScalar. A column-0 line inside arun: |block would break the job map — and that too fails closed, becausepublishwould then be unfindable. npmInstallNamesmisses fail closed — re-confirmed.if true; then npm install a; fireads as nothing,for p in a b; do npm install $p; donereads as nothing, and a trailing comment holding an apostrophe reads as nothing (tokenize throws, so the line is skipped). Every miss becomes a violation, never a silent pass.npm install --prefix /tmp/x astill captures/tmp/xas a name, which can only mask a real gap if a package were literally named after a flag value.- Surfaces not asserted (open, and named in the PR body). The measured closure table above
consumer-sbomhas noeslint-pluginrow — re-read atsupply-chain.yml:86-89and confirmed, so asserting it today would red this PR; and theSECURITY.mdrosters have no machine-readable shape. Neither is regressed here. - Environment note, not a finding. 18 of 999
scripts/*.test.mjsassertions fail locally on an absentbulma-ui/dist(constants-subpath,color-tuple-css,helper-prop-collisions,publishable-manifests,eslint-plugin-docs). Pre-existing and unrelated — everyrelease-wiringtest passes.
🏄 Last set had one rip current and this commit roped it off — no more quietly checking the decoy wave while the real one closes out behind you. Every wipeout it can still take dumps it on the safe side of the break. Good to go.
|
On the fresh review's advisories:
|
Preview DeploymentPreview URL: https://302d02cc.bestax.pages.dev |
allxsmith
added
the
needs-human-review
📸 Story screenshots at handoff —
|
Preview DeploymentPreview URL: https://99b7835a.bestax.pages.dev |
|
🎉 This PR is included in version 5.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Closes #710.
PNPM_PUBLISHEDsays which packages publish, andpublishable-manifests.test.mjsholds that to the release configs. Nothing held it to the workflows that act on it, which is how #706 happened: the ESLint plugin was declared, CI was green, and it published nowhere.This adds a
release-wiringconformance check. For every package inPNPM_PUBLISHEDit asserts:ci.yml'spublishjob, matched onworking-directoryrather than the step name, since a copied step that was renamed but not retargeted still releases the package it came from;<dir>/coveragein theArchive Coveragepath list;supply-chain.yml'sconsumer-sbommatrix, by the name a consumer installs;verify-provenancejob'snpm install, whichverify-attestation.mjsreads its roster back from.Each message names the file, the package and the step to add. A renamed job or step is reported with the constant to update rather than passing.
The root declares no YAML dependency, so the workflows are read by a small indentation-based reader in the file, for the reason
parseWorkspacePackagesalready gives. It reads block mappings, block sequences, and plain, quoted and literal scalars. Anything else reads as missing, and since the check only asserts presence, that fails the check rather than passing it.scripts/release-wiring.test.mjsdrives the rule on fixtures and runs it against the real workflows, including removing one package from each surface of the real files to show each one is really being read.Also here:
CONTRIBUTING.mdsaidpnpm alldoesn't run the conformance checks. It has since #686, so that sentence goes.Not in this PR:
consumer-sbom. It has no row for the plugin today, so asserting it would fail here. The row needs a measurement from a dispatch and lives in a workflow file, which is human-authored, so the assertion follows once the row lands.SECURITY.mdand the security guide. They are prose, and need a shape a check can read first.pnpm allpasses locally.Summary by CodeRabbit