diff --git a/agents/__tests__/base2-progressive-tool-disclosure.test.ts b/agents/__tests__/base2-progressive-tool-disclosure.test.ts index 9b2723a95e..70a72b0594 100644 --- a/agents/__tests__/base2-progressive-tool-disclosure.test.ts +++ b/agents/__tests__/base2-progressive-tool-disclosure.test.ts @@ -53,6 +53,7 @@ const CORE_ALWAYS = [ 'read_subtree', 'list_directory', 'glob', + 'code_search', 'skill', 'suggest_followups', 'list_jobs', @@ -147,6 +148,11 @@ describe('base2 progressive tool disclosure (M1)', () => { expect(tools).toContain('edit_3d_asset') expect(tools).toContain('create_plan') expect(tools).toContain('run_targeted_validation') + expect(tools).toContain('code_search') + }) + + test('CORE_TOOLS includes root content-search tool', () => { + expect(CORE_TOOLS).toContain('code_search') }) test('flag on core-only: CORE present; IMPLEMENT/AUDIT/MEDIA/JOB_EXTRA absent', () => { @@ -755,9 +761,56 @@ describe('tier resolution helpers (M1-T3)', () => { // instead of silently narrowing the runtime tool surface. describe('base2 tier membership — runtime mirror stays in sync', () => { test('BASE2_CORE_TOOL_NAMES equals CORE_TOOLS', () => { + // CORE_TOOLS re-exports BASE2_CORE_TOOL_NAMES by construction, so this is + // intentionally vacuous — it cannot catch a drift. The real progressive + // core-only surface lives in the hand-encoded CORE buildArray inside + // resolveModelToolNames; the tests below tie THAT copy (and the other + // mode-gated modes) to the runtime constant so a one-sided edit fails. expect([...BASE2_CORE_TOOL_NAMES]).toEqual([...CORE_TOOLS]) }) + test('progressive core-only surface matches BASE2_CORE_TOOL_NAMES exactly', () => { + // resolveModelToolNames' progressive CORE buildArray is a SECOND copy of + // CORE membership that no test previously exercised. In the default mode + // (ask_user + write_todos both allowed) the surfaced set must equal the + // runtime constant byte-for-byte, so a tool added/removed on either side + // fails loudly instead of silently narrowing/widening the tool surface. + const coreOnly = resolveModelToolNames({ + mode: 'default', + progressiveToolDisclosure: true, + unlockedTiers: [], + }) + // Bidirectional membership over string sets — avoids the ToolName[] sort() + // widening that would break the AllToolNames[] toEqual overload, while still + // making a one-sided edit to either list fail loudly. + const coreSet = new Set(BASE2_CORE_TOOL_NAMES) + expect(coreOnly.length).toBe(coreSet.size) + for (const name of coreOnly) { + expect(coreSet.has(name)).toBe(true) + } + for (const name of BASE2_CORE_TOOL_NAMES) { + expect(coreSet.has(name)).toBe(true) + } + }) + + test('mode-gated CORE tools stay within BASE2_CORE_TOOL_NAMES', () => { + // fast + noAskUser drops the mode-gated CORE tools (ask_user/write_todos). + // Every remaining surfaced name must still be a declared CORE member so + // the mode-gated variants cannot diverge from (or exceed) the constant. + const gated = resolveModelToolNames({ + mode: 'fast', + noAskUser: true, + progressiveToolDisclosure: true, + unlockedTiers: [], + }) + const coreSet = new Set(BASE2_CORE_TOOL_NAMES) + for (const name of gated) { + expect(coreSet.has(name)).toBe(true) + } + expect(gated).not.toContain('ask_user') + expect(gated).not.toContain('write_todos') + }) + test('BASE2_TIER_TOOL_NAMES.implement equals IMPLEMENT_TOOLS', () => { expect([...BASE2_TIER_TOOL_NAMES.implement]).toEqual([...IMPLEMENT_TOOLS]) }) diff --git a/agents/__tests__/base2-writer-spawn-rules.test.ts b/agents/__tests__/base2-writer-spawn-rules.test.ts new file mode 100644 index 0000000000..c1a2bc8ce6 --- /dev/null +++ b/agents/__tests__/base2-writer-spawn-rules.test.ts @@ -0,0 +1,812 @@ +import { readFileSync } from 'node:fs' + +import { describe, expect, test } from 'bun:test' + +import { createBase2 } from '../base2/base2' +import { isTestCoverageReviewerFinding } from '../base2/gate-reviewer' +import { createReviewer } from '../reviewer/code-reviewer' +import { createSpecialist } from '../specialists/create-specialist' +import { editReceipt, feedListJobs } from './helpers/base2-step-fixtures' +import { + extractInlineFunctionSource, + findMatchingDelimiterEnd, +} from './helpers/extract-inline-function-source' + +// SECURITY_SENSITIVE_* mirror the production base2 handleSteps constants that +// `selectAuxRelevantFiles` closes over. They feed the reconstructed inline +// helper preamble AND are parity-guarded against the live base2.ts source (see +// the 'SECURITY_SENSITIVE_* mirror matches production' test below), so a new +// sensitive glob/name added in base2.ts fails the test instead of silently +// using a stale mirror. +const SECURITY_SENSITIVE_GLOBS: string[] = [ + 'auth', + 'oauth', + 'credentials', + 'session', + 'crypto', + 'keys', + 'secrets', + 'vault', + 'billing', + 'payment', + 'stripe', + 'permissions', + 'rbac', + 'policy', +] + +const SECURITY_SENSITIVE_NAME_SUBSTRINGS: string[] = [ + 'secret', + 'token', + 'apikey', +] + +/** + * Extract a top-level `const NAME = [...]` string-array literal (balanced + * brackets) from base2.ts source so the test can parity-check its + * SECURITY_SENSITIVE_* mirror against production. + */ +function extractStringArrayFromSource( + source: string, + constantName: string, +): string[] { + const declarationStart = source.indexOf(`const ${constantName} = `) + if (declarationStart < 0) { + throw new Error(`Unable to find ${constantName} in base2 source`) + } + const bodyStart = source.indexOf('[', declarationStart) + if (bodyStart < 0) { + throw new Error(`Unable to find array body for ${constantName}`) + } + // Reuse the shared quote/comment-aware bracket-balance helper (the same + // primitive the inline-function extraction uses) instead of re-implementing + // a parallel walk here. + const end = findMatchingDelimiterEnd(source, bodyStart, '[', ']') + if (end < 0) { + throw new Error(`Unable to find end of ${constantName} array`) + } + const body = source.slice(bodyStart + 1, end) + // The naive `','` split below would silently split a quoted value that + // itself contains a comma into two tokens, silently weakening the parity + // guard (a future SECURITY_SENSITIVE_* glob containing a comma would be + // observed as two separate tokens and pass even though the mirror no longer + // matched production). Detect a comma inside a quoted string and fail + // loudly instead, so this check can never silently weaken. + let bodyQuote: string | null = null + for (let index = 0; index < body.length; index += 1) { + const character = body[index] + if (character === '\\') { + index += 1 + continue + } + if (character === '"' || character === "'") { + bodyQuote = bodyQuote === character ? null : character + } else if (character === ',' && bodyQuote) { + throw new Error( + `${constantName} value contains a comma; update extractStringArrayFromSource to a quote-delimited tokenizer`, + ) + } + } + const tokens: string[] = [] + for (const raw of body.split(',')) { + const token = raw.trim().replace(/^['"]|['"]$/g, '') + if (token) tokens.push(token) + } + return tokens +} + +/** + * Build the JSON-safe preamble injected ahead of the reconstructed inline + * helpers, derived from the named SECURITY_SENSITIVE_* constants (the same + * values the parity test asserts against production). + */ +function buildConstantsPreamble(): string { + const globsBody = SECURITY_SENSITIVE_GLOBS.map( + (glob) => ` '${glob}',`, + ).join('\n') + const substrsBody = SECURITY_SENSITIVE_NAME_SUBSTRINGS.map( + (substr) => `'${substr}'`, + ).join(', ') + return `\nconst SECURITY_SENSITIVE_GLOBS = [\n${globsBody}\n];\nconst SECURITY_SENSITIVE_NAME_SUBSTRINGS = [${substrsBody}];\n` +} + +type WriterTargetHelpers = { + selectTestWriterTargets: (files: string[]) => { + groups: Array<{ + targetFiles: string[] + testCommand: string + candidateTests: string[] + packageRoot: string + }> + } + selectDocWriterTargets: (files: string[]) => string[] + selectAuxRelevantFiles: (files: string[]) => string[] + testWriterScopePatterns: (packageRoot: string) => string[] + docWriterScopePatterns: (sourceFiles: string[]) => string[] + isNonTestSourceFile: (filePath: string) => boolean + isPublicApiSourceFile: (filePath: string) => boolean + inferPackageTestCommand: (filePath: string) => string | null +} + +/** + * Reconstruct the serialized handleSteps writer-selection helpers so tests pin + * the live spawn predicates without exporting production internals. + */ +function loadInlineWriterTargetHelpers(): WriterTargetHelpers { + const base2Source = readFileSync( + new URL('../base2/base2.ts', import.meta.url), + 'utf8', + ) + const names = [ + 'normalizeGateFilePath', + 'inferPackageTestCommand', + 'isNonTestSourceFile', + 'isPublicApiSourceFile', + 'inferWorkspaceRootFromPath', + 'selectTestWriterTargets', + 'selectDocWriterTargets', + 'testWriterScopePatterns', + 'docWriterScopePatterns', + // selectAuxRelevantFiles closes over matchesSecuritySensitiveGlob helpers. + 'isAlnumChar', + 'basenameContainsSensitiveName', + 'matchesSecuritySensitiveGlob', + 'selectAuxRelevantFiles', + ] as const + + // SECURITY_SENSITIVE_* are local constants closed over by the inline helpers. + // Reconstruct them from the parity-guarded named constants above so the + // mirror stays authoritative against base2.ts. + const constantsPreamble = buildConstantsPreamble() + // NOTE: unlike loadInlineGateRepairHelpers (gate-repair-parity) and + // loadProductionGateFileContentMarker (e2e), which first transpile base2.ts to + // JS before extractInlineFunctionSource, this site extracts from the RAW TS + // source on purpose: these writer-selection helpers are simple and TS-tolerant, + // and the transpile-to-JS step is unnecessary here. Do not "normalize" this + // call site to match the others without verifying it still extracts correctly. + const helperSource = names + .map((functionName) => extractInlineFunctionSource(base2Source, functionName)) + .join('\n\n') + const transpiler = new Bun.Transpiler({ loader: 'ts', target: 'bun' }) + const combinedJs = transpiler.transformSync( + `${constantsPreamble}\n${helperSource}\nreturn {\n selectTestWriterTargets,\n selectDocWriterTargets,\n selectAuxRelevantFiles,\n testWriterScopePatterns,\n docWriterScopePatterns,\n isNonTestSourceFile,\n isPublicApiSourceFile,\n inferPackageTestCommand,\n}`, + ) + const buildHelpers = new Function(`"use strict";\n${combinedJs}`) as () => WriterTargetHelpers + return buildHelpers() +} + +function advanceToPostEditGitStatus( + gen: Generator, + editedFile: string, +) { + // Code-intent prompts may start with query_index; drain until git_status. + let step = gen.next().value as any + let guard = 0 + while (step?.toolName !== 'git_status' && guard++ < 8) { + if (step?.toolName === 'query_index') { + step = gen.next({ toolResult: [{ type: 'json', value: [] }] } as any) + .value as any + continue + } + if (step?.toolName === 'add_message') { + step = gen.next().value as any + continue + } + step = gen.next({ toolResult: [{ type: 'json', value: {} }] } as any) + .value as any + } + expect(step).toMatchObject({ toolName: 'git_status' }) + expect( + gen.next({ toolResult: [{ type: 'json', value: { status: '' } }] } as any) + .value, + ).toMatchObject({ toolName: 'list_jobs' }) + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'spawn_agent_inline', + }) + expect(gen.next().value).toBe('STEP') + expect( + gen.next({ + stepsComplete: true, + toolResult: [{ type: 'json', value: editReceipt(editedFile) }], + } as any).value, + ).toMatchObject({ toolName: 'git_status' }) +} + +describe('base2 writer spawn roster availability', () => { + test('default implementation mode keeps editor, repair-editor, test-writer, and doc-writer spawnable', () => { + const spawnable = createBase2('default').spawnableAgents ?? [] + for (const agent of [ + 'editor', + 'repair-editor', + 'test-writer', + 'doc-writer', + ]) { + expect(spawnable).toContain(agent) + } + }) + + test('fast mode drops editor/repair-editor but keeps test-writer and doc-writer', () => { + const spawnable = createBase2('fast').spawnableAgents ?? [] + expect(spawnable).toContain('test-writer') + expect(spawnable).toContain('doc-writer') + expect(spawnable).not.toContain('editor') + expect(spawnable).not.toContain('repair-editor') + }) + + test('plan mode withholds mutation writers and editors', () => { + const spawnable = + createBase2('default', { planOnly: true }).spawnableAgents ?? [] + for (const agent of [ + 'editor', + 'repair-editor', + 'test-writer', + 'doc-writer', + ]) { + expect(spawnable).not.toContain(agent) + } + }) +}) + +describe('base2 writer target selection predicates', () => { + const helpers = loadInlineWriterTargetHelpers() + + test('selectTestWriterTargets groups non-test source by package test command', () => { + const selection = helpers.selectTestWriterTargets([ + 'agents/base2/base2.ts', + 'agents/__tests__/base2.test.ts', + 'docs/readme.md', + 'common/src/util/array.ts', + 'notes.txt', + ]) + expect(selection.groups).toHaveLength(2) + const agentsGroup = selection.groups.find((group) => + group.targetFiles.includes('agents/base2/base2.ts'), + ) + const commonGroup = selection.groups.find((group) => + group.targetFiles.includes('common/src/util/array.ts'), + ) + expect(agentsGroup?.testCommand).toBe( + 'cd agents && bun run typecheck && bun test', + ) + expect(agentsGroup?.packageRoot).toBe('agents') + expect(commonGroup?.testCommand).toBe( + 'cd common && bun run typecheck && bun test', + ) + expect(commonGroup?.packageRoot).toBe('common') + }) + + test('selectTestWriterTargets returns no groups for empty, test-only, or non-source files', () => { + expect(helpers.selectTestWriterTargets([]).groups).toEqual([]) + expect( + helpers.selectTestWriterTargets([ + 'agents/__tests__/base2.test.ts', + 'docs/guide.md', + 'package.json', + ]).groups, + ).toEqual([]) + }) + + test('selectDocWriterTargets admits public API source across packages, not only one file type', () => { + expect( + helpers.selectDocWriterTargets([ + 'agents/base2/base2.ts', + 'common/src/util/array.ts', + 'packages/agent-runtime/src/tools/edit.ts', + 'agents/__tests__/base2.test.ts', + 'docs/guide.md', + 'README.md', + ]), + ).toEqual([ + 'agents/base2/base2.ts', + 'common/src/util/array.ts', + 'packages/agent-runtime/src/tools/edit.ts', + ]) + }) + + test('docWriterScopePatterns expands package roots so docs can update across the repo', () => { + expect( + helpers.docWriterScopePatterns([ + 'agents/base2/base2.ts', + 'common/src/util/array.ts', + ]), + ).toEqual( + expect.arrayContaining([ + 'agents/docs/**', + 'agents/**/*.md', + 'common/docs/**', + 'common/**/*.md', + ]), + ) + }) + + test('testWriterScopePatterns stays package-scoped to existing test locations', () => { + expect(helpers.testWriterScopePatterns('agents')).toEqual([ + 'agents/**/*.test.*', + 'agents/**/*.spec.*', + 'agents/**/__tests__/**', + 'agents/**/test/**', + 'agents/**/tests/**', + ]) + expect(helpers.testWriterScopePatterns('.')).toEqual([ + '**/*.test.*', + '**/*.spec.*', + '**/__tests__/**', + '**/test/**', + '**/tests/**', + ]) + }) + + test('selectAuxRelevantFiles keeps writer outputs from re-arming aux gates forever', () => { + expect( + helpers.selectAuxRelevantFiles([ + 'agents/base2/base2.ts', + 'agents/__tests__/base2.test.ts', + 'docs/state.md', + 'packages/sdk/src/__tests__/cache.test.ts', + ]), + ).toEqual(['agents/base2/base2.ts']) + }) + + test('selectAuxRelevantFiles reconstruction fails LOUDLY when a closed-over helper or constant is missing', () => { + // selectAuxRelevantFiles closes over multiple local helpers + // (isNonTestSourceFile / inferPackageTestCommand / isPublicApiSourceFile / + // matchesSecuritySensitiveGlob) plus the SECURITY_SENSITIVE_* constants that + // loadInlineWriterTargetHelpers reconstructs from its names list + preamble. + // If production adds a NEW helper/constant reference inside + // selectAuxRelevantFiles that is NOT added to that names list or preamble, + // the reconstruction must throw a ReferenceError at call time (fail + // closed) instead of silently producing wrong aux-relevance output that + // re-arms the gate loop. Build selectAuxRelevantFiles standalone (no helper + // closures / constants) and assert invoking it throws — pinning the + // fail-closed guarantee. + const base2Source = readFileSync( + new URL('../base2/base2.ts', import.meta.url), + 'utf8', + ) + const transpiler = new Bun.Transpiler({ loader: 'ts', target: 'bun' }) + const helperSource = extractInlineFunctionSource( + base2Source, + 'selectAuxRelevantFiles', + ) + const combinedJs = transpiler.transformSync(helperSource) + const buildHelper = new Function( + `"use strict";\n${combinedJs}\nreturn selectAuxRelevantFiles`, + ) as () => (files: string[]) => string[] + const selectAuxStandalone = buildHelper() + expect(() => selectAuxStandalone(['agents/base2/base2.ts'])).toThrow( + ReferenceError, + ) + }) + + test('SECURITY_SENSITIVE_* mirror matches production base2 constants', () => { + // Parity guard: the inline-helper preamble mirrors the production + // SECURITY_SENSITIVE_GLOBS / SECURITY_SENSITIVE_NAME_SUBSTRINGS constants + // that selectAuxRelevantFiles closes over. If base2.ts adds a sensitive + // glob/name, this assertion fails so the mirror is updated deliberately + // instead of silently weakening security-sensitive aux relevance coverage. + const base2Source = readFileSync( + new URL('../base2/base2.ts', import.meta.url), + 'utf8', + ) + expect( + extractStringArrayFromSource(base2Source, 'SECURITY_SENSITIVE_GLOBS'), + ).toEqual(SECURITY_SENSITIVE_GLOBS) + expect( + extractStringArrayFromSource( + base2Source, + 'SECURITY_SENSITIVE_NAME_SUBSTRINGS', + ), + ).toEqual(SECURITY_SENSITIVE_NAME_SUBSTRINGS) + }) +}) + +describe('findMatchingDelimiterEnd line-comment-at-expression-start heuristic (pinned)', () => { + // Pinned behavior for findMatchingDelimiterEnd, which is shared by three + // call sites (extractInlineFunctionSource, extractStringArrayFromSource, and + // the reviewer-spawn-conditions e2e). isRegexStart is narrowed so that `//` + // is ALWAYS treated as the start of a line comment (a regex literal can never + // begin with an unescaped `/`), even at an expression-start position where + // the previous significant char is an operator/opener such as `=` or `{`. + // Without that narrowing, a `//` comment directly after `=` would be consumed + // as an empty regex and a `}` inside the comment text would terminate the + // balance walk early. + // + // These assertions PIN the corrected behavior so a future change to the + // regex/comment heuristic cannot silently alter the extracted slice without a + // deliberate, reviewed update to this test. Cross-referenced from the header + // doc comment in helpers/extract-inline-function-source.ts. + test('a // comment directly after = is treated as a comment; the walk ends at the real closer', () => { + // `}` at index 26 lives inside the `//` comment text. isRegexStart no longer + // misreads `//` as a regex after `=` (a regex whose first char is '/' would + // need to be written escaped), so the comment branch handles it and the + // in-comment `}` is skipped. The walk reaches the real body-closing `}` at + // the end instead of terminating early at index 26. + const src = '{ foo = // comment with a } brace\n bar: "x" }' + const end = findMatchingDelimiterEnd(src, 0, '{', '}') + expect(src[end]).toBe('}') + // The walk must NOT terminate at the comment's `}` (index 26); it continues + // to the final real closing brace. + expect(end).toBe(src.lastIndexOf('}')) + expect(end).toBeGreaterThan(26) + }) +}) + +describe('isRegexStart keyword-aware expression positions (pinned)', () => { + // Pinned behavior for the keyword-aware isRegexStart check (RF-2-dae01659): + // a regex literal directly after an expression-preceding keyword + // (`return /{…}/`, `typeof /…/`) must be consumed as a regex literal so its + // braces cannot terminate or unbalance the delimiter walk. The earlier + // char-only check saw the identifier tail (`n` of `return`, `f` of `typeof`) + // and misread the `/` as division, leaving the regex's own `}` live for the + // balance walk. + test('a regex directly after `return` is skipped; the walk ends at the real closer', () => { + // Without the keyword check, the `/` after `return` would be read as + // division and the regex's own `}` would close the walk early. + const src = '{ x = return /}/g\n }' + const end = findMatchingDelimiterEnd(src, 0, '{', '}') + expect(src[end]).toBe('}') + expect(end).toBe(src.lastIndexOf('}')) + }) + + test('a regex directly after `typeof` is skipped; the walk ends at the real closer', () => { + const src = '{ t = typeof /{a}/g\n }' + const end = findMatchingDelimiterEnd(src, 0, '{', '}') + expect(src[end]).toBe('}') + expect(end).toBe(src.lastIndexOf('}')) + }) +}) + +describe('base2 writer request predicates and sequential aux gates', () => { + test('test-writer spawns only when the user prompt asks for tests', () => { + const base2 = createBase2('default') + const agentState = { agentId: 'base2-custom' } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Make the requested change now please', + params: {}, + config: base2.programmaticConfig, + } as any) + + advanceToPostEditGitStatus(gen, 'src/a.ts') + expect( + gen.next({ + toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], + } as any).value, + ).toMatchObject({ toolName: 'list_jobs' }) + // No explicit test request -> skip inspect_environment writer path and go + // straight to validation hooks / later gates. + const next = gen.next(feedListJobs()).value as any + expect(next.toolName).not.toBe('inspect_environment') + expect(next).toMatchObject({ toolName: 'run_file_change_hooks' }) + }) + + test('negated test phrasing does not spawn test-writer', () => { + const base2 = createBase2('default') + const agentState = { agentId: 'base2-custom' } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Update the parser without tests', + params: {}, + config: base2.programmaticConfig, + } as any) + + advanceToPostEditGitStatus(gen, 'src/a.ts') + expect( + gen.next({ + toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], + } as any).value, + ).toMatchObject({ toolName: 'list_jobs' }) + const next = gen.next(feedListJobs()).value as any + expect(next.toolName).not.toBe('inspect_environment') + expect(next.toolName).not.toBe('spawn_agent_inline') + }) + + test('explicit test request spawns test-writer with package-scoped handoff', () => { + const base2 = createBase2('default') + const agentState = { agentId: 'base2-custom' } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Add tests for the new gate behavior', + params: {}, + config: base2.programmaticConfig, + } as any) + + advanceToPostEditGitStatus(gen, 'agents/base2/base2.ts') + expect( + gen.next({ + toolResult: [ + { + type: 'json', + value: { status: ' M agents/base2/base2.ts' }, + }, + ], + } as any).value, + ).toMatchObject({ toolName: 'list_jobs' }) + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'inspect_environment', + }) + expect( + gen.next({ toolResult: [{ type: 'json', value: {} }] } as any).value, + ).toMatchObject({ toolName: 'get_affected_tests' }) + expect( + gen.next({ toolResult: [{ type: 'json', value: {} }] } as any).value, + ).toMatchObject({ toolName: 'get_build_targets' }) + const testWriterSpawn = gen.next({ + toolResult: [{ type: 'json', value: {} }], + } as any).value as any + expect(testWriterSpawn).toMatchObject({ + toolName: 'spawn_agent_inline', + input: { agent_type: 'test-writer' }, + }) + expect(testWriterSpawn.input.params.target_files).toEqual([ + 'agents/base2/base2.ts', + ]) + expect(testWriterSpawn.input.params.test_command).toBe( + 'cd agents && bun run typecheck && bun test', + ) + expect(testWriterSpawn.input.handoff.permissions.writablePaths).toEqual( + expect.arrayContaining([ + 'agents/**/*.test.*', + 'agents/**/__tests__/**', + ]), + ) + // Inline spawn is sequential/blocking; writers are not launched via + // spawn_agents batching that would run in parallel with each other. + expect(testWriterSpawn.toolName).toBe('spawn_agent_inline') + }) + + test('doc request spawns doc-writer after optional test gate with multi-root write scope', () => { + const base2 = createBase2('default') + const agentState = { agentId: 'base2-custom' } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Document the public API for this change', + params: {}, + config: base2.programmaticConfig, + } as any) + + advanceToPostEditGitStatus(gen, 'common/src/util/array.ts') + expect( + gen.next({ + toolResult: [ + { + type: 'json', + value: { status: ' M common/src/util/array.ts' }, + }, + ], + } as any).value, + ).toMatchObject({ toolName: 'list_jobs' }) + // Docs-only request still inspects environment, but skips get_affected_tests + // (tests gate is off). test-writer then skips silently because + // requestRequiresTests is false, and doc-writer fires on the same iteration. + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'inspect_environment', + }) + const docWriterSpawn = gen.next({ + toolResult: [{ type: 'json', value: {} }], + } as any).value as any + expect(docWriterSpawn).toMatchObject({ + toolName: 'spawn_agent_inline', + input: { agent_type: 'doc-writer' }, + }) + expect(docWriterSpawn.input.params.source_files).toEqual([ + 'common/src/util/array.ts', + ]) + expect(docWriterSpawn.input.handoff.permissions.writablePaths).toEqual( + expect.arrayContaining(['common/docs/**', 'common/**/*.md']), + ) + expect(docWriterSpawn.input.handoff.permissions.readablePaths).toEqual([ + '**/*', + ]) + }) + + test('combined test+docs request runs writers sequentially, not in parallel', () => { + const base2 = createBase2('default') + const agentState = { agentId: 'base2-custom' } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Add tests and update documentation for the public API', + params: {}, + config: base2.programmaticConfig, + } as any) + + advanceToPostEditGitStatus(gen, 'agents/base2/base2.ts') + expect( + gen.next({ + toolResult: [ + { + type: 'json', + value: { status: ' M agents/base2/base2.ts' }, + }, + ], + } as any).value, + ).toMatchObject({ toolName: 'list_jobs' }) + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'inspect_environment', + }) + expect( + gen.next({ toolResult: [{ type: 'json', value: {} }] } as any).value, + ).toMatchObject({ toolName: 'get_affected_tests' }) + expect( + gen.next({ toolResult: [{ type: 'json', value: {} }] } as any).value, + ).toMatchObject({ toolName: 'get_build_targets' }) + const testWriterSpawn = gen.next({ + toolResult: [{ type: 'json', value: {} }], + } as any).value as any + expect(testWriterSpawn).toMatchObject({ + toolName: 'spawn_agent_inline', + input: { agent_type: 'test-writer' }, + }) + // Only one writer is active at a time: completing test-writer may open + // basher validation, then the next writer gate, never a dual spawn_agents + // batch of test-writer + doc-writer together. + expect(testWriterSpawn.toolName).toBe('spawn_agent_inline') + expect(testWriterSpawn.input.agents).toBeUndefined() + }) +}) + +describe('editor / repair-editor / test-writer cohesion', () => { + test('pure coverage findings route to test-writer, not repair-editor', () => { + expect( + isTestCoverageReviewerFinding( + 'BLOCKING: test coverage missing for changed behavior (add a case to the relevant *.test.ts)', + ), + ).toBe(true) + + const base2 = createBase2('default') + const agentState = { agentId: 'base2' } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Make the requested change now please', + params: {}, + } as any) + + advanceToPostEditGitStatus(gen, 'src/a.ts') + expect( + gen.next({ + toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], + } as any).value, + ).toMatchObject({ toolName: 'list_jobs' }) + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'run_file_change_hooks', + }) + expect( + gen.next({ toolResult: [{ type: 'json', value: [] }] } as any).value, + ).toMatchObject({ toolName: 'git_status' }) + const reviewCall = gen.next({ + toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], + } as any).value as any + expect(reviewCall).toMatchObject({ toolName: 'spawn_agents' }) + const prompt = String(reviewCall.input.agents[0].prompt ?? '') + const fingerprint = + prompt.match(/Snapshot fingerprint \(echo exactly\): ([^\n]+)/)?.[1] ?? + '' + const afterReview = gen.next({ + toolResult: [ + { + type: 'json', + value: [ + { + schemaVersion: 1, + verdict: 'NON_BLOCKING', + snapshotFingerprint: fingerprint, + reviewedFiles: ['src/a.ts'], + findings: [], + coverage: 'missing', + dimensions: { + correctness: 'pass', + security: 'pass', + tests: 'pass', + apiCompatibility: 'pass', + performance: 'pass', + }, + requirementCoverage: [], + }, + ], + }, + ], + } as any) + expect(afterReview.value).toMatchObject({ + toolName: 'add_message', + input: { role: 'user' }, + }) + const repairSpawn = gen.next().value as any + expect(repairSpawn).toMatchObject({ + toolName: 'spawn_agents', + input: { agents: [{ agent_type: 'test-writer' }] }, + }) + expect(repairSpawn.input.agents).toHaveLength(1) + expect(repairSpawn.input.agents[0].agent_type).not.toBe('repair-editor') + }) + + test('mixed code + coverage findings keep repair-editor only (no parallel test-writer)', () => { + const base2 = createBase2('default') + const agentState = { agentId: 'base2' } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Make the requested change now please', + params: {}, + } as any) + + advanceToPostEditGitStatus(gen, 'src/a.ts') + expect( + gen.next({ + toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], + } as any).value, + ).toMatchObject({ toolName: 'list_jobs' }) + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'run_file_change_hooks', + }) + expect( + gen.next({ toolResult: [{ type: 'json', value: [] }] } as any).value, + ).toMatchObject({ toolName: 'git_status' }) + const reviewCall = gen.next({ + toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], + } as any).value as any + const prompt = String(reviewCall.input.agents[0].prompt ?? '') + const fingerprint = + prompt.match(/Snapshot fingerprint \(echo exactly\): ([^\n]+)/)?.[1] ?? + '' + gen.next({ + toolResult: [ + { + type: 'json', + value: [ + { + schemaVersion: 1, + verdict: 'BLOCKING', + snapshotFingerprint: fingerprint, + reviewedFiles: ['src/a.ts'], + findings: ['Fix the edge case.'], + coverage: 'missing', + dimensions: { + correctness: 'pass', + security: 'pass', + tests: 'pass', + apiCompatibility: 'pass', + performance: 'pass', + }, + requirementCoverage: [], + }, + ], + }, + ], + } as any) + const repairSpawn = gen.next().value as any + expect(repairSpawn).toMatchObject({ + toolName: 'spawn_agents', + input: { agents: [{ agent_type: 'repair-editor' }] }, + }) + expect(repairSpawn.input.agents).toHaveLength(1) + expect( + repairSpawn.input.agents.some( + (agent: { agent_type?: string }) => agent.agent_type === 'test-writer', + ), + ).toBe(false) + }) + + test('code-reviewer may run in parallel with validation but cannot observe it unless results are included', () => { + const reviewer = createReviewer('anthropic/claude-opus-4.7') + expect(reviewer.instructionsPrompt).toContain( + 'Validation and other subagent work may be running in parallel', + ) + expect(reviewer.instructionsPrompt).toContain( + 'You cannot observe results from parallel agents unless the prompt explicitly includes those completed results', + ) + expect(reviewer.spawnableAgents).toEqual([]) + expect(reviewer.toolNames).toEqual(['read_files', 'set_output']) + }) + + test('createSpecialist agents never spawn editor/repair-editor/test-writer children', () => { + const specialist = createSpecialist({ + id: 'dependency-reviewer', + displayName: 'Dependency Reviewer', + purpose: 'Review dependency and lockfile correctness.', + focus: ['Manifest and lockfile correctness'], + }) + expect(specialist.spawnableAgents).toEqual([]) + expect(specialist.toolNames).not.toContain('spawn_agents') + expect(specialist.toolNames).not.toContain('spawn_agent_inline') + expect(specialist.toolNames).toContain('set_output') + }) +}) diff --git a/agents/__tests__/gate-repair-parity.test.ts b/agents/__tests__/gate-repair-parity.test.ts index ae1a79c498..e5a5d1964a 100644 --- a/agents/__tests__/gate-repair-parity.test.ts +++ b/agents/__tests__/gate-repair-parity.test.ts @@ -7,6 +7,7 @@ import { parseValidationFailures, type ParsedValidationFailure, } from '../base2/gate-repair' +import { extractInlineFunctionSource } from './helpers/extract-inline-function-source' type GateRepairHelpers = { parseValidationFailures: (failures: string[]) => ParsedValidationFailure[] @@ -24,33 +25,6 @@ const INLINE_HELPER_NAMES: GateRepairFunctionName[] = [ 'buildRepairEditorPrompt', ] -function extractInlineFunctionSource( - source: string, - functionName: string, -): string { - const declarationStart = source.indexOf(`function ${functionName}(`) - if (declarationStart < 0) { - throw new Error(`Unable to find inline ${functionName} declaration`) - } - - const bodyStart = source.indexOf('{', declarationStart) - if (bodyStart < 0) { - throw new Error(`Unable to find inline ${functionName} body`) - } - - let depth = 0 - for (let index = bodyStart; index < source.length; index += 1) { - const character = source[index] - if (character === '{') depth += 1 - if (character === '}') depth -= 1 - if (depth === 0) { - return source.slice(declarationStart, index + 1) - } - } - - throw new Error(`Unable to find end of inline ${functionName} declaration`) -} - function loadInlineGateRepairHelpers(): GateRepairHelpers { const base2Source = readFileSync( new URL('../base2/base2.ts', import.meta.url), @@ -161,3 +135,37 @@ describe('gate-repair helpers — inline copies match canonical exports', () => } }) }) + +describe('extractInlineFunctionSource — regex literals in early walks', () => { + test('does not mis-slice a function with a regex default param', () => { + const source = [ + 'function target(a = /}/, b = 2) {', + ' return a.test("x")', + '}', + 'function other() {}', + ].join('\n') + const sliced = extractInlineFunctionSource(source, 'target') + // The `}` inside the regex default must not terminate the param-list walk, + // and `other`'s body must not be swallowed into the slice. + expect(sliced).toBe( + 'function target(a = /}/, b = 2) {\n return a.test("x")\n}', + ) + expect(sliced).not.toContain('other') + }) + + test('does not mis-slice a function with a regex in a return-type annotation', () => { + const source = [ + 'function target(a: string): { pattern: /}/; name: string } {', + ' return { pattern: /}/, name: a }', + '}', + ].join('\n') + const sliced = extractInlineFunctionSource(source, 'target') + // The annotation's `{` must be consumed as the return type, and the `}` + // inside its regex must not be mistaken for the annotation's closing brace, + // so the real function body is still located. + expect(sliced).toContain('function target(a: string)') + expect(sliced).toContain('pattern: /}/;') + expect(sliced).toContain('return { pattern: /}/, name: a }') + expect(sliced.endsWith('}')).toBe(true) + }) +}) diff --git a/agents/__tests__/general-agent.test.ts b/agents/__tests__/general-agent.test.ts index 98e2dd4072..c71d751819 100644 --- a/agents/__tests__/general-agent.test.ts +++ b/agents/__tests__/general-agent.test.ts @@ -11,6 +11,20 @@ describe('general-agent programmatic tools', () => { expect(agent.toolNames).toContain('task_completed') }) + test('gpt-5 branch adds reasoningOptions and drops file-picker from spawnableAgents', () => { + const agent = createGeneralAgent({ model: 'gpt-5' }) + + expect(agent.reasoningOptions).toEqual({ effort: 'high' }) + expect(agent.spawnableAgents).not.toContain('file-picker') + expect(agent.displayName).toBe('Deep Reasoning General Agent') + + // The shared (opus) surface must remain intact so the two branches do not + // silently converge or regress. + expect(agent.spawnableAgents).toContain('researcher-web') + expect(agent.spawnableAgents).toContain('context-pruner') + expect(agent.programmaticToolNames).toContain('spawn_agent_inline') + }) + test('routes directory-like bootstrap paths through read_subtree', () => { const agent = createGeneralAgent({ model: 'opus' }) const generator = agent.handleSteps!({ @@ -34,17 +48,34 @@ describe('general-agent programmatic tools', () => { }) }) - test('routes ripgrep-style search through code-searcher with required params', () => { - // general-agent is not granted code_search directly; its prompt must tell - // it to spawn code-searcher and to pass the required params.searchQueries, - // otherwise the runtime rejects the direct code_search call and an empty - // code-searcher spawn fails with "Missing required: searchQueries". + test('fires query_index proactively on a qualifying prompt with no paths', () => { + const agent = createGeneralAgent({ model: 'opus' }) + const generator = agent.handleSteps!({ + prompt: 'Audit and fix the codebase for test coverage gaps', + params: {}, + } as any) + + expect(generator.next().value).toEqual({ + toolName: 'query_index', + input: { + query: 'Audit and fix the codebase for test coverage gaps', + limit: 20, + }, + }) + }) + + test('prefers direct code_search and multi-query code-searcher with required params', () => { + // general-agent may call code_search directly for single-pattern work, + // and spawn code-searcher for multi-query batches with + // params.searchQueries. const agent = createGeneralAgent({ model: 'opus' }) - expect(agent.toolNames).not.toContain('code_search') + expect(agent.toolNames).toContain('code_search') expect(agent.instructionsPrompt).toContain('code_search') - expect(agent.instructionsPrompt).toContain('not granted to you') + expect(agent.instructionsPrompt).toContain('prefer direct') + expect(agent.instructionsPrompt).toContain('multi-query') expect(agent.instructionsPrompt).toContain('params.searchQueries') + expect(agent.instructionsPrompt).not.toContain('not granted to you') }) test('binds durable audit shards to composable snapshot receipts', () => { @@ -123,6 +154,47 @@ describe('general-agent programmatic tools', () => { expect((completion.value as any)?.toolName).not.toBe('add_message') }) + test('keeps rejecting when the present receipt is for a different snapshot', () => { + const agent = createGeneralAgent({ model: 'opus' }) + const generator = agent.handleSteps!({ + prompt: 'Audit service completeness', + params: { + sessionSlug: 'readiness', + shardId: 'services', + // snapshotId intentionally absent: the shard is unbound-by-snapshot + // and must fail closed even when a structural receipt for a + // DIFFERENT snapshot is present. + }, + } as any) + + expect(generator.next().value).toMatchObject({ + toolName: 'spawn_agent_inline', + }) + expect(generator.next({ toolResult: [] } as any).value).toBe('STEP') + + const mismatchReceipt = generator.next({ + stepsComplete: true, + agentState: { + messageHistory: [ + { + role: 'tool', + content: [ + { + type: 'json', + value: { structuralReceipt: { snapshot_id: 'snapshot-1' } }, + }, + ], + }, + ], + }, + toolResult: [], + } as any) + + expect(mismatchReceipt.value).toMatchObject({ + toolName: 'add_message', + }) + }) + test('breaks the audit loop after exhausting completion retries', () => { const agent = createGeneralAgent({ model: 'opus' }) const generator = agent.handleSteps!({ diff --git a/agents/__tests__/helpers/base2-step-fixtures.ts b/agents/__tests__/helpers/base2-step-fixtures.ts new file mode 100644 index 0000000000..53b17cbf0d --- /dev/null +++ b/agents/__tests__/helpers/base2-step-fixtures.ts @@ -0,0 +1,57 @@ +/** + * Shared step-feed fixtures for base2 `handleSteps` generator tests. Mirrors + * the role of extract-inline-function-source.ts: the mirror sites + * (base2-writer-spawn-rules and the reviewer-spawn-conditions e2e) import + * these payloads instead of re-declaring near-identical copies. + */ + +/** Minimal pushed background-job digest payload for the post-git_status list_jobs yield. */ +export const LIST_JOBS_RESULT = { + jobs: [], + note: 'No action required unless you need this output.', +} + +/** Wrap `value` as a JSON tool-result feed for a base2 step generator. */ +export function feedJson(value: unknown) { + return { toolResult: [{ type: 'json', value }] } as any +} + +/** Feed the canonical post-git_status list_jobs digest payload. */ +export function feedListJobs() { + return feedJson(LIST_JOBS_RESULT) +} + +/** + * Canonical file_mutation_result receipt (the real production edit-artifact + * shape) for `path`. Feed this instead of a bare `{ file }` so the edited + * file lands in the live changedFiles set and the mid-turn git-status sweep + * absorbs the path into pending gate files. + */ +export function editReceipt(path: string) { + return { + kind: 'file_mutation_result', + version: 1, + operationId: `op-${path}`, + receiptId: `receipt-${path}`, + outcome: 'applied', + authorityTier: 'conditional_commit', + actions: [ + { + actionId: `action-${path}`, + index: 0, + action: 'update', + path, + outcome: 'applied', + beforeHash: 'before', + afterHash: 'after', + }, + ], + authorityReceipt: { + operationId: `op-${path}`, + receiptId: `receipt-${path}`, + actions: [{ actionId: `action-${path}` }], + }, + errors: [], + freshCapabilities: [], + } +} diff --git a/agents/__tests__/helpers/extract-inline-function-source.ts b/agents/__tests__/helpers/extract-inline-function-source.ts new file mode 100644 index 0000000000..74d06fc311 --- /dev/null +++ b/agents/__tests__/helpers/extract-inline-function-source.ts @@ -0,0 +1,397 @@ +/** + * Extract a named top-level function declaration (balanced braces) from + * transpiled source. Shared test utility so the three mirror sites + * (gate-repair-parity, base2-writer-spawn-rules, and the + * reviewer-spawn-conditions e2e) stay in sync instead of re-declaring + * identical copies. + * + * Known limitations (acceptable for the transpiled inputs this serves): + * - Line (double-slash) and block (slash-star ... star-slash) comments are + * skipped during the balance walks, so parens/braces inside them never + * corrupt slicing. + * - Quoted strings, escapes, and comments are also skipped during the final + * body-slice walk (see findMatchingDelimiterEnd), so literal braces inside + * a string or template literal in a prompt-building function body can't + * terminate the slice early and silently truncate the sink. + * - Plain backtick-quoted default template literals are inert during every + * parameter-list/annotation walk (all backtick-quoted content is skipped), so + * ordinary `${...}` interpolation parens/braces inside a default cannot break + * the balance walk. The one real edge is a NESTED backtick inside an + * interpolation (e.g. `` fn = `${`raw`}` `` or a raw backtick inside ${}): + * the inner backtick closes the quoted region early and trailing `${}` braces + * could then be miscounted. Fine for current transpiled output; revisit if + * the helper is shared more broadly. + * - Regex literals ARE skipped by every balance walk (the param-list, + * return-type-annotation, and body-start scans as well as the final + * findMatchingDelimiterEnd body slice), via isRegexStart, so a regex default + * param or a return-type annotation containing a brace/paren (e.g. + * `a = /}/`) can't corrupt those walks' balance. + * - isRegexStart is narrowed so a `//` directly after an operator or opening + * delimiter (e.g. `a = // comment`) is always treated as a line comment, not + * a regex. Without that narrowing, isRegexStart would read the `//` after `=` + * as a regex and a `}` inside the comment could terminate the walk early — + * pinned by the base2-writer-spawn-rules '// comment directly after =' test. + * All three call sites (gate-repair-parity, base2-writer-spawn-rules, and the + * reviewer-spawn-conditions e2e) share this same pinned limit via this helper. + * - isRegexStart is keyword-aware: when the previous significant character is + * an identifier tail, the full token is scanned back and matched against the + * expression-preceding keyword set (REGEX_PRECEDING_KEYWORDS), so a regex + * directly after a keyword (`return /{…}/`, `typeof /…/`) is skipped as a + * literal instead of being misread as division — pinned by the + * base2-writer-spawn-rules keyword-aware tests. + */ +export function extractInlineFunctionSource( + source: string, + functionName: string, +): string { + const declarationStart = source.indexOf(`function ${functionName}(`) + if (declarationStart < 0) { + throw new Error(`Unable to find inline ${functionName} declaration`) + } + + // Locate the parameter-list '(' immediately after `function NAME`. + const openParen = source.indexOf('(', declarationStart) + if (openParen < 0) { + throw new Error(`Unable to find inline ${functionName} parameter list`) + } + + // Walk the parameter list to its matching close paren, tracking nesting so + // destructured object/array params and string literals don't confuse depth. + let parenDepth = 1 + let index = openParen + 1 + let quote: string | null = null + while (index < source.length && parenDepth > 0) { + const character = source[index] + if (quote) { + if (character === '\\') { + index += 2 + continue + } + if (character === quote) quote = null + } else if (character === '"' || character === "'" || character === '`') { + quote = character + } else if ( + character === '/' && + source[index + 1] === '/' && + !isRegexStart(source, index) + ) { + // Skip a `//` line comment so parens/braces inside it can't corrupt the + // balance walk (e.g. `function f(a /* ) */)`). + const newline = source.indexOf('\n', index) + if (newline < 0) break + index = newline + 1 + continue + } else if (character === '/' && source[index + 1] === '*') { + // Skip a `/* ... */` block comment so tokens inside it stay inert. + const close = source.indexOf('*/', index + 2) + if (close < 0) break + index = close + 2 + continue + } else if (character === '/' && isRegexStart(source, index)) { + // Regex literal (e.g. a default param `a = /}/`): skip past the closing + // unescaped '/' so its braces/parens can't unbalance this walk. + const regexEnd = skipRegexLiteral(source, index) + if (regexEnd < 0) break + index = regexEnd + continue + } else if (character === '(' || character === '[' || character === '{') { + parenDepth += 1 + } else if (character === ')' || character === ']' || character === '}') { + parenDepth -= 1 + } + index += 1 + } + if (parenDepth !== 0) { + throw new Error(`Unable to find end of inline ${functionName} parameter list`) + } + + // After the params there may be a return-type annotation whose own braces + // (e.g. a multi-line object-literal type `): { groups: Array<{ ... }> }`) + // must be skipped so the REAL function-body '{' is chosen as bodyStart. Walk + // the annotation with the same quote/brace-aware logic until its brackets + // balance back to depth 0 (a brace-less type such as `: string` simply ends + // at the next whitespace). The body scan below then starts from the + // annotation's end and finds the actual body opener at braceDepth 0. + let bodyStart = -1 + let braceDepth = 0 + quote = null + + // Advance past any whitespace so the token immediately after the close paren + // is what gets inspected for a return-type ':'. + while (index < source.length && /\s/.test(source[index])) index += 1 + if (source[index] === ':') { + // Consume the ':' — the following token is the return-type annotation. + index += 1 + // Skip whitespace between ':' and the annotation start so the FIRST token + // inspected is the annotation's own opening bracket (for an object-literal + // type like `: { groups: ... }`) rather than a leading space. Without this, + // `!sawOpeningBracket && /\s/` breaks immediately on that space, the + // annotation is never consumed, and the real function-body '{' is never + // reached — the body-start scan then mistakes the annotation's '{' for the + // body and slices the function off inside its return-type annotation. + while (index < source.length && /\s/.test(source[index])) index += 1 + let annotationDepth = 0 + let sawOpeningBracket = false + while (index < source.length) { + const character = source[index] + if (quote) { + if (character === '\\') { + index += 2 + continue + } + if (character === quote) quote = null + } else if (character === '"' || character === "'" || character === '`') { + quote = character + } else if ( + character === '/' && + source[index + 1] === '/' && + !isRegexStart(source, index) + ) { + // Skip a `//` line comment in the return-type annotation. + const newline = source.indexOf('\n', index) + if (newline < 0) break + index = newline + 1 + continue + } else if (character === '/' && source[index + 1] === '*') { + // Skip a `/* ... */` block comment in the return-type annotation. + const close = source.indexOf('*/', index + 2) + if (close < 0) break + index = close + 2 + continue + } else if (character === '/' && isRegexStart(source, index)) { + // Regex literal inside the return-type annotation (e.g. a tagged type + // or a regex pattern): skip past its closing '/' so its braces/parens + // can't unbalance the annotation walk. + const regexEnd = skipRegexLiteral(source, index) + if (regexEnd < 0) break + index = regexEnd + continue + } else if (character === '{' || character === '(' || character === '[') { + annotationDepth += 1 + sawOpeningBracket = true + } else if (character === '}' || character === ')' || character === ']') { + annotationDepth -= 1 + if (sawOpeningBracket && annotationDepth <= 0) { + // Balanced back to 0: the annotation ends at this closing bracket. + index += 1 + break + } + } else if (!sawOpeningBracket && /\s/.test(character)) { + // A simple annotation without any brackets ends at the next whitespace. + break + } + index += 1 + } + } + + while (index < source.length && bodyStart < 0) { + const character = source[index] + if (quote) { + if (character === '\\') { + index += 2 + continue + } + if (character === quote) quote = null + } else if (character === '"' || character === "'" || character === '`') { + quote = character + } else if ( + character === '/' && + source[index + 1] === '/' && + !isRegexStart(source, index) + ) { + // Skip a `//` line comment in the body-start scan. + const newline = source.indexOf('\n', index) + if (newline < 0) break + index = newline + 1 + continue + } else if (character === '/' && source[index + 1] === '*') { + // Skip a `/* ... */` block comment in the body-start scan. + const close = source.indexOf('*/', index + 2) + if (close < 0) break + index = close + 2 + continue + } else if (character === '/' && isRegexStart(source, index)) { + // Regex literal in the body before the opener (rare, but keep parity with + // the other walks): skip past its closing '/' so a regex brace can't be + // mistaken for the function body opener. + const regexEnd = skipRegexLiteral(source, index) + if (regexEnd < 0) break + index = regexEnd + continue + } else if (character === '{') { + if (braceDepth === 0) bodyStart = index + else braceDepth += 1 + } else if (character === '}') { + braceDepth -= 1 + } + index += 1 + } + if (bodyStart < 0) { + throw new Error(`Unable to find inline ${functionName} body`) + } + + // Slice from `function NAME(` through the body's matching closing brace, + // preserving the full signature (params + return-type annotation). Uses the + // same quote/escape/comment-aware balance walk as the earlier passes so + // literal braces inside a string or template literal in the body can't cut + // the slice short. + const bodyEnd = findMatchingDelimiterEnd(source, bodyStart, '{', '}') + if (bodyEnd < 0) { + throw new Error(`Unable to find end of inline ${functionName} declaration`) + } + return source.slice(declarationStart, bodyEnd + 1) +} + +/** + * Keywords that leave the following token in expression position, so a `/` + * directly after one of them opens a regex literal rather than being a + * division operator (`return /re/`, `typeof /re/`, `x in /re/`, ...). + */ +const REGEX_PRECEDING_KEYWORDS = new Set([ + 'await', + 'case', + 'delete', + 'do', + 'else', + 'in', + 'instanceof', + 'new', + 'of', + 'return', + 'throw', + 'typeof', + 'void', + 'yield', +]) + +/** + * Returns true when the '/' at `index` opens a regex literal rather than being + * a division operator or part of a comment. Heuristic: the previous + * significant (non-whitespace) character is an operator, an opening delimiter, + * a comma, or nothing (expression start). A division is preceded by a number, + * a closing delimiter `)`, `]`, or `}`, or an identifier that is NOT an + * expression-preceding keyword: when the previous token ends in an identifier + * character, the full token is scanned back and matched against + * REGEX_PRECEDING_KEYWORDS so `return /{…}/` or `typeof /…/` is read as a + * regex, not a division. + */ +function isRegexStart(source: string, index: number): boolean { + // A '/' at `index` that isn't already part of a comment opens a regex literal + // when it is NOT a division operator, i.e. when the previous significant char + // is an operator, opening delimiter, comma, or nothing (expression start). + // Division looks like `a / b`, `) / b`, `] / b`, `} / b`, or an identifier/'/' + // immediately before the slash. This heuristic matches the transpiled inputs. + // + // Narrowing (cross-referenced from the header doc comment): a `//` is ALWAYS + // a line comment, never the start of a regex literal (a regex whose first + // char is '/' would need to be written escaped, so source[index + 1] would be + // '\', not '/'). Returning false here lets the `//` comment branch handle it + // instead of isRegexStart misreading e.g. `a = // comment\n }` after the '=' + // as a regex — which would let a '}' inside that comment terminate the + // balance walk early. Pinned by the base2-writer-spawn-rules '// comment + // directly after =' test. + if (source[index + 1] === '/') return false + let prev = index - 1 + while (prev >= 0 && /\s/.test(source[prev])) prev -= 1 + if (prev < 0) return true + const c = source[prev] + // Numbers and closing delimiters always end a division operand. + if (/[0-9\)\]\}]/.test(c)) return false + // Identifier tail: a char-only check would misread a regex following an + // expression-preceding keyword (e.g. `return /{…}/`, `typeof /…/`) as a + // division. Scan back the full token and admit those keywords instead. + if (/[A-Za-z_$]/.test(c)) { + let tokenStart = prev + while (tokenStart > 0 && /[A-Za-z0-9_$]/.test(source[tokenStart - 1])) { + tokenStart -= 1 + } + return REGEX_PRECEDING_KEYWORDS.has(source.slice(tokenStart, prev + 1)) + } + return true +} + +/** + * Advance past a regex literal whose '/' is at `cursor` (the caller has already + * confirmed isRegexStart). Skips escapes and character classes so quantifier + * braces like `{m,n}` and literal openers/closers inside the pattern can't + * corrupt the caller's balance walk. Returns the index just past the closing + * unescaped '/', or -1 if the literal runs off the end of the source without + * closing. + */ +function skipRegexLiteral(source: string, cursor: number): number { + let idx = cursor + 1 + let inClass = false + while (idx < source.length) { + const ch = source[idx] + if (ch === '\\') idx += 2 + else if (ch === '[') { + inClass = true + idx += 1 + } else if (ch === ']') { + inClass = false + idx += 1 + } else if (ch === '/' && !inClass) { + return idx + 1 + } else idx += 1 + } + return -1 +} + +/** + * Walk forward from `start` (which must point at an `opener`) to the matching + * `closer`, tracking nesting so nested delimiters stay balanced. Quoted + * strings, escapes, line comments (slash-slash), block comments, and regex + * literals are skipped so literal openers/closers inside them can't corrupt the + * balance. Returns the index of the matching `closer`, or -1 if the source ends + * before the delimiters balance back to depth 0. + */ +export function findMatchingDelimiterEnd( + source: string, + start: number, + opener: '{' | '[' | '(', + closer: '}' | ']' | ')', +): number { + let depth = 0 + let cursor = start + let quote: string | null = null + while (cursor < source.length) { + const character = source[cursor] + if (quote) { + if (character === '\\') { + cursor += 2 + continue + } + if (character === quote) quote = null + } else if (character === '"' || character === "'" || character === '`') { + quote = character + } else if ( + character === '/' && + source[cursor + 1] === '/' && + !isRegexStart(source, cursor) + ) { + const newline = source.indexOf('\n', cursor) + if (newline < 0) break + cursor = newline + 1 + continue + } else if (character === '/' && source[cursor + 1] === '*') { + const close = source.indexOf('*/', cursor + 2) + if (close < 0) break + cursor = close + 2 + continue + } else if (character === '/' && isRegexStart(source, cursor)) { + // Regex literal: skip to the closing unescaped '/', ignoring quantifier + // braces like `{m,n}` so they can't unbalance the delimiter walk. + const regexEnd = skipRegexLiteral(source, cursor) + if (regexEnd < 0) break + cursor = regexEnd + continue + } else if (character === opener) { + depth += 1 + } else if (character === closer) { + depth -= 1 + if (depth === 0) return cursor + } + cursor += 1 + } + return -1 +} diff --git a/agents/__tests__/quality-prompt-snapshot.test.ts b/agents/__tests__/quality-prompt-snapshot.test.ts index 7af9083092..38e9e123d8 100644 --- a/agents/__tests__/quality-prompt-snapshot.test.ts +++ b/agents/__tests__/quality-prompt-snapshot.test.ts @@ -234,17 +234,17 @@ describe('shared craftsmanship prompt sections', () => { expect(editor.instructionsPrompt).not.toContain(frontendSection) }) - test('base2 system prompt routes ripgrep-style search through code-searcher', () => { - // The root orchestrator is not granted code_search/find_files_matching_content; - // its prompt must tell it to spawn code-searcher instead of calling them - // directly (otherwise the runtime rejects the call). Guard the semantic - // content without freezing the exact wording. + test('base2 system prompt prefers direct code_search and multi-query code-searcher', () => { + // Root content-search tools are granted; the prompt must prefer direct + // code_search for single-pattern search and code-searcher for multi-query + // batching. Guard the semantic content without freezing the exact wording. const base2 = createBase2('default') expect(base2.systemPrompt).toContain('code-searcher') expect(base2.systemPrompt).toContain('code_search') - expect(base2.systemPrompt).toContain('find_files_matching_content') - expect(base2.systemPrompt).toContain('not granted to you as root') + expect(base2.systemPrompt).toContain('Prefer direct') + expect(base2.systemPrompt).toContain('multi-query') + expect(base2.systemPrompt).not.toContain('not granted to you as root') }) test('base2 system prompt names required spawn params for code-searcher and basher', () => { diff --git a/agents/base2/base2.ts b/agents/base2/base2.ts index db4d86a960..cb5b4b2b80 100644 --- a/agents/base2/base2.ts +++ b/agents/base2/base2.ts @@ -32,20 +32,6 @@ import { type SecretAgentDefinition, } from '../types/secret-agent-definition' -/** Env canary for progressive prompt disclosure (module-level; createBase2 load time). */ -export function isProgressivePromptDisclosureEnvEnabled( - raw: string | undefined, -): boolean { - if (typeof raw !== 'string') return false - const normalized = raw.trim().toLowerCase() - return ( - normalized === '1' || - normalized === 'true' || - normalized === 'yes' || - normalized === 'on' - ) -} - /** * Default for progressive prompt disclosure when the caller omits the * `progressivePromptDisclosure` option. Post-flip (M2): ON by default. The @@ -101,7 +87,7 @@ export function createBase2( // force-on override consulted on this omitted-option path. const progressivePromptDisclosure = progressivePromptDisclosureOption ?? - (isProgressivePromptDisclosureEnvEnabled( + (isProgressiveToolDisclosureEnvEnabled( typeof process === 'object' && process !== null ? process.env?.OPENBUFF_PROGRESSIVE_PROMPT_DISCLOSURE : undefined, @@ -330,8 +316,8 @@ ${ } ${ progressiveToolDisclosure - ? '- **Prefer dedicated harness tools over shell fallbacks:** Repository status is injected automatically by the runtime; do not spawn basher merely to run git status. Use read_files/read_outline/read_subtree/glob/list_directory/query_index for file and codebase inspection instead of shelling out to cat/ls/find/grep. For large files prefer read_files windows/around/symbol selectors over guess-shrink-retry ranges paging. Use basher for commands that do not have a dedicated tool, such as tests, builds, package scripts, and one-off project CLIs. Never embed a multi-KB file body or heredoc (`<<\'EOF\' ... EOF`) inside `basher.params.command`; the transport truncates large payloads and the JSON normalizer intentionally fails closed on truncated input. When edit tools unlock, author files with the dedicated edit surface and run them via a short basher command instead. For ripgrep-style content search, spawn the code-searcher agent (and file-picker for fuzzy file discovery): `code_search`/`find_files_matching_content` are registered runtime tools but are intentionally not granted to you as root, so calling them directly is rejected. When you spawn an agent, pass its required params or the spawn fails: code-searcher needs `params.searchQueries` (an array of { pattern } objects) and basher needs `params.command` (a shell string); put these in `params`, not only in the prose prompt. Correct spawn_agents shape: { "agents": [{ "agent_type": "code-searcher", "prompt": "...", "params": { "searchQueries": [{ "pattern": "..." }] } }] } — prompt and params go INSIDE each agent entry, never as siblings of agents, and agents is a real array (never a JSON string).' - : '- **Prefer dedicated harness tools over shell fallbacks:** Repository status is injected automatically by the runtime; do not spawn basher merely to run git status. Use read_files/read_outline/read_subtree/glob/list_directory/query_index for file and codebase inspection instead of shelling out to cat/ls/find/grep. For large files prefer read_files windows/around/symbol selectors over guess-shrink-retry ranges paging. Use basher for commands that do not have a dedicated tool, such as tests, builds, package scripts, and one-off project CLIs. Never embed a multi-KB file body or heredoc (`<<\'EOF\' ... EOF`) inside `basher.params.command`; the transport truncates large payloads and the JSON normalizer intentionally fails closed on truncated input. Author files with `write_file`/`edit_transaction` and run them via a short basher command instead. For ripgrep-style content search, spawn the code-searcher agent (and file-picker for fuzzy file discovery): `code_search`/`find_files_matching_content` are registered runtime tools but are intentionally not granted to you as root, so calling them directly is rejected. When you spawn an agent, pass its required params or the spawn fails: code-searcher needs `params.searchQueries` (an array of { pattern } objects) and basher needs `params.command` (a shell string); put these in `params`, not only in the prose prompt. Correct spawn_agents shape: { "agents": [{ "agent_type": "code-searcher", "prompt": "...", "params": { "searchQueries": [{ "pattern": "..." }] } }] } — prompt and params go INSIDE each agent entry, never as siblings of agents, and agents is a real array (never a JSON string).' + ? '- **Prefer dedicated harness tools over shell fallbacks:** Repository status is injected automatically by the runtime; do not spawn basher merely to run git status. Use read_files/read_outline/read_subtree/glob/list_directory/query_index for file and codebase inspection instead of shelling out to cat/ls/find/grep. Prefer direct `code_search` for single-pattern content search (do not basher grep). Spawn `code-searcher` for multi-query batch search with `params.searchQueries`. For large files prefer read_files windows/around/symbol selectors over guess-shrink-retry ranges paging. Use basher for commands that do not have a dedicated tool, such as tests, builds, package scripts, and one-off project CLIs. Never embed a multi-KB file body or heredoc (`<<\'EOF\' ... EOF`) inside `basher.params.command`; the transport truncates large payloads and the JSON normalizer intentionally fails closed on truncated input. When edit tools unlock, author files with the dedicated edit surface and run them via a short basher command instead. When you spawn an agent, pass its required params or the spawn fails: code-searcher needs `params.searchQueries` (an array of { pattern } objects) and basher needs `params.command` (a shell string); put these in `params`, not only in the prose prompt. Correct spawn_agents shape: { "agents": [{ "agent_type": "code-searcher", "prompt": "...", "params": { "searchQueries": [{ "pattern": "..." }] } }] } — prompt and params go INSIDE each agent entry, never as siblings of agents, and agents is a real array (never a JSON string).' + : '- **Prefer dedicated harness tools over shell fallbacks:** Repository status is injected automatically by the runtime; do not spawn basher merely to run git status. Use read_files/read_outline/read_subtree/glob/list_directory/query_index for file and codebase inspection instead of shelling out to cat/ls/find/grep. Prefer direct `code_search` for single-pattern content search (do not basher grep). Spawn `code-searcher` for multi-query batch search with `params.searchQueries`. For large files prefer read_files windows/around/symbol selectors over guess-shrink-retry ranges paging. Use basher for commands that do not have a dedicated tool, such as tests, builds, package scripts, and one-off project CLIs. Never embed a multi-KB file body or heredoc (`<<\'EOF\' ... EOF`) inside `basher.params.command`; the transport truncates large payloads and the JSON normalizer intentionally fails closed on truncated input. Author files with `write_file`/`edit_transaction` and run them via a short basher command instead. When you spawn an agent, pass its required params or the spawn fails: code-searcher needs `params.searchQueries` (an array of { pattern } objects) and basher needs `params.command` (a shell string); put these in `params`, not only in the prose prompt. Correct spawn_agents shape: { "agents": [{ "agent_type": "code-searcher", "prompt": "...", "params": { "searchQueries": [{ "pattern": "..." }] } }] } — prompt and params go INSIDE each agent entry, never as siblings of agents, and agents is a real array (never a JSON string).' } # Code Editing Mandates @@ -1280,13 +1266,24 @@ ${disclose(specialistRoutingSection, specialistRoutingPointer)} hasExplicitGitDeliveryIntent(prompt) && initialGitStatusFiles.length > 0 ) { - recordChangedFiles(initialGitStatusFiles) - editsHappened = true - finalResponseGateOpen = false - mutableAgentState.canSuggestFollowups = false - activeWorkState.currentPhase = 'awaiting_validation' - activeWorkState.latestWorkSummary = `Explicit Git delivery request adopted turn-start dirty files: ${initialGitStatusFiles.join(', ')}` - markActiveWorkStateChanged() + // Scope the git-delivery adoption to REVIEWABLE dirty files only. + // Non-reviewable turn-start dirt (docs, session STATE.json, jsonl, + // config) belongs to the whole worktree, not to any one conversation's + // gate; forcing the reviewer to attest to every unrelated dirty path + // turns a clean commit turn into a worktree-wide review. Reviewable + // source/test files the user asked us to commit still enter the gate. + const reviewableDeliveryFiles = selectReviewableGateFiles( + initialGitStatusFiles, + ) + if (reviewableDeliveryFiles.length > 0) { + recordChangedFiles(reviewableDeliveryFiles) + editsHappened = true + finalResponseGateOpen = false + mutableAgentState.canSuggestFollowups = false + activeWorkState.currentPhase = 'awaiting_validation' + activeWorkState.latestWorkSummary = `Explicit Git delivery request adopted turn-start dirty files: ${initialGitStatusFiles.join(', ')}` + markActiveWorkStateChanged() + } } // Track files previously observed dirty in git status so we can safely // prune them from the pending set when they disappear (committed). diff --git a/agents/base2/tool-tiers.ts b/agents/base2/tool-tiers.ts index 5ddd435819..5485e871fd 100644 --- a/agents/base2/tool-tiers.ts +++ b/agents/base2/tool-tiers.ts @@ -1,61 +1,44 @@ import { buildArray } from '@codebuff/common/util/array' +import { + BASE2_CORE_TOOL_NAMES, + BASE2_TIER_TOOL_NAMES, +} from '@codebuff/agent-runtime/util/base2-tool-tiers' + import type { AllToolNames } from '../types/secret-agent-definition' /** Progressive model-visible tool tiers for base2 (M1). */ export type ToolTier = 'core' | 'implement' | 'audit' | 'media_3d' | 'job_extra' +/** + * Tier tool membership is owned by the runtime mirror in + * `packages/agent-runtime/src/util/base2-tool-tiers.ts` + * (`BASE2_CORE_TOOL_NAMES` / `BASE2_TIER_TOOL_NAMES`), because `agent-runtime` + * must not import from `agents/` (wrong dependency direction). `agents` is the + * correct direction, so these constants re-export the runtime truth instead of + * duplicating it. Re-exporting (rather than copying) makes the two lists + * identical BY CONSTRUCTION, so a one-sided edit to either list now fails at + * compile time (missing/mismatched re-export) instead of only failing the + * progressive-disclosure test suite at runtime. + */ + /** Base CORE names without mode conditionals — gates live in resolveModelToolNames. */ -export const CORE_TOOLS = [ - 'spawn_agents', - 'query_index', - 'read_files', - 'read_outline', - 'read_subtree', - 'list_directory', - 'glob', - 'ask_user', - 'skill', - 'suggest_followups', - 'write_todos', - 'list_jobs', - 'check_job', - 'check_background_agent', - 'read_logs', -] as const +export const CORE_TOOLS: readonly string[] = BASE2_CORE_TOOL_NAMES /** Base IMPLEMENT names without mode conditionals. */ -export const IMPLEMENT_TOOLS = [ - 'edit_transaction', - 'create_plan', - 'update_plan_status', - 'inspect_workspace', - 'inspect_environment', - 'get_affected_tests', - 'get_build_targets', - 'run_targeted_validation', - 'run_terminal_command', -] as const +export const IMPLEMENT_TOOLS: readonly string[] = + BASE2_TIER_TOOL_NAMES.implement /** Base AUDIT names without mode conditionals. */ -export const AUDIT_TOOLS = [ - 'inspect_codebase_structure', - 'inspect_feature_completeness', - 'evaluate_audit_coverage', - 'get_change_review_bundle', - 'get_task', -] as const +export const AUDIT_TOOLS: readonly string[] = BASE2_TIER_TOOL_NAMES.audit /** Base MEDIA_3D names without mode conditionals. */ -export const MEDIA_3D_TOOLS = [ - 'read_image', - 'inspect_3d_asset', - 'render_3d_preview', - 'edit_3d_asset', -] as const +export const MEDIA_3D_TOOLS: readonly string[] = + BASE2_TIER_TOOL_NAMES.media_3d /** Base JOB_EXTRA names without mode conditionals. */ -export const JOB_EXTRA_TOOLS = ['kill_job'] as const +export const JOB_EXTRA_TOOLS: readonly string[] = + BASE2_TIER_TOOL_NAMES.job_extra /** Canary-on starts core-only until handleSteps unlocks further tiers. */ export const DEFAULT_UNLOCKED_TIERS_WHEN_PROGRESSIVE: readonly ToolTier[] = [] @@ -224,6 +207,7 @@ export function resolveModelToolNames( 'skill', 'list_directory', 'glob', + 'code_search', 'check_background_agent', 'check_job', 'kill_job', @@ -256,6 +240,7 @@ export function resolveModelToolNames( 'read_subtree', 'list_directory', 'glob', + 'code_search', !noAskUser && 'ask_user', 'skill', 'suggest_followups', diff --git a/agents/e2e/reviewer-spawn-conditions.e2e.test.ts b/agents/e2e/reviewer-spawn-conditions.e2e.test.ts index 40a0952fe6..79c25fbd4a 100644 --- a/agents/e2e/reviewer-spawn-conditions.e2e.test.ts +++ b/agents/e2e/reviewer-spawn-conditions.e2e.test.ts @@ -1,7 +1,10 @@ import { createHash } from 'node:crypto' import { + existsSync, lstatSync, + mkdirSync, mkdtempSync, + readdirSync, readFileSync, readlinkSync, realpathSync, @@ -14,13 +17,37 @@ import { import { tmpdir } from 'node:os' import { isAbsolute, join, relative, resolve, sep } from 'node:path' -import { describe, expect, test } from 'bun:test' +import { afterAll, describe, expect, test } from 'bun:test' import { createBase2 } from '../base2/base2' import { hashGateSnapshotDetails, isAttestableSnapshotFingerprint, } from '../base2/gate-fingerprint' +import { + editReceipt, + feedJson, + feedListJobs, +} from '../__tests__/helpers/base2-step-fixtures' +import { extractInlineFunctionSource } from '../__tests__/helpers/extract-inline-function-source' + +/** + * Shared cwd-local scratch parent for this file's mkdtemp children. Each test + * removes its own mkdtemp child in a finally block; the afterAll below then + * removes the parent once it is empty so no stray `.base2-test-scratch` + * directory survives the run, without disturbing other test files that share + * it. + */ +const SCRATCH_PARENT_DIR = join(process.cwd(), '.base2-test-scratch') + +afterAll(() => { + if ( + existsSync(SCRATCH_PARENT_DIR) && + readdirSync(SCRATCH_PARENT_DIR).length === 0 + ) { + rmSync(SCRATCH_PARENT_DIR, { recursive: true, force: true }) + } +}) function parseGateStateBlock(text: string): { gate: string @@ -36,10 +63,6 @@ function parseGateStateBlock(text: string): { } } -function feedJson(value: unknown) { - return { toolResult: [{ type: 'json', value }] } as any -} - function finishStep(value: unknown) { return { stepsComplete: true, @@ -47,24 +70,97 @@ function finishStep(value: unknown) { } as any } -/** Minimal pushed background-job digest payload for the post-git_status list_jobs yield. */ -const LIST_JOBS_RESULT = { - jobs: [], - note: 'No action required unless you need this output.', -} - -function feedListJobs() { - return feedJson(LIST_JOBS_RESULT) +/** + * Shared `base2ActiveWork` fixture seeding a validated, gate-passed, and + * reviewed agent state. Tests inject their gate-passed file marker, reviewable + * fingerprint, and review receipts to exercise content-drift, external-symlink, + * and symlink-content-drift reopen scenarios without ~30 lines of near-identical + * scaffolding each. + */ +function base2ActiveWorkFixture({ + agentId, + path, + durableFingerprint, + validationSummary, + gatePassedFileMarkers, + reviewedReviewableFingerprint = '', + reviewReceipts = [], +}: { + agentId: string + path: string + durableFingerprint: string + validationSummary: string + gatePassedFileMarkers: Record + reviewedReviewableFingerprint?: string + reviewReceipts?: unknown[] +}) { + return { + agentId, + base2ActiveWork: { + changedFiles: [path], + touchedFiles: [path], + pendingGateFiles: [path], + gatePassedFiles: [path], + gatePassedFileMarkers, + gatePassedPendingFiles: [], + gatePassedFingerprint: durableFingerprint, + gatePassedValidationSummary: validationSummary, + gatePassedReviewerVerdict: 'LOOKS_GOOD', + currentPhase: 'final_response_allowed', + latestWorkSummary: '', + openReviewerBlockers: [], + lastValidationSummary: validationSummary, + nextRequiredAction: '', + lastPinnedStateMessage: '', + reviewedReviewableFingerprint, + reviewReceipts, + }, + } } function gateFileMarker(path: string): string { try { - if (lstatSync(path).isSymbolicLink()) { - const linkText = readlinkSync(path) - // Mirror production: reject symlinks whose resolved target escapes cwd. - const cwd = process.cwd() - const absolutePath = resolve(cwd, path) - const resolvedPath = realpathSync(absolutePath) + // Mirror production readGateFileContentMarker: walk EVERY project-relative + // segment and record each symlink by its 0-based index within the + // segments (``${index}:${link}``). Production rejects intermediate escaping + // symlinks too, not just a final-segment symlink, so a mid-path symlink + // must still take the symlink-sha256 branch (or the + // outside-project-symlink rejection) rather than falling back to a plain + // sha256. This is not a final-segment-only mirror. + const cwd = process.cwd() + const absolutePath = resolve(cwd, path) + const projectRelativePath = relative(cwd, absolutePath) + if ( + projectRelativePath === '..' || + projectRelativePath.startsWith(`..${sep}`) || + isAbsolute(projectRelativePath) + ) { + return 'unreadable:outside-project' + } + const pathSegments = projectRelativePath.split(sep).filter(Boolean) + const symlinkParts: string[] = [] + let entryPath = cwd + for (let index = 0; index < pathSegments.length; index += 1) { + entryPath = join(entryPath, pathSegments[index]) + const entryStat = lstatSync(entryPath) + if (entryStat.isSymbolicLink()) { + symlinkParts.push(`${index}:${readlinkSync(entryPath)}`) + continue + } + if (index < pathSegments.length - 1 && !entryStat.isDirectory()) { + return 'unreadable:not-a-directory' + } + if (index === pathSegments.length - 1 && !entryStat.isFile()) { + return 'unreadable:not-a-file' + } + } + if (pathSegments.length === 0) return 'unreadable:not-a-file' + + const resolvedPath = + symlinkParts.length > 0 ? realpathSync(absolutePath) : absolutePath + // Fail closed BEFORE opening: if the resolved target escapes the project + // root, reject without reading the target bytes. + if (symlinkParts.length > 0) { const resolvedRelative = relative(cwd, resolvedPath) if ( resolvedRelative === '..' || @@ -73,15 +169,17 @@ function gateFileMarker(path: string): string { ) { return 'unreadable:outside-project-symlink' } - const data = readFileSync(path) + } + const data = readFileSync(path) + if (symlinkParts.length > 0) { + // Per-segment index scheme (``${index}:${link}``), NOT a hardcoded `0:`. const hash = createHash('sha256') - .update(`0:${linkText}`) + .update(symlinkParts.join('\0')) .update('\0') .update(data) .digest('hex') return `symlink-sha256:${hash}:${data.length}` } - const data = readFileSync(path) return `sha256:${createHash('sha256').update(data).digest('hex')}:${data.length}` } catch (error) { const code = @@ -103,37 +201,33 @@ function reviewableFingerprint(path: string): string { } /** - * Canonical file_mutation_result receipt (the real production edit-artifact - * shape) for `path`. Feed this instead of a bare `{ file }` so the edited file - * lands in the live changedFiles set before the mid-turn git-status sweep. + * Load the REAL production `readGateFileContentMarker` from base2.ts by + * extracting its inline declaration, transpiling, and evaluating it as a + * standalone function (no module closure needed: it resolves fs/path/crypto + * lazily via process.getBuiltinModule / global require at call time). This is + * the parity oracle the test-local `gateFileMarker` must agree with. + * + * Inline extraction is preferred over importing the member from base2 because + * importing would pull in the whole module and run its top-level module-closure + * side effects (agent/step registries, provider construction, and other init + * work) in this test context. Evaluating this one pure helper in isolation + * keeps the oracle hermetic while still exercising the exact production source. */ -function editReceipt(path: string) { - return { - kind: 'file_mutation_result', - version: 1, - operationId: `op-${path}`, - receiptId: `receipt-${path}`, - outcome: 'applied', - authorityTier: 'conditional_commit', - actions: [ - { - actionId: `action-${path}`, - index: 0, - action: 'update', - path, - outcome: 'applied', - beforeHash: 'before', - afterHash: 'after', - }, - ], - authorityReceipt: { - operationId: `op-${path}`, - receiptId: `receipt-${path}`, - actions: [{ actionId: `action-${path}` }], - }, - errors: [], - freshCapabilities: [], - } +function loadProductionGateFileContentMarker(): (path: string) => string { + const base2Source = readFileSync( + new URL('../base2/base2.ts', import.meta.url), + 'utf8', + ) + const transpiler = new Bun.Transpiler({ loader: 'ts' }) + const base2JavaScript = transpiler.transformSync(base2Source) + const helperSource = extractInlineFunctionSource( + base2JavaScript, + 'readGateFileContentMarker', + ) + const fn = new Function( + `"use strict";\n${helperSource}\nreturn readGateFileContentMarker`, + ) as () => (path: string) => string + return fn() } describe('base2 reviewer spawn conditions e2e', () => { @@ -579,8 +673,108 @@ describe('base2 reviewer spawn conditions e2e', () => { }) }) + test('resumed session with unrelated dirty reviewable files does not re-arm review', () => { + // P0-negative as an e2e: a resumed conversation with a durable gate pass on + // a real file A must NOT re-arm / re-spawn the reviewer when the working + // tree also contains an unrelated (non-task) dirty reviewable file + // src/c.ts. That foreign dirt belongs to another tab or tool, not to this + // conversation's task ledger, so it must not re-open validation/review nor + // surface as an uncommitted-unvalidated file for the git-committer guard. + mkdirSync(join(process.cwd(), '.base2-test-scratch'), { recursive: true }) + const tempDir = mkdtempSync( + join(process.cwd(), '.base2-test-scratch', '.reviewer-resume-foreign-'), + ) + const absoluteA = join(tempDir, 'a.ts') + const pathA = relative(process.cwd(), absoluteA).replace(/\\/g, '/') + const validationSummary = 'No configured file-change hooks ran.' + const base2 = createBase2('default') + try { + // A must be a real, attestable file so its gatePassed marker survives + // the per-file eviction guard (a ledger entry with no stored marker is + // treated as drifted and evicted, which would re-arm the gate and break + // the regression we are asserting). + writeFileSync(absoluteA, 'export const a = 1\n') + const marker = `sha256:${createHash('sha256') + .update(readFileSync(absoluteA)) + .digest('hex')}:${readFileSync(absoluteA).length}` + const agentState = { + agentId: 'base2-custom', + base2ActiveWork: { + changedFiles: [pathA], + touchedFiles: [pathA], + pendingGateFiles: [], + currentPhase: 'final_response_allowed', + latestWorkSummary: '', + openReviewerBlockers: [], + lastValidationSummary: validationSummary, + nextRequiredAction: '', + lastPinnedStateMessage: '', + gatePassedFiles: [pathA], + gatePassedFileMarkers: { [pathA]: marker }, + gatePassedPendingFiles: [pathA], + gatePassedReviewerVerdict: 'LOOKS_GOOD', + gatePassedValidationSummary: validationSummary, + gatePassedFingerprint: gateFingerprint(pathA, validationSummary), + }, + } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Can you confirm whether those earlier reports still hold', + params: {}, + } as any) + + expect(gen.next().value).toMatchObject({ toolName: 'git_status' }) + // A is still dirty + gate-passed; src/c.ts is foreign (not task-related). + expect( + gen.next( + feedJson({ status: ` M ${pathA}\n M src/c.ts` }), + ).value, + ).toMatchObject({ toolName: 'list_jobs' }) + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'spawn_agent_inline', + input: { agent_type: 'context-pruner' }, + }) + const maybePinned = gen.next().value + if (maybePinned !== 'STEP') { + // A pinned-state message here must be the concrete task-ledger state + // (final_response_allowed with the gate already passed), never a + // re-armed gate message. Assert its exact role and content so a + // regression that re-arms the reviewer with a different pinned-state + // add_message cannot be silently consumed. + expect(maybePinned).toMatchObject({ + toolName: 'add_message', + input: { role: 'user' }, + }) + const pinnedContent = (maybePinned as any).input.content as string + expect(pinnedContent).toContain('Current phase: final_response_allowed') + expect(gen.next().value).toBe('STEP') + } + + // The foreign dirty file neither re-arms the gate nor gets adopted into + // the task ledger, so finalization stays open with no pending work. + expect((agentState as any).base2ActiveWork).toMatchObject({ + changedFiles: [pathA], + pendingGateFiles: [], + currentPhase: 'final_response_allowed', + }) + expect((agentState as any).base2ActiveWork.pendingGateFiles).not.toContain( + 'src/c.ts', + ) + expect( + (agentState as any).base2ActiveWork.unreviewedDirtyReviewableFiles, + ).toEqual([]) + expect((agentState as any).uncommittedUnvalidatedFiles).toEqual([]) + expect((agentState as any).canSuggestFollowups).toBe(true) + } finally { + rmSync(tempDir, { recursive: true, force: true }) + } + }) + test('same-size content drift with restored mtime reopens durable validation and review', () => { - const tempDir = mkdtempSync(join(process.cwd(), '.reviewer-gate-drift-')) + mkdirSync(join(process.cwd(), '.base2-test-scratch'), { recursive: true }) + const tempDir = mkdtempSync( + join(process.cwd(), '.base2-test-scratch', '.reviewer-gate-drift-'), + ) const absolutePath = join(tempDir, 'fixture.ts') const path = relative(process.cwd(), absolutePath).replace(/\\/g, '/') const validationSummary = 'Hook typecheck passed.' @@ -592,43 +786,30 @@ describe('base2 reviewer spawn conditions e2e', () => { const originalMarker = `sha256:${createHash('sha256').update(originalData).digest('hex')}:${originalData.length}` const reviewedFingerprint = reviewableFingerprint(path) const durableFingerprint = gateFingerprint(path, validationSummary) - const agentState = { + const agentState = base2ActiveWorkFixture({ agentId: 'base2-content-drift', - base2ActiveWork: { - changedFiles: [path], - touchedFiles: [path], - pendingGateFiles: [path], - gatePassedFiles: [path], - gatePassedFileMarkers: { [path]: originalMarker }, - gatePassedPendingFiles: [], - gatePassedFingerprint: durableFingerprint, - gatePassedValidationSummary: validationSummary, - gatePassedReviewerVerdict: 'LOOKS_GOOD', - currentPhase: 'final_response_allowed', - latestWorkSummary: '', - openReviewerBlockers: [], - lastValidationSummary: validationSummary, - nextRequiredAction: '', - lastPinnedStateMessage: '', - reviewedReviewableFingerprint: reviewedFingerprint, - reviewReceipts: [ - { - gateId: `code-reviewer:${reviewedFingerprint}`, - reviewer: 'code-reviewer', - verdict: 'LOOKS_GOOD', - snapshotFingerprint: reviewedFingerprint, - reviewedFiles: [path], - reviewedFileCount: 1, - dimensions: {}, - findings: [], - findingCount: 0, - requirementCoverage: [], - requirementCoverageCount: 0, - recordedAt: '2025-01-01T00:00:00.000Z', - }, - ], - }, - } + path, + durableFingerprint, + validationSummary, + gatePassedFileMarkers: { [path]: originalMarker }, + reviewedReviewableFingerprint: reviewedFingerprint, + reviewReceipts: [ + { + gateId: `code-reviewer:${reviewedFingerprint}`, + reviewer: 'code-reviewer', + verdict: 'LOOKS_GOOD', + snapshotFingerprint: reviewedFingerprint, + reviewedFiles: [path], + reviewedFileCount: 1, + dimensions: {}, + findings: [], + findingCount: 0, + requirementCoverage: [], + requirementCoverageCount: 0, + recordedAt: '2025-01-01T00:00:00.000Z', + }, + ], + }) const base2 = createBase2('default') const gen = base2.handleSteps!({ @@ -693,8 +874,9 @@ describe('base2 reviewer spawn conditions e2e', () => { }) test('external symlink is rejected without reading its target', () => { + mkdirSync(join(process.cwd(), '.base2-test-scratch'), { recursive: true }) const projectDir = mkdtempSync( - join(process.cwd(), '.reviewer-gate-symlink-'), + join(process.cwd(), '.base2-test-scratch', '.reviewer-gate-symlink-'), ) const externalDir = mkdtempSync(join(tmpdir(), 'reviewer-gate-target-')) const target = join(externalDir, 'target.ts') @@ -719,28 +901,15 @@ describe('base2 reviewer spawn conditions e2e', () => { const reviewedFingerprint = reviewableFingerprint(path) const durableFingerprint = gateFingerprint(path, validationSummary) - const agentState = { + const agentState = base2ActiveWorkFixture({ agentId: 'base2-external-symlink', - base2ActiveWork: { - changedFiles: [path], - touchedFiles: [path], - pendingGateFiles: [path], - gatePassedFiles: [path], - gatePassedFileMarkers: { [path]: 'unreadable:outside-project-symlink' }, - gatePassedPendingFiles: [], - gatePassedFingerprint: durableFingerprint, - gatePassedValidationSummary: validationSummary, - gatePassedReviewerVerdict: 'LOOKS_GOOD', - currentPhase: 'final_response_allowed', - latestWorkSummary: '', - openReviewerBlockers: [], - lastValidationSummary: validationSummary, - nextRequiredAction: '', - lastPinnedStateMessage: '', - reviewedReviewableFingerprint: '', - reviewReceipts: [], + path, + durableFingerprint, + validationSummary, + gatePassedFileMarkers: { + [path]: 'unreadable:outside-project-symlink', }, - } + }) const base2 = createBase2('default') const gen = base2.handleSteps!({ @@ -797,8 +966,13 @@ describe('base2 reviewer spawn conditions e2e', () => { }) test('external symlink target content drift is rejected without reading target', () => { + mkdirSync(join(process.cwd(), '.base2-test-scratch'), { recursive: true }) const projectDir = mkdtempSync( - join(process.cwd(), '.reviewer-gate-symlink-content-'), + join( + process.cwd(), + '.base2-test-scratch', + '.reviewer-gate-symlink-content-', + ), ) const externalDir = mkdtempSync(join(tmpdir(), 'reviewer-gate-target-')) const target = join(externalDir, 'target.ts') @@ -824,28 +998,15 @@ describe('base2 reviewer spawn conditions e2e', () => { const reviewedFingerprint = reviewableFingerprint(path) const durableFingerprint = gateFingerprint(path, validationSummary) - const agentState = { + const agentState = base2ActiveWorkFixture({ agentId: 'base2-symlink-content-drift', - base2ActiveWork: { - changedFiles: [path], - touchedFiles: [path], - pendingGateFiles: [path], - gatePassedFiles: [path], - gatePassedFileMarkers: { [path]: 'unreadable:outside-project-symlink' }, - gatePassedPendingFiles: [], - gatePassedFingerprint: durableFingerprint, - gatePassedValidationSummary: validationSummary, - gatePassedReviewerVerdict: 'LOOKS_GOOD', - currentPhase: 'final_response_allowed', - latestWorkSummary: '', - openReviewerBlockers: [], - lastValidationSummary: validationSummary, - nextRequiredAction: '', - lastPinnedStateMessage: '', - reviewedReviewableFingerprint: '', - reviewReceipts: [], + path, + durableFingerprint, + validationSummary, + gatePassedFileMarkers: { + [path]: 'unreadable:outside-project-symlink', }, - } + }) const base2 = createBase2('default') const gen = base2.handleSteps!({ @@ -985,6 +1146,45 @@ describe('base2 reviewer spawn conditions e2e', () => { }) }) + test('explicit Git delivery adopts only reviewable turn-start dirty files', () => { + // Git-delivery adoption must NOT drag the whole dirty worktree into the + // gate. Non-reviewable dirt (docs, session STATE.json, jsonl) belongs to + // the worktree / other tabs, not to this conversation's review; only + // reviewable source/test files the delivery is committing enter the gate. + const base2 = createBase2('default') + const agentState = { agentId: 'base2-custom' } + const prompt = + 'Check the full validation gate, then commit and push our current changes' + const gen = base2.handleSteps!({ agentState, prompt, params: {} } as any) + + expect(gen.next().value).toMatchObject({ toolName: 'git_status' }) + expect( + gen.next( + feedJson({ + status: + ' M src/owned.ts\n M docs/readme.md\n M .agents/sessions/x/STATE.json\n?? src/new.ts', + }), + ).value, + ).toMatchObject({ toolName: 'list_jobs' }) + expect(gen.next(feedListJobs()).value).toMatchObject({ + toolName: 'spawn_agent_inline', + input: { agent_type: 'context-pruner' }, + }) + expect((agentState as any).base2ActiveWork).toMatchObject({ + changedFiles: ['src/owned.ts', 'src/new.ts'], + touchedFiles: ['src/owned.ts', 'src/new.ts'], + pendingGateFiles: ['src/owned.ts', 'src/new.ts'], + currentPhase: 'awaiting_validation', + }) + // Non-reviewable turn-start dirt is never adopted into the gate. + expect((agentState as any).base2ActiveWork.pendingGateFiles).not.toContain( + 'docs/readme.md', + ) + expect((agentState as any).base2ActiveWork.pendingGateFiles).not.toContain( + '.agents/sessions/x/STATE.json', + ) + }) + test('non-Git turn does not adopt turn-start dirty files', () => { const base2 = createBase2('default') const agentState = { agentId: 'base2-custom' } @@ -1072,4 +1272,128 @@ describe('base2 reviewer spawn conditions e2e', () => { expect((agentState as any).commitScopeBypassAuthorized).toBe(true) expect((agentState as any).canSuggestFollowups).toBeUndefined() }) + + test('test-local gateFileMarker matches production readGateFileContentMarker (parity guard)', () => { + // The test-local `gateFileMarker` (and its symlink escape logic) is a hand + // mirror of the production `readGateFileContentMarker` security-sensitive + // code path. This parity guard prevents silent drift: it runs BOTH the + // test mirror and the real extracted production marker on the exact same + // fixtures and asserts they agree, so the security assertions (e.g. + // external-symlink rejection) verify the real gate, not just the mirror. + const productionMarker = loadProductionGateFileContentMarker() + mkdirSync(join(process.cwd(), '.base2-test-scratch'), { recursive: true }) + const tempDir = mkdtempSync( + join(process.cwd(), '.base2-test-scratch', '.reviewer-marker-parity-'), + ) + const externalDir = mkdtempSync(join(tmpdir(), 'reviewer-marker-target-')) + try { + const regularAbsolute = join(tempDir, 'a.ts') + writeFileSync(regularAbsolute, 'export const a = 1\n') + const regularPath = relative(process.cwd(), regularAbsolute).replace( + /\\/g, + '/', + ) + expect(gateFileMarker(regularPath)).toBe( + productionMarker(regularPath), + ) + + const missingAbsolute = join(tempDir, 'missing.ts') + const missingPath = relative(process.cwd(), missingAbsolute).replace( + /\\/g, + '/', + ) + expect(gateFileMarker(missingPath)).toBe(productionMarker(missingPath)) + + // External symlink: both implementations must reject without reading the + // target. Windows requires elevated privileges to create symlinks; skip there. + if (process.platform !== 'win32') { + const target = join(externalDir, 'target.ts') + writeFileSync(target, 'export const value = 1\n') + const symlinkAbsolute = join(tempDir, 'fixture.ts') + try { + symlinkSync(target, symlinkAbsolute, 'file') + } catch { + return + } + const symlinkPath = relative(process.cwd(), symlinkAbsolute).replace( + /\\/g, + '/', + ) + expect(gateFileMarker(symlinkPath)).toBe( + productionMarker(symlinkPath), + ) + expect(gateFileMarker(symlinkPath)).toBe( + 'unreadable:outside-project-symlink', + ) + } + + // Internal symlink (target under cwd): both implementations must RUN the + // symlink-HASH branch (not the outside-project rejection) and agree on + // the symlink-sha256 marker, exercising the ``${index}:${link}`` scheme. + // Nested under a subdir so the symlink is NOT segment index 0 — this + // catches any drift between a hardcoded `0:` and production's + // per-segment index. Windows requires elevated privileges for file + // symlinks; skip there. + if (process.platform !== 'win32') { + const internalDir = join(tempDir, 'internal') + mkdirSync(internalDir, { recursive: true }) + const internalTarget = join(internalDir, 'target.ts') + writeFileSync(internalTarget, 'export const value = 42\n') + const linkDir = join(tempDir, 'links') + mkdirSync(linkDir, { recursive: true }) + const internalSymlinkAbsolute = join(linkDir, 'link.ts') + try { + symlinkSync(internalTarget, internalSymlinkAbsolute, 'file') + } catch { + return + } + const internalSymlinkPath = relative( + process.cwd(), + internalSymlinkAbsolute, + ).replace(/\\/g, '/') + const internalMarker = gateFileMarker(internalSymlinkPath) + // Both implementations must exercise (not reject) the symlink branch. + expect(internalMarker).toMatch(/^symlink-sha256:/) + expect(internalMarker).toBe(productionMarker(internalSymlinkPath)) + } + + // Mid-path (intermediate-directory) symlink: the symlink is an + // INTERMEDIATE segment, not the final one, so its final segment is a + // regular file. Production walkEverySegment records the intermediate link + // by its own segment index and still returns a symlink-sha256 marker; the + // mirror must agree rather than falling back to a plain sha256 just + // because the final segment is not itself a symlink. This closes the gap + // that only exercised the symlink-hash index branch for final-segment + // symlinks (index = segment-count-minus-1). Windows requires elevated + // privileges for symlinks; skip there. + if (process.platform !== 'win32') { + const realDir = join(tempDir, 'real') + const realSubDir = join(realDir, 'sub') + mkdirSync(realSubDir, { recursive: true }) + const midTarget = join(realSubDir, 'file.ts') + writeFileSync(midTarget, 'export const value = 7\n') + const aliasDir = join(tempDir, 'alias') + let midSymlinkAbsolute: string + try { + symlinkSync(realDir, aliasDir, 'dir') + midSymlinkAbsolute = join(aliasDir, 'sub', 'file.ts') + } catch { + return + } + const midSymlinkPath = relative( + process.cwd(), + midSymlinkAbsolute, + ).replace(/\\/g, '/') + const midMarker = gateFileMarker(midSymlinkPath) + // Production records the intermediate symlink segment with its own + // index, so the marker must be symlink-sha256 (not plain sha256), and + // the mirror must agree with production exactly. + expect(midMarker).toMatch(/^symlink-sha256:/) + expect(midMarker).toBe(productionMarker(midSymlinkPath)) + } + } finally { + rmSync(tempDir, { recursive: true, force: true }) + rmSync(externalDir, { recursive: true, force: true }) + } + }) }) diff --git a/agents/general-agent/general-agent.ts b/agents/general-agent/general-agent.ts index 77aab84d70..faa4d4ae73 100644 --- a/agents/general-agent/general-agent.ts +++ b/agents/general-agent/general-agent.ts @@ -79,6 +79,7 @@ export const createGeneralAgent = (options: { 'query_index', 'read_files', 'read_subtree', + 'code_search', 'task_completed', 'write_audit_findings', ], @@ -94,7 +95,7 @@ export const createGeneralAgent = (options: { !isGpt5 && `If indexed evidence leaves explicit coverage gaps, spawn bounded parallel waves of non-overlapping file-picker/code-searcher/researcher tasks. Join each wave before deciding whether more coverage is needed; do not restart the same discovery through multiple agent layers.`, `File-picker and code-searcher are discovery-only helpers. Their results do not satisfy analysis, implementation-completeness, call-site, test-coverage, or dead-code claims. Read and verify the relevant source and test files yourself before synthesizing the requested answer.`, - `For ripgrep-style content search, spawn the code-searcher agent; \`code_search\` is a registered runtime tool but is intentionally not granted to you, so calling it directly is rejected. When you spawn code-searcher, pass its required params or the spawn fails: code-searcher needs \`params.searchQueries\` (an array of { pattern } objects, e.g. { "params": { "searchQueries": [{ "pattern": "createUser", "flags": "-g *.ts" }] } }); put it in \`params\`, not only in the prose prompt.`, + `For ripgrep-style content search, prefer direct \`code_search\` for single-pattern work (pattern/flags/cwd/maxResults). Spawn code-searcher only for multi-query batch search, and pass required \`params.searchQueries\` (an array of { pattern } objects, e.g. { "params": { "searchQueries": [{ "pattern": "createUser", "flags": "-g *.ts" }] } }); put it in \`params\`, not only in the prose prompt.`, `When params.sessionSlug and params.shardId are provided, this is a durable audit shard. params.snapshotId must be the exact inspect_codebase_structure snapshot; copy it into write_audit_findings.snapshotId. If params.snapshotId is absent or blank, the shard is unbound-by-snapshot and fails closed: do not call write_audit_findings (its snapshot-bound structural receipt cannot satisfy the completion gate); instead analyze the assigned files and return your findings inline. Analyze the assigned files, call write_audit_findings exactly once with structured findings and full subsystem/feature/file/domain coverage, then return only its compact artifact receipt, including structuralReceipt. Do not repeat findings in your final response.`, `Do not stop after announcing a tool call or delegating discovery. In the same final response that contains the requested answer or compact audit receipt, call task_completed. Never call task_completed while required reads, synthesis, coverage, or audit artifact persistence remain unfinished.`, ).join('\n'), diff --git a/agents/guides/editor-writers-and-repair.md b/agents/guides/editor-writers-and-repair.md new file mode 100644 index 0000000000..5d1cd16bdc --- /dev/null +++ b/agents/guides/editor-writers-and-repair.md @@ -0,0 +1,157 @@ +# Editor, Writers, and Repair (Public Spawn Contract) + +How `editor`, `repair-editor`, `test-writer`, and `doc-writer` relate under base2, when the runtime spawns them automatically, and what may run in parallel. + +This is the live public contract from the orchestrator gate and agent templates. It does not invent spawn conditions beyond what base2 and the agent definitions enforce. + +## Roster availability (mode gates) + +| Agent | Default implementation | Fast | Plan-only | +| --- | --- | --- | --- | +| `editor` | Spawnable | Not spawnable (inline `edit_transaction`) | Not spawnable | +| `repair-editor` | Spawnable | Not spawnable | Not spawnable | +| `test-writer` | Spawnable | Spawnable | Not spawnable | +| `doc-writer` | Spawnable | Spawnable | Not spawnable | + +Implementation modes that still run the automated validation/reviewer gate use the aux + repair paths below. Fast / no-validation / plan-only skip that automated gate; plan mode remains read-only for mutation agents. + +## Role split + +| Agent | Owns | Does not own | +| --- | --- | --- | +| `editor` | Non-trivial **implementation** edits after discovery. Self-contained handoff only. Mutates via `edit_transaction`. | Validation, basher, review, git, todos, visual smoke, shell cleanup. Parent-only work stays with the orchestrator. | +| `repair-editor` | **Finding-scoped** fixes: parseable validation diagnostics or stable reviewer finding IDs. Same edit surface as editor plus `read_subtree` for diagnosis. | Unrelated refactors, docs, feature work, or protocol/attestation failures (snapshot mismatch is not a source repair). | +| `test-writer` | New/extended tests under existing test paths only (`*.test.*`, `*.spec.*`, `__tests__/`, `test/`, `tests/`). Reports `requestedValidation` for the parent/basher. | Production source (except when a test is unobservable without a minimal source change — still not the default). Running terminals itself. | +| `doc-writer` | Documentation paths only (`docs/**`, `README*`, `**/*.md`, `**/*.mdx`). Verifies against source; never invents API behavior. | Production source edits. | + +All four use structured `set_output` receipts. Writers expose `status`, `completionKind` (`changed` \| `noop`), `changedFiles`, and `evidence`. The runtime accepts writer receipts only when status is `completed` and either: + +- `completionKind=changed` with non-empty `changedFiles`, or +- `completionKind=noop` with empty `changedFiles` and non-empty `evidence`. + +Empty or partial output blocks finalization for automated writer spawns and marks **reduced assurance** rather than looping forever. + +`set_output` itself is for **spawned subagents**. The root orchestrator must not call it; absence from the root toolset is expected. Nested `{ output: { status, ... } }` is accepted for editor-family agents when the nested object matches the agent schema. + +## Manual spawn (orchestrator) + +Use phase-triggered delegation, not random spawns: + +- **`editor`** — after discovery for non-trivial source changes in default mode. Prompt must be implementation-only: Requirements, Target files, Constraints/non-goals, Patterns, Risks. Omit parent-only work. +- **Direct orchestrator edit** — narrow exception only: one file, roughly ≤12 lines, no behavior/public-contract change, no required tests, no security/concurrency risk, no open reviewer findings. Otherwise use `editor`. +- **`repair-editor`** — validation/reviewer repairs with exact diagnostics or finding IDs. Prefer runtime-owned repair loops over free-form re-edits when the gate already owns the findings. +- **`test-writer` / `doc-writer`** — when documentation or test coverage is required or directly implied by acceptance criteria. Pass `params.target_files` / `params.source_files` (and `test_command` / optional `target_doc_files`) plus a self-contained verified contract in the prompt; writers do not inherit parent history. + +Subagent deadlines: omit top-level `timeout_seconds` for productive editors/writers (`-1` / omitted = no wall-clock deadline) unless the user requests a bound or the child is intentionally diagnostic. + +## Automated aux gates (pre-reviewer) + +When the automated gate is on and edits produced a non-empty pending file set, base2 runs **pre-reviewer aux work once per distinct aux-relevant pending set**, then the final hooks + `code-reviewer` gate. + +**Order (sequential, blocking):** + +1. `test-writer` (predicate-gated) +2. `doc-writer` (predicate-gated) +3. `security-reviewer` (security-sensitive pending paths) +4. Routed reviewer-family specialists (batched where selected) +5. File-change hooks + final `code-reviewer` + +Each aux step uses `spawn_agent_inline` (or `spawn_agents` for specialist batches / repair). The generator **waits** for the child before the next gate. After any aux spawn fires, the loop re-enters so validation/review sees writer outputs in the pending set. + +Done-flags (`testWriterGateDone`, `docWriterGateDone`, …) reset only when the **aux-relevant** pending subset changes — not when writer outputs (new tests/docs) join pending. That prevents infinite re-spawn loops. + +### When automated `test-writer` runs + +All of the following must hold: + +1. Validation gate is active, edits happened, and pending gate files are non-empty. +2. **User prompt requires tests** — positive match on add/write/update/fix/increase/improve + `test(s)` or `test coverage`, and **not** a nearby “do not / don't / without / no … tests|test coverage” negation. +3. Target selection yields at least one group: non-test source files with an inferable package test command (monorepo package roots, ecosystem fallbacks such as `bun test` / `pytest` / `go test`, or project-aware `get_affected_tests` + `get_build_targets` when the prompt requires tests). + +If the prompt does not require tests, or there are no eligible source targets, the gate marks the test-writer step done and **skips silently**. + +After a successful writer receipt, the parent may run the group’s `test_command` via basher. Crash, incomplete receipt, or failed validation → reduced assurance and continue (no infinite retry). + +**Coverage-only reviewer findings:** if the final code-reviewer returns blockers that are **all** pure test-coverage gaps, the harness routes repair to **`test-writer`**, not `repair-editor`. Mixed or code findings still use `repair-editor`. + +### When automated `doc-writer` runs + +All of the following must hold: + +1. Validation gate is active, edits happened, and pending gate files are non-empty. +2. **User prompt requires docs** — match on `docs` / `documentation` / `document` / `readme` / `guide`, without a nearby negation of those terms. +3. `selectDocWriterTargets` keeps **public API source files** only (non-test source under the usual extensions; excludes tests, docs, evals, `.agents/`, generated files, pure config/markdown). + +Write scope for the automated handoff is **package-rooted** from those source paths (`docs/**`, `README*`, `**/*.md`, `**/*.mdx` under each inferred workspace root). The agent may **read** the whole repo to verify contracts; it may **write** only documentation paths. It does not blanket-update every markdown file in the monorepo — targets follow the changed public sources and neighboring doc layout. + +If docs are not required in the prompt or no public-API sources are pending, the step is marked done and skipped. + +### Why writers often “never spawn” + +- Automated spawns are **prompt-gated**: ordinary “implement feature X” without an explicit tests/docs requirement will not fire the aux writers. +- Eligible **source** must exist in the pending set (tests/docs alone do not re-trigger the same aux cycle). +- Plan mode withholds both writers; fast mode still has them on the roster but only under the same prompt/target predicates when the gate runs. +- Manual spawns still require a complete params/handoff envelope; missing structured receipts fail the automated gate’s acceptance checks. +- Prefer stating tests/docs in the user request or acceptance criteria when those deliverables are required — that is the intended trigger, not hoping the gate invents coverage work. + +## Repair loops (after validation or review) + +| Trigger | Repair agent | Parallelism | +| --- | --- | --- | +| Parseable file-change hook failures | `repair-editor` with `VF-*` finding IDs | Sequential: hooks → repair → re-hooks | +| Security-reviewer blockers | `repair-editor` on open security findings | After security aux; then re-validation + fresh security review | +| Specialist blockers | `repair-editor` per specialist finding set | Specialists may have been batched; repair for a blocking specialist is sequential | +| Code-reviewer blockers (code) | `repair-editor` | After final review; then hooks + re-review | +| Code-reviewer blockers (all coverage) | `test-writer` | Same sequential repair → hooks → re-review path | + +Repair budgets may be unlimited by default or capped via createBase2 / env (`OPENBUFF_MAX_REPAIR_ROUNDS`, `OPENBUFF_MAX_REVIEWER_REPAIR_ROUNDS`, `OPENBUFF_MAX_SPECIALIST_REPAIR_ROUNDS`). Incomplete receipts, crashes, or **no snapshot-visible progress** fail closed and stop automatic retry. + +## Cohesion: can they work in parallel? + +**Yes, with hard join rules:** + +| Combination | Allowed? | Notes | +| --- | --- | --- | +| Context agents (file-picker, code-searcher, researchers) in parallel | Yes | Bounded waves ≤8 per `spawn_agents` call; join before dependent edits. | +| Multiple bashers for independent validation commands | Yes | Join all results before finalizing. Sequential if command B depends on A. | +| Static `code-reviewer` / specialists **with** validation still running | Only if review is explicitly validation-independent | Parallel approval is **not** final until validation completes. Prefer validation first for fragile harness/editor work, then review with the summary. | +| Routed specialists in one `spawn_agents` batch | Yes (runtime-owned) | Gate batches selected specialists; attestation/retry is gate-owned. | +| Aux `test-writer` then `doc-writer` then security | **No** (by design) | Sequential blocking yields so each sees a stable pending set. | +| `editor` and `test-writer` on the same change without a join | **No** | Implementation must land before coverage writers or coverage-repair can target real source. | +| `editor` and `repair-editor` on the same findings | **No** | Repair owns open gate findings; do not race a second implementation editor over the same IDs. | +| Root `edit_transaction` while repair-editor runs | Avoid | Fragile debug/fix loops should be read → one edit path → validation, sequential. | + +General rule from the orchestrator: **parallelize context, independent tests, and static review only when they do not depend on each other.** During a fragile repair loop, stay sequential. + +## Handoff shape (writers) + +Automated and manual writer spawns should carry a typed handoff: task ID, objective, requirements, acceptance criteria, context paths with confidence, invariants (`Do not modify production source files.` for pure writers), permissions, and success criteria. + +Minimal manual examples: + +```text +spawn test-writer + prompt: focused tests for ; framework matches package + params.target_files: ["packages/foo/src/bar.ts"] + params.test_command: "cd packages/foo && bun test" +``` + +```text +spawn doc-writer + prompt: document public contract of ; match agents/guides style + params.source_files: ["packages/foo/src/bar.ts"] + params.target_doc_files: ["packages/foo/README.md"] # optional +``` + +```text +spawn repair-editor + handoff findings: RF-… / VF-… with exact text and files + writablePaths: pending gate files only +``` + +## Related guides + +- Automated hooks → code-reviewer finalization: gate awareness section in the orchestrator system prompt (`GATE: PENDING` / `GATE: PASSED`). +- Specialist risk routing and `params.snapshot_id`: `agents/guides/specialist-routing.md`. +- Advisory pre-edit security patterns: `agents/guides/security-review.md`. +- Edit authorization (`edit_transaction`, capabilities): `packages/agent-runtime/docs/deterministic-edit-system.md`. diff --git a/agents/guides/specialist-routing.md b/agents/guides/specialist-routing.md index 26666bd7df..338a714b8f 100644 --- a/agents/guides/specialist-routing.md +++ b/agents/guides/specialist-routing.md @@ -13,3 +13,5 @@ Use specialists when repository evidence or the requested outcome crosses one of Gather the exact source and snapshot evidence before spawning. Advisory specialists inform the plan; reviewer specialists can block their scoped risk dimension. They complement rather than replace targeted validation and the final code-reviewer gate. Post-edit reviewer-family specialists are routed automatically by the orchestrator's gate. Do not manually re-spawn them after edits, after compaction, or merely because set_output is unavailable; wait for the runtime-owned gate result. Manual specialist calls are for pre-edit advisory work or an explicit user request. When you do spawn one, pass its exact params contract: reviewer-family specialists (product-reviewer, performance-specialist, reliability-reviewer, migration-reviewer, compatibility-reviewer, accessibility-reviewer, ux-visual-reviewer, dependency-reviewer, evaluator) require params.snapshot_id set to the gate-assigned opaque `v3:…` token from the parent gate (not the bare hex `snapshotId` from `get_change_review_bundle`). security-reviewer is the exception: it requires params.changed_files plus params.snapshot_fingerprint and does not accept snapshot_id. Spawning with the wrong or missing snapshot key fails the spawn. + +For `editor`, `repair-editor`, `test-writer`, and `doc-writer` spawn rules, aux-gate ordering, writer prompt predicates, and parallel join discipline, see `agents/guides/editor-writers-and-repair.md`. diff --git a/agents/reviewer/code-reviewer.ts b/agents/reviewer/code-reviewer.ts index 49f4969229..a5022086fa 100644 --- a/agents/reviewer/code-reviewer.ts +++ b/agents/reviewer/code-reviewer.ts @@ -40,6 +40,7 @@ export const createReviewer = ( type: 'object', properties: { schemaVersion: { type: 'number' }, + family: { type: 'string', enum: ['reviewer'] }, verdict: { type: 'string', enum: ['LOOKS_GOOD', 'NON_BLOCKING', 'BLOCKING'], @@ -86,6 +87,7 @@ export const createReviewer = ( }, required: [ 'schemaVersion', + 'family', 'verdict', 'snapshotFingerprint', 'reviewedFiles', diff --git a/common/src/tools/params/__tests__/coerce-to-array.test.ts b/common/src/tools/params/__tests__/coerce-to-array.test.ts index dfb31bac9b..40e68cc4ff 100644 --- a/common/src/tools/params/__tests__/coerce-to-array.test.ts +++ b/common/src/tools/params/__tests__/coerce-to-array.test.ts @@ -294,6 +294,149 @@ describe('normalizeSpawnAgentList', () => { expect(normalizeSpawnAgentList([entry])).toEqual([entry]) }) + it('moves a top-level code-searcher pattern into params.searchQueries', () => { + expect( + normalizeSpawnAgentList([ + { + agent_type: 'code-searcher', + pattern: 'foo', + flags: '-g *.ts', + params: {}, + }, + ]), + ).toEqual([ + { + agent_type: 'code-searcher', + pattern: 'foo', + flags: '-g *.ts', + params: { + searchQueries: [{ pattern: 'foo', flags: '-g *.ts' }], + }, + }, + ]) + }) + + it('builds code-searcher searchQueries from params.pattern', () => { + expect( + normalizeSpawnAgentList([ + { + agent_type: 'code-searcher', + params: { pattern: 'bar', cwd: 'src', maxResults: 12 }, + }, + ]), + ).toEqual([ + { + agent_type: 'code-searcher', + params: { + pattern: 'bar', + cwd: 'src', + maxResults: 12, + searchQueries: [{ pattern: 'bar', cwd: 'src', maxResults: 12 }], + }, + }, + ]) + }) + + it('builds code-searcher searchQueries from a patterns array', () => { + expect( + normalizeSpawnAgentList([ + { + agent_type: 'code-searcher', + patterns: ['one', 'two'], + params: {}, + }, + ]), + ).toEqual([ + { + agent_type: 'code-searcher', + patterns: ['one', 'two'], + params: { + searchQueries: [{ pattern: 'one' }, { pattern: 'two' }], + }, + }, + ]) + }) + + it('moves a top-level code-searcher searchQueries array into params', () => { + expect( + normalizeSpawnAgentList([ + { + agent_type: 'code-searcher', + searchQueries: [{ pattern: 'top' }], + params: {}, + }, + ]), + ).toEqual([ + { + agent_type: 'code-searcher', + searchQueries: [{ pattern: 'top' }], + params: { searchQueries: [{ pattern: 'top' }] }, + }, + ]) + }) + + it('wraps a single code-searcher searchQueries object into an array', () => { + expect( + normalizeSpawnAgentList([ + { + agent_type: 'code-searcher', + params: { searchQueries: { pattern: 'solo' } }, + }, + ]), + ).toEqual([ + { + agent_type: 'code-searcher', + params: { searchQueries: [{ pattern: 'solo' }] }, + }, + ]) + }) + + it('prefers nested code-searcher searchQueries over a top-level pattern', () => { + expect( + normalizeSpawnAgentList([ + { + agent_type: 'code-searcher', + pattern: 'top-level', + params: { searchQueries: [{ pattern: 'nested' }] }, + }, + ]), + ).toEqual([ + { + agent_type: 'code-searcher', + pattern: 'top-level', + params: { searchQueries: [{ pattern: 'nested' }] }, + }, + ]) + }) + + it('does not invent code-searcher patterns from prompt prose', () => { + expect( + normalizeSpawnAgentList([ + { + agent_type: 'code-searcher', + prompt: 'Search for normalizeSpawnAgentList', + params: {}, + }, + ]), + ).toEqual([ + { + agent_type: 'code-searcher', + prompt: 'Search for normalizeSpawnAgentList', + params: {}, + }, + ]) + }) + + it('does not move pattern fields for non-code-searcher agents', () => { + const entry = { + agent_type: 'editor', + pattern: 'should-not-move', + patterns: ['a', 'b'], + params: {}, + } + expect(normalizeSpawnAgentList([entry])).toEqual([entry]) + }) + it('repairs a stringified handoff object without decoding nested strings', () => { expect( normalizeSpawnAgentList([ diff --git a/common/src/tools/params/utils.ts b/common/src/tools/params/utils.ts index f613b98315..55d911ec38 100644 --- a/common/src/tools/params/utils.ts +++ b/common/src/tools/params/utils.ts @@ -365,6 +365,117 @@ export function normalizeSpawnAgentList(value: unknown, depth = 0): unknown { paramsRepaired = true } + // Code-searcher: recover params.searchQueries from explicit structured + // fields only (mirror basher command repair). Prefer nested params over + // top-level. Never invent patterns from prompt prose; leave ambiguous + // shapes untouched so Zod fails closed. + if (record.agent_type === 'code-searcher') { + const wrapSearchQueryObject = ( + value: unknown, + ): unknown[] | undefined => { + if ( + value === null || + typeof value !== 'object' || + Array.isArray(value) + ) { + return undefined + } + const query = value as Record + if ( + typeof query.pattern !== 'string' || + query.pattern.trim() === '' + ) { + return undefined + } + return [value] + } + + const buildQueryFromPattern = ( + pattern: string, + source: Record, + ): Record => { + const query: Record = { pattern } + if (typeof source.flags === 'string') query.flags = source.flags + if (typeof source.cwd === 'string') query.cwd = source.cwd + if ( + typeof source.maxResults === 'number' && + Number.isFinite(source.maxResults) + ) { + query.maxResults = source.maxResults + } + return query + } + + const nonEmptyStringPatterns = ( + value: unknown, + ): string[] | undefined => { + if (!Array.isArray(value) || value.length === 0) return undefined + if ( + !value.every( + (entry) => typeof entry === 'string' && entry.trim() !== '', + ) + ) { + return undefined + } + return value as string[] + } + + // Single object at params.searchQueries → one-element array. + if (paramsRecord.searchQueries !== undefined) { + const wrapped = wrapSearchQueryObject(paramsRecord.searchQueries) + if (wrapped) { + paramsRecord.searchQueries = wrapped + paramsRepaired = true + } + // Non-empty string that is not a JSON array was already left alone + // by ARRAY_PARAM_KEYS; do not reinterpret it here. + } else { + // Prefer nested structured fields over top-level aliases. + if ( + typeof paramsRecord.pattern === 'string' && + paramsRecord.pattern.trim() !== '' + ) { + paramsRecord.searchQueries = [ + buildQueryFromPattern(paramsRecord.pattern, paramsRecord), + ] + paramsRepaired = true + } else { + const nestedPatterns = nonEmptyStringPatterns(paramsRecord.patterns) + if (nestedPatterns) { + paramsRecord.searchQueries = nestedPatterns.map((pattern) => ({ + pattern, + })) + paramsRepaired = true + } else if (Array.isArray(record.searchQueries)) { + paramsRecord.searchQueries = record.searchQueries + paramsRepaired = true + } else { + const wrappedTop = wrapSearchQueryObject(record.searchQueries) + if (wrappedTop) { + paramsRecord.searchQueries = wrappedTop + paramsRepaired = true + } else if ( + typeof record.pattern === 'string' && + record.pattern.trim() !== '' + ) { + paramsRecord.searchQueries = [ + buildQueryFromPattern(record.pattern, record), + ] + paramsRepaired = true + } else { + const topPatterns = nonEmptyStringPatterns(record.patterns) + if (topPatterns) { + paramsRecord.searchQueries = topPatterns.map((pattern) => ({ + pattern, + })) + paramsRepaired = true + } + } + } + } + } + } + // Snapshot-scoped specialists verify the supplied fingerprint against // the live review bundle, so recovering an explicitly labelled SHA from // their prompt does not grant authority or bypass freshness checks. This diff --git a/docs/agents-and-tools.md b/docs/agents-and-tools.md index 837c7ea15f..eb34d5de5f 100644 --- a/docs/agents-and-tools.md +++ b/docs/agents-and-tools.md @@ -502,6 +502,20 @@ Mid-turn git-status absorption must not claim foreign worktree dirt from concurr The pure helper lives in `agents/base2/gate-concurrency.ts`. The inline copy is generated via `scripts/generate-gate-helpers.ts` from `gate-concurrency.ts` into the `` marker region of `createBase2.handleSteps` (serialized via `.toString()` / `new Function(...)`). Freshness is enforced by `agents/__tests__/gate-helpers-freshness.test.ts` and by `prebuild:agents` regenerating the region. +### Explicit Git-delivery adoption is reviewable-scoped + +A turn with explicit Git-delivery intent (`commit`/`push`/`stage` the changes — the +only turn type that claims files already dirty at turn start) adopts turn-start +worktree dirt into the gate through **reviewable files only**. The delivery path +runs `selectReviewableGateFiles(initialGitStatusFiles)` instead of absorbing the +full dirty worktree, so non-reviewable dirt (docs, session `STATE.json`, `.jsonl`, +config) that belongs to the worktree or other tabs never enters `pendingGateFiles` +and is never pushed onto the reviewer's attestation list. Reviewable source/test +files the delivery is committing still enter the gate. This mirrors the general +mid-turn absorb rule above and keeps a clean `"commit our changes"` turn from +turning into a worktree-wide review. Regression coverage: +`agents/e2e/reviewer-spawn-conditions.e2e.test.ts`. + ### `AgentState.selfMutatedPaths` Optional JSON-safe `string[]` on `AgentState` in `common/src/types/session-state.ts`. The runtime publishes it after each stream step so mid-turn absorption can credit process-owned writes without sweeping the whole dirty tree. @@ -636,19 +650,20 @@ registry. Calling a tool the agent was not granted is rejected: the runtime fails closed, changes nothing, and returns a diagnostic instead of executing the tool. -Codebase search for orchestrator/base agents (`base2` / `base-deep`) is done -with `query_index` (the local graph index) or by spawning the `code-searcher` -agent. `code_search` is a registered tool in the global registry but is not -granted to those agents, so calling it directly from an orchestrator is -rejected. +Codebase search for orchestrator/base agents (`base2` / `base-deep`) and the +`general-agent` discovery/analysis shard may use `code_search` directly for +single-pattern content search, +`query_index` for graph/index retrieval, or spawn the `code-searcher` agent for +multi-query batch search with `params.searchQueries`. Ungranted tools still +fail closed. The rejection message names the tools the agent actually has available. When the attempted name is a real-but-ungranted registry tool, the message says so; for a likely typo it also suggests a near lexical match ("Did you mean ..."). When you hit this error, pick a tool from the listed available tools, or spawn an agent that provides the capability (for example, spawn `code-searcher` for -codebase search). Do not retry the same unavailable name — the result will not -change. +multi-query batch search). Do not retry the same unavailable name — the result +will not change. ### Background shell jobs (`check_job` / `read_logs` / `kill_job` / `list_jobs`) diff --git a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts index 00d17cef6d..29b3d90ac3 100644 --- a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts +++ b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts @@ -3303,7 +3303,22 @@ describe('buildUnavailableToolMessage', () => { expect(message).toContain('is not available for agent `base2`') }) - it('gives concrete code-searcher recovery for ungranted content-search tools', () => { + it('prefers direct code_search when that tool is already available', () => { + for (const toolName of ['code_search', 'find_files_matching_content']) { + const message = buildUnavailableToolMessage({ + toolName, + agentId: 'base2', + availableTools: ['read_files', 'code_search'], + input: { pattern: 'alpha' }, + }) + + expect(message).toContain('Use the granted `code_search` tool directly') + expect(message).toContain('params.searchQueries') + expect(message).not.toContain('"pattern": "alpha"') + } + }) + + it('gives concrete code-searcher recovery when code_search is unavailable', () => { for (const toolName of ['code_search', 'find_files_matching_content']) { const message = buildUnavailableToolMessage({ toolName, @@ -3313,9 +3328,23 @@ describe('buildUnavailableToolMessage', () => { expect(message).toContain('code-searcher') expect(message).toContain('searchQueries') + expect(message).toContain('"pattern": ""') } }) + it('inlines an explicit input pattern into the code-searcher spawn recipe', () => { + const message = buildUnavailableToolMessage({ + toolName: 'code_search', + agentId: 'base2', + availableTools: ['read_files'], + input: { pattern: 'normalizeSpawnAgentList' }, + }) + + expect(message).toContain('code-searcher') + expect(message).toContain('"pattern": "normalizeSpawnAgentList"') + expect(message).not.toContain('"pattern": ""') + }) + it('suggests the closest granted tool for a likely typo', () => { const message = buildUnavailableToolMessage({ toolName: 'read_file', diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index 62b8e8f063..66a5917ca4 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -1028,8 +1028,9 @@ export function buildUnavailableToolMessage(params: { toolName: string agentId: string availableTools: string[] + input?: unknown }): string { - const { toolName, agentId, availableTools } = params + const { toolName, agentId, availableTools, input } = params const availableList = availableTools.length > 0 ? availableTools.map((name) => `\`${name}\``).join(', ') @@ -1039,12 +1040,26 @@ export function buildUnavailableToolMessage(params: { // granted. Point the model at the granted tools / spawnable agents instead // of letting it guess another unavailable name. if ((toolNames as readonly string[]).includes(toolName)) { - // Concrete recovery for the two content-search tools the root orchestrator - // is intentionally not granted: point the model at the code-searcher agent - // with a copyable params shape instead of the generic sentence. This stays - // message-only; the tool remains fail-closed and nothing is auto-spawned. + // Concrete recovery for content-search tools: prefer direct code_search + // when already granted; otherwise point at the code-searcher spawn recipe. + // This stays message-only; the tool remains fail-closed and nothing is + // auto-spawned. When the rejected input carried an explicit pattern, bake + // that exact string into the spawn recipe instead of a placeholder. if (toolName === 'code_search' || toolName === 'find_files_matching_content') { - return `${base} \`${toolName}\` is a registered tool the root orchestrator is not granted; spawn the code-searcher agent instead: { "agent_type": "code-searcher", "params": { "searchQueries": [{ "pattern": "", "flags": "-g *.ts" }] } }.` + if (availableTools.includes('code_search')) { + return `${base} Use the granted \`code_search\` tool directly (pattern/flags/cwd/maxResults). For multi-query batching, spawn code-searcher with params.searchQueries.` + } + const inputPattern = + input !== null && + typeof input === 'object' && + !Array.isArray(input) && + typeof (input as Record).pattern === 'string' && + ((input as Record).pattern as string).trim() !== '' + ? ((input as Record).pattern as string) + : undefined + const patternJson = + inputPattern !== undefined ? JSON.stringify(inputPattern) : '""' + return `${base} \`${toolName}\` is a registered tool but is not granted to this agent; spawn the code-searcher agent instead: { "agent_type": "code-searcher", "params": { "searchQueries": [{ "pattern": ${patternJson}, "flags": "-g *.ts" }] } }.` } return `${base} \`${toolName}\` is a registered tool but is not granted to this agent; use one of the available tools above, or spawn an agent that provides that capability.` } @@ -1790,6 +1805,7 @@ export async function executeToolCall( toolName, agentId: agentTemplate.id, availableTools, + input, }), }) return abortablePreviousToolCallFinished @@ -1802,6 +1818,7 @@ export async function executeToolCall( : 'Original tool call input' onResponseChunk({ type: 'error', + message: `${toolCall.error}\n\n${inputLabel}:\n${formattedInput}`, userMessage: `The model sent a malformed \`${toolName}\` tool call and is correcting it automatically. No action is needed.`, }) @@ -2834,6 +2851,7 @@ export async function executeCustomToolCall( toolName, agentId: agentTemplate.id, availableTools, + input, }), }) return abortablePreviousToolCallFinished @@ -2851,6 +2869,7 @@ export async function executeCustomToolCall( }) logger.debug( { toolCall, error: toolCall.error }, + `${toolName} error: ${toolCall.error}`, ) return abortablePreviousToolCallFinished diff --git a/packages/agent-runtime/src/util/base2-tool-tiers.ts b/packages/agent-runtime/src/util/base2-tool-tiers.ts index 3ff619f180..bc1047eb5a 100644 --- a/packages/agent-runtime/src/util/base2-tool-tiers.ts +++ b/packages/agent-runtime/src/util/base2-tool-tiers.ts @@ -12,7 +12,20 @@ * beyond the template's mode-appropriate ceiling. */ -/** Base2 CORE tool names — always available when progressive disclosure is on. */ +/** + * Base2 CORE tool names — always available when progressive disclosure is on. + * + * Semantically broader than the template's CORE surface: `ask_user` and + * `write_todos` are listed unconditionally here, but fast/plan-only + * progressive base2 never exposes them (they are mode-gated in the template's + * buildArray). `filterByUnlockedTiers` only *keeps* names already present in + * its input, and `base2` always passes the template's full surface as + * `templateAllows` to cap tier adds, so this never widens the surfaced set. + * The unconditional list is a deliberate ceiling: a runtime-side CORE-only + * path MUST still pass `templateAllows` (or otherwise apply the same mode + * gates), or it could expose `ask_user`/`write_todos` in a mode that forbids + * them. + */ export const BASE2_CORE_TOOL_NAMES: readonly string[] = [ 'spawn_agents', 'query_index', @@ -21,6 +34,7 @@ export const BASE2_CORE_TOOL_NAMES: readonly string[] = [ 'read_subtree', 'list_directory', 'glob', + 'code_search', 'ask_user', 'skill', 'suggest_followups', diff --git a/sdk/src/__tests__/check-job.test.ts b/sdk/src/__tests__/check-job.test.ts index 7779497b55..bed5b5e030 100644 --- a/sdk/src/__tests__/check-job.test.ts +++ b/sdk/src/__tests__/check-job.test.ts @@ -32,7 +32,8 @@ function outputText(result: any): string { return (result.events ?? []) .filter((event: any) => event?.payload?.type === 'output') .map((event: any) => event.payload.data) - .join('')} + .join('') +} let counter = 0 const tempFiles: string[] = [] @@ -511,37 +512,55 @@ describe('checkJob', () => { }) test('follow timeout kills a running job when kill_on_timeout is true', async () => { + // On non-Windows platforms terminateProcessTree terminates the running job + // via a process-group kill: process.kill(-pid, signal). Stub process.kill + // to observe that actual path deterministically (a real pid 1234 may not + // exist), and restore it so the test stays hermetic. + const originalKill = process.kill let killCalled = false + // Exposed fallback that would only run if the group kill errored. + let childKillCalled = false const job = makeJob({ child: { pid: 1234, kill: () => { - killCalled = true + childKillCalled = true return true }, } as unknown as BackgroundJob['child'], }) - const result = value( - await withElapsedFollowTimeout(() => - checkJob({ - jobId: job.jobId, - wait_for: 'never appears', - timeout_seconds: 1, - kill_on_timeout: true, - owner: TRUSTED_OWNER, - }), - ), - ) + try { + ;(process.kill as any) = (pid: number, signal?: NodeJS.Signals | number) => { + // Group kill uses a negated pid (the process-group leader). + killCalled = true + return true + } - expect(killCalled).toBe(true) - expect(result).toMatchObject({ - jobId: job.jobId, - state: 'stopped', - matched: false, - killed: true, - }) - expect(job.status).toBe('stopped') + const result = value( + await withElapsedFollowTimeout(() => + checkJob({ + jobId: job.jobId, + wait_for: 'never appears', + timeout_seconds: 1, + kill_on_timeout: true, + owner: TRUSTED_OWNER, + }), + ), + ) + + expect(killCalled).toBe(true) + expect(childKillCalled).toBe(false) + expect(result).toMatchObject({ + jobId: job.jobId, + state: 'stopped', + matched: false, + killed: true, + }) + expect(job.status).toBe('stopped') + } finally { + process.kill = originalKill + } }) test('follow timeout kill failure surfaces errorMessage through the output union', async () => {