Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
194 changes: 193 additions & 1 deletion assistant/src/__tests__/host-bash-proxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,22 @@ mock.module("../config/loader.js", () => ({
}));

const sentMessages: unknown[] = [];
const sentMessageOptions: unknown[] = [];
const resolvedInteractionIds: string[] = [];
let mockHasClient = false;
let mockCapableClients: Array<{ clientId: string; capabilities: string[] }> = [];
let mockClientRegistry: Map<string, { clientId: string; capabilities: string[] }> = new Map();

mock.module("../runtime/assistant-event-hub.js", () => ({
broadcastMessage: (msg: unknown) => sentMessages.push(msg),
broadcastMessage: (msg: unknown, _conversationId?: string, options?: unknown) => {
sentMessages.push(msg);
sentMessageOptions.push(options);
},
assistantEventHub: {
getMostRecentClientByCapability: (cap: string) =>
cap === "host_bash" && mockHasClient ? { id: "mock-client" } : null,
listClientsByCapability: (_cap: string) => mockCapableClients,
getClientById: (clientId: string) => mockClientRegistry.get(clientId),
},
}));

Expand All @@ -45,11 +53,30 @@ describe("HostBashProxy", () => {

function setup() {
sentMessages.length = 0;
sentMessageOptions.length = 0;
resolvedInteractionIds.length = 0;
mockHasClient = false;
mockCapableClients = [];
mockClientRegistry = new Map();
proxy = new (HostBashProxy as any)();
}

function setupSingleClient(clientId = "client-1") {
const entry = { clientId, capabilities: ["host_bash"] };
mockCapableClients = [entry];
mockClientRegistry.set(clientId, entry);
}

function setupMultipleClients(clientIds: string[]) {
mockCapableClients = clientIds.map((id) => ({
clientId: id,
capabilities: ["host_bash"],
}));
for (const entry of mockCapableClients) {
mockClientRegistry.set(entry.clientId, entry);
}
}

afterEach(() => {
proxy?.dispose();
HostBashProxy.reset();
Expand Down Expand Up @@ -528,4 +555,169 @@ describe("HostBashProxy", () => {
expect(resolvedInteractionIds).toEqual([]);
});
});

describe("target client routing", () => {
test("auto-resolves when exactly one capable client is connected", async () => {
setup();
setupSingleClient("client-abc");

const resultPromise = proxy.request(
{ command: "echo hello" },
"session-1",
);

expect(sentMessages).toHaveLength(1);
const sent = sentMessages[0] as Record<string, unknown>;
expect(sent.targetClientId).toBe("client-abc");

// Options passed to broadcastMessage should also have targetClientId
const opts = sentMessageOptions[0] as Record<string, unknown> | undefined;
expect(opts?.targetClientId).toBe("client-abc");

const requestId = sent.requestId as string;
proxy.resolve(requestId, {
stdout: "hello\n",
stderr: "",
exitCode: 0,
timedOut: false,
});

const result = await resultPromise;
expect(result.isError).toBe(false);
});

test("uses explicit targetClientId when it is valid", async () => {
setup();
setupSingleClient("client-abc");
// Also register a second client so we're sure explicit targeting works
const entry2 = { clientId: "client-xyz", capabilities: ["host_bash"] };
mockCapableClients.push(entry2);
mockClientRegistry.set("client-xyz", entry2);

const resultPromise = proxy.request(
{ command: "echo hello", targetClientId: "client-abc" },
"session-1",
);

expect(sentMessages).toHaveLength(1);
const sent = sentMessages[0] as Record<string, unknown>;
expect(sent.targetClientId).toBe("client-abc");

const opts = sentMessageOptions[0] as Record<string, unknown> | undefined;
expect(opts?.targetClientId).toBe("client-abc");

const requestId = sent.requestId as string;
proxy.resolve(requestId, {
stdout: "ok\n",
stderr: "",
exitCode: 0,
timedOut: false,
});

const result = await resultPromise;
expect(result.isError).toBe(false);
});

test("returns error for explicit targetClientId that is not connected", async () => {
setup();
setupSingleClient("client-abc");

const result = await proxy.request(
{ command: "echo hello", targetClientId: "client-unknown" },
"session-1",
);

// Should return error without broadcasting
expect(result.isError).toBe(true);
expect(result.content).toContain("client-unknown");
expect(result.content).toContain("assistant clients list --capability host_bash");
expect(sentMessages).toHaveLength(0);
});

test("returns error for explicit targetClientId that is connected but lacks host_bash", async () => {
setup();
// Register a client without host_bash capability
mockClientRegistry.set("client-no-bash", {
clientId: "client-no-bash",
capabilities: [],
});

const result = await proxy.request(
{ command: "echo hello", targetClientId: "client-no-bash" },
"session-1",
);

expect(result.isError).toBe(true);
expect(result.content).toContain("client-no-bash");
expect(result.content).toContain("does not support host_bash");
expect(sentMessages).toHaveLength(0);
});

test("returns error when multiple capable clients are connected and no targetClientId", async () => {
setup();
setupMultipleClients(["client-1", "client-2", "client-3"]);

const result = await proxy.request(
{ command: "echo hello" },
"session-1",
);

expect(result.isError).toBe(true);
expect(result.content).toContain("multiple clients support host_bash");
expect(result.content).toContain("3 connected");
expect(result.content).toContain("target_client_id");
expect(result.content).toContain("assistant clients list --capability host_bash");
expect(sentMessages).toHaveLength(0);
});

test("falls through to broadcast when zero capable clients (existing timeout path)", async () => {
setup();
// mockCapableClients is empty (default), so capableClients.length === 0

const resultPromise = proxy.request(
{ command: "echo hello" },
"session-1",
);

// Should still broadcast (no early return)
expect(sentMessages).toHaveLength(1);
const sent = sentMessages[0] as Record<string, unknown>;
expect(sent.type).toBe("host_bash_request");
// targetClientId is undefined when no clients present
expect(sent.targetClientId).toBeUndefined();

// Manually resolve to clean up
const requestId = sent.requestId as string;
proxy.resolve(requestId, {
stdout: "",
stderr: "",
exitCode: 0,
timedOut: false,
});

await resultPromise;
});

test("includes targetClientId in timeout error message when client was resolved", async () => {
setup();
setupSingleClient("client-mac");

jest.useFakeTimers();
try {
const resultPromise = proxy.request(
{ command: "echo slow", timeout_seconds: 30 },
"session-1",
);

// Proxy timeout = 33s; advance past it
jest.advanceTimersByTime(34 * 1000);

const result = await resultPromise;
expect(result.isError).toBe(true);
expect(result.content).toContain("client-mac");
} finally {
jest.useRealTimers();
}
});
});
});
57 changes: 46 additions & 11 deletions assistant/src/daemon/host-bash-proxy.ts
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ interface PendingRequest {
conversationId: string;
/** Detach the abort listener from the caller's signal. No-op when no signal was passed. */
detachAbort: () => void;
targetClientId?: string;
}

