Skip to content

fix: verify macOS npm fixtures on fork pull requests - #216

Merged
shakkernerd merged 3 commits into
openclaw:mainfrom
SebTardif:fix/npm-skip-empty-macos-team-id
Sep 15, 2026
Merged

shakkernerd merged 3 commits into
openclaw:mainfrom
SebTardif:fix/npm-skip-empty-macos-team-id

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Resolves a problem where fork pull requests fail the macOS npm installation fixture when the repository's MACOS_TEAM_ID setting is unavailable, even though the fixture uses an already signed and notarized release.

Why This Change Was Made

The v0.2.39 fixture records its verified public Apple Team ID alongside the release version and always checks the installed executable's signature and notarization. Publication through --packages still requires its separately configured MACOS_TEAM_ID and cannot fall back to the historical fixture's signer.

User Impact

Contributors can run the complete macOS npm fixture from forks. OCM commands and release publication behavior are unchanged.

Evidence

  • Intel macOS and Apple Silicon: all 21 npm tests passed on Python 3.13.15. Both real v0.2.39 fixtures passed with empty fork MACOS_TEAM_ID, including installed-byte/CLI checks, mandatory signature/notarization verification, and rejection of a different expected signer.
  • Linux CI passed the npm suite on Node 22.15 and Node 24; Node 24 also passed installation of the published native fixture.
  • Signer-policy coverage rejects missing or malformed publication settings before installation, keeps fixture identity independent of publisher settings, and rejects altered executable bytes after a real npm install. The same 21-test suite also passed on a separate Linux runner with Python 3.14.4 and npm 11.19.0.

@clawsweeper

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 11, 2026
@clawsweeper

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 14, 2026, 8:32 PM ET / September 15, 2026, 00:32 UTC (Revision 7).

ClawSweeper review

What this changes

The PR pins the historical macOS npm fixture’s signer, preserves signature and notarization checks, and adds signer-policy regression coverage and release documentation.

Merge readiness

Ready for maintainer review

This remains a useful, focused fix: current main and v0.2.46 still depend on unavailable fork configuration. The earlier publication-verification concern is resolved, and no blocking defect remains.

Priority: P2
Reviewed head: 265533fb56c32f143a0f0541bb228909a00021e2

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) A focused repair with cross-architecture native evidence, explicit publication separation, and useful negative controls.
Proof confidence 🦞 diamond lobster (5/6) ✨ media proof bonus Sufficient (linked_artifact): The captured evidence reports real Intel and Apple Silicon npm installations exercising the changed fixture, mandatory signature/notarization checks, and wrong-signer rejection. GitHub verifies successful fixture steps, and the exercised script is byte-identical to this head; raw-log inspection was proxy-blocked.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (linked_artifact): The captured evidence reports real Intel and Apple Silicon npm installations exercising the changed fixture, mandatory signature/notarization checks, and wrong-signer rejection. GitHub verifies successful fixture steps, and the exercised script is byte-identical to this head; raw-log inspection was proxy-blocked.
Evidence reviewed 8 items Current main still contains the failure: Main passes MACOS_TEAM_ID directly to the macOS verifier from the shared smoke path, including the published fixture. An empty value fails the verifier’s required ten-character format.
Latest release retains the same dependency: The supplied latest release, v0.2.46, contains the same environment-dependent fixture verification; this repair is not already shipped there.
Fixture and publication remain separate: The fixture explicitly supplies its pinned signer. The --packages entrypoint supplies no override and therefore requires valid publication configuration before installation. The existing verifier still checks Developer ID, team, identifier, hardened runtime, timestamp, and notarization. Publication continues to depend on successful native validation.
Findings None None.
Security None None.

How this fits together

OCM’s npm validation installs packaged native executables through an isolated registry and checks their bytes, CLI behavior, and macOS signatures. CI uses a historical release fixture, while publication validates packages against the publisher’s configured signer.

flowchart TD
  A[Historical release fixture] --> C[Select expected signer]
  B[Publication packages and team setting] --> C
  C --> D[Validate signer configuration]
  D --> E[Isolated npm installation]
  E --> F[Check bytes and CLI]
  F --> G[Verify signature and notarization]
  G --> H[Pass validation or reject]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Net code growth CLI production +0; test/validation script +136; documentation +15 Growth is confined to fixture signer selection, negative controls, regression coverage, and its documented boundary.

Technical review

Best possible solution:

Keep historical fixture identity explicit and independent of publisher configuration while retaining the same strict macOS verifier for both paths.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: on macOS, the current-main fixture passes an empty MACOS_TEAM_ID to a verifier that rejects it. Related merged PRs corroborate the failure; this review did not execute it.

Is this the best way to solve the issue?

Yes. Pinning the historical fixture’s expected signer repairs fork validation without adding a verification bypass or changing publication identity.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against a5e77df9d9eb.

Labels

