feat(service-ci): opt-in npm OIDC Trusted Publishers#14
Conversation
Adds a new workflow_call input `npm-oidc-enabled` (default: false) to the reusable Service CI workflow. When true, the publish path switches from long-lived ADOBE_BOT_NPM_TOKEN to npm Trusted Publishers (OIDC) with sigstore provenance attestations, matching the model landed in adobe/spacecat-shared PR #1592. Behavior: - Dry-run (PR / non-main push): SR_NO_NPM_AUTH=true is set, NPM_TOKEN is dropped. The consumer's .releaserc.cjs must gate @semantic-release/npm on `process.env.SR_NO_NPM_AUTH === 'true'`. - Release (push to main): NPM_TOKEN dropped, NPM_CONFIG_PROVENANCE=true set, environment claim minted via the npm-publish GitHub Environment (or 'prod' when vpc-enabled, so VPC consumers keep their existing env config and bind their npm trust with --environment prod). - Adds a conditional "Update NPM" step pinning npm@11.13.0 (the OIDC floor is npm >= 11.5.1). Default-false: existing consumers keep token-based publishing unchanged until they explicitly opt in. Documentation for the per-consumer migration (code, server-side env+branch protection, npm trust binding registration, rollback) is added at docs/npm-oidc-migration.md. Security note recorded inline + in the doc: trust bindings MUST be registered against the caller's workflow_ref, not mysticat-ci's job_workflow_ref — otherwise any consumer of the reusable workflow could republish another consumer's package. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Self-review additions on the npm OIDC migration: - Three explicit rollback scenarios (consumer-side flag flip, mysticat-ci PR-level revert, permanent rotation reversal) with a behavior-knob table that maps the input flag to each env var the workflow sets, so the rollback's blast radius is exact. - Four failure modes (FM-A trust-binding 404, FM-B sigstore down, FM-C env-approval stall, FM-D rename) with copy-pasteable recovery commands. - Backward-compat audit table: every env var / step / permission pre-PR vs post-PR (input=false) vs post-PR (input=true). All default-false code paths produce identical behavior to the pre-PR workflow. - Workflow comment on the 'Update NPM' step documents why running AFTER setup-node-npm composite's npm ci is safe in practice (npm 11's publish path is backward-compatible with node_modules installed by npm 10). - Deduplicated the order-of-operations checklist (was accidentally inserted twice in the prior commit). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the missing owner-side plan for cutting v3.0.0 of mysticat-ci and rolling out across the 14 consumer service repos in a controlled, observable, reversible sequence. Four phases with explicit success criteria + rollback triggers per phase: - Phase 0: cut v3.0.0 (pre-cut checks listed inline). - Phase 1: canary (1 repo, ≥ 1 week observation). Selection criteria named (active, low blast radius, not VPC, not in code freeze). Likely canary candidates ranked: spacecat-jobs-dispatcher, spacecat-task-processor, mysticat-projector-service. - Phase 2: pilot batch (3 repos, ≥ 1 week). Diversity criteria: +1 non-VPC, +1 VPC-enabled, +1 multi-package consumer. - Phase 3: bulk rollout (10 remaining repos), coordinated via a tracking issue on mysticat-ci. Suggested ordering documented. - Phase 4: token decommissioning. Gated on every consumer crossing the ≥ 2-cycle threshold, not a calendar date. Each phase has explicit success criteria, rollback triggers, and a named action. Phase 1 fail-stop includes the workflow_ref-vs- job_workflow_ref check on the attestation's workflow.path — catches a mis-registered trust binding before propagation. Adds a "What can go wrong with the rollout itself" section covering the four meta-risks (unpinned mysticat-ci refs, sigstore outages mid-rollout, adobe-bot bus factor, ADOBE_BOT_NPM_TOKEN rotation collision). Adds a communication template owners can copy verbatim when queueing the next consumer for migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
solaris007
left a comment
solaris007
left a comment
There was a problem hiding this comment.
Hey @alinarublea,
Thanks for a genuinely well-engineered migration. The opt-in boolean on the reusable workflow, the default-false backward-compat design, the phased rollout with measurable canary criteria, and the depth of the failure-mode runbook are all the right shape for moving 14 consumers to OIDC trusted publishing safely. There is one blocking defect: the load-bearing publish-path expression does not do what the PR says it does, and several doc sections document the wrong behavior as audited fact.
Note: CI is green, but nothing in CI exercises the npm-oidc-enabled: true path, so green is not evidence the OIDC path works.
Strengths
- Default-false backward compatibility genuinely holds. With neither flag set, every new expression resolves to the pre-PR values (
NPM_TOKEN= the token, others unset/'',environment='prod'iffvpc-enabled, "Update NPM" skipped). The 13 non-opted consumers see zero behavior change. (service-ci.yamllines 154, 194, 272-273) - The
environment:selection expression is correct for all four (vpc x oidc) combinations because every true-branch value is truthy. (service-ci.yamlline 194) id-token: writeis correctly scoped to the release job only; the build/dry-run job does not carry it.npm trust github --repository --file --environment --yesis real, current CLI syntax, and binding to the caller'sworkflow_ref(not the reusablejob_workflow_ref) is the correct trust model. The Phase 1 check that the SLSA attestation'sworkflow.pathpoints at the caller is a strong safeguard.- Strict-equality guidance for
SR_NO_NPM_AUTH === 'true'correctly avoids the truthy-string trap, and the''-vs-unset analysis for the other env vars is handled right. - The phased rollout (canary -> pilot -> bulk -> decommission) with concrete success/rollback criteria, token retention for >=2 cycles before rotation, and the "add a new failure mode before continuing" learning loop are the right operational properties.
Issues
Critical (Must Fix)
1. The NPM_TOKEN expression returns the real token when OIDC is enabled - the migration is inert and fails silently into token publishing. (service-ci.yaml line 272 release, line 154 dry-run)
NPM_TOKEN: ${{ inputs.npm-oidc-enabled && '' || secrets.ADOBE_BOT_NPM_TOKEN }}
GitHub Actions A && B || C returns C whenever B is falsy, and '' is falsy. For npm-oidc-enabled == true: true && '' -> '', then '' || secrets.ADOBE_BOT_NPM_TOKEN -> secrets.ADOBE_BOT_NPM_TOKEN. So NPM_TOKEN holds the real long-lived token, not ''. This is the only one of the four new expressions whose intended true-value is falsy, which is why the other three are correct and this one is not.
Why it is Critical:
- The release job intentionally does not set
SR_NO_NPM_AUTH, so the consumer's.releaserc.cjsguard is false,@semantic-release/npmruns, finds a populatedNPM_TOKEN, and publishes via the token. OIDC token exchange never triggers. - The PR's primary goal (eliminate the long-lived
ADOBE_BOT_NPM_TOKENfrom the publish path) is silently not achieved while the flag is on. The secret is still injected into both the release job and the dry-run job (which runs on feature-branch pushes). - It fails by silently retaining old behavior: publishes succeed, provenance may still emit, and a consumer believes they migrated while still on the token. The only thing that catches it is the manual
npm view _npmUsercanary check. - It inverts the FM-A "fail-fast" guarantee. The doc states the missing-trust-binding case fails fast "because OIDC mode sets
NPM_TOKEN: ''." In reality a missing binding is masked - npm falls back to the token and publishes with the wrong identity. - Latent decommission landmine: every consumer can "migrate" successfully, then Phase 4 (delete
ADOBE_BOT_NPM_TOKEN) breaks all of them at once, with no recent change to point at.
The doc's self-review table, the rollback table, the workflow-contract section, and FM-A all assert NPM_TOKEN = '' in OIDC mode. All are wrong and must be corrected once the code is fixed.
How to fix: make NPM_TOKEN genuinely UNSET (not empty) in OIDC mode, in both steps. The most robust form omits the env key entirely - a pre-step gated on if: ${{ !inputs.npm-oidc-enabled }} that writes NPM_TOKEN to $GITHUB_ENV, or two Semantic Release steps gated on the flag. A bare ternary inversion (${{ !inputs.npm-oidc-enabled && secrets.ADOBE_BOT_NPM_TOKEN || '' }}) fixes the "token is still the real secret" half and yields '', but resolve a disagreement first: your self-review says @semantic-release/npm treats '' as unset, while there is evidence an empty-string NPM_TOKEN can still suppress OIDC (npm tries the empty token rather than falling back to OIDC). Gated steps that omit the variable are correct under both readings, so prefer those. Then re-derive the self-review table from an actual run.
Important (Should Fix)
2. No pre-merge validation exercises the opt-in path, so a one-character expression error reaches 14 pipelines unguarded. (repo-level / service-ci.yaml)
The only CI on this PR is service-ci's own run, which never sets npm-oidc-enabled: true; the OIDC branches are never executed before merge, and actionlint/yaml-schema do not evaluate ternary semantics. The manual self-review was the only check, and it shipped a false assertion (critical finding 1). Your own Phase 0 note says "a typo here breaks every consumer that opts in" - manual reading is demonstrably insufficient here. Add a small matrix job in this repo that renders the publish-path env for both flag states and asserts the results (you cannot print the secret, but you can assert NPM_TOKEN is empty/absent when OIDC is on and present when off, plus the other three vars). That job would have caught finding 1 and turns the self-review table into an executable gate.
3. No fail-fast OIDC preflight, and the npm-publish environment auto-creates unprotected - the doc's main-only enforcement can silently not exist. (service-ci.yaml line 194; docs/npm-oidc-migration.md server-side setup)
The sibling vpc-enabled feature has a hard-fail sanity-check when a consumer sets the flag without doing their side; npm-oidc-enabled has no equivalent. Two failure shapes: (a) a consumer who flips the flag without registering the trust binding hits FM-A (404) at release time on main, the worst moment; (b) referencing a not-yet-created npm-publish environment does not enforce a branch policy - GitHub auto-creates the environment with no protection rules and proceeds, so the "server-enforced main-only deployment_branch_policy" the doc leans on to justify skipping the reviewer gate is absent until the consumer manually creates it. (Mitigating factor: the release job's own if: github.ref == 'refs/heads/main' still blocks non-main publishes within this workflow, so the direct exploit needs a second workflow referencing the unprotected env.) Add a preflight gated on inputs.npm-oidc-enabled that asserts the cheap prerequisites before publish - environment exists with a main-restricted policy, and npm --version satisfies the floor - mirroring the existing VPC sanity-check pattern in this repo.
4. Two load-bearing sections of the runbook are inaccurate. (docs/npm-oidc-migration.md)
The doc is a primary deliverable and operators are told it is "part of the contract," so factual errors matter:
- FM-B's primary recovery tells operators to "inject
NPM_CONFIG_PROVENANCE: 'false'via awith:workaround," then notes it "is not parameterized through the reusable workflow." There is no such input, so the documented primary recovery is not actionable - a wrong first step in a 3am runbook. Make the flag-flip the primary recovery, or add theprovenance-enabledinput the doc gestures at. - The "Security note: binding to reusable workflows" states npm "checks both claims" and that a binding pointed at
mysticat-ciwould let any consumer publish your package (fail-open). This reads as inverted: npm anchors authorization on the caller'sworkflow_ref, and a binding registered against the reusable workflow's repo would match the consumer'srepositoryclaim at runtime and fail closed, not open. The actionable instruction (bind to caller repo + file) is correct either way, so the migration will not break - but please verify the threat-model narrative against npm's trusted-publisher docs before this becomes the authoritative explanation, since it is the doc's central security claim.
5. The rollout plan has no forcing function for Phase 4, risking a permanent dual-path and an undead long-lived secret. (docs/npm-oidc-migration.md)
The doc says the workflow "keeps both paths viable indefinitely," yet gates Phase 4 only on "every consumer has >=2 cycles ... not a calendar date," while admitting the least-active repos "may be weeks out." Deadline-free voluntary migrations across many owners reliably stall with a long tail, and the stragglers block token deletion forever - leaving you carrying both publish paths and the long-lived secret you set out to remove, which is worse than the starting point. The Phase 4 PR is out of scope, but the rollout plan lives in this PR: add a forcing function (a target date for Phase 4, and/or a commitment to migrate laggards or flip the input default at a future major), and reword "indefinitely" to "until Phase 4."
Minor (Nice to Have)
- npm version single source of truth. The floor
>=11.5.1and the pin11.13.0are two numbers to keep in sync. A short comment that 11.13.0 is "latest patch satisfying the >=11.5.1 floor" makes the next bump intentional. (service-ci.yamlline 252) - FM-C and "add a reviewer gate later" snippets weaken the branch policy if followed verbatim. Both set
deployment_branch_policy: { protected_branches: false, custom_branch_policies: true }but never re-add themainentry, so an operator running them under pressure leaves the env accepting any branch until a separate step restoresmain. Include the explicitmainre-add in both snippets. - "Part of the contract" framing. A 636-line prose runbook is documentation, not the enforced contract (the expressions, the trust binding, and the env branch policy are). Given the self-review table is currently wrong, reframing as "operational runbook + rollout plan" avoids anyone trusting the prose over the code.
Recommendations
- Replace the runtime
npm install -g npm@11.13.0with Node-version pinning so the bundled npm satisfies the floor. As written it fetches npm from the public registry on every release, inside the one job withid-token: write- a needless supply-chain fetch and failure point at the most critical moment, for a PR whose purpose is supply-chain hardening. If you keep it, add anpm --versionassertion (a silently-failed upgrade would publish on npm 10 with no trusted-publishing support) and a retry, and note a removal trigger for when runners ship npm >= 11.5.1. (service-ci.yamlline 261) - Add a post-publish "OIDC was actually used" assertion (gated) that checks
npm view <pkg> _npmUseris the GitHub Actions identity and provenance is present, failing loud otherwise - the durable fail-loud control for the whole silent-fallback class, for all 14 consumers not just the canary. Relatedly, make_npmUserthe primary canary gate, not provenance presence:dist.attestationscan be non-empty even on a token-authed publish because provenance signs via the id-token independently of registry auth. - Build the
provenance-enabledescape hatch now (or make provenance non-fatal). Sigstore outages are recurring and fleet-wide; FM-B's current remedy is a per-consumer hotfix or full flip-back, so an outage during a release window could mean up to 14 hotfix PRs. - Confirm the npm-upgrade ordering against
engines.npm. The "Update NPM" step runs after the composite'snpm ci; anengine-strictconsumer whose.nvmrcNode ships npm < 11.5.1 could failnpm cibefore the upgrade. The composite is in this repo, so moving the bump ahead ofnpm ciis an in-repo fix if it bites. - Name a second
adobe-botoperator and a rotation-coordination owner in the rollout plan, rather than caveating the single-credential-holder dependency and the multi-week rotation freeze. - Document the environment-overloading constraint. vpc+oidc consumers bind npm trust to
--environment prod, soprodserves both AWS deploy and npm publish under one set of rules and one identity claim; a future rename breaks publishing too. Partly inherited, so capture it as a deliberate documented constraint.
Out of scope, worth tracking
- The 14 consumer-repo migrations and the Phase 4 token-removal PR are correctly listed as follow-ups.
- A platform helper (composite action or
mysticat-clisubcommand) that derives--repository/--file/--environmentfrom the consumer's context would make the caller-binding rule a mechanical guarantee rather than a doc-discipline one - the highest-leverage way to prevent a cross-consumer mis-binding across 14+ repos. - The
adobe-botPR-bypass plusADOBE_BOT_GITHUB_TOKENmeans provenance attests "built from main," not "reviewed"; a longer-term move to a minimally-scoped GitHub App for the version-bump push is worth tracking. Pre-existing, not introduced here.
Assessment
Ready to merge? No - with fixes. The strategy and the migration mechanism are sound and unusually well-documented, but critical finding 1 means opting in changes nothing about the publish identity (it silently keeps token publishing) while the doc certifies the opposite, so the first canary would be trusting a false fail-fast promise. Fix the NPM_TOKEN expressions (preferably by making the variable unset, not empty), correct the self-review/rollback/FM-A text to match, and add a pre-merge render-test so this class of bug cannot recur. The preflight, the runbook corrections, and the Phase 4 forcing function should land before bulk rollout.
Next Steps
- Fix critical finding 1 in both locations and re-derive the self-review table from a real run.
- Add the pre-merge render-test (finding 2) so the fix is verified and regressions are caught.
- Address the preflight/env-protection gap (finding 3), the two runbook inaccuracies (finding 4), and the Phase 4 forcing function (finding 5).
- Minor items and recommendations are optional, but the post-publish OIDC assertion and dropping the runtime npm install are high-value, low-cost hardening that fit this PR's theme.
| # main-only branch policy. NPM_CONFIG_PROVENANCE=true emits sigstore | ||
| # provenance attestations with every publish. | ||
| # Legacy mode: ADOBE_BOT_NPM_TOKEN authenticates the publish. | ||
| NPM_TOKEN: ${{ inputs.npm-oidc-enabled && '' || secrets.ADOBE_BOT_NPM_TOKEN }} |
There was a problem hiding this comment.
Critical: this returns the real token when OIDC is enabled, so the migration is inert.
${{ inputs.npm-oidc-enabled && '' || secrets.ADOBE_BOT_NPM_TOKEN }} uses the GitHub Actions A && B || C idiom, which returns C whenever B is falsy. '' is falsy, so for npm-oidc-enabled == true: true && '' -> '', then '' || secrets.ADOBE_BOT_NPM_TOKEN -> the token. NPM_TOKEN is the real long-lived secret, not empty. This is the only one of the four new expressions whose intended true-value is falsy, which is why the other three are correct and this one is not.
This release job does not set SR_NO_NPM_AUTH, so @semantic-release/npm runs, sees the token, and publishes via the token - OIDC token exchange never engages. The long-lived secret is not eliminated as the PR claims, and FM-A's "fails fast because NPM_TOKEN: ''" guarantee is inverted (a missing trust binding is silently masked rather than surfaced). It also sets up a Phase 4 landmine: every consumer "migrates" successfully, then deleting ADOBE_BOT_NPM_TOKEN breaks them all at once.
Fix: make NPM_TOKEN genuinely unset (not empty) in OIDC mode - a pre-step gated on if: ${{ !inputs.npm-oidc-enabled }} that writes it to $GITHUB_ENV, or two flag-gated Semantic Release steps. A bare inversion to '' (${{ !inputs.npm-oidc-enabled && secrets.ADOBE_BOT_NPM_TOKEN || '' }}) fixes the "still the real secret" half, but verify first whether npm treats an empty NPM_TOKEN as "use OIDC" or "try the empty token and fail" - omitting the variable is correct under both readings. Then re-derive the self-review/rollback/FM-A text from an actual run.
| # Legacy mode: ADOBE_BOT_NPM_TOKEN authenticates the verify step. | ||
| # Both paths cannot share the same secret slot — NPM_TOKEN is only | ||
| # injected when OIDC is disabled. | ||
| NPM_TOKEN: ${{ inputs.npm-oidc-enabled && '' || secrets.ADOBE_BOT_NPM_TOKEN }} |
There was a problem hiding this comment.
Same bug as the release job: A && B || C with a falsy '' true-branch returns secrets.ADOBE_BOT_NPM_TOKEN when npm-oidc-enabled is true, not ''. Here it is currently masked because this dry-run job also sets SR_NO_NPM_AUTH: 'true', which drops @semantic-release/npm from the dry run so the token goes unused - but fix it alongside the release-job line for consistency and to remove the latent landmine.
| # npm trust to --environment prod. Pure OIDC consumers (no VPC) get the | ||
| # dedicated 'npm-publish' env. Legacy consumers (neither flag set) run | ||
| # without an env claim, matching today's behavior. | ||
| environment: ${{ (inputs.vpc-enabled && 'prod') || (inputs.npm-oidc-enabled && 'npm-publish') || '' }} |
There was a problem hiding this comment.
The expression itself is correct for all four (vpc x oidc) combinations - every true-branch value here is truthy.
The gap is that nothing fails fast when a consumer opts in without completing their side. Referencing a not-yet-created npm-publish environment makes GitHub auto-create it with no protection rules, so the "server-enforced main-only deployment_branch_policy" the doc relies on (to justify skipping the reviewer gate) is absent until the consumer manually creates the env. (Mitigating factor: the release job's own if: github.ref == 'refs/heads/main' still blocks non-main publishes within this workflow.) Consider a preflight step gated on inputs.npm-oidc-enabled that asserts the env exists with a main-restricted policy and npm --version meets the floor - mirroring the existing VPC sanity-check pattern already in this workflow.
…tests
Round-1 review caught a critical defect: the
`${{ inputs.npm-oidc-enabled && '' || secrets.ADOBE_BOT_NPM_TOKEN }}`
ternary returns the real token when OIDC is enabled, because GHA's
`A && B || C` returns C whenever B is falsy and `''` is falsy. The
migration was silently inert. Plus four important findings and several
recommendations. Round-2 self-review via senior-staff-engineer caught
three follow-up bugs in my round-1 fix — addressed inline.
service-ci.yaml:
- Replace NPM_TOKEN ternary with gated `$GITHUB_ENV`-writing pre-steps
for both dry-run and release. In OIDC mode NPM_TOKEN is genuinely
UNSET (not the empty string), correct under both readings of how npm
treats an empty token.
- OIDC preflight asserts the GitHub Environment exists with ONLY 'main'
allowed (exclusivity, not presence) and that npm meets the >= 11.5.1
floor after install. Surfaces `gh api` failures rather than masking
them as policy errors.
- Wrap `npm install -g npm@11.13.0` with 3-attempt retry + explicit
version assertion via `sort -V` so a silently-failed upgrade can't
publish under an unsupported npm.
- Pre/post-publish version snapshot + "Verify OIDC publish identity"
step asserts `_npmUser` contains "GitHub Actions" for the
just-published version. Skips if no new version was published this
run (avoids false-positive against a prior token-era publish).
- Heredoc writes strip trailing newlines from the token to handle
secret stores that append them.
test-publish-path-render.yaml (new):
- Structural grep gate forbids the buggy NPM_TOKEN ternary form and any
inline NPM_TOKEN on the Semantic Release env block; requires the
gated pre-step pattern (verified via heredoc marker count).
- 4-cell matrix (oidc x vpc) renders the publish-path env composition
and asserts the documented outcomes. Uses string matrix values plus
explicit `== 'true'` / `== 'false'` comparisons to sidestep GHA
matrix-value truthiness ambiguity that would otherwise skip the
legacy-arm injection in BOTH matrix cells.
docs/npm-oidc-migration.md:
- Re-derive the self-review table to match the gated-step pattern.
- Rewrite "Security note: binding to reusable workflows" — npm
authorizes against the caller's `workflow_ref` only (per npm docs:
"validation checks the calling workflow's name"). A binding
misregistered against mysticat-ci's reusable workflow fails CLOSED
at publish time, not fail-OPEN. The original threat-model narrative
was inverted; the actionable instruction (bind to caller repo + file)
was correct either way.
- FM-B: flag-flip is now the primary recovery; remove the documented
but non-existent `with: NPM_CONFIG_PROVENANCE: 'false'` workaround.
- FM-C and "add a reviewer gate later" snippets re-add 'main' to the
`deployment_branch_policy` after the PUT (the API clears it).
- Phase 4 forcing function: target = v3.0.0 + 3 months; laggard policy
flips the input default at +30 days; token revocation at +60 days
regardless of per-consumer cycle count.
- Reframe "doc is part of the contract" -> "operational runbook +
rollout plan"; the enforced contract is the workflow expressions,
the trust binding, and the env's branch policy.
- Document `ADOBE_BOT_GITHUB_TOKEN` requires `Administration: read` on
the consumer repo for the preflight to read env settings.
Validation (local):
- YAML parses cleanly (yaml.safe_load).
- All 4 structural grep checks pass against the actual file.
- npm view _npmUser output shape verified against @sigstore/sign,
@actions/core, and vite — all return
"GitHub Actions <npm-oidc-no-reply@github.com>" (string form).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The structural-check job had a bash comment containing the literal text
`${{ !inputs.npm-oidc-enabled` with no closing `}}` on the same line.
Inside a `run: |` block scalar, `#` is plain text rather than a comment,
so GHA's expression parser scans the whole script string, sees the
unbalanced `${{`, and fails workflow validation before any job starts.
Rewrite the comment to describe the pattern in prose rather than
embedding the literal sequence.
The actual grep regex on the next line uses `\\\$\\{\\{` (escaped via
both YAML and bash double-quote rules) so it doesn't contain the literal
trigger sequence and parses cleanly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @alinarublea,
This is a re-review following the 5 findings from the initial review (1 Critical, 4 Important). All have been addressed in commits 5992641 and a8d72d3. The gated-step pattern, test workflow, OIDC preflight, runbook corrections, and Phase 4 forcing function are all well-executed. One new issue surfaced during this pass.
Strengths
- Critical NPM_TOKEN bug fixed correctly. The broken ternary is replaced by gated pre-steps that leave
NPM_TOKENgenuinely UNSET in OIDC mode. The inline comment explaining why the ternary fails (''is falsy in GHA expressions) prevents reintroduction. (service-ci.yamllines 151-170, 391-410) - Executable regression gate added.
test-publish-path-render.yamlstructurally asserts no inlineNPM_TOKEN:on the Semantic Release env, requires gated steps with!inputs.npm-oidc-enabled, and matrix-renders all (oidc x vpc) combinations. CI is green across all 4 cells plus the structural check. - OIDC preflight validates env policy before publish. Checks environment existence, deployment_branch_policy presence, and that only
mainis allowed. Surfaces actionable::error::annotations on failure. (service-ci.yamllines 279-360) - Runbook corrected. FM-B recovery now correctly recommends flipping back to legacy mode (not an unimplemented
provenance-enabledinput). Security note now correctly explains that npm checksworkflow_ref(caller) and that misregistration is fail-closed, not fail-open. (docs/npm-oidc-migration.mdlines 897-920, 1259-1294) - Phase 4 forcing function documented. Hard deadline (v3 tag + 3 months), default flip at +30 days, token deletion at +60 days. Prevents the indefinite dual-path stall. (
docs/npm-oidc-migration.mdlines 1188-1212) - Fail-closed by construction. In OIDC mode, the
NPM_TOKENinjection step is skipped entirely (gated on!inputs.npm-oidc-enabled), so there is no codepath where a misconfigured consumer silently falls back to token publishing. - Environment precedence correctly reasoned. VPC wins because those consumers bind npm trust to
--environment prod. The expression and rationale are both documented inline.
Issues
Important (Should Fix)
1. "Capture pre-publish registry version" step runs AFTER Semantic Release, making the identity verification inert. (service-ci.yaml line 429 vs line 412)
The step's comment says "we snapshot the registry version BEFORE running Semantic Release, then compare to AFTER." But in the actual file ordering, "Semantic Release" is at line 412 and "Capture pre-publish registry version" is at line 429 - it runs after the publish. In the common case (registry propagates within milliseconds), the "pre" capture picks up the already-published version, the verification step sees POST == PRE, prints "No publish in this run," and skips the identity check entirely.
Why it matters: The post-publish _npmUser verification was the prior review's recommended "durable fail-loud control" for the silent-token-fallback class of bugs. With the step in the wrong position, that control is inert for the majority of releases. The preventive control (NPM_TOKEN genuinely absent) is intact and load-bearing, so this is not a security gap, but the detective control that catches regressions is broken.
How to fix: Move the "Capture pre-publish registry version" step to immediately before "Semantic Release" (between "Configure NPM_TOKEN (legacy mode only) - release" and "Semantic Release").
2. OIDC preflight does not validate can_admins_bypass. (service-ci.yaml OIDC preflight step)
The migration doc (line 643) lists can_admins_bypass: false as a required setting, but the preflight only checks deployment_branch_policy. A consumer could pass the preflight with can_admins_bypass: true, allowing an admin to approve a deployment from a non-main branch.
Why it matters: The env-level can_admins_bypass is part of the documented security model. The preflight already checks branch policy strictness; adding this check is consistent with its existing scope.
How to fix: After parsing ENV_BODY, extract and assert:
CAN_BYPASS=$(printf '%s' "$ENV_BODY" | jq -r '.can_admins_bypass // true')
if [ "$CAN_BYPASS" = "true" ]; then
echo "::error::Environment '${ENV_NAME}' allows admins to bypass. Set can_admins_bypass: false."
exit 1
fiMinor (Nice to Have)
- npm version pin (
11.13.0) will drift. The pinned version is conservative and the floor assertion is correct, but there is no mechanism (Dependabot, cron) to surface when a newer patch is available. Consider a comment noting a quarterly review cadence, or widening tonpm@11. (service-ci.yamlline 362) - npm upgrade retry loop does not distinguish retryable from non-retryable errors. A 403 (rate limit) and a disk-full error both get the same 3x5s retry. Low impact since the version assertion catches all failures. (
service-ci.yamllines 378-382) - Identity verification degrades to warning when
_npmUseris empty. This is intentional ("registry hiccup" tolerance) but means a registry metadata outage could mask a token-based publish. The window is small. (service-ci.yamlline 515)
Recommendations
- Track removal of
npm install -g npm@11.13.0when Node LTS ships with npm >= 11.5.1 bundled. This eliminates a public-registry fetch inside the most privileged job (id-token: write+ AWS credentials live). The exact version pin and version assertion mitigate the supply-chain risk, but removing the step entirely is the cleanest long-term fix. - Consider adding
--ignore-scriptsto the npm install (npm install -g npm@11.13.0 --ignore-scripts). Thenpmpackage has no install scripts, but making the intent explicit prevents a future supply-chain vector. - Improve preflight error message for token permission failures. The most common first-run failure will be
ADOBE_BOT_GITHUB_TOKENlackingAdministration: readon a consumer repo, which surfaces as "environment does not exist." Distinguishing 403 from 404 in the error output would reduce debugging friction. - Consider a cron-based health-check workflow that weekly validates the canary package's
_npmUseris still"GitHub Actions". Catches silent regressions from platform changes between releases.
Assessment
Ready to merge? With fixes - move the pre-publish capture step before Semantic Release (finding 1). The can_admins_bypass check (finding 2) is a hardening improvement that could land in a fast follow-up.
Next Steps
- Move "Capture pre-publish registry version" to before "Semantic Release" so the identity verification is functional.
- Optionally add
can_admins_bypassto the preflight (can be a follow-up commit or separate PR). - Minor items and recommendations are non-blocking.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 1s | Cost: $4.86 | Commit: a8d72d3d0c6203026beab56311973a9d7e0eb1f0
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…bypass Two Important findings from MysticatBot's re-review of PR #14: 1. "Capture pre-publish registry version" ran AFTER "Semantic Release", making the post-publish OIDC identity check inert (PRE == POST in the common case → "no publish in this run" short-circuit). Moved the capture step to run before Semantic Release and added a "MUST run before Semantic Release" inline comment so the ordering survives future edits. 2. OIDC preflight did not validate can_admins_bypass, allowing an admin to approve a deployment from a non-main branch and defeat the main-only branch policy. Added an assertion that the env's can_admins_bypass is the literal string "false". Polarity is inverted on purpose (anything other than "false" fails) so missing, null, and future API-shape drift all fail closed. Comment enumerates the three drift cases. Used jq's has()/then/else form rather than `.can_admins_bypass // true` because `//` treats false the same as null and would fire on correctly-configured consumers. Doc: updated the high-level preflight summary in npm-oidc-migration.md so the description matches what the preflight now asserts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @alinarublea,
This is a re-review following the 2 Important findings from the prior review. Both are addressed in commit bcdb3856.
Strengths
- Step ordering fixed correctly (
service-ci.yamllines 435-471): "Capture pre-publish registry version" now runs before "Semantic Release" as intended. The added comment block explicitly explains why ordering matters and names the specific failure class (silent-token-fallback bypass via POST == PRE short-circuit). This prevents future editors from accidentally reordering. can_admins_bypassvalidation is fail-closed and thorough (service-ci.yamllines 330-351): The check usesjq'shas()form to distinguish missing-field from false-field, avoiding the//operator pitfall wherefalse // truereturnstrue. The inline comment enumerates all three drift cases (field missing, field null, future API shape change) and explains why the obvious shortcut is wrong. Actionable error message tells the consumer exactly what to fix and where.- Documentation updated in lockstep (
docs/npm-oidc-migration.md): The preflight description now reflects thecan_admins_bypasscheck immediately, keeping docs and behavior aligned. - Error messages are runbook-grade: Both the bypass-check failure and the environment-not-found path tell the operator the current value, which environment is misconfigured, and the exact Settings path to fix it.
Assessment
Ready to merge? Yes.
Both prior findings are addressed cleanly. The can_admins_bypass gate closes a real configuration-drift gap in the security model, and the step reordering restores the detective control that catches silent token fallback. CI is passing across all 4 render-matrix cells plus the structural check. No new issues introduced.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 37s | Cost: $3.63 | Commit: bcdb3856dc7ff74eccd074b5d5443a570db9dbb2
If this code review was useful, please react with 👍. Otherwise, react with 👎.
3 participants
Summary
Adds an opt-in
npm-oidc-enabledinput to the reusableservice-ci.yamlworkflow, mirroring the model landed inadobe/spacecat-shared#1592. Existing consumers see zero behavior change until they explicitly flip the input. To be cut asv3.0.0ofadobe/mysticat-ci.When
npm-oidc-enabled: true:SR_NO_NPM_AUTH=trueand noNPM_TOKEN(consumer's.releaserc.cjsmust gate@semantic-release/npmonprocess.env.SR_NO_NPM_AUTH === 'true').NPM_CONFIG_PROVENANCE=true, noNPM_TOKEN, andenvironment: npm-publish(orprodwhenvpc-enabledis also true, so VPC consumers keep their existing env config and bind their npm trust with--environment prod).Update NPMstep pinsnpm@11.13.0(OIDC floor is>= 11.5.1).When
npm-oidc-enabled: false(default): unchanged — legacyADOBE_BOT_NPM_TOKENpath.Why this matters
ADOBE_BOT_NPM_TOKENas a long-lived org secret once all consumers have migrated. Trapped one cycle in spacecat-shared already (silent token rot broke an unrelated PR; the OIDC binding registered the same morning bypassed the bad token entirely).mainin the caller repo via the caller's workflow.Release + rollout strategy
Full plan in
docs/npm-oidc-migration.md→ "Release + rollout strategy". Summary:Phase 0 — Cut
v3.0.0(when this PR merges)v3.0.0; the floatingv3tag picks it up via the existingrelease.yaml.Phase 1 — Canary (1 repo, ≥ 1 week observation)
spacecat-jobs-dispatcher→spacecat-task-processor→mysticat-projector-service.main.npm view @adobe/<pkg> _npmUserreturns"GitHub Actions".npm view @adobe/<pkg> dist.attestationsnon-empty.workflow.pathfield points at the canary's caller workflow, NOTadobe/mysticat-ci/.github/workflows/service-ci.yaml. (If it points at the latter, the trust binding was registered againstjob_workflow_ref— fail-stop and re-register againstworkflow_ref.)npm-oidc-enabled: false, fix in a follow-up, re-enter Phase 1.Phase 2 — Pilot batch (3 repos, ≥ 1 week)
--environment prodbinding) + 1 multi-package consumer (validates the per-package binding registration loop).Phase 3 — Bulk rollout (remaining 10 repos)
adobe/mysticat-ci(one checkbox per repo).Phase 4 — Token decommissioning (gated on every consumer crossing the ≥ 2-cycle threshold)
NPM_TOKENinjection branch in the conditionals.ADOBE_BOT_NPM_TOKENfrom org secrets.adobe-bot:npm token revoke <id>.v4.0.0to reflect the contract change.Meta-risks for the rollout itself (documented in full):
mysticat-ci@mainpins on any consumer (anti-pattern but harmless under default-false).adobe-botbus factor (lock in a second operator).ADOBE_BOT_NPM_TOKENrotation collision mid-rollout (pause rotation or prioritize token-dependent consumers).Self-review
Audited before opening this PR. Default-false code paths produce identical behavior to the pre-PR workflow:
NPM_TOKENon dry-run env''SR_NO_NPM_AUTHon dry-run env''evaluates as unset for GH)'true'NPM_TOKENon release env''NPM_CONFIG_PROVENANCEon release env'')'true'environment:claim'prod'ifvpc-enabled, else none'prod'ifvpc-enabled, else'npm-publish'Update NPMstepif:evaluates false)npm@11.13.0)id-token: writepermissionThe
''-vs-unset distinction is functionally inert for the three consumer-facing reads:@semantic-release/npmchecksprocess.env.NPM_TOKENtruthiness;''is falsy, identical to unset.process.env.SR_NO_NPM_AUTH === 'true'(strict equality) returns false for both''andundefined.process.env.NPM_CONFIG_PROVENANCE = ''is read by npm as "config not set," identical to unset.Rollback (backwards compatibility)
The contract is designed so that every backwards transition is a single flag flip.
Scenario A — Consumer-side rollback (after opting in)
Symptom: a consumer flipped
npm-oidc-enabled: true, then a regression / outage / policy reversal makes the token path preferable again.Procedure (single PR on the consumer repo):
npm-oidc-enabled: true→false(or remove the input).mainuses the legacyNPM_TOKENpath automatically..releaserc.cjsSR_NO_NPM_AUTHguard is a no-op when the env var is unset.engines.npm: >=11.5.1is harmless to keep.npm-publishGitHub Environment can stay in place.Scenario B — mysticat-ci PR-level rollback
Symptom: this PR caused a regression for a default-false consumer.
Procedure:
git revert <merge-sha>and cut a hotfixv3.0.1(or do not cutv3at all and require consumers to remain on@v2). The self-review table above audits every default-false code path; if a regression slips through, the revert restoresservice-ci.yamlverbatim with zero downstream coordination required.Scenario C — Permanent revert post-rotation
Symptom: all consumers migrated,
ADOBE_BOT_NPM_TOKENwas rotated out, then a policy decision is made to revert.Procedure: mint a new
ADOBE_BOT_NPM_TOKEN, push to the org secret store. For each consumer, flipnpm-oidc-enabled: true→false. Optionally revoke npm trust bindings (npm trust revoke <pkg> --id <id>) for cleanliness, not required.The reusable workflow's conditional logic keeps both paths viable indefinitely. Removal of the legacy path is a separate decision and not part of this PR (it's Phase 4 of the rollout).
Failure modes when
npm-oidc-enabled: trueDocumented in full in
docs/npm-oidc-migration.md. Quick reference:NPM_TOKEN=''so no silent fallback). Recovery: register the binding OR flip back tofalse.NPM_CONFIG_PROVENANCE: 'false'override OR flip back tofalse. Watch https://status.sigstore.dev/.gh api PUT(gh command in the doc) OR approve via UI.Consumer migration
See
docs/npm-oidc-migration.md→ "Order of operations checklist" for the full per-consumer checklist:SR_NO_NPM_AUTH === 'true'guard to.releaserc.cjs.engines.npmto>=11.5.1 <12.0.0.npm-publishGitHub Environment + tighten branch protection onmain.adobe-bot:npm trust github @adobe/<pkg> --repository adobe/<consumer-repo> --file <caller-workflow>.yaml --environment npm-publish --yesfor each package.npm-oidc-enabled: truein the caller workflow.Critical security note (binding to reusable workflows)
When
service-ci.yamlruns viaworkflow_call, GitHub mints an OIDC token with both:workflow_ref— the caller's workflow (adobe/spacecat-api-service/.github/workflows/main.yaml@refs/heads/main)job_workflow_ref— this reusable workflow (adobe/mysticat-ci/.github/workflows/service-ci.yaml@refs/tags/v3.0.0)Bind packages to
workflow_ref(the caller), NOTjob_workflow_ref(mysticat-ci). A binding pointing atmysticat-ci/service-ci.yamlwould let any consumer of this reusable workflow publish another consumer's package. The npm CLI'snpm trust githubbinds againstworkflow_refwhen you pass--repository <consumer-repo> --file <caller-filename>.This is documented inline in the workflow comment and in
docs/npm-oidc-migration.md. The Phase 1 success criteria includes an explicit check on the attestation'sworkflow.pathfield to catch a mis-registered binding before the rollout propagates.Test plan
v3.0.0.--environment prodbinding path.Out of scope (follow-ups, not blocking this PR)
spacecat-shared/scripts/setup-npm-trusted-publishers.sh, parameterized for the caller repo + package list).ADOBE_BOT_NPM_TOKENfrom org secrets + the legacy branch fromservice-ci.yaml.provenance-enabledinput on the reusable workflow (parameterizesNPM_CONFIG_PROVENANCEas a sigstore-outage break-glass). Add only if FM-B becomes recurring.Commits
af52621— initial workflow change + first version ofdocs/npm-oidc-migration.md.5ddfdb4— self-review: expanded rollback (scenarios A/B/C), four failure modes, backward-compat audit table, deduplicated checklist.e300314— release + phased rollout strategy (Phases 0–4, canary selection criteria, communication template, meta-risks).🤖 Generated with Claude Code