mcp: npm release CI pipeline

[bpowers]

Summary

• Phase 1 (Scaffolding): Removed stale darwin-x64 package, fixed Windows target triple to x86_64-pc-windows-gnu, added publishConfig and repository fields to all package.json files (wrapper + generated platform packages), updated integration test to cover all 4 platforms and new fields.
• Phase 2 (Cross-Build): Added Dockerfile.cross and scripts/cross-build.sh for local Docker-based cross-compilation (Linux x64/arm64 musl, Windows x64 mingw). Includes ELF format validation and exit code smoke test.
• Phase 3 (CI Workflow): Created .github/workflows/mcp-release.yml with 4 jobs: version validation, 4-platform build matrix (zigbuild for Linux/Windows, native for macOS), platform package publish, wrapper publish. Uses OIDC trusted publishing with provenance attestation and least-privilege permissions.
• Tests: 11 new automated tests across 2 test files (build_npm_packages.rs, mcp_release_workflow.rs) covering all 21 acceptance criteria that can be statically verified.
• Docs: Updated CLAUDE.md files, added human test plan.

Test plan

• cargo test -p simlin-mcp passes (76 tests, 0 failures)
• Pre-commit hooks pass (Rust fmt/clippy/test, TypeScript lint/build/tsc/test, pysimlin)
• Phase 4 manual steps: create @simlin npm org, bootstrap publish, configure Trusted Publisher, push mcp-v* tag, verify end-to-end install (see docs/test-plans/2026-03-12-mcp-npm-release.md)
• Run cross-build.sh with Docker available to verify binary output

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bb72afd9c0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Align cross-build Rust version with repo toolchain

The cross-build image defaults to RUST_VERSION=1.87.0, while the repo is pinned to Rust 1.94.0 in rust-toolchain.toml; this means scripts/cross-build.sh compiles with an older compiler than normal project builds unless callers remember to override the build arg. As soon as simlin-mcp or a dependency needs a post-1.87 feature, local cross-build validation will fail even though standard builds succeed.

Useful? React with 👍 / 👎.

[claude]

Code Review

[P3] Weak test assertions in ac2_5_chmod_for_non_windows_binaries don't verify per-binary chmod

File: src/simlin-mcp/tests/mcp_release_workflow.rs:202-216

Each assertion checks text.contains("chmod +x") && text.contains("mcp-darwin-arm64") as two independent whole-file searches. Since mcp-darwin-arm64 appears in many other workflow lines (artifact names, cp commands, build matrix), the second condition is always true as long as the platform exists in the workflow at all. This means the test effectively only checks that chmod +x appears somewhere in the file -- if someone removed the chmod for one specific platform while keeping the others, the test would still pass. A line-level check (e.g. finding lines that contain both chmod +x and the specific binary path) would actually verify the intended invariant.

---

Overall Correctness Verdict: Correct

The patch is well-structured and free of blocking issues. The CI workflow correctly separates build and publish concerns, uses least-privilege permissions, and the OIDC trusted publishing setup follows best practices (no registry-url, --provenance on all publish commands, id-token: write only on publish jobs). The build scripts, package.json files, Dockerfile, and integration tests are all consistent and functional. The test weakness noted above is non-blocking.

[codecov]

Codecov Report

❌ Patch coverage is 88.93130% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.01%. Comparing base (0fa352a) to head (9aa8bdb).

Files with missing lines	Patch %	Lines
src/simlin-mcp/tests/mcp_release_workflow.rs	88.78%	25 Missing ⚠️
src/simlin-mcp/tests/build_npm_packages.rs	89.74%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
+ Coverage   76.92%   77.01%   +0.08%     
==========================================
  Files         143      144       +1     
  Lines       36549    36810     +261     
==========================================
+ Hits        28115    28348     +233     
- Misses       8434     8462      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Code Review

[P2] Cargo.lock lists serde_yaml but Cargo.toml specifies serde_yml

File: Cargo.lock (lines around the simlin-mcp dependency block and the new serde_yaml package entry)

The Cargo.toml dev-dependency is serde_yml = "0.0.12", but the Cargo.lock adds serde_yaml (version 0.9.34+deprecated) — a different crate — as a direct dependency of simlin-mcp. Grepping the full Cargo.lock confirms there is no serde_yml entry at all. The test code uses serde_yml::Value and serde_yml::from_str, which only compile against the serde_yml crate. CI passes because cargo test --workspace (no --locked) silently re-resolves the dependency, but the committed lock file is stale: it locks the wrong crate and is missing the intended one. Anyone running cargo test --locked or relying on the lock file for reproducibility will hit a resolution failure. Regenerating with cargo update -p serde_yml (or just cargo test and committing the resulting lock file) should fix it.

