Skip to content

Commit 4bb8328

Browse files
committed
fix(subagent): reconcile refreshed catalogs with the policy and harden the policy write path
- refreshProviderModels clamps a [secondary_model] binding or pool entry whose model vanished from the refreshed catalog, so the discovery service no longer rejects the whole provider patch as CONFIG_INVALID - policy validation names [secondary_model].default_model when that field is the one that fails to resolve - SubagentModelPolicyService serializes commits so the If-Match version check and the write run as one transition - POST /config accepts the legacy secondary_model metadata echoed by GET and drops it on write instead of rejecting the round trip - policy PUT/DELETE report only the mutating call as a validation failure - discovery test stub validates the legacy section through the schema
1 parent 9ee3308 commit 4bb8328

10 files changed

Lines changed: 140 additions & 18 deletions

File tree

packages/agent-core-v2/src/session/subagent/policy.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -183,21 +183,25 @@ function assertModelResolves(
183183
field: string,
184184
context: SubagentPolicyValidationContext,
185185
): SubagentPolicyModelInfo {
186+
const label =
187+
field === 'models'
188+
? `[secondary_model.models] entry "${alias}"`
189+
: `[secondary_model].default_model "${alias}"`;
186190
let info: SubagentPolicyModelInfo | undefined;
187191
try {
188192
info = context.resolveModel(alias);
189193
} catch (error) {
190194
throw new Error2(
191195
ErrorCodes.CONFIG_INVALID,
192-
`[secondary_model.models] entry "${alias}" could not be resolved: ${error instanceof Error ? error.message : String(error)}`,
196+
`${label} could not be resolved: ${error instanceof Error ? error.message : String(error)}`,
193197
{ cause: error, details: { section: SECONDARY_MODEL_SECTION, field, model: alias } },
194198
);
195199
}
196200
if (info === undefined) {
197-
throw invalid(
198-
`[secondary_model.models] entry "${alias}" could not be resolved: Model "${alias}" is not configured in config.toml.`,
199-
{ field, model: alias },
200-
);
201+
throw invalid(`${label} could not be resolved: Model "${alias}" is not configured in config.toml.`, {
202+
field,
203+
model: alias,
204+
});
201205
}
202206
return info;
203207
}

