fix/macos-packaged-readonly-startup#2122
Open
lost-particles wants to merge 2 commits into
Open
Conversation
Fixes: PostHog#2098 Detect App Translocation bundle paths and non-root read-only volumes using mount(8). Show a blocking dialog before the window or services start. Ignores read-only on sealed macOS to avoid false positives.
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/code/src/main/utils/macos-packaged-install-guard.ts:38
Root mount (`/`) is never selected as `best` because `"${mp}/"` expands to `"//"` and no normal absolute path starts with `"//"`. As a result, the `best.mountPoint === "/"` branch in the caller is dead code — `best` is always `null` for paths that only fall under root. The guard still returns `false` for those paths (via the `!best` branch), so behaviour is correct today, but the explicit root exclusion is never exercised, and any future reader will be misled about what paths actually reach that check.
```suggestion
const under =
resolvedPath === mp ||
resolvedPath.startsWith(mp === "/" ? "/" : `${mp}/`);
```
### Issue 2 of 3
apps/code/src/main/utils/macos-packaged-install-guard.ts:72-75
`execFileSync` is called without a `timeout`. On a machine with a hung NFS or SMB share, `/sbin/mount` can stall indefinitely, freezing app startup — exactly the symptom this PR is trying to cure. Adding a short timeout (e.g. 3 s) ensures the guard degrades gracefully rather than blocking.
```suggestion
output = execFileSync("/sbin/mount", {
encoding: "utf8",
maxBuffer: 10 * 1024 * 1024,
timeout: 3000,
});
```
### Issue 3 of 3
apps/code/src/main/utils/macos-packaged-install-guard.test.ts:131-138
**Non-hermetic test for the read-only mount path**
`isMacosPackagedUnsafeBundleLocation` calls the real `/sbin/mount` here. The test passes on CI because `/Volumes/build/out` is not actually mounted anywhere, not because the writable-mount code path was exercised correctly. A machine where that path happened to be mounted read-only would produce a flaky failure. Neither test case in this describe block covers the `isMacosPathOnReadOnlyNonRootMount` branch at all — both exercise only the translocation shortcut or the silent error-catch fallback. Consider adding a test that injects a mock mount table (using `isMacosPathOnReadOnlyNonRootMountFromTable` directly, matching the pattern used in the `isMacosPathOnReadOnlyNonRootMountFromTable` describe block) to prove the read-only detection path works inside `isMacosPackagedUnsafeBundleLocation`.
Reviews (1): Last reviewed commit: "fix/macos-packaged-readonly-startup" | Re-trigger Greptile |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/code/src/main/utils/macos-packaged-install-guard.ts:25
Mount points whose display names contain ` (` — e.g. `/Volumes/My Backup (2)` — are parsed incorrectly. `indexOf(" (")` stops at the first occurrence inside the name, so `mountPoint` is truncated and the options field picks up the tail of the display name. Using `lastIndexOf` anchors to the options block that macOS always places at the end of the line, matching the `line.endsWith(")")` invariant already enforced above.
```suggestion
const openParen = afterOn.lastIndexOf(" (");
```
### Issue 2 of 2
apps/code/src/main/utils/macos-packaged-install-guard.test.ts:9-45
The `isMacosAppTranslocationPath` suite has four independent cases with identical structure (two string inputs, one boolean output) but is written as separate `it()` calls. Per the team's style guidance, prefer parameterised tests here — `it.each` would let the signal/noise ratio improve and make adding edge-cases (e.g. case-insensitive segment, partial segment substring) a one-liner. The same applies to `isMacosPathOnReadOnlyNonRootMountFromTable`, where the flat cases on a shared `table` fixture are natural `it.each` rows.
Reviews (2): Last reviewed commit: "minor fixes" | Re-trigger Greptile |
| const onMarker = line.indexOf(" on "); | ||
| if (onMarker === -1) continue; | ||
| const afterOn = line.slice(onMarker + 4); | ||
| const openParen = afterOn.indexOf(" ("); |
Contributor
There was a problem hiding this comment.
Mount points whose display names contain
( — e.g. /Volumes/My Backup (2) — are parsed incorrectly. indexOf(" (") stops at the first occurrence inside the name, so mountPoint is truncated and the options field picks up the tail of the display name. Using lastIndexOf anchors to the options block that macOS always places at the end of the line, matching the line.endsWith(")") invariant already enforced above.
Suggested change
| const openParen = afterOn.indexOf(" ("); | |
| const openParen = afterOn.lastIndexOf(" ("); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/utils/macos-packaged-install-guard.ts
Line: 25
Comment:
Mount points whose display names contain ` (` — e.g. `/Volumes/My Backup (2)` — are parsed incorrectly. `indexOf(" (")` stops at the first occurrence inside the name, so `mountPoint` is truncated and the options field picks up the tail of the display name. Using `lastIndexOf` anchors to the options block that macOS always places at the end of the line, matching the `line.endsWith(")")` invariant already enforced above.
```suggestion
const openParen = afterOn.lastIndexOf(" (");
```
How can I resolve this? If you propose a fix, please make it concise.
Author
|
@jonathanlab Please review these changes |
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.
Fixes: #2098
Problem
Packaged PostHog Code users on macOS can end up launching from App Translocation or a read-only volume (e.g. DMG). The app may start partially, then updates, local tasks, or cloud flows misbehave with little in-app explanation. This hurts anyone installing from Downloads, disk images, or other flows that Gatekeeper exposes as a restricted runtime path—not only power users reading
main.log.Closes #2098
Changes
app.whenReady(), before logs, window creation, orinitializeServices()): packaged macOS only. If we detect an unsafe bundle location, show a blockingdialog.showMessageBoxSyncexplaining read-only/translocation-style behavior, thenapp.quit().macos-packaged-install-guard.ts: consolidated helpers—AppTranslocationinapp.getAppPath()orprocess.execPath./sbin/mountoutput: longest prefix match for resolvedexePath; unsafe if mount options containread-onlyand mount point is not/(avoids sealed-root false positives).macos-packaged-install-guard.test.ts: table-driven tests for mount parsing and combinedisMacosPackagedUnsafeBundleLocation.No renderer UI screenshots (native Electron dialog only).
How did you test this?
pnpm --filter code exec vitest run src/main/utils/macos-packaged-install-guard.test.ts(all tests passing when run).pnpm --filter code package; openPostHog Code.appfrom a normal writable path (should behave as before); open from a read-only-mounted DMG/volume or a translocated path—expect the blocking dialog and exit after OK. Devpnpm devshould be unchanged (app.isPackaged).