From 41d6ae536f02bdc91e98af3abda99f2dbe036b75 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 24 May 2026 00:32:18 -0700 Subject: [PATCH] =?UTF-8?q?v0.41:=20subagent=20hardening=20=E2=80=94=20Bug?= =?UTF-8?q?=201=20+=20Bug=203=20+=20Approach=20C=20composable=20prompt?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent fixes to src/core/minions/handlers/subagent.ts. Each is covered by its own test set; bundled in one commit because they touch overlapping lines of subagent.ts (cleaner than 3 hunk-split commits). Bug 1 — rate-lease default 8 → 32 + `unlimited` sentinel src/core/minions/handlers/subagent.ts:61 Pre-v0.41 the default cap of 8 starved 10-concurrency batches on upstreams with no provider-side rate limit (Azure/Bedrock/self-hosted). New resolveLeaseCap() bumps default to 32, accepts `unlimited`/`none` as POSITIVE_INFINITY sentinel, throws on NaN/negative/zero with a paste-ready hint. Codex pass-1 #7 caught the original `=0`/`NaN`-uncapped semantics as dangerous (universal convention is "0 means disabled"). Pinned by test/rate-leases-uncapped.test.ts (15 cases). Bug 3 — strip `provider:` prefix at Anthropic SDK call site src/core/minions/handlers/subagent.ts:439, ~:895 `gbrain agent run --model anthropic:claude-sonnet-4-6` pre-fix sent the qualified string straight to client.messages.create which Anthropic rejects with "model not found." New stripProviderPrefix() applies at the one SDK call site; `model` stays qualified everywhere else (persistence, recipe lookup, capability gate). Pinned by 4 new test/subagent-handler.test.ts cases. Approach C — composable system prompt renderer w/ per-tool usage_hint src/core/minions/system-prompt.ts (NEW) src/core/minions/types.ts (ToolDef.usage_hint + SubagentHandlerData.system_no_tool_preamble) src/core/minions/tools/brain-allowlist.ts (BRAIN_TOOL_USAGE_HINTS) src/core/minions/handlers/subagent.ts (wiring) Bug 4 absorbed: pre-v0.41 DEFAULT_SYSTEM was one generic line that gave the model no guidance on WHICH tool to reach for. The field-report case was a `shell` tool sitting unused because nothing told the model to reach for it. New deterministic renderer splices a tool-usage preamble listing each tool's name + usage_hint; closing paragraph names shell/bash explicitly + tells the model brain tools write to the DB (not local files). Determinism preserved for Anthropic prompt-cache marker stability. Pinned by 13 cases in test/system-prompt.test.ts (determinism, opt-out, plugin tools, cache safety). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/core/minions/handlers/subagent.ts | 72 ++++++++- src/core/minions/system-prompt.ts | 111 ++++++++++++++ src/core/minions/tools/brain-allowlist.ts | 33 +++++ src/core/minions/types.ts | 26 ++++ test/rate-leases-uncapped.test.ts | 160 ++++++++++++++++++++ test/subagent-handler.test.ts | 43 ++++++ test/system-prompt.test.ts | 170 ++++++++++++++++++++++ 7 files changed, 611 insertions(+), 4 deletions(-) create mode 100644 src/core/minions/system-prompt.ts create mode 100644 test/rate-leases-uncapped.test.ts create mode 100644 test/system-prompt.test.ts diff --git a/src/core/minions/handlers/subagent.ts b/src/core/minions/handlers/subagent.ts index 5dda236aa..fcead52f3 100644 --- a/src/core/minions/handlers/subagent.ts +++ b/src/core/minions/handlers/subagent.ts @@ -48,6 +48,7 @@ import { logSubagentHeartbeat, } from './subagent-audit.ts'; import { resolveModel, isAnthropicProvider, TIER_DEFAULTS } from '../../model-config.ts'; +import { buildSystemPrompt, DEFAULT_SUBAGENT_SYSTEM } from '../system-prompt.ts'; import { toolLoop as gatewayToolLoop } from '../../ai/gateway.ts'; import type { ChatToolDef, ChatMessage, ChatBlock, ChatResult, ToolHandler } from '../../ai/gateway.ts'; import { classifyCapabilities } from '../../ai/capabilities.ts'; @@ -58,9 +59,36 @@ import { randomUUIDv7 } from 'bun'; const DEFAULT_MODEL = 'claude-sonnet-4-6'; const DEFAULT_MAX_TURNS = 20; const DEFAULT_RATE_KEY = 'anthropic:messages'; -const DEFAULT_MAX_CONCURRENT = Number(process.env.GBRAIN_ANTHROPIC_MAX_INFLIGHT ?? '8'); + +/** + * Resolve the rate-lease cap from the env var. + * + * undefined → 32 (default; was 8 pre-v0.41, starved 10-concurrency batches) + * "unlimited" → POSITIVE_INFINITY (Azure / Bedrock / self-hosted with no upstream cap) + * "none" → POSITIVE_INFINITY (alias) + * positive number → that number + * anything else → throws (NaN / "0" / negative / typo — fail loud, NOT silent uncap) + * + * Codex pass-1 #7 caught the original `=0` and `NaN` silently uncapping; + * "0 means disabled" is the universal convention, so we use an explicit + * `unlimited` sentinel instead. Misconfig fails at startup with a hint. + */ +export function resolveLeaseCap(raw: string | undefined): number { + if (raw === undefined) return 32; + if (raw === 'unlimited' || raw === 'none') return Number.POSITIVE_INFINITY; + const n = Number(raw); + if (Number.isFinite(n) && n > 0) return n; + throw new Error( + `GBRAIN_ANTHROPIC_MAX_INFLIGHT="${raw}" is invalid. ` + + `Use a positive integer, "unlimited" (or "none"), or omit for default 32.`, + ); +} +const DEFAULT_MAX_CONCURRENT = resolveLeaseCap(process.env.GBRAIN_ANTHROPIC_MAX_INFLIGHT); const DEFAULT_LEASE_TTL_MS = 120_000; -const DEFAULT_SYSTEM = 'You are a helpful assistant running as a gbrain subagent.'; +// v0.41 Approach C: DEFAULT_SUBAGENT_SYSTEM lives in ./system-prompt.ts +// so the renderer and the handler share one source of truth. Kept as +// a re-export alias here for back-compat with any external importer. +const DEFAULT_SYSTEM = DEFAULT_SUBAGENT_SYSTEM; // ── Injectable surfaces (for tests) ───────────────────────── @@ -184,7 +212,11 @@ export function makeSubagentHandler(deps: SubagentDeps) { fallback: TIER_DEFAULTS.subagent, }); const maxTurns = data.max_turns ?? DEFAULT_MAX_TURNS; - const systemPrompt = data.system ?? DEFAULT_SYSTEM; + // v0.41 Approach C: systemPrompt is now built AFTER toolDefs (a few + // lines below) so the renderer can splice a tool-usage preamble + // listing each available tool's usage_hint. The renderer is + // deterministic so the Anthropic prompt-cache marker on the system + // block stays a hit across turns. // v0.38 S1.10 — feature flag for the gateway-native tool loop. When ON, // route ALL subagent jobs through gateway.toolLoop() (works for every @@ -219,6 +251,13 @@ export function makeSubagentHandler(deps: SubagentDeps) { ? filterAllowedTools(registry, data.allowed_tools) : registry; + // v0.41 Approach C: render the final system prompt now that toolDefs + // is known. Splices a deterministic tool-usage preamble listing each + // tool's usage_hint. Caller can opt out via data.system_no_tool_preamble. + const systemPrompt = buildSystemPrompt(toolDefs, data.system, { + no_tool_preamble: data.system_no_tool_preamble, + }); + logSubagentSubmission({ caller: 'worker', remote: true, @@ -437,7 +476,10 @@ export function makeSubagentHandler(deps: SubagentDeps) { // complexity; for v0.15 we lean on the 120s TTL + abort-on-signal. try { const params: Anthropic.MessageCreateParamsNonStreaming = { - model, + // v0.41 Bug 3: strip `provider:` prefix at the SDK call site only. + // `model` stays qualified everywhere else (persistence, recipe + // lookup at recipeIdFromModel(), capability gate). + model: stripProviderPrefix(model), max_tokens: 4096, system: [ { type: 'text', text: systemPrompt, cache_control: { type: 'ephemeral' } }, @@ -897,6 +939,28 @@ function recipeIdFromModel(modelString: string): string { return idx > 0 ? modelString.slice(0, idx) : 'anthropic'; } +/** + * Strip the `provider:` prefix from a model string. Returns the bare + * model id the Anthropic Messages API expects. Idempotent on already-bare + * strings. + * + * stripProviderPrefix('anthropic:claude-sonnet-4-6') === 'claude-sonnet-4-6' + * stripProviderPrefix('claude-sonnet-4-6') === 'claude-sonnet-4-6' + * + * v0.41 Bug 3 — pre-fix, `gbrain agent run --model anthropic:claude-sonnet-4-6` + * sent the prefixed string straight into `client.messages.create()`, which + * Anthropic rejects with "model not found." Omitting `--model` worked because + * `resolveModel()` returns the bare id; explicit-model users hit the bug. + * + * Used ONLY at the SDK call site. The wider `model` variable stays + * qualified everywhere else (persistence, recipe lookup, capability gate) + * because those readers want the provider info. + */ +export function stripProviderPrefix(modelString: string): string { + const idx = modelString.indexOf(':'); + return idx > 0 ? modelString.slice(idx + 1) : modelString; +} + /** * D5 — adapt v1 Anthropic content blocks to v2 ChatBlock shape on read. * Symmetric in the other direction is handled by persisting ChatBlock[] as-is diff --git a/src/core/minions/system-prompt.ts b/src/core/minions/system-prompt.ts new file mode 100644 index 000000000..eabf9a27a --- /dev/null +++ b/src/core/minions/system-prompt.ts @@ -0,0 +1,111 @@ +/** + * v0.41 Approach C — composable subagent system prompt renderer. + * + * Field-report case: a `shell` tool sat in the registry and the subagent + * never used it because the default system prompt was one generic line + * (`'You are a helpful assistant running as a gbrain subagent.'`) and gave + * the model no guidance on WHICH tool to reach for. This module fixes that + * by splicing a deterministic tool-guidance preamble into the system prompt + * based on the actual `toolDefs` array, including any plugin-registered + * tools the core has no knowledge of. + * + * Three properties matter for the Anthropic prompt-cache contract: + * + * 1. Deterministic — same `(toolDefs, userSystem, opts)` input produces + * byte-identical output. The Anthropic `cache_control: ephemeral` + * marker on the system block wraps OUR rendered string; if rendering + * drifted across runs, the cache marker would miss on every turn. + * 2. Generative — works for plugin tools the core has never heard of. + * A downstream plugin that registers `playwright_navigate` with a + * `usage_hint` gets that hint surfaced automatically. + * 3. Opinionated — the closing paragraph names `shell`/`bash` explicitly + * AND tells the model that brain tools (`put_page`, `search`, `query`) + * write to the brain DB, not the local filesystem. Both halves of the + * field-report bug (subagent didn't reach for shell + subagent wrote + * brain pages when files were wanted) collapse into one fix. + * + * Override paths preserved: caller's `data.system` overrides the default + * generic line; `data.system_no_tool_preamble: true` skips the preamble + * splice entirely so the caller's prompt stays byte-for-byte as provided. + */ + +import type { ToolDef } from './types.ts'; + +export const DEFAULT_SUBAGENT_SYSTEM = + 'You are a helpful assistant running as a gbrain subagent.'; + +export interface SystemPromptOpts { + /** Skip the auto-generated tool guidance preamble. */ + no_tool_preamble?: boolean; +} + +/** + * Render the final system prompt sent to the Anthropic Messages API. + * + * Composition order: + * - userSystem (or DEFAULT) — the caller's preamble. + * - blank line. + * - tool guidance preamble (when toolDefs is non-empty AND opts.no_tool_preamble !== true). + * + * Output is fully deterministic for a given input — required for the + * Anthropic prompt-cache marker on the system block. + */ +export function buildSystemPrompt( + toolDefs: ToolDef[], + userSystem: string | undefined, + opts: SystemPromptOpts = {}, +): string { + const base = userSystem ?? DEFAULT_SUBAGENT_SYSTEM; + if (opts.no_tool_preamble || toolDefs.length === 0) return base; + return base + '\n\n' + renderToolPreamble(toolDefs); +} + +/** + * Render the tool-usage preamble. Pure function; no I/O. + * + * Preamble shape (exact bytes, do not reorder — cache-marker stability): + * + * You have the following tools available. Reach for them by default — do NOT + * describe file contents, hypothetical shell output, or planned database + * writes in prose. Call the tool. + * + * - `tool_name` — usage_hint (when present) + * - `tool_name` (when no usage_hint) + * ... + * + * When the task asks you to write a file, run a command, or modify the + * filesystem, prefer a `shell` or `bash` tool if one is in your registry. + * Brain tools (`put_page`, `search`, `query`) write to the gbrain database, + * not to local files. + * + * Tools are listed in the order the caller passed them — NO sorting. The + * caller's order matches the order in the Anthropic `tools:` array, which + * matches the order the model sees in its tool-choice context. Sorting + * would diverge from that and confuse cache-marker reasoning. + */ +export function renderToolPreamble(toolDefs: ToolDef[]): string { + const lines: string[] = []; + for (const t of toolDefs) { + if (t.usage_hint && t.usage_hint.trim()) { + // Normalize any embedded whitespace/newlines so the rendered line + // stays single-line. Defense against a plugin author shipping a + // multi-line hint (which would corrupt the preamble layout). + const hint = t.usage_hint.replace(/\s+/g, ' ').trim(); + lines.push(`- \`${t.name}\` — ${hint}`); + } else { + lines.push(`- \`${t.name}\``); + } + } + return [ + 'You have the following tools available. Reach for them by default — do NOT', + 'describe file contents, hypothetical shell output, or planned database', + 'writes in prose. Call the tool.', + '', + ...lines, + '', + 'When the task asks you to write a file, run a command, or modify the', + 'filesystem, prefer a `shell` or `bash` tool if one is in your registry.', + 'Brain tools (`put_page`, `search`, `query`) write to the gbrain database,', + 'not to local files.', + ].join('\n'); +} diff --git a/src/core/minions/tools/brain-allowlist.ts b/src/core/minions/tools/brain-allowlist.ts index cedd4f46e..f01dfe595 100644 --- a/src/core/minions/tools/brain-allowlist.ts +++ b/src/core/minions/tools/brain-allowlist.ts @@ -66,6 +66,36 @@ export const BRAIN_TOOL_ALLOWLIST: ReadonlySet = new Set([ 'find_anomalies', ]); +/** + * v0.41 Approach C: per-tool usage_hint surfaced verbatim in the subagent + * system prompt's tool preamble. Each entry tells the model WHEN to reach + * for the tool (the description tells the model HOW). One line per tool; + * no embedded newlines. + * + * Field-report driver: the renderer in `src/core/minions/system-prompt.ts` + * surfaces these so a model with `shell` + brain tools in its registry + * knows brain tools write to the gbrain DB (NOT local files) and to reach + * for shell when the task asks for filesystem work. + * + * Keyed by OP name (pre-`brain_` prefix). Optional — tools without an entry + * just render as `- \`name\`` with no hint suffix. + */ +export const BRAIN_TOOL_USAGE_HINTS: Readonly> = { + query: 'Use for natural-language semantic search across the brain (vector + keyword hybrid). Returns ranked passages with citations. First choice when the user asks a question of the brain.', + search: 'Use for hybrid keyword + vector search returning ranked page hits. Use over `query` when you want page-level not chunk-level results (e.g. "find pages about X").', + get_page: 'Read a brain page by its slug. Returns the full markdown body + frontmatter + linked pages.', + list_pages: 'List pages by type or slug-prefix filter. Use when you need to enumerate (e.g. "list all `people/` pages") instead of search.', + file_list: 'List uploaded files (attachments) by slug-prefix or content type. NOT the local filesystem — only files the brain has stored.', + file_url: 'Get a presigned URL for a brain-stored file. Read-only; expires.', + get_backlinks: 'List every page that links TO the given slug. Use for "what references this".', + traverse_graph: 'Walk the typed-edge graph starting from a slug (e.g. `works_at`, `founded`, `invested_in`). Use for relationship queries.', + resolve_slugs: 'Resolve free-form entity names to canonical slugs (e.g. "Alice" → `people/alice-example`). Use before any tool that takes a slug if the user gave a name not a slug.', + get_ingest_log: 'Read the brain ingestion log for diagnostic / verification queries.', + put_page: 'Write a markdown page to the gbrain DATABASE (NOT the local filesystem). Page becomes searchable + linkable. Slug must match the agent\'s allowed namespace.', + get_recent_salience: 'Read pages ranked by emotional + activity salience over a recency window. Use for "what\'s been on my mind lately".', + find_anomalies: 'Read cohort-level activity outliers (e.g. tag-cohort or type-cohort with unusual recent volume). Use for "what\'s unusual lately".', +}; + /** Matches Anthropic's tool-name constraint. No dots. */ const ANTHROPIC_NAME_RE = /^[a-zA-Z0-9_-]{1,64}$/; @@ -223,6 +253,9 @@ export function buildBrainTools(opts: BuildBrainToolsOpts): ToolDef[] { // v0.15 ships only idempotent brain tools (every allow-listed op is // deterministic over its input; put_page re-writes the same slug). idempotent: true, + // v0.41 Approach C: surface usage_hint to the system-prompt renderer. + // Keyed by the unprefixed op name. Undefined when no hint is registered. + usage_hint: BRAIN_TOOL_USAGE_HINTS[op.name], async execute(input: unknown, ctx: ToolCtx): Promise { const opCtx = buildOpContext({ engine: ctx.engine, diff --git a/src/core/minions/types.ts b/src/core/minions/types.ts index b5942103b..bec5ce4e3 100644 --- a/src/core/minions/types.ts +++ b/src/core/minions/types.ts @@ -449,6 +449,18 @@ export interface SubagentHandlerData { * and direct CLI submitters set it. */ allowed_slug_prefixes?: string[]; + /** + * v0.41 Approach C: opt out of the auto-generated tool-usage preamble + * that `buildSystemPrompt()` splices into `system`. Default behavior + * (omitted or false) prepends a deterministic preamble listing each + * tool's name + usage_hint. Set to `true` to keep `system` byte-for-byte + * as provided. + * + * Use when the caller has hand-tuned a complete system prompt for a + * specific subagent (no benefit from auto-generated guidance, prompt + * cache hits ride entirely on the caller-supplied prefix). + */ + system_no_tool_preamble?: boolean; } /** @@ -499,6 +511,20 @@ export interface ToolDef { input_schema: Record; idempotent: boolean; execute(input: unknown, ctx: ToolCtx): Promise; + /** + * v0.41 Approach C: one-line hint surfaced verbatim in the subagent + * system prompt's tool preamble. Tells the model WHEN to use this tool. + * The `description` tells the model HOW; the `usage_hint` tells WHEN. + * + * Field-report case: a `shell` tool sat in the registry and the subagent + * never used it because nothing told the model "to write a file, use + * shell." Per-tool hints surface that directly. Plugin authors get this + * affordance for free. + * + * Optional — omitted tools just don't get a hint line. Must be a single + * line (no embedded newlines) so the rendered preamble stays scannable. + */ + usage_hint?: string; } /** diff --git a/test/rate-leases-uncapped.test.ts b/test/rate-leases-uncapped.test.ts new file mode 100644 index 000000000..5b81b735f --- /dev/null +++ b/test/rate-leases-uncapped.test.ts @@ -0,0 +1,160 @@ +/** + * v0.41 Bug 1: `unlimited` sentinel + POSITIVE_INFINITY semantics for the + * Anthropic rate-lease cap. Closes the field-report bug where a default cap + * of 8 starved a 10-concurrency batch on an Azure-Sweden endpoint that had + * no upstream rate limit. + * + * Pure-function tests for `resolveLeaseCap()` cover the input matrix + * (default, "unlimited", "none", positive int, NaN, zero, negative, typo). + * + * Integration tests against PGLite verify `acquireLease(..., Infinity)` + * always returns acquired=true, still inserts the lease row (so TTL + * pruning + crash recovery still work), and parallel acquires don't + * deadlock on the advisory lock path. + * + * Codex pass-1 #7 caught the original `=0` + `NaN`-as-uncapped semantics + * as dangerous (universal convention is "0 means disabled"); these tests + * pin the corrected fail-loud behavior. + */ + +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { MinionQueue } from '../src/core/minions/queue.ts'; +import { acquireLease, renewLease } from '../src/core/minions/rate-leases.ts'; +import { resolveLeaseCap } from '../src/core/minions/handlers/subagent.ts'; + +describe('resolveLeaseCap (pure)', () => { + test('undefined returns default 32 (was 8 pre-v0.41)', () => { + expect(resolveLeaseCap(undefined)).toBe(32); + }); + + test('"unlimited" returns POSITIVE_INFINITY', () => { + expect(resolveLeaseCap('unlimited')).toBe(Number.POSITIVE_INFINITY); + }); + + test('"none" returns POSITIVE_INFINITY (alias)', () => { + expect(resolveLeaseCap('none')).toBe(Number.POSITIVE_INFINITY); + }); + + test('positive integer returns that integer', () => { + expect(resolveLeaseCap('50')).toBe(50); + expect(resolveLeaseCap('1')).toBe(1); + }); + + test('positive float returns that float', () => { + // Implementation note: SQL int conversion will truncate, but the parser + // itself stays Number-typed. The `> 0` guard accepts floats. + expect(resolveLeaseCap('2.5')).toBe(2.5); + }); + + test('"0" THROWS (universal "0 means disabled" convention; codex #7 fix)', () => { + expect(() => resolveLeaseCap('0')).toThrow(/invalid/); + expect(() => resolveLeaseCap('0')).toThrow(/unlimited/); + }); + + test('negative number THROWS', () => { + expect(() => resolveLeaseCap('-5')).toThrow(/invalid/); + }); + + test('typo THROWS loudly with hint (NOT silent uncap)', () => { + expect(() => resolveLeaseCap('trnety')).toThrow(/invalid/); + expect(() => resolveLeaseCap('trnety')).toThrow(/unlimited/); + }); + + test('empty string THROWS', () => { + // Number('') is 0, which fails the > 0 guard. + expect(() => resolveLeaseCap('')).toThrow(/invalid/); + }); + + test('whitespace-only THROWS', () => { + expect(() => resolveLeaseCap(' ')).toThrow(/invalid/); + }); + + test('hint mentions the invalid input verbatim for paste-back debugging', () => { + try { + resolveLeaseCap('garbage'); + throw new Error('expected throw'); + } catch (e) { + expect((e as Error).message).toContain('"garbage"'); + } + }); +}); + +describe('acquireLease with cap=POSITIVE_INFINITY (integration)', () => { + let engine: PGLiteEngine; + let queue: MinionQueue; + let owner: number; + + beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({ database_url: '' }); + await engine.initSchema(); + queue = new MinionQueue(engine); + }); + + afterAll(async () => { + await engine.disconnect(); + }); + + beforeEach(async () => { + await engine.executeRaw('DELETE FROM subagent_rate_leases'); + await engine.executeRaw('DELETE FROM minion_jobs'); + const j = await queue.add('owner', {}); + owner = j.id; + }); + + test('always returns acquired=true regardless of activeCount', async () => { + // Drive the lease table to 50 active leases under Infinity cap. + for (let i = 0; i < 50; i++) { + const j = await queue.add('owner', {}); + const r = await acquireLease(engine, 'anthropic:messages', j.id, Number.POSITIVE_INFINITY); + expect(r.acquired).toBe(true); + expect(r.maxConcurrent).toBe(Number.POSITIVE_INFINITY); + } + // 51st acquire still wins. + const r = await acquireLease(engine, 'anthropic:messages', owner, Number.POSITIVE_INFINITY); + expect(r.acquired).toBe(true); + }); + + test('lease row is still inserted (so TTL pruning + crash recovery still work)', async () => { + const r = await acquireLease(engine, 'anthropic:messages', owner, Number.POSITIVE_INFINITY, { + ttlMs: 10_000, + }); + expect(r.acquired).toBe(true); + expect(r.leaseId).toBeDefined(); + const rows = await engine.executeRaw<{ count: string }>( + 'SELECT count(*)::text AS count FROM subagent_rate_leases WHERE id = $1', + [r.leaseId!], + ); + expect(parseInt(rows[0]!.count, 10)).toBe(1); + }); + + test('renewLease still works under Infinity cap', async () => { + const r = await acquireLease(engine, 'anthropic:messages', owner, Number.POSITIVE_INFINITY, { + ttlMs: 10_000, + }); + expect(r.acquired).toBe(true); + const renewed = await renewLease(engine, r.leaseId!, 20_000); + expect(renewed).toBe(true); + }); + + test('parallel acquires under Infinity cap do not deadlock on advisory lock', async () => { + const jobs: Array> = []; + for (let i = 0; i < 10; i++) jobs.push(queue.add('owner', {})); + const owners = (await Promise.all(jobs)).map(j => j.id); + // Fire 10 parallel acquires on the same key under Infinity cap. + const acquires = await Promise.all( + owners.map(o => acquireLease(engine, 'anthropic:messages', o, Number.POSITIVE_INFINITY)), + ); + // All 10 should win. + for (const r of acquires) { + expect(r.acquired).toBe(true); + expect(r.leaseId).toBeDefined(); + } + // And the table should have 10 rows. + const rows = await engine.executeRaw<{ count: string }>( + `SELECT count(*)::text AS count FROM subagent_rate_leases WHERE key = 'anthropic:messages'`, + ); + expect(parseInt(rows[0]!.count, 10)).toBe(10); + }); +}); diff --git a/test/subagent-handler.test.ts b/test/subagent-handler.test.ts index e76336304..145cfc3d8 100644 --- a/test/subagent-handler.test.ts +++ b/test/subagent-handler.test.ts @@ -18,6 +18,7 @@ import { MinionQueue } from '../src/core/minions/queue.ts'; import { makeSubagentHandler, RateLeaseUnavailableError, + stripProviderPrefix, type MessagesClient, } from '../src/core/minions/handlers/subagent.ts'; import type { ToolDef, MinionJobContext } from '../src/core/minions/types.ts'; @@ -468,6 +469,48 @@ describe('subagent handler lease behavior', () => { expect(parseInt(rows[0]!.c, 10)).toBe(0); }); + test('v0.41 Bug 3: stripProviderPrefix strips `anthropic:` qualified model', async () => { + expect(stripProviderPrefix('anthropic:claude-sonnet-4-6')).toBe('claude-sonnet-4-6'); + }); + + test('v0.41 Bug 3: stripProviderPrefix is idempotent on bare names', async () => { + expect(stripProviderPrefix('claude-sonnet-4-6')).toBe('claude-sonnet-4-6'); + }); + + test('v0.41 Bug 3: stripProviderPrefix handles edge inputs', async () => { + expect(stripProviderPrefix('')).toBe(''); + // Leading colon = no valid provider name; pass through unchanged. + // The `idx > 0` guard (not `>= 0`) makes this intentional. + expect(stripProviderPrefix(':')).toBe(':'); + expect(stripProviderPrefix('a:b:c')).toBe('b:c'); // only strips first prefix + }); + + test('v0.41 Bug 3: handler passes bare model id to Anthropic SDK when data.model is qualified', async () => { + const calls: Array = []; + const client: MessagesClient = { + async create(params) { + calls.push(params); + return { + content: [{ type: 'text', text: 'ok' }], + stop_reason: 'end_turn', + usage: { input_tokens: 1, output_tokens: 1 }, + role: 'assistant', + } as unknown as Anthropic.Message; + }, + }; + const handler = makeSubagentHandler({ + engine, client, toolRegistry: [], maxConcurrent: 100, rateLeaseKey: 'k_prefix', + }); + const ctx = await makeCtx({ + prompt: 'hello', + model: 'anthropic:claude-sonnet-4-6', // qualified — the field-report bug case + }); + await handler(ctx); + expect(calls.length).toBe(1); + // The SDK MUST receive the bare model id, not the prefixed one. + expect(calls[0]!.model).toBe('claude-sonnet-4-6'); + }); + test('throws RateLeaseUnavailableError when cap full', async () => { // Preload the cap with a stale-looking-but-live lease owned by a // different job. diff --git a/test/system-prompt.test.ts b/test/system-prompt.test.ts new file mode 100644 index 000000000..2072ff41e --- /dev/null +++ b/test/system-prompt.test.ts @@ -0,0 +1,170 @@ +/** + * v0.41 Approach C — system-prompt renderer unit tests. + * + * Pins the deterministic-output contract that the Anthropic prompt-cache + * marker on the system block depends on. Drifting the renderer's output + * across invocations would silently miss the cache on every turn. + * + * Covers: + * - DEFAULT_SUBAGENT_SYSTEM fallback when userSystem is undefined. + * - Empty toolDefs → no preamble splice (falls through to bare system). + * - Single tool, no usage_hint → bare list entry. + * - Single tool, with usage_hint → list entry with " — hint" suffix. + * - Multi-tool — order preserved from input (no sort). + * - usage_hint with embedded newlines → normalized to single line. + * - usage_hint whitespace-only string → treated as missing. + * - no_tool_preamble opt-out → returns userSystem unchanged. + * - Determinism — two calls with identical input produce byte-identical output. + * - Plugin tool integration — registry-agnostic, works for any ToolDef. + * - Closing paragraph naming shell/bash + brain DB distinction present. + * - userSystem override fully preserved as the leading bytes (cache-hit safety). + */ + +import { describe, test, expect } from 'bun:test'; +import { + buildSystemPrompt, + renderToolPreamble, + DEFAULT_SUBAGENT_SYSTEM, +} from '../src/core/minions/system-prompt.ts'; +import type { ToolDef } from '../src/core/minions/types.ts'; + +function fakeTool(name: string, opts: { usage_hint?: string } = {}): ToolDef { + return { + name, + description: `description of ${name}`, + input_schema: { type: 'object' as const }, + idempotent: true, + usage_hint: opts.usage_hint, + async execute() { + return null; + }, + }; +} + +describe('buildSystemPrompt', () => { + test('empty toolDefs + undefined userSystem → returns the DEFAULT bare system', () => { + const out = buildSystemPrompt([], undefined); + expect(out).toBe(DEFAULT_SUBAGENT_SYSTEM); + }); + + test('empty toolDefs + custom userSystem → returns userSystem unchanged (no preamble)', () => { + const userSystem = 'You are a curator of brand archives.'; + const out = buildSystemPrompt([], userSystem); + expect(out).toBe(userSystem); + }); + + test('no_tool_preamble=true keeps userSystem byte-for-byte even with tools', () => { + const userSystem = 'Hand-tuned system prompt that should not be modified.'; + const tools = [fakeTool('search', { usage_hint: 'do searches' })]; + const out = buildSystemPrompt(tools, userSystem, { no_tool_preamble: true }); + expect(out).toBe(userSystem); + }); + + test('tools without usage_hint render as bare list entries', () => { + const tools = [fakeTool('search'), fakeTool('get_page')]; + const out = buildSystemPrompt(tools, undefined); + expect(out).toContain('- `search`'); + expect(out).toContain('- `get_page`'); + expect(out).not.toContain('- `search` —'); + expect(out).not.toContain('- `get_page` —'); + }); + + test('tools with usage_hint render with " — hint" suffix', () => { + const tools = [fakeTool('shell', { usage_hint: 'Run a shell command.' })]; + const out = buildSystemPrompt(tools, undefined); + expect(out).toContain('- `shell` — Run a shell command.'); + }); + + test('preamble preserves caller-supplied tool order (no sort)', () => { + const tools = [ + fakeTool('z_last', { usage_hint: 'late' }), + fakeTool('a_first', { usage_hint: 'early' }), + ]; + const out = buildSystemPrompt(tools, undefined); + const idxZ = out.indexOf('- `z_last`'); + const idxA = out.indexOf('- `a_first`'); + expect(idxZ).toBeGreaterThan(-1); + expect(idxA).toBeGreaterThan(-1); + expect(idxZ).toBeLessThan(idxA); // input order preserved + }); + + test('usage_hint with embedded newlines is normalized to single line', () => { + const tools = [ + fakeTool('shell', { + usage_hint: 'Line one.\nLine two.\n\tIndented line three.', + }), + ]; + const out = buildSystemPrompt(tools, undefined); + // Whole hint collapses to single line; preamble layout intact. + expect(out).toContain('- `shell` — Line one. Line two. Indented line three.'); + // No literal newlines inside the hint line. + const hintLine = out.split('\n').find(l => l.startsWith('- `shell`')); + expect(hintLine).toBeDefined(); + expect(hintLine!).not.toContain('\n'); + }); + + test('whitespace-only usage_hint treated as missing', () => { + const tools = [fakeTool('foo', { usage_hint: ' \t ' })]; + const out = buildSystemPrompt(tools, undefined); + expect(out).toContain('- `foo`'); + expect(out).not.toContain('- `foo` —'); + }); + + test('closing paragraph names shell/bash + brain DB distinction (field-report fix)', () => { + const tools = [fakeTool('shell', { usage_hint: 'Run commands.' })]; + const out = buildSystemPrompt(tools, undefined); + expect(out).toContain('`shell` or `bash` tool'); + expect(out).toContain('gbrain database'); + expect(out).toContain('not to local files'); + }); + + test('userSystem is the leading bytes of the output (Anthropic prompt-cache safety)', () => { + const userSystem = 'Custom-tuned-prefix-for-cache-hit-stability.'; + const tools = [fakeTool('search', { usage_hint: 'do searches' })]; + const out = buildSystemPrompt(tools, userSystem); + expect(out.startsWith(userSystem)).toBe(true); + // Two newlines between userSystem and the preamble. + expect(out.startsWith(userSystem + '\n\n')).toBe(true); + }); + + test('determinism: identical input → byte-identical output', () => { + const tools = [ + fakeTool('a', { usage_hint: 'do A' }), + fakeTool('b'), + fakeTool('c', { usage_hint: 'do C' }), + ]; + const userSystem = 'System.'; + const a = buildSystemPrompt(tools, userSystem); + const b = buildSystemPrompt(tools, userSystem); + expect(a).toBe(b); + expect(a.length).toBe(b.length); + }); + + test('plugin tool (registry-agnostic) gets its usage_hint surfaced', () => { + // Simulate a downstream OpenClaw plugin that registers a custom tool. + const tools = [ + fakeTool('playwright_navigate', { + usage_hint: 'Drive a browser to a URL and wait for load.', + }), + ]; + const out = buildSystemPrompt(tools, undefined); + expect(out).toContain('- `playwright_navigate` — Drive a browser to a URL and wait for load.'); + }); +}); + +describe('renderToolPreamble (pure)', () => { + test('renders header + bullets + footer in canonical order', () => { + const tools = [fakeTool('search', { usage_hint: 'do searches' })]; + const preamble = renderToolPreamble(tools); + const lines = preamble.split('\n'); + // Header is first 3 lines. + expect(lines[0]).toContain('You have the following tools available'); + expect(lines[1]).toContain('describe file contents'); + expect(lines[2]).toContain('Call the tool'); + // Then a blank line, then the tool list, then a blank, then the closer. + expect(lines[3]).toBe(''); + expect(lines[4]).toContain('- `search`'); + expect(lines[5]).toBe(''); + expect(lines[6]).toContain('When the task asks you to write a file'); + }); +});