fix(pty): fix tmux absolute-path test to pass in CI

[arnestrickmann]

Summary

• Adds process.env.PATH setup in the tmux spawn test so resolveCommandPath('tmux') finds the mocked /opt/homebrew/bin/tmux binary
• Changes require('node-pty') to await import('node-pty') in startPty so vitest can intercept the mock (vitest does not intercept CJS require() calls)
• Preserves all tmux resolution logic from fix(pty): resolve tmux to absolute path before spawning #1470 — only fixes the test that was silently broken

Test plan

• pnpm exec vitest run — all 655 tests pass (including the previously failing tmux test)
• pnpm run type-check — clean
• pnpm run lint — no new warnings
• pnpm run format — no changes

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
docs	Ready	Preview, Comment	Mar 14, 2026 5:38am

[greptile-apps]

Greptile Summary

This PR fixes the tmux absolute-path test that was silently broken in CI. It makes two targeted changes: (1) replaces require('node-pty') with await import('node-pty') in startPty so Vitest's ESM mock interception actually intercepts the module load, and (2) prepends /opt/homebrew/bin to process.env.PATH inside the test so resolveCommandPath('tmux') resolves to the mocked binary path before the freshly-reset module is imported.

• ptyManager.ts: require('node-pty') → await import('node-pty'). This is the root cause fix. Vitest intercepts vi.mock('node-pty', …) for dynamic ESM imports but not for CJS require() calls, so the mock was never active before. The redundant @typescript-eslint/no-var-requires suppression comment is also removed. No production behaviour changes — the import is still lazy and the existing try/catch is preserved.
• ptyManager.test.ts: Prepends /opt/homebrew/bin to process.env.PATH and restores it after startPty. The resolvedCommandPathCache is cleared by vi.resetModules() in beforeEach, so the fresh module correctly picks up the modified PATH at resolution time. One concern: process.env.PATH is restored after startPty but is not wrapped in try/finally, so if startPty rejects the PATH mutation will leak into subsequent tests.

Confidence Score: 4/5

• Safe to merge with one minor test-isolation issue to address
• The production-code change is minimal, correct, and has no behavioural impact at runtime. The test fix is logically sound — PATH is set before the fresh module is loaded, which is the right order given vi.resetModules(). The only deficiency is the missing try/finally guard around the PATH mutation, which could leak state to subsequent tests if startPty ever throws in CI. This is a test-quality concern rather than a correctness or safety blocker.
• src/test/main/ptyManager.test.ts — the process.env.PATH restore at line 359 should be inside a try/finally block

Important Files Changed

Filename	Overview
src/main/services/ptyManager.ts	Replaces synchronous require('node-pty') with await import('node-pty') so Vitest's ESM mock interception works; also removes the now-redundant eslint-disable comment. No behavioural change in production — the import is still lazy (called at invocation time), startPty was already async, and the try/catch error path is preserved.
src/test/main/ptyManager.test.ts	Prepends /opt/homebrew/bin to process.env.PATH before calling startPty so resolveCommandPath('tmux') finds the mocked binary. The PATH is restored after the call, but the restore is not guarded by try/finally, so a failure in startPty would leave the PATH mutation in place for subsequent tests.

Sequence Diagram

sequenceDiagram
    participant Test as ptyManager.test.ts
    participant VI as Vitest (mock engine)
    participant PM as ptyManager.ts (startPty)
    participant NodePty as node-pty (mocked)

    Note over Test: beforeEach → vi.resetModules()<br/>clears module registry + resolvedCommandPathCache

    Test->>Test: process.env.PATH = "/opt/homebrew/bin:..."
    Test->>PM: await import('../../main/services/ptyManager')
    Test->>PM: startPty({ id, cwd, shell, tmux:true })

    PM->>PM: resolveCommandPathCached('tmux')<br/>reads process.env.PATH → finds /opt/homebrew/bin/tmux
    PM->>VI: await import('node-pty')
    VI-->>PM: { spawn: nodePtySpawnMock }  [mock intercepted ✓]
    PM->>NodePty: pty.spawn('/opt/homebrew/bin/tmux', [...], opts)
    NodePty-->>PM: mockProc

    PM-->>Test: IPty
    Test->>Test: process.env.PATH = origPath  ⚠️ not in finally
    Test->>Test: expect(nodePtySpawnMock).toHaveBeenCalledWith('/opt/homebrew/bin/tmux', ...)

Loading

Comments Outside Diff (1)

1. src/test/main/ptyManager.test.ts, line 352-359 (link)

   PATH not restored on startPty failure

   process.env.PATH = origPath is only reached when startPty() resolves successfully. If the call throws or rejects — e.g., a mock is misconfigured, a CI environment differs — the mutation leaks into every subsequent test in the suite. Since vi.resetModules() only resets the module registry and resolveCommandPath reads process.env.PATH dynamically at call time, a leaked /opt/homebrew/bin prefix could silently affect other tests that rely on tmux-resolution or PATH-sensitive logic.

   Use try/finally to guarantee cleanup regardless of outcome, or use Vitest's built-in vi.stubEnv which restores the original value automatically:

   // Option A – try/finally
   try {
     await startPty({
       id: 'claude-main-task-tmux',
       cwd: '/tmp/task',
       shell: '/bin/zsh',
       tmux: true,
     });
   } finally {
     process.env.PATH = origPath;
   }
   
   // Option B – vi.stubEnv (auto-restored after the test)
   vi.stubEnv('PATH', `/opt/homebrew/bin${process.env.PATH ? ':' + process.env.PATH : ''}`);
   

_{Last reviewed commit: 572cf02}