---

Verdict: Correct (with caveat)

The patch is functionally correct — the CI workflow, build scripts, platform JS launcher, and tests are all well-structured and the logic is sound. The Cargo.lock inconsistency above is a non-blocking reproducibility issue (builds auto-resolve without --locked) but should be cleaned up.

[claude]

Code Review: mcp: npm release CI pipeline

Findings

[P2] Cargo.lock is inconsistent with Cargo.toml dev-dependency name

File: Cargo.lock (lines added in diff) / src/simlin-mcp/Cargo.toml:23

The Cargo.toml declares serde_yml = "0.0.12" as a dev-dependency, but the Cargo.lock resolves and lists serde_yaml version 0.9.34+deprecated — an entirely different crate (serde_yaml was deprecated by David Tolnay; serde_yml is its community successor). The test code at src/simlin-mcp/tests/mcp_release_workflow.rs:13 uses serde_yml::Value, which requires the serde_yml crate to be resolved. Building with cargo test --locked would fail because the lockfile doesn't match the manifest, and running without --locked would silently rewrite the lockfile. Regenerate Cargo.lock (e.g., cargo update -p serde_yml) so it correctly resolves serde_yml instead of serde_yaml.

---

Overall Correctness Verdict

Correct with caveat. The Cargo.lock/Cargo.toml mismatch is the only substantive issue. The workflow structure, publish logic, security permissions, platform mappings, test coverage, and shell scripts are all well-designed. The npm view idempotency guards, OIDC-only auth, tag validation, chmod +x after artifact download, and per-job permission scoping are all correct. The lockfile mismatch won't block most local development (since cargo test without --locked auto-resolves), but it should be fixed for correctness and reproducibility.

[claude]

Code Review

[P2] Cargo.lock is out of sync with Cargo.toml — serde_yaml vs serde_yml

File: Cargo.lock (lines in the simlin-mcp dependency block)
Also: src/simlin-mcp/Cargo.toml:23

Cargo.toml adds serde_yml = "0.0.12" as a dev-dependency, and the test code (mcp_release_workflow.rs) imports serde_yml::Value / serde_yml::from_str. However, the committed Cargo.lock contains serde_yaml (version 0.9.34+deprecated) — a different crate — and has no entry for serde_yml at all. Running cargo build --locked -p simlin-mcp (or any locked build) will fail because the lock file doesn't match the manifest. Normal cargo build silently regenerates the lock file so the mismatch isn't caught locally, but it breaks reproducible builds and will cause CI failures if --locked is ever added. Regenerating the lock file with cargo update -p serde_yml (or just cargo check -p simlin-mcp and committing the updated Cargo.lock) should fix it.

---

Overall Correctness

Correct, with the caveat above. The workflow structure, permissions model, platform matrix, JS launcher, build scripts, Dockerfile, and tests are all well-constructed. The Cargo.lock mismatch is a non-blocking issue since the current CI commands don't use --locked, but it should be fixed to keep the lock file accurate.

[claude]

Code Review: PR #394 — mcp: npm release CI pipeline

Findings

[P2] Cargo.lock is out of sync with Cargo.toml for the serde_yml dependency

File: src/simlin-mcp/Cargo.toml:23 / Cargo.lock

Cargo.toml specifies serde_yml = "0.0.12" (the successor crate to serde_yaml), and the test code imports serde_yml::Value. However, the committed Cargo.lock contains serde_yaml = "0.9.34+deprecated" as a direct dependency of simlin-mcp and has no entry for serde_yml at all. This means the lockfile was generated when the dep was still serde_yaml, and wasn't regenerated after the rename. Without --locked, Cargo silently resolves the mismatch at build time, but cargo test --locked -p simlin-mcp will fail because the lockfile doesn't contain the serde_yml package. Regenerating the lockfile (cargo update -p serde_yml) should fix this.

---

Overall Correctness Verdict