export class HostBashProxy {
Expand Down Expand Up @@ -69,6 +70,7 @@ export class HostBashProxy {
working_dir?: string;
timeout_seconds?: number;
env?: Record<string, string>;
targetClientId?: string;
},
conversationId: string,
signal?: AbortSignal,
Expand All @@ -78,6 +80,30 @@ export class HostBashProxy {
return Promise.resolve(result);
}

const capableClients = assistantEventHub.listClientsByCapability("host_bash");

let resolvedTargetClientId: string | undefined;

if (input.targetClientId) {
const target = assistantEventHub.getClientById(input.targetClientId);
if (!target || !target.capabilities.includes("host_bash")) {
return Promise.resolve({
content: `Error: client "${input.targetClientId}" is not connected or does not support host_bash. Run \`assistant clients list --capability host_bash\` to see available clients.`,
isError: true,
});
}
resolvedTargetClientId = input.targetClientId;
} else if (capableClients.length === 1) {
// Auto-resolve when exactly one capable client is connected.
resolvedTargetClientId = capableClients[0].clientId;
} else if (capableClients.length > 1) {
return Promise.resolve({
content: `Error: multiple clients support host_bash (${capableClients.length} connected). Specify which client to use with \`target_client_id\`. Run \`assistant clients list --capability host_bash\` to see client IDs and labels.`,
isError: true,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve host_bash execution when multiple clients connected

Returning an error whenever capableClients.length > 1 makes host_bash unusable in multi-client sessions, because no current caller can actually provide a target client: HostShellTool still defines no target_client_id input and its HostBashProxy.instance.request(...) calls never pass targetClientId (see assistant/src/tools/host-terminal/host-shell.ts). Before this change, commands still executed by routing to a recent capable client; after this commit they fail with an instruction users/LLM cannot satisfy through the existing tool interface.

Useful? React with 👍 / 👎.

}
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Outdated
// capableClients.length === 0: fall through — existing timeout/error path handles it.

const requestId = uuid();

return new Promise<ToolExecutionResult>((resolve, reject) => {
Expand All @@ -98,10 +124,13 @@ export class HostBashProxy {
{ requestId, command: input.command },
"Host bash proxy request timed out",
);
const timeoutMessage = resolvedTargetClientId
? `Host bash proxy timed out waiting for response from client ${resolvedTargetClientId}`
: "Host bash proxy timed out waiting for client response";
resolve(
formatShellOutput(
"",
"Host bash proxy timed out waiting for client response",
timeoutMessage,
null,
true,
timeoutSec,
Expand Down Expand Up @@ -139,20 +168,26 @@ export class HostBashProxy {
timeoutSec,
conversationId,
detachAbort,
targetClientId: resolvedTargetClientId,
});

try {
broadcastMessage({
type: "host_bash_request",
requestId,
broadcastMessage(
{
type: "host_bash_request",
requestId,
conversationId,
command: input.command,
working_dir: input.working_dir,
timeout_seconds: input.timeout_seconds,
targetClientId: resolvedTargetClientId,
...(input.env && Object.keys(input.env).length > 0
? { env: input.env }
: {}),
},
conversationId,
command: input.command,
working_dir: input.working_dir,
timeout_seconds: input.timeout_seconds,
...(input.env && Object.keys(input.env).length > 0
? { env: input.env }
: {}),
});
{ targetClientId: resolvedTargetClientId },
);
} catch (err) {
clearTimeout(timer);
this.pending.delete(requestId);
Expand Down
Loading