Skip to content

fix(release): use real npm trusted publishing (OIDC) for publish workflow - #82

Merged
LukasParke merged 7 commits into
mainfrom
fix/publish-oidc-trusted-publishing
Jul 28, 2026
Merged

fix(release): use real npm trusted publishing (OIDC) for publish workflow#82
LukasParke merged 7 commits into
mainfrom
fix/publish-oidc-trusted-publishing

Conversation

@LukasParke

@LukasParke LukasParke commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Problem

The release workflow was never actually using OIDC, despite logging that it was.

changesets/action@v1 decides its auth path by looking at process.env.NPM_TOKEN (src/index.ts:80-120). The workflow passed the secret as NODE_AUTH_TOKEN, so the action took the "no token" branch and printed:

No NPM_TOKEN found, but OIDC is available - using npm trusted publishing

...while the credential actually in use was the .npmrc that actions/setup-node writes when given registry-url. That's how @openrouter/agent@0.8.0 shipped (npm user robertyeakel, provenance from publishConfig.provenance, not from trusted publishing).

The E404 ... PUT https://registry.npmjs.org/@openrouter%2fmcp was that token lacking rights to create a new package name — npm answers 404, not 403, in that case.

Real OIDC couldn't have worked here regardless:

  • node-version: 22 ships npm 10.x, which has no lib/utils/oidc.js at all. Trusted publishing needs npm >= 11.5.1 / Node >= 22.14 (docs).
  • Trusted publishing also can't create a package's first version (npm/cli#8544). @openrouter/mcp@0.0.1 has since been bootstrapped manually, so that blocker is cleared.

Changes

  • Node 22 → 24 (npm 11.16.0). pnpm publish shells out to whichever npm is on PATH (@pnpm/run-npm), so the runner's npm is what performs the token exchange.
  • Removed registry-url and all NODE_AUTH_TOKEN wiring. These have to go together: with registry-url set but no token in the environment, @npmcli/config's env-replace.js leaves the literal ${NODE_AUTH_TOKEN} in .npmrc and npm sends it as a bearer token, which 401s the npm info calls changesets makes before publishing. npm's OIDC flow sets the auth token itself (lib/commands/publish.js:147, before getCredentialsByURI).
  • Dropped --provenance from the dry-run step. Trusted publishing attests automatically, and dry runs never reach the exchange.

Required npm settings (not in this PR)

For each of @openrouter/agent and @openrouter/mcp, add a trusted publisher:

Field Value
Publisher GitHub Actions
Organization OpenRouterTeam
Repository typescript-agent
Workflow filename publish.yaml
Environment npm-publish
Allowed actions npm publish

Or via CLI (npm >= 11.15, requires account-level 2FA):

npm trust github @openrouter/agent --repo OpenRouterTeam/typescript-agent \
  --file publish.yaml --env npm-publish --allow-publish
npm trust github @openrouter/mcp --repo OpenRouterTeam/typescript-agent \
  --file publish.yaml --env npm-publish --allow-publish

All fields are case-sensitive and npm does not validate them on save — a mismatch only surfaces as a failed publish.

Verification

  • Full pnpm run build + pnpm run test pass locally on Node 24.18.0 / npm 11.16.0 (both packages, all suites).
  • .github/workflows/publish.yaml parses as valid YAML; confirmed no step retains NODE_AUTH_TOKEN and only GITHUB_TOKEN / GH_TOKEN env remain.
  • pnpm publish --dry-run on packages/mcp produces a valid 82-file tarball with @openrouter/agent resolved to 0.8.0.
  • Confirmed repository.url in both package.json files matches the GitHub repo, which npm requires for OIDC publishes.

Follow-ups

  • Once a publish succeeds through OIDC, delete the NPM_TOKEN secret from the npm-publish environment and revoke the token on npm. Leaving it in place keeps the old attack surface open (and 2FA-bypass granular tokens are being deprecated for publishing).
  • Consider setting each package to "Require two-factor authentication and disallow tokens" afterwards.
  • packages/*/publishConfig.provenance: true is now redundant under trusted publishing but harmless; left alone to keep manual/local publishes attested where possible.

Open in Devin Review

…flow

The release workflow claimed to use OIDC but never did. changesets/action
only checks `process.env.NPM_TOKEN`, and the workflow passed the secret as
`NODE_AUTH_TOKEN`, so the action logged "No NPM_TOKEN found, but OIDC is
available - using npm trusted publishing" while the actual credential came
from the .npmrc written by the `registry-url` option of actions/setup-node.

That token is scoped to selected packages, so the first publish of
@openrouter/mcp failed with `E404 PUT /@openrouter%2fmcp` -- npm returns 404,
not 403, when a credential may not create a package name.

Real OIDC could not have worked either: `node-version: 22` ships npm 10.x,
which has no `lib/utils/oidc.js` and therefore no token-exchange support
(requires npm >= 11.5.1).

Changes:
- Bump the runner to Node 24 (npm 11.16.0). `pnpm publish` shells out to
  whichever `npm` is on PATH, so this is what performs the exchange.
- Drop `registry-url` and every NODE_AUTH_TOKEN. With `registry-url` set but
  no token in the environment, npm leaves the literal `${NODE_AUTH_TOKEN}`
  placeholder in .npmrc and sends it as a bearer token, which breaks the
  `npm info` lookups changesets runs before publishing.
- Drop `--provenance` from the dry-run; trusted publishing attests
  automatically and dry runs never reach the exchange anyway.

Requires a trusted publisher on npmjs.com for each package:
repo OpenRouterTeam/typescript-agent, workflow file publish.yaml,
environment npm-publish.
devin-ai-integration[bot]

This comment was marked as resolved.

@cortex-github-agent cortex-github-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Workflow-only fix that correctly root-causes why OIDC/trusted publishing was never actually engaging (NODE_AUTH_TOKEN masked the OIDC path, registry-url + missing token left a literal ${NODE_AUTH_TOKEN} in .npmrc, Node 22's npm 10 lacks OIDC support). The Node 22→24 bump, removal of registry-url/NODE_AUTH_TOKEN, and dropped --provenance on dry-run are all consistent with how changesets/action and npm's OIDC flow actually work. No code/logic bugs found in the diff itself.

Findings (2)

🟡 minor · .agents/skills/changeset-versioning/SKILL.md
Doc still describes the publish step as running pnpm publish --provenance --access public, which no longer matches the actual flow (changeset publish / no explicit --provenance). Pre-existing drift, not introduced by this PR, but worth a follow-up doc update so operators aren't confused about how provenance/OIDC is actually applied now.

🟡 minor · .github/workflows/publish.yaml
End-to-end correctness of the OIDC exchange can't be verified in this PR — it depends on the npm-side trusted-publisher config (workflow filename/environment match) that the PR body explicitly defers to a manual follow-up. Until that config is applied, both the push-triggered and workflow_dispatch live-publish paths will fail with ENEEDAUTH rather than silently misauthenticate, which is a safe failure mode but should be tracked so the next release attempt isn't a surprise.

@cortex-github-agent cortex-github-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Workflow-only fix that correctly root-causes why OIDC/trusted publishing was never actually engaging (NODE_AUTH_TOKEN masked the OIDC path in changesets/action, registry-url + missing token left a literal ${NODE_AUTH_TOKEN} in .npmrc, and Node 22's bundled npm 10 lacks OIDC support). The Node 22→24 bump, removal of registry-url/NODE_AUTH_TOKEN, and dropped --provenance on the dry-run step are all consistent with how changesets/action and npm's OIDC exchange actually work. No logic bugs found in the diff; verified against repo state (no committed .npmrc, job-level environment: npm-publish and id-token: write already present and unchanged).

Findings (2)

🟡 minor · .agents/skills/changeset-versioning/SKILL.md
Doc still describes the publish step as running pnpm publish --provenance --access public, which no longer matches the actual flow (changeset publish, no explicit --provenance flag). Pre-existing drift, not introduced by this PR, but worth a follow-up update so operators aren't confused about how provenance/OIDC applies now.

🟡 minor · .github/workflows/publish.yaml:52-58
End-to-end correctness of the OIDC exchange can't be verified from the workflow file alone — it depends on the npm-side trusted-publisher config (exact workflow filename/environment match) that the PR body explicitly defers to a manual follow-up. Until that's applied on npmjs.org for both packages, both the push-triggered and workflow_dispatch live-publish paths will fail with ENEEDAUTH rather than silently misauthenticate (a safe failure mode), but this should be tracked so the next release attempt isn't a surprise.

Addresses PR review feedback.

Add a "Verify npm supports trusted publishing" step that fails the release
run early if the runner's npm is older than 11.5.1. The original breakage was
hard to diagnose precisely because an npm without OIDC support surfaces as a
cryptic `E404 PUT` / `ENEEDAUTH` at publish time; this turns a node-version
regression into an explicit error instead. Boundary-tested: 11.5.0 fails,
11.5.1 passes.

Update .agents/skills/changeset-versioning/SKILL.md, which had drifted:

- It documented the publish step as `pnpm publish --provenance --access
  public`. The workflow runs `pnpm exec changeset publish --no-git-checks`,
  and under trusted publishing npm attests automatically with no
  `--provenance` flag.
- It claimed publishing is "release-gated via workflow_dispatch -- it does
  NOT auto-publish on push to main". The workflow has triggered on
  `push: main` for its whole history (that is exactly how the failing run
  that prompted this work was invoked), so the doc actively misled operators
  about when a release goes out. Reframed the coordination point as the
  Version Packages PR merge.
- Documented the OIDC prerequisites (id-token, Node 24+, per-package trusted
  publisher fields, npm's lack of validation on save) and the first-publish
  bootstrap limitation from npm/cli#8544.
- Noted that `--dry-run` never reaches the token exchange, so it cannot
  verify auth.
- Fixed stale details: single-package changeset prompt (mcp exists now) and
  "lint with eslint" (repo uses biome).
@LukasParke

Copy link
Copy Markdown
Contributor Author

Thanks both — addressed in df3b4cc.

1. SKILL.md doc drift (cortex, both reviews) — Fixed, and it was worse than the --provenance mismatch you flagged. The same doc also claimed:

Publishing is release-gated via workflow_dispatch — it does NOT auto-publish on push to main.

That has never been true; publish.yaml has had a push: main trigger for its entire history. gh run list shows 8 of the last 9 release runs were event: push, including the two failures that prompted this PR. So the doc was actively misleading operators about when a release actually goes out. I reframed the release-coordination point as "hold the Version Packages PR until the matching SDK release is out", since merging that PR is what triggers a publish. Also documented the OIDC prerequisites and the npm/cli#8544 first-publish limitation, and fixed two smaller stale bits (@openrouter/mcp missing from the changeset prompt list; "lint with eslint" → biome).

2. Live publish depends on the runner's npm doing the exchange (devin) — Checked the specific mechanism, and PATH resolution is safe here. @pnpm/run-npm does prepend path.dirname(process.execPath) to PATH before spawning npm, and pnpm/action-setup installs @pnpm/exe (standalone bundled-Node pnpm), so a bundled npm shadowing Node 24's was a fair thing to worry about. I unpacked both: @pnpm/exe@10.22.0 and @pnpm/linux-x64@10.22.0 ship only a pnpm binary — no npm. node_modules/.bin has no npm shim either. So PATH npm (Node 24's 11.16.0) is what runs.

Rather than just assert that, I added a guard step so the invariant is enforced rather than assumed:

- name: Verify npm supports trusted publishing

It fails the run with an actionable ::error:: if npm --version < 11.5.1. Boundary-tested with sort -V: 10.9.8 and 11.5.0 fail, 11.5.1 / 11.16.0 / 12.0.1 pass. This directly targets what made the original bug expensive — npm 10 doesn't say "I can't do OIDC", it just 404s on a PUT.

3. npm-side config can't be verified from this PR (both) — Correct, and it's deliberately out of band; the repo owner is applying it. Two notes:

  • @openrouter/mcp@0.0.1 is already on npm (published manually at 20:05 UTC during this investigation), so the first-publish blocker is cleared and both packages can now have trusted publishers attached.
  • Agreed the failure mode is safe: no token remains in the workflow, so a config mismatch fails closed with ENEEDAUTH rather than silently falling back to a credential with the wrong scope. The pnpm -r publish --dry-run path can't pre-validate this either — dry runs never reach the token exchange — which I've now called out in both the workflow comment and the skill doc so the next release attempt isn't a surprise.

devin-ai-integration[bot]

This comment was marked as resolved.

cortex-github-agent[bot]

This comment was marked as resolved.

@cortex-github-agent cortex-github-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Update review of df3b4cc: the workflow change is unchanged in substance and still correct; the new npm-version guard step is sound (the sort -V comparison correctly requires npm >= 11.5.1 and fails closed with an actionable annotation). My prior doc-drift finding is resolved, but the SKILL.md rewrite also reverses a recorded release-gating decision (auto-publish on push to main), which deserves an explicit owner sign-off rather than a doc edit — hence advisory rather than approve.

Findings (3)

🟡 minor · .agents/skills/changeset-versioning/SKILL.md:7
NEW: the doc now states publishing happens automatically on push to main, retiring the decision recorded in @openrouter/agent@0.1.1 ("release-gate publishing via workflow_dispatch", PR #4). The description matches actual behavior — changesets/action receives publish: on the push leg (.github/workflows/publish.yaml:88-96) — so the gate has been broken in code, and this PR resolves the conflict in favor of the code. Under OIDC there is no token step left in the loop, so the npm-publish environment's protection rules become the only gate between a merge and a live publish. Needs an explicit call from the release owner: keep auto-publish (and then also fix the now-contradictory mode input help text at publish.yaml:12-18) or drop publish: from the push leg to restore the gate.

🟡 minor · .github/workflows/publish.yaml:65-76
OPEN (partially mitigated, re-anchored from prior review): the new guard catches the stale-npm failure mode early, but the remaining unverifiable dependency is the npm-side trusted-publisher config (case-sensitive repo / workflow filename publish.yaml / environment npm-publish) which the PR body defers to manual follow-up. Until it is applied for both packages, publishes fail with ENEEDAUTH/E404-on-PUT rather than misauthenticating — safe, but track it so the next release attempt isn't a surprise.

🟡 minor · .agents/skills/changeset-versioning/SKILL.md:55-66
RESOLVED (prior finding): the stale pnpm publish --provenance --access public line is replaced with the actual command, an OIDC/trusted-publishing auth section, the dry-run caveat, and the biome lint fix. Verified biome is the repo's linter.

…red publishing

Both reviewers flagged that the SKILL.md correction surfaced a real
contradiction rather than creating one: the 'publish' mode help text still
said to run it only AFTER merging the Version Packages PR, implying the merge
itself does not publish, while the push leg has been passing publish: to
changesets/action all along.

Documents current behavior in both places and records the divergence from the
original release-gating decision (@openrouter/agent@0.1.1, PR #4), including
that the npm-publish environment has no protection rules. No behavior change:
this PR does not touch publish triggering. Restoring the gate is an owner
decision, called out in the PR thread.
@LukasParke

Copy link
Copy Markdown
Contributor Author

Second round addressed in 703e8a7. Both reviewers landed on the same real point, and it needs your call, @LukasParke.

The auto-publish gate (cortex SKILL.md:7, devin publish.yaml:87-103)

Both are factually right, and I verified the specifics:

  • packages/agent/CHANGELOG.md:215 does record an explicit decision in @openrouter/agent@0.1.1"release-gate publishing via workflow_dispatch" (#4).
  • That gate is not in effect. changesets/action receives publish: on the push leg, so a merge to main publishes. gh run list confirms 8 of the last 9 release runs were event: push.
  • gh api .../environments/npm-publish returns protection_rules: [] and deployment_branch_policy: null — so cortex is right that with the token gone, nothing stands between a merge and a live publish.

To be clear about scope: this PR does not change publish triggering. The push trigger and the publish: input are untouched — the diff is Node 22→24, token/registry-url removal, and the npm guard. My doc edit described behavior that predates this PR; it didn't retire the decision. But cortex is right that quietly documenting away a recorded decision inside an auth-fix PR is the wrong way for that to surface, so I've made the divergence explicit instead of smoothing it over.

What I changed, no behavior touched:

  • publish.yaml mode help text: it still told operators to run publish "only AFTER the Version Packages PR has been merged to main", implying merging doesn't publish. That was the internal contradiction devin flagged. Now notes the merge normally publishes on its own and this mode is for retry/force.
  • SKILL.md: records the divergence from PR chore: add changeset for type fix and null sanitization #4, points at the exact lever to restore the gate (drop publish: from that step, or condition it on workflow_dispatch), and notes the empty npm-publish protection rules.

Your decision — I deliberately did not pick:

  1. Keep auto-publish. Nothing more to do; the docs now match. Consider adding a required reviewer to the npm-publish environment, since OIDC removed the incidental token-shaped speed bump.
  2. Restore the gate from PR chore: add changeset for type fix and null sanitization #4. One-line change, happy to push it here or as a separate PR — it's a behavior change and doesn't really belong in an auth fix.

Trusted-publisher config tracking (both, repeated) — Acknowledged and unchanged: still an out-of-band prerequisite you're applying. @openrouter/mcp@0.0.1 is already on npm, so both packages can have publishers attached now. Fails closed (ENEEDAUTH / E404 on PUT) on any mismatch, never a silent misauth. The npm-version guard added in df3b4cc removes the other half of that failure mode, which is the half that was actually diagnosable from inside the repo.

All CI green on 703e8a7's parent (lint, typecheck, unit, e2e, structural-gate).

@cortex-github-agent cortex-github-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Update review of 703e8a7: the workflow logic is unchanged from the last round (OIDC wiring + npm >= 11.5.1 guard, both verified sound), and the update addresses my prior line comment head-on — SKILL.md now names the divergence from PR #4's release-gating intent, gives the exact remediation (drop publish: from the push leg), flags that npm-publish has no protection rules, and the mode input help text at publish.yaml:15-20 no longer contradicts auto-publish. No unresolved correctness or security defects; the residual items are an out-of-repo npm config step and a release-policy call, both explicitly documented.

Findings (2)

🟡 minor · .agents/skills/changeset-versioning/SKILL.md:7-10
RESOLVED-as-documented (prior round's NEW finding): the auto-publish-on-push reality is no longer silently swapped in for the PR #4 decision — the doc states the divergence, points at the publish: input on the push leg as the cause, and gives the fix if gating is wanted back. publish.yaml:15-20 was updated in step. The underlying policy call (accept auto-publish vs. restore the gate) is still an owner decision, not a code defect; if it stays as-is, adding required reviewers to the npm-publish environment is the cheap mitigation, since the doc's own note says nothing currently stands between a merge and a live publish (that claim is about GitHub settings I cannot verify from the repo).

🟡 minor · .github/workflows/publish.yaml:68-79
OPEN (unchanged, re-anchored): the npm-side trusted-publisher records (case-sensitive org/repo, workflow filename publish.yaml, environment npm-publish) still have to be created manually for both packages before any publish succeeds. The version guard covers the stale-npm path only; a field mismatch surfaces solely as ENEEDAUTH / E404-on-PUT at publish time.

… policy

Closes the two remaining review threads.

Reviewers noted that with the npm token gone, nothing stood between a merge
and a live publish. Investigating that turned up something sharper: npm's
GitHub trusted publisher can only pin org, repo, workflow filename, and
environment -- there is no branch or ref claim (see `npm trust github`
flags). The `npm-publish` environment also has no deployment branch policy
(`deployment_branch_policy: null`) and no required reviewers. Combined, a
`workflow_dispatch` from any branch could have minted an OIDC credential and
published to npm. Under token auth this mattered less; OIDC mints credentials
on demand from whatever ref reaches the workflow.

Add a job-level `if: github.ref == 'refs/heads/main'` as the in-repo backstop.
The `push` trigger is already limited to main, so this only constrains
`workflow_dispatch`: dispatch from main still works, dispatch from a feature
branch no longer runs.

Also settle the policy question the reviewers escalated rather than leaving it
open: auto-publish on push to main is the intended behavior. The changeset
flow already gates releases (no changeset -> no version bump -> nothing to
publish) and the Version Packages PR is the human review point, so a second
manual publish step adds ceremony without adding control. Documented the
alternative (condition `publish:` on `workflow_dispatch`) for whoever revisits
this, plus a table of what actually protects a release and which environment
settings remain unconfigured as defense-in-depth follow-ups.
devin-ai-integration[bot]

This comment was marked as resolved.

cortex-github-agent[bot]

This comment was marked as resolved.

@cortex-github-agent cortex-github-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Update review of f8983f7: adds a job-level if: github.ref == 'refs/heads/main' ref guard plus a "what actually protects a release" table in SKILL.md. The guard is a sound accident backstop and breaks no legitimate path (push-to-main and main dispatches still run), and all prior findings are now either fixed or explicitly documented. Remaining items are minor accuracy/DX notes plus the unchanged out-of-repo npm config step — no correctness or security defect blocking merge.

Findings (4)

🟡 minor · .github/workflows/publish.yaml:49-55
NEW: the comment (and the matching SKILL.md line "the job-level if is what prevents a workflow_dispatch from an arbitrary branch reaching npm") overstates the guard. workflow_dispatch executes the workflow file from the selected ref, so anyone with push access can dispatch a branch whose copy drops the if. It genuinely stops the accidental case, but the server-side control is the npm-publish environment deployment branch policy — which under OIDC is the actual ref restriction, not merely defense-in-depth, since npm's trusted publisher carries no ref claim. Re-word so the next reader doesn't treat the if as sufficient.

nit · .github/workflows/publish.yaml:55
NEW: the ref guard also skips mode=publish + dry-run from feature branches, which was a safe pre-merge diagnostic (that leg never reaches the OIDC exchange). Optional: if: github.ref == 'refs/heads/main' || (github.event_name == 'workflow_dispatch' && inputs.dry-run).

🟡 minor · .github/workflows/publish.yaml:110-113
OPEN (unchanged, re-anchored): the per-package npm trusted-publisher records (case-sensitive org/repo, workflow filename publish.yaml, environment npm-publish) still must be created manually before any publish succeeds; a mismatch surfaces only as ENEEDAUTH / E404-on-PUT at publish time. The npm-version guard covers the stale-npm path only.

🟡 minor · .agents/skills/changeset-versioning/SKILL.md:7-22
RESOLVED (rounds 1-3): publish-command drift fixed, the divergence from PR #4's release-gating intent is now stated outright with auto-publish declared the intended behavior and the exact revert path given, and the protections table records that the npm-publish environment has neither a branch policy nor required reviewers. publish.yaml:15-20 no longer contradicts auto-publish.

Addresses the two findings on f8983f7.

The previous comment (and SKILL.md) claimed the job-level ref guard is what
prevents publishing from an arbitrary branch. That was wrong: workflow_dispatch
executes the workflow file from the selected ref, so a branch whose copy drops
the guard simply ignores it. The guard prevents *accidents*, not a determined
actor. Since npm's trusted publisher carries no branch/ref claim, the
`npm-publish` environment's deployment branch policy is the only server-side
ref restriction -- reframed as the actual fix rather than defense-in-depth.

Also restore the off-main `mode=publish` + `dry-run` diagnostic, which the
blanket guard had skipped. That leg never reaches the OIDC exchange, so it
cannot publish.

The exemption is deliberately narrower than the reviewers' suggested
`inputs.dry-run`: with `mode=version` the changesets step runs with `publish:`
wired up and ignores dry-run entirely, so exempting dry-run generally would
have let `mode=version` + `dry-run: true` from a feature branch publish for
real. Scoped to `mode == 'publish' && dry-run`, verified against a truth table
of all seven ref/event/mode/dry-run combinations: every legitimate path runs,
both off-main publishing paths are blocked.

`inputs.dry-run` is safe to use in expressions despite the hyphen -- confirmed
against run 27694931266, which took the live leg and skipped the dry-run leg.
cortex-github-agent[bot]

This comment was marked as resolved.

@cortex-github-agent cortex-github-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Update review of a0b7933: the ref guard is now correctly scoped (main, or workflow_dispatch + mode=publish + dry-run) and both the workflow comment and SKILL.md now describe it accurately as accident-only, naming the npm-publish deployment branch policy as the real server-side control. The author also caught a subtlety my earlier suggestion missed — a blanket dry-run exemption would be unsafe because the mode=version changesets leg ignores dry-run and still has publish: wired. Only an unblocking YAML nit and the unchanged out-of-repo npm config step remain.

Findings (3)

nit · .github/workflows/publish.yaml:67
NEW: the continuation line is indented 7 spaces vs the folded block's 6, so YAML treats it as more-indented and preserves the line break — the if expression reaches Actions with an embedded newline. The runner's lexer treats \n as whitespace so it should still evaluate, but dropping one space removes the dependency and matches the folded if: at publish.yaml:104-106. Suggestion attached.

🟡 minor · .github/workflows/publish.yaml:119-122
OPEN (unchanged, re-anchored): per-package npm trusted-publisher records must still be created manually (case-sensitive org/repo, workflow filename publish.yaml, environment npm-publish); a mismatch surfaces only as ENEEDAUTH / E404-on-PUT at publish time. Separately, the npm-publish deployment branch policy (main) is now documented as the actual ref control rather than defense-in-depth — worth doing at the same time as the trusted-publisher setup.

🟡 minor · .github/workflows/publish.yaml:49-67
RESOLVED (prior round's NEW findings): the guard comment no longer claims to be a security boundary, the exemption for off-main dry-runs is back and correctly narrowed to mode=publish (documented reason: mode=version ignores dry-run and would publish for real), and SKILL.md's protections table matches. Verified the branch-dry-run path runs only build/test + the dry-run step — the changesets step and the publish-detection step are both skipped.

…ingle-line

The continuation line was indented 7 spaces against the folded block's 6, so
YAML treated it as more-indented and preserved the newline, embedding a \n in
the if expression. Actions' lexer treats \n as whitespace so it evaluated
correctly, but there is no reason to depend on that. Re-verified the full
ref/event/mode/dry-run truth table is unchanged.

@cortex-github-agent cortex-github-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Update review of a2dd118: the only change from a0b7933 is the folded-scalar indentation fix — all three if: continuation lines now sit at the block indent, so the expression folds to a single line (github.ref == 'refs/heads/main' || (github.event_name == 'workflow_dispatch' && inputs.mode == 'publish' && inputs.dry-run)) with no embedded newline and no logic change. Every code-level finding from earlier rounds is now resolved; the sole residual item is the out-of-repo npm/GitHub configuration.

Findings (2)

nit · .github/workflows/publish.yaml:64-67
RESOLVED (prior round): continuation indentation normalized to 6 spaces; the if expression now folds to one line, matching the style of the step-level folded if further down the file. Boolean logic unchanged — push-to-main and main dispatches run, off-main dispatches run only for mode=publish + dry-run.

🟡 minor · .github/workflows/publish.yaml:119-122
OPEN (unchanged, final residual): nothing publishes until the per-package npm trusted-publisher records exist (case-sensitive org/repo, workflow filename publish.yaml, environment npm-publish), and the npm-publish deployment branch policy should be set to main at the same time — per the PR's own SKILL.md table that is the only server-side ref restriction under OIDC. Both are GitHub/npm settings, not repo config, so they cannot be verified from this diff.

Two spots still described publishing as a required manual workflow_dispatch
step, contradicting the auto-publish framing established at the top of the
same doc:

- "Step 2: Publish" read as a mandatory step after merging the Version
  Packages PR. That merge is a push to main, which already publishes.
  Reframed as the retry/force path, matching the mode input help text in
  publish.yaml that was corrected in 703e8a7.
- The Configuration list still labeled publish.yaml as "(workflow_dispatch)",
  omitting the push: main trigger that is the primary release path.

Same doc-drift class flagged in review; no behavior change.

Co-Authored-By: Claude <noreply@anthropic.com>
@LukasParke

Copy link
Copy Markdown
Contributor Author

Swept the PR for anything still outstanding. Every code-level finding across all five rounds is resolved, all 6 review threads are marked resolved, CI is green, and cortex approved a2dd118. What was left was one more instance of the doc-drift class reviewers flagged in round 1 — pushed as 0c440c4.

Residual doc drift (self-found, same class as cortex's round-1 finding)

The auto-publish reframing landed in publish.yaml's mode help text in 703e8a7 but two spots in SKILL.md were missed, which left the doc contradicting itself:

  • "Step 2: Publish" still read as a required manual step after merging the Version Packages PR — while the top of the same doc says a merge to main publishes. Reframed as the retry/force path, matching the workflow's help text.
  • Configuration list labeled publish.yaml as (workflow_dispatch), omitting the push: main trigger that is the primary release path — the exact wrong impression that made the original PR chore: add changeset for type fix and null sanitization #4 gate erosion hard to spot.

Docs only; no workflow behavior change.

Verified the folded-scalar fix actually holds

Re-parsed a2dd118 rather than trusting the diff. Both folded if: expressions now carry an identical trailing \n and no embedded newline:

job if:  "github.ref == 'refs/heads/main' || (...inputs.mode == 'publish' && inputs.dry-run)\n"
step if: "github.event_name == 'push' || (...inputs.mode == 'version')\n"

The trailing newline is inherent to > folding and present on the pre-existing step-level if too, so this matches file style exactly. Truth table unchanged.


Blocking, and not fixable in this PR — @LukasParke

I re-checked live state rather than restating the PR body. Nothing has been configured yet, so merging this today means the next release fails.

Prerequisite Live state
@openrouter/agent trusted publisher ❌ Not set (trustedPublisher: null; latest 0.8.0 still _npmUser: robertyeakel)
@openrouter/mcp trusted publisher ❌ Not set (0.0.1 exists, so the first-publish blocker is cleared)
npm-publish deployment branch policy null — all branches allowed
npm-publish required reviewers protection_rules: []
NPM_TOKEN secret ⚠️ Still present in the npm-publish environment

Three things, in order:

  1. Attach the trusted publisher to both packages (case-sensitive; npm does not validate on save). Both are publishable now.
  2. Set the npm-publish deployment branch policy to main. Per this PR's own table, under OIDC this is the only server-side ref restriction — npm's trusted publisher carries no branch/ref claim, and the job-level if is accident-only since workflow_dispatch runs the workflow file from the selected ref. Worth doing in the same sitting as step 1.
  3. After the first successful OIDC publish, delete NPM_TOKEN and revoke it on npm. It is inert to this workflow (no step reads it), so it is not a correctness risk — but it is a live credential with publish rights sitting in the release environment, and leaving it there preserves exactly the attack surface this PR removes.

On devin's "confirm against a real run before relying on it for production": that can't be satisfied pre-merge. The OIDC exchange requires the trusted publisher to exist, and dry-runs never reach it — so the first real proof is the first post-merge release. The failure mode is safe (ENEEDAUTH / E404 on PUT, never a silent misauth), and the npm-version guard covers the half that was diagnosable in-repo. Suggest treating the next release as the verification step and watching that run.

@cortex-github-agent cortex-github-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Update review of 0c440c4: the workflow file is byte-identical to the already-approved a2dd118 (same blob 92a35b6); the only changes are two SKILL.md doc corrections — Step 2 reframed as a retry/force path rather than a required second step, and the Configuration bullet now lists both triggers. Both match actual behavior. No new issues; the sole residual item is the out-of-repo npm/GitHub configuration.

Findings (2)

nit · .agents/skills/changeset-versioning/SKILL.md:69-79
RESOLVED (last of the doc-drift thread): Step 2 no longer reads as a mandatory post-merge publish step — it states that merging the Version Packages PR publishes on its own and scopes manual dispatch to retry/force cases, which matches changeset publish semantics (publishes only versions not yet on npm). The Configuration bullet at SKILL.md:117 now names both push: main and workflow_dispatch.

🟡 minor · .github/workflows/publish.yaml:119-122
OPEN (unchanged, final residual): publishes will fail until the per-package npm trusted-publisher records exist (case-sensitive org/repo, workflow filename publish.yaml, environment npm-publish), and the npm-publish deployment branch policy should be set to main in the same sitting — by the PR's own analysis that is the only server-side ref restriction under OIDC. Both are GitHub/npm settings, unverifiable from this diff.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

Open in Devin Review

@@ -73,7 +119,10 @@ jobs:
publish: pnpm exec changeset publish --no-git-checks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Every push to main can now trigger a real publish

The step-level condition at .github/workflows/publish.yaml:111-113 runs changesets/action@v1 on github.event_name == 'push', and that action's publish: pnpm exec changeset publish leg runs whenever there are no pending changesets. Combined with the new push: main trigger, this means any merge to main with an on-disk version higher than npm will publish automatically (the documented new behavior). Reviewers should confirm the npm-publish environment's deployment branch policy and required reviewers are configured as the PR's follow-up notes state, since the job-level if guard is explicitly not a security boundary — it travels with the dispatched ref.

(Refers to lines 109-119)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@LukasParke
LukasParke merged commit 89ae3ff into main Jul 28, 2026
10 of 11 checks passed
@LukasParke
LukasParke deleted the fix/publish-oidc-trusted-publishing branch July 28, 2026 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant