Skip to content

Review 5533

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 1 revision

#5533 — fix(scripts): trusted-publishing default points at release.yml

bhamodi · open, approve posted · reviewed at 13f9091e067 · view on GitHub

Verdict: approve — the loop merges it

Problem

A maintainer runs node scripts/npm/setup-trusted-publishing.mjs --setup-trust with no flags to register npm's trusted publisher, and the script registers deploy.yml — a workflow that publishes nothing to npm. The next release then dies at the OIDC check, and there is no NPM_TOKEN to fall back on. This is the honest "prevents a defect nobody has hit yet" case: the reachable state is the next re-run of the script, not today's registry.

Solution

(1 decision · 1 runtime line of 22 added)

  1. --workflow default 'deploy.yml' → 'release.yml' — the fix, and it traces to the stated problem.

npm will only accept a publish from one named workflow file per package, and the script is what tells npm which file that is. Its built-in answer was left behind when publishing moved to a different file. The change updates that answer, so a maintainer who runs the script with no flags names the workflow that actually publishes. Nothing about how publishing works changes — only what the script assumes when you don't tell it. The header and option-comment rewrite are evidence of that one decision rather than more decisions, and the four console.error/console.log reformats carry no runtime judgement.

Inside its owner the fix is complete: workflow is threaded through validation (:561), matching (:435), the npm trust github --file argument (:456) and the manual printout (:529) — no hardcoded filename survives, so the PR body's grep claim holds. Across the seam it is not. Four artifacts restate which workflow to trust — release.yml:9,13, docs/release.md:49,73,75, packages/vega/README.md:197,200, and the script itself at :41,141 — and after this change 3 of the 4 say release.yml while one still says deploy.yml.

Impact

Nobody on the next npm release. The script never runs in CI — grepping .github/ finds one hit and it is a comment in release.yml:13. The audience is one person: the next maintainer who runs the script, who stops silently registering a website-only workflow. An audit run with no flags now reports packages currently trusting deploy.yml as CONFLICT rather than "already configured" — that is the fix working, and it is the prompt to re-run with --replace.

Newly exposed: docs/release.md:75 states the default as deploy.yml. Today the doc and the code agree and both are wrong; the moment this merges the doc contradicts the shipped default — and the doc is the more authoritative-looking artifact. That is new, not still-broken.

API

No API change to any published package. The CLI default is the caller-facing surface, and it moved:

~ --workflow default: 'deploy.yml' → 'release.yml'   (maintainer CLI only; no package barrel)
    docs: stale — docs/release.md:75 still documents the old default

Theme targets

n/a — no styles; the style grep returns 0. No new theme targets.

Ossification

Nothing ossifies. No prop, export, type or theme target. A changed default reaches every default invocation and no codemod can carry it, which is exactly the case where a stated reason is owed — and the PR body carries it: "deploy.yml's own header now says 'WEBSITE-ONLY — it does not publish to npm'. A maintainer re-running the script with defaults would re-point trust at a workflow that never publishes and break the next release at the OIDC check." That is a named reason the old default was wrong, not a defence of the new one.

Breaking

  • API — no. --workflow is still accepted, same type, same comma validation (:561); anyone passing it explicitly is unaffected.
  • Visual — no. Nothing renders; it is a Node CLI. CI agrees — "No component packages changed", pr-visual skipped.
  • Theme — no. No targets, tokens or overrides touched.
  • Behaviour — yes, and it is the point. All four script paths walked (audit, --dry-run, --bootstrap, --setup-trust); only configMatches and the npm trust github --file argument change, both because workflow holds a different value. Empty, loading, error, disabled and boundary are not reachable — the diff adds no state, no conditional, no early return and no default beyond this one.

Performance & resources

Zero effects — not React. No listeners, no observers, no layout reads. No dependency added: the diff against package.json and pnpm-lock.yaml is 0 lines, and CI reports no bundle change. A one-shot maintainer CLI, never invoked by CI, so there is no hot flow and nothing to price.

