Browse Source

fix(timeout-policy): warn on configured-but-unregistered tool names

ds-review-bot flagged that a typo'd or stale config key (e.g. web_fech for
web_fetch) silently applies the timeout to nothing — the tools/execute lookup
just never matches. Mirror dsh-tool-subagent's lifecycle-driven handling of a
configured-but-unregistered provider: on every tools/change (and once at load),
logger.warn each configured name still absent from ctx.tools, warning each name
at most once so a late registration silences it. Not a load-time throw — the
tool set is dynamic (cordis.yml load order, HMR), so a real tool may register
later.

Declare inject = ['tools'] since the plugin now reads ctx.tools synchronously
in apply (previously only inside event callbacks). Regenerate config-catalog
(Requires: tools) and event-producer-consumer graph.
Dudu-0223 3 months ago
parent
commit
3265bdbf70

+ 3 - 1
docs/config-catalog.md

@@ -604,6 +604,8 @@ Source: [`packages/core/system-prompt/src/index.ts:179`](../packages/core/system
 
 ## `@deepseek-ai/dsh-timeout-policy`
 
+Requires: `tools`
+
 ```ts config-catalog
 /**
  * Plugin config: per-tool timeout policy, keyed by the model-facing tool name.
@@ -623,7 +625,7 @@ export interface ToolTimeoutPolicy {
 }
 ```
 
-Source: [`packages/timeout/timeout-policy/src/index.ts:61`](../packages/timeout/timeout-policy/src/index.ts)
+Source: [`packages/timeout/timeout-policy/src/index.ts:64`](../packages/timeout/timeout-policy/src/index.ts)
 
 ## `@deepseek-ai/dsh-tool-fs`
 

+ 1 - 1
docs/event-producer-consumer.md

@@ -31,7 +31,7 @@ This matrix shows which packages dispatch each harness-owned event and which pac
 | `subagent/start` | `emit` | [`packages/subagent/subagent/src/index.ts:91`](../packages/subagent/subagent/src/index.ts) | [`subagent`](../packages/subagent/subagent) (`events.dispatch`) | [`hooks-claude`](../packages/hooks/hooks-claude) |
 | `system-prompt/assemble` | `waterfall` | [`packages/core/system-prompt/src/index.ts:38`](../packages/core/system-prompt/src/index.ts) | [`system-prompt`](../packages/core/system-prompt) (`waterfall`) | - |
 | `system-prompt/change` | `emit` | [`packages/core/system-prompt/src/index.ts:44`](../packages/core/system-prompt/src/index.ts) | [`system-prompt`](../packages/core/system-prompt) (`emit`) | - |
-| `tools/change` | `emit` | [`packages/core/tools/src/index.ts:118`](../packages/core/tools/src/index.ts) | [`tools`](../packages/core/tools) (`emit`) | - |
+| `tools/change` | `emit` | [`packages/core/tools/src/index.ts:118`](../packages/core/tools/src/index.ts) | [`tools`](../packages/core/tools) (`emit`) | [`timeout-policy`](../packages/timeout/timeout-policy) |
 | `tools/execute` | `waterfall` | [`packages/core/tools/src/index.ts:97`](../packages/core/tools/src/index.ts) | [`tools`](../packages/core/tools) (`waterfall`) | [`timeout-policy`](../packages/timeout/timeout-policy) |
 | `tools/post-execute` | `waterfall` | [`packages/core/tools/src/index.ts:113`](../packages/core/tools/src/index.ts) | [`tools`](../packages/core/tools) (`waterfall`) | [`hooks-claude`](../packages/hooks/hooks-claude), [`hooks-codex`](../packages/hooks/hooks-codex) |
 | `tools/pre-execute` | `waterfall` | [`packages/core/tools/src/index.ts:77`](../packages/core/tools/src/index.ts) | [`tools`](../packages/core/tools) (`waterfall`) | [`hooks-claude`](../packages/hooks/hooks-claude), [`hooks-codex`](../packages/hooks/hooks-codex) |

+ 2 - 0
packages/timeout/timeout-policy/README.md

@@ -25,6 +25,8 @@ Per-tool policy, keyed by the model-facing tool name. There is deliberately **no
 |---|---|---|
 | `tools` | `Record<string, { timeoutMs }>` | Per-tool timeout policy; an unlisted tool gets no deadline. `timeoutMs` is required per configured tool and must be positive finite. |
 
+A configured tool name that never registers (a typo like `web_fech`, or a stale key) would silently apply the timeout to nothing. Because the tool set is dynamic (plugins register in `cordis.yml` order, HMR re-registers), this is not a load-time error — a real tool may register later. Instead, on every `tools/change` (and once at load) the plugin `logger.warn`s each configured name still absent from `ctx.tools`, warning each name at most once so a late registration silences it. This mirrors `dsh-tool-subagent`'s lifecycle-driven handling of a configured-but-unregistered provider name.
+
 ### Behavior
 
 For a **configured** tool the listener:

+ 35 - 0
packages/timeout/timeout-policy/src/index.ts

@@ -46,6 +46,9 @@ export const TOOL_TIMEOUT = 'TOOL_TIMEOUT'
 /** Cordis plugin name used by loader diagnostics. */
 export const name = 'timeout-policy'
 
+/** The tool registry seam this plugin wraps (`tools/execute`) and reads (`tools/change`, `get`). */
+export const inject = ['tools']
+
 /** Per-tool timeout policy. `timeoutMs` is required and must be positive finite. */
 export interface ToolTimeoutPolicy {
   /** The per-call cooperative deadline for this tool, in milliseconds. */
@@ -103,6 +106,15 @@ export function toolTimeoutResult(callId: CallId, timeoutMs: number): ToolExecut
  * `tools/post-execute` sees the caller's own signal, and replaces the result
  * with {@link toolTimeoutResult} when its own timer fired. An unconfigured tool
  * delegates untouched.
+ *
+ * A configured tool name that is never registered is almost always a typo or a
+ * stale config key (e.g. `web_fech` for `web_fetch`): the wrapper would then
+ * silently never fire for the intended tool. Since the tool set is dynamic
+ * (plugins register in `cordis.yml` order, and HMR re-registers), this cannot
+ * be a load-time hard error — a real tool may register later. Instead, mirror
+ * `dsh-tool-subagent`'s lifecycle-driven approach: on every `tools/change` (and
+ * once at apply), `logger.warn` each configured name still absent from the
+ * registry, warning each name at most once so a late registration silences it.
  */
 export function apply(ctx: Context, config: Config): void {
   // schemastery (Config) has already filled `tools` with its {} default.
@@ -111,6 +123,29 @@ export function apply(ctx: Context, config: Config): void {
     assertPositiveFinite(toolName, policy.timeoutMs)
   }
 
+  // Warn once per configured name that no registered tool matches, so a typo'd
+  // or stale config key is visible instead of silently applying to nothing. A
+  // name that later registers is dropped from `pending` before it is warned; a
+  // name that never registers is warned at most once (moved to `warned`), so a
+  // busy `tools/change` stream cannot spam the same key.
+  const pending = new Set(Object.keys(resolved.tools))
+  const warned = new Set<string>()
+  const warnUnknownToolNames = (): void => {
+    const nowUnknown: string[] = []
+    for (const name of pending) {
+      if (ctx.tools.get(name) !== undefined) { pending.delete(name); continue }
+      if (!warned.has(name)) { warned.add(name); nowUnknown.push(name) }
+    }
+    if (nowUnknown.length > 0) {
+      ctx.logger.warn(
+        `timeout-policy: configured timeout for unregistered tool(s) ${nowUnknown.map(n => `"${n}"`).join(', ')} `
+        + '— check for a typo or stale config key; the timeout applies to nothing until the tool registers.',
+      )
+    }
+  }
+  ctx.on('tools/change', warnUnknownToolNames)
+  warnUnknownToolNames()
+
   ctx.on('tools/execute', async (exec, next): Promise<ToolExecutionResult> => {
     const timeoutMs = resolved.tools[exec.name]?.timeoutMs
     // Unconfigured tool: no deadline, delegate unchanged.

+ 58 - 1
packages/timeout/timeout-policy/tests/timeout-policy.spec.ts

@@ -84,6 +84,62 @@ describe('timeout-policy config validation', () => {
   })
 })
 
+describe('timeout-policy unknown-tool-name diagnostics', () => {
+  it('warns for a configured tool name that is never registered (typo/stale key)', async () => {
+    const ctx = new Context()
+    await ctx.plugin(SystemPrompt)
+    await ctx.plugin(ToolRegistry)
+    const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined)
+    // web_fech is a typo for web_fetch, and no tool by that name is registered.
+    await ctx.plugin(timeoutPolicy, { tools: { web_fech: { timeoutMs: 30_000 } } })
+    expect(warn).toHaveBeenCalledTimes(1)
+    expect(warn.mock.calls[0]?.[0]).toContain('"web_fech"')
+    expect(warn.mock.calls[0]?.[0]).toContain('unregistered tool')
+  })
+
+  it('does NOT warn when the configured tool is already registered at load', async () => {
+    const ctx = new Context()
+    await ctx.plugin(SystemPrompt)
+    await ctx.plugin(ToolRegistry)
+    ctx.tools.register(fastTool)
+    const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined)
+    await ctx.plugin(timeoutPolicy, { tools: { fast: { timeoutMs: 30_000 } } })
+    expect(warn).not.toHaveBeenCalled()
+  })
+
+  it('does NOT warn once a configured tool registers LATER (load-order safe)', async () => {
+    const ctx = new Context()
+    await ctx.plugin(SystemPrompt)
+    await ctx.plugin(ToolRegistry)
+    const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined)
+    // Plugin loads before the tool it configures — the initial check would warn,
+    // so register first is the interesting case: mount with a not-yet-present
+    // name, then register it; the tools/change listener must clear it.
+    await ctx.plugin(timeoutPolicy, { tools: { late: { timeoutMs: 30_000 } } })
+    expect(warn).toHaveBeenCalledTimes(1) // absent at load → warned once
+    warn.mockClear()
+    ctx.tools.register({ ...fastTool, name: 'late' }) // now it registers
+    // A subsequent tools/change must NOT re-warn the now-registered name.
+    ctx.tools.register({ ...fastTool, name: 'other' })
+    expect(warn).not.toHaveBeenCalled()
+  })
+
+  it('warns at most once per unknown name across repeated tools/change', async () => {
+    const ctx = new Context()
+    await ctx.plugin(SystemPrompt)
+    await ctx.plugin(ToolRegistry)
+    const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined)
+    await ctx.plugin(timeoutPolicy, { tools: { ghost: { timeoutMs: 30_000 } } })
+    expect(warn).toHaveBeenCalledTimes(1) // apply-time check
+    // Each register/unregister emits tools/change; the ghost stays unknown but
+    // must not be warned again.
+    const dispose = ctx.tools.register(fastTool)
+    dispose()
+    ctx.tools.register({ ...fastTool, name: 'another' })
+    expect(warn).toHaveBeenCalledTimes(1)
+  })
+})
+
 describe('timeout-policy delegation (unconfigured / fast)', () => {
   it('delegates an UNCONFIGURED tool unchanged and does not touch exec.signal', async () => {
     const ctx = await setup({ other: { timeoutMs: 50 } })
@@ -234,13 +290,14 @@ describe('timeout-policy disposal (HMR safety)', () => {
 })
 
 describe('dsh-timeout-policy real-load-path guard', () => {
-  it('has no default export and keeps name/Config through unwrapExports', () => {
+  it('has no default export and keeps name/inject/Config through unwrapExports', () => {
     expect('default' in timeoutPolicy).toBe(false)
 
     const loader = Object.create(Loader.prototype) as Loader
     const unwrapped = loader.unwrapExports(timeoutPolicy) as Record<string, unknown>
     expect(unwrapped).toBe(timeoutPolicy)
     expect(unwrapped.name).toBe('timeout-policy')
+    expect(unwrapped.inject).toEqual(['tools'])
     expect(typeof unwrapped.apply).toBe('function')
     expect(unwrapped.Config).toBeDefined()
   })