Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ describe('ClaudeTrustService', () => {
const ctx: IExecutionContext = {
root: undefined,
supportsLocalSpawn: false,
isWindows: false,
exec: vi.fn().mockImplementation(async (command: string, args: string[] = []) => {
if (command === 'sh') {
return { stdout: '/home/remote-user', stderr: '' };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ function makeCtx(): IExecutionContext {
return {
root: undefined,
supportsLocalSpawn: false,
isWindows: false,
exec: vi.fn().mockImplementation(async (command: string) => {
if (command === 'sh') return { stdout: '/home/remote-user', stderr: '' };
return { stdout: '', stderr: '' };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ vi.mock('@main/core/dependencies/probe', () => ({
function makeExecutionContext(): IExecutionContext {
return {
supportsLocalSpawn: false,
isWindows: false,
exec: vi.fn(async () => ({ stdout: '', stderr: '' })),
execStreaming: vi.fn(async () => {}),
dispose: vi.fn(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ function makeCtx(): IExecutionContext {
return {
root: undefined,
supportsLocalSpawn: false,
isWindows: false,
exec: vi.fn(),
execStreaming: vi.fn(),
dispose: vi.fn(),
Expand Down
72 changes: 72 additions & 0 deletions apps/emdash-desktop/src/main/core/dependencies/probe.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { describe, expect, it, vi } from 'vitest';
import type { IExecutionContext } from '@main/core/execution-context/types';
import { resolveCommandPath } from './probe';

function makeCtx(
isWindows: boolean,
handler: (command: string, args: string[]) => Promise<{ stdout: string; stderr: string }>
): IExecutionContext {
return {
root: undefined,
supportsLocalSpawn: false,
isWindows,
exec: vi.fn().mockImplementation(handler),
execStreaming: vi.fn(),
dispose: vi.fn(),
} as unknown as IExecutionContext;
}

describe('resolveCommandPath', () => {
it('uses `which` on POSIX hosts', async () => {
const ctx = makeCtx(false, async () => ({ stdout: '/usr/local/bin/claude\n', stderr: '' }));

const path = await resolveCommandPath('claude', ctx);

expect(ctx.exec).toHaveBeenCalledWith('which', ['claude'], { timeout: 5000 });
expect(path).toBe('/usr/local/bin/claude');
});

it('uses `where` on Windows hosts', async () => {
const ctx = makeCtx(true, async () => ({ stdout: 'C:\\bin\\claude.exe\n', stderr: '' }));

const path = await resolveCommandPath('claude', ctx);

expect(ctx.exec).toHaveBeenCalledWith('where', ['claude'], { timeout: 5000 });
expect(path).toBe('C:\\bin\\claude.exe');
});

// Regression: a Windows client connected to a POSIX remote over SSH must run
// `which` on the remote host, not `where`. The resolve command follows the
// execution context's platform, never the local client's. See issue #2474.
it('uses `which` for a POSIX SSH context even when the client is Windows', async () => {
const ctx = makeCtx(false, async () => ({
stdout: '/home/dev/.local/bin/claude\n',
stderr: '',
}));

const path = await resolveCommandPath('claude', ctx);

expect(ctx.exec).toHaveBeenCalledWith('which', ['claude'], { timeout: 5000 });
expect(ctx.exec).not.toHaveBeenCalledWith('where', expect.anything(), expect.anything());
expect(path).toBe('/home/dev/.local/bin/claude');
});

it('returns the first match and trims surrounding whitespace', async () => {
const ctx = makeCtx(true, async () => ({
stdout: 'C:\\bin\\claude.exe\r\nC:\\other\\claude.exe\r\n',
stderr: '',
}));

const path = await resolveCommandPath('claude', ctx);

expect(path).toBe('C:\\bin\\claude.exe');
});

it('returns null when resolution fails', async () => {
const ctx = makeCtx(false, async () => {
throw new Error('not found');
});

expect(await resolveCommandPath('claude', ctx)).toBeNull();
});
});
12 changes: 8 additions & 4 deletions apps/emdash-desktop/src/main/core/dependencies/probe.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,24 @@ import type { ProbeResult } from './types';
const WHICH_TIMEOUT_MS = 5_000;
const VERSION_PROBE_TIMEOUT_MS = 10_000;

// `where` on Windows, `which` on macOS/Linux
const RESOLVE_CMD = process.platform === 'win32' ? 'where' : 'which';
// The resolve command must match the platform the context executes on, not the
// local client: a Windows client connected to a POSIX remote over SSH must run
// `which` on the remote host, never `where`.
function resolveCmd(ctx: IExecutionContext): string {
return ctx.isWindows ? 'where' : 'which';
}

/**
* Resolves the absolute path of a command binary.
* Uses `where` on Windows and `which` on macOS/Linux.
* Uses `where` on Windows hosts and `which` on macOS/Linux hosts.
* Returns `null` if the command is not found or the resolution fails.
*/
export async function resolveCommandPath(
command: string,
ctx: IExecutionContext
): Promise<string | null> {
try {
const { stdout } = await ctx.exec(RESOLVE_CMD, [command], { timeout: WHICH_TIMEOUT_MS });
const { stdout } = await ctx.exec(resolveCmd(ctx), [command], { timeout: WHICH_TIMEOUT_MS });
const firstLine = stdout.trim().split('\n')[0]?.trim();
return firstLine ?? null;
} catch {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ function buildNonInteractiveGitEnv(): NodeJS.ProcessEnv {
export class LocalExecutionContext implements IExecutionContext {
readonly root: string;
readonly supportsLocalSpawn = true;
readonly isWindows = process.platform === 'win32';

private readonly _lifetime = new AbortController();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export function buildSshCommand(
export class SshExecutionContext implements IExecutionContext {
readonly root?: string;
readonly supportsLocalSpawn = false;
readonly isWindows = false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Hardcoded false assumes all SSH targets are POSIX

isWindows = false is correct for the documented use-case (remote Linux/macOS servers), and the JSDoc on the interface acknowledges this. However, the assumption is baked into the class and will silently misbehave if an SSH-to-Windows target (e.g. OpenSSH on Windows Server) is ever introduced — which does not exist there and every probe would fail just like the original bug. Worth a TODO comment flagging this limitation so it is visible if a Windows SSH target needs to be supported later.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/emdash-desktop/src/main/core/execution-context/ssh-execution-context.ts
Line: 40

Comment:
**Hardcoded `false` assumes all SSH targets are POSIX**

`isWindows = false` is correct for the documented use-case (remote Linux/macOS servers), and the JSDoc on the interface acknowledges this. However, the assumption is baked into the class and will silently misbehave if an SSH-to-Windows target (e.g. OpenSSH on Windows Server) is ever introduced — `which` does not exist there and every probe would fail just like the original bug. Worth a TODO comment flagging this limitation so it is visible if a Windows SSH target needs to be supported later.

How can I resolve this? If you propose a fix, please make it concise.


private readonly _lifetime = new AbortController();

Expand Down
9 changes: 9 additions & 0 deletions apps/emdash-desktop/src/main/core/execution-context/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ export interface IExecutionContext {
*/
readonly supportsLocalSpawn: boolean;

/**
* Whether the host this context executes on is Windows. Consumers must use
* this — not the local `process.platform` — when picking platform-specific
* commands (e.g. `where` vs `which`), so a Windows client connected to a
* POSIX remote over SSH still runs the right command on the remote host.
* SSH remotes are POSIX, so this is always false for them.
*/
readonly isWindows: boolean;

/** Run a command and buffer all output. Rejects on non-zero exit code. */
exec(command: string, args?: string[], opts?: ExecOptions): Promise<ExecResult>;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function makeContext(exec: MockExec, root = '/repo'): IExecutionContext {
return {
root,
supportsLocalSpawn: false,
isWindows: false,
exec: (_cmd, args = [], _opts) => exec(_cmd, args),
execStreaming: async (_cmd, _args, onChunk) => {
onChunk('');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ describe('WorktreeService', () => {
const remoteCtx = {
root: '/remote/repo',
supportsLocalSpawn: false,
isWindows: false,
exec: vi.fn().mockResolvedValue({ stdout: '', stderr: '' }),
execStreaming: vi.fn().mockResolvedValue(undefined),
dispose: vi.fn(),
Expand Down Expand Up @@ -149,6 +150,7 @@ describe('WorktreeService', () => {
const ctx: IExecutionContext = {
root: repoDir,
supportsLocalSpawn: false,
isWindows: false,
exec,
execStreaming: async () => {},
dispose: () => {},
Expand Down Expand Up @@ -228,6 +230,7 @@ describe('WorktreeService', () => {
const ctx: IExecutionContext = {
root: repoDir,
supportsLocalSpawn: false,
isWindows: false,
exec,
execStreaming: async () => {},
dispose: () => {},
Expand Down Expand Up @@ -306,6 +309,7 @@ describe('WorktreeService', () => {
const ctx: IExecutionContext = {
root: repoDir,
supportsLocalSpawn: false,
isWindows: false,
exec,
execStreaming: async () => {},
dispose: () => {},
Expand Down Expand Up @@ -431,6 +435,7 @@ describe('WorktreeService', () => {
const ctx: IExecutionContext = {
root: repoDir,
supportsLocalSpawn: false,
isWindows: false,
exec,
execStreaming: async () => {},
dispose: () => {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ const terminal: Terminal = {

const ctx = {
supportsLocalSpawn: true,
isWindows: false,
exec: vi.fn(),
execStreaming: vi.fn(),
dispose: vi.fn(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const terminal: Terminal = {

const ctx = {
supportsLocalSpawn: false,
isWindows: false,
exec: vi.fn(),
execStreaming: vi.fn(),
dispose: vi.fn(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,7 @@ describe('legacy-port table passes', () => {
const tmuxExec = {
root: undefined,
supportsLocalSpawn: false,
isWindows: false,
exec: async (command: string, args: string[] = []) => {
calls.push({ command, args });
if (
Expand Down