Repository navigation
ci: publish the eslint plugin, and gate its generated metadata - #709
Conversation
`@allxsmith/eslint-plugin-bestax` has sat on npm at `0.0.0-development` since #686, because the release wiring is in `.github/` and was handed over as diffs rather than applied. The package builds, typechecks, tests and covers in CI and then publishes nowhere, with CI green the whole way — which is the silent failure #686's own description warned about. Closes #706. Four changes, and one deliberately held back. The release step runs after `Semantic Release (bulma-ui)`, which is load-bearing rather than tidy: the plugin declares `@allxsmith/bestax-bulma` in runtime `dependencies` and `pnpm publish` resolves `workspace:^` against `bulma-ui/package.json` at pack time, so running it first in a release bumping both computes the published range from the version bulma-ui had on the way in. A consumer resolving that range can then land on a version predating whatever the release added — for this plugin, the `./constants` subpath it imports. `gen:eslint-meta:check` existed in `package.json` and ran in no CI step, so the plugin's generated metadata had a staleness gate only inside a local `pnpm all`, unlike the catalog and MCP index which both have steps. It sits with them now, and root `CLAUDE.md` is updated: it said "no CI step yet" and its quality-gates roster named only two of the generated artefacts. The coverage path and the consumer-SBOM matrix leg are the other two. `check-consumer-sbom.mjs` needs nothing — no per-package roster, only a minimum, and the entry counts in the comments are explicitly not a gate. That leg resolves the `latest` dist-tag, which the placeholder already satisfies, so it works before the first real release. The provenance roster is NOT here, and the reason is narrower than "the version does not exist" — `latest` resolves fine. It is that the placeholder was published by hand and carries no attestation, so `npm audit signatures` would fail against it. It goes in after the first CI release, as #706 records. Every release step here shares one property worth stating: none is guarded, so a failure in any of them skips the MCP index restamp below and leaves `main` carrying a stale index (#521). This adds one more step that can do that rather than a new way for it to happen. On the security contract in .github/CLAUDE.md, for a reviewer who should not have to re-derive it: I1 is untouched, since the release job holds the app token and the committer identity and no model-auth token. I2 is untouched, since nothing here posts. Against the blocking-exposure list there is no new trigger, no looser condition, no new scope, PAT, endpoint or PR checkout — the new step runs in the existing release job on the credentials already there for its siblings — and no new action `uses:`, so the repo-wide SHA pin is unaffected. What is genuinely new is one more package leaving for npm, which is the point, and it goes through the trusted publisher already configured against the placeholder rather than any credential added here. The SBOM leg joins a job that already enforces egress at block under `contents: read`.
|
Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
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 (8)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughCI now validates ESLint plugin metadata, archives its coverage, releases the plugin through semantic-release, suppresses release annotations, and includes the package in consumer SBOM generation. ChangesESLint Plugin Release Integration
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions CI
participant Release as semantic-release
participant NPM as npm registry
participant SBOM as Consumer SBOM job
CI->>Release: Run semantic-release in eslint-plugin
Release->>NPM: Publish `@allxsmith/eslint-plugin-bestax`
SBOM->>NPM: Resolve the published package
SBOM->>SBOM: Generate the consumer SBOM
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 8 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 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://ddd51627.bestax.pages.dev |
|
deep-review: fresh — please read 3831f24 |
![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 · 4 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | successComment: false is deprecated in the pinned @semantic-release/github@12.0.9 — it logs a DEPRECATION line and names successCommentCondition as the replacement. Correct today; a future major removing it reinstates exactly the commenting this block exists to prevent. |
eslint-plugin/release.config.js:90 |
| 2 | 🔵 Advisory | Security | A fifth package now publishes while verify-provenance's roster still names four, so that job stays green without ever checking this plugin's attestation. Deliberate and tracked on #706, but nothing forces the follow-up. |
.github/workflows/supply-chain.yml:1031 |
| 3 | 🔵 Advisory | Correctness | PR body (for the maintainer, not the fix agent): the reason given for deferring the roster names the wrong step. npm audit signatures stays green when provenance is merely absent — this workflow says so itself at line 1036. The step that would fail on the placeholder is verify-attestation.mjs, which fails closed on a missing attestation. The decision is right; the mechanism cited is not. |
.github/workflows/supply-chain.yml:1036 |
| 4 | 🔵 Advisory | Robustness | Point-in-time counts, one row for the lot: consumer-sbom-meta.mjs still says "the other three" legs twice, supply-chain.yml:687 still says "any of four matrix legs", and the PR body's "one feat, three fix, one refactor" is well short of the eslint-plugin-scoped history on main. Deleting the numeral is the fix, not updating it. |
scripts/consumer-sbom-meta.mjs:28 |
Overall: The change is sound and I could not break it. I verified the
mechanical claims empirically rather than by reading: gen:eslint-meta:check
regenerates byte-identically against the committed metadata.ts (so the header
change in gen-eslint-meta.mjs and the artefact are in sync) and it runs fine
with bulma-ui/dist absent, which is what makes its placement before Build
safe; eslint-plugin/coverage really is produced, so the new Archive Coverage
path is not a no-op; pnpm exec semantic-release resolves from
eslint-plugin/; check:conformance is green and format:check passes; and
successComment: false / releasedLabels: false are both accepted by
12.0.9's canBeDisabled validator, with success.js short-circuiting the
entire PR/issue walk on the first — so the first release genuinely cannot
comment or label. The ordering rationale holds too: @allxsmith/bestax-bulma
really is workspace:^ in runtime dependencies, and the new step sits after
the bulma-ui release that rewrites that manifest. The riskiest part is the one
thing CI cannot exercise — consumer-sbom never runs on a pull_request, so
the new matrix leg is unverified until a dispatch. Look there first.
Residual risk:
- The new SBOM leg is untestable from this PR.
consumer-sbomfires only
onrelease,scheduleandworkflow_dispatch, so nothing here proves the
leg works. Refuted as far as it can be statically: the slug follows the only
other scoped entry,structuralNames()reconstructs both
bestax-consumer-closure-allxsmith-eslint-plugin-bestaxand
consumer-closure:@allxsmith/eslint-plugin-bestaxexactly as the workflow
writes them,0.0.0-developmentpassesassertVersion's SEMVER (prerelease
is in the grammar), and the artifact basename still matches the
bestax-*sbom*globsign-sbom/attach-sbomdownload by. Open: a
dispatch is the only way to confirm. - The placeholder tarball's manifest. If the hand publish had been an
npm publish,workspace:^would have reached the registry verbatim and
npm installin the new leg would fail three times and red it (#412's
shape). Largely refuted:require-pnpm-publish.mjsis wired to both
prepackandprepublishOnlyand refuses an npm packer bynpm_execpath,
and SECURITY.md records a hand publish, which that guard permits only for
pnpm. Not fully closed —--ignore-scriptsskips the hook, as the script's
own header says, and I had no registry access to read the published manifest. - Closure floor.
MIN_EXPECTED_PACKAGESis 3 distinct catalogued names.
Refuted: npm auto-installs peers (that is why bulma-ui's measured closure is
five — react, react-dom and scheduler are peers), so this leg resolves the
plugin plus bulma-ui's five pluseslint@^10's tree. Expect the largest
closure of any leg, likely overtaking bestax-migrate's — accurate for what
npm resolves, and consistent with this job's documented refusal of
--omit=peer, but worth knowing before reading the document. - The new CI gate reds other branches.
gen:eslint-meta:checknow runs on
every PR, so any open branch that changed a bulma-ui TSDoc deprecation or
text alias without regenerating fails on a file it did not touch. That is the
gate working as intended; it is the same class as #518/#520 and worth
expecting rather than debugging. - Trust boundary: unchanged. The release step adds no trigger, no
if:, no
scope, no PAT, no PR checkout, and no newuses:— it runs in the existing
publish job onsteps.app-token.outputs.token, the credential already there
for its four siblings, and holds no model-auth token (I1 untouched). The SBOM
leg joins a job already ategress-policy: blockundercontents: readwith
the assertion step in place, and installs with--ignore-scripts, so nothing
new leaves the job and nothing new executes in it (I2 untouched).
🏄 Total cruiser, dude — that plugin's been sitting on the beach at
0.0.0-developmentsince #686 and this one finally paddles it out past the
break. Muting the comments on the first drop is the classy move; just don't
forget to come back for the provenance roster once the tag's in the water.
Good to go. 🤙
`successComment: false` is deprecated in the pinned `@semantic-release/github` and names its own replacement in the warning it logs. Both spellings skip the PR and issue walk in 12.0.9 — `success.js` branches on each separately — but a future major removing the deprecated one restores the default template and reinstates exactly the commenting that block exists to prevent, silently, on a version bump. `successCommentCondition: false` is the spelling that survives. `releasedLabels: false` stays. It is redundant while the comment skip stands, because 12.0.9 applies the label inside the comment's own try block so no comment means no label — but that coupling is an implementation detail of one version rather than a contract, and this is the option that says what is wanted if a later version separates them. Three counts in prose went stale the moment a fifth leg existed: two in `consumer-sbom-meta.mjs` describing what the other legs do, and one in the `sign-sbom` comment. `fragile-prose` scans `.github/**/*.yml` and not `.mjs`, so only one of the three would ever have been caught. Also corrected in the PR body: the reason for holding the provenance roster back. `latest` resolves, so it is not a missing version, and `npm audit signatures` stays green when provenance is merely absent — that job's own comment says so. The step that fails is `verify-attestation.mjs`, which opens the attestation and fails closed when there is none, and the hand-published placeholder has none. Third mechanism cited for the same correct decision.
|
Round 1 handled at 1 — fixed, and the deprecation is the interesting part.
3 — fixed, and this is the third mechanism I have cited for the same decision. You are right and the workflow says it outright: 4 — fixed. Two counts in 2 — accepted, not fixed. A fifth package publishes while On the residual you flagged as the riskiest: agreed that |
|
deep-review: fresh — please read 7b6b2ea |
Preview DeploymentPreview URL: https://6ba59bb5.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 4 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Security | A fifth package now publishes while verify-provenance's install roster still names four, so that job stays green having never opened this plugin's attestation. The deferral is correct — I read the packument, and the placeholder carries dist.signatures but no provenance, so verify-attestation.mjs would fail closed on it — but nothing forces the follow-up beyond #706 step 3. |
.github/workflows/supply-chain.yml:1029 |
| 2 | 🔵 Advisory | Robustness | The new release step sits immediately above the unguarded MCP restamp and is the least-proven of the five (first OIDC publish under this name). A failure there in a run that also released bulma-ui leaves main carrying a stale MCP index (#521) and reds every open branch. Accepted and documented; relocating it below the restamp would cost that step its "runs last on purpose" property, so it is not a free fix. |
.github/workflows/ci.yml:367 |
| 3 | 🔵 Advisory | Robustness | The failure class behind #706 still has no gate. Nothing ties a package's release.config.js — or check-conformance's own declared pnpm-publish roster — to a Semantic Release (<pkg>) step, a coverage path, or a consumer-SBOM leg, so the next package can repeat this exactly: green CI, published nowhere. This PR fixes the instance, not the class. |
scripts/check-conformance.mjs:2047 |
| 4 | 🔵 Advisory | Robustness | Point-in-time evidence, one row for the lot. The diff adds a tally to a code comment ("PR #300 carries four such notices" — true today; I checked that the four notices are there) while the same commit deleted three of that class elsewhere, and the sweep missed verify-oidc-context.mjs:3 ("which is all four of them" — five packages publish with pnpm publish now) and the "four closures" measurements at check-consumer-sbom.mjs:311,442. The consumer-SBOM closure table gains no row for the new leg (already recorded on #706), and the PR body's "conformance 15/15" reads 21 checks today. Move the evidence to the issue and link it. |
eslint-plugin/release.config.js:81 |
Overall: Sound, and I could not break it. The two things this PR turns on are the ones I verified hardest, both against sources rather than by reading the diff: successCommentCondition: false genuinely closes the whole-history comment walk in the pinned 12.0.9, and the new metadata gate is real and correctly placed. The riskiest part is the one thing no CI event can exercise — the first release itself, where the OIDC trusted publisher for this name is exercised for the first time and a rejection would land after the tag is already pushed. Look there first; everything else is bounded.
Residual risk:
successCommentCondition: falsesilently reverting to "comment". Refuted at the source, three ways:verify.js'scanBeDisabledacceptsfalsefor that key;resolve-config.jspasses it through verbatim, with noisNildefault that could swallow it (contrastlabels/releasedLabels, which need an explicit=== falsearm); andsuccess.js:68takes its ownelse if (successCommentCondition === false)branch before the associated-PR GraphQL walk. The tempting bug —falsefalling into thecanCommentOnIssueternary atsuccess.js:196, where falsy means comment — cannot happen, because that ternary is reachable only inside theelse.releasedLabels: falseis redundant-but-harmless exactly as the comment says: the label POST atsuccess.js:223sits inside the comment's owntry.- The new SBOM leg, unexercisable from a PR. Pushed past static checking this round by reading the registry: the placeholder's manifest carries
dependencies: {"@allxsmith/bestax-bulma": "^5.15.2"}— soworkspace:^was resolved at pack time, and #412's uninstallable shape is refuted at the registry rather than inferred from theprepackguard — andpeerDependencies: {eslint: "^10.0.0"}has a solution (eslint@10.11.0islatest), so npm's auto-peer install cannot ERESOLVE.0.0.0-developmentmatchesassertVersion's anchored SEMVER, and the closure clearsMIN_EXPECTED_PACKAGES(3) with room. Open: only a dispatch proves the syft/guard path end to end. - The first release failing after the tag is pushed. Open by construction and unchanged by this PR:
verify-oidc-context.mjsasserts that an OIDC context exists, not that npm accepts this repo as trusted publisher for this name, and everypreparestep runs before anypublish. Bounded by placement — the step is last of the five, so a rejection costs no other package's release. - The new gate failing open. Refuted:
&&-chained so any non-zero aborts, the generator throws on an empty table or an anchor that stopped matching, and--intent-to-addcovers a newly added file. I confirmed it passes on a clean tree withbulma-ui/distabsent, which is what makes its placement beforeBuildsafe. I could not stage a doctored artefact in this sandbox, so the fail path rests on shape-identity withgen:mcp:checkrather than on a run. - The ordering rationale. Holds: bulma-ui's
@semantic-release/npmprepare writes the new version intobulma-ui/package.jsonbefore the plugin's publish step reads it, and a failure in any earlier release step aborts the job, so the published range can never be computed from a stale version. - Trust boundary: unchanged. No new trigger, no looser
if:, no new scope, PAT, endpoint,uses:or PR checkout — the release step runs in the existing publish job on the App token already there for its four siblings, and holds no model-auth token (I1 untouched); the SBOM leg joins a job already ategress-policy: blockundercontents: readthat installs with--ignore-scripts(I2 untouched). The one genuinely new thing leaving the job is a package going to npm, which is the point.
🏄 Clean set, this one. The plugin's been bobbing out back at
0.0.0-developmentsince #686, and this finally drops in on it — swapping tosuccessCommentConditionbefore paddling is what keeps the first ride from spraying a few hundred old PRs sitting in the lineup. Good to go; just circle back for the provenance roster once the tag's in the water. 🤙
The comment justifying the suppressed release notices carried a count of how many notices one old PR has — added by the same commit that deleted three counts of exactly that class elsewhere. The argument does not need the number, and #706 holds the evidence with instructions for looking. Three more of the same, two of which the earlier sweep missed: `verify-oidc-context.mjs` said its roster "is all four of them", which a fifth pnpm-published package falsifies, and `check-consumer-sbom.mjs` measured entries "across the four closures" twice. The class this instance belongs to is now #710: nothing ties a package declared in `PNPM_PUBLISHED` to a `Semantic Release (<pkg>)` step, a coverage path, or a consumer-SBOM leg, so the next package added can repeat #706 with CI green throughout. It wants writing once the plugin has published, because the fourth item in that list — the provenance roster — is deliberately incomplete until then, and a check covering it would red this branch for doing the right thing.
|
Round 2 handled at 4 — fixed, and the hit lands. The same commit that deleted three counts added one: the comment justifying the suppression carried a tally of how many notices an old PR has. Gone; the argument never needed the number, and #706 holds the evidence with instructions for looking. Plus the two the sweep found — 3 — filed as #710 rather than fixed here. You are right that this PR fixes the instance, not the class, and I am not adding it to this branch, and the reason is not size. A fourth item belongs in that list — 1 — standing, and thank you for closing it at the registry. You read the packument and confirmed the placeholder carries 2 — standing, accepted. The step is the least-proven of the five and sits above the unguarded restamp. I agree relocating is not free: below the restamp it loses the "after bulma-ui" property that the ordering depends on. It is documented where it sits. On the residual you called riskiest — the first release exercising this name's trusted publisher for the first time, where a rejection lands after the tag is pushed — that is open by construction and I am not closing it in this PR. Worth noting what bounds it: the step is last, so a rejection costs no other package's release, and the recovery is a re-run once the publisher is fixed, not a yanked version. A |
|
deep-review: fresh — please read 8410fcc |
Preview DeploymentPreview URL: https://c7fa6bd5.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 4 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Security | A fifth package starts publishing while verify-provenance's install roster still names four, so that job stays green having never opened this plugin's attestation. The deferral is right and unavoidable here — verify-attestation.mjs fails closed and the hand-published placeholder carries no attestation — but the window it opens is exactly the one this workflow's own comment at line 1055 warns about: "add a fifth published package to one and not the other and it ships unverified, silently, green." Nothing but #706 step 3 closes it. |
.github/workflows/supply-chain.yml:1029 |
| 2 | 🔵 Advisory | Robustness | Point-in-time counts, one row for the lot. 8410fcc's sweep left three of the same class standing: supply-chain.yml:366 and consumer-sbom-meta.mjs:73 say "the one scoped package" / "the scoped package" where the matrix now has two, and consumer-sbom-meta.test.mjs:13 still reads "Three of the four packages". Deletion is the fix, not a fresh numeral. (For the maintainer, not the fix agent: the PR body's "conformance 15/15" reads 21 checks on this tree.) |
scripts/consumer-sbom-meta.mjs:73 |
| 3 | 🔵 Advisory | Robustness | The failure class still has no gate. Nothing ties PNPM_PUBLISHED to a Semantic Release (<pkg>) step, a coverage path, or a consumer-SBOM leg, so the next package can repeat #706 with CI green throughout. Now tracked as #710 and correctly deferred — a check covering the fourth surface would red this branch for deliberately holding that surface back. |
scripts/check-conformance.mjs:2071 |
| 4 | 🔵 Advisory | Robustness | The new step is the least-proven of the five (first OIDC publish under this name) and sits above the unguarded MCP restamp, so a failure there in a run that also released bulma-ui leaves main on a stale index (#521) and reds every branch cut afterwards. Accepted and documented in the step's own comment; moving it below the restamp would cost that step its "runs last on purpose" property, so it is not a free fix. |
.github/workflows/ci.yml:380 |
Overall: Sound, and I could not break it. I verified the mechanics rather
than read them: the whole suite is green on this tree after a build (turbo 8/8,
node --test scripts/*.test.mjs clean), check:conformance passes 21/21,
gen:eslint-meta:check regenerates metadata.ts byte-identically and reads
only bulma-ui/src plus typescript out of node_modules — which is what
makes its placement before Build safe — and CI's own Build and Test is
green on 8410fcc, so the new gate step really ran. The riskiest part is the one
nothing can exercise from a PR: the first release itself, where the trusted
publisher for this name is used for the first time and a rejection would land
after the tag is already pushed. Look there first; the rest is bounded by
placement.
Residual risk:
- The first release's whole-history notes hitting GitHub's release-body
limit. That would fail@semantic-release/github'spublishafter the
npm publish and the tag push — the expensive shape. Refuted by measurement
against this repo's own first releases: the1.0.0sections are 4,736 chars
(create-bestax, 2025-10-03), 25,775 (bestax-migrate, 2026-07-20) and 41,258
(bestax-mcp, 2026-08-12) — roughly 670 chars/day of growth, so a first release
today lands near 67k against a 125k ceiling. Comfortable now; not comfortable
forever, which is one more reason the bounding tag #706 wants is worth having. successCommentCondition: falsesilently reverting to "comment". Refuted
at the installed source:verify.js:41acceptsfalsethrough
canBeDisabled,resolve-config.jspasses it verbatim, andsuccess.js:68
takes its ownelse if (successCommentCondition === false)branch before
the associated-PR GraphQL walk — so thecanCommentOnIssueternary at line
196, where falsy means comment, is unreachable.releasedLabels: falseis
redundant-but-harmless exactly as the comment claims.- The new SBOM leg, unexercisable from a PR (
consumer-sbomfires only on
release/schedule/workflow_dispatch). Pushed as far as it goes without a
dispatch: I ranconsumer-sbom-meta.mjs specagainst the real leg and it
resolvesspec=@allxsmith/eslint-plugin-bestax,expect=on a schedule and
pins…@1.0.0/expect=1.0.0on its own release tag while leaving a sibling
leg onlatest;structuralNames()reconstructs both
bestax-consumer-closure-allxsmith-eslint-plugin-bestaxand
consumer-closure:@allxsmith/eslint-plugin-bestaxexactly as the workflow
writes them;0.0.0-developmentsits insideassertVersion's anchored
semver; and the closure (plugin + bulma-ui's five + eslint 10's tree) clears
the floor of 3 distinct names with room. Open: only a dispatch proves the
syft path end to end. - Ordering. Holds.
@allxsmith/bestax-bulmareally isworkspace:^in
runtimedependenciesand the plugin really does import
@allxsmith/bestax-bulma/constants(eslint-plugin/src/lib/values.ts:43),
which bulma-ui exports as a subpath;@semantic-release/npm'sprepare
writes the new version intobulma-ui/package.jsonbefore this step's
pnpm publishreads it, and any earlier release step failing aborts the job,
so the published range cannot be computed from a stale version. Inside the
plugin, publish order isnpm(no-op) →exec(pnpm publish) →github,
so the release event that pins this leg's SBOM fires after the tarball is on
the registry. - The tarball shipping empty. Refuted:
files: ["dist"], and root
pnpm run buildisturbo run build, which includes
@allxsmith/eslint-plugin-bestax(buildistsc,outDir: ./dist) and
orders bulma-ui first viadependsOn: ["^build"]. TheBuildstep runs
before every release step. - Trust boundary: unchanged, and checked against
.github/CLAUDE.mdon
mainrather than the checkout. No new trigger and no looserif:— the
publishjob is stillpush+refs/heads/mainonly; no new scope, PAT,
endpoint,uses:or PR checkout — the step runs in the existing job on
steps.app-token.outputs.token, the credential already there for its four
siblings, and holds no model-auth token (I1 untouched); the SBOM leg joins a
job already ategress-policy: blockundercontents: readthat installs
with--ignore-scripts(I2 untouched, and the one change that touches posting
removes it). The one genuinely new thing leaving the job is a package going
to npm, which is the point.
🏄 Third look at this set and it's still glassy, dude. The tally sweep in
8410fcc was the right call — the argument never needed the number — even if
three little ones are still bobbing out back. Publish-wise it's clean: comments
muted on the first drop, the plugin builds before it paddles, and all that's
left is circling back for the provenance roster once the tag's in the water.
Good to go. 🤙
Six more of the same class, in the files a fifth publishable package falsifies. Two described the scoped case as though there were one of it, one was a test NAME carrying a count of the other legs, one described what the job used to do to a fixed number of packages, and two more turned up adjacent to the first two once those were fixed. Three sweeps to get through this, which is the argument for the rule rather than against it: every one of these read as correct when written, and none of them is caught by anything — `fragile-prose` scans `.github/**/*.yml`, so of the nine counts this branch has now removed it would have flagged two. The one at `supply-chain.yml:271` stays: it carries `bestax:count-ok` with a reason, which is the mechanism for a count that is deliberately a measurement.
|
deep-review: fresh — please read bc47767 |
Preview DeploymentPreview URL: https://d611ee02.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 1 blocking · 4 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Coverage | bc47767b renamed this test to "every other leg" while the loop still names three of the four other legs. The fifth leg this PR adds is exercised by no installSpec assertion — in the one file whose header says it is the coverage for a job no PR event can run. |
scripts/consumer-sbom-meta.test.mjs:99 |
| 2 | 🔵 Advisory | Security | A fifth package starts publishing while verify-provenance's install roster still names four, so that job goes green having never opened this plugin's attestation. The deferral is forced — verify-attestation.mjs fails closed and the hand-published placeholder has no attestation — but the window is the one this workflow's own comment warns about, and only #706 step 3 closes it. |
.github/workflows/supply-chain.yml:1029 |
| 3 | 🔵 Advisory | Robustness | Point-in-time counts, one row for the lot. The third sweep is titled "finish", and the same class survives in files it did not open: consumer-sbom-meta.mjs:151 still reads "today for those three" in the very paragraph whose line above it fixed; check-conformance.mjs:2044 "the other three" and :2081 "the three CLIs" over a four-entry map; lib/pnpm-publish.mjs:10,13,14. Deletion is the fix, and none of these is reachable by fragile-prose. |
scripts/consumer-sbom-meta.mjs:151 |
| 4 | 🔵 Advisory | Robustness | The new release step is the least-proven of the five — first OIDC publish under this name — and sits above the unguarded MCP restamp, so a failure there in a run that also released bulma-ui leaves main on a stale index (#521) and reds every branch cut afterwards. Accepted and documented in the step's own comment; moving it below the restamp would cost that step its "runs last on purpose" property, so it is not free. |
.github/workflows/ci.yml:380 |
| 5 | 🔵 Advisory | Robustness | The failure class still has no gate: nothing ties PNPM_PUBLISHED to a Semantic Release (<pkg>) step, a coverage path or a consumer-SBOM leg, so the next package repeats #706 with CI green throughout. Tracked as #710 and correctly deferred — a check covering the fourth surface would red this branch for deliberately holding that surface back. The measured closure table at supply-chain.yml:86 likewise gains no row for the new leg (also on #706). |
scripts/check-conformance.mjs:2071 |
Overall: Sound, and the mechanics hold up when exercised rather than read.
gen:eslint-meta:check regenerates metadata.ts byte-identically on this tree —
so the template edit in gen-eslint-meta.mjs and the committed artefact are in
step — check:conformance passes 21/21 including fragile-prose,
format:check is clean, and the 15 red tests under
node --test "scripts/*.test.mjs" are all the bulma-ui/dist is absent build
guard in this sandbox rather than anything this branch did. The release config
is verified at the installed source: verify.js:41 accepts
successCommentCondition: false through canBeDisabled, resolve-config.js
passes it verbatim, and success.js:68 takes its own
else if (successCommentCondition === false) branch before the
associated-PR GraphQL walk — so the canCommentOnIssue ternary at line 196,
where falsy means comment, is unreachable. The one blocking item is small and
lives in a test file; the riskiest part is still the thing nothing can exercise
from a PR, the first release itself. Look there first.
Residual risk:
- The first release really will fire. Confirmed rather than assumed:
main
carries 28eslint-plugin-scoped commits, many of themfeat/fix, and no
@allxsmith/eslint-plugin-bestax@*tag exists — so merging cuts1.0.0over
the whole-history commit range with no further commit. That is the PR's own
claim and it checks out. - The whole-history walk commenting anyway. Refuted at the installed 12.0.9
source (above).releasedLabels: falseis redundant-but-harmless exactly as
the comment claims — the label POST atsuccess.js:223sits inside the
comment's owntry. - The new SBOM leg, unexercisable from a PR (
consumer-sbomfires only on
release/schedule/workflow_dispatch). Pushed as far as static work goes:
check-consumer-sbom.mjsneeds no per-package roster — only
MIN_EXPECTED_PACKAGES = 3, which the plugin plus bulma-ui's closure plus
eslint 10's tree clears —0.0.0-developmentsits insideassertVersion's
anchored SEMVER, and the basename still matches thebestax-*sbom*glob
sign-sbom/attach-sbomdownload by. Open: only a dispatch proves the
syft path end to end, and finding 1 is the cheapest thing standing in for it. - Ordering. Holds.
@allxsmith/bestax-bulmareally isworkspace:^in the
plugin's runtimedependencies, identical in shape to the three CLIs whose
releases already work through the samepnpmPublishPluginshelper;
@semantic-release/npm'spreparewrites the new version into
bulma-ui/package.jsonbefore this step'spnpm publishreads it, and any
earlier release step failing aborts the job, so the published range cannot be
computed from a stale version. - The new gate failing open. Refuted:
&&-chained so any non-zero aborts,
--intent-to-addcovers a newly added file, and the generator throws on an
empty table or a dead anchor. It passed here withbulma-ui/distabsent,
which is what makes its placement beforeBuildsafe. - Trust boundary: unchanged, and checked against
.github/CLAUDE.mdon
mainrather than the checkout — which this PR does not touch. No new trigger
and no looserif:; no new scope, PAT, endpoint,uses:or PR checkout — the
release step runs in the existingpublishjob on
steps.app-token.outputs.token, the credential already there for its four
siblings, and holds no model-auth token (I1 untouched); the SBOM leg joins a
job already ategress-policy: blockundercontents: readthat installs
with--ignore-scripts(I2 untouched, and the one change that touches posting
removes it). The one genuinely new thing leaving the job is a package going
to npm, which is the point.
🏄 Fourth look at this set and it is still glassy, dude — the plugin has been
bobbing at0.0.0-developmentsince #686 and this finally drops in on it.
Only thing floating loose is a test that says it covers every leg and then
paddles right past the new one. One line and you are clean. 🤙
The rename in the last commit was worse than the count it removed: the test was called "every other leg" while its loop named three of the four, so the leg this branch adds was asserted by nothing — in the one file whose header says it IS the coverage for a job no pull-request event can run. The loop carries every leg now, including the scoped one it omitted, and the comment says why that matters rather than leaving it to be rediscovered. A mutant that pins every package instead of the released one fails seven tests, so the assertions are live rather than decorative. Four more present-tense counts, in files the earlier sweeps did not open: the paragraph describing what the other legs resolve to, the sibling-dependency exemption describing a four-entry map as three CLIs, and the pnpm-publish header counting the configs it exists to deduplicate. Two the review named are staying, because they are a different thing. "#532 moved the other three" and "the other three once one real release had proved it" describe migrations that happened, to a number of packages that existed at the time. A count of a past event is a fact; a count of a present set is a projection that goes stale. Only the second kind is worth deleting, and treating every numeral as the first kind would make the history unreadable.
|
Round 4 handled at 1 — fixed. The rename in 3 — four fixed, two refused, and the line between them is worth stating. Fixed: the paragraph describing what the other legs resolve to, the sibling-dependency exemption describing a four-entry map as "the three CLIs", and the Not fixed, deliberately: If that distinction is wrong I would rather hear it than keep applying it, because it is the rule I have been sweeping by for three rounds. 2, 4, 5 — standing, unchanged. The provenance window, the least-proven step above the unguarded restamp, and the ungated failure class. All three are accepted with reasons in earlier rounds, all three are on #706 or #710, and none has moved. One note on the count sweep, since this is the fourth round it has appeared in. Nine removed across three commits and each sweep found more, which is the argument for the rule rather than against it — every one read as correct when written. But |
Preview DeploymentPreview URL: https://4763b1d3.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 4 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Security | A fifth package starts publishing while verify-provenance's install roster still names four, so that job goes green having never opened this plugin's attestation. The deferral is forced, and I confirmed it against the registry rather than the PR body: the placeholder's dist carries signatures and no attestations, so verify-attestation.mjs fails closed on it. Only #706 step 3 closes the window this workflow's own comment warns about. |
.github/workflows/supply-chain.yml:1029 |
| 2 | 🔵 Advisory | Robustness | The failure class still has no gate: nothing ties PNPM_PUBLISHED to a Semantic Release (<pkg>) step, a coverage path or a consumer-SBOM leg, so the next package repeats #706 with CI green throughout. Tracked as #710 and correctly deferred — a check covering the fourth surface (the provenance roster) would red this branch for deliberately holding it back. |
scripts/check-conformance.mjs:2071 |
| 3 | 🔵 Advisory | Robustness | The new release step is the least-proven of the five — first OIDC publish under this name, and a trusted-publisher rejection lands after the tag is pushed — and it sits above the unguarded MCP restamp, so a failure there in a run that also released bulma-ui leaves main on a stale index (#521). Accepted and documented in the step's own comment; moving it below the restamp would cost that step its "runs last on purpose" property, so it is not free. |
.github/workflows/ci.yml:380 |
| 4 | 🔵 Advisory | Robustness | Point-in-time evidence, one row for the lot. Two of the count edits deleted the qualifier and kept the numeral, which loosens a measurement instead of dropping it: :311 now reads "428 entries across the closures" of a set that has since grown, with no run id to bound it (its sibling at :442 keeps one). Going the other way, the test header's reworded "mis-pins any package whose release triggered the run" overstates — an indexOf/lastIndexOf slip can only mis-pin a scoped package. Deleting the numeral and the widened clause is the fix. |
scripts/check-consumer-sbom.mjs:311 |
Overall: Sound, and it holds up when exercised rather than read. I could not
find a defect in any executable path this diff touches. The riskiest part is the
one thing no PR event can reach: the first release itself — a whole-history
commit range, a trusted publisher used for the first time under this name, and a
new consumer-SBOM leg that only a release/schedule/dispatch run exercises.
Look there first; the rest is bounded, and both of the load-bearing decisions
(publish after bulma-ui, and suppress the first release's whole-history
comment walk) check out at their sources.
What I verified, and how
successCommentCondition: falsereally closes the walk, in the pinned
@semantic-release/github@12.0.9on this tree — not from the docs:
verify.js:41,47validate both options through
canBeDisabled = (validator) => (value) => value === false || validator(value),
so neither errors inverifyConditions;resolve-config.js:34passes
successCommentConditionthrough untouched (unlikelabels/releasedLabels,
which haveisNildefaults);success.js:68hits
else if (successCommentCondition === false)and logs "Skip commenting" before
anyassociatedPRsGraphQL call; andreleasedLabelsis applied at
success.js:222, inside the per-issue commenttry, so no comment means no
label — exactly as the config comment claims.- The other four configs really are bare — greped; only this one passes options.
- The ordering rationale is real.
eslint-plugin/package.jsondeclares
"@allxsmith/bestax-bulma": "workspace:^"in runtimedependencies, and
src/lib/values.ts/src/rules/no-color-as-surface.tsimport
@allxsmith/bestax-bulma/constants, which the published library exports. - The new SBOM leg works against the registry as it stands today.
npm view
says the placeholder is pnpm-published —
dependencies = { '@allxsmith/bestax-bulma': '^5.15.2' }, not a raw
workspace:^— so a consumer install resolves, and the closure clears
MIN_EXPECTED_PACKAGES = 3comfortably.assertVersion('0.0.0-development')
andartifactBasename('allxsmith-eslint-plugin-bestax', '0.0.0-development')
both succeed (ran them); the slug matches the@allxsmith/bestax-bulma → allxsmith-bestax-bulmaconvention, and every downstream glob is prefix-based
(bestax-*sbom*), so nothing assumes a segment count. - The metadata gate is real and correctly placed.
pnpm gen:eslint-meta
regeneratesmetadata.tsbyte-identically (working tree stayed clean), and the
generator readsbulma-ui/srcthroughlib/props-extract.mjs— nodist—
which is what makes its placement beforeBuildsafe. pnpm exec semantic-releaseresolves fromeslint-plugin/
(/…/node_modules/.bin/semantic-release), and the config loads and yields the
expected seven-plugin list — the package declares nosemantic-releaseof its
own, same as the other four.- Suite state: after a full build,
node --test "scripts/*.test.mjs"is
923/923 green,check:conformancepasses 21/21 (including
release-docs-sync, which derives its roster from the workspace and so already
covers this package), andprettier --checkis clean on every changed file. - The focus commit's own claim, mutation-tested. Dropping the
parsed.package !== pkgguard ininstallSpec(pin every leg) fails 3 tests in
this file, including the loope314fea1extended — so the new leg's assertion
is live, not decorative. The thread I opened onbc47767bis resolved.
Residual risk:
- The first release's notes overflowing GitHub's release-body limit — the
whole-history range is the point of the@semantic-release/githubblock, and it
also decides how big the body is. Refuted by arithmetic:maincarries 1,894
commits, 419 of themfeat|fix|perf|revertsubjects, plus 27BREAKING CHANGE
footers;bestax-mcp@1.0.0's body measured 41,244 characters over its 269
rendered entries (~153 chars/entry), which puts this one near 65–75k against a
125,000 limit. Roughly half, so it clears — but note the failure shape if it
ever did not:@semantic-release/githubruns afterpublish, so the tarball
and tag would already be gone. - The comment/label walk firing anyway — refuted at the source three ways
(validator, config resolution, branch), see the details block. Nothing is left
to a version's behaviour except the deprecation this branch already moved off. - The SBOM leg being unexercisable from a PR — partially refuted: the decision
logic is covered byconsumer-sbom-meta.test.mjs(32/32) and the registry state
it depends on resolves today. What no test or PR can reach is syft's actual
output for this closure; only aworkflow_dispatchshows that, and the
measured-closure table in that job still has no row for it (recorded on #706). - The plugin's own pin path is asserted by nothing — open, and marginal.
installSpec pins the package the release names(:86) still names only
@allxsmith/bestax-bulma, so the exact path the first release takes (a scoped
plugin tag → that leg pinned) has no assertion. The code is generic and the
two-@case is covered by the bulma assertion, so the residual is small; it is
named because this file is, by its own header, the only coverage this job has. - A sixth surface being missed the way the first five were — checked rather
than assumed:CONTRIBUTING.md's dry-run recipe and OIDC publisher list, its
mirror in the docs, andSECURITY.md's table all already name this package, and
release-docs-syncderives that roster from the workspace, so it cannot
silently exempt it. The only roster still short isverify-provenance's, which
is row 1.
🏄 Five packages, five release steps — the plugin finally gets to paddle out
instead of watching from the beach. The whole-history comment walk got muted
before it could spam a few hundred old PRs, the counts got swept 'til the prose
stopped lying, and the new leg is actually in the test now. Clean ride, good to go.
Two of my own count edits made their sentences worse rather than better, which is a fair thing to have caught. One deleted the qualifier and kept the numeral: "428 entries across the closures" of a set that has since grown, with nothing bounding it. That is a looser claim than the one it replaced, not a tighter one — its sibling check had the shape right all along, carrying the run id the measurement came from. So this one carries the run and drops the total, and says why the bound matters as much as the result. The other widened a precise claim while removing a count. "Mis-pins any package whose release triggered the run" is wrong: an indexOf/lastIndexOf slip can only mis-pin a package whose name contains a second `@`, which is what the case is about. Precision restored. The rule this sweep has been applying, stated now that it has been wrong in both directions: a count of a past event bounded by what produced it is evidence and stays; a count of a present set is a projection and goes; and neither is an excuse to make the surrounding sentence say less than it knew.
|
deep-review: fresh — please read f3fc0a7 |
Preview DeploymentPreview URL: https://7a3343ed.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 5 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Correctness | f3fc0a7's restored precision names a failure the code cannot produce. Under an indexOf slip, parseReleaseTag('@allxsmith/bestax-bulma@5.12.0') finds @ at index 0, trips the at < 1 guard and returns null — so installSpec falls back to latest. The scoped leg is left unpinned, not "mis-pinned". The coverage half of the sentence is exactly right; only the consequence is not. Deletion is the fix: drop and then mis-pins the scoped one whose release triggered the run, or say "leaves it on latest". |
scripts/consumer-sbom-meta.test.mjs:15 |
| 2 | 🔵 Advisory | Security | A fifth package starts publishing while verify-provenance's install roster still names four, so that job goes green having never opened this plugin's attestation. The deferral is forced — verify-attestation.mjs fails closed and the hand-published placeholder has none — and it is the exact window that step's own comment warns about ("add a fifth published package to one and not the other and it ships unverified, silently, green"). Only #706 step 3 closes it. |
.github/workflows/supply-chain.yml:1029 |
| 3 | 🔵 Advisory | Robustness | Two documents go from true to false at merge, not at follow-up. SECURITY.md says the plugin's "only version on the registry is a 0.0.0-development placeholder", and docs/docs/guides/security.md lists the CI-released, provenance-carrying packages as four. Both are accurate on this tree and both are falsified by the first release this PR triggers, with no gate to notice. Deliberate and on #706 step 3 — recorded so the follow-up is not optional. |
SECURITY.md:11 |
| 4 | 🔵 Advisory | Robustness | The failure class still has no gate: nothing ties PNPM_PUBLISHED to a Semantic Release (<pkg>) step, a coverage path, or a consumer-SBOM leg, so the next package repeats #706 with CI green throughout. Tracked as #710 and correctly deferred — a check covering the fourth surface (the provenance roster) would red this branch for deliberately holding it back. The measured closure table at supply-chain.yml:86 likewise gains no row for the new leg. |
scripts/check-conformance.mjs:2071 |
| 5 | 🔵 Advisory | Robustness | Point-in-time evidence, one row for the lot: this commit adds run id 33263381732 to a code comment. It is the good shape of the class — bounded by what produced it, matching the sibling check two hundred lines down, and in a .mjs that fragile-prose does not scan either way — but it is still a dated observation living in source. Move the evidence to the issue and link it. |
scripts/check-consumer-sbom.mjs:311 |
Overall: Sound, and I could not break it. The focus commit is a two-hunk
prose correction and it is right on the substance — the "428 entries" number
really had lost its bound while its sibling had kept one — though the second
hunk trades one imprecision for another (row 1). Everything executable in the
PR holds up when run rather than read: gen:eslint-meta:check regenerates
metadata.ts byte-identically on this tree (exit 0) and reads only
bulma-ui/src plus prettier, never bulma-ui/dist, which is what makes its
placement before Build safe; check:conformance is 21/21; the 32 tests in
consumer-sbom-meta.test.mjs pass and now include the new leg. The riskiest
part is the one nothing in CI can exercise — the first release itself, where
the trusted publisher for this name is used for the first time over a
whole-history commit range, and a rejection lands after the tag is pushed.
Look there first.
Residual risk:
- The new SBOM leg is unexercisable from a PR (
consumer-sbomfires on
release/schedule/workflow_dispatchonly). Pushed as far as static
checking goes:parseReleaseTagsplits
@allxsmith/eslint-plugin-bestax@1.0.0on the last@correctly;
artifactBasenameyields
bestax-consumer-sbom-allxsmith-eslint-plugin-bestax-<version>, which matches
bothsign-sbom'sbestax-consumer-sbom-*.{spdx,cdx}.jsonloop and
attach-sbom'sbestax-*sbom*download pattern; the guard's expected root
namebestax-consumer-closure-allxsmith-eslint-plugin-bestaxis exactly what
the install step's heredoc writes;0.0.0-developmentsits inside
assertVersion's anchored SEMVER (prerelease is in the grammar); and the
egress allowlist needs no change, since the only new traffic is
registry.npmjs.org, already listed. Open: only a dispatch proves the
syft path end to end. successCommentCondition: falsesilently reverting to "comment". Refuted
at the installed source (12.0.9), three ways:verify.js:41types it
canBeDisabled(isNonEmptyString), sofalsevalidates;resolve-config.js
passes it through verbatim with noisNildefault to swallow it; and
success.js:67takes its ownelse if (successCommentCondition === false)
branch before the associated-PR GraphQL walk, so thecanCommentOnIssue
path where falsy means comment is unreachable.releasedLabels: falseis
redundant-but-correct exactly as the comment claims — the label POST at
success.js:223sits inside the comment's owntry.- The new gate failing open. Refuted by running it, not reading it:
pnpm run gen:eslint-meta:checkexits 0 on this tree, so the header change in
gen-eslint-meta.mjsand the committedmetadata.tsare in sync, and the
--intent-to-addcovers a newly added file the waygen:mcp:checkdoes. - The first release firing at all, and over the whole history. Premise
confirmed rather than assumed: no@allxsmith/eslint-plugin-bestax@*tag
exists andsearch/commitsfindsfeat(eslint-plugin)commits onmain, so
a1.0.0will cut. Open and accepted in the PR body: with no
lastReleasethe commit range is the whole repository, so the changelog and
the GitHub release body cover it. Bounded, not closed — only the tag #706
wants fixes it. - Ordering. Holds. The plugin declares
@allxsmith/bestax-bulma: "workspace:^"in runtimedependencies(and is in
SIBLING_RUNTIME_DEPS), the new step is last of five and after
Semantic Release (bulma-ui), whose@semantic-release/npmprepare has
already written the new version intobulma-ui/package.jsonby the time
pnpm publishpacks the plugin; any earlier release step failing aborts the
job rather than leaving a stale version to resolve against. - Not verified here, stated rather than implied: npm access is blocked in
this sandbox, so I could not read the0.0.0-developmentpackument to confirm
thatworkspace:^was resolved at pack time (#412's uninstallable shape) or
thatlatestpoints at it. Theprepack/prepublishOnlyguard
(require-pnpm-publish.mjs, wired to both hooks) is the local evidence, and
--ignore-scriptsbypasses it, as its own header says. - Trust boundary: unchanged, checked against
.github/CLAUDE.mdonmain
rather than the checkout. No new trigger and no looserif:—publishis
stillpush+refs/heads/main,consumer-sbomstill
release/schedule/dispatch; no new scope, PAT, endpoint,uses:or PR
checkout — the release step runs in the existing job on
steps.app-token.outputs.token, the credential already there for its four
siblings, and holds no model-auth token (I1 untouched); the SBOM leg joins a
job already ategress-policy: blockundercontents: readwith the
assertion step in place, installing with--ignore-scripts(I2 untouched, and
the one change that touches posting removes it). The one genuinely new thing
leaving the job is a fifth package going to npm through the trusted publisher
already configured against the placeholder — the purpose of the PR, not a new
credential.
🏄 Sixth ride on this wave and it's still holding its shape, dude. That last
paddle-back was the humble kind — owning that a couple of your own count edits
made the sentence say less than it knew is rarer than fixing a bug. One tiny
ripple left in the restored wording (anindexOfslip drops the leg to
latest, it doesn't mis-pin it), and then it's just circling back for the
provenance roster once the tag's in the water. Good to go. 🤙
The header comment said an indexOf slip "mis-pins" the scoped leg. It
does not. `parseReleaseTag` takes `lastIndexOf('@')`, and on
`@allxsmith/bestax-bulma@5.12.0` an indexOf slip finds the `@` at index
0, trips the `at < 1` guard and returns null — so `installSpec` falls
back to `latest` and the leg goes unpinned. Wrong version and no version
are different failures, and only the second one is reachable.
The coverage claim above it was already right and is unchanged: most
packages are unscoped, so the slip passes every test that does not name
a scoped one.
|
Round 6: one fix, one piece of my own bookkeeping that was wrong, one refusal. 1. "mis-pins" (fixed,
The coverage half is untouched, since it was already right. 3. Not recorded, despite what I said last round. I claimed this one was on 2 and 4. Standing, accepted, unchanged. #706 hunk 5 for the roster window, 5. Refuting this one. The run id is the bound that makes the sentence Head |
|
deep-review: fresh — please read 2487db0 |
Preview DeploymentPreview URL: https://8eaf08b7.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 5 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Security | A fifth package starts publishing while verify-provenance's install roster still names four, so that job goes green having never opened this plugin's attestation. The deferral is forced — verify-attestation.mjs fails closed and the hand-published placeholder has none — and it is the exact window that job's own comment warns about. Only #706 step 3 closes it. |
.github/workflows/supply-chain.yml:1029 |
| 2 | 🔵 Advisory | Robustness | Two documents go from true to false minutes after merge, not at follow-up. SECURITY.md says the plugin has no supported line and that its only registry version is a 0.0.0-development placeholder carrying no provenance; docs/docs/guides/security.md:15 names the CI-released, provenance-carrying packages without it. Merging fires the release that falsifies both, with no gate to notice. On #706 step 3. |
SECURITY.md:11 |
| 3 | 🔵 Advisory | Robustness | The failure class still has no gate: nothing ties PNPM_PUBLISHED to a Semantic Release (<pkg>) step, a coverage path, or a consumer-SBOM leg, so the next package repeats #706 with CI green throughout. Tracked as #710 and correctly deferred — a check covering the fourth surface (the provenance roster) would red this branch for deliberately holding it back. |
scripts/check-conformance.mjs:2071 |
| 4 | 🔵 Advisory | Robustness | The new release step is the least-proven of the five — first OIDC publish under this name, and a trusted-publisher rejection lands after the tag is already pushed — and it sits above the unguarded MCP restamp, so a failure there in a run that also released bulma-ui leaves main on a stale index (#521). Accepted and documented in the step's own comment; moving it below the restamp would cost that step its "runs last on purpose" property. |
.github/workflows/ci.yml:380 |
| 5 | 🔵 Advisory | Robustness | Point-in-time evidence, one row for the lot: the diff adds run id 33263381732 to a code comment. It is the good shape of the class — bounded by the run that produced it, matching its sibling two hundred lines down — but it is still a dated observation living in source, in a file fragile-prose does not scan. Move the evidence to the issue and link it. |
scripts/check-consumer-sbom.mjs:311 |
Overall: Sound, and I could not find a defect in any executable path this
diff touches. The focus commit (2487db04) is a two-line correction to the test
header and it is right: I built the indexOf mutant and ran the suite against
it — parseReleaseTag('@allxsmith/bestax-bulma@5.12.0') returns null, not a
mis-split, so installSpec falls back to latest and the leg goes unpinned,
exactly as the new wording says. The other half of that sentence holds too: the
mutant fails 4 tests and every one of them names @allxsmith/bestax-bulma,
so "passes every test that does not name a scoped package" is literally true.
The riskiest part is the one thing no PR event can reach — the first release
itself, with a whole-history commit range, a trusted publisher exercised for the
first time under this name, and a consumer-SBOM leg only
release/schedule/dispatch runs. Look there first.
What I verified, and how
- The focus commit, empirically. Copied
consumer-sbom-meta.mjswith
lastIndexOf('@')→indexOf('@')and ran the real suite against it:
4 failures —parseReleaseTag splits a scoped tag on the LAST @,
installSpec pins the package the release names,
installSpec refuses a release tag carrying a junk version,
main emits spec and expect when the release names this leg. All four name
the scoped package; the other 28 pass. Under that mutantinstallSpec
returns the bare package name (→latest), never a wrong version. - The new test assertion is live, and uniquely so.
SCOPED_PLUGINin the
"leaves every other leg on latest" loop is not decorative: a compare-by-scope
slip (parsed.package.split('/')[0] !== pkg.split('/')[0]) fails exactly
one test — that one — and the three unscoped entries all pass it. That
settles my earlier 🟡 the right way: the fix landed and it bites. successCommentCondition: falsereally closes the whole-history walk, at
the source in the pinned@semantic-release/github@12.0.9:verify.js:41
validates it throughcanBeDisabled,resolve-config.jspasses it through
untransformed, andsuccess.js:68short-circuits the entire associated-PR and
issue walk.releasedLabelsis applied atsuccess.js:223, inside that
walk, which is exactly what makesreleasedLabels: falsethe belt the comment
says it is.- The new gate is real and correctly placed.
pnpm run gen:eslint-meta:check
regeneratesmetadata.tsbyte-identically here and exits 0 — and it does so
withbulma-ui/distabsent, which is what makes its position beforeBuild
safe.gen-eslint-meta.mjsgoes throughscripts/lib/props-extract.mjsover
bulma-ui/src, no dist. - The coverage path is not a no-op.
pnpm exec turbo run test:coverage --filter=@allxsmith/eslint-plugin-bestaxproduceseslint-plugin/coverage/
(clover.xml, lcov.info, lcov-report), andturbo.jsondeclares
@allxsmith/eslint-plugin-bestax#test:coveragewithoutputs: coverage/**. - The release step cannot pack an empty tarball. The release job runs
pnpm run build(→turbo run build) before minting the app token;
eslint-plugin'sbuild: tscproduceddist/here, andfiles: ["dist"]. - Nothing else needs a per-package entry.
verify-oidc-context.mjs,
require-pnpm-publish.mjsandnpm-release-info.mjscarry no package roster
(grepped all five names — only prose references to #412).
check-consumer-sbom.mjsderives the scratch-project name frommatrix.slug
(bestax-consumer-closure-${slug}), so the new leg needs no roster there, and
MIN_EXPECTED_PACKAGES = 3is a floor the plugin's closure clears. - The ordering rationale holds.
@allxsmith/bestax-bulmaisworkspace:^
in the plugin's runtimedependencies; semantic-release runs everyprepare
step (including@semantic-release/git's commit) before anypublishstep,
so by the time the plugin'spnpm publishpacks,bulma-ui/package.jsonon
disk already carries the bumped version. - Whole tree green.
pnpm run check:conformance21/21 including
fragile-prose;node --test "scripts/*.test.mjs"923/923 after a build
(the 15 reds before it are all thebulma-ui/dist is absentguard);
eslint-plugin157/157. - "Merging this publishes 1.0.0" is true.
maincarries numerous
feat|fix|refactor(eslint-plugin)subjects — read through the commits API,
since the local clone is shallow — and no@allxsmith/eslint-plugin-bestax@*
tag exists.
Residual risk:
verify-provenancenever opens the plugin's attestation. Open, row 1.
Forced byverify-attestation.mjsfailing closed on the placeholder, and it
is the shape that job's own comment names. Only the follow-up closes it.- Nothing gates the wiring itself. Open, row 3 — #710. A sixth package can
arrive with a release config and aPNPM_PUBLISHEDentry and noci.yml
step, and every check stays green. - The whole-history comment walk reappearing silently. Refuted at the
source, three ways (see the details block): the validator, the config
resolver, and the single branch insuccess.jsthat owns both the comment and
the label. - The new gate step going green while doing nothing. Refuted: it uses the
samegit add --intent-to-add+git diff --exit-codeshape as
gen:mcp:check, so a newly added generated file is visible to the diff, and
it regenerates and exits 0 on a clean tree here. - The new SBOM leg being unexercisable. Partly refuted rather than closed:
every decision the leg makes in code —installSpecfor the plugin's name on
both a naming and a non-naming release,artifactBasenamefor its slug, and
thebestax-consumer-closure-<slug>structural exemption — is driven and
correct when called directly. The registry install and the syft scan stay
untested until someone fires aworkflow_dispatch, and no PR event can. - The unguarded MCP restamp. Open, row 4, and pre-existing: this adds a
fifth step that can skip it, not a new way for it to be skipped.
🏄 Seventh set of the session and the board's still under him — this one's a
two-line comment trim that finally names the right wipeout, and I ran the
mutant to prove it. Wiring's clean, gates are real, tests bite. Paddle it in;
just keep an eye on that provenance roster still sitting out past the break.
|
Round 7 reviewed Deferred on purpose, with an issue holding it. The provenance roster (#706 Refused. The run id in The one substantive thing rounds 6 and 7 found got fixed: the test header Head Worth knowing before merging, since no CI event can reach it: this cuts |
|
🎉 This PR is included in version 5.16.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.3.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.2.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Closes #706.
@allxsmith/eslint-plugin-bestaxhas been on npm at0.0.0-developmentsince #686 merged. The release wiring lives in.github/, which is human-authored, so it was handed over as diffs and never applied. The package builds, typechecks, tests and covers in CI and then publishes nowhere — with CI green the whole way, which is the silent failure #686's own description warned about.What lands
ci.ymlSemantic Release (eslint-plugin), after the bulma-ui stepci.ymleslint-plugin/coveragein Archive Coverageci.ymlESLint plugin metadata up to daterunninggen:eslint-meta:checksupply-chain.ymlconsumer-sbommatrix legPlus the consequences of those four: root
CLAUDE.mdsaid "no CI step yet" and its quality-gates roster named two of the three generated artefacts;eslint-plugin/CLAUDE.mdsaid the metadata gate runs only inpnpm all; the generated header said the same, so it is fixed at the template ingen-eslint-meta.mjsand regenerated rather than hand-edited; and a justification inconsumer-sbom-meta.mjscounted how many packages are unscoped, which a fifth leg falsifies.The release step runs after
Semantic Release (bulma-ui), and the order is load-bearing. The plugin declares@allxsmith/bestax-bulmain runtimedependenciesandpnpm publishresolvesworkspace:^againstbulma-ui/package.jsonat pack time. Run it first in a release bumping both and the published range is computed from the version bulma-ui had on the way in, so a consumer resolving it can land on a version predating whatever that release added — for this plugin, the./constantssubpath it imports.Merging this publishes 1.0.0
There is no
@allxsmith/eslint-plugin-bestax@*tag andmainalready carries release-worthy commits under the scope —git log origin/main --format=%s | grep -cE '^(feat|fix|perf)\(eslint-plugin\)'counts them. So the first run after this lands cuts1.0.0with no further commit.1.0.0rather than0.1.0is deliberate, and it is what decides the next section.Why this config is not bare, when the other four are
With no tag there is no
lastRelease, so the first run's commit range is the whole repository history. The scope inreleaseRulesdoes not narrow that — those rules decide the release type, not what lands incontext.commits— and this repo demonstrates it on itself:bestax-migrate's changelog carries**bulma-ui:**entries.Left bare,
@semantic-release/githubwalks that range for associated PRs and issues and posts a release notice plus areleasedlabel on each. That is not hypothetical here:@allxsmith/bestax-bulma@5.6.0,create-bestax@3.3.0,bestax-migrate@1.0.0andbestax-mcp@1.0.0. The last two are 1.0.0 releases of packages feat(bulma-ui): add consistent gap prop to Columns, aliasing gapSize #300 has nothing to do with.bestax-release-bot[bot], plus the label.released.So each of the first three first-releases did this, and a fourth would add a false claim to several hundred more PRs — irreversibly, and each one a comment event in a repository full of comment-triggered automation.
Precedent for the noise is not an argument for more of it, and the asymmetry is what decides it rather than taste: not commenting can be undone later; commenting on the whole history cannot. Restoring the default is a one-line change once a tag exists and the range is bounded to this package's own commits, and #706 tracks that as the follow-up.
Accepted in exchange: the
1.0.0changelog will cover the whole history, the waybestax-mcp@1.0.0's 269-line one does. Only a bounding tag fixes that, and a bounding tag is what starting at1.0.0rules out.Not in this PR, deliberately
Three things would state something false before a CI-published version exists, so they go in after the first release — #706, step 3:
verify-provenance's install roster. Two wrong reasons before the right one, so to be exact:latestresolves, so it is not that the version is missing; andnpm audit signaturesstays green when provenance is merely absent, as that job's own comment says, so it is not that step either. The step that fails isverify-attestation.mjs, which opens the attestation and fails closed when there is none — and the hand-published placeholder has none.SECURITY.md's supported-versions table.docs/docs/guides/security.md, which names the packages carrying npm provenance.The consumer-SBOM leg is here, for the same reason inverted: it resolves the
latestdist-tag on a non-release event, which the placeholder satisfies, so it works today and describes a stub until a real version replaces it.Security review notes
Per
.github/CLAUDE.md's blast-radius table these are middle and bottom row, and the invariants it asks to be named:steps.app-token.outputs.tokenandGIT_COMMITTER_*; there is noCLAUDE_CODE_OAUTH_TOKENin it, so no model-auth token shares a job with code execution.if:; no new scope, PAT, endpoint or PR checkout — the new step runs in the existing release job on the credentials already there for its siblings; no new actionuses:, so rule 1's repo-wide SHA pin is unaffected; no--allowedTools/--disallowedToolschange.blockundercontents: read, so rule 10's "a new job ships at block" does not apply.Known, and not fixed here
None of the release steps is guarded, so a failure in any of them skips the MCP index restamp below and leaves
maincarrying a stale index (#521). That is already true of the four existing steps; this adds one more step that can trip it rather than a new way for it to happen. Guarding the restamp is a workflow-logic change well beyond this PR.Also recorded on #706: the consumer-SBOM job's measured closure table gains no row for the new leg and the job's own comment says to re-measure from a dispatch rather than edit the numbers; and nothing cross-checks the hand-written
package→slugpair, which is pre-existing for the current legs.Verification
pnpm allgreen locally — threeTasks:summaries, every conformance check passing. The new release step is byte-identical to thebestax-mcpone bar the directory, checked programmatically rather than by eye.gen:eslint-meta:checkpasses on a clean tree and fails on a stale committed artefact, checked by staging a doctoredmetadata.ts— the check regenerates before diffing, so a working-tree edit alone cannot simulate staleness.Summary by CodeRabbit
Chores
Documentation