Repository navigation
ci: import the signing certificate as a named .p12 - #21043
Merged
Merged
Conversation
The first nightly after #20996 failed at Import signing cert with "SecKeychainItemImport: Unknown format in import": the decoded certificate file had no .p12 extension, and security import infers the format from it. This case now requires a .p12 file and -f pkcs12; the next commit fixes it. Refs cmuxterm-hq#2053. Bead cx-yzoow.
signing-keychain.sh create decoded the certificate into a file without an
extension, and security import refused it ("Unknown format in import"),
which failed Import signing cert in every nightly variant after #20996. The
file is now certificate.p12 in a private directory, the import passes
-f pkcs12, and the decode keeps the trailing newline of the old inline step.
Refs cmuxterm-hq#2053. Bead cx-yzoow.
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description check The description clearly explains the nightly signing failure, the fix, the tests run, the unverified real import, and the internal-only changelog entry. The Proof and Checklist sections from the templ… Title check The title is concise, specific, and accurately describes the primary change: importing the signing certificate as a named .p12 file. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Cmux Cloud Persistent Session And Early Input PASS: The pull request changes only CI signing-certificate import cleanup and related tests. It does not change Cloud terminal creation, cmux-tui transport, manual renderer gating, input ownership, au… Cmux Swift Actor Isolation PASS: The pull request changes only scripts/ci/signing-keychain.sh and tests/test_notarize_nightly_dmg.sh. The authoritative diff contains no Swift production changes, so it introduces no Swift 6 acto… Cmux Swift Blocking Runtime PASS: The pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. It introduces no production Swift changes and no synchronization constructs covered by th…Cmux Browser Automation Off-Main The PR changes only scripts/ci/signing-keychain.sh and tests/test_notarize_nightly_dmg.sh. The diff contains no browser socket commands, WebKit/AppKit routing, socketWorkerMethods, mainActor, or proce… Cmux Expensive Synchronous Load The pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. The reviewed diff contains no Swift files and no agent-history loading or interactive Swift cal…Cmux Cache Substitution Correctness PASS: The pull request changes only Bash CI logic and a Bash test. It does not contain production Swift, TypeScript, or JavaScript changes. The diff updates certificate-file naming, security import…Cmux No Hacky Sleeps The pull request adds no fixed sleep, timer, polling loop, backoff, or wall-clock wait. The production shell change only changes certificate file naming, format selection, and cleanup. The existing 60… Cmux Algorithmic Complexity PASS: The pull request changes a CI shell script and its test. It adds temporary-directory creation, file decoding, explicit security import -f pkcs12, and recursive cleanup. It does not add nested …Cmux Swift Concurrency The pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. The diff introduces no cmux-owned Swift code and no Swift concurrency patterns.Cmux Swift @ConcurrentThe pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. The authoritative diff contains no Swift files or Swift async functions, so it cannot violate t…Cmux Swift Package Boundaries PASS: The pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. It introduces no production Swift changes, so the Swift package boundary rule does not ap…Cmux Swiftpm Lockfiles PASS: The pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. It does not change a SwiftPM package, Xcode project package references,.gitignore, wor…Cmux Swift Logging PASS: The pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. Neither file contains Swift logging. The diff adds shell command output and test assertio…Cmux User-Facing Error Privacy PASS: The diff changes an internal CI signing helper and its test. The helper runs from GitHub Actions signing jobs and uses security import; the changed output is CI/operator diagnostics, not cmux …Cmux Full Internationalization The PR changes only CI signing-keychain logic and its shell test. It adds no Swift UI text, app catalog entries, web UI/API copy, metadata, markdown, or changelog content. The added comments and test … Cmux Swiftui State Layout PASS: The pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. The diff contains shell and test-script changes, with no SwiftUI,ObservableObject, `@P…Cmux Architecture Rethink PASS: The pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. It introduces no Swift architecture change, lifecycle ownership change, timing repair, ob…Cmux Swift Auxiliary Window Close Shortcuts PASS: The pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. The authoritative diff contains no Swift changes and no auxiliary window code. The cmux a…Cmux Source Artifacts The diff changes only the hand-written CI script scripts/ci/signing-keychain.shand the testtests/test_notarize_nightly_dmg.sh. It adds runtime temporary-directory cleanup and.p12handling, bu…Cmux No Test Or Debug Seam In Production Source PASS. The pull request changes only scripts/ci/signing-keychain.shandtests/test_notarize_nightly_dmg.sh. The authoritative diff contains no Swift files under a productionSources/path, so it …
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Contributor
Author
|
Correctness review (subagent): approve, no blocking finding.
|
lawrencecchen
force-pushed
the
ci-signing-keychain-import-format
branch
from
October 16, 2026 05:17
74e9809 to
1f8dfb8
Compare
Contributor
|
Probably yours: CI failed on
Matched log linesNot re-run automatically: Written by |
Contributor
|
Agents: |
Contributor
|
Merge receipt for
Labeled |
github-actions
Bot
added
the
merged-unverified
lawrencecchen
added a commit
that referenced
this pull request
Oct 16, 2026
* ci: real macOS import test for the signing keychain tool tests/test_signing_keychain_real_import.sh runs the real signing-keychain.sh create, a codesign with the imported identity, and delete on macOS with a throwaway self-signed identity and a random password (no secret), in the new signing-keychain-macos.yml workflow on a fresh runner. The fake-security case alone did not catch the import format failure fixed in #21043. Refs cmuxterm-hq#2053. Bead cx-yzoow. * ci: real import test runs only in CI or by opt-in; push trigger only on main Review findings on #21142. Refs cmuxterm-hq#2053. Bead cx-yzoow. * ci: select the pinned Xcode in the real import job; push on both trunks The macOS-jobs guard requires scripts/select-ci-xcode.sh in every macOS job. Refs cmuxterm-hq#2053. Bead cx-yzoow.
This was referenced Oct 16, 2026
lawrencecchen
added a commit
that referenced
this pull request
Oct 16, 2026
The certificate secret's .p12 carries Apple's Developer ID G2 intermediate, so security import adds it and import-apple-developer-id-intermediates.sh then failed on "already in build.keychain" in every signing job (first main nightly with #21043, run 38857402565). The helper now accepts a certificate that is already in the keychain; its count check still requires both intermediates. Refs cmuxterm-hq#2053. Bead cx-yzoow.
lawrencecchen
added a commit
that referenced
this pull request
Oct 16, 2026
…added (#21143) * ci: accept a Developer ID intermediate that the signing .p12 already added Since a2380e7 / #21043 the .p12 import works (named certificate.p12, -f pkcs12), and it adds the Developer ID G2 intermediate that the certificate carries. Then import-apple-developer-id-intermediates.sh ran security add-certificates for the same certificate, which exits 1 with "already in build.keychain", and every Import signing cert step failed (main nightly run 38857402565 on d16b03c, all 3 arches). add_intermediate now treats only that "already in" result as success; any other add-certificates error still fails. The existing count check still requires both Developer ID intermediates in the keychain. * ci: accept an already-present intermediate only when its SHA-1 matches Review follow-up: "already in" alone is error text. The keychain must hold the exact vendored certificate (SHA-1 fingerprint from openssl, compared with security find-certificate -Z), else the import still fails. The hash list is captured first, so grep -q cannot end security early under pipefail. * ci: require a 40-hex SHA-1 before the fingerprint match, and match it as a fixed string Security review P3 (hq-84).
lawrencecchen
added a commit
that referenced
this pull request
Oct 16, 2026
…added (#21145) * ci: accept a Developer ID intermediate that the signing .p12 already added Since a2380e7 / #21043 the .p12 import works (named certificate.p12, -f pkcs12), and it adds the Developer ID G2 intermediate that the certificate carries. Then import-apple-developer-id-intermediates.sh ran security add-certificates for the same certificate, which exits 1 with "already in build.keychain", and every Import signing cert step failed (main nightly run 38857402565 on d16b03c, all 3 arches). add_intermediate now treats only that "already in" result as success; any other add-certificates error still fails. The existing count check still requires both Developer ID intermediates in the keychain. * ci: accept an already-present intermediate only when its SHA-1 matches Review follow-up: "already in" alone is error text. The keychain must hold the exact vendored certificate (SHA-1 fingerprint from openssl, compared with security find-certificate -Z), else the import still fails. The hash list is captured first, so grep -q cannot end security early under pipefail. * ci: require a 40-hex SHA-1 before the fingerprint match, and match it as a fixed string Security review P3 (hq-84).
This was referenced Oct 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix forward for #20996. The first nightly on main after it (https://github.com/manaflow-ai/cmux/actions/runs/38723131700) failed in
Import signing certfor all three variants:scripts/ci/signing-keychain.sh createdecoded the certificate intocertificate.XXXXXX(no extension).security importinfers the format from the file name; the old inline step used/tmp/cert.p12. The file is nowcertificate.p12inside a privatemktemp -ddirectory, the import passes-f pkcs12, and the decode usesprintf '%s\n'like the oldecho. The directory is removed right after the import and again bydelete.The run published nothing (the failure is before any signature).
Testing
tests/test_notarize_nightly_dmg.shnow requires a.p12file and-f pkcs12in thesecurity importcall. First commit: fails. Second commit: passes (Linux VM, fakesecurity).tests/test_ci_self_hosted_guard.shkeychain checks pass.Changelog
none
Summary by cubic
Fixes the nightly CI failure in
Import signing certwheresecurity importcouldn't infer the format of the decoded certificate because it lacked a.p12extension.certificate.p12in a privatemktemp -ddirectory, and the import passes-f pkcs12..p12file name and the explicit format flag.Written for commit 1f8dfb8. Summary will update on new commits.
Summary by CodeRabbit