Skip to content

Commit 16795bb

Browse files
committed
fix(shell): advertise foreground-only timeout default
Summary run_shell schema timeout copy now says the default is foreground-only and to omit timeout on background:true. Advertise uses the same default as resolveShellTimeoutMs when only maxTimeoutMs is set. Verification bun test ./src/plugins/shell-guard-plugin.test.ts bun run typecheck bun run check
1 parent 1705799 commit 16795bb

4 files changed

Lines changed: 87 additions & 53 deletions

File tree

‎src/agent/tools.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -592,7 +592,11 @@ export async function createAgentToolset(
592592
const baseTools: AgentTool[] = [
593593
...fromToolRunner(posixTools).map((tool) => {
594594
let definition = advertiseEditFileLineRange(
595-
advertiseShellGuardTimeout(tool.definition, shellTimeout?.defaultMs),
595+
advertiseShellGuardTimeout(
596+
tool.definition,
597+
shellTimeout?.defaultMs,
598+
shellTimeout?.maxMs,
599+
),
596600
);
597601
if (getEvidenceArchive !== undefined)
598602
definition = advertiseArchiveSurface(definition);

‎src/plugins/shell-guard-plugin.test.ts‎

Lines changed: 58 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -497,40 +497,25 @@ describe("background run_shell (shellGuardPlugin)", () => {
497497
});
498498

499499
describe("advertiseShellGuardTimeout", () => {
500-
test("rewrites run_shell timeout description when a settings default is set", () => {
501-
const rewritten = advertiseShellGuardTimeout(
502-
{
503-
name: "run_shell",
504-
description: "Execute a shell command",
505-
inputSchema: {
506-
type: "object",
507-
properties: {
508-
command: { type: "string" },
509-
timeout: {
510-
type: "number",
511-
description: "Timeout in milliseconds (default: 30000)",
512-
},
513-
},
514-
required: ["command"],
515-
},
516-
},
517-
120_000,
518-
);
519-
const timeout = (
520-
rewritten.inputSchema["properties"] as Record<
521-
string,
522-
{ description: string }
523-
>
524-
)["timeout"];
525-
expect(timeout?.description).toContain("120000");
526-
expect(timeout?.description).not.toContain("30000");
527-
expect((timeout as { default?: number } | undefined)?.default).toBe(
528-
120_000,
529-
);
530-
});
500+
function timeoutSchema(def: {
501+
inputSchema: { properties?: unknown };
502+
}): { description?: string; default?: number } | undefined {
503+
const properties = def.inputSchema["properties"] as
504+
| Record<string, { description?: string; default?: number }>
505+
| undefined;
506+
return properties?.["timeout"];
507+
}
531508

532-
test("advertises default 120000 when settings default is unset", () => {
533-
const rewritten = advertiseShellGuardTimeout({
509+
function runShellDef(): {
510+
name: string;
511+
description: string;
512+
inputSchema: {
513+
type: "object";
514+
properties: Record<string, { type: string; description?: string }>;
515+
required: string[];
516+
};
517+
} {
518+
return {
534519
name: "run_shell",
535520
description: "Execute a shell command",
536521
inputSchema: {
@@ -544,18 +529,48 @@ describe("advertiseShellGuardTimeout", () => {
544529
},
545530
required: ["command"],
546531
},
547-
});
548-
const timeout = (
549-
rewritten.inputSchema["properties"] as Record<
550-
string,
551-
{ description: string; default?: number }
552-
>
553-
)["timeout"];
554-
expect(timeout?.description).toContain("120000");
532+
};
533+
}
534+
535+
test("rewrites run_shell timeout description when a settings default is set", () => {
536+
const rewritten = advertiseShellGuardTimeout(runShellDef(), 120_000);
537+
const timeout = timeoutSchema(rewritten);
538+
expect(timeout?.description).toContain("foreground default: 120000");
539+
expect(timeout?.description).toMatch(
540+
/omit on background:true for no timeout/,
541+
);
542+
expect(timeout?.description).not.toContain("30000");
543+
expect(timeout?.default).toBe(120_000);
544+
});
545+
546+
test("advertises default 120000 when settings default is unset", () => {
547+
const rewritten = advertiseShellGuardTimeout(runShellDef());
548+
const timeout = timeoutSchema(rewritten);
549+
expect(timeout?.description).toContain("foreground default: 120000");
555550
expect(timeout?.default).toBe(120_000);
556551
expect(timeout?.description).not.toContain("30000");
557552
expect(timeout?.description).not.toContain("15000");
558-
expect(timeout?.description).not.toMatch(/no default|omit/i);
553+
expect(timeout?.description).toMatch(
554+
/omit on background:true for no timeout/,
555+
);
556+
});
557+
558+
test("advertised default matches resolver when only maxTimeoutMs is set", () => {
559+
const maxMs = 60_000;
560+
const resolved = resolveShellTimeoutMs({
561+
requested: undefined,
562+
background: false,
563+
maxMs,
564+
});
565+
const rewritten = advertiseShellGuardTimeout(
566+
runShellDef(),
567+
undefined,
568+
maxMs,
569+
);
570+
const timeout = timeoutSchema(rewritten);
571+
expect(resolved).toBe(60_000);
572+
expect(timeout?.default).toBe(resolved);
573+
expect(timeout?.description).toContain(`foreground default: ${resolved}`);
559574
});
560575

561576
test("leaves other tools unchanged", () => {
@@ -585,6 +600,7 @@ describe("advertiseShellGuardTimeout", () => {
585600
)["background"];
586601
expect(background).toBeDefined();
587602
expect(background?.description).toContain("shell_collect");
603+
expect(background?.description).toMatch(/omit timeout/i);
588604
expect(background?.description).toMatch(
589605
/does not change the retained shell cwd/i,
590606
);

‎src/plugins/shell-guard-plugin.ts‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -82,13 +82,19 @@ export function formatShellTimeoutNotice(timeoutMs: number): string {
8282

8383
/**
8484
* Stock tools-posix still advertises timeout default 30000. Shell-guard's
85-
* foreground default is 120s (settings.shell.timeoutMs overrides); rewrite the
86-
* definition the model sees so schema and behavior agree. Corbits Code-only —
87-
* does not patch interchange.
85+
* foreground default is 120s (settings.shell.timeoutMs overrides; maxTimeoutMs
86+
* clamps that default path); rewrite the definition the model sees so schema
87+
* and behavior agree. Corbits Code-only — does not patch interchange.
88+
*
89+
* Schema `default` is the foreground omit path. Description says omit
90+
* timeout on background:true so the model does not copy 120000 onto
91+
* background calls (a copied value becomes a requested timeout and kills
92+
* the job).
8893
*/
8994
export function advertiseShellGuardTimeout(
9095
definition: ToolDefinition,
9196
defaultMs?: number,
97+
maxMs?: number,
9298
): ToolDefinition {
9399
if (definition.name !== "run_shell") return definition;
94100
const schema = definition.inputSchema;
@@ -101,17 +107,20 @@ export function advertiseShellGuardTimeout(
101107
const cwdProp = properties["cwd"];
102108
const nextProperties = { ...properties };
103109
const advertisedDefault =
104-
defaultMs !== undefined && defaultMs > 0
105-
? defaultMs
106-
: DEFAULT_FOREGROUND_SHELL_TIMEOUT_MS;
110+
resolveShellTimeoutMs({
111+
requested: undefined,
112+
background: false,
113+
...(defaultMs !== undefined ? { defaultMs } : {}),
114+
...(maxMs !== undefined ? { maxMs } : {}),
115+
}) ?? DEFAULT_FOREGROUND_SHELL_TIMEOUT_MS;
107116
if (
108117
timeout !== undefined &&
109118
typeof timeout === "object" &&
110119
timeout !== null
111120
) {
112121
nextProperties["timeout"] = {
113122
...(timeout as Record<string, unknown>),
114-
description: `Timeout in milliseconds (default: ${advertisedDefault})`,
123+
description: `Timeout in milliseconds (foreground default: ${advertisedDefault}; omit on background:true for no timeout)`,
115124
default: advertisedDefault,
116125
};
117126
}
@@ -127,7 +136,8 @@ export function advertiseShellGuardTimeout(
127136
description:
128137
"Set true to run without holding the turn open (prefer this for builds, test suites, and dev servers). " +
129138
"Returns a shell_id immediately; the exit status and output are delivered when the process finishes. " +
130-
"Use shell_collect to collect or cancel. Does not change the retained shell cwd.",
139+
"Use shell_collect to collect or cancel. Omit timeout for no timeout. " +
140+
"Does not change the retained shell cwd.",
131141
};
132142
return {
133143
...definition,

‎src/subagent/run.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -709,11 +709,15 @@ async function runSubAgentInner(
709709
: runController.signal;
710710

711711
try {
712-
const shellDefaultMs = params.shellTimeout?.defaultMs;
712+
const shellTimeout = params.shellTimeout;
713713
let tools = fromToolRunner(posixTools).map((tool) => ({
714714
...tool,
715715
definition: advertiseEditFileLineRange(
716-
advertiseShellGuardTimeout(tool.definition, shellDefaultMs),
716+
advertiseShellGuardTimeout(
717+
tool.definition,
718+
shellTimeout?.defaultMs,
719+
shellTimeout?.maxMs,
720+
),
717721
),
718722
}));
719723

0 commit comments

Comments
 (0)