fix: verify macOS npm fixtures on fork pull requests - #216
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. Reviewed September 14, 2026, 8:32 PM ET / September 15, 2026, 00:32 UTC (Revision 7). ClawSweeper reviewWhat this changesThe 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 Review scores
Verification
How this fits togetherOCM’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]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest 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. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
f15fbdf to
3d2b7cb
Compare
|
41cbd73 to
009c726
Compare
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>
009c726 to
265533f
Compare
|
Fixed: fork pull requests now retain full macOS npm signature and notarization verification while publication requires its separate signer configuration. With thanks to @SebTardif. |
What Problem This Solves
Resolves a problem where fork pull requests fail the macOS npm installation fixture when the repository's
MACOS_TEAM_IDsetting 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
--packagesstill requires its separately configuredMACOS_TEAM_IDand 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
MACOS_TEAM_ID, including installed-byte/CLI checks, mandatory signature/notarization verification, and rejection of a different expected signer.