Skip to content

ci(release): 4463 / Phase 1 - Switch npm publishing to OIDC trusted publishing#4466

Merged
cliffhall merged 13 commits into
mainfrom
ci/npm-trusted-publishing
Jul 4, 2026
Merged

ci(release): 4463 / Phase 1 - Switch npm publishing to OIDC trusted publishing#4466
cliffhall merged 13 commits into
mainfrom
ci/npm-trusted-publishing

Conversation

@cliffhall

@cliffhall cliffhall commented Jul 4, 2026

Copy link
Copy Markdown
Member

Summary

Phase 1 of #4463: make release.yml's npm publishing work via trusted publishing (OIDC) instead of the dead NPM_TOKEN, and nudge all four TypeScript packages into the next release matrix.

Both June release attempts (26882432128, 27626321345) published to PyPI but failed every npm leg with a disguised auth error (E404 on the publish PUT) — the token is expired and non-refreshable. Each @modelcontextprotocol/* package's npm trusted publisher is being registered against this workflow (release.yml) + the release environment, so the workflow must authenticate via OIDC.

Changes

.github/workflows/release.yml (publish-npm job) — mirrors modelcontextprotocol/inspector#1199:

  • Add permissions: id-token: write (required to mint the OIDC token; contents: read added explicitly since setting permissions drops defaults)
  • Install npm@^11.5.1 — trusted publishing requires npm ≥ 11.5.1, Node 22 bundles npm 10.x
  • Remove NODE_AUTH_TOKEN from the publish step
  • Set NPM_CONFIG_PROVENANCE: "true" so packages get provenance attestations
  • Run npm test --if-present before build/publish — previously nothing between tagging and publishing ran a test

publish-pypi job: run pytest before build/publish (guarded by the same tests-directory check python.yml uses; pyright already ran)

scripts/release.py: count .md changes when building the release matrix — READMEs ship inside the published artifacts, and .py/.ts-only detection would have ignored this PR's README touches entirely (review finding)

publish-pypi job (cont.): skip-existing: true on the PyPI publish action so re-runs tolerate already-uploaded files (review finding)

typescript.yml / python.yml: removed the release: [published] triggers and publish jobs — they never fired (releases are created with GITHUB_TOKEN, which suppresses workflow triggers), could never succeed under OIDC (wrong workflow in the claims), and typescript.yml's still referenced the deleted NPM_TOKEN. Both workflows are now pure CI (review finding). This makes release.yml the only workflow that publishes, matching the trusted-publisher binding and RELEASING.md's description. CI now also runs on pushes to any branch (not just main) so same-repo branches get CI before a PR exists; a per-ref concurrency group cancels superseded runs on rapid amend-pushes (fork PRs were and remain covered by pull_request).

Daily cron removedrelease.yml is now workflow_dispatch-only (renamed "Release"): the schedule produced a year of unattended runs stuck awaiting approval, real releases were always dispatched manually, and same-day runs collide on the date-based tag. Phase 2 (#4463) replaces the trigger with release: [published].

Docs: new root RELEASING.md (publishing mechanics as of this PR + failed-publish retry runbook), linked from README alongside the other maintainer docs

CI trigger behavior after this PR

typescript.yml and python.yml are pure CI (detect-packagestestbuild, all packages each run) and fire as follows:

Scenario CI runs? Via
Fork contributor opens a PR pull_request
Fork contributor pushes more commits to the PR branch pull_request (synchronize)
Same-repo branch pushed, no PR yet (new — was main-only) push
Same-repo branch amended ✅ — superseded run on the ref is auto-cancelled push + per-ref concurrency
Same-repo branch with an open PR ✅ runs under both events (accepted trade-off at this repo's scale) push + pull_request
Push/merge to main push

Fork pushes never fire this repo's push trigger (they happen in the fork), which is why pull_request remains essential for contributor coverage.

⚠️ The same environment-case bug inspector hit applies here: the actual GitHub environment is named Release (capital R), while this yaml and the npm trusted-publisher config say release. npm compares the OIDC environment claim case-sensitively, so the environment must be deleted and recreated as lowercase release before the first OIDC publish (exact commands in #4463, Phase 0). Bonus: deleting it automatically fails the 31 stale waiting release runs, clearing that backlog in one stroke. Requires repo admin.

README touch in each TypeScript server so scripts/release.py includes all four in the next npm matrix (they're stranded on npm at 2026.1.26 while PyPI advanced to 2026.6.16):

  • everything: add the License section the other three servers already have
  • filesystem / memory / sequential-thinking: link the package's npm page under the intro

Merging is publish-safe

Merging this PR publishes nothing. Publishing only happens when a release-environment deployment is approved on a release.yml run (cron runs queue daily but sit awaiting approval). The actual release will be dispatched deliberately once the npm trusted-publisher registrations are complete for all four packages.

If a package's trusted publisher isn't registered by then, its matrix leg fails alone (fail-fast: false) and catches up in a later release.

Prerequisites before dispatching the release (not part of this PR)

  • Rename GitHub environment Releaserelease — done 2026-07-04 (delete + recreate; also auto-failed all 31 stale waiting runs and deleted the dead env-level NPM_TOKEN secret)
  • npm trusted publisher registered for all four packages: server-everything, server-memory, server-filesystem, server-sequential-thinking (workflow release.yml, environment release) — done 2026-07-04
  • No NPM_TOKEN secrets remain anywhere; going full OIDC — no tokens will be issued again

Part of #4463.

🤖 Generated with Claude Code

The NPM_TOKEN used by publish-npm is dead (npm classic-token sunset) —
both June release attempts failed publishing with a disguised auth error
(E404 on the publish PUT) after PyPI legs succeeded. Each
@modelcontextprotocol/* package's npm trusted publisher is being bound to
this workflow (release.yml) + the release environment, so publish-npm now
authenticates via OIDC instead of a token:

- add id-token: write permission to publish-npm (required to mint the
  OIDC token; contents: read added explicitly since setting permissions
  drops the defaults)
- upgrade the npm CLI in the publish job (trusted publishing requires
  npm >= 11.5.1; Node 22 bundles an older npm)
- drop NODE_AUTH_TOKEN from the publish step

Also touch each TypeScript server's README (add everything's missing
License section; link each package's npm page) so scripts/release.py
includes all four packages in the next npm publish matrix — they've been
stranded on npm at 2026.1.26 while PyPI advanced to 2026.6.16.

See #4463 for the rollout plan.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
cliffhall and others added 2 commits July 4, 2026 09:43
Align with modelcontextprotocol/inspector#1199:
- set NPM_CONFIG_PROVENANCE so published packages get provenance
  attestations
- pin the npm upgrade to ^11.5.1 instead of latest

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
Gate both publish jobs on the package's test suite: npm test
--if-present before the npm build/publish, and pytest (when a tests/
or test/ directory exists, matching python.yml's guard) before the
PyPI build/publish. Previously nothing between tagging and publishing
ran a single test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 2m 15s —— View job


Review: OIDC trusted publishing

  • Read PR diff against origin/main
  • Review release.yml changes (OIDC, permissions, npm version, tests)
  • Review README changes
  • Post review feedback

The OIDC workflow changes are correct and closely mirror the inspector#1199 precedent. But there's one significant problem: the README touches won't actually do what the PR says they will.


🔴 Blocking: README (.md) edits will not add the packages to the npm matrix

The stated purpose of the four README edits is "so scripts/release.py includes all four in the next npm matrix." That won't happen. The matrix is built by find_changed_packageshas_changes, which filters the diff down to .py/.ts files only and ignores everything else:

# scripts/release.py:115-117
changed_files = [Path(f) for f in output.stdout.splitlines()]
relevant_files = [f for f in changed_files if f.suffix in [".py", ".ts"]]
return len(relevant_files) >= 1

A .md-only change to a package directory returns has_changes() == False, so none of everything, filesystem, memory, sequential-thinking will be selected into npm_packages. The workflow's OIDC plumbing will be correct but will have nothing to publish for these four.

To actually pull them into the matrix you need a .py/.ts change in each package (e.g. a trivial no-op edit / bumped comment in a .ts file), or alternatively add .md to the relevant_files suffix filter. The README improvements themselves are worth keeping — they're just not sufficient to trigger the release.

Fix this →


🟢 Workflow changes — look good

  • permissions block (contents: read + id-token: write) is correct. Since setting permissions drops the defaults, adding contents: read explicitly is needed for the tag checkout. 👍
  • Provenance (NPM_CONFIG_PROVENANCE: "true") will work: all four package.json have a matching repository.url (github.com/modelcontextprotocol/servers), the repo is public, and id-token: write is present — the three prerequisites for provenance attestations.
  • Test-before-build ordering is safe. All four packages have "test": "vitest run --coverage" with @vitest/coverage-v8 in devDeps, and vitest runs against source. (Also prepare runs npm run build during npm ci, so dist/ already exists by the time tests run.)
  • NODE_AUTH_TOKEN removal is correct for OIDC.

🟡 Minor notes (non-blocking)

  1. registry-url still set in setup-node — this writes an .npmrc with //registry.npmjs.org/:_authToken=${NODE_AUTH_TOKEN}, which now expands to empty. It's harmless because npm ≥ 11.5.1 detects OIDC and mints its own token (this matches inspector#1199), but worth being aware of if a publish ever fails with a confusing auth error.
  2. npm install -g npm@^11.5.1 uses an unpinned caret. Fine for now, but a future 11.x could theoretically regress; consider pinning an exact version if you want reproducible publishes.
  3. PyPI test step uses uv run pytest while every other uv call in the job uses --frozen. Minor consistency nit — uv run --frozen pytest would match the surrounding style.
  4. The permissions: block was added to publish-npm but not to create-release/update-packages — those are unchanged and already have what they need, so no action; just confirming it's intentional.

✅ Prereqs correctly flagged

The Releaserelease environment casing issue and the trusted-publisher registration are well-documented in the PR body as out-of-band prerequisites. No concerns there — just make sure the environment rename happens before the first dispatch, since npm compares the OIDC environment claim case-sensitively.


Bottom line: the publishing mechanics are solid and merge-safe as described. The one thing that needs addressing before this achieves its goal is the matrix-detection gap — the README-only touches won't enroll the four TS packages in the release. A tiny .ts change per package (or extending the suffix filter) will.

Note: I can't modify files under .github/workflows/ (GitHub App permission limitation), so the workflow observations above are review-only.
• branch ci/npm-trusted-publishing

…ytest

Review findings from the PR:
- release.py's has_changes() only counted .py/.ts files, so the README
  touches meant to enroll the four TS packages in the npm matrix would
  have been ignored. Include .md — READMEs ship inside the published
  package (npm tarball / PyPI long_description), so doc changes are
  release-relevant.
- uv run --frozen pytest for consistency with the job's other uv calls.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
@cliffhall

cliffhall commented Jul 4, 2026

Copy link
Copy Markdown
Member Author

Addressed in f4c86ff:

  • 🔴 Matrix-detection gap — fixed by extending release.py's suffix filter to include .md rather than adding no-op .ts edits. Rationale: READMEs ship inside the published artifact (npm tarball / PyPI long_description), so doc changes are legitimately release-relevant — this also fixes the general "stranded package needs an artificial code touch" failure mode, and covers everything, whose only other change since 2026.6.16 is package.json (.json wasn't counted either).
  • 🟡 1. registry-url — leaving as-is, matching inspector#1199 (harmless empty-token .npmrc; npm ≥ 11.5.1 prefers OIDC).
  • 🟡 2. npm caret pin — keeping ^11.5.1 for parity with inspector; can revisit if a publish ever misbehaves.
  • 🟡 3. uv run --frozen pytest — done, matches the surrounding calls.
  • 🟡 4 — confirmed intentional: update-packages/create-release keep their existing permissions.

@claude review

@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 52s —— View job


Re-review of f4c86ff9

  • Read updated diff against origin/main
  • Re-verify matrix-detection fix (.md suffix)
  • Check for side effects of the new filter
  • Confirm minor fixes (--frozen pytest)
  • Post review

🟢 Blocking issue — resolved

The matrix-detection gap is genuinely fixed. scripts/release.py:118 now includes .md in the suffix filter:

relevant_files = [f for f in changed_files if f.suffix in [".py", ".ts", ".md"]]

I traced it through find_changed_packageshas_changes: with .md counted, the README touches in everything, filesystem, memory, and sequentialthinking now each return has_changes() == True, so all four get selected into npm_packages. This is the cleaner fix vs. no-op .ts edits, and the rationale holds — READMEs ship inside the npm tarball and PyPI long_description, so they're legitimately release-relevant artifact content. It also permanently removes the "stranded package needs an artificial code touch" failure mode. No release.py tests exist that this change would break. 👍

One thing worth a conscious sign-off (non-blocking): this broadens the release trigger repo-wide, not just for this release. From now on any .md-only change in a package dir — a typo fix, a badge tweak — will enroll that package in the next release matrix and cut a new version. For these reference servers that's arguably desirable (docs are part of the published artifact), and it matches the stated intent, so I'd keep it. Just flagging that the behavior change outlives this one release. Note .json is still excluded, so a lone package.json/pyproject.toml bump without a .py/.ts/.md change still won't trigger — everything is covered here only because of its README edit, not its package.json change.

🟢 Other fixes — confirmed

  • uv run --frozen pytest (release.yml:143) — done, now consistent with the surrounding --frozen calls, and correctly guarded by the same tests/test directory check python.yml uses. ✅
  • registry-url / npm caret pin / update-packages permissions — acknowledged as intentional-and-matching-inspector; all reasonable calls, no objection.

Workflow mechanics (unchanged since last review, still good)

permissions block (contents: read + id-token: write), provenance prerequisites, test-before-build ordering, and NODE_AUTH_TOKEN removal all still check out.


Bottom line: the one blocking item from the prior review is properly resolved and the minor fix landed. LGTM — merge-safe as described, with the out-of-band prerequisites (Releaserelease environment rename, trusted-publisher registration) correctly tracked in the PR body. The only thing to be deliberate about is that the .md trigger is a lasting change to release-selection behavior, not a one-off.

Note: I can't modify files under .github/workflows/ (GitHub App permission limitation), so workflow observations are review-only.
• branch ci/npm-trusted-publishing

@cliffhall

Copy link
Copy Markdown
Member Author

Acknowledging the sign-off note: yes, the .md release-trigger broadening is deliberate and intended to outlive this release — docs are published artifact content, and it removes the artificial-touch failure mode permanently. (It also becomes moot under the #4463 Phase 2 redesign, where matrix selection moves to a registry-diff guard.) Also for the record: the Releaserelease environment rename prerequisite is already done (2026-07-04), so the remaining pre-dispatch item is just the server-sequential-thinking trusted-publisher registration — 3 of 4 are registered.

@cliffhall cliffhall marked this pull request as ready for review July 4, 2026 19:58
Describes how packages publish (release.yml + release environment,
trusted publishing on npm and PyPI, no registry tokens, provenance),
what a release run does, and how to recover from a failed publish leg
(rerun --failed within the window, or ride the next release). Linked
from the root README alongside the other maintainer docs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
@cliffhall cliffhall requested a review from olaservo July 4, 2026 20:23
cliffhall and others added 3 commits July 4, 2026 16:31
Rewrite RELEASING.md as the reference for the target process rather
than a snapshot of the interim state: semver via changesets for
TypeScript / CalVer for Python, versions changed only by PRs on main,
publishing triggered by creating a GitHub Release, per-package
registry-diff guard, OIDC trusted publishing with no registry tokens,
and the failed-publish retry runbook. Implementation tracked in #4463.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
RELEASING.md documents the pipeline this PR ships (scheduled/dispatch
CalVer releases, OIDC publishing, retry runbook), with a footer noting
the planned Phase 2 changes tracked in #4463. The doc gets updated
when that work merges, not before.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review — focus on RELEASING.md: verify every claim in it matches what this branch actually implements once merged (triggers, change detection, versioning, publish-job steps and their order, guard behavior, auth, retry mechanics, environment approvals). Flag anything the doc asserts that the workflows/scripts on this branch don't actually do, and anything notable they do that the doc misstates.

@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 3m 3s —— View job


RELEASING.md accuracy review

I checked every claim in RELEASING.md against release.yml, scripts/release.py, and the other two workflows on this branch (typescript.yml, python.yml). Most of the doc is accurate, but two claims are contradicted by what's actually on the branch — one of them materially.


🔴 "publish exclusively from release.yml" and "there are no registry tokens" are both false on this branch

RELEASING.md:7"All packages publish exclusively from the release.yml GitHub Actions workflow…"
RELEASING.md:9"Authentication is OIDC trusted publishing on both registries — there are no registry tokens."

Neither holds, because release.yml is not the only workflow with a publish job. Both typescript.yml and python.yml still carry publish jobs that fire on release publication:

# .github/workflows/typescript.yml
on:
  release:
    types: [published]
...
  publish:
    if: github.event_name == 'release'
    environment: release
    steps:
      - name: Publish package
        run: npm publish --access public
        env:
          NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}   # ← still the dead token, no OIDC, no npm@11 upgrade, no provenance

python.yml has the same release: [published]publish job (that one uses PyPI trusted publishing, so it's tokenless — but it still contradicts "exclusively").

Two concrete consequences the doc doesn't reflect:

  1. The token claim is literally wrong. typescript.yml's publish job still references secrets.NPM_TOKEN and does not install npm ≥ 11.5.1 or set provenance — so it can neither OIDC-publish nor token-publish once the secret is deleted (per the PR's "no NPM_TOKEN secrets remain anywhere"). The doc says there are no tokens; the branch still has a token reference.
  2. "Exclusively" is wrong for the manual-release path. v2 release pipeline, Phase 1: OIDC trusted publishing — ✅ complete (validation release 2026.7.4) #4463 Phase 2's stated direction is "publishing triggered by manually-created GitHub Releases." A human publishing a GitHub Release will trigger these two publish jobs — a second, conflicting publish path (the TS one broken). (Note: release.yml's own create-release step uses gh release create with GITHUB_TOKEN, and GitHub suppresses further workflow triggers from GITHUB_TOKEN-created events — so the automated cron path won't self-trigger them. But that's a subtle mitigation, not what the doc's absolute wording implies, and it evaporates the moment a release is published by a user/PAT.)

Recommendation: either (a) soften the doc to "the go-forward automated release path is release.yml" and acknowledge the legacy release: [published] publish jobs, or (b) better, remove/neutralize the stale publish jobs in typescript.yml/python.yml so the doc's "exclusively / no tokens" claims become true. Given the dead-NPM_TOKEN reference is exactly the failure mode this PR is fixing, (b) is worth doing before the first dispatch. Either way the doc and the branch currently disagree.


🟡 The "double-publish guard" is npm-only — the doc attributes it to every job

RELEASING.md:18"Each job: … → abort if this version already exists on the registry (double-publish guard) → run the package's tests → build → publish."
RELEASING.md:32"the double-publish guard keeps already-published packages safe."

Only the npm job has an explicit guard (release.yml:191-199, the npm view … contains([$version]) check → exit 1). The PyPI job (release.yml:120-157) has no version-exists step — its order is checkout → uv syncpyright → tests → build → pypa/gh-action-pypi-publish. There's no skip-existing, so on a re-run of an already-published PyPI leg it relies on PyPI's own duplicate rejection (a hard error), not the "abort … guard" the doc describes. The doc's "Each job" phrasing overstates; it's accurate for npm, not PyPI. Suggest wording like "the npm job aborts if the version already exists; PyPI relies on the registry rejecting duplicate uploads."

(Also minor: the PyPI job runs pyright before tests — the doc's generic "run the package's tests" glosses over it. Harmless.)


🟢 Minor: "a fresh run mints a new version" only holds across UTC-days

RELEASING.md:41"a fresh run mints a new version."

gen_version() is f"{year}.{month}.{day}" (scripts/release.py:124-127), so two dispatches on the same calendar day produce the same version — the second git tag -a "$VERSION" (release.yml:103) collides and the tag push fails rather than minting a new version. The doc's advice ("don't re-dispatch expecting a retry") stays correct in spirit, but the reason is date-granularity + tag collision, not "a new version each run." Worth a half-sentence caveat.


✅ Claims I verified as accurate

  • Triggers (release.yml:3-6): daily cron: '0 10 * * *' + workflow_dispatch → doc "daily schedule or manually via workflow_dispatch." ✓
  • release environment gating on update-packages, publish-pypi, publish-npm, create-release. ✓
  • Change detection.py/.ts/.md suffix filter (release.py:118), diffed against last release tag (git rev-list --tags --max-count=1, release.yml:25). ✓
  • CalVer 2026.7.4 example matches {year}.{month}.{day} (no zero-pad). ✓
  • npm job order checkout@tag → npm ci → guard → tests → build → publish. ✓
  • npm OIDC: id-token: write + contents: read (release.yml:168-170), npm ≥ 11.5.1 upgrade, NODE_AUTH_TOKEN removed, NPM_CONFIG_PROVENANCE: "true". ✓
  • PyPI: pypa/gh-action-pypi-publish + id-token: write, trusted publishing. ✓
  • fail-fast: false on both publish matrices. ✓
  • Retry command gh run rerun <run-id> --failed and its "original workflow snapshot / ~30-day window / re-runs only failed legs at the original tag" description — consistent with the tag-pinned ref: …version checkout and standard GitHub re-run semantics. ✓
  • Environment approvals section — out-of-band repo config, correctly flagged as such. ✓

Bottom line: the retry runbook and the release.yml-specific mechanics are accurately documented. The one thing to fix before relying on this doc is the "exclusively / no tokens" framing — typescript.yml and python.yml still publish on release: [published], and the TS one still points at the dead NPM_TOKEN. Tightening the guard wording (npm-only) and the same-day-version caveat are smaller cleanups.

Note: I can't modify files under .github/workflows/ (GitHub App permission limitation), so the workflow observations above are review-only — but the actionable fixes here are in RELEASING.md (doc wording) and, ideally, retiring the stale publish jobs.
• branch ci/npm-trusted-publishing

…ims true

Review findings: RELEASING.md claimed publishing happens exclusively
from release.yml with no registry tokens, but typescript.yml and
python.yml still carried release:[published] publish jobs — never
fired (releases are created with GITHUB_TOKEN, which suppresses
workflow triggers) and unable to succeed under OIDC (typescript.yml's
still referenced the deleted NPM_TOKEN). Remove both jobs and their
release triggers; the workflows are now pure CI.

Also:
- add skip-existing to the PyPI publish action so re-runs tolerate
  already-uploaded files, and describe the per-registry guard
  accurately in RELEASING.md (npm aborts; PyPI skips)
- correct the fresh-dispatch note: same-day dispatches collide on the
  date-granular tag rather than minting a new version

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
@cliffhall

Copy link
Copy Markdown
Member Author

Addressed in 06a8169:

  • 🔴 "exclusively / no tokens" — made true rather than softened (your option b): removed the release: [published] triggers and publish jobs from typescript.yml and python.yml. They never fired (GITHUB_TOKEN-created releases don't trigger workflows), could never succeed under OIDC, and the TS one still referenced the deleted NPM_TOKEN — and as you noted, they'd have become a live conflicting publish path the moment v2 release pipeline, Phase 1: OIDC trusted publishing — ✅ complete (validation release 2026.7.4) #4463 moves to human-created releases. Both workflows are now pure CI (push/PR → detect → test → build).
  • 🟡 Guard is npm-only — fixed both sides: added skip-existing: true to the PyPI publish action, and RELEASING.md now describes the per-registry behavior accurately (npm aborts on existing version; PyPI skips already-uploaded files) plus the pyright step.
  • 🟢 Same-day dispatch — corrected: the "Never" bullet now explains the real mechanics (date-granular version → same-day tag collision; later day → new version; neither retries).

@claude review — please re-verify RELEASING.md against the branch as it now stands.

@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 1m 5s —— View job


RELEASING.md re-verification against 06a8169e

  • Re-read RELEASING.md, release.yml, scripts/release.py
  • Confirm typescript.yml / python.yml publish jobs removed (the "exclusively / no tokens" fix)
  • Verify PyPI skip-existing guard + doc's per-registry wording
  • Verify same-day-dispatch mechanics wording
  • Grep the whole workflows dir for stray tokens / publish paths

All three items from the last review are properly fixed, and I found nothing new that the doc misstates. Details below.


🟢 "exclusively / no tokens" — now true

  • typescript.yml and python.yml are pure CI: both trigger only on push: [main] / pull_request, and neither carries a release: trigger or a publish job anymore (jobs are detect-packagestestbuild). ✅
  • Grepped .github/workflows/** for NPM_TOKEN / NODE_AUTH_TOKENzero hits. The only publish steps left in the entire dir are publish-pypi / publish-npm in release.yml, both OIDC. So RELEASING.md:7 ("publish exclusively from release.yml") and RELEASING.md:9 ("there are no registry tokens") are now literally accurate. This is the stronger option (b) — good call.

🟢 Double-publish guard — now described per-registry and correct

RELEASING.md:18 now reads "the npm job aborts if the version already exists; the PyPI action skips files already uploaded (skip-existing)" and release.yml:158 has skip-existing: true on pypa/gh-action-pypi-publish. Both halves match the branch. ✅ The "(plus pyright for Python)" clause also matches — release.yml:137-139 runs uv run --frozen pyright before tests.

🟢 Same-day dispatch — now mechanically accurate

RELEASING.md:41 now explains date-granular version → same-day tag collision (release.yml:103 git tag -a "$VERSION") vs. a later day minting a new version, neither retrying the failed one. Matches gen_version() (release.py:124-127) and the tag-push logic. ✅


🟡 One small imprecision left (non-blocking)

The numbered flow at RELEASING.md:18 presents a single linear order — "install → double-publish guard → run tests → build → publish" — but that order only holds literally for the npm job (the Check if version exists step at release.yml:192-200 genuinely runs before tests). For PyPI there is no pre-test guard step; the skip-existing behavior happens inside the final publish action (release.yml:154-158), i.e. last, not third. The very next sentence ("The guard differs by registry…") does clarify the mechanism, so a careful reader isn't misled — but the arrow diagram's placement of "double-publish guard" is npm-accurate and PyPI-approximate. If you want it airtight, a half-clause like "(for PyPI the skip happens at the publish step)" would close it. Purely cosmetic; I would not block on it.


✅ Everything else re-verified as accurate

Triggers (release.yml:3-6 cron + workflow_dispatch), release-environment gating on all four gated jobs, .py/.ts/.md change detection diffed against git rev-list --tags --max-count=1, CalVer example, fail-fast: false on both publish matrices, npm OIDC plumbing (id-token: write + contents: read, npm ≥ 11.5.1, NODE_AUTH_TOKEN gone, NPM_CONFIG_PROVENANCE: "true"), PyPI trusted publishing, the gh run rerun --failed runbook and its ~30-day / original-snapshot / failed-legs-only semantics, and the environment-approvals section — all still line up with the branch.

Bottom line: RELEASING.md now matches the branch. The two blocking claims are true, the guard wording is per-registry, and the same-day caveat is correct. The only remaining thing is the cosmetic "guard placement is npm-linear, PyPI-at-publish" nuance in the numbered list — optional. LGTM.

Note: I can't modify files under .github/workflows/, so the workflow observations above are review-only.
• branch ci/npm-trusted-publishing

@cliffhall

Copy link
Copy Markdown
Member Author

Applied the cosmetic nuance too (see latest commit): the guard sentence now states placement per registry — npm aborts before tests, PyPI's skip happens at the publish step. RELEASING.md is now verified line-by-line against the branch. Closing out the review loop — thanks for the thorough passes.

@cliffhall cliffhall changed the title ci(release): switch npm publishing to OIDC trusted publishing ci(release): 4463 / Phase 1 - Switch npm publishing to OIDC trusted publishing Jul 4, 2026
cliffhall and others added 2 commits July 4, 2026 17:22
Releases are now triggered deliberately via workflow_dispatch. The
schedule produced a year of unattended runs stuck awaiting environment
approval, every real release was manually dispatched anyway, and two
runs on the same UTC day collide on the date-based tag. Rename the
workflow to "Release" accordingly and update RELEASING.md.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
Previously push-triggered CI ran only on main, so same-repo branches
got no CI until a PR was opened (fork contributions were and remain
covered by pull_request, which fires on open and on every subsequent
push). Run on all pushes, and add a per-ref concurrency group so rapid
amend-pushes cancel superseded runs. Same-repo branches with an open
PR will run under both events; acceptable at this repo's scale.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review — full-PR pass please: all workflow changes (release.yml, typescript.yml, python.yml), scripts/release.py, RELEASING.md, README, and the PR description itself. The branch has grown since the last full review (dead publish jobs removed, cron removed, CI triggers broadened to all pushes with per-ref concurrency, PyPI skip-existing, doc revisions) — verify it all hangs together and that the docs/description match the code exactly.

@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 2m 24s —— View job


Full-PR review pass

  • Read full diff against origin/main
  • release.yml — OIDC, cron removal, job order, guards
  • typescript.yml / python.yml — CI triggers, concurrency, publish-job removal
  • scripts/release.py — change detection
  • RELEASING.md — every claim vs code
  • README changes
  • PR description vs. code

I traced every workflow, the script, the doc, and the PR description against the branch as it now stands. It all hangs together — nothing in the docs or description contradicts the code. Two non-blocking observations below; neither should hold up merge.


🟢 Everything cross-checks

release.yml

  • on: workflow_dispatch: only — cron gone, renamed Release. ✓ (matches "dispatch-only" / "there is no scheduled/automatic release")
  • publish-npm: contents: read + id-token: write, npm install -g npm@^11.5.1, npm ci → version-exists guard → npm test --if-presentnpm run buildnpm publish with NPM_CONFIG_PROVENANCE: "true" and no NODE_AUTH_TOKEN. Order matches RELEASING.md §18 for npm exactly. ✓
  • publish-pypi: uv syncpyright → tests (guarded by [ -d tests ] || [ -d test ], --frozen pytest) → uv build → publish with skip-existing: true. ✓
  • Both publish matrices fail-fast: false; create-metadata gated if: github.repository_owner == 'modelcontextprotocol'; update-packages/publish-*/create-release all environment: release. ✓

