fix review finding: cap the detailed reminder's argument payload
This commit is contained in:
@@ -8,12 +8,13 @@ An advisory loop-breaker, not a model-facing tool: it never appears in the tool
|
||||
- id: repeat-tool-guard
|
||||
name: '@deepseek-ai/dsh-repeat-tool-guard'
|
||||
config:
|
||||
thresholds: [3, 5, 8] # default; consecutive counts that trigger a reminder
|
||||
include: [] # tool-name patterns to track; empty ⇒ all tools
|
||||
exclude: [todo_write] # tool-name patterns transparent to the chain
|
||||
thresholds: [3, 5, 8] # default; consecutive counts that trigger a reminder
|
||||
include: [] # tool-name patterns to track; empty ⇒ all tools
|
||||
exclude: [todo_write] # tool-name patterns transparent to the chain
|
||||
argumentsPreviewChars: 500 # default; cap on arguments quoted in the detailed reminder
|
||||
```
|
||||
|
||||
`thresholds` fails loud at plugin load: an empty list, a non-integer, a value below 2, or a duplicate throws, never a silent fall-back to defaults. The list is normalized to ascending order; the FIRST threshold delivers a short generic nudge, every later threshold delivers the detailed form naming the tool, the run length, and the canonical arguments.
|
||||
`thresholds` fails loud at plugin load: an empty list, a non-integer, a value below 2, or a duplicate throws, never a silent fall-back to defaults; `argumentsPreviewChars` equally rejects anything but an integer >= 1. The list is normalized to ascending order; the FIRST threshold delivers a short generic nudge, every later threshold delivers the detailed form naming the tool, the run length, and the canonical arguments — head-truncated at `argumentsPreviewChars` with an omitted-count marker, so a looping `write`/`edit` payload cannot ride into the next request unbounded (the chain key always compares the FULL canonical string; the cap bounds the reminder, never the detection).
|
||||
|
||||
`include`/`exclude` entries support `*` wildcards and are predicates over whatever tools exist at call time, not references to registry entries — a pattern matching no currently registered tool is NOT an error (`exclude: [mcp_*]` stays valid in a deployment that loads no MCP tools), unlike `toolOrder`'s referent check.
|
||||
|
||||
|
||||
@@ -59,12 +59,21 @@ export interface Config {
|
||||
include?: string[]
|
||||
/** Tool-name patterns transparent to the chain (neither count nor reset). */
|
||||
exclude?: string[]
|
||||
/**
|
||||
* Maximum characters of canonical arguments quoted in the DETAILED reminder
|
||||
* (default 500). Large payloads (a `write` body, a long command) would
|
||||
* otherwise ride into the next request unbounded — precisely in a loop
|
||||
* scenario; the cap bounds the reminder, never the detection (the chain key
|
||||
* always compares the FULL canonical string).
|
||||
*/
|
||||
argumentsPreviewChars?: number
|
||||
}
|
||||
|
||||
export const Config: z<Config> = z.object({
|
||||
thresholds: z.array(z.number()).default([3, 5, 8]),
|
||||
include: z.array(z.string()).default([]),
|
||||
exclude: z.array(z.string()).default([]),
|
||||
argumentsPreviewChars: z.number().default(500),
|
||||
})
|
||||
|
||||
/**
|
||||
@@ -128,6 +137,16 @@ function wildcardToRegExp(pattern: string): RegExp {
|
||||
return new RegExp(`^${escaped.replaceAll('*', '.*')}$`)
|
||||
}
|
||||
|
||||
/**
|
||||
* Head-truncate the canonical arguments for quoting in the detailed reminder,
|
||||
* marking how much was omitted. Bounds only the model-visible text — the
|
||||
* chain key always uses the full canonical string.
|
||||
*/
|
||||
function previewArguments(canonical: string, cap: number): string {
|
||||
if (canonical.length <= cap) return canonical
|
||||
return `${canonical.slice(0, cap)}… (+${canonical.length - cap} more chars)`
|
||||
}
|
||||
|
||||
/**
|
||||
* Validate `thresholds` per the fail-loud contract and return them sorted
|
||||
* ascending (the escalation rule reads `thresholds[0]` as the gentle tier, so
|
||||
@@ -173,11 +192,15 @@ interface Chain {
|
||||
* @param config - validated {@link Config}; `thresholds` is re-checked fail-loud here.
|
||||
*/
|
||||
export function apply(ctx: Context, config: Config): void {
|
||||
// schemastery's .default() guarantees the arrays are set after validation.
|
||||
// schemastery's .default() guarantees the fields are set after validation.
|
||||
const thresholds = validateThresholds(config.thresholds as number[])
|
||||
const thresholdSet = new Set(thresholds)
|
||||
const includePatterns = (config.include as string[]).map(wildcardToRegExp)
|
||||
const excludePatterns = (config.exclude as string[]).map(wildcardToRegExp)
|
||||
const argumentsPreviewChars = config.argumentsPreviewChars as number
|
||||
if (!Number.isInteger(argumentsPreviewChars) || argumentsPreviewChars < 1) {
|
||||
throw new Error(`repeat-tool-guard: invalid argumentsPreviewChars ${argumentsPreviewChars} — must be an integer >= 1`)
|
||||
}
|
||||
|
||||
const chains = new Map<AgentId, Chain>()
|
||||
|
||||
@@ -206,7 +229,9 @@ export function apply(ctx: Context, config: Config): void {
|
||||
const count = chain !== undefined && chain.key === key ? chain.count + 1 : 1
|
||||
chains.set(exec.agent.id, { key, count })
|
||||
if (!thresholdSet.has(count)) return undefined
|
||||
const text = count === thresholds[0] ? GENTLE_REMINDER : detailedReminder(exec.name, count, canonical)
|
||||
const text = count === thresholds[0]
|
||||
? GENTLE_REMINDER
|
||||
: detailedReminder(exec.name, count, previewArguments(canonical, argumentsPreviewChars))
|
||||
return { content: [{ type: 'text', text }], source: PLUGIN_SOURCE }
|
||||
}
|
||||
|
||||
|
||||
@@ -90,6 +90,28 @@ describe('threshold escalation', () => {
|
||||
})
|
||||
|
||||
describe('chain semantics', () => {
|
||||
it('caps the detailed reminder arguments at argumentsPreviewChars (detection still keys on the full string)', async () => {
|
||||
const ctx = await harness({ thresholds: [2, 3], argumentsPreviewChars: 24 })
|
||||
const bigPayload = 'x'.repeat(400)
|
||||
const adapter = new MockAdapter([
|
||||
toolCallResponse('c1', 'probe', { body: bigPayload }),
|
||||
toolCallResponse('c2', 'probe', { body: bigPayload }),
|
||||
toolCallResponse('c3', 'probe', { body: bigPayload }),
|
||||
textResponse('done'),
|
||||
])
|
||||
ctx.llm.registerAdapter(['mock'], adapter)
|
||||
const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' })
|
||||
agent.send([{ type: 'text', text: 'go' }])
|
||||
await waitForIdle(ctx, agent)
|
||||
|
||||
const found = reminders(agent)
|
||||
expect(found).toHaveLength(2) // gentle at 2, detailed at 3 — full-key matching survived the cap
|
||||
const detailed = found[1]!.text
|
||||
expect(detailed).toContain('- arguments: {"body":"xxxxxxxxxxxxxx') // 24-char head
|
||||
expect(detailed).toContain('… (+387 more chars)')
|
||||
expect(detailed).not.toContain(bigPayload)
|
||||
})
|
||||
|
||||
it('a different tracked call resets the chain', async () => {
|
||||
const ctx = await harness()
|
||||
const adapter = new MockAdapter([
|
||||
@@ -369,4 +391,11 @@ describe('config validation fails loud', () => {
|
||||
const ctx = await spine()
|
||||
await expect(ctx.plugin(RepeatToolGuard, { thresholds: [3, 3] })).rejects.toThrow(/duplicates/)
|
||||
})
|
||||
|
||||
it('rejects a non-positive or fractional argumentsPreviewChars', async () => {
|
||||
const ctx = await spine()
|
||||
await expect(ctx.plugin(RepeatToolGuard, { argumentsPreviewChars: 0 })).rejects.toThrow(/argumentsPreviewChars/)
|
||||
const ctx2 = await spine()
|
||||
await expect(ctx2.plugin(RepeatToolGuard, { argumentsPreviewChars: 12.5 })).rejects.toThrow(/argumentsPreviewChars/)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user