Skip to content

Commit 8be41a1

Browse files
committed
fix(agent): treat gpt shell as advertised
The worker prefix lists shell, but the call gate only matched the default bash wire, so a shell call promoted run_shell. An exclude of manage_tasks now denies both update_plan and the engine id; shell and run_shell follow the same rule.
1 parent acc4bed commit 8be41a1

5 files changed

Lines changed: 131 additions & 35 deletions

File tree

‎src/agent/tool-aliases.ts‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -106,16 +106,19 @@ export function enginesForAdvertisedWire(
106106
return table.get(wire) ?? table.get(wire.toLowerCase());
107107
}
108108

109+
/** Wire this profile advertises for `engine`, including a fold that replaces it. */
110+
function profileWireName(engine: string, profile: ToolProfile): string {
111+
const fold = PROFILE_FOLDS[profile];
112+
if (fold !== undefined && fold.engines.includes(engine)) return fold.wire;
113+
return advertisedToolName(engine, profile);
114+
}
115+
109116
/** Every wire any profile advertises for `engine`, including fold wires. */
110117
export function advertisedNamesForEngine(engine: string): readonly string[] {
111118
const names: string[] = [];
112119
const seen = new Set<string>();
113120
for (const profile of toolProfiles()) {
114-
const fold = PROFILE_FOLDS[profile];
115-
const wire =
116-
fold !== undefined && fold.engines.includes(engine)
117-
? fold.wire
118-
: advertisedToolName(engine, profile);
121+
const wire = profileWireName(engine, profile);
119122
if (seen.has(wire)) continue;
120123
seen.add(wire);
121124
names.push(wire);
@@ -205,16 +208,23 @@ export function advertisedToolName(
205208
}
206209

207210
/** True when `name` (wire, engine, or hidden alias) is covered by an
208-
* advertised or activated listing (which may store wire or engine ids). */
211+
* advertised or activated listing (which may store wire or engine ids).
212+
* A call is advertised when this profile's wire is on the list, and also
213+
* when the default wire is — primary prefixes still store `bash` while a
214+
* gpt worker prefix stores `shell`. */
209215
export function nameMatchesAdvertisedListing(
210216
name: string,
211217
isListed: (candidate: string) => boolean,
218+
profile: ToolProfile = "default",
212219
): boolean {
213220
if (isListed(name)) return true;
214221
const engine = engineToolName(name);
215222
if (engine !== name && isListed(engine)) return true;
216-
const wire = advertisedToolName(engine);
217-
return wire !== name && isListed(wire);
223+
const wires =
224+
profile === "default"
225+
? [profileWireName(engine, profile)]
226+
: [profileWireName(engine, profile), profileWireName(engine, "default")];
227+
return wires.some((wire) => wire !== name && isListed(wire));
218228
}
219229

220230
// update_plan dispatches through translateUpdatePlanArgs, so advertising

‎src/agent/tool-search.test.ts‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1051,6 +1051,62 @@ describe("advertisedToolNamesForWorker", () => {
10511051
expect(build).not.toContain("write_file");
10521052
});
10531053

1054+
test("gpt shell is advertised, so a shell call does not promote run_shell", async () => {
1055+
const prefix = advertisedToolNamesForWorker({
1056+
allow: BUILD_TOOLS,
1057+
profile: "gpt",
1058+
});
1059+
const runner = createDynamicToolRunner([
1060+
{
1061+
kind: "string",
1062+
definition: {
1063+
name: "run_shell",
1064+
description: "run a shell command",
1065+
inputSchema: { type: "object", properties: {}, required: [] },
1066+
},
1067+
handler: async () => "ran:shell",
1068+
},
1069+
{
1070+
kind: "string",
1071+
definition: {
1072+
name: "apply_patch",
1073+
description: "apply a patch",
1074+
inputSchema: { type: "object", properties: {}, required: [] },
1075+
},
1076+
handler: async () => "ran:patch",
1077+
},
1078+
]);
1079+
const advertised = createAdvertisedToolset({
1080+
sessionMode: "orchestrator",
1081+
toolAvailability: { languageServerAvailable: false },
1082+
getProvider: () => ({ providerName: "openai", model: "gpt-5-codex" }),
1083+
builtInPrefix: prefix,
1084+
});
1085+
expect(advertised.isAdvertised("shell")).toBe(true);
1086+
expect(advertised.isAdvertised("run_shell")).toBe(true);
1087+
expect(advertised.isAdvertised("apply_patch")).toBe(true);
1088+
const promoted: string[] = [];
1089+
runner.setCallGate((name) => advertised.isAdvertised(name));
1090+
runner.setOnUndeclaredCall((name) => {
1091+
promoted.push(name);
1092+
advertised.activated.activate([name]);
1093+
});
1094+
const signal = new AbortController().signal;
1095+
const shell = await runner.run(
1096+
{ id: "shell", name: "shell", arguments: { command: "true" } },
1097+
signal,
1098+
);
1099+
const patch = await runner.run(
1100+
{ id: "patch", name: "apply_patch", arguments: {} },
1101+
signal,
1102+
);
1103+
expect(shell.content).toBe("ran:shell");
1104+
expect(shell.isError).toBeUndefined();
1105+
expect(patch.content).toBe("ran:patch");
1106+
expect(patch.isError).toBeUndefined();
1107+
expect(promoted).toEqual([]);
1108+
});
1109+
10541110
test("every non-default profile projects allowlisted engines through its wire table", () => {
10551111
const allow = BUILD_TOOLS as readonly string[];
10561112
for (const profile of toolProfiles()) {

‎src/session/assemble-runtime.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -452,17 +452,17 @@ export function createAdvertisedToolset(args: {
452452
);
453453
};
454454
const isAdvertised = (name: string): boolean => {
455-
const denied = deniedFor(args.getProvider());
455+
const provider = args.getProvider();
456+
const profile = toolProfileForModel(provider);
457+
const denied = deniedFor(provider);
456458
if (denied.includes(name) || denied.includes(canonicalToolName(name))) {
457459
return false;
458460
}
459-
const effective = foldFileToolNames(
460-
prefix,
461-
toolProfileForModel(args.getProvider()),
462-
);
461+
const effective = foldFileToolNames(prefix, profile);
463462
return nameMatchesAdvertisedListing(
464463
name,
465464
(n) => effective.includes(n) || activated.has(n),
465+
profile,
466466
);
467467
};
468468
const flushPromotions = (): boolean => {

‎src/subagent/capability-preflight.test.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,40 @@ describe("preflightCapabilities", () => {
364364
expect(result.unavailable.code).toBe("unknown_tool");
365365
expect(result.unavailable.tool).toBe("bash");
366366
});
367+
368+
test("gpt exclude denies the wire and the engine together", () => {
369+
const cases = [
370+
{ exclude: "manage_tasks", required: ["update_plan", "manage_tasks"] },
371+
{ exclude: "update_plan", required: ["update_plan", "manage_tasks"] },
372+
{ exclude: "run_shell", required: ["shell", "run_shell"] },
373+
{ exclude: "shell", required: ["shell", "run_shell"] },
374+
] as const;
375+
for (const entry of cases) {
376+
for (const required of entry.required) {
377+
const result = preflightCapabilities({
378+
required: [required],
379+
resolvedFilter: { mode: "exclude", tools: [entry.exclude] },
380+
knownEngines: DEFAULT_KNOWN_ENGINES,
381+
agentLabel: "coder",
382+
toolProfile: "gpt",
383+
});
384+
const label = `${entry.exclude} blocks ${required}`;
385+
expect(result.ok, label).toBe(false);
386+
if (result.ok) continue;
387+
expect(result.unavailable.code, label).toBe("permission_static");
388+
}
389+
}
390+
for (const required of ["manage_tasks", "update_plan"] as const) {
391+
const result = preflightCapabilities({
392+
required: [required],
393+
resolvedFilter: { mode: "exclude", tools: ["read_file"] },
394+
knownEngines: DEFAULT_KNOWN_ENGINES,
395+
agentLabel: "coder",
396+
toolProfile: "gpt",
397+
});
398+
expect(result.ok, required).toBe(true);
399+
}
400+
});
367401
});
368402

369403
describe("formatCapabilityUnavailable", () => {

‎src/subagent/capability-preflight.ts‎

Lines changed: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,9 @@ function enginesCoveredByDirector(
245245
(engine) => named.has(engine) || postFilter.has(engine),
246246
);
247247
}
248+
// An explicit exclude names the engine. Post-filter mounts (manage_tasks)
249+
// are not exempt here — update_plan and manage_tasks must agree, and the
250+
// engine-id path applies the same denylist.
248251
return engines.filter((engine) => !named.has(engine));
249252
}
250253

@@ -353,6 +356,21 @@ export function preflightCapabilities(
353356
unavailable: { code: "missing_binary", tool: resolved },
354357
};
355358
}
359+
// Explicit exclude wins over the post-filter mount exemption and over a
360+
// live MCP mount. Otherwise gpt update_plan (wire path) is
361+
// permission_static while manage_tasks (engine-id path) still passes.
362+
// shell / run_shell share that rule. run.ts strips named tools in
363+
// exclude mode, so an excluded MCP tool must reject here.
364+
if (deny !== undefined && deny.has(resolved)) {
365+
return {
366+
ok: false,
367+
unavailable: {
368+
code: "permission_static",
369+
tool: resolved,
370+
alternatives: rerouteAlternatives(resolved),
371+
},
372+
};
373+
}
356374
if (!postFilter.has(resolved) && !isLiveMcp) {
357375
if (allow !== undefined && !allow.has(resolved)) {
358376
return {
@@ -364,28 +382,6 @@ export function preflightCapabilities(
364382
},
365383
};
366384
}
367-
if (deny !== undefined && deny.has(resolved)) {
368-
return {
369-
ok: false,
370-
unavailable: {
371-
code: "permission_static",
372-
tool: resolved,
373-
alternatives: rerouteAlternatives(resolved),
374-
},
375-
};
376-
}
377-
} else if (isLiveMcp && deny !== undefined && deny.has(resolved)) {
378-
// An explicit exclude naming a live MCP tool still withholds it —
379-
// run.ts strips named tools in exclude mode, so the mount would drop
380-
// it and the requirement must reject here, not as a stale snapshot.
381-
return {
382-
ok: false,
383-
unavailable: {
384-
code: "permission_static",
385-
tool: resolved,
386-
alternatives: rerouteAlternatives(resolved),
387-
},
388-
};
389385
}
390386
if (!seen.has(resolved)) {
391387
seen.add(resolved);

0 commit comments

Comments
 (0)