Take the signing identity from the keychain, and classify it correctly - #45
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change distinguishes ad-hoc signatures from self-signed certificates by using code-signature flags. It updates installation decisions, tests, documentation, and signing-identity mismatch guidance. ChangesSigning identity and installation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The documented installation recovery changes have no remaining actionable merge risk based on the supplied evidence. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
frobware
force-pushed
the
check-install-identity-classification
branch
from
September 19, 2026 18:55
dea826d to
c6494ea
Compare
check-install exists to stop you installing a bundle launchd will then refuse to start, and it decided that on the wrong signal: isAdHoc was the absence of a team identifier. Any certificate not issued through an Apple developer account has no team either, so a bundle signed with one was called ad-hoc and refused over a registered agent. The two are not alike. Ad-hoc has no certificate, so the requirement falls back to a code hash that changes on every build, which is exactly why it cannot satisfy a recorded launch requirement. A certificate produces a requirement naming its leaf hash, which does not move. The code directory carries an adhoc flag saying which is which, so read that instead of guessing from the team. The advice printed with the refusal was wrong in the same place, and so was INSTALL.md. make uninstall does not clear the launch requirement macOS recorded against the label: uninstall first, install a differently signed bundle, and launchd still rejects the job with EX_CONFIG, and the record then rejects the identity you started with too. An unregister followed by a register does clear it, in seconds, with no admin rights. Uninstalling still matters, because with no installed bundle there is nothing for this check to compare against, but it is not the step that clears anything. Both now say the whole sequence and which part cannot be skipped. INSTALL.md also stops calling the brackets in a certificate name a team id. The team id is the OU, and is what codesign reports as TeamIdentifier.
frobware
force-pushed
the
check-install-identity-classification
branch
from
September 19, 2026 19:00
c6494ea to
fac5f92
Compare
frobware
changed the title
local.mk existed to hold a string the keychain already knows. With one codesigning certificate there is now nothing to set up: ask for it, at about fifteen milliseconds, and sign with it. None still means ad-hoc, so a fresh checkout builds. Several means refusing to choose, because picking the wrong certificate signs against a requirement macOS did not record and the grant goes; the refusal prints each candidate as the line to paste. local.mk still wins where it exists, and is still what you want for a certificate the keychain does not report as valid -- a self-signed one is not listed, so it has to be named.
The install notes had you read an identity out of find-identity and retype it into local.mk. The build asks the keychain itself now, so with one certificate there is nothing to write, and the file is for the two cases that need it: more than one certificate, or one the keychain does not report as valid. Also corrects the Xcode path, which is Settings > Apple Accounts, and says the certificate is per machine so the other Macs showing "Not in Keychain" is expected rather than something to fix.
The final form of the file, with how to get the value next to the line it goes on: the Xcode path for a certificate, find-identity for the exact string, and why you probably need neither. Everything in it is commented out, so copying it unedited leaves the build deriving the identity as it would have anyway, rather than trying to sign with a placeholder.
frobware
changed the title
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
1 participant
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.
Two related things about the signing identity: finding it, and judging it. Four commits, each self-contained.
Judging it
check-installexists to stop you installing a bundle that launchd will then refuse to start. It decided that on the wrong signal:Ad-hoc signatures have no team identifier, but neither does any certificate that was not issued through an Apple developer account. So a bundle signed with one was called ad-hoc and refused over a registered agent, having just been built and signed successfully.
Ad-hoc has no certificate, so the designated requirement falls back to a cdhash that changes on every build -- which is exactly why it cannot satisfy a recorded launch requirement. A certificate produces a requirement naming its leaf hash, which does not move. The code directory carries an
adhocflag saying which is which, so read that rather than guessing from the team.Finding it
local.mkexisted to hold a string the keychain already knows. The build now asks for it, at about fifteen milliseconds, so with one codesigning certificate there is nothing to configure at all.local.mkstill wins where it exists, and is still needed for a certificate the keychain does not report as valid -- a self-signed one is not listed, so it has to be named.INSTALL.mdfollows the same shape, andlocal.mk.exampleputs the instructions next to the line they describe. Everything in the example is commented out, so copying it unedited leaves the build deriving the identity as it would have anyway.The advice was wrong in the same place
Both the message printed with the refusal and
INSTALL.mdsaidmake uninstallclears the launch requirement macOS records against the agent's label, and that unregistering does not. Both halves are backwards, and both were measured:EX_CONFIG(78). The record is then rewritten so the identity you started with is rejected too.make unregisterfollowed bymake registerclears it, in seconds, without admin rights orsfltool.Uninstalling does still matter, but not for the reason given: with no installed bundle there is nothing for
check-installto compare against, so it is what gets you past the refusal. Both texts now give the whole sequence and say which step cannot be skipped.INSTALL.mdalso stops calling the brackets in a certificate name a team id -- the team id is theOU, which is whatcodesignreports asTeamIdentifier-- corrects the Xcode path, which is Settings > Apple Accounts, and says a certificate is per machine so other Macs showing "Not in Keychain" is expected rather than broken.Test plan
make cigreen: 110 tests, 0 failures, plist lint, every target parses. Four tests added.testAdHocBundleReportsItsIdentifierAndNoTeambuilds a genuinely ad-hoc bundle withcodesignand still classifies it correctly, so the flags path is exercised against a real signature.local.mkpresent. It produces the identical string on two machines.EX_CONFIGclaims come from doing it, on two machines, in both directions.