typescript.yml / python.yml — grepped both: no environment:, no publish, no id-token, no release: trigger, no token refs. Now pure push/pull_request CI (detect → test → build). Repo-wide grep for NPM_TOKEN/NODE_AUTH_TOKENzero hits, and schedule/cronzero hits. So RELEASING.md's "publish exclusively from release.yml" and "no registry tokens" are literally true. ✓

concurrencygroup: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true. Because push (ref refs/heads/<branch>) and pull_request (ref refs/pull/N/merge) land in different groups, a same-repo PR runs under both without cancelling each other — matches the description's "runs under both events" row. ✓

scripts/release.pyhas_changes now .suffix in [".py", ".ts", ".md"] (line 118). Root-level RELEASING.md/README.md sit outside src/*/, so has_changes (run per package dir) ignores them — only the four per-package README.md edits enroll packages, and they're all npm/TS, so PyPI matrix stays empty this release. Consistent with "merging publishes nothing" and the npm-only enrollment. ✓

README changes — root README.md adds a RELEASING.md link; everything gains a License section; filesystem/memory/sequentialthinking each get an npm-page link. These are the .md touches that enroll the four packages. ✓

PR description — every itemized change (permissions, npm@11, provenance, token removal, npm test --if-present, PyPI pytest+skip-existing, .md detection, dead-job removal, all-branch push + per-ref concurrency, cron removal/rename) matches the diff. Prereq checklist (env rename, 3-of-4 → now all trusted publishers) is out-of-band and correctly flagged. ✓


🟡 Two non-blocking observations

  1. cancel-in-progress: true also applies to main pushes, not just branch amend-pushes. The comment (# Superseded runs on the same ref are cancelled (e.g. rapid amend-pushes)) frames it as a branch concern, but two quick merges to main will cancel the earlier main CI run (same refs/heads/main group). That's almost always fine for these packages, but it's a slightly broader behavior than the comment implies — worth a conscious "yes, intended for main too."

  2. The npm version-exists guard assumes the package already exists on npm (pre-existing, not introduced here). npm view --json | jq … under the default pipefail shell: for a package that has never been published, npm view exits non-zero (E404) and the step fails before publish. Harmless for this PR — all four TS packages already exist on npm (stranded at 2026.1.26) — but if a brand-new TS server is ever enrolled via this workflow, that guard would abort its first publish. Flagging only so it's on the radar for a future package; no action needed for this PR.


Bottom line: the branch is internally consistent and the docs/description match the code exactly. RELEASING.md's triggers, change-detection, versioning, per-job step order, per-registry guard behavior, OIDC auth, and retry mechanics all line up with release.yml + release.py. LGTM — merge-safe as described, with the out-of-band prerequisites already tracked as done.

Note: I can't modify files under .github/workflows/ (GitHub App permission limitation), so the workflow observations above are review-only.
• branch ci/npm-trusted-publishing

Per-ref cancellation is for superseded branch amend-pushes; on main,
every merge keeps its own CI run so failures stay attributable to a
specific commit.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AViME1R9ES2UXT1uQDx1TG
@cliffhall

cliffhall commented Jul 4, 2026

Copy link
Copy Markdown
Member Author

Both observations actioned in 5850690 / #4463:

  • 🟡 1. (cancellation on main) — decided the opposite of "intended": main runs should each keep their own CI for per-commit attributability. cancel-in-progress is now conditional (github.ref != 'refs/heads/main'), so amend-push cancellation applies to branches only, and the comment says so explicitly.
  • 🟡 2. (guard vs never-published package) — agreed it's pre-existing and out of scope here; recorded on v2 release pipeline, Phase 1: OIDC trusted publishing — ✅ complete (validation release 2026.7.4) #4463's Phase 2 release.yml-rework item as a hard requirement: the redesigned registry-diff guard must treat "package not found" as "publish it", so a brand-new server's first publish isn't aborted by the E404.

@claude review — one confirmation pass on the concurrency change, since it touched workflow code after your LGTM.

@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 57s —— View job


Confirmation pass — concurrency change (58506903)

  • Read the concurrency diff on typescript.yml / python.yml
  • Verify the cancel-in-progress expression + comment
  • Post confirmation

I diffed the two workflows against origin/main and confirmed the concurrency block is identical in both:

# Superseded runs on the same ref are cancelled (e.g. rapid amend-pushes),
# except on main, where every merge keeps its own CI run
concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}

🟢 Confirmed correct

  • Expression is valid. cancel-in-progress accepts an expression that evaluates to a boolean; ${{ github.ref != 'refs/heads/main' }} resolves to true on branch refs and false on refs/heads/main. This is the documented idiom for "cancel everywhere except one ref."
  • Behavior matches the stated intent. Branch amend-pushes (refs/heads/<branch>) → in-progress run cancelled; main pushes → in-progress run protected. This directly implements your "opposite of intended" decision from the trigger comment.
  • Comment now matches the code — it explicitly calls out the main exception, resolving the earlier mismatch I flagged (where the comment framed cancellation as a branch-only concern while the config applied it to main too).
  • PR-vs-push interaction unchanged. push (refs/heads/<branch>) and pull_request (refs/pull/N/merge) still land in different groups, so a same-repo PR's two runs don't cancel each other — consistent with the "runs under both events" row in the PR description.

🟡 One nuance worth a conscious sign-off (non-blocking)

cancel-in-progress: false protects the in-progress run on main, but it does not guarantee every main commit gets a completed run. GitHub cancels any pending run in a concurrency group when a newer run is queued, regardless of cancel-in-progress. So under rapid back-to-back merges to main (A, B, C faster than CI finishes): A runs to completion, B queues, C arrives and cancels the pending B, then C runs once A finishes — B never gets its own completed run.

In practice merges to main on this repo are spaced well apart, so this is very unlikely to bite, and the per-commit run you care about (the one actively executing) is always preserved. But if you want literally every main commit to retain an independent, uncancellable run, the group would need to include the SHA (e.g. group: ${{ github.workflow }}-${{ github.ref }}-${{ github.sha }}) — at the cost of never coalescing redundant main runs at all. I'd only do that if per-commit CI attributability on main is a hard requirement; otherwise the current config is the right trade-off and I'd leave it.


Bottom line: the concurrency change is correct, valid, and matches both the comment and your stated intent. LGTM — the only caveat is the pending-run-cancellation nuance on main, which is inherent to GitHub's concurrency model, not a defect in this config, and almost certainly a non-issue at this repo's merge cadence.

Note: I can't modify files under .github/workflows/ (GitHub App permission limitation), so this is a review-only confirmation.
• branch ci/npm-trusted-publishing

@cliffhall

Copy link
Copy Markdown
Member Author

Sign-off on the pending-run nuance: accepted as-is. The in-progress main run is always protected, coalescing queued main runs under burst merges is fine (the newest run tests the union anyway), and per-commit attributability isn't a hard requirement worth a SHA-keyed group's cost. Review loop closed — thanks for three thorough passes.

@cliffhall cliffhall merged commit 7097923 into main Jul 4, 2026
36 checks passed
@cliffhall cliffhall deleted the ci/npm-trusted-publishing branch July 4, 2026 23:03
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.

v2 release pipeline, Phase 1: OIDC trusted publishing — ✅ complete (validation release 2026.7.4)

1 participant