fix for trusted publishing #272
Conversation
|
commit: |
🐳 Docker Images PublishedDefault (no Python): FROM cloudflare/sandbox:0.0.0-pr-272-931a690With Python: FROM cloudflare/sandbox:0.0.0-pr-272-931a690-pythonVersion: Use the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
The OIDC trusted publishing migration is the right direction, but the workflow has critical logic flaws that will cause failures.
Blocking Issues
1. Beta release overwrites stable releases (line 229)
The beta publish steps run unconditionally after the changesets action. When a "Version Packages" PR merges:
- Changesets action publishes stable version (e.g.,
1.2.3) - Beta steps immediately modify package.json to
0.0.0-abc123 - Beta publish overwrites the stable release on npm
Add conditional to line 229:
- name: Modify package.json version for beta
if: steps.changesets.outputs.published != 'true'
run: npx tsx .github/version-script.tsAnd add the same condition to all subsequent beta steps (lines 233, 236, 239, 245, 261, 277).
2. Top-level permissions break changesets action (lines 8-10)
Top-level permissions set contents: read, but the changesets action needs contents: write to create "Version Packages" PRs. Top-level permissions restrict all jobs.
Remove lines 8-10:
permissions:
id-token: write # Required for OIDC
contents: readThe publish-release job already declares its own permissions at lines 150-153.
3. Docker images pushed before npm publish verification (line 188)
Docker images with latest tags are pushed starting at line 188, before the changesets action runs at line 220. If npm publish fails, Docker latest points to an unpublished version.
Move Docker build steps (lines 188-218) after line 227, or add conditionals based on steps.changesets.outputs.published.
4. Beta npm publish authentication (line 277)
Line 277 runs npm publish from packages/sandbox subdirectory. OIDC requires NODE_AUTH_TOKEN environment variable set by setup-node's registry-url parameter. Verify this works from the subdirectory context.
If publish fails with authentication errors, either:
- Run from root directory, or
- Explicitly pass
NODE_AUTH_TOKEN: ${{ env.NODE_AUTH_TOKEN }}
Important Issues
5. Version synchronization confusion
Documentation says beta releases happen "on every push to main" and stable releases happen "when changesets are merged." Current implementation does both on changeset merges. If this is intended, document it clearly. If not, make them mutually exclusive (see issue #1).
6. Workspace dependency resolution timing
Beta publish calls resolve-workspace-versions.ts (line 233) after modifying version to 0.0.0-{hash} (line 230). This may cause incorrect dependency versions in the published beta package if workspace dependencies reference the modified version.
Test that published beta package has correct dependency versions.
Nice to Have
7. Add OIDC token verification
Before line 277, add:
- name: Verify OIDC token
run: |
if [ -z "$ACTIONS_ID_TOKEN_REQUEST_TOKEN" ]; then
echo "Error: OIDC token not available"
exit 1
fi8. Docker build duplication
Beta builds (lines 245-275) rebuild images already built for stable (lines 188-218). This doubles build time. Consider retagging existing images instead of rebuilding.
Security
OIDC migration improves security by removing long-lived tokens. Verify npm trusted publishing is configured for @cloudflare/sandbox before merging. Repository owner check at line 19 prevents malicious forks from publishing.
Verdict
Needs fixes before merge. Issues #1, #2, and #3 are blocking.
There was a problem hiding this comment.
Claude Code Review
Critical pre-merge requirement: Confirm that the npm trusted publisher is configured on npmjs.com for @cloudflare/sandbox before merging this PR. Without that configuration, all publishes will fail.
The OIDC implementation is correct, but there are three issues:
1. npm Version Specificity
The npm upgrade step should specify a minimum version rather than using latest:
- name: Upgrade npm to v11.5.1+ for OIDC trusted publishing
run: npm install -g npm@^11.5.1Node 24 ships with npm 11.x, but being explicit about the minimum version (11.5.1+) makes the requirement clear and prevents future issues.
2. Documentation Not Updated
CLAUDE.md lines 376-393 document the release process but don't mention OIDC publishing. This section should be updated to reflect:
- npm publishing now uses OIDC (no tokens)
- Trusted publisher configuration required on npmjs.com
- npm v11.5.1+ requirement
3. Deployment Risk
Removing NPM_TOKEN before confirming OIDC works creates risk. Recommended sequence:
- Configure trusted publisher on npmjs.com (do this first)
- Merge this PR
- Monitor first publish closely with ability to rollback
If the trusted publisher isn't configured yet, consider keeping NPM_TOKEN temporarily - npm auto-detects OIDC and prefers it when available, so both can coexist during transition.
Verification
The package.json doesn't need publishConfig.provenance - the NPM_CONFIG_PROVENANCE=true env var is the correct approach. The registry-url configuration and permissions are properly set.
Verdict: Needs fixes before merge (npm version specificity + docs update). Must confirm trusted publisher is configured on npmjs.com.
| bun-version: latest | ||
|
|
||
| - name: Upgrade npm for OIDC trusted publishing | ||
| run: npm install -g npm@latest |
There was a problem hiding this comment.
Specify minimum npm version instead of latest for clarity:
run: npm install -g npm@^11.5.1npm 11.5.1+ is required for OIDC trusted publishing. While Node 24 ships with npm 11.x, being explicit prevents confusion.
2 participants
No description provided.