Repository navigation
ci: harden the npm publish path - #1226
Open
ParidelPooya wants to merge 7 commits into
Open
ParidelPooya wants to merge 7 commits into
ParidelPooya wants to merge 7 commits into
Conversation
The publish job holds id-token: write, so code that runs in it can publish to npm. Its npm ci ran the install scripts of every monorepo dependency, including node-llama-cpp and electron-winstaller for the telemetry apps. The publish job needs none of them. npm ci --ignore-scripts skips those scripts. npm publish still runs the prepublishOnly and prepack scripts of our own packages. A local run of npm ci --ignore-scripts, npm run build and npm run test passed.
A tag such as actions/checkout@v7 can move to new code at any time. 49 action references used a tag. Four of them were aws-actions/configure-aws-credentials@v6, in jobs that assume an AWS role. otel-conformance-tests.yml called a reusable workflow at @main. The publish and Scorecard workflows already pinned by SHA. Each reference now names the commit SHA of its tag, with the version in a comment. The reusable workflow is pinned to the current main commit. Dependabot keeps SHA pins up to date. Its github-actions entry now also covers .github/actions/*, so the npm-ci composite action gets updates too.
The publish job ran for any published release, on any tagged commit. It had no approval step, and it did not run the package checks that pull requests run. So a release on an unreviewed commit, or a tarball with missing files, would go to npm. The job now: - runs in the npm-publish environment, so a repository admin can require a reviewer before it starts, - fails when the release tag is not on main or v1.x, - runs verify-declared-files-shipped.mjs and the core check:package (attw and publint) before it publishes, - holds id-token: write at the job level, not for the whole workflow. Every release tag since the history was squashed is on main, so the tag check accepts them.
CODEOWNERS covered the ADRs, a few core files and the prompts. It did not cover the workflows, the composite action, the Dependabot config, the package.json files, the lockfile or the root rollup config. Those files decide what CI runs, what the packages install and what gets published. They now need a review from the JS SDK maintainers. The rule takes effect when branch protection on main requires review from code owners.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
The OTel conformance workflow loads the conformance tests from conformance_test_ref, which defaults to main. A SHA pin on the orchestrator ran an old orchestrator against new tests. templates.test.ts asserts the @main reference for that reason, and it failed on the pin. The reference is back on @main, with a comment that says why. The workflow is in the aws org and owned by the same team.
fetch-depth: 0 also fetches tags. Git resolves "origin/main" as a tag before it tries refs/remotes/origin/main. So a tag named origin/main on a commit outside main passed the check. The check now uses refs/remotes/origin/$branch. A local probe with such a tag passes the old form and rejects the new one.
This comment has been minimized.
This comment has been minimized.
The tag check had no test, and CONTRIBUTING.md rule 4 requires one for every condition. Rule 2 prefers removing logic to adding it. The npm-publish environment already requires a maintainer to approve each publish. The approval shows the tag and its commit. So the reviewer checks that the tag is on main or v1.x, and the job keeps no logic for it. fetch-depth: 0 served only that check, so it goes too.
Codex AI reviewNo confirmed findings. The prior findings no longer apply because the custom branch gate was removed. Tests were not run during this read-only review. Before merge, confirm the documented Reviewed commit |
SilanHe
requested changes
Nov 13, 2026
| /package.json @aws/aws-lambda-durable-functions-js-maintain | ||
| /package-lock.json @aws/aws-lambda-durable-functions-js-maintain | ||
| /packages/*/package.json @aws/aws-lambda-durable-functions-js-maintain | ||
| /rollup.config.js @aws/aws-lambda-durable-functions-js-maintain |
Contributor
There was a problem hiding this comment.
The root rollup.config.js is covered, but each published package's own build config is not: packages/*/rollup.config.mjs, packages/*/tsconfig.build.json, and packages/aws-durable-execution-sdk-js-telemetry-mcp/esbuild.mjs. These decide what goes into the tarball, so they belong to the release path too. For example:
/packages/*/rollup.config.mjs @aws/aws-lambda-durable-functions-js-maintain
/packages/*/tsconfig.build.json @aws/aws-lambda-durable-functions-js-maintain
/packages/*/esbuild.mjs @aws/aws-lambda-durable-functions-js-maintain
🤖 AI-assisted comment
| # Name the remote ref in full. fetch-depth: 0 also fetches tags, and | ||
| # Git resolves a tag named "origin/main" before the remote branch. | ||
| for branch in main v1.x; do | ||
| if git merge-base --is-ancestor HEAD "refs/remotes/origin/$branch"; then |
Contributor
There was a problem hiding this comment.
Nit: v1.x in this loop never matters. A release tagged on a v1.x commit runs the v1.x copy of npm-publish.yml, which still has plain npm ci, no environment: and no tag check. The v1.x branch needs its own backport of this hardening.
🤖 AI-assisted comment
| # tarball that misses a declared file or has wrong type resolution. | ||
| - name: Verify every package ships the files its package.json declares | ||
| run: node .github/workflows/scripts/test-publish/verify-declared-files-shipped.mjs | ||
| - name: Check the core package with attw and publint |
Contributor
There was a problem hiding this comment.
Nit: check:package (attw + publint) runs only on the core package. -testing, -otel, -telemetry-exporter and -telemetry-mcp are published without it. Worth running it for every package in iterate-publish-npm.sh's list, or noting why only core needs it.
🤖 AI-assisted comment
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #1225.
Summary
The publish job can publish every package in this repo to npm. This PR limits what can run in that job and what it can publish. Each commit fixes one item from #1225.
npm ciran the install scripts of every monorepo dependency while the job heldid-token: write. Examples arenode-llama-cppandelectron-winstaller, which only the telemetry apps need.npm ci --ignore-scripts.npm publishstill runs theprepublishOnlyandprepackscripts of our own packages.uses:references to third-party actions used a tag. Four wereaws-actions/configure-aws-credentials@v6in jobs that assume an AWS role..github/actions/*.npm-publishenvironment, so a maintainer approves each publish and checks that the tag is onmainorv1.x. It runsverify-declared-files-shipped.mjsand the corecheck:packagebefore it publishes.id-token: writemoves from the workflow to the job.package.jsonfiles, the lockfile and the root rollup config had no code owner.Before merge: a repository admin creates the environment
The
npm-publishenvironment does not exist yet. If it is missing when a release runs, GitHub creates it with no protection rules. So the approval step only works after an admin does this:npm-publish.The npm trusted publisher settings name no environment today, so the publish keeps working. After the first release that passes this gate, the trusted publisher can also be bound to
npm-publish. Then only this environment can publish.Testing
npm ci --ignore-scripts,npm run buildandnpm run testpass locally. These are the publish job's steps.verify-declared-files-shipped.mjsandnpm run check:package -w packages/aws-durable-execution-sdk-jspass on that install.Not pinned
The OTel conformance workflow still calls
opentelemetry-orchestrator.yml@main. It loads the conformance tests fromconformance_test_ref, which also defaults tomain. A SHA pin would run an old orchestrator against new tests, andtemplates.test.tsasserts the@mainreference. The workflow is in theawsorg and owned by the same team.