Skip to content

Commit 2b35df5

Browse files
rsalusclaudegithub-actions[bot]
authored
fix: rename agent_spec.format → outputFormat + guard registration collisions (#1127) (#1144)
* fix: rename agent_spec.format → outputFormat + guard registration collisions (#1127) The flat composite registration in buildRegistrationSchema used a silent "first wins" merge across per-action fields. agent_spec.format (enum full|prompt-only, default full) was registered before doctor.format and init.format (enum table|json), so the SDK-level schema injected the wrong default into every orchestrate call and rejected doctor/init's valid format values at the MCP boundary. Parity tests missed it because they dispatch() directly, bypassing the registered SDK schema. Rename agent_spec.format → outputFormat to eliminate the current collision. Add a narrow collision-detection guard that throws at registration time when two actions declare the same field with incompatible base types, different enum value sets, or different defaults — catching the class of bug structurally. Constraint drift (min/max/optionality) remains first-wins since handler-level schemas re-validate. New regression tests exercise the real exarchos_orchestrate registry through the registration schema, confirming doctor({}), doctor({format:'json'}), init({nonInteractive:true}), and agent_spec({outputFormat:...}) are all reachable end-to-end. Closes #1127. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(mcp): classify ZodLiteral/NativeEnum/Union as enum for collision detection Addresses CodeRabbit feedback on PR #1144: the contract classifier treated ZodLiteral, ZodNativeEnum, and ZodUnion as 'other', which meant describeContractConflict couldn't spot mismatches between two actions declaring the same field with incompatible literal values, native-enum sets, or union-of-literals sets — exactly the #1127-class hazard the registration-time guard is meant to catch. Teach extractEnumValues to pull values from: * ZodLiteral (1-member enum; JSON-stringified for non-string values) * ZodNativeEnum (TS enum values, dedup-stringified) * ZodUnion whose branches are all ZodLiteral Heterogeneous unions (e.g. z.union([z.string(), z.array(z.string())])) still fall through to baseKind, preserving current behavior — only the enum-ish cases get new scrutiny. Also adds the regression tests CodeRabbit called out: * default-only collision ('full' vs 'json' on same base type) * literal-valued field with different values * union-of-literals with divergent value sets Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 parent 9b24427 commit 2b35df5

6 files changed

Lines changed: 347 additions & 21 deletions

File tree

documentation/reference/tools/orchestrate.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -360,15 +360,15 @@ Retrieve an agent specification for subagent dispatch. Returns the agent's syste
360360
{
361361
"action": "agent_spec",
362362
"agent": "implementer",
363-
"format": "full"
363+
"outputFormat": "full"
364364
}
365365
```
366366

367367
| Parameter | Required | Type | Description |
368368
|-----------|----------|------|-------------|
369369
| `agent` | yes | string (enum) | Agent identifier from the registered spec list |
370370
| `context` | no | object | Key-value pairs for template variable interpolation in prompts |
371-
| `format` | no | `"full"` \| `"prompt-only"` (default: `"full"`) | `full` returns the complete spec; `prompt-only` returns just the system prompt |
371+
| `outputFormat` | no | `"full"` \| `"prompt-only"` (default: `"full"`) | `full` returns the complete spec; `prompt-only` returns just the system prompt. Renamed from `format` in #1127 to avoid a registration collision with the `format: "table" \| "json"` parameter on `doctor` and `init`. |
372372

373373
Phases: all. Role: `any`.
374374

servers/exarchos-mcp/src/agents/handler.test.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { handleAgentSpec, agentSpecSchema } from './handler.js';
66
describe('handleAgentSpec', () => {
77
it('AgentSpec_ValidAgent_ReturnsFullSpec', async () => {
88
// Arrange
9-
const args = { agent: 'implementer' as const, format: 'full' as const };
9+
const args = { agent: 'implementer' as const, outputFormat: 'full' as const };
1010

1111
// Act
1212
const result = await handleAgentSpec(args);
@@ -62,7 +62,7 @@ describe('handleAgentSpec', () => {
6262
requirements: 'Must validate email format',
6363
filePaths: 'src/login.ts, src/login.test.ts',
6464
},
65-
format: 'full' as const,
65+
outputFormat: 'full' as const,
6666
};
6767

6868
// Act
@@ -87,7 +87,7 @@ describe('handleAgentSpec', () => {
8787
context: {
8888
taskDescription: 'Build it',
8989
},
90-
format: 'full' as const,
90+
outputFormat: 'full' as const,
9191
};
9292

9393
// Act
@@ -112,7 +112,7 @@ describe('handleAgentSpec', () => {
112112
reviewScope: 'PR #42',
113113
designRequirements: 'DR-1: Must have tests',
114114
},
115-
format: 'prompt-only' as const,
115+
outputFormat: 'prompt-only' as const,
116116
};
117117

118118
// Act
@@ -137,29 +137,29 @@ describe('handleAgentSpec', () => {
137137
});
138138

139139
describe('agentSpecSchema', () => {
140-
it('should accept valid full format', () => {
140+
it('should accept valid full outputFormat', () => {
141141
const result = agentSpecSchema.safeParse({
142142
agent: 'implementer',
143-
format: 'full',
143+
outputFormat: 'full',
144144
});
145145
expect(result.success).toBe(true);
146146
});
147147

148-
it('should accept valid prompt-only format', () => {
148+
it('should accept valid prompt-only outputFormat', () => {
149149
const result = agentSpecSchema.safeParse({
150150
agent: 'reviewer',
151-
format: 'prompt-only',
151+
outputFormat: 'prompt-only',
152152
});
153153
expect(result.success).toBe(true);
154154
});
155155

156-
it('should default format to full', () => {
156+
it('should default outputFormat to full', () => {
157157
const result = agentSpecSchema.safeParse({
158158
agent: 'fixer',
159159
});
160160
expect(result.success).toBe(true);
161161
if (result.success) {
162-
expect(result.data.format).toBe('full');
162+
expect(result.data.outputFormat).toBe('full');
163163
}
164164
});
165165

servers/exarchos-mcp/src/agents/handler.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ const AGENT_IDS = ALL_AGENT_SPECS.map(s => s.id) as [string, ...string[]];
1616
export const agentSpecSchema = z.object({
1717
agent: z.enum(AGENT_IDS),
1818
context: z.record(z.string(), z.string()).optional(),
19-
format: z.enum(['full', 'prompt-only']).default('full'),
19+
outputFormat: z.enum(['full', 'prompt-only']).default('full'),
2020
});
2121

2222
type AgentSpecArgs = z.infer<typeof agentSpecSchema>;
@@ -52,7 +52,7 @@ function interpolatePrompt(
5252
// ─── Handler ────────────────────────────────────────────────────────────────
5353

5454
export async function handleAgentSpec(args: AgentSpecArgs): Promise<ToolResult> {
55-
const { agent, context = {}, format = 'full' } = args;
55+
const { agent, context = {}, outputFormat = 'full' } = args;
5656

5757
// Find spec by agent ID
5858
const spec: AgentSpec | undefined = ALL_AGENT_SPECS.find(s => s.id === agent);
@@ -72,7 +72,7 @@ export async function handleAgentSpec(args: AgentSpecArgs): Promise<ToolResult>
7272
const { systemPrompt, unresolvedVars } = interpolatePrompt(spec.systemPrompt, context);
7373

7474
// Format: prompt-only
75-
if (format === 'prompt-only') {
75+
if (outputFormat === 'prompt-only') {
7676
return {
7777
success: true,
7878
data: {

servers/exarchos-mcp/src/orchestrate/composite.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,7 @@ describe('handleOrchestrate', () => {
419419
const args = {
420420
action: 'agent_spec',
421421
agent: 'implementer',
422-
format: 'full',
422+
outputFormat: 'full',
423423
};
424424

425425
// Act
@@ -428,7 +428,7 @@ describe('handleOrchestrate', () => {
428428
// Assert
429429
expect(result).toBe(expected);
430430
expect(handleAgentSpec).toHaveBeenCalledWith(
431-
{ agent: 'implementer', format: 'full' },
431+
{ agent: 'implementer', outputFormat: 'full' },
432432
STATE_DIR,
433433
);
434434
});

servers/exarchos-mcp/src/registry.test.ts

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,188 @@ describe('buildRegistrationSchema', () => {
113113
const schema = buildRegistrationSchema(testActions);
114114
expect(schema).toBeInstanceOf(z.ZodObject);
115115
});
116+
117+
// ─── Collision-detection guard (regression for #1127) ─────────────────────
118+
119+
it('should throw when two actions declare the same field with incompatible enums', () => {
120+
const colliding: readonly ToolAction[] = [
121+
{
122+
name: 'first',
123+
description: 'First action',
124+
schema: z.object({ format: z.enum(['full', 'prompt-only']).default('full') }),
125+
phases: new Set(['ideate']),
126+
roles: new Set(['any']),
127+
},
128+
{
129+
name: 'second',
130+
description: 'Second action',
131+
schema: z.object({ format: z.enum(['table', 'json']).optional() }),
132+
phases: new Set(['ideate']),
133+
roles: new Set(['any']),
134+
},
135+
];
136+
137+
expect(() => buildRegistrationSchema(colliding)).toThrow(/collides/);
138+
expect(() => buildRegistrationSchema(colliding)).toThrow(/first|second/);
139+
});
140+
141+
it('should throw when two actions declare the same field with incompatible base types', () => {
142+
const colliding: readonly ToolAction[] = [
143+
{
144+
name: 'a',
145+
description: 'A',
146+
schema: z.object({ limit: z.number().int() }),
147+
phases: new Set(['ideate']),
148+
roles: new Set(['any']),
149+
},
150+
{
151+
name: 'b',
152+
description: 'B',
153+
schema: z.object({ limit: z.string() }),
154+
phases: new Set(['ideate']),
155+
roles: new Set(['any']),
156+
},
157+
];
158+
159+
expect(() => buildRegistrationSchema(colliding)).toThrow(/collides/);
160+
});
161+
162+
it('should throw when two actions share a field whose defaults differ', () => {
163+
// Guards the "defaults diverge" arm of describeContractConflict: same
164+
// base type (string), no enum, but mismatched defaults would otherwise
165+
// let the first declaration silently shadow the second at the
166+
// registration boundary.
167+
const colliding: readonly ToolAction[] = [
168+
{
169+
name: 'first',
170+
description: 'First action',
171+
schema: z.object({ mode: z.string().default('full') }),
172+
phases: new Set(['ideate']),
173+
roles: new Set(['any']),
174+
},
175+
{
176+
name: 'second',
177+
description: 'Second action',
178+
schema: z.object({ mode: z.string().default('json') }),
179+
phases: new Set(['ideate']),
180+
roles: new Set(['any']),
181+
},
182+
];
183+
184+
expect(() => buildRegistrationSchema(colliding)).toThrow(/collides/);
185+
expect(() => buildRegistrationSchema(colliding)).toThrow(/Default values differ/);
186+
});
187+
188+
it('should throw when two actions share a literal-valued field with different values', () => {
189+
// Regression: before this fix, z.literal was classified as 'other' and
190+
// defaults=none on both sides silently passed — two actions could bind
191+
// the same field to incompatible literal values without detection.
192+
const colliding: readonly ToolAction[] = [
193+
{
194+
name: 'first',
195+
description: 'First',
196+
schema: z.object({ tag: z.literal('alpha') }),
197+
phases: new Set(['ideate']),
198+
roles: new Set(['any']),
199+
},
200+
{
201+
name: 'second',
202+
description: 'Second',
203+
schema: z.object({ tag: z.literal('beta') }),
204+
phases: new Set(['ideate']),
205+
roles: new Set(['any']),
206+
},
207+
];
208+
209+
expect(() => buildRegistrationSchema(colliding)).toThrow(/collides/);
210+
});
211+
212+
it('should throw when a union-of-literals field diverges across actions', () => {
213+
// Union-of-literals is the hand-rolled form of z.enum(). Same contract
214+
// semantics must apply: mismatched value sets must collide.
215+
const colliding: readonly ToolAction[] = [
216+
{
217+
name: 'first',
218+
description: 'First',
219+
schema: z.object({
220+
mode: z.union([z.literal('a'), z.literal('b')]),
221+
}),
222+
phases: new Set(['ideate']),
223+
roles: new Set(['any']),
224+
},
225+
{
226+
name: 'second',
227+
description: 'Second',
228+
schema: z.object({
229+
mode: z.union([z.literal('a'), z.literal('c')]),
230+
}),
231+
phases: new Set(['ideate']),
232+
roles: new Set(['any']),
233+
},
234+
];
235+
236+
expect(() => buildRegistrationSchema(colliding)).toThrow(/collides/);
237+
});
238+
239+
it('should allow two actions to share a field when their schemas are structurally identical', () => {
240+
const compatible: readonly ToolAction[] = [
241+
{
242+
name: 'create_pr',
243+
description: 'Create',
244+
schema: z.object({ prId: z.string().min(1) }),
245+
phases: new Set(['ideate']),
246+
roles: new Set(['any']),
247+
},
248+
{
249+
name: 'merge_pr',
250+
description: 'Merge',
251+
schema: z.object({ prId: z.string().min(1) }),
252+
phases: new Set(['ideate']),
253+
roles: new Set(['any']),
254+
},
255+
];
256+
257+
expect(() => buildRegistrationSchema(compatible)).not.toThrow();
258+
});
259+
260+
it('should not collide on format across the real orchestrate registry (#1127 regression)', () => {
261+
const orchestrate = TOOL_REGISTRY.find((t) => t.name === 'exarchos_orchestrate')!;
262+
expect(() => buildRegistrationSchema(orchestrate.actions)).not.toThrow();
263+
});
264+
265+
it('should accept doctor format values against the real orchestrate registration schema', () => {
266+
const orchestrate = TOOL_REGISTRY.find((t) => t.name === 'exarchos_orchestrate')!;
267+
const schema = buildRegistrationSchema(orchestrate.actions);
268+
269+
// Regression for #1127: before the fix, agent_spec.format (full|prompt-only)
270+
// shadowed doctor/init.format (table|json), making these payloads fail
271+
// validation at the registered-tool boundary.
272+
expect(schema.safeParse({ action: 'doctor' }).success).toBe(true);
273+
expect(schema.safeParse({ action: 'doctor', format: 'json' }).success).toBe(true);
274+
expect(schema.safeParse({ action: 'doctor', format: 'table' }).success).toBe(true);
275+
expect(schema.safeParse({ action: 'init', nonInteractive: true }).success).toBe(true);
276+
expect(schema.safeParse({ action: 'init', format: 'json' }).success).toBe(true);
277+
});
278+
279+
it('should expose agent_spec outputFormat on the real orchestrate registration schema', () => {
280+
const orchestrate = TOOL_REGISTRY.find((t) => t.name === 'exarchos_orchestrate')!;
281+
const schema = buildRegistrationSchema(orchestrate.actions);
282+
283+
expect(
284+
schema.safeParse({
285+
action: 'agent_spec',
286+
agent: 'implementer',
287+
outputFormat: 'full',
288+
}).success,
289+
).toBe(true);
290+
expect(
291+
schema.safeParse({
292+
action: 'agent_spec',
293+
agent: 'implementer',
294+
outputFormat: 'prompt-only',
295+
}).success,
296+
).toBe(true);
297+
});
116298
});
117299

118300
// ─── Type Coercion Tests ─────────────────────────────────────────────────────

0 commit comments

Comments
 (0)