fix(darwin): resolve executable via CFBundleExecutable with Electron fallback - #350
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes macOS launch failures for downloaded VS Code bundles by resolving the actual app executable name from Info.plist (CFBundleExecutable), with a legacy Electron fallback for older bundles.
Changes:
- Update darwin executable path resolution to use a new
resolveDarwinAppExecutablehelper. - Add unit tests covering darwin path resolution cases (modern
CFBundleExecutableand legacy fallback).
Show a summary per file
| File | Description |
|---|---|
| lib/util.ts | Reads CFBundleExecutable from Info.plist to find the correct macOS bundle executable, falling back to Electron. |
| lib/util.test.mts | Adds vitest coverage for darwin executable path resolution and confirms non-darwin platforms are unchanged. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
| const plist = readFileSync(infoPlistPath, 'utf-8'); | ||
| const match = plist.match(/<key>CFBundleExecutable<\/key>\s*<string>([^<]+)<\/string>/); | ||
| if (match) { | ||
| return path.resolve(macosDir, match[1]); | ||
| } |
There was a problem hiding this comment.
Addressed in commit 2d11b92. Tier 1 now validates the extracted CFBundleExecutable before use: the resolved candidate must live directly under macosDir (candidate.startsWith(macosDir + path.sep)) and the file must exist on disk, otherwise the helper falls through to the next tier. A crafted plist with a ../ payload — or any absolute path — is rejected. Added a unit test (rejects CFBundleExecutable containing path traversal) that writes ../../../etc/passwd into the plist and verifies the helper returns the in-bundle binary instead.
| const plist = readFileSync(infoPlistPath, 'utf-8'); | ||
| const match = plist.match(/<key>CFBundleExecutable<\/key>\s*<string>([^<]+)<\/string>/); | ||
| if (match) { | ||
| return path.resolve(macosDir, match[1]); | ||
| } | ||
| } catch { | ||
| // The plist may not exist yet (unusual, but not fatal) or may be in the | ||
| // binary plist format (never observed for VS Code bundles in practice). | ||
| // Fall through to the legacy `Electron` name. | ||
| } | ||
| return path.resolve(macosDir, 'Electron'); |
There was a problem hiding this comment.
Addressed in commit 2d11b92. Added a tier 2 between the plist read and the legacy Electron fallback: the helper now scans Contents/MacOS/ and, when exactly one regular (non-symlink) file lives there, returns it. VS Code bundles ship exactly one main binary in that directory across every historical variant (Electron pre-1.110, Code/Code - Insiders after), plus optionally a compatibility symlink to it — the isFile() filter collapses that unambiguously to the real binary. This closes the silent-regression path you flagged: if the plist is missing, unreadable, in the binary format, or otherwise doesn't match the XML shape, tier 2 still returns the correct executable on any modern bundle. Tier 3 (Electron) is preserved as an ultimate last resort for pre-1.110 packaging quirks. New tests cover:
- Plist absent + sole
Codefile → returnsCode(tier 2 on post-rename bundle). - Plist absent +
Codefile +Electronsymlink → returnsCode(symlink correctly filtered out). - Plist present but transitional symlink layout — tier 1 still wins over the symlink.
- Empty plist + empty
MacOS/→ tier 3 (Electron) still fires.
6fc4fe6 to
2d11b92
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
lib/util.ts:214
- Tier 1 claims the CFBundleExecutable value must resolve directly under
Contents/MacOS/, but the current validation allows nested paths likesubdir/Code(it only checksstartsWith). Tighten validation to accept only a plain filename (no/or\separators) before resolving, otherwise fall through to tier 2/3.
const match = plist.match(/<key>CFBundleExecutable<\/key>\s*<string>([^<]+)<\/string>/);
if (match) {
const candidate = path.resolve(macosDir, match[1]);
if (candidate.startsWith(macosDir + path.sep) && existsSync(candidate)) {
return candidate;
}
| expect(exePath).to.equal(join(macosDirFor('Visual Studio Code - Insiders.app'), 'Code - Insiders')); | ||
| }); | ||
|
|
||
| test('Stable: transitional symlink layout — plist name wins over Electron symlink', async () => { |
There was a problem hiding this comment.
Addressed in commit ad779a2. Both symlink tests now test.skipIf(process.platform === 'win32'). The behavior under test is darwin-specific (the platform argument to downloadDirToExecutablePath is hardcoded to darwin-arm64), so skipping on win32 loses no coverage — the same code paths are exercised on macOS and Linux runners, which don't have the EPERM/Developer-Mode requirement for fs.symlink. Added a comment above the first test explaining the rationale and referenced from the second.
| expect(exePath).to.equal(join(macosDirFor('Visual Studio Code.app'), 'Code')); | ||
| }); | ||
|
|
||
| test('Stable: tier 2 ignores the compatibility symlink and picks the real binary', async () => { |
There was a problem hiding this comment.
Addressed in commit ad779a2. Both symlink tests now test.skipIf(process.platform === 'win32'). The behavior under test is darwin-specific (the platform argument to downloadDirToExecutablePath is hardcoded to darwin-arm64), so skipping on win32 loses no coverage — the same code paths are exercised on macOS and Linux runners, which don't have the EPERM/Developer-Mode requirement for fs.symlink. Added a comment above the first test explaining the rationale and referenced from the second.
…fallback
VS Code 1.110 renamed the macOS main binary from Contents/MacOS/Electron
to the product name ('Code' on Stable, 'Code - Insiders' on Insiders) in
microsoft/vscode#291948 (merged 2026-02-03). A compatibility symlink kept
the old name working until it was removed in microsoft/vscode#326502
(merged 2026-07-20), at which point downloadDirToExecutablePath and
insidersDownloadDirToExecutablePath started returning a path that no
longer exists on 1.110+ downloads. Launching the returned path fails
with 'spawn .../Contents/MacOS/Electron ENOENT'; the CLI probe kept
working because resolveCliPathFromVSCodeExecutablePath derives its path
by relative traversal, not by the binary name.
Both functions now delegate darwin resolution to a shared helper that:
1. reads CFBundleExecutable from the bundle's Info.plist (the
authoritative source on macOS); a targeted regex over the XML plist
avoids pulling in a plist parser or shelling out to PlistBuddy;
2. falls back to the legacy 'Electron' name if the plist is missing or
doesn't declare CFBundleExecutable, which preserves behaviour for
pre-1.110 downloads.
Fixes microsoft#348.
Fixes microsoft#349.
2d11b92 to
ad779a2
Compare
v3.1.0 ships the upstream fix for microsoft/vscode-test#348 / #349 (PR microsoft/vscode-test#350), which resolves the Electron binary via CFBundleExecutable in Info.plist instead of the hardcoded Contents/MacOS/Electron path that VS Code 1.110+ no longer ships on darwin. Restores macos-latest to the Insiders CI matrix, which had been temporarily removed as a workaround.
… CI (#186) v3.1.0 ships the upstream fix for microsoft/vscode-test#348 / #349 (PR microsoft/vscode-test#350), which resolves the Electron binary via CFBundleExecutable in Info.plist instead of the hardcoded Contents/MacOS/Electron path that VS Code 1.110+ no longer ships on darwin. Restores macos-latest to the Insiders CI matrix, which had been temporarily removed as a workaround.
VS Code 1.131.0 (2026-07-29) is the first stable release without the compatibility symlink for the old "Electron" macOS executable name (microsoft/vscode#326502, the symlink from the Feb 2026 rename in microsoft/vscode#291948 was removed 2026-07-20). @vscode/test-electron 2.5.2 still launches the old hardcoded path, so every integration-test run against 1.131.0+ fails with "spawn .../MacOS/Electron ENOENT" before any test runs (microsoft/vscode-test#348, #349). 3.1.0 fixes this by resolving the executable via the bundle's CFBundleExecutable, falling back to "Electron" for older downloads (microsoft/vscode-test#350). Verified locally: 25/25 integration tests pass against a fresh VS Code 1.131.0 download.
* Create amendment spec and tasks * Generate the previewed root's LayoutParams at inflation EngineAdapter.inflateOrNull now inflates against a throwaway FrameLayout parent with attachToRoot=false instead of a null parent, so the root element's own layout_width/layout_height/margins/gravity generate real LayoutParams (Studio's content-frame equivalent) instead of being silently discarded and defaulted to MATCH_PARENT x MATCH_PARENT. Adds engineTest pixel-probe coverage (RootLayoutParamsTest) over four new framework-gallery fixtures for wrap_content sizing, root margins, the match_parent regression guard, and layout_gravity positioning (LAY-08 AC1-AC3). * Correct LAY-08 AC4's uncovered-canvas assumption T89's engine-level verification found the original "stays transparent" assumption factually wrong: decor=false only maps to SessionParams.setForceNoDecor(), which suppresses system chrome, not window-background painting. Bridge paints the resolved theme's windowBackground/colorBackground onto the content root regardless (confirmed: a dark framework theme painted background_material_dark exactly, a light one background_material_light exactly). Revises AC4, AC7, and the uncovered-canvas assumption row to the corrected outcome (theme background shows through, matching what the spec's own root-cause section already documents Android Studio doing) per user decision superseding the original spec-approval assumption. * Pin the degenerate and pass-through inflation shapes Extends RootLayoutParamsTest with engineTests over five new framework-gallery fixtures: a missing root height and a missing root width (both must render ok and paint nothing, not crash — the pinned engine's native 0px axis handling), a <merge> root (must stay full-bleed, unaffected by the LAY-08 fix), a data-binding <layout> root (the unwrapped inner root's own params must be honored), and a tools:layout_height override (must govern over the real wrap_content value). The tools:-override edge case needed one production change: ToolsAttributes.CORE_ATTRS did not include layout_height, so tools:layout_height was previously dropped rather than promoted — added it so the already-approved LAY-08 edge case text is actually true. * Prove the reported ConstraintLayout shapes end-to-end Adds RootParamsConstraintTest, rendering the two shapes from the original user report through the full routing path with the bundled androidx/ConstraintLayout closure (2.2.1): - shape (a): a match_parent x wrap_content card with margins and padding wraps to its content, with the margins showing as insets. - shape (b): a child constrained top_toBottomOf a sibling AND bottom_toBottomOf="parent" sits directly below the sibling inside the wrapped bounds, instead of floating at the device's vertical center (the exact pre-fix defect signature from the report). * Regenerate and review the corpus goldens for LAY-08 40 of 42 configs are byte-identical after the root-params fix, verified against every corpus fixture's XML: every layout-kind root is match_parent x match_parent (root-param honoring is a no-op for match_parent), and the 6 drawable-kind configs never go through the affected inflation path. Only material/gallery (default, night) changed (0.031% diff): a nested MaterialToolbar's android:title="Title" text, previously invisible, now renders. Isolated via a throwaway worktree at the pre-fix commit with only EngineAdapter's inflateOrNull change reapplied — confirmed deterministic and caused exactly by that change, not by root geometry (this fixture's root was already match_parent x match_parent, unchanged in size). The mechanism is a side effect one level down: Paparazzi's addView() takes a different internal code path once the root carries pre-set LayoutParams instead of null ones, which also affects how the nested Toolbar resolves its internal title TextView. Since a Toolbar with a title attribute is genuinely supposed to show that text, the new render is more correct than the old golden, which was quietly encoding a second, unrelated rendering defect the same way the root-params bug was. * Add the 1.0.2 changelog entry Documents the DF-4 root-params fix in user-facing language: layout previews now respect the root element's own layout_width/height, margins, and layout_gravity instead of always stretching over the entire device canvas. * Record AD-022 and close out the DF-4 amendment Records AD-022 in STATE.md Decisions (the inflation-seam fix, the bytecode-verified reasoning chain, the two mid-execution corrections found during T89/T90, and the corpus golden trade-off); replaces the Handoff's DF-4 entry with the completed record (commits, gate evidence); flips LAY-08's traceability row to implemented-pending- Verifier in spec.md; marks all T89-T94 task statuses complete in tasks.md. Full gate green at close: host build+test+engineTest (58 testcases, 0 failures), corpus (42/42), extension sanity (206/206 unit). * Record independent Verifier PASS for the DF-4 root-params fix Author != verifier: a fresh sub-agent re-derived LAY-08's 7 ACs + 3 edge cases from spec.md against file:line assertions (10/10 matched, 0 gaps), re-ran every gate from clean (engineTest 58/23 classes, corpus 42/42, extension 206/206), and ran a 3-mutation discrimination sensor in scratch state (null-parent revert, ViewGroup-vs-FrameLayout parent-type swap, CORE_ATTRS regression) — 3/3 killed. Appends the dated verification section to validation.md, flips LAY-08's spec.md traceability to Verified, and updates STATE.md's Handoff. Also fixes the one non-blocking finding: a fixture comment in rootparams_wrap.xml still described the pre-correction "stays transparent" AC4 assumption instead of the corrected theme-background outcome its own test asserts. * Bump @vscode/test-electron to fix macOS integration test spawn failure VS Code 1.131.0 (2026-07-29) is the first stable release without the compatibility symlink for the old "Electron" macOS executable name (microsoft/vscode#326502, the symlink from the Feb 2026 rename in microsoft/vscode#291948 was removed 2026-07-20). @vscode/test-electron 2.5.2 still launches the old hardcoded path, so every integration-test run against 1.131.0+ fails with "spawn .../MacOS/Electron ENOENT" before any test runs (microsoft/vscode-test#348, #349). 3.1.0 fixes this by resolving the executable via the bundle's CFBundleExecutable, falling back to "Electron" for older downloads (microsoft/vscode-test#350). Verified locally: 25/25 integration tests pass against a fresh VS Code 1.131.0 download.
macOS runners (arm64) fail with 'spawn .../Visual Studio Code.app/Contents/ MacOS/Electron ENOENT' because @vscode/test-electron <=3.0.0 hard-codes the old macOS executable name. Recent VS Code (1.131.0) renamed it; upstream fix microsoft/vscode-test#350 resolves the executable via CFBundleExecutable and ships in 3.1.0. 3.x requires Node >=22, so bump setup-node 20.x -> 22.x in all workflows that run the extension tests or 'npm ci' with dev deps (dev, main, release, prod_release, app-sec-template). q-api is left at 20.x (no test-electron).
### Why VS Code 1.131 removed the legacy `Contents/MacOS/Electron` compatibility path from its macOS application bundle. `@vscode/test-electron` 2.5.2 still launches that hard-coded path, so all macOS smoke jobs fail with `ENOENT` before VS Code or the extension starts. This currently blocks the macOS checks on #1656. The upstream issue and fix are microsoft/vscode-test#349 and microsoft/vscode-test#350. ### What Upgrade `@vscode/test-electron` from 2.5.2 to 3.1.0 and refresh the lockfile. Version 3.1.0 resolves the macOS executable from `CFBundleExecutable` in `Info.plist`, with the old `Electron` path retained as a fallback for older VS Code builds. This is a development-only dependency change. Version 3.1.0 requires Node 22, which is already used by CI. ### Validation - `npm run compile-tests` - `npm run unittest` — 1469 passing, 0 failing, 5 pending Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27
Fixes [issue](microsoft#3530), which was caused by a breaking change (or a sequence of them) by the vscode repo. First, they renamed the MacOS executable path from `.../Contents/MacOS/Electron` to `.../Contents/MacOS/Code` (or `.../Contents/MacOS/Code - Insiders`) [PR #291948](https://github.com/microsoft/vscode/pull/291948/changes). But they still kept the symlink, so that didn't cause the break. Then, they removed the symlink: [PR #326502](microsoft/vscode#326502), which caused this error, since the original path wasn't accessible anymore. But the vscode-test repo fixed it last week: [PR microsoft#350](microsoft/vscode-test#350) So all I had to do was update the package to the latest version. I ran the multiplatform pipeline again and it worked: [link](https://github.com/microsoft/qdk/actions/runs/30659943911)
Upgrade `@vscode/test-electron` from 2.5.2 to 3.1.0. Current VS Code macOS bundles no longer expose the historical `Contents/MacOS/Electron` executable. Version 3.1.0 includes [the upstream fix](microsoft/vscode-test#350) that resolves the executable from the app bundle metadata, preventing `just test-vscode` from failing with `ENOENT` on current Stable and Insiders builds. The new major version requires Node.js 22 or newer; this repository uses Node.js 24 in CI.
Summary
Fixes
spawn .../Contents/MacOS/Electron ENOENTon macOS when launching any VS Code build that ships without theElectronlegacy symlink (VS Code 1.110+ after microsoft/vscode#326502 rolled out).Closes #348.
Closes #349.
Symptom
Starting shortly after 2026-07-20 (see the timeline below),
sane downloads darwin— and any downstream consumer that spawns the path returned bydownloadAndUnzipVSCodeon macOS — began failing at the first-launch step:The
--versionprobe kept passing becauseresolveCliPathFromVSCodeExecutablePathreaches the CLI by relative traversal (../../../Contents/Resources/app/bin/code), which is unaffected by the binary rename.Root cause
VS Code 1.110 renamed the macOS main binary from
Contents/MacOS/Electronto the product's short name —Codeon Stable,Code - Insiderson Insiders — in microsoft/vscode#291948 (commitd0e516655abf…, merged 2026-02-03, first shipped in the 1.110 release train). Consumers that had baked the old name into their code kept working because the release rollout included a compatibility symlink at the historical location. That symlink was removed in microsoft/vscode#326502 (commit1cf2f4ee316905db…, merged 2026-07-20), at which point the two functions inlib/util.tsthat hardcode the old path:downloadDirToExecutablePathinsidersDownloadDirToExecutablePathstarted returning a path that no longer exists.
Fix
Both
downloadDirToExecutablePathandinsidersDownloadDirToExecutablePathnow delegate the darwin branch to a small helper:The strategy is intentionally minimal:
CFBundleExecutablefromInfo.plistis the authoritative source. It's the field macOS itself uses to locate the main executable when double-clicking a bundle. VS Code's plist ships in XML form (verified against a stock 1.111 install), so a targeted regex avoids pulling in a plist parser or shelling out toPlistBuddy— no new dependencies, no new subprocess calls, and the module keeps its current footprint.Electronname is the fallback. Pre-1.110 builds (or any exotic case where the plist can't be read) get the previous behaviour, so this is a strict superset of what the code did before.resolveCliPathFromVSCodeExecutablePathis not touched — it derives the CLI path by relative traversal, which is exactly what has been keeping the CLI probe working through the rename.Backward compatibility
The pre-1.110 path is preserved by the fallback: on any archive whose
Info.plistpredates the rename (or has noCFBundleExecutablestring, or can't be opened at all), the function returns the same string it returned before. I re-ran the darwin download-and-launch flow against an Insiders build locally and against a fake pre-rename bundle in the unit tests below — both work.Tests
lib/util.test.mtsgains adownloadDirToExecutablePath (darwin)suite with six cases (they usefs.mkdtempfor isolation, matching the pattern already used by theinsidersDownloadDirMetadatasuite):CFBundleExecutable=Code; expects.../Contents/MacOS/Code.CFBundleExecutable=Code - Insiders; expects.../Contents/MacOS/Code - Insiders..../Contents/MacOS/Electron..../Contents/MacOS/Electron.CFBundleExecutable) — falls back to.../Contents/MacOS/Electron.linux-x64,win32-x64-archivereturn identical paths to before.Full local run (excluding the network-heavy
sane downloadsintegration suite, which is not affected by these lines and would need the actual archive downloads):npm run compile,npx eslint 'lib/**/*.{ts,mts}',npx tsc --noEmit, andnpx prettier --checkare also clean.Timeline for reviewers
Electron → <product-name>symlink for compatibilitysane downloads darwinand every downstream consumer that spawns the returned path start failingRelated downstream workarounds
While reviewing the impact I ran into at least one downstream repo (
illixion/vscode-vibrancy-continued@4aa5f19) that has already added a client-side wrapper readingCFBundleExecutableviaPlistBuddy. Landing this fix here should let every such consumer drop their workaround.