Label changes:

  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P2: This fixes reproducible fork-contributor validation failures with a bounded change and no end-user runtime modification.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (linked_artifact): The captured evidence reports real Intel and Apple Silicon npm installations exercising the changed fixture, mandatory signature/notarization checks, and wrong-signer rejection. GitHub verifies successful fixture steps, and the exercised script is byte-identical to this head; raw-log inspection was proxy-blocked.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured evidence reports real Intel and Apple Silicon npm installations exercising the changed fixture, mandatory signature/notarization checks, and wrong-signer rejection. GitHub verifies successful fixture steps, and the exercised script is byte-identical to this head; raw-log inspection was proxy-blocked.

Evidence

What I checked:

  • Current main still contains the failure: Main passes MACOS_TEAM_ID directly to the macOS verifier from the shared smoke path, including the published fixture. An empty value fails the verifier’s required ten-character format. (scripts/tests/test_npm_install.py:367, a5e77df9d9eb)
  • Latest release retains the same dependency: The supplied latest release, v0.2.46, contains the same environment-dependent fixture verification; this repair is not already shipped there. (scripts/tests/test_npm_install.py:367, fc9f330476d3)
  • Fixture and publication remain separate: The fixture explicitly supplies its pinned signer. The --packages entrypoint supplies no override and therefore requires valid publication configuration before installation. The existing verifier still checks Developer ID, team, identifier, hardened runtime, timestamp, and notarization. Publication continues to depend on successful native validation. (scripts/tests/test_npm_install.py:424, 265533fb56c3)
  • Real macOS fixture evidence: The captured PR body reports actual v0.2.39 installations on Intel and Apple Silicon with empty fork configuration, unchanged executable bytes, mandatory signature/notarization verification, and wrong-signer rejection. GitHub confirms both named fixture steps succeeded in https://github.com/openclaw/ocm/actions/runs/34911200180 on head 41cbd73. Raw log downloads were blocked by the reviewer proxy; their contents were not independently read. (41cbd731fcc9)
  • Proof and re-review source continuity: GitHub contents metadata for both the exercised head and the previous reviewed head identifies fixture blob 50e28b6b2710404f16308dd26ac350d40192bde8, identical to the fully inspected current checkout file. The previous completed review retained no findings. Local comparison with the previous commit was unavailable; GitHub blob identity established continuity for this file. (scripts/tests/test_npm_install.py, 265533fb56c3)
  • Independent reports distinguish this repair: The merged work at fix: isolate runtime install lifecycle state #131, fix: cloned environments load plugins from their source #215, and fix: detect existing Claude logins in supervised gateways #222 explicitly identifies the macOS fixture failure as separate work tracked here; none supersedes this repair.

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • fuller-stack-dev: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (6 earlier review cycles)
  • reviewed 2026-09-11T17:43:29.954Z sha f15fbdf :: needs real behavior proof before merge. :: [P1] Keep npm publication verification strict
  • reviewed 2026-09-11T20:02:38.425Z sha 3d2b7cb :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-11T20:24:49.253Z sha 3d2b7cb :: needs maintainer review before merge. :: none
  • reviewed 2026-09-15T00:03:28.353Z sha 41cbd73 :: blocked before merge. :: none
  • reviewed 2026-09-15T00:12:18.847Z sha 41cbd73 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-15T00:19:41.656Z sha 009c726 :: needs maintainer review before merge. :: none

@SebTardif
SebTardif force-pushed the fix/npm-skip-empty-macos-team-id branch from f15fbdf to 3d2b7cb Compare September 11, 2026 19:58
@SebTardif SebTardif changed the title fix(ci): skip macOS notarization when MACOS_TEAM_ID is empty fix(ci): skip fork npm notarization without weakening publish Sep 11, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper

Keep npm publication verification strict

--packages now fails closed when MACOS_TEAM_ID is empty or malformed. Only --published-binary-fixture skips. Live --packages reject vs fixture skip vs valid-id verifier flags are in the PR body.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Sep 11, 2026
@shakkernerd shakkernerd self-assigned this Sep 14, 2026
@shakkernerd shakkernerd changed the title fix(ci): skip fork npm notarization without weakening publish fix: verify macOS npm fixtures on fork pull requests Sep 14, 2026
@shakkernerd
shakkernerd force-pushed the fix/npm-skip-empty-macos-team-id branch from 41cbd73 to 009c726 Compare September 15, 2026 00:14
SebTardif and others added 3 commits September 15, 2026 01:25
Fork PRs set MACOS_TEAM_ID to an empty string. The published-binary
fixture still passed that value to verify-macos-release.sh, so the npm
macos-15 jobs failed before proving install or byte preservation.

Keep the notarization check when a 10-character team id is present.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Restrict the empty MACOS_TEAM_ID skip to the fork published-binary
fixture. --packages still requires a 10-character team id on macOS
before install, matching publish-npm.yml.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@shakkernerd
shakkernerd force-pushed the fix/npm-skip-empty-macos-team-id branch from 009c726 to 265533f Compare September 15, 2026 00:28
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Sep 15, 2026
@shakkernerd
shakkernerd merged commit 066ae81 into openclaw:main Sep 15, 2026
10 checks passed
@shakkernerd

Copy link
Copy Markdown
Member

Fixed: fork pull requests now retain full macOS npm signature and notarization verification while publication requires its separate signer configuration.

With thanks to @SebTardif.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants