Skip to content

Fix packaged terminal helper resolution - #22

Merged
sambitcreate merged 2 commits into
mainfrom
feature/terminal-asar-spawn-helper
Aug 11, 2026
Merged

Fix packaged terminal helper resolution#22
sambitcreate merged 2 commits into
mainfrom
feature/terminal-asar-spawn-helper

Conversation

@sambitcreate

@sambitcreate sambitcreate commented Aug 11, 2026

Copy link
Copy Markdown
Owner

What changed

  • Normalize node-pty package paths from Electron virtual app.asar and node_modules.asar locations to their real unpacked directories before any filesystem discovery or permission repair.
  • Isolate the production spawn-helper resolver and enforce 100% line, branch, and function coverage for the ASAR rewrite and discovery contract.
  • Expand terminal service coverage across input, resize, output, snapshots, history, ownership, teardown, natural exit, and session limits.
  • Make macOS package verification require both arm64 and x64 node-pty helpers to be ASAR-unpacked, regular non-symlink files with mode 0755.
  • Add a workspace-backed Electron E2E that opens a real PTY, runs a shell command, verifies the workspace cwd, observes persisted output, and closes the drawer.

Root cause

The packaged app correctly shipped node-pty under app.asar.unpacked, and the release hook already made spawn-helper executable. Electron ASAR-aware filesystem calls also exposed a virtual app.asar placeholder as a regular non-executable file. Aiden enumerated that virtual location and attempted to chmod it, which failed with ENOTDIR before node-pty could use the valid executable under app.asar.unpacked.

Why tests missed it

Four independent blind spots lined up:

  1. Terminal unit tests injected spawnHelperPaths or prepareSpawnHelper, bypassing the production package-directory resolver.
  2. Release-hook tests verified chmod behavior only against ordinary unpacked files, not runtime ASAR lookup.
  3. Package verification checked signing and layout but did not assert the node-pty runtime resource contract.
  4. The deterministic E2E profile intentionally had no folder workspace and only asserted that Show terminal was disabled, so no renderer-to-shell terminal path ran.

The new tests close all four gaps. The bug-critical resolver is CI-gated at 100/100/100 coverage. The broader terminal service is currently 95.46% lines, 81.20% branches, and 94.74% functions, with CI floors of 95/80/90 to prevent regression. Behavior that only exists in a packaged layout is additionally enforced by package verification rather than relying on code coverage alone.

User impact

Opening the integrated terminal in the installed macOS app no longer fails with the terminal:create / spawn-helper ENOTDIR error.

Validation

  • npm run test:terminal:coverage — resolver 100% line/branch/function coverage; terminal service floors passed
  • npm test — 1,295/1,295 application tests, 33/33 native worktree-remover tests, and 41/41 Rust broker tests
  • npm run test:e2e — 7/7 Electron E2Es, including the real workspace terminal flow
  • npm run type-check and npm run type-check:e2e
  • targeted ESLint and git diff --check
  • npm run package — fresh signed development package built and after-sign verification passed
  • npm run package:verify — hardened package and both unpacked executable node-pty helpers verified

@sambitcreate
sambitcreate marked this pull request as ready for review August 11, 2026 19:48

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • resolveNodePtyDiskPackageDir regex ASAR normalization — new terminal-spawn-helper.ts uses (/([/\\])app\.asar([/\\])/u, "$1app.asar.unpacked$2") to normalize package-directory paths before any filesystem operation, mirroring node-pty's own helperPath.replace('app.asar', 'app.asar.unpacked') but requiring path-separator delimiters on both sides, which prevents double-.unpacked rewriting that node-pty's naive .replace() would produce.
  • resolveNodePtySpawnHelperPaths seam — a readDirectory injectable lets regression tests exercise the exact production resolver including normalization and discovery without requiring an Electron ASAR mount; normalization runs before readdir so neither discovery nor chmod ever targets a virtual ASAR path.
  • defaultSpawnHelperPaths delegation — the production resolver now delegates to resolveNodePtySpawnHelperPaths, replacing the old inlined logic that enumerated the virtual ASAR prebuilds directory and appended duplicate unpacked entries.
  • Terminal service test coverage expansionfakePty and ownerState now record writes, resizes, data/exit events, and IPC sends; 7 new tests cover output buffering, history append+flush, IPC notification channels, teardown scoping by workspace and web-contents, per-document session caps, and history-flush deduplication.
  • Package verification hardeningverify-macos-package.mjs gains verifyPackagedNodePtyResources that asserts both darwin-arm64 and darwin-x64 spawn-helper entries are ASAR-unpacked regular files with mode 0755 on the unpacked filesystem copy; wired into verifyMacPackage.
  • E2E workspace terminal spec — new Playwright test seeds a folder workspace, opens the terminal drawer, types echo $((314159+271828)); pwd, polls the persisted history file for 585987 and the workspace directory, and closes the drawer.
  • pretest integrationtest:terminal:coverage is now part of pretest with coverage floors of 100/100/100 on the resolver and 95/80/90 on the terminal service.

Pullfrog  | View workflow run | Using DeepSeek Pro𝕏

@sambitcreate
sambitcreate merged commit c8053ba into main Aug 11, 2026
4 checks passed
@sambitcreate
sambitcreate deleted the feature/terminal-asar-spawn-helper branch August 11, 2026 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant