Skip to content

Commit 183e563

Browse files
icebob-aiclaude
andcommitted
Fix code review issues in OrchestratorMixin
- Move _routeToAgent action from static schema to merged() hook so it is only registered when strategy === 'llm-router' - Add agentName validation in _routeToAgent handler: throws if agent is not in discoverAgents() list - Add description field to _routeToAgent action definition - Add integration test for unknown agent name rejection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 795ae19 commit 183e563

2 files changed

Lines changed: 126 additions & 13 deletions

File tree

src/orchestrator.mixin.ts

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,36 @@ export default function OrchestratorMixin(_mixinOpts?: OrchestratorMixinOptions)
1919
_mixinOpts = _.defaultsDeep(_mixinOpts, {});
2020

2121
const schema = {
22+
merged(this: ServiceInstance, schema: Record<string, unknown>) {
23+
const settings = schema.settings as Record<string, unknown> | undefined;
24+
const agent = settings?.agent as Record<string, unknown> | undefined;
25+
if (agent?.strategy === "llm-router") {
26+
const actions = (schema.actions || {}) as Record<string, unknown>;
27+
actions._routeToAgent = {
28+
description: "Route a task to a discovered agent",
29+
visibility: "protected",
30+
params: {
31+
agentName: { type: "string" },
32+
task: { type: "string" }
33+
},
34+
async handler(this: ServiceInstance, ctx: ServiceInstance) {
35+
const agents = this.discoverAgents();
36+
if (
37+
!agents.some(
38+
(a: DiscoveredAgent) => a.name === ctx.params.agentName
39+
)
40+
) {
41+
throw new Error(
42+
`Agent not found: ${ctx.params.agentName}`
43+
);
44+
}
45+
return this.delegateTo(ctx.params.agentName, ctx.params.task);
46+
}
47+
};
48+
schema.actions = actions;
49+
}
50+
},
51+
2252
events: {
2353
"$services.changed"(this: ServiceInstance) {
2454
if (this.settings.agent?.strategy === "llm-router") {
@@ -126,19 +156,6 @@ export default function OrchestratorMixin(_mixinOpts?: OrchestratorMixinOptions)
126156
}
127157
},
128158

129-
actions: {
130-
_routeToAgent: {
131-
visibility: "private",
132-
params: {
133-
agentName: { type: "string" },
134-
task: { type: "string" }
135-
},
136-
async handler(this: ServiceInstance, ctx: ServiceInstance) {
137-
return this.delegateTo(ctx.params.agentName, ctx.params.task);
138-
}
139-
}
140-
},
141-
142159
async started(this: ServiceInstance) {
143160
if (this.settings.agent?.strategy === "llm-router") {
144161
this._updateRouteToAgent();

test/integration/orchestrator.test.ts

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -389,6 +389,14 @@ describe("OrchestratorMixin E2E", () => {
389389

390390
expect(routeTool).toBeUndefined();
391391

392+
// Verify the _routeToAgent action is NOT registered on the service
393+
await expect(
394+
broker.call("orchestrator-5._routeToAgent", {
395+
agentName: "test",
396+
task: "test"
397+
})
398+
).rejects.toThrow();
399+
392400
await broker.destroyService(orchSvc);
393401
await broker.destroyService(llmSvc);
394402
});
@@ -464,4 +472,92 @@ describe("OrchestratorMixin E2E", () => {
464472
await broker.destroyService(orchSvc);
465473
await broker.destroyService(llmSvc);
466474
});
475+
476+
it("should reject _routeToAgent call with unknown agent name", async () => {
477+
const adapter = new FakeAdapter({
478+
responses: [
479+
{
480+
content: null,
481+
tool_calls: [
482+
{
483+
id: "call_bad_agent",
484+
type: "function" as const,
485+
function: {
486+
name: "_routeToAgent",
487+
arguments: JSON.stringify({
488+
agentName: "non-existent-agent",
489+
task: "Do something"
490+
})
491+
}
492+
}
493+
]
494+
},
495+
"Sorry, I could not find that agent."
496+
]
497+
});
498+
499+
const llmSvc = broker.createService({
500+
name: "llm.orch7",
501+
mixins: [LLMService()],
502+
settings: { adapter }
503+
});
504+
505+
const realAgent = broker.createService({
506+
name: "real-agent",
507+
mixins: [AgentMixin()],
508+
settings: {
509+
agent: {
510+
llm: "llm.orch7",
511+
description: "A real agent"
512+
}
513+
},
514+
actions: {
515+
doWork: {
516+
description: "Do some work",
517+
params: {},
518+
async handler() {
519+
return "done";
520+
}
521+
}
522+
}
523+
});
524+
525+
const orchSvc = broker.createService({
526+
name: "orchestrator-7",
527+
mixins: [OrchestratorMixin(), AgentMixin()],
528+
settings: {
529+
agent: {
530+
llm: "llm.orch7",
531+
description: "Validating orchestrator",
532+
strategy: "llm-router"
533+
}
534+
},
535+
actions: {}
536+
});
537+
538+
await broker.waitForServices(["llm.orch7", "real-agent", "orchestrator-7"]);
539+
540+
// Trigger discovery
541+
broker.broadcastLocal("$services.changed");
542+
543+
const result = await broker.call("orchestrator-7.run", {
544+
task: "Route to unknown agent"
545+
});
546+
547+
expect(result).toBe("Sorry, I could not find that agent.");
548+
549+
// Verify error was sent back as tool result
550+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
551+
const internalAdapter = (llmSvc as any)._adapter as FakeAdapter;
552+
const secondCall = internalAdapter.calls[1];
553+
const toolMsg = (secondCall.messages as { role: string; content: string }[]).find(
554+
m => m.role === "tool"
555+
);
556+
expect(toolMsg).toBeDefined();
557+
expect(toolMsg!.content).toContain("Agent not found: non-existent-agent");
558+
559+
await broker.destroyService(orchSvc);
560+
await broker.destroyService(realAgent);
561+
await broker.destroyService(llmSvc);
562+
});
467563
});

0 commit comments

Comments
 (0)