fix(system-prompt): reject a toolOrder that names an unregistered tool
Review follow-up (#196): a listed name with no registered tool was silently ignored; misconfiguration must block work instead. The check lives in the assembly — the earliest moment the registered tool set exists (tool plugins register after the service constructs) and the only universal one (cordis has no "all plugins loaded" event; registrations change at any time). assemble() is now async so the throw surfaces as a rejection rather than a synchronous escape from a Promise-returning method. Blast radius, pinned by a loop-level test: the rejection reaches the turn's outer catch — the turn closes balanced with an `error` reason, agent/error mirrors it, no step opens, no request/header is logged, no request reaches the adapter, and the agent returns to idle; every turn fails identically until the config is fixed. A boot-time validation pass was considered and rejected (recorded in the RFC). The general principle — misconfiguration fails loud, never a silent skip — is added to AGENTS.md.
This commit is contained in:
@@ -91,4 +91,27 @@ describe('loop-level canonical tool order', () => {
|
||||
expect(adapter.requests[0]?.tools?.map(tool => tool.name)).toEqual(['zulu', 'alpha', 'mike'])
|
||||
expect(Object.isFrozen(adapter.requests[0])).toBe(true)
|
||||
})
|
||||
|
||||
it('fails the turn — no model request — when toolOrder names an unregistered tool', async () => {
|
||||
// The assemble rejection escapes to runTurn's outer catch: the open turn
|
||||
// closes with an `error` reason (agent/error mirrors it), no step opens,
|
||||
// no request/header is logged, the adapter never sees a request, and the
|
||||
// agent returns to idle — a misconfigured deployment fails every turn
|
||||
// deterministically instead of silently reordering nothing.
|
||||
const adapter = new MockAdapter([textResponse('never sent')])
|
||||
const ctx = await harness(adapter, ['ghost', TOOL_ORDER_REST])
|
||||
registerNamed(ctx, 'alpha')
|
||||
const errors: Error[] = []
|
||||
ctx.on('agent/error', (_agent, _turn, _step, error) => void errors.push(error))
|
||||
const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' })
|
||||
agent.send([{ type: 'text', text: 'go' }])
|
||||
await waitForIdle(ctx, agent)
|
||||
expect(adapter.requests).toHaveLength(0)
|
||||
expect(errors.map(e => e.message)).toEqual(['toolOrder lists unregistered tool "ghost"; registered tools: alpha'])
|
||||
expect(foldRequestHeader(agent.session.events)).toBeUndefined()
|
||||
const end = agent.session.events.find(e => e.type === 'turn/end')
|
||||
expect(end?.type === 'turn/end' && end.data.reason).toMatchObject({ kind: 'error', step: 1 })
|
||||
// The turn is balanced (turn/start → turn/end) with no step events inside.
|
||||
expect(agent.session.events.some(e => e.type === 'step/start')).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -7,7 +7,7 @@ System prompt assembly registry. Plugins contribute ordered text sections, tool-
|
||||
| Key | Default | Meaning |
|
||||
|---|---|---|
|
||||
| `persona` | `''` | The deployment persona: the ONE deployment-authored prompt fragment, rendered as the order-0 `deployment:persona` section and shared by every agent in the context (subagents included). A template — complete `{{…}}` groups are interpreted strictly against the registered variables (the shipped loop registers `{{model}}`/`{{cwd}}`), with no escape syntax for literal braces yet. Empty ⇒ the section is dropped at render. |
|
||||
| `toolOrder` | — | Explicit model-facing tool order, as a list of `ToolSchema.name`s with one `'<unlisted-tools>'` rest entry (`TOOL_ORDER_REST`): listed tools take their listed position, names with no registered tool are ignored, unlisted tools land at the rest entry in lexicographic name order. Absent ⇒ plain lexicographic name order. Applied to the collected tools BEFORE the `system-prompt/assemble` waterfall — like the sections' `order` sort, it canonicalizes what the registry contributed (registration order is a plugin-load artifact), and a waterfall listener that mutates the list owns the determinism of what it emits. A list without exactly one rest entry, or with duplicates, throws at load. Why a central list and not per-plugin weights: [Explicit model-facing tool order](../../../docs/rfc/implemented/feature/2026-07-06-explicit-tool-order.md). |
|
||||
| `toolOrder` | — | Explicit model-facing tool order, as a list of `ToolSchema.name`s with one `'<unlisted-tools>'` rest entry (`TOOL_ORDER_REST`): listed tools take their listed position, unlisted tools land at the rest entry in lexicographic name order. Absent ⇒ plain lexicographic name order. Applied to the collected tools BEFORE the `system-prompt/assemble` waterfall — like the sections' `order` sort, it canonicalizes what the registry contributed (registration order is a plugin-load artifact), and a waterfall listener that mutates the list owns the determinism of what it emits. Misconfiguration fails loud: a list without exactly one rest entry, or with duplicates, throws at load; a listed name with no registered tool rejects every `assemble()` — under the shipped loop the turn fails before any model request. Why a central list and not per-plugin weights: [Explicit model-facing tool order](../../../docs/rfc/implemented/feature/2026-07-06-explicit-tool-order.md). |
|
||||
|
||||
## Service: `SystemPrompt` (ctx key: `systemPrompt`)
|
||||
|
||||
@@ -16,7 +16,7 @@ System prompt assembly registry. Plugins contribute ordered text sections, tool-
|
||||
- `ctx.systemPrompt.section(section: PromptSection): () => void` Contribute a section. Duplicate names throw. Disposed with the calling fiber.
|
||||
- `ctx.systemPrompt.tools(provider: () => ToolSchema[]): () => void` Contribute tool schemas (evaluated at each assembly). Disposed with the calling fiber.
|
||||
- `ctx.systemPrompt.variable(name: string, provider: (context) => string | undefined): () => void` Contribute a prompt variable, referenced from section text as `{{name}}`. Duplicate or unreferenceable names throw; `undefined` means "no value for this assembly". Disposed with the calling fiber.
|
||||
- `ctx.systemPrompt.assemble(context?: AssembleContext): Promise<PromptAssembly>` Assemble the prompt for one caller. Runs through the `system-prompt/assemble` waterfall.
|
||||
- `ctx.systemPrompt.assemble(context?: AssembleContext): Promise<PromptAssembly>` Assemble the prompt for one caller. Runs through the `system-prompt/assemble` waterfall. Rejects when a configured `toolOrder` names a tool no provider contributed.
|
||||
|
||||
### Events
|
||||
|
||||
|
||||
@@ -120,10 +120,13 @@ const GROUP_AT = /^\{\{([^{}]*)\}\}/
|
||||
export const TOOL_ORDER_REST = '<unlisted-tools>'
|
||||
|
||||
/**
|
||||
* Validate a configured tool-order list at service construction: the
|
||||
* {@link TOOL_ORDER_REST} rest entry exactly once, no duplicate names. Returns the list
|
||||
* (or undefined when unconfigured); throws otherwise, failing the service at
|
||||
* load — a bad order config must never reach an assembly.
|
||||
* Validate a configured tool-order list's shape at service construction:
|
||||
* the {@link TOOL_ORDER_REST} rest entry exactly once, no duplicate names.
|
||||
* Returns the list (or undefined when unconfigured); throws otherwise,
|
||||
* failing the service at load — a bad order config must never reach an
|
||||
* assembly. Whether every listed name matches a registered tool is checked
|
||||
* at each assembly instead ({@link orderTools}): tool plugins register after
|
||||
* this service constructs, so the tool set does not exist yet here.
|
||||
*/
|
||||
function validateToolOrder(toolOrder: string[] | undefined): string[] | undefined {
|
||||
if (toolOrder === undefined) return undefined
|
||||
@@ -141,12 +144,22 @@ function validateToolOrder(toolOrder: string[] | undefined): string[] | undefine
|
||||
/**
|
||||
* Order collected tool schemas by the validated policy: with no configured
|
||||
* list, plain lexicographic name order; with one, listed names take their
|
||||
* listed position and every unlisted tool lands at the {@link TOOL_ORDER_REST} rest entry in
|
||||
* lexicographic name order. Never drops a tool, and both sorts are stable, so
|
||||
* tools sharing a name keep their collection order.
|
||||
* listed position and every unlisted tool lands at the
|
||||
* {@link TOOL_ORDER_REST} rest entry in lexicographic name order. A listed
|
||||
* name with no collected tool throws — misconfiguration fails loud, and this
|
||||
* is the earliest moment the registered tool set exists to check against
|
||||
* (tool plugins register after the service constructs, so load time is too
|
||||
* early): the assembly rejects, failing the caller's turn before any model
|
||||
* request. Never drops a tool, and both sorts are stable, so tools sharing a
|
||||
* name keep their collection order.
|
||||
*/
|
||||
function orderTools(tools: ToolSchema[], toolOrder: string[] | undefined): ToolSchema[] {
|
||||
if (toolOrder === undefined) return tools.sort(compareToolNames)
|
||||
const registered = new Set(tools.map(tool => tool.name))
|
||||
const unknown = toolOrder.filter(name => name !== TOOL_ORDER_REST && !registered.has(name))
|
||||
if (unknown.length > 0) {
|
||||
throw new Error(`toolOrder lists unregistered tool${unknown.length > 1 ? 's' : ''} ${unknown.map(name => `"${name}"`).join(', ')}; registered tools: ${[...registered].sort().join(', ') || '(none)'}`)
|
||||
}
|
||||
const listed = new Set(toolOrder)
|
||||
const rest = tools.filter(tool => !listed.has(tool.name)).sort(compareToolNames)
|
||||
return toolOrder.flatMap(name =>
|
||||
@@ -174,13 +187,17 @@ export interface Config {
|
||||
persona?: string
|
||||
/**
|
||||
* Explicit model-facing tool order, as a list of `ToolSchema.name`s: listed
|
||||
* tools take their listed position, names with no registered tool are
|
||||
* ignored, and tools absent from the list are inserted at the
|
||||
* {@link TOOL_ORDER_REST} (`'<unlisted-tools>'`) entry in lexicographic name order. A
|
||||
* configured list must contain the rest entry exactly once and no duplicate names —
|
||||
* anything else throws at load; a bad order config must never reach a
|
||||
* model request. When omitted, tools are ordered lexicographically by name.
|
||||
* Applied to the tools {@link SystemPrompt.assemble} collects, BEFORE the
|
||||
* tools take their listed position, and tools absent from the list are
|
||||
* inserted at the {@link TOOL_ORDER_REST} (`'<unlisted-tools>'`) entry in
|
||||
* lexicographic name order. A configured list must contain the rest entry
|
||||
* exactly once, no duplicate names, and no name without a registered tool —
|
||||
* a misconfigured order blocks work instead of silently reaching a model
|
||||
* request: shape violations throw at load, and an unregistered name rejects
|
||||
* every assembly (failing the turn before any model request — the earliest
|
||||
* moment the registered tool set exists to check against, since tool
|
||||
* plugins register after this service constructs). When omitted, tools are
|
||||
* ordered lexicographically by name. Applied to the tools
|
||||
* {@link SystemPrompt.assemble} collects, BEFORE the
|
||||
* `system-prompt/assemble` waterfall — like the sections' `order` sort, it
|
||||
* canonicalizes what the registry contributed (registration order is a
|
||||
* plugin-load artifact); a waterfall listener that mutates the tool list
|
||||
@@ -394,7 +411,8 @@ export class SystemPrompt extends Service {
|
||||
* against `context` and sorted by order, tools collected from all providers
|
||||
* and put in the canonical model-facing order ({@link Config.toolOrder}, or
|
||||
* lexicographic name order when unconfigured — provider registration order
|
||||
* is a plugin-load artifact and never reaches the assembly), and every
|
||||
* is a plugin-load artifact and never reaches the assembly; a configured
|
||||
* order naming a tool no provider contributed rejects the assembly), and every
|
||||
* registered variable resolved against `context` into `assembly.variables`.
|
||||
* Tool schemas are deep-cloned because adapters and request waterfalls may
|
||||
* mutate schema objects. Runs through the `system-prompt/assemble`
|
||||
@@ -408,7 +426,10 @@ export class SystemPrompt extends Service {
|
||||
* see {@link AssembleContext}).
|
||||
* @returns the assembly after the waterfall has run.
|
||||
*/
|
||||
assemble(context: AssembleContext = {}): Promise<PromptAssembly> {
|
||||
// async so the misconfigured-toolOrder throw in orderTools surfaces as a
|
||||
// rejection: a Promise-returning method must not throw synchronously
|
||||
// (`assemble().catch(...)` would miss it).
|
||||
async assemble(context: AssembleContext = {}): Promise<PromptAssembly> {
|
||||
const variables: Record<string, string | undefined> = {}
|
||||
for (const [name, provider] of this.variableProviders) {
|
||||
variables[name] = provider(context)
|
||||
|
||||
@@ -18,6 +18,8 @@ function names(assembly: PromptAssembly): string[] {
|
||||
}
|
||||
|
||||
describe('SystemPrompt tool order', () => {
|
||||
// The ONE place the public constant's value is pinned; everything else
|
||||
// (tests and deployment configs alike) references TOOL_ORDER_REST.
|
||||
it('exports the rest entry as "<unlisted-tools>"', () => {
|
||||
expect(TOOL_ORDER_REST).toBe('<unlisted-tools>')
|
||||
})
|
||||
@@ -40,12 +42,25 @@ describe('SystemPrompt tool order', () => {
|
||||
expect(names(await backward.systemPrompt.assemble())).toEqual(['alpha', 'zulu'])
|
||||
})
|
||||
|
||||
it('applies a configured toolOrder: listed positions, rest at the rest entry lexicographically, absent names ignored', async () => {
|
||||
const ctx = await mount({ toolOrder: ['todo_write', 'ghost', TOOL_ORDER_REST, 'bash'] })
|
||||
it('applies a configured toolOrder: listed positions, rest at the rest entry lexicographically', async () => {
|
||||
const ctx = await mount({ toolOrder: ['todo_write', TOOL_ORDER_REST, 'bash'] })
|
||||
ctx.systemPrompt.tools(() => [tool('bash'), tool('echo_b'), tool('todo_write'), tool('echo_a')])
|
||||
expect(names(await ctx.systemPrompt.assemble())).toEqual(['todo_write', 'echo_a', 'echo_b', 'bash'])
|
||||
})
|
||||
|
||||
it('rejects the assembly when toolOrder names a tool that is not registered (misconfiguration blocks work)', async () => {
|
||||
const ctx = await mount({ toolOrder: ['todo_write', 'ghost', TOOL_ORDER_REST, 'wraith'] })
|
||||
ctx.systemPrompt.tools(() => [tool('bash'), tool('todo_write')])
|
||||
await expect(ctx.systemPrompt.assemble()).rejects.toThrow(
|
||||
'toolOrder lists unregistered tools "ghost", "wraith"; registered tools: bash, todo_write')
|
||||
})
|
||||
|
||||
it('names the single unregistered tool when no tools are registered at all', async () => {
|
||||
const ctx = await mount({ toolOrder: ['ghost', TOOL_ORDER_REST] })
|
||||
await expect(ctx.systemPrompt.assemble()).rejects.toThrow(
|
||||
'toolOrder lists unregistered tool "ghost"; registered tools: (none)')
|
||||
})
|
||||
|
||||
it('keeps collection order between tools that share a name (stable sort)', async () => {
|
||||
const ctx = await mount()
|
||||
ctx.systemPrompt.tools(() => [tool('dup', 'first'), tool('anchor'), tool('dup', 'second')])
|
||||
|
||||
@@ -24,7 +24,7 @@ Because the package wires no logger entry, an ACP leaf has **nothing to get wron
|
||||
|---|---|---|
|
||||
| `model` | (required) | the per-session agent template the bridge creates agents from |
|
||||
| `persona` | — | the deployment persona template (may reference `{{model}}`/`{{cwd}}`), routed to `dsh-system-prompt` |
|
||||
| `toolOrder` | — | explicit model-facing tool order (a name list with one `'<unlisted-tools>'` rest entry; absent — lexicographic), routed to `dsh-system-prompt` |
|
||||
| `toolOrder` | — | explicit model-facing tool order (a name list with one `'<unlisted-tools>'` rest entry; absent — lexicographic; an unregistered name fails each turn at prompt assembly), routed to `dsh-system-prompt` |
|
||||
| `persistenceRoot` | `./.sessions` | the JSONL backend's root directory |
|
||||
|
||||
The leaf supplies the swappable backends: an LLM adapter (`llm-deepseek` for the real model, `llm-replay` for keyless snapshot replay) and a bash executor (`bash-local`).
|
||||
|
||||
@@ -25,7 +25,7 @@ The leaf `cordis.yml` supplies only the **swappable backends** — an LLM adapte
|
||||
|---|---|---|
|
||||
| `model` | (required) | the pre-created `main` agent's model |
|
||||
| `persona` | — | the deployment persona template (may reference `{{model}}`), routed to `dsh-system-prompt` |
|
||||
| `toolOrder` | — | explicit model-facing tool order (a name list with one `'<unlisted-tools>'` rest entry; absent — lexicographic), routed to `dsh-system-prompt` |
|
||||
| `toolOrder` | — | explicit model-facing tool order (a name list with one `'<unlisted-tools>'` rest entry; absent — lexicographic; an unregistered name fails each turn at prompt assembly), routed to `dsh-system-prompt` |
|
||||
| `persistenceRoot` | `./.sessions` | the JSONL backend's root directory |
|
||||
| `welcome` | `ready.` | the stdin-chat banner |
|
||||
| `resumeSessionId` | — | resume a persisted session id instead of starting fresh (sourced from an env var in the leaf) |
|
||||
|
||||
Reference in New Issue
Block a user