Resolve cask system languages before artifact installation - #23612
Conversation
`Cask::Config`'s `languages` default is a `LazyObject` that is only forced while serialising the config at the end of artifact installation. That late resolution forks the whole process (`Utils.popen` running `defaults read`) after the installer has already used fork-hostile frameworks such as Security.framework for signing checks. Memoise `MacOS.languages` at the start of `Cask::Installer#install_artifacts`, which both installs and upgrades go through, so the fork happens before any artifact or signing work and the later lazy resolution becomes a memo lookup. Follow-up to Homebrew#23607, requested in Homebrew#23606 (comment).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e13641522
ℹ️ 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".
Cask upgrades call `Quarantine.signing_identity` for existing apps before `Cask::Installer#install_artifacts` runs, so on the first upgrade the languages fork could still happen after Security.framework work. Prime `MacOS.languages` before the first Security call in `signing_identity` as well; memoisation makes repeat calls free. Addresses Codex review feedback on the pull request.
There was a problem hiding this comment.
Pull request overview
Primes MacOS.languages at the start of Cask::Installer#install_artifacts on macOS so the defaults read -g AppleLanguages fork happens before any artifact/signing work, avoiding late-process forks during config serialization.
Changes:
- Add a macOS-specific
Cask::Installer#install_artifactsprepend that eagerly resolvesMacOS.languages. - Load the new macOS extension from the OS cask installer extension loader.
- Add a spec asserting languages resolution happens before artifact
install_phase.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Library/Homebrew/test/cask/installer_spec.rb | Adds an ordering spec for resolving system languages before artifact installation. |
| Library/Homebrew/extend/os/mac/cask/installer.rb | Prepends Cask::Installer#install_artifacts on macOS to call MacOS.languages before artifact work. |
| Library/Homebrew/extend/os/cask/installer.rb | Requires the new macOS installer extension when running on macOS. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Assert the ordering invariant (languages first, artifact/Security call present) instead of an exact call sequence, and return a representative value from the stubbed `MacOS.languages`. - Use American English spelling in the new installer comment.
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks for PR! Game for this fix just not sure it's quite right yet.
Move the eager `MacOS.languages` resolution to a single early, shared site per review feedback: `Cask::Installer#prelude` is run-once, is called first by `#install` and `#fetch`, and is called explicitly by `Cask::Upgrade` before the signing-identity scan. It is not run by uninstalls or when no casks are being processed. Remove the `install_artifacts` and `Quarantine.signing_identity` hooks this replaces.
@dot-agi Please disable this on your Homebrew PRs 🙇🏻 |
|
Done — disabled. The Codex GitHub app no longer has access to my |
brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?AI disclosure: this PR was produced with Claude Code (model: Claude Fable 5), in the same session as #23606 and #23607. It is the follow-up requested by @MikeMcQuaid in #23606 (comment). Verified locally as described below.
Follow-up to #23607 (analysis in #23606). Preventive hardening, not a standalone bug fix.
What this does
Cask::Config'slanguagesdefault is aLazyObject. Nothing forces it untilsave_config_fileserializes the config at the very end of artifact installation — the JSON generator'srespond_to?(:to_json)probe forces it as a side effect.brewprocess (Utils.popenrunningdefaults read -g AppleLanguages) after the installer has already run in-process Security.framework signing checks. That is the exact fork that crashed in Cask upgrade segfault: Ruby GC releases Security.framework objects on the child side of fork inUtils.popen#23606.MacOS.languagesinCask::Installer#prelude, via a newextend/os/mac/cask/installer.rb.preludeis the run-once early setup shared by cask operations:#installand#fetchcall it first, andCask::Upgradecalls it explicitly before scanning the existing apps' signing identities. So the fork happens before any download, artifact or signing work, and the later lazy resolution becomes a memo lookup.preludeis not run by uninstalls, and never when no casks are being processed, so nothing changes for formula-only or cask-less commands.Why
CFReleaserunning in fork children). This removes the remaining late-install fork entirely: config serialization no longer spawns processes as a side effect, and the fork happens at an explicit, early point while the process is still clean.Utils.popen#23606 incident, the crashed fork children returned empty output and"languages":[]was written to the cask'sconfig.json.Reproduction
Verification
brew style,brew typecheckand the changed spec suites (cask/installer,cask/quarantine) all pass locally.MacOS.languagesresolves first in#prelude, and before any artifactinstall_phasein a full#installrun.MacOS.languagescall commented out, the prelude spec fails withexpected: :languages / got: :prelude_requirements; restored, everything passes.