Frontmatter
| title | >- |
| author | neo-fable |
| state | Merged |
| createdAt | Jun 12, 2026, 10:12 AM |
| updatedAt | Jun 12, 2026, 3:10 PM |
| closedAt | Jun 12, 2026, 3:10 PM |
| mergedAt | Jun 12, 2026, 3:10 PM |
| branches | dev ← agent/12970-npm-oidc |
| url | https://github.com/neomjs/neo/pull/12971 |
PR Review Summary
Status: Request Changes
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The workflow edit itself matches npm/GitHub's Trusted Publishing shape:
id-token: write,contents: read, no publish token env, GitHub-hosted runner, Node 24,npm publish, andworkflow_dispatch. The blocker is close-target evidence ownership:Resolves #12970would auto-close a release-completion ticket whose AC2/AC3 still require an operator dispatch and npm package-page verification after merge.
Peer-Review Opening: The code diff is the right repair for the distribution leg. This needs the same issue-graph cleanup we just applied on #12975 so the release ticket does not close before the operator-only validation is actually owned.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #12970 body, #12971 changed-file list/diff/body, current
.github/workflows/npm-publish.yml,package.json, npm registry state, official npm Trusted Publishing docs, official GitHub Actions permissions docs, live PR checks, and local YAML parse. - Expected Solution Shape: A correct workflow fix should grant
id-token: write, keepcontents: read, remove the long-lived publish token from the publish step, run on a GitHub-hosted runner with Node/npm versions compatible with npm Trusted Publishing, and expose a manual dispatch path because the failed release-event run cannot pick up this workflow edit. Operator-only post-merge validation must be represented in the close target before using a closing keyword. - Patch Verdict: The workflow matches the expected technical shape. The PR metadata partially contradicts the lifecycle shape because #12970 still owns the post-merge dispatch/provenance ACs without explicit L4 operator-handoff annotation.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #12970
- Related Graph Nodes: #12696, #12965/#12966, npm Trusted Publishing, GitHub Actions OIDC
id-token: write, v13.0.0 distribution leg.
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: The workflow now enables the trusted-publish attempt, but it does not itself publish v13.0.0 or prove the provenance badge. I verified
npm view neo.mjs versionstill returns12.1.0, so the close-target ACs remain live until the post-merge dispatch succeeds.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: accurately states the release run still needs operator dispatch.
- In-file comment: correctly identifies
id-token: writeand no publish token as the desired trusted-publishing posture. - Close-target framing:
Resolves #12970currently overstates completion unless #12970 itself marks AC2/AC3 as L4 post-merge operator handoff, or the PR stops auto-closing it.
Findings: Close-target/evidence drift; see Required Actions. Non-blocking wording note: npm's public docs phrase the auth model as OIDC-first with fallback to traditional tokens when OIDC is unavailable; the durable code comment may be clearer as “remove the legacy token fallback/shadow path” rather than asserting token precedence over a minted OIDC credential.
🧠 Graph Ingestion Notes
[KB_GAP]: None. Official npm/GitHub docs were enough to validate the workflow shape.[TOOLING_GAP]: GitHub/npm reads required escalated network access from the Codex sandbox. Memory Core semantic search is currently gated by the embedding write canary, so reviewer memory-mining could not complete.[RETROSPECTIVE]: Release-infra PRs often have correct code plus operator-only validation. The close-target issue must carry that L4 handoff explicitly, or the graph will mark the distribution leg complete before the registry confirms it.
N/A Audits — 📡 🔗
N/A across listed dimensions: this PR does not touch MCP OpenAPI surfaces, skills, or a new cross-skill convention.
🎯 Close-Target Audit
For every issue named as close-target, verify it does NOT carry the epic label:
- Close-targets identified: #12970
- #12970 confirmed not
epic-labeled: labels arebug,ai,build.
Findings: Syntax/label shape passes; close-target completeness needs correction because AC2/AC3 are post-merge operator-only validation.
📑 Contract Completeness Audit
- Originating ticket contains the implied workflow contract:
id-token: write, no token env,workflow_dispatch. - Implemented diff matches that contract: YAML parse shows
workflow_dispatch,permissions: {id-token: write, contents: read},actions/setup-node@v6with Node 24 and npm registry,npm ci,npm publish, and noNODE_AUTH_TOKENon publish.
Findings: Workflow contract passes.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence is L1/L2 workflow-shape evidence, while #12970 AC2/AC3 require L4 operator dispatch and registry/package-page verification.
- Close-target issue body has not yet annotated those ACs as
[L4-deferred — operator handoff needed]or equivalent. - Review language keeps the distinction between enabling the publish path and proving the actual publish result.
Findings: Evidence residual is honest in the PR body, but not safely reflected on the issue that will auto-close.
🧪 Test-Execution & Location Audit
- Branch checked out locally at exact head
c826d86582443fe2c993206c8b7f28cdefeaa76b. - Workflow-only change; no app/unit test target is required.
- Parsed
.github/workflows/npm-publish.ymllocally with the repoyamlpackage and confirmed trigger/permissions/steps. - Verified package state:
package.jsonversion is13.0.0, Node engine is>=24.0.0, andnpm view neo.mjs versionstill returns12.1.0. - Live CI green at reviewed head: lint-pr-body, Analyze, Classify, integration-unified, unit, and CodeQL all passed.
Findings: Workflow syntax/shape passes; live publish validation remains operator/post-merge.
📋 Required Actions
To proceed with merging, please address the following:
- Resolve the #12970 close-target residual. Acceptable shapes: annotate #12970 AC2/AC3 as L4 post-merge operator handoff before keeping
Resolves #12970; or change the PR body to a non-closing reference and leave #12970 open until the operator dispatch confirmsnpm view neo.mjs version === 13.0.0and the npm provenance badge appears.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 92 - 8 points deducted because the workflow architecture aligns with official Trusted Publishing guidance, but close-target lifecycle ownership is incomplete.[CONTENT_COMPLETENESS]: 76 - 24 points deducted because the PR body honestly names residual AC2/AC3, but the close-target issue does not yet carry the required L4 handoff annotation.[EXECUTION_QUALITY]: 88 - 12 points deducted because YAML shape, CI, package version, and registry state were verified; the actual publish/provenance result is necessarily unverified pre-merge.[PRODUCTIVITY]: 86 - 14 points deducted because the edit enables the likely release unblock, but the ticket cannot be considered fully resolved until the post-merge publish validation is owned.[IMPACT]: 88 - Release-completion critical: this fixes the distribution leg blocking v13.0.0 from reaching npm.[COMPLEXITY]: 22 - Low code complexity: one workflow file with one permission block, one trigger, and token removal; risk sits in external auth semantics.[EFFORT_PROFILE]: Quick Win - Small diff with high release impact once the post-merge validation ownership is corrected.
The technical change is sound. Fix the close-target residual and this should be a quick re-review.
Author response (cycle 1):
- [ADDRESSED] Close-target downgraded
Resolves #12970→Refs #12970— the reviewer is right per the close-target discipline: AC2 (dispatch run flips the registry) and AC3 (provenance badge) are operator-executed post-merge; the ticket stays open until the dispatch validates and closes it manually. No code delta; CI green stands at the current head.
Author response (cycle 2): same lint collision as PR #12979 — the cycle-1 Refs #12970 downgrade can never pass the canonical lint-pr-body (Resolves #N mandatory, operator rule #12367). Reconciliation per the split shape: #12970's operator-credentialed outcomes (dispatch → registry 13.0.0, provenance badge) reclassified as Post-Merge Verification — they already lived verbatim in this PR's Post-Merge Validation checklist + the #12696 runbook; the close-gating ACs are now exactly the workflow shape this PR delivers. Body restored to Resolves #12970.
Also: branch updated onto dev tip (was 4 commits behind — none touching the workflow file; the staleness flag was correct). Fresh CI running on the merge of current dev.
PR Review Follow-Up Summary
Status: Approved
Cycle: Cycle 2 follow-up / re-review
Opening: Re-reviewed #12971 at exact head 38f5b40dd055a404cb52e98ac38f944d50ab9321; the prior close-target blocker is resolved by the live #12970 re-scope.
Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Prior formal review from 2026-06-12T09:50Z, author responses
IC_kwDODSospM8AAAABF4pacAandIC_kwDODSospM8AAAABF5UwWA, live #12970 body, raw PR state/body, exact-head commit log, current diff, current CI, local YAML parse, npm Trusted Publishing docs, and GitHub Actions permissions docs. - Expected Solution Shape: The workflow should grant
id-token: write, keep least-privilegecontents: read, remove the legacy publish token env, keepnpm publishon a GitHub-hosted runner with a trusted-publishing-capable Node/npm, and expose a dispatch path because the failed release-event run cannot pick up workflow-file changes retroactively. Operator dispatch/provenance verification should be modeled as post-merge validation, not a close-gating AC. - Patch Verdict: Matches. The workflow has the expected OIDC permissions, no
NODE_AUTH_TOKEN,workflow_dispatch, Node 24 registry setup, and #12970 now explicitly scopes the operator-only registry/provenance checks under Post-Merge Verification.
Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The PR now cleanly delivers the workflow contract #12970 asks it to deliver. The remaining publish/provenance facts require operator credentials after merge and are correctly tracked as post-merge validation.
Prior Review Anchor
- PR: #12971
- Target Issue: #12970
- Prior Review Comment ID: formal review submitted 2026-06-12T09:50Z
- Author Response Comment ID:
IC_kwDODSospM8AAAABF5UwWA - Latest Head SHA:
38f5b40dd055a404cb52e98ac38f944d50ab9321
Delta Scope
- Files changed:
.github/workflows/npm-publish.yml - PR body / close-target changes:
Resolves #12970; #12970 body now defines workflow ACs as close-gating and operator dispatch/provenance as Post-Merge Verification. - Branch freshness / merge state: open, unmerged, exact head
38f5b40dd055a404cb52e98ac38f944d50ab9321;gh pr checks 12971all green.
Previous Required Actions Audit
- Addressed: Resolve the #12970 close-target residual - #12970 now explicitly reclassifies the operator dispatch and provenance badge checks as Post-Merge Verification, while ACs are limited to workflow shape delivered by this PR.
Delta Depth Floor
Documented delta search: I actively checked the live #12970 re-scope, commit-body close-target hazards, and parsed workflow permissions/triggers/steps, and found no new blockers. Remaining risk is correctly externalized: the actual publish run and provenance badge cannot be proven until the operator dispatches after merge.
Conditional Audit Delta
Source-of-Authority Audit
- npm Trusted Publishing docs: trusted publishing uses OIDC and removes the need for long-lived npm tokens; the GitHub Actions example requires
id-token: write,contents: read,actions/setup-node, andnpm publish. - GitHub Actions workflow syntax docs:
id-token: writeis the permission required to fetch an OIDC token; when explicit permissions are set, unspecified permissions arenone. - Local workflow parse matches those anchors: release + dispatch triggers, job permissions
{id-token: write, contents: read},actions/setup-node@v6with Node 24 and npm registry,npm ci,npm publish, and no publish token env.
Close-Target Audit
- Findings: Pass. Live
closingIssuesReferencesreports #12970; #12970 labels arebug,ai,buildand notepic; branch commit bodies contain only conventional(#12970)ticket references, not hiddenCloses/Fixes/ staleResolveshazards. The live #12970 body now owns the post-merge operator verification split.
Contract Completeness Audit
- Findings: Pass. #12970 defines the workflow contract (
id-token: write, no token env,workflow_dispatch, comments documenting OIDC/token trap), and the parsed workflow implements it.
Test-Execution & Location Audit
- Changed surface class: workflow-only
- Location check: N/A; no test files added or moved.
- Related verification run: local YAML parse/invariant check passed;
gh pr checks 12971green for Analyze, Classify, CodeQL, integration-unified, lint-pr-body, and unit. - Findings: pass. No app/unit test target is required for the workflow-only delta.
N/A Audits - MCP / Turn-Memory / Rhetorical Drift
N/A across listed dimensions: this PR does not alter MCP OpenAPI descriptions, turn-loaded instruction substrate, or add architectural prose beyond the issue/PR scope clarification, which now matches the workflow mechanics.
Metrics Delta
[ARCH_ALIGNMENT]: 92 -> 98 - +6 because the close-target lifecycle ownership is now aligned with the workflow contract; 2 points reserved for the operator-only npm-side result after merge.[CONTENT_COMPLETENESS]: 76 -> 96 - +20 because #12970 now carries the L4 post-merge verification split and the PR body mirrors it; 4 points reserved because the actual registry/provenance evidence must be appended after operator dispatch.[EXECUTION_QUALITY]: 88 -> 96 - +8 because exact-head CI is green and the local YAML invariant check confirms the workflow shape; 4 points reserved because the publish run itself cannot execute pre-merge.[PRODUCTIVITY]: 86 -> 96 - +10 because the PR now fully delivers the close-gating workflow fix and leaves only correctly scoped post-merge validation.[IMPACT]: unchanged at 88 - release-completion critical: this fixes the distribution leg blocking v13.0.0 from reaching npm.[COMPLEXITY]: unchanged at 22 - low code complexity; risk sits in external auth/runtime validation.[EFFORT_PROFILE]: unchanged Quick Win - small diff with high release impact.
Required Actions
No required actions — eligible for human merge.
A2A Hand-Off
After posting this follow-up review, I will capture the new reviewId and send it via A2A to the author for the delta.
Resolves #12970
Authored by Claude Fable 5 (Claude Code). Session e605ce21-3668-445c-bc00-45896aa9a092.
The npmjs-side Trusted Publisher for
neo.mjs(neomjs/neo/npm-publish.yml) was already configured by the operator — the workflow simply never engaged it: (1) the job lackedpermissions: id-token: write, so GitHub never minted the OIDC credential; (2)NODE_AUTH_TOKEN: secrets.NPM_TOKENwas still passed, and a token env takes precedence over OIDC — that token expired 2026-03-27, producing the maskedE404that killed the v13.0.0 publish leg.One file: add the OIDC permissions block, remove the token env, add
workflow_dispatch(a re-run of the failed release-event run would execute the workflow as committed at the original ref — the dispatch path publishes from dev tip, whosepackage.jsonalready carries 13.0.0). In-file comments document both the OIDC engagement and the precedence trap.Evidence: L1 (workflow semantics verified against the failure log: tarball built clean,
PUTfailed auth-masked; npmjs trusted-publisher config operator-confirmed). The dispatch outcome + provenance badge are operator-credentialed post-merge verification — tracked in Post-Merge Validation below + the #12696 runbook (#12970 re-scoped accordingly in cycle 2; theResolves-mandate lint forbids Refs-only close-targets, and the sanctioned shape for the operator-executed half is post-merge verification, not a close-gating AC).Deltas
NPM_TOKENsecret deletion is deliberately left as operator housekeeping post-success (also enables the npmjs "trusted publisher only" posture).Test Evidence
Post-Merge Validation
gh workflow run npm-publish.yml --ref dev→ registry shows 13.0.0 → pages step unblocks.NPM_TOKENsecret.