Visual evidence

None — not renderable: a Node maintainer script, no DOM, no CSS, no component. That is consistent with Breaking · Visual being "no". What was run instead, at the head:

  • node --check scripts/npm/setup-trusted-publishing.mjs → parses.
  • prettier --check at head → clean; the same file at origin/main → fails. So the four reformatted blocks in the diff repair pre-existing drift rather than introducing it. Cause: lint-staged's prettier glob is *.{ts,tsx,md} (package.json:98) and does not cover .mjs.

A11y & i18n

n/a, established by grep rather than by argument: aria-|role=|useTranslator|t('@astryx → 0. No rendered output, no AT surface, no user-facing string — the script's console output goes to a maintainer's terminal and has never gone through the catalog. No direction-sensitive property, no CSS. CI: pr-a11y skipped, pr-rtl skipped, PR Analysis Report "No accessibility violations detected".

Judgement

approve and merge. A defect fix against the existing contract — no new concept, no design change — so nothing here needs a human ruling. No prior review and no prior review comments exist on this PR.

1. docs/release.md:73,75 still names deploy.yml as the workflow to trust
   → the next maintainer who follows the doc rather than the defaults runs
     `npm trust github <pkg> --file deploy.yml` and re-points trust at the
     website-only workflow — the exact failure this PR removes from the
     default path                       · docs/release.md:73,75

Pre-existing — #3111 moved publishing and left both the script default and the doc behind — so it does not block. But :75 documents the very default this diff changes, which is why it is worth folding in here rather than filing.

Pre-existing and not the author's, said plainly so it is not read as a finding: docs/release.md:25-49 documents the whole old topology — a publish job on push to main, pnpm -r publish, and a workflow_ref paragraph concluding deploy.yml — none of which is true of release.yml. That is a larger doc repair and should not ride on this PR.

Two more things found and not spent on the author: 4 of the 22 added lines are prettier reformats of pre-existing violations, .mjs sitting outside lint-staged's prettier glob, which is zero runtime effect and possibly worth widening the glob; and docs/release.md:61 claims "There is no package.json script alias for it" while package.json:53 defines setup-trusted-publishing, a third independent staleness in the same doc. Vercel is red here and equally red on #5530, #5531 and #5532 — repo-wide, not this branch. All other checks pass; review-required is pending on the community-contribution gate, which is what an approve releases.

Not verified: what the npm registry's trust config actually says today for the 12 packages — that needs an authenticated npm trust list, so whether they are currently on deploy.yml and would need --replace is unknown; and that release.yml publishes successfully under this trust config end to end, which only a real dispatched release proves. The topology was verified: both the publish and canary jobs carry id-token: write, and deploy.yml has none.

The review, as posted

Thanks, this is right — release.yml is the only thing publishing. docs/release.md:73,75 carries the same stale deploy.yml and would still point you at it; worth folding in here.

Full review

[Reviewed by Robohands]

No inlines: the finding points at docs/release.md, which is not in the diff, so GitHub has no line to anchor it to.

Rounds

One review, two gate passes.

  • Gate 1 — failed on three counts. The review read as though the doc gap arrived with this branch: docs/release.md was already stale before it existed, since the PR that moved publishing left both the script default and the doc behind, and the text the author reads never said so. The verdict itself was graded correct — a partial fix for a live bug still lands, and blocking would have charged a contributor for that earlier PR's debt.
  • Gate 2 — clean. "Carries the same stale deploy.yml" says in three words that the doc had the defect already, without a sentence of process explanation, and it still tells the author this is worth folding in. 27 words against a 30-word cap, no inlines, and the draft neither smuggles a block in as a nit nor softens a defect that reaches anyone on a release.

What changed before posting

Posted as drafted, with a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5533) line appended before the attribution.

Clone this wiki locally