Correct — modulo the lockfile sync issue above (which is non-blocking if CI doesn't enforce --locked for cargo test). The workflow structure, JS launcher changes, build scripts, platform package scaffolding, and static validation tests are all well-designed and internally consistent. Security permissions (least-privilege, OIDC, no stored tokens) are correctly implemented. The npm view guards for idempotent re-runs and the dev fallback for Windows MSVC are nice touches.

[claude]

Code Review: PR #394 — mcp: npm release CI pipeline

Findings

[P1] Cargo.lock is inconsistent with Cargo.toml for serde_yml dev-dependency

src/simlin-mcp/Cargo.toml:23 specifies serde_yml = "0.0.12", and the test code in mcp_release_workflow.rs uses serde_yml::Value and serde_yml::from_str. However, the Cargo.lock contains no [[package]] entry for serde_yml — it only has serde_yaml (version 0.9.34+deprecated), listed as a dependency of simlin-mcp. These are distinct crates on crates.io. The lock file appears to have been generated when the dependency was serde_yaml, and then the Cargo.toml was changed to serde_yml without regenerating the lock. This means cargo build --locked or cargo test --locked will fail, and a fresh cargo test will modify the committed Cargo.lock. The implementation plan (test-requirements.md) also refers to serde_yaml, confirming the rename happened after lock generation. Either switch back to serde_yaml (matching the lock) or regenerate the lock file with serde_yml resolved.

File: src/simlin-mcp/Cargo.toml, line 23 / Cargo.lock, lines 2053–2062

---

Overall Correctness

Not correct. The Cargo.lock / Cargo.toml mismatch for the serde_yml dev-dependency will cause --locked builds to fail and means the committed lock file does not represent a reproducible build. The rest of the PR (workflow structure, JS launcher changes, build script updates, test coverage) is well-designed and correct. Fixing the lock file inconsistency should be straightforward — either regenerate the lock with the current Cargo.toml or switch the dep name to match the lock.

[claude]

Code Review

I reviewed all 20 changed files in this PR covering the MCP npm release CI pipeline (scaffolding cleanup, cross-build tooling, GitHub Actions workflow, and static validation tests).

Findings

No bugs found.

Notes

• The workflow structure is well-designed: validate → build (4-platform matrix) → publish-platform → publish-wrapper, with proper job dependency ordering.
• Security model is sound: top-level contents: read, id-token: write only on publish jobs, no stored npm tokens, OIDC trusted publishing.
• The 11 new static validation tests in build_npm_packages.rs and mcp_release_workflow.rs provide good regression coverage for structural properties of the workflow and build scripts.
• Publish steps are idempotent (guarded by npm view checks), making partial-failure reruns safe.
• The cross-build.sh script correctly skips the execution smoke test on non-x86_64 Linux hosts.
• The simlin-mcp.js launcher now correctly targets x86_64-pc-windows-gnu for npm packages while preserving a dev fallback to the MSVC triple.

Overall Correctness Verdict

Correct. The patch is well-structured, properly tested, and free of bugs. Existing code and tests will not break.

[claude]

Code Review: mcp: npm release CI pipeline

Findings

No blocking bugs found. The PR is well-structured with thorough test coverage of the CI workflow, platform scaffolding, and cross-build tooling.

Minor observations (non-blocking):

1. mcp_release_workflow.rs:362-368 — Fragile Zig version extraction. The dockerfile_zig_version_matches_workflow test uses find_map to grab the first version: '...' line from the entire workflow YAML. This currently works because the Zig step is the first to use that exact key format, but if a step with a bare version: key were added above it, the test would silently check the wrong value. Consider matching on a more specific pattern (e.g., checking that the preceding line contains "zig" or "setup-zig"). Not a bug today — just fragile.

2. serde_yaml (deprecated) pulled in as transitive dependency. The serde_yml = "0.0.12" dev-dependency brings in serde_yaml 0.9.34+deprecated via unsafe-libyaml. This works fine but the serde_yml crate itself is at a 0.0.x version. Worth keeping an eye on for a future migration but not actionable now.

Verdict: Correct

The patch does not break existing code or tests. The workflow, platform packages, launcher script, Dockerfile, and cross-build script are all consistent with each other. Security posture (least-privilege permissions, OIDC auth, provenance attestation, publish guards) is sound. The 11 new tests provide good static validation of workflow structure and build script output.

[claude]

Code Review

I reviewed all 19 changed files in this PR, focusing on the CI workflow, build scripts, Dockerfile, JS launcher, package.json configurations, and the test suite.

Findings

No bugs found. The implementation is well-structured:

• Workflow security: Correct use of least-privilege permissions (contents: read at top level, id-token: write only on publish jobs), OIDC trusted publishing (no stored tokens), and provenance attestation on all npm publish calls.
• Build matrix: All 4 platform targets are covered with appropriate build strategies (zigbuild for cross-compilation, native for macOS arm64).
• Idempotent publishing: Each publish step is guarded by npm view to skip already-published versions, enabling safe reruns after partial failure.
• Binary permissions: chmod +x correctly applied to non-Windows binaries after artifact download (since actions/upload-artifact@v4 strips Unix permissions).
• Cross-build script: Properly handles root-owned files from Docker via a cleanup container, and skips the execution smoke test on non-x86_64 Linux hosts.
• Version consistency: Version sourced from Cargo.toml with tag validation, and tests enforce sync between Dockerfile.cross, rust-toolchain.toml, and the workflow.
• JS launcher: Clean platform detection with a thoughtful Windows dev fallback (MSVC triple) for local development.
• Test coverage: 11 new tests covering workflow structure, build script output, version sync, and security properties — all statically verifiable without a live CI run.

Overall Correctness Verdict

Correct. The patch is free of bugs and blocking issues. Existing code and tests will not break. The CI pipeline, build tooling, and test suite are consistent and well-tested.

[claude]

Code Review: mcp: npm release CI pipeline

I reviewed all changed files in this PR: the GitHub Actions workflow, cross-build tooling (Dockerfile, scripts), JS launcher updates, build script template, package.json changes, and all 11 new tests across 2 test files.

Findings

No blocking bugs found. The implementation is well-structured:

• Workflow security: OIDC trusted publishing with proper permission scoping (contents: read at workflow level, id-token: write only on publish jobs). No stored npm tokens.
• Build matrix: All 4 platform targets covered with correct build methods (zigbuild for Linux/Windows cross-compilation, native for macOS arm64).
• Idempotent publishes: Each npm publish is guarded by an npm view existence check, making reruns safe after partial failures.
• Version sync: Single source of truth in Cargo.toml, validated against tag at CI time, propagated to wrapper and platform packages via jq and build-npm-packages.sh.
• JS launcher: Clean platform resolution with a practical MSVC dev fallback for Windows developers, proper signal forwarding, and exit code mirroring.
• Tests: The 11 static validation tests against the workflow YAML and scripts are a pragmatic approach to catching regressions without live CI runs.
• Cross-build: Dockerfile and script handle host architecture detection correctly, skip execution smoke test on non-x86_64 Linux hosts, and use a Docker volume for incremental builds.

Minor observations (non-blocking)

• The Cargo.lock records serde_yaml (deprecated) as the resolved dependency for the serde_yml dev-dependency in Cargo.toml. This works but is worth being aware of -- serde_yaml carries a +deprecated suffix in its version.
• The implementation plan docs in docs/implementation-plans/ include earlier design iterations (e.g., phase 03 shows dtolnay/rust-toolchain@stable and a cache key without rust-toolchain.toml), while the actual implementation correctly uses @master with a pinned version and includes rust-toolchain.toml in the hash. These are just plan docs so this is fine.

Overall Correctness Verdict

Correct. The patch is free of bugs and blocking issues. Existing code and tests will not break. The workflow, build tooling, and tests are well-designed for a production npm release pipeline.

[bpowers]

Review Cycle Summary

This PR went through 10 iterations of automated review (Codex + Claude), resulting in 8 improvement commits addressing substantive feedback. The final iteration had no actionable findings from either reviewer.

Key improvements made during review:

Security & correctness:

• Tightened publish job guards to refs/tags/mcp-v (not just refs/tags/) preventing accidental publish via workflow_dispatch on arbitrary tags
• Added --locked to all release builds so a stale Cargo.lock causes a build failure rather than silently resolving different dependencies
• Replaced dtolnay/rust-toolchain@stable with pinned version from rust-toolchain.toml ensuring release binaries use the same compiler as the rest of the project
• Added npm view existence checks before every npm publish for safe re-runs after partial failures

Robustness:

• Fixed cross-build smoke test to check both OS (uname -s) and architecture (uname -m) -- an arm64 Linux host cannot run x86_64 binaries
• Fixed Docker cleanup glob expansion (sh -c 'rm -rf /dist/*' instead of literal /dist/*)
• Added root-owned artifact cleanup via Docker before rebuild
• Included rust-toolchain.toml in CI cache key to invalidate on toolchain bumps
• Cached .crates.toml/.crates2.json alongside ~/.cargo/bin for correct cargo install behavior on cache hits

Code quality:

• Replaced deprecated serde_yaml with serde_yml
• Pinned cargo-zigbuild version in Dockerfile.cross to match CI workflow
• Added Windows MSVC dev fallback in JS launcher for local development
• Strengthened tests: chmod assertions verify same-line containment, Zig version test uses structured YAML parsing, publish idempotency test counts occurrences

Test coverage grew from 11 to 20 tests across the two test files, covering workflow structure, version parity, publish idempotency, toolchain pinning, cache correctness, and cross-build script behavior.