mirror of
https://github.com/garrytan/gbrain.git
synced 2026-08-14 08:53:22 +00:00
fix(minions): verify the finally-resolved model at the subagent gate — the models.subagent config path bypassed capability checks (#3919)
Wave-assembled from PR #3919 by @Masashi-Ono0611. Co-Authored-By: masashiono0611 <masashi.ono.0611@gmail.com>
This commit is contained in:
Garry Tan
committed by
Sina Matian
Sina Matian
co-authored by
masashiono0611
masashiono0611
parent
3fa0a5acb5
commit
94ec7e31e0
@@ -48,6 +48,7 @@ import {
|
||||
logSubagentHeartbeat,
|
||||
} from './subagent-audit.ts';
|
||||
import { resolveModel, isAnthropicProvider, TIER_DEFAULTS } from '../../model-config.ts';
|
||||
import { splitProviderModelId, normalizeModelId } from '../../model-id.ts';
|
||||
import { resolveAnthropicKey } from '../../ai/anthropic-key.ts';
|
||||
import { buildSystemPrompt, DEFAULT_SUBAGENT_SYSTEM } from '../system-prompt.ts';
|
||||
import { toolLoop as gatewayToolLoop } from '../../ai/gateway.ts';
|
||||
@@ -211,31 +212,79 @@ export function makeSubagentHandler(deps: SubagentDeps) {
|
||||
// Default is the legacy path so v0.38 patch releases ship the same
|
||||
// behavior as v0.37. Users dogfood the gateway path by flipping the flag.
|
||||
//
|
||||
// Refuse-at-handler-entry when the model literally lacks tool calling
|
||||
// OR is from an unknown provider. The queue.ts gate already catches this
|
||||
// for queue-submitted jobs; the check here covers direct `gbrain agent run`
|
||||
// invocations and any code path that bypasses the queue's capability check.
|
||||
if (data.model) {
|
||||
const verdict = classifyCapabilities(data.model);
|
||||
if (verdict === 'unusable:no_tools') {
|
||||
throw new Error(
|
||||
`subagent job rejected: data.model "${data.model}" lacks native tool calling. ` +
|
||||
`The subagent loop dispatches brain ops via tool calls — without tool support the loop has no way to run.`,
|
||||
);
|
||||
}
|
||||
if (verdict === 'unknown') {
|
||||
throw new Error(
|
||||
`subagent job rejected: data.model "${data.model}" references an unknown provider. ` +
|
||||
`Use format provider:model where provider matches a recipe in src/core/ai/recipes/.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
const model = data.model
|
||||
?? await resolveModel(engine, {
|
||||
tier: 'subagent',
|
||||
configKey: 'models.subagent',
|
||||
fallback: TIER_DEFAULTS.subagent,
|
||||
});
|
||||
|
||||
// Refuse-at-handler-entry on the FINAL resolved model, not just an
|
||||
// explicit data.model: `models.subagent` config resolves through
|
||||
// resolveModel's explicit-key branch, which does NOT run
|
||||
// enforceSubagentCapable's silent fallback (only the inherited
|
||||
// models.default / tier / env branches do). An explicitly chosen
|
||||
// tool-incapable model must be refused loudly here rather than silently
|
||||
// run a loop that has no way to dispatch tools.
|
||||
// The queue.ts gate already catches explicit data.model at submit; this
|
||||
// check additionally covers config-resolved models, direct `gbrain agent
|
||||
// run` invocations, and any path that bypasses the queue's check.
|
||||
//
|
||||
// Exception (#1151 precedent): a replayed job whose last persisted
|
||||
// message is a terminal assistant turn (no tool_use blocks) needs NO
|
||||
// further provider call — the path-specific replay logic below returns
|
||||
// the already-committed result. Don't let a capability refusal (e.g.
|
||||
// config repointed to a tool-incapable model between submit and replay)
|
||||
// dead-letter completed work.
|
||||
{
|
||||
const lastRows = await engine.executeRaw<{ role: string; content_blocks: unknown }>(
|
||||
`SELECT role, content_blocks FROM subagent_messages
|
||||
WHERE job_id = $1 ORDER BY message_idx DESC LIMIT 1`,
|
||||
[ctx.id],
|
||||
);
|
||||
const lastRow = lastRows[0];
|
||||
const lastBlocks = lastRow
|
||||
? (typeof lastRow.content_blocks === 'string'
|
||||
? JSON.parse(lastRow.content_blocks)
|
||||
: lastRow.content_blocks) as Array<{ type?: string; text?: unknown }>
|
||||
: null;
|
||||
// Terminal = assistant turn with NO pending tool dispatch in EITHER
|
||||
// block vocabulary (legacy Anthropic `tool_use`, gateway `tool-call`)
|
||||
// AND actual text (the gateway terminal rule) — a pending-tool replay
|
||||
// must still pass the gate before the loop resumes on the provider.
|
||||
const alreadyTerminal = lastRow?.role === 'assistant'
|
||||
&& Array.isArray(lastBlocks)
|
||||
&& !lastBlocks.some(b => b?.type === 'tool_use' || b?.type === 'tool-call')
|
||||
&& lastBlocks.some(b => b?.type === 'text' && typeof b.text === 'string' && b.text.trim() !== '');
|
||||
const modelSource = data.model ? 'data.model' : 'the resolved subagent model config';
|
||||
// A BARE Anthropic id (`claude-sonnet-4-6`, no `provider:` prefix) is a
|
||||
// supported value everywhere else: isAnthropicProvider() has an explicit
|
||||
// bare-`claude-` branch, the legacy path strips the prefix before calling
|
||||
// the Messages API, and this handler's own DEFAULT_MODEL is bare.
|
||||
// classifyCapabilities() resolves through the recipe registry, which
|
||||
// requires an explicit provider, so classifying the raw value would
|
||||
// report `unknown` and refuse a working config. Normalize first — the
|
||||
// dream cycle does the same before queueing children
|
||||
// (src/core/cycle/synthesize.ts). Narrow on purpose: only bare ids that
|
||||
// isAnthropicProvider() recognizes are normalized, so a bare
|
||||
// non-Anthropic id still classifies as `unknown` and is still refused.
|
||||
const modelForVerdict = splitProviderModelId(model).provider === null && isAnthropicProvider(model)
|
||||
? normalizeModelId(model)
|
||||
: model;
|
||||
const verdict = alreadyTerminal ? 'ok' : classifyCapabilities(modelForVerdict);
|
||||
if (verdict === 'unusable:no_tools') {
|
||||
throw new Error(
|
||||
`subagent job rejected: ${modelSource} "${model}" lacks native tool calling. ` +
|
||||
`The subagent loop dispatches brain ops via tool calls — without tool support the loop has no way to run.`,
|
||||
);
|
||||
}
|
||||
if (verdict === 'unknown') {
|
||||
throw new Error(
|
||||
`subagent job rejected: ${modelSource} "${model}" references an unknown provider. ` +
|
||||
`Use format provider:model where provider matches a recipe in src/core/ai/recipes/.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
const maxTurns = data.max_turns ?? DEFAULT_MAX_TURNS;
|
||||
// #2778: per-turn output cap — data.max_tokens → config → 8192 default.
|
||||
const maxOutputTokens = resolveMaxOutputTokens(
|
||||
|
||||
@@ -709,3 +709,135 @@ describe('subagent handler output-token cap (#2778)', () => {
|
||||
expect(texts.some(t => t.includes('truncated') && t.includes('DROPPED'))).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('handler-entry capability gate on the config-resolved model', () => {
|
||||
test('config-resolved models.subagent lacking tool calling is refused at dispatch', async () => {
|
||||
// The queue submit gate only sees explicit data.model. A job that omits
|
||||
// data.model resolves `models.subagent` inside the handler — pre-fix that
|
||||
// path bypassed the capability check entirely and a tool-incapable model
|
||||
// (declared supports_tools: false) ran the loop anyway.
|
||||
await engine.setConfig('models.subagent', 'minimax:MiniMax-M2');
|
||||
try {
|
||||
const client = new FakeMessagesClient([
|
||||
{ content: [{ type: 'text', text: 'should never run' }] as any, stop_reason: 'end_turn' },
|
||||
]);
|
||||
const handler = makeSubagentHandler({ engine, client, toolRegistry: [] });
|
||||
const ctx = await makeCtx({ prompt: 'hi' }); // no data.model → queue gate passes
|
||||
await expect(handler(ctx)).rejects.toThrow(/lacks native tool calling/);
|
||||
expect(client.calls.length).toBe(0); // refused before any provider call
|
||||
} finally {
|
||||
await engine.unsetConfig('models.subagent');
|
||||
}
|
||||
});
|
||||
|
||||
test('already-terminal replay is NOT refused when config points at a tool-incapable model', async () => {
|
||||
// #1151 precedent: a job whose last persisted message is a terminal
|
||||
// assistant turn needs no provider call — the replay short-circuit
|
||||
// returns the committed result. A capability refusal here (config
|
||||
// repointed between submit and replay) would dead-letter completed work.
|
||||
// Gateway loop ON: that's the only routing where the capability gate is
|
||||
// the deciding check (the legacy path's Anthropic pin refuses
|
||||
// non-Anthropic models regardless). The gateway path's terminal
|
||||
// early-return runs before any provider client is constructed, so no
|
||||
// API key is needed.
|
||||
await engine.setConfig('models.subagent', 'minimax:MiniMax-M2');
|
||||
await engine.setConfig('agent.use_gateway_loop', 'true');
|
||||
try {
|
||||
const client = new FakeMessagesClient([]);
|
||||
const handler = makeSubagentHandler({ engine, client, toolRegistry: [] });
|
||||
const ctx = await makeCtx({ prompt: 'hi' });
|
||||
// Persist a completed transcript: seed user + terminal assistant.
|
||||
await engine.executeRaw(
|
||||
`INSERT INTO subagent_messages (job_id, message_idx, role, content_blocks)
|
||||
VALUES ($1, 0, 'user', $2::text::jsonb), ($1, 1, 'assistant', $3::text::jsonb)`,
|
||||
[
|
||||
ctx.id,
|
||||
JSON.stringify([{ type: 'text', text: 'hi' }]),
|
||||
JSON.stringify([{ type: 'text', text: 'committed answer' }]),
|
||||
],
|
||||
);
|
||||
const result = await handler(ctx);
|
||||
expect(result.result).toBe('committed answer');
|
||||
// Replay short-circuit: no new turns persisted beyond the 2 seeded rows
|
||||
// (a provider call would have appended an assistant row — and would have
|
||||
// failed anyway, since no gateway credentials are configured here).
|
||||
const rows = await engine.executeRaw<{ count: string }>(
|
||||
`SELECT count(*)::text AS count FROM subagent_messages WHERE job_id = $1`,
|
||||
[ctx.id],
|
||||
);
|
||||
expect(parseInt(rows[0]!.count, 10)).toBe(2);
|
||||
} finally {
|
||||
await engine.unsetConfig('models.subagent');
|
||||
await engine.unsetConfig('agent.use_gateway_loop');
|
||||
}
|
||||
});
|
||||
|
||||
test('non-terminal gateway replay (pending tool-call) IS still refused on a tool-incapable model', async () => {
|
||||
// A gateway transcript persists pending dispatch as `tool-call` blocks.
|
||||
// A replay that still needs the loop to resume on the provider must NOT
|
||||
// slip past the capability gate via the terminal exception.
|
||||
await engine.setConfig('models.subagent', 'minimax:MiniMax-M2');
|
||||
await engine.setConfig('agent.use_gateway_loop', 'true');
|
||||
try {
|
||||
const client = new FakeMessagesClient([]);
|
||||
const handler = makeSubagentHandler({ engine, client, toolRegistry: [] });
|
||||
const ctx = await makeCtx({ prompt: 'hi' });
|
||||
await engine.executeRaw(
|
||||
`INSERT INTO subagent_messages (job_id, message_idx, role, content_blocks)
|
||||
VALUES ($1, 0, 'user', $2::text::jsonb), ($1, 1, 'assistant', $3::text::jsonb)`,
|
||||
[
|
||||
ctx.id,
|
||||
JSON.stringify([{ type: 'text', text: 'hi' }]),
|
||||
JSON.stringify([{ type: 'tool-call', toolCallId: 'tc_1', toolName: 'echo', input: {} }]),
|
||||
],
|
||||
);
|
||||
await expect(handler(ctx)).rejects.toThrow(/lacks native tool calling/);
|
||||
} finally {
|
||||
await engine.unsetConfig('models.subagent');
|
||||
await engine.unsetConfig('agent.use_gateway_loop');
|
||||
}
|
||||
});
|
||||
|
||||
test('a BARE Anthropic id in models.subagent still runs (not refused as unknown provider)', async () => {
|
||||
// A bare `claude-*` id (no `provider:` prefix) is a supported config value
|
||||
// everywhere else: isAnthropicProvider() has an explicit bare-`claude-`
|
||||
// branch and the legacy path strips the prefix before calling the Messages
|
||||
// API. classifyCapabilities() resolves through the recipe registry, which
|
||||
// requires an explicit provider, so classifying the raw value would report
|
||||
// `unknown` and this gate would refuse a config that works.
|
||||
await engine.setConfig('models.subagent', 'claude-sonnet-4-6');
|
||||
try {
|
||||
const client = new FakeMessagesClient([
|
||||
{ content: [{ type: 'text', text: 'ran on the bare id' }] as any, stop_reason: 'end_turn' },
|
||||
]);
|
||||
const handler = makeSubagentHandler({ engine, client, toolRegistry: [] });
|
||||
const ctx = await makeCtx({ prompt: 'hi' }); // no data.model → config resolves
|
||||
const result = await handler(ctx);
|
||||
expect(result.result).toBe('ran on the bare id');
|
||||
// The provider call carries the bare id (the legacy path strips any prefix).
|
||||
expect(client.calls.length).toBe(1);
|
||||
expect(client.calls[0]!.model).toBe('claude-sonnet-4-6');
|
||||
} finally {
|
||||
await engine.unsetConfig('models.subagent');
|
||||
}
|
||||
});
|
||||
|
||||
test('a bare NON-Anthropic id in models.subagent is still refused', async () => {
|
||||
// The normalization above is deliberately narrow: only bare ids that
|
||||
// isAnthropicProvider() recognizes get an `anthropic:` prefix for the
|
||||
// verdict. A bare `gpt-5` is not a recipe-resolvable model, so the gate
|
||||
// must keep reporting `unknown` rather than classifying it as Anthropic.
|
||||
await engine.setConfig('models.subagent', 'gpt-5');
|
||||
try {
|
||||
const client = new FakeMessagesClient([
|
||||
{ content: [{ type: 'text', text: 'should never run' }] as any, stop_reason: 'end_turn' },
|
||||
]);
|
||||
const handler = makeSubagentHandler({ engine, client, toolRegistry: [] });
|
||||
const ctx = await makeCtx({ prompt: 'hi' });
|
||||
await expect(handler(ctx)).rejects.toThrow(/references an unknown provider/);
|
||||
expect(client.calls.length).toBe(0);
|
||||
} finally {
|
||||
await engine.unsetConfig('models.subagent');
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user