packages/agent-core-v2/src/session/subagent/subagentModelPolicyService.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ import {
3434
export class SubagentModelPolicyService implements ISubagentModelPolicyService {
3535
declare readonly _serviceBrand: undefined;
3636

37+
private commitChain: Promise<unknown> = Promise.resolve();
38+
3739
constructor(
3840
@IConfigService private readonly config: IConfigService,
3941
@IFlagService private readonly flags: IFlagService,
@@ -148,7 +150,19 @@ export class SubagentModelPolicyService implements ISubagentModelPolicyService {
148150
}
149151
}
150152

151-
private async commit(
153+
private commit(
154+
section: LegacySecondaryModelConfig | undefined,
155+
expectedVersion: string | undefined,
156+
): Promise<void> {
157+
const run = this.commitChain.then(
158+
() => this.commitNow(section, expectedVersion),
159+
() => this.commitNow(section, expectedVersion),
160+
);
161+
this.commitChain = run.catch(() => undefined);
162+
return run;
163+
}
164+
165+
private async commitNow(
152166
section: LegacySecondaryModelConfig | undefined,
153167
expectedVersion: string | undefined,
154168
): Promise<void> {

packages/agent-core-v2/test/app/kosongConfig/discovery.test.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { IAgentIdentity } from '#/app/agentIdentity/agentIdentity';
77
import { IBootstrapService } from '#/app/bootstrap/bootstrap';
88
import { IConfigService } from '#/app/config/config';
99
import {
10+
LegacySecondaryModelConfigSchema,
1011
normalizeLegacySecondaryModel,
1112
toPersistedSecondaryModel,
1213
} from '#/session/subagent/policy';
@@ -71,9 +72,7 @@ function stubLogService(): ILogService {
7172
function stubSubagentModelPolicy(): ISubagentModelPolicyService {
7273
const prepare = (input: unknown): PreparedSubagentPolicyMutation => {
7374
const policy = normalizeLegacySecondaryModel(
74-
input === null || input === undefined
75-
? undefined
76-
: (input as Parameters<typeof normalizeLegacySecondaryModel>[0]),
75+
input === null || input === undefined ? undefined : LegacySecondaryModelConfigSchema.parse(input),
7776
);
7877
return { policy, section: toPersistedSecondaryModel(policy) };
7978
};

packages/agent-core-v2/test/session/subagent/subagentModelPolicyService.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,26 @@ describe('SubagentModelPolicyService', () => {
125125
expect(await codeOf(() => service.clear(before))).toBe(ErrorCodes.CONFIG_VERSION_CONFLICT);
126126
});
127127

128+
it('serializes concurrent commits so a stale expectedVersion cannot slip past the version check', async () => {
129+
const service = setup({ [SECONDARY_MODEL_SECTION]: { defaultModel: 'acme/sol' } });
130+
const replace = config.replace.bind(config);
131+
config.replace = async (domain, value) => {
132+
await new Promise((resolve) => setTimeout(resolve, 0));
133+
await replace(domain, value);
134+
};
135+
const version = service.get().resourceVersion;
136+
const [first, second] = await Promise.allSettled([
137+
service.set({ mode: 'default', defaultModel: 'acme/luna' }, version),
138+
service.set({ mode: 'force', defaultModel: 'acme/sol' }, version),
139+
]);
140+
expect(first.status).toBe('fulfilled');
141+
expect(second.status).toBe('rejected');
142+
expect((second as PromiseRejectedResult).reason).toMatchObject({
143+
code: ErrorCodes.CONFIG_VERSION_CONFLICT,
144+
});
145+
expect(config.get(SECONDARY_MODEL_SECTION)).toEqual({ defaultModel: 'acme/luna', defaultEffort: undefined });
146+
});
147+
128148
it('getEffective reports inherit while the feature is disabled and keeps the configured policy', () => {
129149
const service = setup(
130150
{ [SECONDARY_MODEL_SECTION]: { defaultModel: 'acme/sol', force: true } },

packages/agent-core-v2/test/session/subagent/subagentModelsValidation.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ describe('SessionSubagentModelsValidationService', () => {
9696
expect(isError2(error)).toBe(true);
9797
expect((error as Error2).code).toBe(ErrorCodes.CONFIG_INVALID);
9898
expect((error as Error2).message).toContain(
99-
'[secondary_model.models] entry "provider/typo" could not be resolved',
99+
'[secondary_model].default_model "provider/typo" could not be resolved',
100100
);
101101
});
102102

@@ -128,8 +128,9 @@ describe('SessionSubagentModelsValidationService', () => {
128128
expect(isError2(error)).toBe(true);
129129
expect((error as Error2).code).toBe(ErrorCodes.CONFIG_INVALID);
130130
expect((error as Error2).message).toContain(
131-
'[secondary_model.models] entry "provider/typo" could not be resolved',
131+
'[secondary_model].default_model "provider/typo" could not be resolved',
132132
);
133+
expect((error as Error2).message).not.toContain('[secondary_model.models]');
133134
});
134135

135136
it('constructs fine for a valid pool', () => {

packages/agent-gateway/src/protocol/rest-config.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ export const configResponseSchema = z.object({
3434
export type ConfigResponse = z.infer<typeof configResponseSchema>;
3535

3636
const optionalModelAlias = z.string().min(1).optional();
37+
const droppedLegacyMetadata = z.unknown().optional();
3738

3839
export const legacySecondaryModelRequestSchema = z
3940
.object({
@@ -44,6 +45,23 @@ export const legacySecondaryModelRequestSchema = z
4445
defaultEffort: z.string().optional(),
4546
models: z.record(z.string(), z.string()).optional(),
4647
force: z.boolean().optional(),
48+
max_context_size: droppedLegacyMetadata,
49+
maxContextSize: droppedLegacyMetadata,
50+
max_input_size: droppedLegacyMetadata,
51+
maxInputSize: droppedLegacyMetadata,
52+
max_output_size: droppedLegacyMetadata,
53+
maxOutputSize: droppedLegacyMetadata,
54+
capabilities: droppedLegacyMetadata,
55+
display_name: droppedLegacyMetadata,
56+
displayName: droppedLegacyMetadata,
57+
reasoning_key: droppedLegacyMetadata,
58+
reasoningKey: droppedLegacyMetadata,
59+
adaptive_thinking: droppedLegacyMetadata,
60+
adaptiveThinking: droppedLegacyMetadata,
61+
support_efforts: droppedLegacyMetadata,
62+
supportEfforts: droppedLegacyMetadata,
63+
off_effort: droppedLegacyMetadata,
64+
offEffort: droppedLegacyMetadata,
4765
})
4866
.strict();
4967
export type LegacySecondaryModelRequest = z.infer<typeof legacySecondaryModelRequestSchema>;

packages/agent-gateway/src/routes/subagentModelPolicy.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -172,12 +172,13 @@ export function registerSubagentModelPolicyRoutes(app: PolicyRouteHost, core: Sc
172172
await core.accessor.get(IConfigService).ready;
173173
const expectedVersion = parseIfMatch(req.headers['if-match']);
174174
await policyService().set(fromWirePolicy(req.body), expectedVersion);
175-
publish(['secondary_model']);
176-
requestLog(req)?.info({ changedFields: ['secondary_model'] }, 'subagent model policy updated');
177-
respond(typedReply, req.id);
178175
} catch (error) {
179176
fail(req as PolicyRequest, typedReply, error);
177+
return;
180178
}
179+
publish(['secondary_model']);
180+
requestLog(req)?.info({ changedFields: ['secondary_model'] }, 'subagent model policy updated');
181+
respond(typedReply, req.id);
181182
},
182183
);
183184
app.put(putRoute.path, putRoute.options, putRoute.handler as unknown as Parameters<PolicyRouteHost['put']>[2]);
@@ -200,12 +201,13 @@ export function registerSubagentModelPolicyRoutes(app: PolicyRouteHost, core: Sc
200201
await core.accessor.get(IConfigService).ready;
201202
const expectedVersion = parseIfMatch(req.headers['if-match']);
202203
await policyService().clear(expectedVersion);
203-
publish(['secondary_model']);
204-
requestLog(req)?.info({ changedFields: ['secondary_model'] }, 'subagent model policy cleared');
205-
respond(typedReply, req.id);
206204
} catch (error) {
207205
fail(req as PolicyRequest, typedReply, error);
206+
return;
208207
}
208+
publish(['secondary_model']);
209+
requestLog(req)?.info({ changedFields: ['secondary_model'] }, 'subagent model policy cleared');
210+
respond(typedReply, req.id);
209211
},
210212
);
211213
app.delete(deleteRoute.path, deleteRoute.options, deleteRoute.handler as unknown as Parameters<PolicyRouteHost['delete']>[2]);

packages/agent-gateway/test/config.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,21 @@ describe('server-v2 /api/v1/config secondary_model replacement and request atomi
291291
expect(await diskToml()).not.toContain('force');
292292
});
293293

294+
it('accepts legacy secondary_model metadata echoed by GET and drops it on write', async () => {
295+
await boot(
296+
`${MODELS_TOML}[secondary_model]\ndefault_model = "provider/fast"\nmax_context_size = 1000\ncapabilities = ["thinking"]\n`,
297+
);
298+
const echoed = (await getConfig()).secondary_model as Record<string, unknown>;
299+
expect(echoed).toMatchObject({ defaultModel: 'provider/fast', maxContextSize: 1000 });
300+
301+
const res = await post({ secondary_model: { ...echoed, default_effort: 'low' } });
302+
expect(res.body.code).toBe(0);
303+
expect((await getConfig()).secondary_model).toEqual({
304+
defaultModel: 'provider/fast',
305+
defaultEffort: 'low',
306+
});
307+
});
308+
294309
it('pool -> default drops the models table', async () => {
295310
await boot(MODELS_TOML);
296311
await post({

packages/oauth/src/refreshProviderModels.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,28 @@ function clampDanglingDefault(config: PythinkerConfigShape): void {
319319
}
320320
}
321321

322+
// The same refresh can drop a model that `[secondary_model]` binds. A dangling
323+
// default binding clears the section (subagents inherit the caller model again);
324+
// a dangling pool entry is pruned so the rest of the pool keeps working. The
325+
// discovery service validates the section against the refreshed catalog and
326+
// would otherwise reject the whole provider patch.
327+
function clampDanglingSecondaryModel(config: PythinkerConfigShape): void {
328+
const section = config.secondaryModel;
329+
if (section === undefined) return;
330+
const bound = section.defaultModel ?? section.model;
331+
if (bound !== undefined && readModel(config, bound) === undefined) {
332+
config.secondaryModel = undefined;
333+
return;
334+
}
335+
if (section.models === undefined) return;
336+
const models = Object.fromEntries(
337+
Object.entries(section.models).filter(([alias]) => readModel(config, alias) !== undefined),
338+
);
339+
if (Object.keys(models).length !== Object.keys(section.models).length) {
340+
config.secondaryModel = { ...section, models };
341+
}
342+
}
343+
322344
function clearDefaultThinkingWhenDefaultRemoved(
323345
config: PythinkerConfigShape,
324346
previousDefaultModel: string | undefined,
@@ -433,6 +455,7 @@ export async function refreshProviderModels(
433455
preserveSecondaryModelAliases(config, next);
434456
restoreDefaultSelection(next, config.defaultModel, config.thinking?.enabled);
435457
clampDanglingDefault(next);
458+
clampDanglingSecondaryModel(next);
436459
clearDefaultThinkingWhenDefaultRemoved(next, config.defaultModel);
437460

438461
if (providerModelsEqual(config, next, providerId, refreshedAliasKeys)) {
@@ -584,6 +607,7 @@ export async function refreshProviderModels(
584607
if (changedProviders.length > 0 || hasUnreportedConfigChange) {
585608
restoreDefaultSelection(next, config.defaultModel, config.thinking?.enabled);
586609
clampDanglingDefault(next);
610+
clampDanglingSecondaryModel(next);
587611
clearDefaultThinkingWhenDefaultRemoved(next, config.defaultModel);
588612
for (const providerId of providersToRemoveBeforeSet) {
589613
await host.removeProvider(providerId);
@@ -695,6 +719,7 @@ export async function refreshProviderModels(
695719
if (changedProviders.length > 0) {
696720
restoreDefaultSelection(next, config.defaultModel, config.thinking?.enabled);
697721
clampDanglingDefault(next);
722+
clampDanglingSecondaryModel(next);
698723
clearDefaultThinkingWhenDefaultRemoved(next, config.defaultModel);
699724
for (const providerId of providersToRemoveBeforeSet) {
700725
await host.removeProvider(providerId);

packages/oauth/test/models-dev-refresh.test.ts

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,31 @@ describe('refreshProviderModels modelsDev directory providers', () => {
238238
expect(patch.providers?.[PROVIDER_ID]).toBeUndefined();
239239
expect(patch.defaultModel).toBeUndefined();
240240
expect(patch.thinking).toBeUndefined();
241-
expect(patch.secondaryModel).toEqual(base.secondaryModel);
241+
expect(patch.secondaryModel).toBeUndefined();
242+
});
243+
244+
it('prunes vanished pool entries from secondary_model but keeps the rest of the pool', async () => {
245+
vi.stubGlobal(
246+
'fetch',
247+
vi.fn(async () => jsonResponse({ 'brand-new-guy': makeDocument()['brand-new-guy'] })),
248+
);
249+
const base = makeBaseConfig();
250+
base.models = {
251+
...base.models,
252+
'other/kept': { provider: 'other', model: 'kept' },
253+
};
254+
base.secondaryModel = {
255+
defaultModel: 'other/kept',
256+
models: { 'other/kept': '', [`${PROVIDER_ID}/deepseek-v4-flash`]: 'fast' },
257+
};
258+
259+
const { host, calls } = makeHost(base);
260+
await refreshProviderModels(host);
261+
262+
expect(lastPatch(calls).secondaryModel).toEqual({
263+
defaultModel: 'other/kept',
264+
models: { 'other/kept': '' },
265+
});
242266
});
243267

244268
it('reports a failure without writing when an entry lists no usable models', async () => {

0 commit comments

Comments
 (0)