From 58805caa0cca6e90c48c73ff1b5822dc984b96c5 Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Thu, 23 Jul 2026 15:17:05 +0300 Subject: [PATCH 01/11] Share audit-receipt helper and harden general-agent spawn guidance Extract containsStructuralAuditReceipt into @codebuff/common/util/audit-receipt and rewire both the general-agent handleSteps completion gate and spawn-agent-utils buildRuntimeAgentReceipt gate to the shared helper so the two receipt checks cannot drift; cover the helper with a unit test. - general-agent prompt now routes ripgrep-style search through code-searcher (code_search is a registered runtime tool but intentionally not granted to general-agent) and names the required params.searchQueries. - add a code-searcher searchQueries recovery hint in validateAgentInput so an empty spawn gets an actionable message instead of a bare schema error. - add general-agent audit-loop tests covering the receipt-present break and the retry-exhaustion break. - fix the base2 reviewer-spawn coverage-evidence wording from 'below' to 'above' to match where the list is rendered. All changes pass typecheck and the affected test suites. --- agents/__tests__/general-agent.test.ts | 96 +++++++++++++++++++ agents/base2/base2.ts | 4 +- agents/general-agent/general-agent.ts | 38 +------- .../src/util/__tests__/audit-receipt.test.ts | 76 +++++++++++++++ common/src/util/audit-receipt.ts | 37 +++++++ .../__tests__/tool-validation-error.test.ts | 27 ++++++ .../tools/handlers/tool/spawn-agent-utils.ts | 36 +------ 7 files changed, 246 insertions(+), 68 deletions(-) create mode 100644 common/src/util/__tests__/audit-receipt.test.ts create mode 100644 common/src/util/audit-receipt.ts diff --git a/agents/__tests__/general-agent.test.ts b/agents/__tests__/general-agent.test.ts index 955c0c11dd..98e2dd4072 100644 --- a/agents/__tests__/general-agent.test.ts +++ b/agents/__tests__/general-agent.test.ts @@ -34,6 +34,19 @@ 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". + const agent = createGeneralAgent({ model: 'opus' }) + + expect(agent.toolNames).not.toContain('code_search') + expect(agent.instructionsPrompt).toContain('code_search') + expect(agent.instructionsPrompt).toContain('not granted to you') + expect(agent.instructionsPrompt).toContain('params.searchQueries') + }) + test('binds durable audit shards to composable snapshot receipts', () => { const agent = createGeneralAgent({ model: 'opus' }) const params = agent.inputSchema?.params?.properties @@ -71,4 +84,87 @@ describe('general-agent programmatic tools', () => { 'Audit completion was rejected', ) }) + + test('breaks the audit loop once a matching structural receipt is present', () => { + const agent = createGeneralAgent({ model: 'opus' }) + const generator = agent.handleSteps!({ + prompt: 'Audit service completeness', + params: { + sessionSlug: 'readiness', + shardId: 'services', + snapshotId: 'snapshot-1', + }, + } as any) + + expect(generator.next().value).toMatchObject({ + toolName: 'spawn_agent_inline', + }) + expect(generator.next({ toolResult: [] } as any).value).toBe('STEP') + + const completion = generator.next({ + stepsComplete: true, + agentState: { + messageHistory: [ + { + role: 'tool', + content: [ + { + type: 'json', + value: { structuralReceipt: { snapshot_id: 'snapshot-1' } }, + }, + ], + }, + ], + }, + toolResult: [], + } as any) + + expect(completion.done).toBe(true) + expect((completion.value as any)?.toolName).not.toBe('add_message') + }) + + test('breaks the audit loop after exhausting completion retries', () => { + const agent = createGeneralAgent({ model: 'opus' }) + const generator = agent.handleSteps!({ + prompt: 'Audit service completeness', + params: { + sessionSlug: 'readiness', + shardId: 'services', + snapshotId: 'snapshot-1', + }, + } as any) + + const noReceiptStep = { + stepsComplete: true, + agentState: { messageHistory: [] }, + toolResult: [], + } as any + + // First completion step: rejected -> add_message (retries 0 -> 1). + expect(generator.next().value).toMatchObject({ + toolName: 'spawn_agent_inline', + }) + expect(generator.next({ toolResult: [] } as any).value).toBe('STEP') + expect(generator.next(noReceiptStep).value).toMatchObject({ + toolName: 'add_message', + }) + + // Second completion step: rejected -> add_message (retries 1 -> 2). + expect(generator.next().value).toMatchObject({ + toolName: 'spawn_agent_inline', + }) + expect(generator.next({ toolResult: [] } as any).value).toBe('STEP') + expect(generator.next(noReceiptStep).value).toMatchObject({ + toolName: 'add_message', + }) + + // Third completion step: retries exhausted -> break without add_message. + expect(generator.next().value).toMatchObject({ + toolName: 'spawn_agent_inline', + }) + expect(generator.next({ toolResult: [] } as any).value).toBe('STEP') + const final = generator.next(noReceiptStep) + expect(final.done).toBe(true) + expect((final.value as any)?.toolName).not.toBe('add_message') + }) }) diff --git a/agents/base2/base2.ts b/agents/base2/base2.ts index 07c83a236b..755d35bbca 100644 --- a/agents/base2/base2.ts +++ b/agents/base2/base2.ts @@ -2201,7 +2201,7 @@ ${specialistRoutingSection} coverageEvidenceDetails, 'Validation gate summary: Reviewer running concurrently with validation (static-review-only mode).', '', - 'Return the required structured review object. Echo snapshotFingerprint exactly, list every reviewed file, evaluate all review dimensions, and map every user requirement to evidence. Co-changed test files are listed under "Coverage evidence" below; they are readable and citable even though they are not part of the reviewed fingerprint. Test-coverage requirements are satisfied (not uncertain) when a Coverage evidence test file covers the changed behavior — cite that test file and the covering test name(s) as the requirement evidence. Use coverage: missing only when no covering test exists in the Coverage evidence list or anywhere in the repo, and requirement status uncertain only when you could not inspect a listed Coverage evidence test file at all.', + 'Return the required structured review object. Echo snapshotFingerprint exactly, list every reviewed file, evaluate all review dimensions, and map every user requirement to evidence. Co-changed test files are listed under "Coverage evidence" above; they are readable and citable even though they are not part of the reviewed fingerprint. Test-coverage requirements are satisfied (not uncertain) when a Coverage evidence test file covers the changed behavior — cite that test file and the covering test name(s) as the requirement evidence. Use coverage: missing only when no covering test exists in the Coverage evidence list or anywhere in the repo, and requirement status uncertain only when you could not inspect a listed Coverage evidence test file at all.', ].join('\n'), }, ], @@ -2920,7 +2920,7 @@ ${specialistRoutingSection} coverageEvidenceDetails, `Validation gate summary: ${validationSummary}`, '', - 'Return the required structured review object. Echo snapshotFingerprint exactly, list every reviewed file, evaluate all review dimensions, and map every user requirement to evidence. Co-changed test files are listed under "Coverage evidence" below; they are readable and citable even though they are not part of the reviewed fingerprint. Test-coverage requirements are satisfied (not uncertain) when a Coverage evidence test file covers the changed behavior — cite that test file and the covering test name(s) as the requirement evidence. Use coverage: missing only when no covering test exists in the Coverage evidence list or anywhere in the repo, and requirement status uncertain only when you could not inspect a listed Coverage evidence test file at all.', + 'Return the required structured review object. Echo snapshotFingerprint exactly, list every reviewed file, evaluate all review dimensions, and map every user requirement to evidence. Co-changed test files are listed under "Coverage evidence" above; they are readable and citable even though they are not part of the reviewed fingerprint. Test-coverage requirements are satisfied (not uncertain) when a Coverage evidence test file covers the changed behavior — cite that test file and the covering test name(s) as the requirement evidence. Use coverage: missing only when no covering test exists in the Coverage evidence list or anywhere in the repo, and requirement status uncertain only when you could not inspect a listed Coverage evidence test file at all.', ].join('\n'), }, ], diff --git a/agents/general-agent/general-agent.ts b/agents/general-agent/general-agent.ts index 802ffffbde..0f1177bb5f 100644 --- a/agents/general-agent/general-agent.ts +++ b/agents/general-agent/general-agent.ts @@ -1,4 +1,5 @@ import { buildArray } from '@codebuff/common/util/array' +import { containsStructuralAuditReceipt } from '@codebuff/common/util/audit-receipt' import { publisher } from '../constants' @@ -93,6 +94,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.`, `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. 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'), @@ -178,7 +180,7 @@ export const createGeneralAgent = (options: { const auditRequested = Boolean(sessionSlug && shardId) if ( auditRequested && - !hasSuccessfulAuditReceipt( + !containsStructuralAuditReceipt( stepResult.agentState?.messageHistory, expectedSnapshotId, ) && @@ -199,40 +201,6 @@ export const createGeneralAgent = (options: { break } - function hasSuccessfulAuditReceipt( - value: unknown, - expectedSnapshotId: string, - ): boolean { - let found = false - const visit = (item: unknown, depth = 0): void => { - if (found || !item || depth > 12) return - if (Array.isArray(item)) { - for (const nested of item) visit(nested, depth + 1) - return - } - if (typeof item !== 'object') return - const record = item as Record - const receipt = record.structuralReceipt - if ( - receipt && - typeof receipt === 'object' && - !Array.isArray(receipt) - ) { - const snapshotId = (receipt as Record).snapshot_id - if ( - typeof snapshotId === 'string' && - (!expectedSnapshotId || snapshotId === expectedSnapshotId) - ) { - found = true - return - } - } - for (const nested of Object.values(record)) visit(nested, depth + 1) - } - visit(value) - return found - } - function shouldProactivelyQueryIndex(value: unknown): value is string { if (typeof value !== 'string') return false const text = value.trim() diff --git a/common/src/util/__tests__/audit-receipt.test.ts b/common/src/util/__tests__/audit-receipt.test.ts new file mode 100644 index 0000000000..d27a0ab5f0 --- /dev/null +++ b/common/src/util/__tests__/audit-receipt.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it } from 'bun:test' + +import { containsStructuralAuditReceipt } from '../audit-receipt' + +describe('containsStructuralAuditReceipt', () => { + it('returns true when a nested structuralReceipt.snapshot_id matches the expected id', () => { + const value = { + messageHistory: [ + { role: 'user', content: 'noise' }, + { + role: 'tool', + content: [ + { type: 'text', text: 'more noise' }, + { + type: 'json', + value: { + structuralReceipt: { snapshot_id: 'snapshot-1' }, + }, + }, + ], + }, + ], + } + + expect(containsStructuralAuditReceipt(value, 'snapshot-1')).toBe(true) + }) + + it('returns true for any structuralReceipt with a string snapshot_id when expected id is undefined or empty', () => { + const value = [ + { structuralReceipt: { snapshot_id: 'whatever' } }, + ] + + expect(containsStructuralAuditReceipt(value)).toBe(true) + expect(containsStructuralAuditReceipt(value, '')).toBe(true) + }) + + it('returns false when the expected id does not match any present receipt', () => { + const value = { + nested: { + deep: { structuralReceipt: { snapshot_id: 'snapshot-1' } }, + }, + } + + expect(containsStructuralAuditReceipt(value, 'snapshot-2')).toBe(false) + }) + + it('returns false when there is no structuralReceipt anywhere', () => { + expect( + containsStructuralAuditReceipt({ foo: { bar: [{ baz: 1 }] } }, 'snapshot-1'), + ).toBe(false) + }) + + it('returns false for non-object inputs', () => { + expect(containsStructuralAuditReceipt(null, 'snapshot-1')).toBe(false) + expect(containsStructuralAuditReceipt('snapshot-1', 'snapshot-1')).toBe(false) + expect(containsStructuralAuditReceipt(42, 'snapshot-1')).toBe(false) + expect(containsStructuralAuditReceipt(undefined, 'snapshot-1')).toBe(false) + }) + + it('returns false when structuralReceipt exists but snapshot_id is missing or non-string', () => { + expect( + containsStructuralAuditReceipt({ structuralReceipt: {} }), + ).toBe(false) + expect( + containsStructuralAuditReceipt({ + structuralReceipt: { snapshot_id: 123 }, + }), + ).toBe(false) + expect( + containsStructuralAuditReceipt( + { structuralReceipt: { snapshot_id: 123 } }, + 'snapshot-1', + ), + ).toBe(false) + }) +}) diff --git a/common/src/util/audit-receipt.ts b/common/src/util/audit-receipt.ts new file mode 100644 index 0000000000..67eaeab76b --- /dev/null +++ b/common/src/util/audit-receipt.ts @@ -0,0 +1,37 @@ +/** + * Recursively searches an arbitrary value for a nested `structuralReceipt` + * object whose `snapshot_id` matches `expectedSnapshotId`. When + * `expectedSnapshotId` is empty/undefined, any structuralReceipt with a string + * `snapshot_id` counts as a match. Depth-bounded (<=12) to stay safe on deeply + * nested or cyclic-looking inputs. Shared by the general-agent handleSteps gate + * and the runtime buildRuntimeAgentReceipt gate so the two do not drift. + */ +export function containsStructuralAuditReceipt( + value: unknown, + expectedSnapshotId?: string, +): boolean { + let found = false + const visit = (item: unknown, depth = 0): void => { + if (found || !item || depth > 12) return + if (Array.isArray(item)) { + for (const nested of item) visit(nested, depth + 1) + return + } + if (typeof item !== 'object') return + const record = item as Record + const receipt = record.structuralReceipt + if (receipt && typeof receipt === 'object' && !Array.isArray(receipt)) { + const snapshotId = (receipt as Record).snapshot_id + if ( + typeof snapshotId === 'string' && + (!expectedSnapshotId || snapshotId === expectedSnapshotId) + ) { + found = true + return + } + } + for (const nested of Object.values(record)) visit(nested, depth + 1) + } + visit(value) + return found +} 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 cd707350d9..3bd3688893 100644 --- a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts +++ b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts @@ -1735,6 +1735,33 @@ describe('tool validation error handling', () => { expect(message).toContain('Preserve params field names exactly.') }) + it('gives code-searcher a searchQueries recovery hint on empty params', async () => { + const { validateAgentInput } = + await import('../tools/handlers/tool/spawn-agent-utils') + const codeSearcher = { + ...testAgentTemplate, + id: 'code-searcher', + inputSchema: { + params: z.object({ + searchQueries: z.array(z.object({ pattern: z.string() })), + }), + }, + } + + let message = '' + try { + validateAgentInput(codeSearcher, 'code-searcher', undefined, {}) + } catch (error) { + message = error instanceof Error ? error.message : String(error) + } + + expect(message).toContain('Missing required: searchQueries') + expect(message).toContain('spawn code-searcher with') + expect(message).toContain('"searchQueries"') + expect(message).toContain('required array of objects') + expect(message).toContain('Preserve params field names exactly.') + }) + it('publishes a structured failure result when Basher is missing command', async () => { const parent: AgentTemplate = { ...testAgentTemplate, diff --git a/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-utils.ts b/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-utils.ts index 2ca4f82fd9..7966d3208e 100644 --- a/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-utils.ts +++ b/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-utils.ts @@ -9,6 +9,7 @@ import { } from '@codebuff/common/util/agent-id-parsing' import { withTimeout } from '@codebuff/common/util/promise' import { generateCompactId } from '@codebuff/common/util/string' +import { containsStructuralAuditReceipt } from '@codebuff/common/util/audit-receipt' import { agentHandoffSchema, agentReceiptSchema, @@ -1122,36 +1123,6 @@ function containsToolCall(value: unknown, toolName: string): boolean { return found } -function containsStructuralAuditReceipt( - value: unknown, - expectedSnapshotId?: string, -): boolean { - let found = false - const visit = (item: unknown, depth = 0): void => { - if (found || !item || depth > 12) return - if (Array.isArray(item)) { - for (const nested of item) visit(nested, depth + 1) - return - } - if (typeof item !== 'object') return - const record = item as Record - const receipt = record.structuralReceipt - if (receipt && typeof receipt === 'object' && !Array.isArray(receipt)) { - const snapshotId = (receipt as Record).snapshot_id - if ( - typeof snapshotId === 'string' && - (!expectedSnapshotId || snapshotId === expectedSnapshotId) - ) { - found = true - return - } - } - for (const nested of Object.values(record)) visit(nested, depth + 1) - } - visit(value) - return found -} - function extractReceiptEvidence(params: { output: unknown agentType: string @@ -1665,7 +1636,10 @@ export function validateAgentInput( (issuePaths.has('snapshot_fingerprint') || Object.hasOwn(paramsRecord ?? {}, 'snapshot_id')) ? '\n\nRecovery: replace params.snapshot_id with params.snapshot_fingerprint, or add params.snapshot_fingerprint when it is missing. Retain params.changed_files and preserve both canonical field names exactly.' - : '' + : normalizedAgentType === 'code-searcher' && + issuePaths.has('searchQueries') + ? '\n\nRecovery: spawn code-searcher with { "agent_type": "code-searcher", "params": { "searchQueries": [{ "pattern": "", "flags": "-g *.ts" }] } }. searchQueries is a required array of objects each with a non-empty string "pattern"; queries mentioned only in prompt prose are never executed.' + : '' const paramsContract = formatAgentParamsContract(inputSchema.params) throw new Error( `Invalid params for agent ${agentType}: ${formatValidationIssues({ issues: result.error.issues })}\n\nExact params contract (from the child agent schema): ${paramsContract}\nPreserve params field names exactly.${recoveryHint}\n\nOriginal params value:\n${formatValueForError(params ?? {})}`, From b3965c6f012828b52e127239daa22340d2527f25 Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Thu, 23 Jul 2026 19:20:38 +0300 Subject: [PATCH 02/11] Harden reviewer gate and edit capability recovery Tighten the reviewer gate and edit-application paths so authorization and review coverage cannot be silently bypassed. Skip the validation/reviewer gate in PLAN-only mode, where no source edits exist to attest to. Make co-changed tests first-class reviewable files so reviewer attestation covers them instead of always reporting coverage as missing/uncertain; keep them separately identifiable as coverage evidence. Recapture the review snapshot after validation (validation-first), since hooks can mutate generated or formatted output before review. Preserve valid read authorization on deterministic edit_transaction and str_replace preflight failures, and only require a fresh whole-transaction re-read when a capability is genuinely stale or out of scope, so a real freshness failure cannot be sidestepped by refreshing one path while replaying stale tokens for the rest. Scope the edit-application coordinator's positive-evidence confirmation to paths that actually produced a client change, so a no-op path no longer forces a false rejection. Block replace_range in a transaction once a prior non-range edit has shifted the file's line count, preventing it from splicing the wrong lines while its original-snapshot hash check still passes. Broaden spawn_agents validation diagnostics with canonical recovery hints for the reviewer family, dependency-manager, librarian, and get_build_targets, and harden audit-receipt traversal against deep/cyclic structures. Adds regression tests covering each behavior. --- agents/__tests__/base2.test.ts | 370 +----------------- agents/__tests__/gate-paths-parity.test.ts | 39 +- agents/__tests__/gate-paths.test.ts | 22 +- agents/base2/base2.ts | 237 +++-------- agents/base2/gate-paths.ts | 22 +- .../e2e/reviewer-spawn-conditions.e2e.test.ts | 60 +++ .../src/util/__tests__/audit-receipt.test.ts | 19 + common/src/util/audit-receipt.ts | 22 +- .../process-edit-transaction.test.ts | 144 ++++++- .../src/__tests__/process-str-replace.test.ts | 4 + .../__tests__/read-files-edit-state.test.ts | 109 ++++-- .../__tests__/tool-validation-error.test.ts | 135 +++++++ .../src/process-edit-transaction.ts | 61 +-- .../agent-runtime/src/process-str-replace.ts | 21 +- .../edit-application-coordinator.test.ts | 67 ++++ .../tool/edit-application-coordinator.ts | 71 +++- .../tools/handlers/tool/edit-transaction.ts | 57 ++- .../tools/handlers/tool/spawn-agent-utils.ts | 29 +- .../src/tools/handlers/tool/str-replace.ts | 19 +- .../src/tools/handlers/tool/write-file.ts | 2 +- .../agent-runtime/src/tools/tool-executor.ts | 18 +- 21 files changed, 835 insertions(+), 693 deletions(-) diff --git a/agents/__tests__/base2.test.ts b/agents/__tests__/base2.test.ts index 813f74fc7d..8e384d5879 100644 --- a/agents/__tests__/base2.test.ts +++ b/agents/__tests__/base2.test.ts @@ -3820,22 +3820,13 @@ describe('base2 verification and reviewer gates', () => { } as any).value as any expect(reviewCall).toMatchObject({ toolName: 'spawn_agents' }) const reviewPrompt = reviewCall.input.agents[0].prompt as string - // Co-changed test files are surfaced as readable "Coverage evidence" so - // the reviewer can confirm coverage; without this the reviewer could never - // see the changed test file and always reported coverage missing/uncertain. expect(reviewPrompt).toContain( - 'Coverage evidence (co-changed tests; readable and citable, not part of the reviewed fingerprint):', + 'list every pending changed file in reviewedFiles (including tests)', ) expect(reviewPrompt).toContain( - 'Co-changed test files are listed under "Coverage evidence"', - ) - expect(reviewPrompt).toContain('satisfied (not uncertain)') - expect(reviewPrompt).toContain( - 'when a Coverage evidence test file covers the changed behavior', - ) - expect(reviewPrompt).toContain( - 'Use coverage: missing only when no covering test exists in the Coverage evidence list or anywhere in the repo', + 'Changed tests are first-class review targets and may also be cited as coverage evidence.', ) + expect(reviewPrompt).not.toContain('not part of the reviewed fingerprint') }) test('reviewer attestation citing the changed test file clears the gate test-coverage requirement', () => { @@ -4295,40 +4286,8 @@ describe('base2 verification and reviewer gates', () => { }) }) -describe('base2 static-review-only concurrency (M3.1)', () => { - test('default path (staticReviewOnly absent) does not spawn the reviewer in the background', () => { - const base2 = createBase2('default') - const agentState = { agentId: 'base2-custom' } - const gen = base2.handleSteps!({ - agentState, - prompt: 'Make the requested change now please', - params: {}, - } as any) - - expect(gen.next().value).toMatchObject({ toolName: 'git_status' }) - expect( - gen.next({ toolResult: [{ type: 'json', value: { status: '' } }] } as any) - .value, - ).toMatchObject({ toolName: 'spawn_agent_inline' }) - expect(gen.next().value).toBe('STEP') - expect( - gen.next({ - stepsComplete: true, - toolResult: [{ type: 'json', value: { file: 'src/a.ts' } }], - } as any).value, - ).toMatchObject({ toolName: 'git_status' }) - const afterGit = gen.next({ - toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], - } as any) - // No background reviewer: validation runs first (foreground), exactly as - // the existing sequential behavior. - expect(afterGit.value).toMatchObject({ toolName: 'run_file_change_hooks' }) - expect( - (agentState as any).base2ActiveWork.staticReviewerJobId, - ).toBeUndefined() - }) - - test('staticReviewOnly spawns the reviewer in the background before validation and stashes jobId', () => { +describe('base2 validation-first reviewer snapshots', () => { + test('staticReviewOnly still validates before spawning the final reviewer', () => { const base2 = createBase2('default') const agentState = { agentId: 'base2-custom', @@ -4352,323 +4311,20 @@ describe('base2 static-review-only concurrency (M3.1)', () => { toolResult: [{ type: 'json', value: { file: 'src/a.ts' } }], } as any).value, ).toMatchObject({ toolName: 'git_status' }) - const bgSpawn = gen.next({ + const validation = gen.next({ toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], } as any) - // Reviewer spawned in the background BEFORE validation hooks run. - expect(bgSpawn.value).toMatchObject({ toolName: 'spawn_agents' }) - const agents = (bgSpawn.value as any).input.agents as any[] - expect(agents[0].agent_type).toBe('code-reviewer') - expect(agents[0].background).toBe(true) - expect(agents[0].prompt as string).toContain( - 'Reviewer running concurrently with validation (static-review-only mode).', - ) - - const afterReport = gen.next({ - toolResult: [ - { - type: 'json', - value: { - background: true, - jobId: 'bg-reviewer-1', - message: 'Reviewer running in the background.', - }, - }, - ], - } as any) - expect((afterReport.value as any).toolName).toBe('run_file_change_hooks') - expect((agentState as any).base2ActiveWork.staticReviewerJobId).toBe( - 'bg-reviewer-1', - ) - }) - - test('staticReviewOnly: validation failure does not consult the background reviewer', () => { - const base2 = createBase2('default') - const agentState = { - agentId: 'base2-custom', - base2ActiveWork: { staticReviewOnly: true }, - } - const gen = base2.handleSteps!({ - agentState, - prompt: 'Make the requested change now please', - params: {}, - } as any) + expect(validation.value).toMatchObject({ toolName: 'run_file_change_hooks' }) + expect((agentState as any).base2ActiveWork.staticReviewerJobId).toBeUndefined() - expect(gen.next().value).toMatchObject({ toolName: 'git_status' }) - expect( - gen.next({ toolResult: [{ type: 'json', value: { status: '' } }] } as any) - .value, - ).toMatchObject({ toolName: 'spawn_agent_inline' }) - expect(gen.next().value).toBe('STEP') - expect( - gen.next({ - stepsComplete: true, - toolResult: [{ type: 'json', value: { file: 'src/a.ts' } }], - } as any).value, - ).toMatchObject({ toolName: 'git_status' }) - const bgSpawn = gen.next({ - toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], - } as any) - expect(bgSpawn.value).toMatchObject({ toolName: 'spawn_agents' }) - - const afterReport = gen.next({ - toolResult: [ - { - type: 'json', - value: { background: true, jobId: 'bg-reviewer-2', message: 'ok' }, - }, - ], - } as any) - expect((afterReport.value as any).toolName).toBe('run_file_change_hooks') - expect((agentState as any).base2ActiveWork.staticReviewerJobId).toBe( - 'bg-reviewer-2', - ) - - // Validation fails (unparseable failure -> blocked, no repair loop). - const afterHooks = gen.next({ - toolResult: [ - { - type: 'json', - value: [{ hookName: 'typecheck', exitCode: 1, stderr: 'boom' }], - }, - ], - } as any) - // Validation failure cancels speculative review before surfacing the - // blocking validation diagnostics. - expect((afterHooks.value as any).toolName).toBe('check_background_agent') - expect((afterHooks.value as any).input).toMatchObject({ - jobId: 'bg-reviewer-2', - cancel: true, - }) - const blocked = gen.next({ toolResult: [] } as any) - expect((blocked.value as any).toolName).toBe('add_message') - const text = (blocked.value as any).input.content as string - expect(text).toContain('Verification gate') - expect( - (agentState as any).base2ActiveWork.staticReviewerJobId, - ).toBeUndefined() - }) - - test('staticReviewOnly: validation pass joins the background reviewer via check_background_agent', () => { - const base2 = createBase2('default') - const agentState = { - agentId: 'base2-custom', - base2ActiveWork: { staticReviewOnly: true }, - } - const gen = base2.handleSteps!({ - agentState, - prompt: 'Make the requested change now please', - params: {}, - } as any) - - expect(gen.next().value).toMatchObject({ toolName: 'git_status' }) - expect( - gen.next({ toolResult: [{ type: 'json', value: { status: '' } }] } as any) - .value, - ).toMatchObject({ toolName: 'spawn_agent_inline' }) - expect(gen.next().value).toBe('STEP') - expect( - gen.next({ - stepsComplete: true, - toolResult: [{ type: 'json', value: { file: 'src/a.ts' } }], - } as any).value, - ).toMatchObject({ toolName: 'git_status' }) - const bgSpawn = gen.next({ - toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], - } as any) - expect(bgSpawn.value).toMatchObject({ toolName: 'spawn_agents' }) - expect((bgSpawn.value as any).input.agents[0].background).toBe(true) - - const afterReport = gen.next({ - toolResult: [ - { - type: 'json', - value: { background: true, jobId: 'bg-reviewer-3', message: 'ok' }, - }, - ], - } as any) - expect((afterReport.value as any).toolName).toBe('run_file_change_hooks') - expect((agentState as any).base2ActiveWork.staticReviewerJobId).toBe( - 'bg-reviewer-3', - ) - - // Validation passes (hooks skipped). - const afterHooks = gen.next({ - toolResult: [ - { - type: 'json', - value: [ - { - validationStatus: 'hooks_skipped', - message: - 'Configured file-change hooks were skipped because none matched the changed files.', - configuredHookCount: 1, - changedFiles: ['src/a.ts'], - }, - ], - }, - ], - } as any) - // Join the background reviewer instead of a foreground spawn_agents, using - // the stashed jobId. - expect(afterHooks.value).toMatchObject({ - toolName: 'check_background_agent', - }) - expect((afterHooks.value as any).input).toMatchObject({ - jobId: 'bg-reviewer-3', - timeout_seconds: 120, - }) - expect((afterHooks.value as any).input).not.toHaveProperty('wait_for') - - // The reviewer returns LOOKS_GOOD via the background result; feed it through - // the existing collectReviewerBlockers / getReviewerFinalizationVerdict - // path (unchanged) to reach finalization. - const gatePassed = gen.next( - attestedBackgroundReviewerResult(agentState, 'LOOKS_GOOD') as any, - ) - expect(gatePassed.value).toMatchObject({ - toolName: 'add_message', - input: { role: 'user' }, - }) - expect((gatePassed.value as any).input.content.toLowerCase()).toContain( - 'reviewer gate passed with looks_good', - ) - expect((agentState as any).base2ActiveWork).toMatchObject({ - currentPhase: 'final_response_allowed', - pendingGateFiles: [], - gatePassedReviewerVerdict: 'LOOKS_GOOD', - staticReviewerJobId: undefined, - }) - }) - - test('staticReviewOnly: background reviewer NON_BLOCKING result finalizes after validation pass', () => { - const base2 = createBase2('default') - const agentState = { - agentId: 'base2-custom', - base2ActiveWork: { staticReviewOnly: true }, - } - const gen = base2.handleSteps!({ - agentState, - prompt: 'Make the requested change now please', - params: {}, - } as any) - - expect(gen.next().value).toMatchObject({ toolName: 'git_status' }) - expect( - gen.next({ toolResult: [{ type: 'json', value: { status: '' } }] } as any) - .value, - ).toMatchObject({ toolName: 'spawn_agent_inline' }) - expect(gen.next().value).toBe('STEP') - expect( - gen.next({ - stepsComplete: true, - toolResult: [{ type: 'json', value: { file: 'src/a.ts' } }], - } as any).value, - ).toMatchObject({ toolName: 'git_status' }) - expect( - gen.next({ - toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], - } as any).value, - ).toMatchObject({ toolName: 'spawn_agents' }) - expect( - gen.next({ - toolResult: [ - { type: 'json', value: { background: true, jobId: 'bg-reviewer-4' } }, - ], - } as any).value, - ).toMatchObject({ toolName: 'run_file_change_hooks' }) - const afterHooks = gen.next({ - toolResult: [{ type: 'json', value: [] }], - } as any) - - expect(afterHooks.value).toMatchObject({ - toolName: 'check_background_agent', - input: { jobId: 'bg-reviewer-4', timeout_seconds: 120 }, - }) - expect((afterHooks.value as any).input).not.toHaveProperty('wait_for') - - const gatePassed = gen.next( - attestedBackgroundReviewerResult(agentState, 'NON_BLOCKING', [ - 'Minor suggestion.', - ]) as any, - ) - expect(gatePassed.value).toMatchObject({ toolName: 'add_message' }) - expect((gatePassed.value as any).input.content).toContain( - 'Reviewer gate passed with NON_BLOCKING', - ) - expect((agentState as any).base2ActiveWork).toMatchObject({ - currentPhase: 'final_response_allowed', - pendingGateFiles: [], - gatePassedReviewerVerdict: 'NON_BLOCKING', - staticReviewerJobId: undefined, - }) - }) - - test('staticReviewOnly: background reviewer BLOCKING result reopens the turn after validation pass', () => { - const base2 = createBase2('default') - const agentState = { - agentId: 'base2-custom', - base2ActiveWork: { staticReviewOnly: true }, - } - const gen = base2.handleSteps!({ - agentState, - prompt: 'Make the requested change now please', - params: {}, - } as any) - - expect(gen.next().value).toMatchObject({ toolName: 'git_status' }) - expect( - gen.next({ toolResult: [{ type: 'json', value: { status: '' } }] } as any) - .value, - ).toMatchObject({ toolName: 'spawn_agent_inline' }) - expect(gen.next().value).toBe('STEP') - expect( - gen.next({ - stepsComplete: true, - toolResult: [{ type: 'json', value: { file: 'src/a.ts' } }], - } as any).value, - ).toMatchObject({ toolName: 'git_status' }) - expect( - gen.next({ - toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], - } as any).value, - ).toMatchObject({ toolName: 'spawn_agents' }) - expect( - gen.next({ - toolResult: [ - { type: 'json', value: { background: true, jobId: 'bg-reviewer-5' } }, - ], - } as any).value, - ).toMatchObject({ toolName: 'run_file_change_hooks' }) - const afterHooks = gen.next({ + const review = gen.next({ toolResult: [{ type: 'json', value: [] }], } as any) - - expect(afterHooks.value).toMatchObject({ - toolName: 'check_background_agent', - input: { jobId: 'bg-reviewer-5', timeout_seconds: 120 }, - }) - expect((afterHooks.value as any).input).not.toHaveProperty('wait_for') - - const afterReview = gen.next( - attestedBackgroundReviewerResult(agentState, 'BLOCKING', [ - 'Fix the static path.', - ]) as any, - ) - expect(afterReview.value).toMatchObject({ - toolName: 'add_message', - input: { role: 'user' }, - }) - expect((afterReview.value as any).input.content).toContain( - 'BLOCKING: Fix the static path.', - ) - expect((agentState as any).base2ActiveWork).toMatchObject({ - currentPhase: 'blocked', - pendingGateFiles: ['src/a.ts'], - openReviewerBlockers: ['BLOCKING: Fix the static path.'], - nextRequiredAction: - 'Resolve the reviewer feedback below before any unrelated work, final response, or another review.', + expect(review.value).toMatchObject({ + toolName: 'spawn_agents', + input: { agents: [{ agent_type: 'code-reviewer' }] }, }) + expect((review.value as any).input.agents[0]).not.toHaveProperty('background') }) }) diff --git a/agents/__tests__/gate-paths-parity.test.ts b/agents/__tests__/gate-paths-parity.test.ts index 5cba25b868..44ca05c6c6 100644 --- a/agents/__tests__/gate-paths-parity.test.ts +++ b/agents/__tests__/gate-paths-parity.test.ts @@ -188,13 +188,11 @@ describe('gate-path helpers — inline copies match canonical exports', () => { const pathInputs: string[] = [ // reviewable source (expect true) 'src/foo.ts', - // __tests__/ path (false) + // Tests are first-class reviewable files. 'src/__tests__/foo.ts', - // .test.ts path (false) 'src/foo.test.ts', - // JS-flavored test/spec files (false) — parity must hold across the - // broadened (?:tsx?|jsx?|mjs|cjs) exclusion so a `.test.js`/`.spec.mjs` - // test file is not classified as reviewable source by the inline copy. + // JS-flavored test/spec files are reviewable and remain separately + // identifiable as coverage evidence. 'src/foo.test.js', 'src/foo.spec.mjs', 'src/foo.test.cjs', @@ -234,7 +232,7 @@ describe('gate-path helpers — inline copies match canonical exports', () => { // selectReviewableGateFiles normalizes + filters + dedupes internally, so // pass raw path lists. const listInputs: string[][] = [ - // mixed source + bookkeeping: only source survives, normalized + deduped + // mixed source + tests + bookkeeping: source and tests survive [ 'src/foo.ts', './src/foo.ts', @@ -367,12 +365,12 @@ describe('gate-path helpers — canonical export behavior', () => { expect(normalizeGateFilePath(' ')).toBe('') }) - test('selectReviewableGateFiles keeps reviewable source and drops tests/generated/docs/data/bookkeeping', () => { + test('selectReviewableGateFiles keeps source and tests and drops generated/docs/data/bookkeeping', () => { const selected = selectReviewableGateFiles([ // reviewable source (kept): .ts and .py 'src/foo.ts', 'scripts/tool.py', - // __tests__/ and .test/.spec (dropped) + // __tests__/ and .test/.spec (kept) 'src/__tests__/foo.ts', 'src/foo.test.ts', 'src/foo.spec.ts', @@ -392,7 +390,13 @@ describe('gate-path helpers — canonical export behavior', () => { 'evals/case.ts', '.agents/sessions/slug/STATE.json', ]) - expect(selected).toEqual(['src/foo.ts', 'scripts/tool.py']) + expect(selected).toEqual([ + 'src/foo.ts', + 'scripts/tool.py', + 'src/__tests__/foo.ts', + 'src/foo.test.ts', + 'src/foo.spec.ts', + ]) // empty list -> [] expect(selectReviewableGateFiles([])).toEqual([]) }) @@ -421,7 +425,11 @@ describe('gate-path helpers — canonical export behavior', () => { 'src/__tests__/feature.ts', ] - expect(selectReviewableGateFiles(inputs)).toEqual(['src/feature.ts']) + expect(selectReviewableGateFiles(inputs)).toEqual([ + 'src/feature.ts', + 'src/feature.test.ts', + 'src/__tests__/feature.ts', + ]) expect(selectCoverageEvidenceFiles(inputs)).toEqual([ 'src/feature.test.ts', 'src/__tests__/feature.ts', @@ -443,12 +451,9 @@ describe('gate-path helpers — canonical export behavior', () => { ).toEqual(['src/feature.test.ts', 'src/__tests__/feature.ts']) }) - test('JS-flavored test files are excluded from reviewable source and treated as coverage evidence', () => { - // RF-3/RF-9: widen the test/spec exclusion so JS-flavored test files - // (foo.test.mjs/.cjs/.jsx, foo.spec.*) are treated as tests rather than - // reviewable source. Without this, the reviewable include regex (which - // already accepts mjs|cjs|jsx as source extensions) would classify a JS - // test file as reviewable source. + test('JS-flavored test files are reviewable and treated as coverage evidence', () => { + // Tests participate in reviewer attestation while remaining identifiable + // as coverage evidence. for (const testFile of [ 'src/foo.test.mjs', 'src/foo.test.cjs', @@ -457,7 +462,7 @@ describe('gate-path helpers — canonical export behavior', () => { 'src/foo.spec.cjs', 'src/foo.spec.jsx', ]) { - expect(isReviewableGateFile(testFile)).toBe(false) + expect(isReviewableGateFile(testFile)).toBe(true) expect(isCoverageEvidenceFile(testFile)).toBe(true) } // Non-test JS-flavored source stays reviewable and is not coverage evidence. diff --git a/agents/__tests__/gate-paths.test.ts b/agents/__tests__/gate-paths.test.ts index 03b4857471..745c768ed8 100644 --- a/agents/__tests__/gate-paths.test.ts +++ b/agents/__tests__/gate-paths.test.ts @@ -134,7 +134,7 @@ describe('gate-paths helpers', () => { expect(gateFileSetsEqual(['a', 'b'], ['a', 'c'])).toBe(false) }) - test('isReviewableGateFile includes real source and excludes tests/generated/docs/data/bookkeeping', () => { + test('isReviewableGateFile includes source and tests while excluding generated/docs/data/bookkeeping', () => { // Reviewable source extensions (expect true). for (const reviewable of [ 'src/a.ts', @@ -148,19 +148,19 @@ describe('gate-paths helpers', () => { 'cmd/main.go', 'svc/App.java', 'app/Main.kt', - ]) { - expect(isReviewableGateFile(reviewable)).toBe(true) - } - - // Tests, generated code, docs, data/config, env, and bookkeeping dirs - // (expect false). - for (const excluded of [ 'src/__tests__/a.ts', 'src/a.test.ts', 'src/a.spec.tsx', 'src/a.test.mjs', 'src/a.spec.cjs', 'src/a.test.jsx', + ]) { + expect(isReviewableGateFile(reviewable)).toBe(true) + } + + // Generated code, docs, data/config, env, and bookkeeping dirs + // (expect false). + for (const excluded of [ 'src/a.generated.ts', 'src/a.generated.tsx', 'README.md', @@ -206,14 +206,14 @@ describe('gate-paths helpers', () => { } }) - test('selectReviewableGateFiles normalizes, dedupes, drops empties, keeps only reviewable source', () => { + test('selectReviewableGateFiles normalizes, dedupes, and keeps source and tests', () => { const cwd = process.cwd().replace(/\\/g, '/').replace(/\/+$/, '') const result = selectReviewableGateFiles([ 'src/a.ts', './src/a.ts', // dedupes with src/a.ts after normalization `${cwd}/src/a.ts`, // in-cwd absolute -> src/a.ts (dedupe) 'src\\b.ts', // backslashes -> src/b.ts - 'src/a.test.ts', // test file excluded + 'src/a.test.ts', // test file included 'src/a.generated.ts', // generated excluded 'README.md', // doc excluded 'docs/guide.ts', // docs/ excluded @@ -221,7 +221,7 @@ describe('gate-paths helpers', () => { '', // empty dropped ' ', // whitespace dropped ]) - expect(result).toEqual(['src/a.ts', 'src/b.ts']) + expect(result).toEqual(['src/a.ts', 'src/b.ts', 'src/a.test.ts']) }) test('selectCoverageEvidenceFiles normalizes, dedupes, and keeps only test files', () => { diff --git a/agents/base2/base2.ts b/agents/base2/base2.ts index 755d35bbca..0f5e52c802 100644 --- a/agents/base2/base2.ts +++ b/agents/base2/base2.ts @@ -446,10 +446,12 @@ ${specialistRoutingSection} const mutableAgentState = (agentState ?? {}) as Base2AgentState const agentId = mutableAgentState.agentId const configuredHasNoValidation = config?.hasNoValidation + const configuredPlanOnly = config?.planOnly === true const runValidationGate = - typeof configuredHasNoValidation === 'boolean' + !configuredPlanOnly && + (typeof configuredHasNoValidation === 'boolean' ? !configuredHasNoValidation - : agentId !== 'base2-fast' && agentId !== 'base2-fast-no-validation' + : agentId !== 'base2-fast' && agentId !== 'base2-fast-no-validation') // M3 (R1a–R1d) automated phase-gate predicates. These mirror the // advisory glob list in securityReviewSection (quality-prompt-section.ts) // so the automated gate and the advisory prompt agree on what is @@ -479,13 +481,6 @@ ${specialistRoutingSection} const reviewerAgentType = 'code-reviewer' const MAX_REPAIR_ROUNDS = 3 const MAX_REVIEWER_NO_VERDICT_RETRIES = 1 - // static-review-only concurrency (M3.1): when the reviewer is configured - // for static-only review, it can run concurrently with the blocking - // validation hooks. Defaults to false so the existing sequential - // validation-then-reviewer behavior is preserved. - const staticReviewOnlyEnabled = !!( - mutableAgentState.base2ActiveWork?.staticReviewOnly ?? false - ) const existingActiveWorkState = mutableAgentState.base2ActiveWork const hadPendingGateFiles = !!existingActiveWorkState && @@ -2125,95 +2120,22 @@ ${specialistRoutingSection} break } - // Verification gate: after the model thinks it's done, run configured - // file-change hooks (typecheck/lint/test). If any failed, surface the - // failures and keep the turn open so the model fixes them. The runtime's - // max step limit bounds pathological retry loops; the gate itself must - // not silently skip validation after repeated failures. - // - // static-review-only concurrency (M3.1): when the reviewer is - // static-only (base2ActiveWork.staticReviewOnly), spawn it in the - // background BEFORE the blocking validation hooks so static review - // runs concurrently. The join contract is preserved: a validation - // failure still `continue`s below and ignores this background job; we - // only `check_background_agent` for its result if validation passes. + // Validation runs before the final reviewer because hooks may mutate + // generated or formatted output. The exact source and test snapshot is + // re-captured at the reviewer spawn boundary below. const requiredReviewerAgentType = activeWorkState.requiredReviewerRevalidation ?? reviewerAgentType - const staticReviewConcurrency = - runReviewerGate && - editsHappened && - staticReviewOnlyEnabled && - requiredReviewerAgentType === reviewerAgentType - // Fix 1/2/3 (reviewer-gate scoping): the final code-reviewer must only - // ever be asked to attest to reviewable *source* files, never - // bookkeeping/docs/plan artifacts (e.g. .agents/sessions/** - // STATE.json/EVENTS.jsonl/STATUS.md/LESSONS.md). Drive the reviewer's - // snapshot details and pending-file attestation off the reviewable - // subset so the fingerprint the reviewer echoes matches what - // attestation checks. Validation-hook behavior is unchanged; only the - // reviewer spawn/attestation scoping uses this subset. - const reviewablePendingFiles = selectReviewableGateFiles( + let reviewablePendingFiles = selectReviewableGateFiles( Array.from(pendingGateFiles), ) - const reviewSnapshotDetails = buildGateSnapshotDetails( + let reviewSnapshotDetails = buildGateSnapshotDetails( reviewablePendingFiles, '', ) - const reviewSnapshotFingerprint = hashGateSnapshotDetails( + let reviewSnapshotFingerprint = hashGateSnapshotDetails( reviewSnapshotDetails, ) - // Fingerprint of the reviewable subset for the R5 skip-re-review - // short-circuit. Equal to reviewSnapshotFingerprint today, but kept - // as its own binding so the intent (reviewable-set identity) is clear - // and stable if the review snapshot inputs ever change. - const reviewableFingerprint = hashGateSnapshotDetails( - buildGateSnapshotDetails(reviewablePendingFiles, ''), - ) - // Co-changed test files are excluded from the reviewable fingerprint - // set (isReviewableGateFile drops tests), which historically made it - // impossible for the reviewer to confirm coverage — it could never see - // the changed test file, so it always reported coverage missing/ - // uncertain and the gate looped. Surface them as readable, additive - // "coverage evidence" context. This does NOT enter reviewSnapshot - // Fingerprint or attestation (those stay scoped to reviewable source). - const coverageEvidenceFiles = selectCoverageEvidenceFiles( - Array.from(pendingGateFiles), - ) - const coverageEvidenceDetails = coverageEvidenceFiles.length - ? buildGateSnapshotDetails(coverageEvidenceFiles, '') - : '(no co-changed test files)' - if (staticReviewConcurrency && !activeWorkState.staticReviewerJobId) { - const bgReview = yield { - toolName: 'spawn_agents', - input: { - agents: [ - { - agent_type: requiredReviewerAgentType, - background: true, - prompt: [ - 'Review the completed default-flow code changes before finalization.', - '', - `Pending changed files: ${reviewablePendingFiles.join(', ') || '(unknown)'}`, - `Snapshot fingerprint (echo exactly): ${reviewSnapshotFingerprint}`, - 'Snapshot details (read for file membership; do not echo):', - reviewSnapshotDetails, - 'Coverage evidence (co-changed tests; readable and citable, not part of the reviewed fingerprint):', - coverageEvidenceDetails, - 'Validation gate summary: Reviewer running concurrently with validation (static-review-only mode).', - '', - 'Return the required structured review object. Echo snapshotFingerprint exactly, list every reviewed file, evaluate all review dimensions, and map every user requirement to evidence. Co-changed test files are listed under "Coverage evidence" above; they are readable and citable even though they are not part of the reviewed fingerprint. Test-coverage requirements are satisfied (not uncertain) when a Coverage evidence test file covers the changed behavior — cite that test file and the covering test name(s) as the requirement evidence. Use coverage: missing only when no covering test exists in the Coverage evidence list or anywhere in the repo, and requirement status uncertain only when you could not inspect a listed Coverage evidence test file at all.', - ].join('\n'), - }, - ], - }, - } as any - const bgJobId = extractBackgroundAgentJobId( - (bgReview as any) && (bgReview as any).toolResult, - ) - if (bgJobId) { - activeWorkState.staticReviewerJobId = bgJobId - } - } + let reviewableFingerprint = reviewSnapshotFingerprint let validationSummary = 'No file changes were detected, so no validation hooks ran.' const validationInfrastructureBypassed = @@ -2777,6 +2699,20 @@ ${specialistRoutingSection} activeWorkState.currentPhase = 'awaiting_review' } + // Validation has completed. Re-capture the exact source+test snapshot + // immediately before any final reviewer spawn or skip decision. + reviewablePendingFiles = selectReviewableGateFiles( + Array.from(pendingGateFiles), + ) + reviewSnapshotDetails = buildGateSnapshotDetails( + reviewablePendingFiles, + '', + ) + reviewSnapshotFingerprint = hashGateSnapshotDetails( + reviewSnapshotDetails, + ) + reviewableFingerprint = reviewSnapshotFingerprint + let reviewerFinalizationVerdict: 'LOOKS_GOOD' | 'NON_BLOCKING' | '' = reviewerProtocolBypassAuthorized ? 'NON_BLOCKING' : '' if (reviewerProtocolBypassAuthorized) { @@ -2882,52 +2818,29 @@ ${specialistRoutingSection} ) { activeWorkState.lastReviewerGateSkipReason = '' markActiveWorkStateChanged() - let reviewerToolResult: unknown - if (staticReviewConcurrency && activeWorkState.staticReviewerJobId) { - const checkResult = yield { - toolName: 'check_background_agent', - input: { - jobId: activeWorkState.staticReviewerJobId, - timeout_seconds: 120, - }, - } as any - // check_background_agent returns { result } where `result` is the - // subagent's final output (same shape a foreground spawn_agents - // toolResult wraps). Wait for the background reviewer to settle - // rather than matching only one verdict token: LOOKS_GOOD and - // NON_BLOCKING both pass, while BLOCKING must be parsed below as - // actionable feedback. On error/timeout with no result, fall - // through to the existing 'did not return LOOKS_GOOD/NON_BLOCKING' - // blocked handling below. - reviewerToolResult = - (checkResult as any)?.toolResult?.result ?? - (checkResult as any)?.toolResult - } else { - const review = yield { - toolName: 'spawn_agents', - input: { - agents: [ - { - agent_type: requiredReviewerAgentType, - prompt: [ - `Review the completed ${requiredReviewerAgentType} changes before finalization.`, - '', - `Pending changed files: ${reviewablePendingFiles.join(', ') || '(unknown)'}`, - `Snapshot fingerprint (echo exactly): ${reviewSnapshotFingerprint}`, - 'Snapshot details (read for file membership; do not echo):', - reviewSnapshotDetails, - 'Coverage evidence (co-changed tests; readable and citable, not part of the reviewed fingerprint):', - coverageEvidenceDetails, - `Validation gate summary: ${validationSummary}`, - '', - 'Return the required structured review object. Echo snapshotFingerprint exactly, list every reviewed file, evaluate all review dimensions, and map every user requirement to evidence. Co-changed test files are listed under "Coverage evidence" above; they are readable and citable even though they are not part of the reviewed fingerprint. Test-coverage requirements are satisfied (not uncertain) when a Coverage evidence test file covers the changed behavior — cite that test file and the covering test name(s) as the requirement evidence. Use coverage: missing only when no covering test exists in the Coverage evidence list or anywhere in the repo, and requirement status uncertain only when you could not inspect a listed Coverage evidence test file at all.', - ].join('\n'), - }, - ], - }, - } as any - reviewerToolResult = (review as any) && (review as any).toolResult - } + const review = yield { + toolName: 'spawn_agents', + input: { + agents: [ + { + agent_type: requiredReviewerAgentType, + prompt: [ + `Review the completed ${requiredReviewerAgentType} changes before finalization.`, + '', + `Pending changed files: ${reviewablePendingFiles.join(', ') || '(unknown)'}`, + `Snapshot fingerprint (echo exactly): ${reviewSnapshotFingerprint}`, + 'Snapshot details (read for file membership; do not echo):', + reviewSnapshotDetails, + `Validation gate summary: ${validationSummary}`, + '', + 'Return the required structured review object. Echo snapshotFingerprint exactly, list every pending changed file in reviewedFiles (including tests), evaluate all review dimensions, and map every user requirement to evidence. Changed tests are first-class review targets and may also be cited as coverage evidence. Use coverage: missing only when no covering test exists in the changed files or elsewhere in the repo.', + ].join('\n'), + }, + ], + }, + } as any + let reviewerToolResult: unknown = + (review as any) && (review as any).toolResult const reviewerCrashedBeforeAttestation = detectReviewerCrash(reviewerToolResult) let attestationIssues = reviewerCrashedBeforeAttestation @@ -2961,8 +2874,6 @@ ${specialistRoutingSection} `Snapshot fingerprint (echo exactly): ${reviewSnapshotFingerprint}`, 'Snapshot details (read for file membership; do not echo):', reviewSnapshotDetails, - 'Coverage evidence (co-changed tests; readable and citable, not part of the reviewed fingerprint):', - coverageEvidenceDetails, `Validation gate summary: ${validationSummary}`, '', 'Protocol errors from the prior response:', @@ -3559,8 +3470,10 @@ ${specialistRoutingSection} continue } if (editsHappened) { - activeWorkState.lastReviewerGateSkipReason = - 'validation-and-reviewer-gates-disabled' + const disabledGateReason = configuredPlanOnly + ? 'plan-only-automatic-finalization-gate-disabled' + : 'validation-and-reviewer-gates-disabled' + activeWorkState.lastReviewerGateSkipReason = disabledGateReason markActiveWorkStateChanged() emitGateTelemetry({ currentPhase: activeWorkState.currentPhase, @@ -3568,19 +3481,21 @@ ${specialistRoutingSection} pendingFiles: Array.from(pendingGateFiles), reviewerStatus: 'skipped', validationStatus: 'skipped', - skipReason: 'validation-and-reviewer-gates-disabled', + skipReason: disabledGateReason, }) yield { toolName: 'add_message', input: { role: 'user', content: [ - 'Validation and reviewer gates are disabled for this agent mode; skipping automated gate checks even though edits were detected.', + configuredPlanOnly + ? 'PLAN-only mode disables the automatic validation/reviewer finalization gate; manual reviewer-family analysis remains available.' + : 'Validation and reviewer gates are disabled for this agent mode; skipping automated gate checks even though edits were detected.', `Pending edited files: ${Array.from(pendingGateFiles).join(', ') || '(unknown files)'}`, formatGateStateBlock( 'validation/reviewer', 'skipped', - `validation-and-reviewer-gates-disabled: skipped automated gate checks for pending files: ${Array.from(pendingGateFiles).join(', ') || '(unknown files)'}`, + `${disabledGateReason}: skipped automated gate checks for pending files: ${Array.from(pendingGateFiles).join(', ') || '(unknown files)'}`, ), ].join('\n'), }, @@ -4197,12 +4112,10 @@ ${specialistRoutingSection} // Kept byte-identical (minus the export keyword) because handleSteps is // serialized via .toString() + new Function(...) and cannot reference // module-scope imports; a parity test asserts behavioral equality. - // Returns true only for reviewable source files; excludes tests, - // generated code, docs, config/data (.jsonl bookkeeping like - // EVENTS.jsonl), .env files, and docs/ / evals/ / .agents/ paths. + // Returns true for reviewable source and test files; excludes generated + // code, docs, config/data (.jsonl bookkeeping like EVENTS.jsonl), .env + // files, and docs/ / evals/ / .agents/ paths. function isReviewableGateFile(filePath: string): boolean { - if (/__tests__\//.test(filePath)) return false - if (/\.(test|spec)\.(?:tsx?|jsx?|mjs|cjs)$/.test(filePath)) return false if (/\.generated\.tsx?$/.test(filePath)) return false if (/\.(md|mdx|json|jsonl|yml|yaml|toml)$/.test(filePath)) return false if (/(^|\/)\.env($|\.)/.test(filePath)) return false @@ -5496,36 +5409,6 @@ ${specialistRoutingSection} return remaining } - function extractBackgroundAgentJobId( - toolResult: unknown, - ): string | undefined { - // The background spawn_agents report is an array of tool-result parts; - // the background entry has value.background === true with a jobId. Also - // tolerate a direct object shape (value or the part itself) so this - // stays robust to small runtime report-shape variations. - const candidates: unknown[] = [] - if (Array.isArray(toolResult)) { - for (const part of toolResult) { - const value = - part && (part as any).type === 'json' ? (part as any).value : part - candidates.push(value) - } - } else if (toolResult && typeof toolResult === 'object') { - candidates.push((toolResult as any).value ?? toolResult) - } - for (const value of candidates) { - if ( - value && - typeof value === 'object' && - (value as any).background === true && - typeof (value as any).jobId === 'string' - ) { - return (value as any).jobId - } - } - return undefined - } - function collectReviewerBlockers(toolResult: unknown): string[] { // First check for structured reviewer outputs (e.g. JSON with a // verdict field). When present and BLOCKING, surface findings as the diff --git a/agents/base2/gate-paths.ts b/agents/base2/gate-paths.ts index dac53753d9..b5a7476d2b 100644 --- a/agents/base2/gate-paths.ts +++ b/agents/base2/gate-paths.ts @@ -66,16 +66,12 @@ export function gateFileSetsEqual(left: string[], right: string[]): boolean { return left.every((file) => rightFiles.has(file)) } -// Returns true only for reviewable source files. Everything else — tests, -// generated code, docs, config/data files (including .jsonl bookkeeping like -// EVENTS.jsonl), .env files, and anything under docs/, evals/, or .agents/ — -// is excluded so the final code-reviewer gate never fires on -// bookkeeping/docs/plan artifacts. Mirrors the exclusion style of -// isPublicApiSourceFile in base2.ts but ALSO drops `.jsonl` and `.env` -// basenames. Operates on an already-normalized path (caller normalizes). +// Returns true for reviewable source and test files. Generated code, docs, +// config/data files (including .jsonl bookkeeping like EVENTS.jsonl), .env +// files, and anything under docs/, evals/, or .agents/ remain excluded so the +// final code-reviewer gate never attests to bookkeeping/docs/plan artifacts. +// Operates on an already-normalized path (caller normalizes). export function isReviewableGateFile(filePath: string): boolean { - if (/__tests__\//.test(filePath)) return false - if (/\.(test|spec)\.(?:tsx?|jsx?|mjs|cjs)$/.test(filePath)) return false if (/\.generated\.tsx?$/.test(filePath)) return false if (/\.(md|mdx|json|jsonl|yml|yaml|toml)$/.test(filePath)) return false if (/(^|\/)\.env($|\.)/.test(filePath)) return false @@ -101,11 +97,9 @@ export function selectReviewableGateFiles(files: string[]): string[] { return reviewableFiles } -// Co-changed test files (the complement of isReviewableGateFile's test -// exclusion). These are surfaced to the final reviewer as readable -// "coverage evidence" so it can confirm the changed behavior is tested, -// WITHOUT adding tests to the reviewed-for-defects fingerprint set. -// Operates on an already-normalized path (caller normalizes). +// Co-changed test files remain identifiable for coverage-specific prompting, +// but they are also first-class reviewable files and participate in the final +// reviewer fingerprint and reviewedFiles attestation. export function isCoverageEvidenceFile(filePath: string): boolean { if (/__tests__\//.test(filePath)) return true if (/\.(test|spec)\.(?:tsx?|jsx?|mjs|cjs)$/.test(filePath)) return true diff --git a/agents/e2e/reviewer-spawn-conditions.e2e.test.ts b/agents/e2e/reviewer-spawn-conditions.e2e.test.ts index ff9ba02752..028bb7909c 100644 --- a/agents/e2e/reviewer-spawn-conditions.e2e.test.ts +++ b/agents/e2e/reviewer-spawn-conditions.e2e.test.ts @@ -137,6 +137,66 @@ describe('base2 reviewer spawn conditions e2e', () => { expect(done.done).toBe(true) }) + test('PLAN-only skips the automatic finalization gate with an explicit reason', () => { + const base2 = createBase2('default', { planOnly: true }) + const agentState = { agentId: 'base2-plan' } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Plan the lifecycle change.', + params: {}, + config: base2.programmaticConfig, + } as any) + + expect(gen.next().value).toMatchObject({ toolName: 'query_index' }) + expect(gen.next(feedJson([])).value).toMatchObject({ toolName: 'add_message' }) + expect(gen.next(feedJson([])).value).toMatchObject({ toolName: 'git_status' }) + expect(gen.next(feedJson({ status: '' })).value).toMatchObject({ + toolName: 'spawn_agent_inline', + }) + expect(gen.next().value).toBe('STEP') + expect(gen.next(finishStep({ file: '.agents/sessions/x/PLAN.md' })).value).toMatchObject({ + toolName: 'git_status', + }) + const skipped = gen.next( + feedJson({ status: ' M .agents/sessions/x/PLAN.md' }), + ) + expect(skipped.value).toMatchObject({ toolName: 'add_message' }) + const text = (skipped.value as any).input.content as string + expect(text).toContain('plan-only-automatic-finalization-gate-disabled') + expect(parseGateStateBlock(text)).toMatchObject({ + gate: 'validation/reviewer', + status: 'skipped', + }) + expect(base2.spawnableAgents).toContain('code-reviewer') + }) + + test('EXECUTE_PLAN retains automatic validation before reviewer spawn', () => { + const base2 = createBase2('default', { executePlan: true }) + const gen = base2.handleSteps!({ + agentState: { agentId: 'base2-execute-plan' }, + prompt: 'Execute the lifecycle change.', + params: {}, + config: base2.programmaticConfig, + } as any) + + expect(gen.next().value).toMatchObject({ toolName: 'query_index' }) + expect(gen.next(feedJson([])).value).toMatchObject({ toolName: 'add_message' }) + expect(gen.next(feedJson([])).value).toMatchObject({ toolName: 'git_status' }) + expect(gen.next(feedJson({ status: '' })).value).toMatchObject({ + toolName: 'spawn_agent_inline', + }) + expect(gen.next().value).toBe('STEP') + expect(gen.next(finishStep({ file: 'src/lifecycle.ts' })).value).toMatchObject({ + toolName: 'git_status', + }) + expect( + gen.next(feedJson({ status: ' M src/lifecycle.ts' })).value, + ).toMatchObject({ + toolName: 'run_file_change_hooks', + input: { files: ['src/lifecycle.ts'] }, + }) + }) + test('no edits detected emits passed no-edits gate and does not spawn reviewer', () => { const base2 = createBase2('default') const agentState = { agentId: 'base2-custom' } diff --git a/common/src/util/__tests__/audit-receipt.test.ts b/common/src/util/__tests__/audit-receipt.test.ts index d27a0ab5f0..4ade257c0f 100644 --- a/common/src/util/__tests__/audit-receipt.test.ts +++ b/common/src/util/__tests__/audit-receipt.test.ts @@ -50,6 +50,25 @@ describe('containsStructuralAuditReceipt', () => { ).toBe(false) }) + it('terminates and returns false for a cyclic object graph without a receipt', () => { + const value: Record = { nested: {} } + value.self = value + ;(value.nested as Record).parent = value + + expect(containsStructuralAuditReceipt(value, 'snapshot-1')).toBe(false) + }) + + it('returns true for a receipt nested deeper than the old depth limit', () => { + let value: Record = { + structuralReceipt: { snapshot_id: 'snapshot-deep' }, + } + for (let depth = 0; depth < 13; depth += 1) { + value = { nested: value } + } + + expect(containsStructuralAuditReceipt(value, 'snapshot-deep')).toBe(true) + }) + it('returns false for non-object inputs', () => { expect(containsStructuralAuditReceipt(null, 'snapshot-1')).toBe(false) expect(containsStructuralAuditReceipt('snapshot-1', 'snapshot-1')).toBe(false) diff --git a/common/src/util/audit-receipt.ts b/common/src/util/audit-receipt.ts index 67eaeab76b..7a30096f84 100644 --- a/common/src/util/audit-receipt.ts +++ b/common/src/util/audit-receipt.ts @@ -1,23 +1,35 @@ +const MAX_TRAVERSAL_DEPTH = 32 + /** * Recursively searches an arbitrary value for a nested `structuralReceipt` * object whose `snapshot_id` matches `expectedSnapshotId`. When * `expectedSnapshotId` is empty/undefined, any structuralReceipt with a string - * `snapshot_id` counts as a match. Depth-bounded (<=12) to stay safe on deeply - * nested or cyclic-looking inputs. Shared by the general-agent handleSteps gate - * and the runtime buildRuntimeAgentReceipt gate so the two do not drift. + * `snapshot_id` counts as a match. Depth-bounded (<=32) to stay conservative + * on deeply nested inputs, with visited-object tracking for cyclic/shared graphs. + * Shared by the general-agent handleSteps gate and the runtime + * buildRuntimeAgentReceipt gate so the two do not drift. */ export function containsStructuralAuditReceipt( value: unknown, expectedSnapshotId?: string, ): boolean { let found = false + const visited = new WeakSet() const visit = (item: unknown, depth = 0): void => { - if (found || !item || depth > 12) return + if ( + found || + !item || + depth > MAX_TRAVERSAL_DEPTH || + typeof item !== 'object' + ) { + return + } + if (visited.has(item)) return + visited.add(item) if (Array.isArray(item)) { for (const nested of item) visit(nested, depth + 1) return } - if (typeof item !== 'object') return const record = item as Record const receipt = record.structuralReceipt if (receipt && typeof receipt === 'object' && !Array.isArray(receipt)) { diff --git a/packages/agent-runtime/src/__tests__/process-edit-transaction.test.ts b/packages/agent-runtime/src/__tests__/process-edit-transaction.test.ts index c19231d446..d551c95049 100644 --- a/packages/agent-runtime/src/__tests__/process-edit-transaction.test.ts +++ b/packages/agent-runtime/src/__tests__/process-edit-transaction.test.ts @@ -1119,7 +1119,7 @@ describe('processEditTransaction', () => { } }) - it('does not coalesce same-file str_replace edits across other edit types', async () => { + it('blocks a replace_range that follows a same-file str_replace before later edits', async () => { const initialContent = 'const a = 1\nconst b = 1\nconst c = 1\n' const result = await processEditTransaction({ initialContentByPath: new Map([['src/helper.ts', initialContent]]), @@ -1178,13 +1178,18 @@ describe('processEditTransaction', () => { expect('error' in result).toBe(true) if ('error' in result) { + // The replace_range at edit index 1 follows a str_replace that already + // changed src/helper.ts, so the line-shift guard aborts before the third + // edit is ever reached. expect(result.failures[0]).toEqual( expect.objectContaining({ - editIndex: 2, + editIndex: 1, path: 'src/helper.ts', }), ) - expect(result.failures[0]?.errorMessage).toContain('basedOnRead') + expect(result.failures[0]?.errorMessage).toContain( + 'a prior non-replace_range edit changed this file earlier in the transaction', + ) } }) @@ -1629,4 +1634,137 @@ describe('processEditTransaction', () => { expect(retry.failures[0]!.errorMessage).toBe(message) } }) + + it('reports a narrowed sub-range when replace_range targets fewer lines than the capability covers', async () => { + const initial = 'alpha\nbeta\ngamma\n' + const issuer = { projectId: '/project', runId: 'run-narrowed-subrange' } + const result = await processEditTransaction({ + initialContentByPath: new Map([['src/file.ts', initial]]), + logger, + readCapabilityIssuer: issuer, + edits: [ + { + type: 'replace_range', + path: 'src/file.ts', + ...readAuthorization({ + path: 'src/file.ts', + startLine: 1, + endLine: 3, + content: 'alpha\nbeta\ngamma', + issuer, + }), + startLine: 2, + endLine: 2, + newContent: 'BETA', + }, + ], + }) + + expect('files' in result).toBe(true) + if ('files' in result) { + expect(result.files[0].content).toBe('alpha\nBETA\ngamma\n') + expect(result.files[0].messages).toContain( + 'Replaced lines 2-2 in src/file.ts within the readCapability-covered range.', + ) + } + }) + + it('aborts overlapping replace_range edits in one transaction without applying changes', async () => { + const initial = 'one\ntwo\nthree\nfour\n' + const issuer = { projectId: '/project', runId: 'run-overlap-abort' } + const result = await processEditTransaction({ + initialContentByPath: new Map([['src/file.ts', initial]]), + logger, + readCapabilityIssuer: issuer, + edits: [ + { + type: 'replace_range', + path: 'src/file.ts', + ...readAuthorization({ + path: 'src/file.ts', + startLine: 1, + endLine: 2, + content: 'one\ntwo', + issuer, + }), + startLine: 1, + endLine: 2, + newContent: 'ONE\nTWO', + }, + { + type: 'replace_range', + path: 'src/file.ts', + ...readAuthorization({ + path: 'src/file.ts', + startLine: 2, + endLine: 3, + content: 'two\nthree', + issuer, + }), + startLine: 2, + endLine: 3, + newContent: 'TWO\nTHREE', + }, + ], + }) + + expect('error' in result).toBe(true) + if ('error' in result) { + expect(result.failures[0]).toEqual( + expect.objectContaining({ editIndex: 1, path: 'src/file.ts' }), + ) + expect(result.failures[0]?.errorMessage).toContain( + 'overlap a prior replace_range', + ) + } + expect('files' in result).toBe(false) + }) + + it('blocks a replace_range after a prior non-replace_range edit changed the same file', async () => { + const initial = 'const a = 1\nconst b = 1\nconst c = 1\n' + const issuer = { projectId: '/project', runId: 'run-nonrange-block' } + const result = await processEditTransaction({ + initialContentByPath: new Map([['src/file.ts', initial]]), + logger, + readCapabilityIssuer: issuer, + edits: [ + { + type: 'str_replace', + path: 'src/file.ts', + replacements: [ + { + oldString: 'const a = 1', + newString: 'const a = 1\nconst inserted = true', + allowMultiple: false, + }, + ], + }, + { + type: 'replace_range', + path: 'src/file.ts', + ...readAuthorization({ + path: 'src/file.ts', + startLine: 3, + endLine: 3, + content: 'const c = 1', + issuer, + }), + startLine: 3, + endLine: 3, + newContent: 'const c = 2', + }, + ], + }) + + expect('error' in result).toBe(true) + if ('error' in result) { + expect(result.failures[0]).toEqual( + expect.objectContaining({ editIndex: 1, path: 'src/file.ts' }), + ) + expect(result.failures[0]?.errorMessage).toContain( + 'a prior non-replace_range edit changed this file earlier in the transaction', + ) + } + expect('files' in result).toBe(false) + }) }) diff --git a/packages/agent-runtime/src/__tests__/process-str-replace.test.ts b/packages/agent-runtime/src/__tests__/process-str-replace.test.ts index 3702ec159e..2d1e5a3ec1 100644 --- a/packages/agent-runtime/src/__tests__/process-str-replace.test.ts +++ b/packages/agent-runtime/src/__tests__/process-str-replace.test.ts @@ -1447,6 +1447,10 @@ function test3() { expect(result).toHaveProperty('error') if ('error' in result) { expect(result.error).toContain('different project, path, or agent run') + expect(result.error).toContain('Cross-path and cross-run capability replay') + expect(result.error).not.toContain('may refer to content that changed') + expect(result.error).not.toContain('content may have been removed') + expect(result.error).not.toContain('Before attempting another str_replace') } }) diff --git a/packages/agent-runtime/src/__tests__/read-files-edit-state.test.ts b/packages/agent-runtime/src/__tests__/read-files-edit-state.test.ts index 5bd545c4e8..777801754c 100644 --- a/packages/agent-runtime/src/__tests__/read-files-edit-state.test.ts +++ b/packages/agent-runtime/src/__tests__/read-files-edit-state.test.ts @@ -620,10 +620,15 @@ describe('read_files edit-state recovery', () => { expect(fileProcessingState.promisesByPath[path]).toBeUndefined() }) - it('invalidates prepared str_replace state when the client rejects the patch', async () => { + it('preserves authorization when the client explicitly rejects str_replace without applying', async () => { const path = 'src/rejected.ts' const diskContent = 'export const value = 1\n' const fileProcessingState = createFileProcessingState() + fileProcessingState.strictReadBeforeEdit = true + fileProcessingState.readAuthorizationsByPath = { [path]: true } + fileProcessingState.readAuthorizationHashesByPath = { + [path]: getContentHash(diskContent), + } const result = await handleStrReplace({ previousToolCallFinished: Promise.resolve(), @@ -660,8 +665,11 @@ describe('read_files edit-state recovery', () => { errorMessage: 'client rejected patch', }) } - expect(fileProcessingState.failedEditRequiresReadByPath[path]).toBe(true) + expect( + fileProcessingState.failedEditRequiresReadByPath[path], + ).toBeUndefined() expect(fileProcessingState.promisesByPath[path]).toBeUndefined() + expect(fileProcessingState.readAuthorizationsByPath?.[path]).toBe(true) }) it('[ABI-M05] sends exact whole-file bytes and rejects an unconfirmed client result', async () => { @@ -724,7 +732,7 @@ describe('read_files edit-state recovery', () => { expect(fileProcessingState.promisesByPath[path]).toBeUndefined() }) - it('does not grant write authorization or retain prepared state when the client rejects write_file', async () => { + it('preserves existing write authorization when the client explicitly rejects write_file', async () => { const path = 'src/rejected-write.ts' const diskContent = 'export const value = 1\n' const fileProcessingState = createFileProcessingState() @@ -769,15 +777,17 @@ describe('read_files edit-state recovery', () => { errorMessage: 'client rejected write', }) } - expect(fileProcessingState.failedEditRequiresReadByPath[path]).toBe(true) - expect(fileProcessingState.promisesByPath[path]).toBeUndefined() - expect(fileProcessingState.readAuthorizationsByPath?.[path]).toBeUndefined() expect( - fileProcessingState.readAuthorizationHashesByPath?.[path], + fileProcessingState.failedEditRequiresReadByPath[path], ).toBeUndefined() + expect(fileProcessingState.promisesByPath[path]).toBeUndefined() + expect(fileProcessingState.readAuthorizationsByPath?.[path]).toBe(true) + expect(fileProcessingState.readAuthorizationHashesByPath?.[path]).toBe( + getContentHash(diskContent), + ) }) - it('detects a write_file client error in any output part and revokes authorization', async () => { + it('detects a late explicit write_file rejection and preserves authorization', async () => { const path = 'src/rejected-late-write.ts' const fileProcessingState = createFileProcessingState() fileProcessingState.strictReadBeforeEdit = true @@ -813,9 +823,11 @@ describe('read_files edit-state recovery', () => { } as any) expect(result.output).toHaveLength(2) - expect(fileProcessingState.failedEditRequiresReadByPath[path]).toBe(true) + expect( + fileProcessingState.failedEditRequiresReadByPath[path], + ).toBeUndefined() expect(fileProcessingState.promisesByPath[path]).toBeUndefined() - expect(fileProcessingState.readAuthorizationsByPath?.[path]).toBeUndefined() + expect(fileProcessingState.readAuthorizationsByPath?.[path]).toBe(true) }) it('registers write_file processing before waiting for previous tool completion', async () => { @@ -1153,10 +1165,15 @@ describe('read_files edit-state recovery', () => { } }) - it('blocks later str_replace calls after edit_transaction preflight fails', async () => { + it('preserves valid read authorization after edit_transaction preflight fails', async () => { const path = 'src/helper.ts' const diskContent = 'export const value = 1\n' const fileProcessingState = createFileProcessingState() + fileProcessingState.strictReadBeforeEdit = true + fileProcessingState.readAuthorizationsByPath = { [path]: true } + fileProcessingState.readAuthorizationHashesByPath = { + [path]: getContentHash(diskContent), + } const transactionResult = await handleEditTransaction({ previousToolCallFinished: Promise.resolve(), @@ -1193,8 +1210,12 @@ describe('read_files edit-state recovery', () => { if (output.type === 'json') { expect(output.value).toHaveProperty('errorMessage') } - expect(fileProcessingState.failedEditRequiresReadByPath[path]).toBe(true) + expect( + fileProcessingState.failedEditRequiresReadByPath[path], + ).toBeUndefined() + expect(fileProcessingState.readAuthorizationsByPath?.[path]).toBe(true) + let followUpApplied = false const strReplaceResult = await handleStrReplace({ previousToolCallFinished: Promise.resolve(), toolCall: { @@ -1215,27 +1236,23 @@ describe('read_files edit-state recovery', () => { logger, requestOptionalFile: async ({ filePath }: { filePath: string }) => filePath === path ? diskContent : null, - requestClientToolCall: async () => { - throw new Error('should not apply blocked edit') + requestClientToolCall: async (clientToolCall: any) => { + followUpApplied = true + return confirmedMutationOutput(clientToolCall) }, writeToClient: () => {}, } as any) + expect(followUpApplied).toBe(true) const replaceOutput = strReplaceResult.output[0] - expect(fileProcessingState.editRereadRequirementsByPath?.[path]).toEqual({ - reason: 'preflight_failed', - sourceTool: 'edit_transaction', - }) + expect(fileProcessingState.editRereadRequirementsByPath?.[path]).toBeUndefined() expect(replaceOutput.type).toBe('json') if (replaceOutput.type === 'json') { - expect(replaceOutput.value).toHaveProperty('errorMessage') - expect( - String((replaceOutput.value as { errorMessage?: string }).errorMessage), - ).toContain('previous edit_transaction preflight failed') + expect(replaceOutput.value).not.toHaveProperty('errorMessage') } }) - it('marks all transaction paths as requiring re-read when client rejects a patch', async () => { + it('preserves valid authorization for all transaction paths when the client explicitly rejects without applying', async () => { const path = 'src/helper.ts' const otherPath = 'src/other.ts' const diskContentByPath: Record = { @@ -1243,6 +1260,15 @@ describe('read_files edit-state recovery', () => { [otherPath]: 'export const other = 1\n', } const fileProcessingState = createFileProcessingState() + fileProcessingState.strictReadBeforeEdit = true + fileProcessingState.readAuthorizationsByPath = { + [path]: true, + [otherPath]: true, + } + fileProcessingState.readAuthorizationHashesByPath = { + [path]: getContentHash(diskContentByPath[path]), + [otherPath]: getContentHash(diskContentByPath[otherPath]), + } const transactionResult = await handleEditTransaction({ previousToolCallFinished: Promise.resolve(), @@ -1302,11 +1328,16 @@ describe('read_files edit-state recovery', () => { } expect(fileProcessingState.promisesByPath[path]).toBeUndefined() expect(fileProcessingState.promisesByPath[otherPath]).toBeUndefined() - expect(fileProcessingState.failedEditRequiresReadByPath[path]).toBe(true) - expect(fileProcessingState.failedEditRequiresReadByPath[otherPath]).toBe( - true, - ) + expect( + fileProcessingState.failedEditRequiresReadByPath[path], + ).toBeUndefined() + expect( + fileProcessingState.failedEditRequiresReadByPath[otherPath], + ).toBeUndefined() + expect(fileProcessingState.readAuthorizationsByPath?.[path]).toBe(true) + expect(fileProcessingState.readAuthorizationsByPath?.[otherPath]).toBe(true) + let followUpApplied = false const strReplaceResult = await handleStrReplace({ previousToolCallFinished: Promise.resolve(), toolCall: { @@ -1316,7 +1347,7 @@ describe('read_files edit-state recovery', () => { path, replacements: [ { - oldString: 'export const value = 2', + oldString: 'export const value = 1', newString: 'export const value = 3', allowMultiple: false, }, @@ -1327,23 +1358,19 @@ describe('read_files edit-state recovery', () => { logger, requestOptionalFile: async ({ filePath }: { filePath: string }) => diskContentByPath[filePath] ?? null, - requestClientToolCall: async () => { - throw new Error('should not apply blocked edit') + requestClientToolCall: async (clientToolCall: any) => { + followUpApplied = true + return confirmedMutationOutput(clientToolCall) }, writeToClient: () => {}, } as any) + expect(followUpApplied).toBe(true) const replaceOutput = strReplaceResult.output[0] - expect(fileProcessingState.editRereadRequirementsByPath?.[path]).toEqual({ - reason: 'application_rejected', - sourceTool: 'edit_transaction', - }) + expect(fileProcessingState.editRereadRequirementsByPath?.[path]).toBeUndefined() expect(replaceOutput.type).toBe('json') if (replaceOutput.type === 'json') { - expect(replaceOutput.value).toHaveProperty('errorMessage') - expect( - String((replaceOutput.value as { errorMessage?: string }).errorMessage), - ).toContain('previous edit_transaction application was rejected') + expect(replaceOutput.value).not.toHaveProperty('errorMessage') } }) @@ -3393,7 +3420,7 @@ describe('read_files edit-state recovery', () => { expect(fileProcessingState.failedEditRequiresReadByPath[path]).toBe(true) }) - it('replace_range preserves strict read authorization on success and only flags re-read after client errors', async () => { + it('replace_range preserves authorization on success but revokes it for stale client snapshots', async () => { const path = 'src/helper.ts' let diskContent = 'export const value = 1\n' const fileProcessingState = createFileProcessingState() @@ -3459,6 +3486,10 @@ describe('read_files edit-state recovery', () => { expect(errorResult.output[0]?.type).toBe('json') expect(fileProcessingState.failedEditRequiresReadByPath[path]).toBe(true) + expect( + fileProcessingState.editRereadRequirementsByPath?.[path], + ).toMatchObject({ reason: 'stale_snapshot' }) + expect(fileProcessingState.readAuthorizationsByPath?.[path]).toBeUndefined() }) it('strict replace_range rejects a legacy pathless expectedHash as authorization', async () => { 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 3bd3688893..5518747ef1 100644 --- a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts +++ b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts @@ -561,6 +561,33 @@ describe('tool validation error handling', () => { } }) + it('gives actionable recovery when get_build_targets receives no changed files', () => { + const result = parseRawToolCall({ + rawToolCall: { + toolName: 'get_build_targets', + toolCallId: 'get-build-targets-empty-files-tool-call-id', + input: { files: [] }, + }, + }) + + expect('error' in result).toBe(true) + if ('error' in result) { + expect(result.error).toContain('Raw validation issues:') + expect(result.error).toContain('"code": "too_small"') + expect(result.error).toContain( + '`files` must be a non-empty array of changed project-relative file paths', + ) + expect(result.error).toContain( + '{ "files": ["packages/agent-runtime/src/tools/tool-executor.ts"] }', + ) + expect(result.error).toContain( + 'do not call `get_build_targets`; skip build-target discovery until a concrete changed-file list exists', + ) + expect(result.formattedInput).toContain(':') + expect(result.formattedInput).toContain('"files": []') + } + }) + it('should parse stringified params for spawn_agents entries', () => { const result = parseRawToolCall({ rawToolCall: { @@ -735,6 +762,26 @@ describe('tool validation error handling', () => { } }) + it('rejects mis-braced serialized spawn payloads with field-placement guidance', () => { + const result = parseRawToolCall({ + rawToolCall: { + toolName: 'spawn_agents', + toolCallId: 'spawn-agents-misbraced-tool-call-id', + input: + '{"agents":[{"agent_type":"thinker"}],"prompt":"outside-agent"}', + }, + }) + + expect('error' in result).toBe(true) + if ('error' in result) { + expect(result.error).toContain( + '`prompt`, `params`, and `handoff` must be inside each agent object', + ) + expect(result.error).toContain('check every brace and bracket') + expect(result.error).toContain('ambiguous brace nesting') + } + }) + it('gives spawn-specific recovery for truncated agent JSON', () => { const result = parseRawToolCall({ rawToolCall: { @@ -1699,6 +1746,94 @@ describe('tool validation error handling', () => { expect(message).toContain('Preserve params field names exactly.') }) + it('gives reliability-reviewer snapshot_id recovery with its normalized ID', async () => { + const { validateAgentInput } = + await import('../tools/handlers/tool/spawn-agent-utils') + const reliabilityReviewer = { + ...testAgentTemplate, + id: 'reliability-reviewer', + inputSchema: { + params: z.object({ snapshot_id: z.string() }), + }, + } + + let message = '' + try { + validateAgentInput( + reliabilityReviewer, + 'reliability-reviewer', + undefined, + {}, + ) + } catch (error) { + message = error instanceof Error ? error.message : String(error) + } + + expect(message).toContain('Missing required: snapshot_id') + expect(message).toContain('params.snapshot_id') + expect(message).toContain('"agent_type": "reliability-reviewer"') + expect(message).toContain('"snapshot_id": ""') + expect(message).toContain('exact current snapshot fingerprint') + expect(message).toContain('get_change_review_bundle') + }) + + it('gives dependency-manager canonical manager and operation recovery', async () => { + const { validateAgentInput } = + await import('../tools/handlers/tool/spawn-agent-utils') + const dependencyManager = { + ...testAgentTemplate, + id: 'dependency-manager', + inputSchema: { + params: z.object({ + manager: z.string(), + operation: z.enum(['add', 'remove', 'sync', 'restore', 'update']), + }), + }, + } + + let message = '' + try { + validateAgentInput(dependencyManager, 'dependency-manager', undefined, {}) + } catch (error) { + message = error instanceof Error ? error.message : String(error) + } + + expect(message).toContain('manager:') + expect(message).toContain('operation:') + expect(message).toContain('"required":["manager","operation"]') + expect(message).toContain('"manager": "npm"') + expect(message).toContain('"operation": "add"') + expect(message).toContain('repository manifest/environment evidence') + expect(message).toContain('add, remove, sync, restore, or update') + expect(message).toContain( + 'Do not infer dependency mutation authorization from a validation failure.', + ) + }) + + it('gives librarian canonical repoUrl recovery on empty params', async () => { + const { validateAgentInput } = + await import('../tools/handlers/tool/spawn-agent-utils') + const librarian = { + ...testAgentTemplate, + id: 'librarian', + inputSchema: { + params: z.object({ repoUrl: z.string().url() }), + }, + } + + let message = '' + try { + validateAgentInput(librarian, 'librarian', undefined, {}) + } catch (error) { + message = error instanceof Error ? error.message : String(error) + } + + expect(message).toContain('Missing required: repoUrl') + expect(message).toContain('params.repoUrl') + expect(message).toContain('"repoUrl": "https://github.com//"') + expect(message).toContain('a URL only in prompt prose is not used') + }) + it('rejects security-reviewer snapshot_id with canonical params recovery', async () => { const { validateAgentInput } = await import('../tools/handlers/tool/spawn-agent-utils') diff --git a/packages/agent-runtime/src/process-edit-transaction.ts b/packages/agent-runtime/src/process-edit-transaction.ts index 51aad0f90a..4c84436550 100644 --- a/packages/agent-runtime/src/process-edit-transaction.ts +++ b/packages/agent-runtime/src/process-edit-transaction.ts @@ -105,6 +105,7 @@ export async function processEditTransaction(params: { { startLine: number; endLine: number; lineDelta: number }[] >() const failures: TransactionFailure[] = [] + const pathsWithNonRangeEdit = new Set() for (let editIndex = 0; editIndex < edits.length; editIndex++) { const edit = edits[editIndex] if (!edit) continue @@ -136,6 +137,25 @@ export async function processEditTransaction(params: { break } + // A replace_range authenticates against ORIGINAL-snapshot coordinates, but a + // prior non-replace_range content edit on the same path changes the working + // line count without being reflected in those coordinates. Rather than try to + // compute str_replace/structured/patch line deltas, block the replace_range so + // it can never silently splice the wrong lines while its capability hash check + // (validated against the original snapshot) still passes. + if ( + effectiveEdit.type === 'replace_range' && + pathsWithNonRangeEdit.has(effectiveEdit.path) + ) { + failures.push({ + editIndex, + ...(effectiveEdit.id && { id: effectiveEdit.id }), + path: effectiveEdit.path, + errorMessage: `replace_range blocked for ${effectiveEdit.path}: a prior non-replace_range edit changed this file earlier in the transaction, so replace_range's original-snapshot line coordinates can no longer be safely applied. Split this into a separate transaction or use str_replace/rewrite_symbol for this path.`, + }) + break + } + const currentContent = workingContentByPath.get(effectiveEdit.path) const result = await processTransactionEdit({ edit: rangeAdjustment.edit, @@ -167,6 +187,9 @@ export async function processEditTransaction(params: { } workingContentByPath.set(effectiveEdit.path, result.content) + if (rangeAdjustment.edit.type !== 'replace_range') { + pathsWithNonRangeEdit.add(effectiveEdit.path) + } if (rangeAdjustment.edit.type === 'replace_range') { const originalRange = rangeAdjustment.edit.originalRange ?? { startLine: rangeAdjustment.edit.startLine, @@ -196,7 +219,7 @@ export async function processEditTransaction(params: { tool: 'edit_transaction', error: [ `edit_transaction aborted during preflight at edit ${firstFailure.editIndex + 1} of ${edits.length}, so NO files were changed.`, - 'The detailed cause is listed once in failures below. Re-read only when that failure explicitly requires it, then retry the whole related transaction from one current snapshot.', + 'The detailed cause is listed once in failures below. If that failure requires fresh read/capability state, re-read EVERY target in this transaction in the current run, then rebuild and retry the whole transaction from that one current snapshot. Do not refresh only the first failed path while replaying stale tokens for the remaining targets.', ].join('\n\n'), failures, } @@ -356,21 +379,6 @@ async function processTransactionEdit(params: { switch (edit.type) { case 'str_replace': { const initialContent = await initialContentPromise - if (typeof initialContent === 'string') { - return processStrReplace({ - path: edit.path, - replacements: edit.replacements, - atomic: true, - transactionContext: true, - requireFreshReadCapability, - readCapabilityScope: readCapabilityIssuer - ? { ...readCapabilityIssuer, path: edit.path } - : undefined, - initialContentPromise: Promise.resolve(initialContent), - logger, - }) - } - return processStrReplace({ path: edit.path, replacements: edit.replacements, @@ -409,11 +417,21 @@ async function processTransactionEdit(params: { : lines.at(-1) === '' ? lines.length - 1 : lines.length + const authorizationTarget = edit.originalRange ?? { + startLine: edit.startLine, + endLine: edit.endLine, + } { const decoded = decodeReadCapabilityToken(edit.readCapability) if (typeof decoded === 'string') { return { error: decoded } } + // Invariant: the runtime always supplies readCapabilityIssuer in + // production, so project/path/run scope is enforced by the scope check + // below. Do not hard-require the issuer here: non-runtime/test callers + // may omit it. When it is absent the capability metadata match plus the + // observed-content-hash freshness check still gate the edit, but + // cross-scope replay is not detectable in that case. const scope = readCapabilityIssuer ? { ...readCapabilityIssuer, path: edit.path } : undefined @@ -443,10 +461,6 @@ async function processTransactionEdit(params: { error: `replace_range blocked for ${edit.path}: the readCapability-covered content is stale. Re-read lines ${edit.capabilityStartLine}-${edit.capabilityEndLine} and retry with the fresh token.`, } } - const authorizationTarget = edit.originalRange ?? { - startLine: edit.startLine, - endLine: edit.endLine, - } if ( authorizationTarget.startLine < edit.capabilityStartLine || authorizationTarget.endLine > edit.capabilityEndLine @@ -465,13 +479,6 @@ async function processTransactionEdit(params: { error: `replace_range ${edit.startLine}-${edit.endLine} is outside ${edit.path} (${visibleLineCount} lines).`, } } - const currentRange = lines - .slice(edit.startLine - 1, edit.endLine) - .join('\n') - const authorizationTarget = edit.originalRange ?? { - startLine: edit.startLine, - endLine: edit.endLine, - } const narrowedTarget = authorizationTarget.startLine !== edit.capabilityStartLine || authorizationTarget.endLine !== edit.capabilityEndLine diff --git a/packages/agent-runtime/src/process-str-replace.ts b/packages/agent-runtime/src/process-str-replace.ts index 72170d725a..9270408fdf 100644 --- a/packages/agent-runtime/src/process-str-replace.ts +++ b/packages/agent-runtime/src/process-str-replace.ts @@ -95,6 +95,16 @@ const FAILED_EDIT_INLINE_RECOVERY_GUIDANCE = [ ].join('\n') function addFailedEditRecoveryGuidance(error: string): string { + // Scope mismatches are authenticity failures, not evidence that file content + // changed or disappeared. Keep their recovery precise while preserving the + // cap.v3 project/path/run anti-replay boundary. + if ( + error.includes( + 'read capability belongs to a different project, path, or agent run', + ) + ) { + return error + } return `${error}\n\n${ error.includes('Recovery capability for candidate 1:') ? FAILED_EDIT_INLINE_RECOVERY_GUIDANCE @@ -180,6 +190,7 @@ export async function processStrReplace(params: { const validatedReadRanges = new Map() const readCapabilityWarnings: string[] = [] const preflightErrors: string[] = [] + const capabilityAuthorityErrors: string[] = [] let hadNoOpSkip = false // Decode any token-form basedOnRead up front so the rest of the pipeline only @@ -207,7 +218,7 @@ export async function processStrReplace(params: { requireBoundCapability: requireFreshReadCapability, }) if (authorityError) { - preflightErrors.push( + capabilityAuthorityErrors.push( `Invalid basedOnRead for replacement ${i + 1}: ${authorityError}`, ) } @@ -226,6 +237,14 @@ export async function processStrReplace(params: { } } + if (capabilityAuthorityErrors.length > 0) { + return { + tool: 'str_replace' as const, + path, + error: capabilityAuthorityErrors.join('\n\n'), + } + } + // Reject obviously-bogus string anchors (stubs like "dummy", or anything that // does not decode) on EVERY file, large or small. This is the only basedOnRead // check that runs on small files; valid "cap...." tokens decode to objects and diff --git a/packages/agent-runtime/src/tools/handlers/tool/__tests__/edit-application-coordinator.test.ts b/packages/agent-runtime/src/tools/handlers/tool/__tests__/edit-application-coordinator.test.ts index f0f7bd84ea..437208eadf 100644 --- a/packages/agent-runtime/src/tools/handlers/tool/__tests__/edit-application-coordinator.test.ts +++ b/packages/agent-runtime/src/tools/handlers/tool/__tests__/edit-application-coordinator.test.ts @@ -108,6 +108,73 @@ describe('edit application coordinator', () => { ) }) + it('confirms an applied transaction when a no-op path is excluded from confirmationPaths', async () => { + const clientOutput = [ + { + type: 'json', + value: { + kind: 'file_mutation_result', + version: 1, + operationId: 'operation', + outcome: 'applied', + actions: [ + { + actionId: 'operation:0', + index: 0, + action: 'update', + path: 'a.ts', + outcome: 'applied', + beforeHash: 'before', + afterHash: 'after', + }, + ], + authorityTier: 'portable_path', + receiptId: 'operation', + errors: [], + freshCapabilities: [], + }, + }, + ] as any + + // b.ts resolved to a no-op and was excluded from client changes, so the + // client never emits an `applied` action for it. Scoping confirmation to + // only the paths that actually changed lets the transaction be confirmed. + const state = getFileProcessingValues({ + promisesByPath: { 'a.ts': [], 'b.ts': [] }, + }) + let committed = false + const result = await coordinateEditApplication({ + toolName: 'edit_transaction', + fileProcessingState: state, + paths: ['a.ts', 'b.ts'], + confirmationPaths: ['a.ts'], + rejectionRequiresRead: false, + apply: async () => clientOutput, + onApplied: () => { + committed = true + }, + }) + + expect(result.status).toBe('applied') + expect(committed).toBe(true) + + // Without confirmationPaths the default requires BOTH paths, so the same + // output is wrongly reported as rejected — this locks in that the scoping + // is what fixes the false-negative. + const defaultState = getFileProcessingValues({ + promisesByPath: { 'a.ts': [], 'b.ts': [] }, + }) + const defaultResult = await coordinateEditApplication({ + toolName: 'edit_transaction', + fileProcessingState: defaultState, + paths: ['a.ts', 'b.ts'], + rejectionRequiresRead: false, + apply: async () => clientOutput, + }) + + expect(defaultResult.status).toBe('rejected') + }) + it('drops syntax-rejected prepared state without revoking fresh authorization', async () => { const state = getFileProcessingValues({ promisesByPath: { 'a.ts': [] }, diff --git a/packages/agent-runtime/src/tools/handlers/tool/edit-application-coordinator.ts b/packages/agent-runtime/src/tools/handlers/tool/edit-application-coordinator.ts index d55a0979fe..105577a1a4 100644 --- a/packages/agent-runtime/src/tools/handlers/tool/edit-application-coordinator.ts +++ b/packages/agent-runtime/src/tools/handlers/tool/edit-application-coordinator.ts @@ -118,6 +118,41 @@ export function editOutputHasError( return hasExplicitError(output) } +function outputIndicatesStaleSnapshot(value: unknown, depth = 0): boolean { + if (depth > 6 || value === null || value === undefined) return false + if (Array.isArray(value)) { + return value.some((item) => outputIndicatesStaleSnapshot(item, depth + 1)) + } + if (typeof value === 'string') { + return /(?:stale\s+(?:snapshot|hash|range)|content\s+(?:changed|mismatch)|expected\s+hash)/i.test( + value, + ) + } + if (typeof value !== 'object') return false + return Object.values(value as Record).some((nested) => + outputIndicatesStaleSnapshot(nested, depth + 1), + ) +} + +function outputIndicatesUnconfirmedApplication( + value: unknown, + depth = 0, +): boolean { + if (depth > 6 || value === null || value === undefined) return false + if (Array.isArray(value)) { + return value.some((item) => + outputIndicatesUnconfirmedApplication(item, depth + 1), + ) + } + if (typeof value === 'string') { + return /could not confirm/i.test(value) + } + if (typeof value !== 'object') return false + return Object.values(value as Record).some((nested) => + outputIndicatesUnconfirmedApplication(nested, depth + 1), + ) +} + export function invalidatePreparedEditPaths(params: { fileProcessingState: FileProcessingState paths: Iterable @@ -177,11 +212,16 @@ export async function coordinateEditApplication(params: { fileProcessingState: FileProcessingState paths: Iterable apply: () => Promise> - rejectionRequiresRead?: boolean wholeFileContentByPath?: ReadonlyMap onApplied?: () => void + rejectionRequiresRead?: boolean + confirmationPaths?: Iterable }): Promise> { const paths = [...new Set(params.paths)] + const confirmationPaths = + params.confirmationPaths === undefined + ? new Set(paths) + : new Set(params.confirmationPaths) let output: CodebuffToolOutput try { output = await params.apply() @@ -209,22 +249,39 @@ export async function coordinateEditApplication(params: { } if (editOutputHasError(output)) { - if (params.rejectionRequiresRead !== false) { + if (outputIndicatesStaleSnapshot(output)) { invalidatePreparedEditPaths({ fileProcessingState: params.fileProcessingState, paths, - reason: 'application_rejected', + reason: 'stale_snapshot', + sourceTool: params.toolName, + }) + } else if (outputIndicatesUnconfirmedApplication(output)) { + // An unconfirmed-application wrapper (e.g. an empty client result wrapped + // into a 'could not confirm' error) may reflect a partial write, so it + // always requires a re-read regardless of `rejectionRequiresRead`. + invalidatePreparedEditPaths({ + fileProcessingState: params.fileProcessingState, + paths, + reason: 'application_unconfirmed', sourceTool: params.toolName, }) } else { - for (const path of paths) { - delete params.fileProcessingState.promisesByPath[path] - } + // An explicit client rejection confirms that no prepared mutation was + // applied. Drop speculative prepared state, but preserve valid read + // authorization; unlike unconfirmed output or a throw, this outcome is + // deterministic and does not require a re-read. + invalidatePreparedEditPaths({ + fileProcessingState: params.fileProcessingState, + paths, + revokeReadAuthorization: params.rejectionRequiresRead ?? true, + requiresFreshRead: params.rejectionRequiresRead ?? true, + }) } return { status: 'rejected', output } } - if (!hasPositiveApplicationEvidence(output, new Set(paths))) { + if (!hasPositiveApplicationEvidence(output, confirmationPaths)) { invalidatePreparedEditPaths({ fileProcessingState: params.fileProcessingState, paths, diff --git a/packages/agent-runtime/src/tools/handlers/tool/edit-transaction.ts b/packages/agent-runtime/src/tools/handlers/tool/edit-transaction.ts index 2da12d2df1..25bc257bb1 100644 --- a/packages/agent-runtime/src/tools/handlers/tool/edit-transaction.ts +++ b/packages/agent-runtime/src/tools/handlers/tool/edit-transaction.ts @@ -394,27 +394,25 @@ export const handleEditTransaction = (async ( } if ('error' in transactionResult) { - const failedPaths = new Set( - transactionResult.failures.length > 0 - ? transactionResult.failures.map((failure) => failure.path) - : uniquePaths, - ) - const preserveAuthorizedPaths = [...failedPaths].filter((path) => - freshWholeFileAuthorizationPaths.has(path), - ) - const requireFreshReadPaths = [...failedPaths].filter( - (path) => !freshWholeFileAuthorizationPaths.has(path), - ) - invalidatePreparedEditPaths({ - fileProcessingState, - paths: preserveAuthorizedPaths, - requiresFreshRead: false, - }) + const failureText = transactionResult.failures + .map((failure) => failure.errorMessage) + .join('\n') + const requiresFreshCapability = + /different project, path, or agent run|Invalid basedOnRead|readCapability-covered content is stale|normalized capability metadata does not match/i.test( + failureText, + ) + // Deterministic preflight failures made no writes and preserve existing + // authorization. Capability freshness/scope failures are different: retrying + // the atomic transaction requires one new snapshot for every target so no + // stale token from another path is replayed alongside the refreshed failure. invalidatePreparedEditPaths({ fileProcessingState, - paths: requireFreshReadPaths, - reason: 'preflight_failed', - sourceTool: 'edit_transaction', + paths: uniquePaths, + revokeReadAuthorization: requiresFreshCapability, + requiresFreshRead: requiresFreshCapability, + ...(requiresFreshCapability + ? { reason: 'stale_capability' as const, sourceTool: 'edit_transaction' } + : {}), }) return { @@ -422,7 +420,12 @@ export const handleEditTransaction = (async ( { type: 'json', value: { - errorMessage: transactionResult.error, + errorMessage: requiresFreshCapability + ? [ + transactionResult.error, + `Atomic recovery requires fresh read state for every transaction target in this run: ${uniquePaths.join(', ')}. Re-read all targets and rebuild the complete transaction with only those fresh capabilities; do not refresh only the first failed path or replay any other stale token.`, + ].join('\n') + : transactionResult.error, failures: transactionResult.failures, }, }, @@ -563,6 +566,18 @@ export const handleEditTransaction = (async ( }) clientChanges.sort((a, b) => a.index - b.index) + // Only paths that produced an actual client change can emit an `applied` + // action, so scope the positive-evidence confirmation set to those paths. + // A content edit that resolved to a no-op is excluded from `clientChanges` + // and must not be required for confirmation. + const confirmationPaths = new Set() + for (const { change } of clientChanges) { + confirmationPaths.add(change.path) + if (change.type === 'move' && typeof change.destinationPath === 'string') { + confirmationPaths.add(change.destinationPath) + } + } + const appliedFiles: { path: string patch: string @@ -572,6 +587,8 @@ export const handleEditTransaction = (async ( toolName: 'edit_transaction', fileProcessingState, paths: uniquePaths, + confirmationPaths, + rejectionRequiresRead: false, apply: () => requestClientToolCall({ toolCallId: toolCall.toolCallId, diff --git a/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-utils.ts b/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-utils.ts index 7966d3208e..8a73d40a38 100644 --- a/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-utils.ts +++ b/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-utils.ts @@ -1622,6 +1622,17 @@ export function validateAgentInput( ), ) const normalizedAgentType = normalizeAgentIdForLookup(agentType) + const reviewerFamilyRequiredSnapshotIds = new Set([ + 'product-reviewer', + 'performance-specialist', + 'reliability-reviewer', + 'migration-reviewer', + 'compatibility-reviewer', + 'accessibility-reviewer', + 'ux-visual-reviewer', + 'dependency-reviewer', + 'evaluator', + ]) const paramsRecord = params && typeof params === 'object' && !Array.isArray(params) ? (params as Record) @@ -1629,17 +1640,23 @@ export function validateAgentInput( const recoveryHint = normalizedAgentType === 'basher' && issuePaths.has('command') ? '\n\nRecovery: spawn Basher with { "agent_type": "basher", "params": { "command": "" } }. A command mentioned only in prompt prose is never executed.' - : normalizedAgentType === 'compatibility-reviewer' && + : reviewerFamilyRequiredSnapshotIds.has(normalizedAgentType) && issuePaths.has('snapshot_id') - ? '\n\nRecovery: set params.snapshot_id to the exact current snapshot fingerprint from get_change_review_bundle, for example { "agent_type": "compatibility-reviewer", "params": { "snapshot_id": "" } }. Do not invent or reuse a stale fingerprint.' + ? `\n\nRecovery: set params.snapshot_id to the exact current snapshot fingerprint from get_change_review_bundle, for example { "agent_type": "${normalizedAgentType}", "params": { "snapshot_id": "" } }. Do not invent or reuse a stale fingerprint.` : normalizedAgentType === 'security-reviewer' && (issuePaths.has('snapshot_fingerprint') || Object.hasOwn(paramsRecord ?? {}, 'snapshot_id')) ? '\n\nRecovery: replace params.snapshot_id with params.snapshot_fingerprint, or add params.snapshot_fingerprint when it is missing. Retain params.changed_files and preserve both canonical field names exactly.' - : normalizedAgentType === 'code-searcher' && - issuePaths.has('searchQueries') - ? '\n\nRecovery: spawn code-searcher with { "agent_type": "code-searcher", "params": { "searchQueries": [{ "pattern": "", "flags": "-g *.ts" }] } }. searchQueries is a required array of objects each with a non-empty string "pattern"; queries mentioned only in prompt prose are never executed.' - : '' + : normalizedAgentType === 'dependency-manager' && + (issuePaths.has('manager') || issuePaths.has('operation')) + ? '\n\nRecovery: place both canonical keys in params, for example { "agent_type": "dependency-manager", "params": { "manager": "npm", "operation": "add" } }. manager must come from repository manifest/environment evidence. operation must be one of add, remove, sync, restore, or update. Do not infer dependency mutation authorization from a validation failure.' + : normalizedAgentType === 'librarian' && + issuePaths.has('repoUrl') + ? '\n\nRecovery: set params.repoUrl to a GitHub URL, for example { "agent_type": "librarian", "params": { "repoUrl": "https://github.com//" } }. params.repoUrl must have the form https://github.com//; a URL only in prompt prose is not used.' + : normalizedAgentType === 'code-searcher' && + issuePaths.has('searchQueries') + ? '\n\nRecovery: spawn code-searcher with { "agent_type": "code-searcher", "params": { "searchQueries": [{ "pattern": "", "flags": "-g *.ts" }] } }. searchQueries is a required array of objects each with a non-empty string "pattern"; queries mentioned only in prompt prose are never executed.' + : '' const paramsContract = formatAgentParamsContract(inputSchema.params) throw new Error( `Invalid params for agent ${agentType}: ${formatValidationIssues({ issues: result.error.issues })}\n\nExact params contract (from the child agent schema): ${paramsContract}\nPreserve params field names exactly.${recoveryHint}\n\nOriginal params value:\n${formatValueForError(params ?? {})}`, diff --git a/packages/agent-runtime/src/tools/handlers/tool/str-replace.ts b/packages/agent-runtime/src/tools/handlers/tool/str-replace.ts index 3cc967ab6f..2360feb389 100644 --- a/packages/agent-runtime/src/tools/handlers/tool/str-replace.ts +++ b/packages/agent-runtime/src/tools/handlers/tool/str-replace.ts @@ -327,14 +327,20 @@ export const handleStrReplace = (async ( // the agent only needs to fix the syntax, not re-read the file or switch // tools. (Fix C circuit breaker only counts real processing failures.) if (!strReplaceResult.preflightSyntaxError) { - if (!hadFreshWholeFileAuthorization) { + const requiresFreshCapability = + /(?:readCapability|basedOnRead).*(?:stale|different project, path, or agent run)|(?:stale|different project, path, or agent run).*(?:readCapability|basedOnRead)/is.test( + strReplaceResult.error, + ) + if (requiresFreshCapability) { markEditRequiresFreshRead({ fileProcessingState, path, - reason: 'preflight_failed', + reason: 'stale_capability', sourceTool: 'str_replace', }) } + // Deterministic no-match/ambiguity preflight failures do not mutate the + // client and therefore preserve any valid read authorization. // Fix C: a hard error consumes the per-path failure budget. fileProcessingState.consecutiveStrReplaceFailuresByPath[path] = (fileProcessingState.consecutiveStrReplaceFailuresByPath[path] ?? 0) + 1 @@ -372,11 +378,10 @@ export const handleStrReplace = (async ( toolName: 'str_replace', fileProcessingState, paths: [path], - // Processing/preflight failures never reached the client. Preserve a - // verified whole-file authorization; scoped/stale paths were explicitly - // marked above. Client-side rejection after a prepared edit still requires - // a fresh read through the coordinator's default path. - rejectionRequiresRead: !('error' in strReplaceResult), + rejectionRequiresRead: false, + // Deterministic processing failures and explicit client rejections preserve + // valid read authorization. Stale capability and uncertain application + // outcomes are marked separately and still require a fresh read. wholeFileContentByPath: 'content' in strReplaceResult ? new Map([[path, strReplaceResult.content]]) diff --git a/packages/agent-runtime/src/tools/handlers/tool/write-file.ts b/packages/agent-runtime/src/tools/handlers/tool/write-file.ts index febc87ced0..8812497123 100644 --- a/packages/agent-runtime/src/tools/handlers/tool/write-file.ts +++ b/packages/agent-runtime/src/tools/handlers/tool/write-file.ts @@ -462,7 +462,7 @@ export const handleWriteFile = (async ( toolName: 'write_file', fileProcessingState, paths: [path], - rejectionRequiresRead: !('error' in writeFileResult), + rejectionRequiresRead: false, wholeFileContentByPath: 'content' in writeFileResult ? new Map([[path, writeFileResult.content]]) diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index f28a3612f3..297e0f27c0 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -705,6 +705,22 @@ function getToolValidationHint( ): string | undefined { const fieldHint = issues ? getFieldSpecificHint(toolName, issues) : undefined + if ( + toolName === 'get_build_targets' && + (issues ?? []).some( + (issue) => + issue.code === 'too_small' && + issue.path?.length === 1 && + issue.path[0] === 'files', + ) + ) { + return [ + '`files` must be a non-empty array of changed project-relative file paths.', + 'Example: { "files": ["packages/agent-runtime/src/tools/tool-executor.ts"] }', + 'When there are no changed files, do not call `get_build_targets`; skip build-target discovery until a concrete changed-file list exists.', + ].join('\n') + } + if (toolName === 'str_replace') { const base = [ 'Expected shape: { "path": string, "replacements": [{ "oldString": string, "newString": string, "allowMultiple"?: boolean }] }.', @@ -732,7 +748,7 @@ function getToolValidationHint( if (toolName === 'spawn_agents') { const base = [ 'Expected shape: { "agents": [{ "agent_type": string, "prompt"?: string, "params"?: object, "handoff"?: object }] }.', - 'Pass agents as an array of objects. Valid stringified or double-stringified JSON is repaired automatically, but truncated JSON and non-object entries are rejected. Do not stringify each agent entry.', + 'Pass agents as an array of objects. `prompt`, `params`, and `handoff` must be inside each agent object; check every brace and bracket when a field appears misplaced. Valid stringified or double-stringified JSON is repaired automatically, but ambiguous brace nesting, truncated JSON, and non-object entries are rejected without guessing or auto-repair. Do not stringify each agent entry.', ].join('\n') const hasHandoffIssue = (issues ?? []).some(isSpawnAgentHandoffIssue) if (!hasHandoffIssue) return base From 1b29f63f058e6c23b707dccfeda682e210613636 Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Thu, 23 Jul 2026 19:32:44 +0300 Subject: [PATCH 03/11] Reject mis-braced spawn_agents payloads with field-placement guidance Detect a top-level spawn_agents payload where prompt/params/handoff appear as siblings of the agents array, and return the base spawn_agents guidance before the non-strict schema silently drops the stray key. --- .../agent-runtime/src/tools/tool-executor.ts | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index 297e0f27c0..b9bfbda943 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -890,6 +890,37 @@ function formatInvalidInputExcerpts( return excerpts.join('\n\n') || formatValueForError(input, 2_000) } +// Reject a mis-braced spawn_agents payload where `prompt`, `params`, or +// `handoff` appear as siblings of `agents` at the top level. These fields +// belong INSIDE each agent object; the non-strict top-level schema would +// otherwise silently drop the stray key, hiding the mistake. Detect it before +// safeParse strips the sibling and surface the base spawn_agents guidance. +function detectMisbracedSpawnPayload(args: { + input: unknown + toolCallId: string + rawInput: unknown +}): ToolCallError | undefined { + const { input, toolCallId, rawInput } = args + if (!input || typeof input !== 'object' || Array.isArray(input)) { + return undefined + } + const record = input as Record + if (!Array.isArray(record.agents)) return undefined + const misplacedKeys = ['prompt', 'params', 'handoff'].filter((key) => + Object.hasOwn(record, key), + ) + if (misplacedKeys.length === 0) return undefined + const hint = getToolValidationHint('spawn_agents', undefined, input) + const summary = `misplaced top-level field(s) ${misplacedKeys.join(', ')} alongside \`agents\`` + return { + toolName: 'spawn_agents', + toolCallId, + input: rawInput, + error: `Invalid parameters for spawn_agents: ${summary}.${hint ? `\n\n${hint}` : ''}`, + formattedInput: formatInvalidInputExcerpts(input, []), + } +} + function isFileChangingTool(toolName: string): boolean { return ( toolName === 'apply_patch' || @@ -1068,6 +1099,16 @@ export function parseRawToolCall(params: { toolName, repairSetOutputData(toolName, processedParameters.input), ) + if (toolName === 'spawn_agents') { + const misbracedError = detectMisbracedSpawnPayload({ + input: repairedInput, + toolCallId: rawToolCall.toolCallId, + rawInput: rawToolCall.input, + }) + if (misbracedError) { + return misbracedError + } + } const result = paramsSchema.safeParse(repairedInput) if (!result.success) { From b2a2e7b3ae6410544a5c891c380a505961bfe7fb Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Thu, 23 Jul 2026 20:43:10 +0300 Subject: [PATCH 04/11] Fix base2 e2e prompts and rewrite-symbol assertion; add classifier test Align two base2 reviewer-spawn e2e prompts to include a code-intent keyword so proactive query_index fires as the first yield. Update the rewrite-symbol client-rejection assertion to match the branch's deterministic-rejection semantics (rejection preserves read authorization). Add a base2 test pinning which prompts trigger proactive query_index vs skip to git_status. --- agents/__tests__/base2.test.ts | 33 +++++++++++++++++++ .../e2e/reviewer-spawn-conditions.e2e.test.ts | 4 +-- .../src/__tests__/rewrite-symbol.test.ts | 4 ++- 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/agents/__tests__/base2.test.ts b/agents/__tests__/base2.test.ts index 8e384d5879..12a489d2dd 100644 --- a/agents/__tests__/base2.test.ts +++ b/agents/__tests__/base2.test.ts @@ -4684,4 +4684,37 @@ describe('base2 test-writer aux-gate completion path', () => { expect(nextYield.input.agent_type).not.toBe('test-writer') } }) + + test('proactive query_index fires only for code-intent prompts', () => { + const firstYield = (prompt: string) => { + const base2 = createBase2('default') + const gen = base2.handleSteps!({ + agentState: { agentId: 'base2-classify' }, + prompt, + params: {}, + config: base2.programmaticConfig, + } as any) + return gen.next().value as any + } + + // A code-intent prompt with no concrete file path triggers a proactive + // query_index (mode: 'search') as the very first step. + expect( + firstYield('Refactor the authentication module code.'), + ).toMatchObject({ toolName: 'query_index', input: { mode: 'search' } }) + + // A prompt naming a concrete file path already identifies the relevant + // file, so proactive retrieval is skipped and the turn starts at git_status. + expect(firstYield('Update src/app.ts with the new export')).toMatchObject({ + toolName: 'git_status', + }) + + // Too-short prompts skip proactive retrieval. + expect(firstYield('fix it')).toMatchObject({ toolName: 'git_status' }) + + // Continuation prompts skip proactive retrieval. + expect( + firstYield('continue working on the previous task'), + ).toMatchObject({ toolName: 'git_status' }) + }) }) diff --git a/agents/e2e/reviewer-spawn-conditions.e2e.test.ts b/agents/e2e/reviewer-spawn-conditions.e2e.test.ts index 028bb7909c..db32fe14d4 100644 --- a/agents/e2e/reviewer-spawn-conditions.e2e.test.ts +++ b/agents/e2e/reviewer-spawn-conditions.e2e.test.ts @@ -142,7 +142,7 @@ describe('base2 reviewer spawn conditions e2e', () => { const agentState = { agentId: 'base2-plan' } const gen = base2.handleSteps!({ agentState, - prompt: 'Plan the lifecycle change.', + prompt: 'Plan the lifecycle change in the code.', params: {}, config: base2.programmaticConfig, } as any) @@ -174,7 +174,7 @@ describe('base2 reviewer spawn conditions e2e', () => { const base2 = createBase2('default', { executePlan: true }) const gen = base2.handleSteps!({ agentState: { agentId: 'base2-execute-plan' }, - prompt: 'Execute the lifecycle change.', + prompt: 'Execute the lifecycle change in the code.', params: {}, config: base2.programmaticConfig, } as any) diff --git a/packages/agent-runtime/src/__tests__/rewrite-symbol.test.ts b/packages/agent-runtime/src/__tests__/rewrite-symbol.test.ts index 28316e3571..d746aed61a 100644 --- a/packages/agent-runtime/src/__tests__/rewrite-symbol.test.ts +++ b/packages/agent-runtime/src/__tests__/rewrite-symbol.test.ts @@ -205,7 +205,9 @@ describe('rewrite_symbol handler', () => { expect(outputJson(result).errorMessage).toContain('client rejected patch') expect(state.consecutiveStrReplaceFailuresByPath['svc.ts']).toBe(3) - expect(state.failedEditRequiresReadByPath['svc.ts']).toBe(true) + // An explicit client rejection is deterministic: no prepared mutation was + // applied, so it preserves read authorization and does not force a re-read. + expect(state.failedEditRequiresReadByPath['svc.ts']).toBeUndefined() }) test('errors clearly when the symbol is not found', async () => { From 04b55fa27c77769ad654b2c970c750306be65269 Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Thu, 23 Jul 2026 21:06:16 +0300 Subject: [PATCH 05/11] Harden structural-receipt traversal for shared nodes beyond the depth cap Track the shallowest depth each object was visited at (WeakMap instead of WeakSet) so a shared receipt first reached beyond MAX_TRAVERSAL_DEPTH via a deep path is still found through a later shorter path; equal-or-deeper revisits and cycles are still skipped. Adds a regression test. --- .../src/util/__tests__/audit-receipt.test.ts | 25 +++++++++++++++++++ common/src/util/audit-receipt.ts | 12 ++++++--- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/common/src/util/__tests__/audit-receipt.test.ts b/common/src/util/__tests__/audit-receipt.test.ts index 4ade257c0f..d1b561a601 100644 --- a/common/src/util/__tests__/audit-receipt.test.ts +++ b/common/src/util/__tests__/audit-receipt.test.ts @@ -76,6 +76,31 @@ describe('containsStructuralAuditReceipt', () => { expect(containsStructuralAuditReceipt(undefined, 'snapshot-1')).toBe(false) }) + it('finds a receipt via a shorter path even after the shared node was first reached deeper', () => { + // Shared node holds the receipt a few levels below it. + const shared: Record = { + level1: { level2: { structuralReceipt: { snapshot_id: 'deep-shared' } } }, + } + + // Build a deep wrapper chain (well over MAX_TRAVERSAL_DEPTH = 32 levels) + // whose innermost node points at `shared`. From this side the receipt sits + // beyond the depth budget, so this path alone cannot reach it. + let deepChain: Record = { shared } + for (let depth = 0; depth < 40; depth += 1) { + deepChain = { nested: deepChain } + } + + // Deep chain is the FIRST key so Object.values visits it first, recording + // `shared` at a deep depth; the short path is a LATER key that re-reaches + // `shared` with a much shallower depth and enough budget to find it. + const root: Record = { + deep: deepChain, + short: shared, + } + + expect(containsStructuralAuditReceipt(root, 'deep-shared')).toBe(true) + }) + it('returns false when structuralReceipt exists but snapshot_id is missing or non-string', () => { expect( containsStructuralAuditReceipt({ structuralReceipt: {} }), diff --git a/common/src/util/audit-receipt.ts b/common/src/util/audit-receipt.ts index 7a30096f84..56715089ef 100644 --- a/common/src/util/audit-receipt.ts +++ b/common/src/util/audit-receipt.ts @@ -5,7 +5,10 @@ const MAX_TRAVERSAL_DEPTH = 32 * object whose `snapshot_id` matches `expectedSnapshotId`. When * `expectedSnapshotId` is empty/undefined, any structuralReceipt with a string * `snapshot_id` counts as a match. Depth-bounded (<=32) to stay conservative - * on deeply nested inputs, with visited-object tracking for cyclic/shared graphs. + * on deeply nested inputs, with min-depth revisit tracking for cyclic/shared + * graphs: each object records the shallowest depth it was reached at, so a + * later shorter path (which has more remaining depth budget) re-traverses, + * while equal-or-deeper revisits (including cycles) are skipped. * Shared by the general-agent handleSteps gate and the runtime * buildRuntimeAgentReceipt gate so the two do not drift. */ @@ -14,7 +17,7 @@ export function containsStructuralAuditReceipt( expectedSnapshotId?: string, ): boolean { let found = false - const visited = new WeakSet() + const visited = new WeakMap() const visit = (item: unknown, depth = 0): void => { if ( found || @@ -24,8 +27,9 @@ export function containsStructuralAuditReceipt( ) { return } - if (visited.has(item)) return - visited.add(item) + const existing = visited.get(item) + if (existing !== undefined && existing <= depth) return + visited.set(item, depth) if (Array.isArray(item)) { for (const nested of item) visit(nested, depth + 1) return From e53e8471fa2a7e66522f825f2ea90b740d2bb221 Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Thu, 23 Jul 2026 21:31:47 +0300 Subject: [PATCH 06/11] Reduce spawn_agents mis-brace and code_search misuse round-trips Fold a stray top-level prompt/params/handoff sibling into a single-agent spawn_agents array when unambiguous (multi-agent and unexpected-sibling cases still fail closed); add a corrected-shape example to the spawn_agents validation hint; emit a concrete copyable code-searcher spawn recovery for ungranted code_search/find_files_matching_content instead of the generic not-granted line (message-only, still fail-closed); and reinforce the base2 spawn guidance with the correct spawn_agents shape. Adds focused tests. --- agents/base2/base2.ts | 2 +- .../__tests__/tool-validation-error.test.ts | 100 +++++++++++++++++- .../agent-runtime/src/tools/tool-executor.ts | 88 ++++++++++++--- 3 files changed, 171 insertions(+), 19 deletions(-) diff --git a/agents/base2/base2.ts b/agents/base2/base2.ts index 0f5e52c802..132b023159 100644 --- a/agents/base2/base2.ts +++ b/agents/base2/base2.ts @@ -208,7 +208,7 @@ ${ ? '- **Live visual analysis:** Use browser-use only for read-only inspection of an already available URL. Do not start dev servers or request browser interactions in plan mode.' : '- **Live visual verification:** Visual verification extends beyond web apps. Image artifacts from 3D renders (e.g. Blender frames), image/video exports, generated diagrams, and charts must be inspected with read_image, not inferred from text logs alone. The workflow is: render/export -> poll the background job to completion -> read_image the emitted artifacts -> assess the result -> make a targeted edit -> re-render. Polling (check_job/check_background_agent/read_logs) is only the bridge to artifact inspection; do not re-poll a finished or unchanging job indefinitely. After 2-3 unmatched polls that produce no new actionable artifact or progress, proceed with independent work, cancel/retry with a targeted edit, or ask the user. For web app visual checks specifically, start any long-running dev server through a BACKGROUND basher, keep its returned jobId, use check_job to wait for readiness, then spawn browser-use for screenshots/navigation/interaction.' } -- **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. 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. +- **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. 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). # Code Editing Mandates 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 5518747ef1..6fb669e8cf 100644 --- a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts +++ b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts @@ -767,8 +767,11 @@ describe('tool validation error handling', () => { rawToolCall: { toolName: 'spawn_agents', toolCallId: 'spawn-agents-misbraced-tool-call-id', + // Two agent entries with a stray top-level `prompt`: it is ambiguous + // which agent the sibling belongs to, so this stays fail-closed and + // is not auto-repaired (unlike the single-agent fold case below). input: - '{"agents":[{"agent_type":"thinker"}],"prompt":"outside-agent"}', + '{"agents":[{"agent_type":"thinker"},{"agent_type":"file-picker"}],"prompt":"outside-agent"}', }, }) @@ -782,6 +785,86 @@ describe('tool validation error handling', () => { } }) + it('repairs a single-agent stray-sibling spawn payload by folding prompt into the entry', () => { + const result = parseRawToolCall({ + rawToolCall: { + toolName: 'spawn_agents', + toolCallId: 'spawn-agents-single-agent-fold-tool-call-id', + input: { + agents: [ + { + agent_type: 'code-searcher', + params: { searchQueries: [{ pattern: 'x' }] }, + }, + ], + prompt: 'find x', + }, + }, + }) + + expect('error' in result).toBe(false) + if (!('error' in result)) { + expect(result.input.agents[0].prompt).toBe('find x') + expect(result.input.agents[0].params).toEqual({ + searchQueries: [{ pattern: 'x' }], + }) + } + }) + + it('still rejects a multi-entry spawn array with stray sibling fields', () => { + const result = parseRawToolCall({ + rawToolCall: { + toolName: 'spawn_agents', + toolCallId: 'spawn-agents-multi-entry-stray-sibling-tool-call-id', + input: { + agents: [{ agent_type: 'a' }, { agent_type: 'b' }], + prompt: 'x', + }, + }, + }) + + expect('error' in result).toBe(true) + if ('error' in result) { + expect(result.error).toContain( + '`prompt`, `params`, and `handoff` must be inside each agent object', + ) + } + }) + + it('does not overwrite an existing in-entry prompt when folding a single-agent stray sibling', () => { + const result = parseRawToolCall({ + rawToolCall: { + toolName: 'spawn_agents', + toolCallId: 'spawn-agents-single-agent-no-overwrite-tool-call-id', + input: { + agents: [{ agent_type: 'code-searcher', prompt: 'inner' }], + prompt: 'outer', + }, + }, + }) + + expect('error' in result).toBe(false) + if (!('error' in result)) { + expect(result.input.agents[0].prompt).toBe('inner') + } + }) + + it('shows the corrected spawn shape inline when agents is an invalid JSON string', () => { + const result = parseRawToolCall({ + rawToolCall: { + toolName: 'spawn_agents', + toolCallId: 'spawn-agents-corrected-shape-hint-tool-call-id', + input: { agents: 'not valid json {' }, + }, + }) + + expect('error' in result).toBe(true) + if ('error' in result) { + expect(result.error).toContain('Corrected example:') + expect(result.error).toContain('INSIDE each agent object') + } + }) + it('gives spawn-specific recovery for truncated agent JSON', () => { const result = parseRawToolCall({ rawToolCall: { @@ -2457,7 +2540,7 @@ describe('tool validation error handling', () => { describe('buildUnavailableToolMessage', () => { it('explains a known-but-ungranted tool without suggesting a near match', () => { const message = buildUnavailableToolMessage({ - toolName: 'code_search', + toolName: 'run_terminal_command', agentId: 'base2', availableTools: ['query_index', 'read_files', 'glob'], }) @@ -2466,6 +2549,19 @@ describe('buildUnavailableToolMessage', () => { expect(message).toContain('is not available for agent `base2`') }) + it('gives concrete code-searcher recovery for ungranted content-search tools', () => { + for (const toolName of ['code_search', 'find_files_matching_content']) { + const message = buildUnavailableToolMessage({ + toolName, + agentId: 'base2', + availableTools: ['read_files'], + }) + + expect(message).toContain('code-searcher') + expect(message).toContain('searchQueries') + } + }) + 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 b9bfbda943..87cfd96efb 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -628,6 +628,13 @@ 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. + 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" }] } }.` + } 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.` } // Case 2: likely a typo/near-miss of a granted tool. @@ -749,6 +756,7 @@ function getToolValidationHint( const base = [ 'Expected shape: { "agents": [{ "agent_type": string, "prompt"?: string, "params"?: object, "handoff"?: object }] }.', 'Pass agents as an array of objects. `prompt`, `params`, and `handoff` must be inside each agent object; check every brace and bracket when a field appears misplaced. Valid stringified or double-stringified JSON is repaired automatically, but ambiguous brace nesting, truncated JSON, and non-object entries are rejected without guessing or auto-repair. Do not stringify each agent entry.', + 'Corrected example: { "agents": [{ "agent_type": "code-searcher", "prompt": "", "params": { "searchQueries": [{ "pattern": "" }] } }] } — note prompt/params live INSIDE each agent object, and agents is a real array, not a JSON string.', ].join('\n') const hasHandoffIssue = (issues ?? []).some(isSpawnAgentHandoffIssue) if (!hasHandoffIssue) return base @@ -890,34 +898,79 @@ function formatInvalidInputExcerpts( return excerpts.join('\n\n') || formatValueForError(input, 2_000) } -// Reject a mis-braced spawn_agents payload where `prompt`, `params`, or +// Handle a mis-braced spawn_agents payload where `prompt`, `params`, or // `handoff` appear as siblings of `agents` at the top level. These fields // belong INSIDE each agent object; the non-strict top-level schema would -// otherwise silently drop the stray key, hiding the mistake. Detect it before -// safeParse strips the sibling and surface the base spawn_agents guidance. +// otherwise silently drop the stray key, hiding the mistake. In the +// unambiguous single-agent case we fold the stray siblings into that one +// entry and return a repaired input for safeParse. Every ambiguous case +// (multi-entry arrays, unexpected sibling keys) still fails closed with the +// base spawn_agents guidance. +type DetectMisbracedSpawnResult = + | { repairedInput: Record } + | { error: ToolCallError } + function detectMisbracedSpawnPayload(args: { input: unknown toolCallId: string rawInput: unknown -}): ToolCallError | undefined { +}): DetectMisbracedSpawnResult | undefined { const { input, toolCallId, rawInput } = args if (!input || typeof input !== 'object' || Array.isArray(input)) { return undefined } const record = input as Record if (!Array.isArray(record.agents)) return undefined - const misplacedKeys = ['prompt', 'params', 'handoff'].filter((key) => - Object.hasOwn(record, key), - ) + const foldableKeys = ['prompt', 'params', 'handoff'] as const + const misplacedKeys = foldableKeys.filter((key) => Object.hasOwn(record, key)) if (misplacedKeys.length === 0) return undefined + + // Unambiguous single-agent repair: fold the stray prompt/params/handoff + // siblings INTO the single agent entry without overwriting existing in-entry + // values, then hand the corrected shape back for safeParse (which reruns the + // per-entry normalize preprocess). Never fold into a multi-entry array (it is + // ambiguous which agent the sibling belongs to). Any sibling key outside + // {prompt, params, handoff} other than the legitimate end-step param makes + // the intent ambiguous, so those fall through to the fail-closed error below. + const allowedSiblingKeys = new Set([ + ...foldableKeys, + endsAgentStepParam, + ]) + const hasUnexpectedSibling = Object.keys(record).some( + (key) => key !== 'agents' && !allowedSiblingKeys.has(key), + ) + if (record.agents.length === 1 && !hasUnexpectedSibling) { + const entry = parseJsonBounded(record.agents[0]) + if (entry !== null && typeof entry === 'object' && !Array.isArray(entry)) { + const entryRecord = entry as Record + const merged: Record = { ...entryRecord } + for (const key of foldableKeys) { + if (Object.hasOwn(record, key) && !Object.hasOwn(entryRecord, key)) { + merged[key] = record[key] + } + } + const repairedInput: Record = { agents: [merged] } + // Preserve any legitimate non-agent top-level key the schema still + // accepts (e.g. the end-step param); drop the folded stray siblings. + for (const [key, value] of Object.entries(record)) { + if (key === 'agents') continue + if ((foldableKeys as readonly string[]).includes(key)) continue + repairedInput[key] = value + } + return { repairedInput } + } + } + const hint = getToolValidationHint('spawn_agents', undefined, input) const summary = `misplaced top-level field(s) ${misplacedKeys.join(', ')} alongside \`agents\`` return { - toolName: 'spawn_agents', - toolCallId, - input: rawInput, - error: `Invalid parameters for spawn_agents: ${summary}.${hint ? `\n\n${hint}` : ''}`, - formattedInput: formatInvalidInputExcerpts(input, []), + error: { + toolName: 'spawn_agents', + toolCallId, + input: rawInput, + error: `Invalid parameters for spawn_agents: ${summary}.${hint ? `\n\n${hint}` : ''}`, + formattedInput: formatInvalidInputExcerpts(input, []), + }, } } @@ -1095,18 +1148,21 @@ export function parseRawToolCall(params: { ) } - const repairedInput = repairTerminalCommandScalars( + let repairedInput = repairTerminalCommandScalars( toolName, repairSetOutputData(toolName, processedParameters.input), ) if (toolName === 'spawn_agents') { - const misbracedError = detectMisbracedSpawnPayload({ + const misbraced = detectMisbracedSpawnPayload({ input: repairedInput, toolCallId: rawToolCall.toolCallId, rawInput: rawToolCall.input, }) - if (misbracedError) { - return misbracedError + if (misbraced) { + if ('error' in misbraced) { + return misbraced.error + } + repairedInput = misbraced.repairedInput } } const result = paramsSchema.safeParse(repairedInput) From 8ee6565af3b7a044ff2f69614ae9c2465b057c6f Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Thu, 23 Jul 2026 21:42:03 +0300 Subject: [PATCH 07/11] Add regression test for stringified mis-braced spawn_agents payload Reproduces the real-world failure where the whole agents value is a JSON string with a stray prompt pair floating as a sibling array element; asserts it fails closed with corrected-shape guidance. --- .../__tests__/tool-validation-error.test.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) 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 6fb669e8cf..f7b13f6163 100644 --- a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts +++ b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts @@ -865,6 +865,30 @@ describe('tool validation error handling', () => { } }) + it('rejects the original stringified agents array with a mis-braced prompt and points at the corrected shape', () => { + // Reproduces the exact real-world failure: the whole `agents` value was + // emitted as a JSON string, and inside it a stray `"prompt": "..."` pair + // floats as a sibling *array element* rather than a key inside the first + // agent object. That inner text is not valid JSON, so it cannot be + // auto-repaired and must fail closed with corrected-shape guidance. + const result = parseRawToolCall({ + rawToolCall: { + toolName: 'spawn_agents', + toolCallId: 'spawn-agents-stringified-misbraced-prompt-tool-call-id', + input: { + agents: + '[{"agent_type": "code-searcher", "params": {"searchQueries": [{"pattern": "serialized handleSteps", "flags": "-g *.ts"}]}}, "prompt": "Find the test in the agents test suite."}]', + }, + }, + }) + + expect('error' in result).toBe(true) + if ('error' in result) { + expect(result.error).toContain('Corrected example:') + expect(result.error).toContain('INSIDE each agent object') + } + }) + it('gives spawn-specific recovery for truncated agent JSON', () => { const result = parseRawToolCall({ rawToolCall: { From eed843bf24eccc91b90b8234d24ef44c9a1276ce Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Thu, 23 Jul 2026 21:55:16 +0300 Subject: [PATCH 08/11] Log mis-braced spawn_agents auto-repair and test the folded path Add a debug log on the single-agent mis-braced spawn_agents auto-repair path, recording only the folded sibling key names and toolCallId so prompt/params/handoff values are never leaked. Thread an optional logger through parseRawToolCall and detectMisbracedSpawnPayload from executeToolCall to enable it. Add a processStream integration test asserting the folded repair reaches the spawn handler: the repaired payload emits a spawn_agents tool_call and produces no error event. --- .../__tests__/tool-validation-error.test.ts | 69 +++++++++++++++++++ .../agent-runtime/src/tools/tool-executor.ts | 14 +++- 2 files changed, 81 insertions(+), 2 deletions(-) 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 f7b13f6163..abd0f2e5db 100644 --- a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts +++ b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts @@ -2088,6 +2088,75 @@ describe('tool validation error handling', () => { }) }) + it('repairs a single-agent mis-braced spawn payload and publishes it to the handler', async () => { + const parent: AgentTemplate = { + ...testAgentTemplate, + toolNames: ['spawn_agents', 'end_turn'], + spawnableAgents: ['basher'], + } + const basher: AgentTemplate = { + ...testAgentTemplate, + id: 'basher', + inputSchema: { params: z.object({ command: z.string().min(1) }) }, + toolNames: ['run_terminal_command'], + spawnableAgents: [], + } + const misbracedSpawn: StreamChunk = { + type: 'tool-call', + toolName: 'spawn_agents', + toolCallId: 'basher-misbraced-fold-tool-call-id', + input: { + agents: [{ agent_type: 'basher', params: { command: 'bun test' } }], + prompt: 'Run the tests', + }, + } + async function* mockStream() { + yield misbracedSpawn + return promptSuccess('mock-message-id') + } + const responseChunks: (string | PrintModeEvent)[] = [] + const sessionState = getInitialSessionState(mockFileContext) + + await processStream({ + ...agentRuntimeImpl, + agentContext: {}, + agentState: sessionState.mainAgentState, + agentStepId: 'test-step-id', + agentTemplate: parent, + ancestorRunIds: [], + clientSessionId: 'test-session', + fileContext: mockFileContext, + fingerprintId: 'test-fingerprint', + fullResponse: '', + localAgentTemplates: { 'test-agent': parent, basher }, + messages: [], + prompt: 'test prompt', + repoId: undefined, + repoUrl: undefined, + runId: 'test-run-id', + signal: new AbortController().signal, + stream: mockStream(), + system: 'test system', + tools: {}, + userId: 'test-user', + userInputId: 'test-input-id', + onCostCalculated: async () => {}, + onResponseChunk: (chunk) => responseChunks.push(chunk), + }) + + const events = responseChunks.filter( + (chunk): chunk is PrintModeEvent => typeof chunk !== 'string', + ) + // The payload was repaired (folded), not rejected, so no error event. + expect(events.some((event) => event.type === 'error')).toBe(false) + // A tool_call for spawn_agents was published to the handler. + expect(events.find((event) => event.type === 'tool_call')).toMatchObject({ + type: 'tool_call', + toolName: 'spawn_agents', + toolCallId: 'basher-misbraced-fold-tool-call-id', + }) + }) + it('should still emit tool_call and tool_result for valid tool calls', async () => { // Create an agent that has read_files tool const agentWithReadFiles: AgentTemplate = { diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index 87cfd96efb..a9fc148bf0 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -914,8 +914,9 @@ function detectMisbracedSpawnPayload(args: { input: unknown toolCallId: string rawInput: unknown + logger?: Logger }): DetectMisbracedSpawnResult | undefined { - const { input, toolCallId, rawInput } = args + const { input, toolCallId, rawInput, logger } = args if (!input || typeof input !== 'object' || Array.isArray(input)) { return undefined } @@ -957,6 +958,12 @@ function detectMisbracedSpawnPayload(args: { if ((foldableKeys as readonly string[]).includes(key)) continue repairedInput[key] = value } + // Log only the folded key names and the toolCallId; never the payload + // values themselves, to avoid leaking prompt/params/handoff content. + logger?.debug( + { toolCallId, misplacedKeys }, + 'spawn_agents: auto-repaired mis-braced single-agent payload by folding stray sibling field(s) into the agent entry', + ) return { repairedInput } } } @@ -1129,8 +1136,9 @@ export function parseRawToolCall(params: { input: unknown providerOptions?: ProviderMetadata } + logger?: Logger }): CodebuffToolCall | ToolCallError { - const { rawToolCall } = params + const { rawToolCall, logger } = params const toolName = rawToolCall.toolName const processedParameters = parseStringifiedToolInput( @@ -1157,6 +1165,7 @@ export function parseRawToolCall(params: { input: repairedInput, toolCallId: rawToolCall.toolCallId, rawInput: rawToolCall.input, + logger, }) if (misbraced) { if ('error' in misbraced) { @@ -1305,6 +1314,7 @@ export async function executeToolCall( input, providerOptions: params.providerOptions, }, + logger, }) // Filter out restricted tools - emit error instead of tool call/result From 4cd8c3b57648af6c66fe0de8203dd3a28b2b609e Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Thu, 23 Jul 2026 22:40:04 +0300 Subject: [PATCH 09/11] Reframe git-committer gate-block messages as normal ordering The two git-committer block messages in tool-executor now describe the validation/reviewer gate re-arming on every edit and clearing automatically, framing the block as expected ordering rather than a failure so agents wait one gate cycle instead of retrying in a loop. gitDisciplineSection carries matching wait-and-commit guidance, and the run-agent-step-tools and quality-prompt-snapshot tests pin the new wording so the behavior-changing messages stay covered. --- .../__tests__/quality-prompt-snapshot.test.ts | 10 +++++++ agents/base2/quality-prompt-section.ts | 2 +- .../__tests__/run-agent-step-tools.test.ts | 29 +++++++++++++++++++ .../agent-runtime/src/tools/tool-executor.ts | 4 +-- 4 files changed, 42 insertions(+), 3 deletions(-) diff --git a/agents/__tests__/quality-prompt-snapshot.test.ts b/agents/__tests__/quality-prompt-snapshot.test.ts index c4f98d944b..1bd6001ce4 100644 --- a/agents/__tests__/quality-prompt-snapshot.test.ts +++ b/agents/__tests__/quality-prompt-snapshot.test.ts @@ -120,6 +120,16 @@ describe('shared craftsmanship prompt sections', () => { expect(gitDisciplineSection).toContain( 'Spawning git-committer is not available yet', ) + // Wait-and-commit guidance added for gate ergonomics: the gate re-arms per + // edit, the block is normal ordering (not an error), and the commit lands + // automatically once the gate clears. + expect(gitDisciplineSection).toContain('re-arms on every new edit') + expect(gitDisciplineSection).toContain( + 'Treat this as normal ordering, not an error', + ) + expect(gitDisciplineSection).toContain( + 'the commit will land automatically once the gate clears', + ) }) test('gateAwarenessSection contains the required gate-awareness topics (not byte-frozen)', () => { diff --git a/agents/base2/quality-prompt-section.ts b/agents/base2/quality-prompt-section.ts index 22b8b441f8..35a337b698 100644 --- a/agents/base2/quality-prompt-section.ts +++ b/agents/base2/quality-prompt-section.ts @@ -156,7 +156,7 @@ export const gitDisciplineSection = `# Git Discipline When the user asks to commit, stage, branch, or push changes, delegate the full git workflow to the \`git-committer\` agent rather than running raw \`git\` commands yourself. Pass exact task-owned paths whenever known. The git-committer agent handles repository/worktree inspection, ownership-safe staging, commit-message composition, remote freshness checks, and explicitly authorized non-force feature-branch pushes. - **Pass owned_paths in params (REQUIRED):** spawn git-committer with a real params object whose owned_paths is the array of task-owned, project-relative file paths to stage. owned_paths is a required field and a hard allowlist, so omitting it (an empty or prompt-only spawn) fails the spawn outright. The param key is literally \`owned_paths\` — not \`paths\`, \`filePaths\`, or \`files\`; any other key name is ignored and the spawn fails with "Missing required: owned_paths". Optional params keys: branch_name, branch_switch, allow_dirty_branch, push (defaults to false; never set true unless the user explicitly asked to push), and remote. Put these in params, not only in the prose prompt. -- **Commit only after the gate is green:** the automated validation/reviewer gate reviews your UNCOMMITTED worktree changes (the diff against HEAD). Committing or pushing first empties that review set, so reviewers can no longer see the change and the gate cannot attest to it. Run the gate first; only commit/push (via git-committer) once validation and review are green for the changed files. If the user asks to commit before the gate has run, run the gate first, then commit. The runtime enforces this ordering: spawning git-committer before the gate passes fails with "Spawning git-committer is not available yet", so wait for the gate to report passed for the pending files, then spawn the committer. +- **Commit only after the gate is green:** the automated validation/reviewer gate reviews your UNCOMMITTED worktree changes (the diff against HEAD). Committing or pushing first empties that review set, so reviewers can no longer see the change and the gate cannot attest to it. Run the gate first; only commit/push (via git-committer) once validation and review are green for the changed files. If the user asks to commit before the gate has run, run the gate first, then commit. The runtime enforces this ordering: spawning git-committer before the gate passes fails with "Spawning git-committer is not available yet", so wait for the gate to report passed for the pending files, then spawn the committer. The gate re-arms on every new edit: a fresh code change sets it back to pending, so a commit request that immediately follows an edit must wait one more gate cycle even if a prior cycle already passed. Treat this as normal ordering, not an error — the gate runs and clears automatically, so do not retry the committer spawn in a tight loop; wait for the passed signal, then spawn it once. When the user asks to commit right after an edit, tell them the commit will land automatically once the gate clears rather than surfacing the block as a failure. - **Never push to the remote repository** unless the user explicitly asks you to. Direct default-branch pushes require separate explicit authorization; force pushes remain prohibited. - **Never alter git config** (no \`git config user.name/email\`, no \`--global\` flags). - **Never commit secrets** — scan staged content for tokens, API keys, and credentials before committing. The git-committer agent does this automatically. diff --git a/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts b/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts index 481939311f..20a53d1c4b 100644 --- a/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts +++ b/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts @@ -611,6 +611,21 @@ describe('runAgentStep - set_output tool', () => { ), }), ) + // Pin the reworded gate-block guidance: the message frames the block as + // normal ordering (the gate re-arms per edit and clears automatically), + // not a failure, so this behavior-changing wording stays covered. + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining('normal ordering, not a failure'), + }), + ) + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining('clears automatically'), + }), + ) expect(chunks).not.toContainEqual( expect.objectContaining({ type: 'tool_call', @@ -929,6 +944,20 @@ describe('runAgentStep - set_output tool', () => { ), }), ) + // Pin the reworded uncommitted-unvalidated gate-block guidance: normal + // ordering framing plus the auto-clearing explanation. + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining('normal ordering, not a failure'), + }), + ) + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining('clears automatically'), + }), + ) expect(chunks).not.toContainEqual( expect.objectContaining({ type: 'tool_call', diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index a9fc148bf0..7e57b37d83 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -1529,7 +1529,7 @@ export async function executeToolCall( onResponseChunk({ type: 'error', message: - 'Spawning `git-committer` is not available yet. The validation/reviewer gate must pass before committing. Wait for the automated gate to complete, then commit.', + 'Spawning `git-committer` is not available yet. This is normal ordering, not a failure: the validation/reviewer gate re-arms whenever code changes, so a fresh edit sets it back to pending and the committer is withheld until that gate passes for the changed files. The gate runs and clears automatically — do not retry the spawn now. Wait for the gate to report passed for the pending files, then spawn git-committer; the commit will succeed on the next attempt.', }) if (filteredAgents.length === 0) { return abortablePreviousToolCallFinished @@ -1690,7 +1690,7 @@ export async function executeToolCall( onResponseChunk({ type: 'error', message: - 'Spawning `git-committer` is not available yet. The requested commit would stage changes that have not passed the validation/reviewer gate. Validate the working-tree changes first, then commit.', + 'Spawning `git-committer` is not available yet. This is normal ordering, not a failure: the requested commit would stage changes that have not passed the validation/reviewer gate (a recent edit left working-tree files pending). The gate runs and clears automatically for those files — do not retry the spawn now. Wait for it to report passed, then spawn git-committer; the commit will succeed on the next attempt.', }) if (filteredAgents.length === 0) { return abortablePreviousToolCallFinished From 1b00596b1630b3d9e5e9304b43218ea6e1f45073 Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Fri, 24 Jul 2026 00:39:25 +0300 Subject: [PATCH 10/11] Route all-coverage reviewer findings exclusively to test-writer Export isTestCoverageReviewerFinding and mirror it inline in handleSteps so pure coverage blockers spawn test-writer only; mixed or code findings stay on repair-editor. Covered by unit, parity, and base2 integration tests. --- agents/__tests__/base2.test.ts | 114 ++++++- agents/__tests__/gate-reviewer-parity.test.ts | 142 +++++++++ agents/__tests__/gate-reviewer.test.ts | 61 ++++ agents/base2/base2.ts | 298 ++++++++++++------ agents/base2/gate-reviewer.ts | 9 + 5 files changed, 531 insertions(+), 93 deletions(-) create mode 100644 agents/__tests__/gate-reviewer-parity.test.ts diff --git a/agents/__tests__/base2.test.ts b/agents/__tests__/base2.test.ts index 12a489d2dd..9272c9702b 100644 --- a/agents/__tests__/base2.test.ts +++ b/agents/__tests__/base2.test.ts @@ -80,6 +80,7 @@ function attestedReviewerResult( reviewCall: any, verdict: 'LOOKS_GOOD' | 'NON_BLOCKING' | 'BLOCKING' = 'LOOKS_GOOD', findings: string[] = [], + coverage: 'covered' | 'missing' | 'n/a' = 'covered', ) { const prompt = String(reviewCall?.input?.agents?.[0]?.prompt ?? '') const fingerprint = @@ -101,7 +102,7 @@ function attestedReviewerResult( snapshotFingerprint: fingerprint, reviewedFiles: files, findings, - coverage: 'covered', + coverage, dimensions: { correctness: 'pass', security: 'pass', @@ -2772,6 +2773,117 @@ describe('base2 verification and reviewer gates', () => { expect(gen.next().value).toBe('STEP') }) + // All-coverage blocker sets must not co-spawn repair-editor. + test('all-coverage reviewer findings route exclusively to 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) + + expect(gen.next().value).toMatchObject({ toolName: 'git_status' }) + expect( + gen.next({ toolResult: [{ type: 'json', value: { status: '' } }] } as any) + .value, + ).toMatchObject({ toolName: 'spawn_agent_inline' }) + expect(gen.next().value).toBe('STEP') + expect( + gen.next({ + stepsComplete: true, + toolResult: [{ type: 'json', value: { file: 'src/a.ts' } }], + } as any).value, + ).toMatchObject({ toolName: 'git_status' }) + expect( + gen.next({ + toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], + } as any).value, + ).toMatchObject({ toolName: 'run_file_change_hooks' }) + const reviewCall = gen.next({ + toolResult: [{ type: 'json', value: [] }], + } as any).value + expect(reviewCall).toMatchObject({ toolName: 'spawn_agents' }) + // coverage: 'missing' with empty findings produces only the synthetic + // all-coverage blocker classified by isTestCoverageReviewerFinding. + const afterReview = gen.next( + attestedReviewerResult(reviewCall, 'NON_BLOCKING', [], 'missing') as any, + ) + expect(afterReview.value).toMatchObject({ + toolName: 'add_message', + input: { role: 'user' }, + }) + expect((afterReview.value as any).input.content).toContain('test-writer') + expect((afterReview.value as any).input.content).not.toContain( + 'to repair-editor', + ) + + 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') + expect((agentState as any).base2ActiveWork.nextRequiredAction).toContain( + 'Test-writer must add coverage', + ) + }) + + test('mixed coverage and code findings keep repair-editor only', () => { + const base2 = createBase2('default') + const agentState = { agentId: 'base2' } + const gen = base2.handleSteps!({ + agentState, + prompt: 'Make the requested change now please', + params: {}, + } as any) + + expect(gen.next().value).toMatchObject({ toolName: 'git_status' }) + expect( + gen.next({ toolResult: [{ type: 'json', value: { status: '' } }] } as any) + .value, + ).toMatchObject({ toolName: 'spawn_agent_inline' }) + expect(gen.next().value).toBe('STEP') + expect( + gen.next({ + stepsComplete: true, + toolResult: [{ type: 'json', value: { file: 'src/a.ts' } }], + } as any).value, + ).toMatchObject({ toolName: 'git_status' }) + expect( + gen.next({ + toolResult: [{ type: 'json', value: { status: ' M src/a.ts' } }], + } as any).value, + ).toMatchObject({ toolName: 'run_file_change_hooks' }) + const reviewCall = gen.next({ + toolResult: [{ type: 'json', value: [] }], + } as any).value + expect(reviewCall).toMatchObject({ toolName: 'spawn_agents' }) + // Code finding + coverage missing => mixed set must stay on repair-editor. + const afterReview = gen.next( + attestedReviewerResult( + reviewCall, + 'BLOCKING', + ['Fix the edge case.'], + 'missing', + ) as any, + ) + expect(afterReview.value).toMatchObject({ + toolName: 'add_message', + input: { role: 'user' }, + }) + expect((afterReview.value as any).input.content).toContain('repair-editor') + + 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[0].agent_type).not.toBe('test-writer') + }) + test('repair-editor with mutation progress continues into re-validation even when receipt is blocked', () => { const base2 = createBase2('default') const agentState = { agentId: 'base2' } diff --git a/agents/__tests__/gate-reviewer-parity.test.ts b/agents/__tests__/gate-reviewer-parity.test.ts new file mode 100644 index 0000000000..6f8538ad19 --- /dev/null +++ b/agents/__tests__/gate-reviewer-parity.test.ts @@ -0,0 +1,142 @@ +import { readFileSync } from 'node:fs' + +import { describe, expect, test } from 'bun:test' + +import { isTestCoverageReviewerFinding } from '../base2/gate-reviewer' + +// Parity: inline base2 mirror must match the gate-reviewer export used for +// exclusive all-coverage → test-writer routing. +type GateReviewerHelpers = { + isTestCoverageReviewerFinding: (text: string) => boolean +} + +type GateReviewerFunctionName = keyof GateReviewerHelpers +type InlineHelperFactory = () => GateReviewerHelpers + +const INLINE_HELPER_NAMES: GateReviewerFunctionName[] = [ + 'isTestCoverageReviewerFinding', +] + +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 loadInlineGateReviewerHelpers(): GateReviewerHelpers { + 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 = INLINE_HELPER_NAMES.map((functionName) => + extractInlineFunctionSource(base2JavaScript, functionName), + ).join('\n\n') + const buildHelpers = new Function( + `"use strict";\n${helperSource}\nreturn { isTestCoverageReviewerFinding }`, + ) as InlineHelperFactory + + return buildHelpers() +} + +describe('gate-reviewer helpers — inline copies match canonical exports', () => { + test('isTestCoverageReviewerFinding parity across representative inputs', () => { + const inlineHelpers = loadInlineGateReviewerHelpers() + + const inputs: unknown[] = [ + // coverage-only findings (expect true) + 'BLOCKING: test coverage missing for changed behavior (add a case to the relevant *.test.ts)', + 'test coverage is insufficient', + 'TEST COVERAGE missing', + 'BLOCKING: coverage gap: add a case to src/foo.test.ts for the new behavior', + 'coverage missing; extend widget.test.tsx', + // generic test/coverage mentions that keep the repair-editor path (false) + 'BLOCKING: add tests for the parser', + 'BLOCKING: this path is not tested', + 'BLOCKING: coverage of edge cases is unclear', + 'BLOCKING: update foo.test.ts to match the new API', + 'BLOCKING: fix the null dereference in parse()', + '', + ' ', + // non-string inputs (false) + undefined, + null, + 42, + {}, + ['test coverage'], + ] + + for (const input of inputs) { + expect( + inlineHelpers.isTestCoverageReviewerFinding(input as string), + ).toBe(isTestCoverageReviewerFinding(input as string)) + } + }) +}) + +// Direct behavioral coverage for the canonical gate-reviewer.ts export (as +// opposed to the parity suite above, which only asserts the inline base2 copy +// matches the export). These assertions also keep the export consumed so it +// is not dead code. +describe('gate-reviewer helpers — canonical export behavior', () => { + test('isTestCoverageReviewerFinding keys on the test-coverage bigram or coverage plus a .test.* token', () => { + expect( + isTestCoverageReviewerFinding( + 'BLOCKING: test coverage missing for changed behavior (add a case to the relevant *.test.ts)', + ), + ).toBe(true) + expect(isTestCoverageReviewerFinding('TEST COVERAGE missing')).toBe(true) + expect( + isTestCoverageReviewerFinding('coverage missing; extend widget.test.tsx'), + ).toBe(true) + }) + + test('isTestCoverageReviewerFinding stays conservative for generic test/coverage mentions', () => { + expect( + isTestCoverageReviewerFinding('BLOCKING: add tests for the parser'), + ).toBe(false) + expect( + isTestCoverageReviewerFinding( + 'BLOCKING: coverage of edge cases is unclear', + ), + ).toBe(false) + expect( + isTestCoverageReviewerFinding( + 'BLOCKING: update foo.test.ts to match the new API', + ), + ).toBe(false) + expect(isTestCoverageReviewerFinding('')).toBe(false) + }) + + test('isTestCoverageReviewerFinding rejects non-string inputs', () => { + expect( + isTestCoverageReviewerFinding(undefined as unknown as string), + ).toBe(false) + expect(isTestCoverageReviewerFinding(null as unknown as string)).toBe( + false, + ) + expect(isTestCoverageReviewerFinding(42 as unknown as string)).toBe(false) + }) +}) diff --git a/agents/__tests__/gate-reviewer.test.ts b/agents/__tests__/gate-reviewer.test.ts index d79cb3313d..534cb787b2 100644 --- a/agents/__tests__/gate-reviewer.test.ts +++ b/agents/__tests__/gate-reviewer.test.ts @@ -8,6 +8,7 @@ import { collectReviewerFindingRecords, detectReviewerCrash, getReviewerFinalizationVerdict, + isTestCoverageReviewerFinding, stripReviewerPreamble, } from '../base2/gate-reviewer' @@ -611,4 +612,64 @@ describe('gate-reviewer helpers', () => { ), ).toBe('') }) + + test('isTestCoverageReviewerFinding keys on the test-coverage bigram or coverage plus a .test.* token', () => { + // The synthetic coverage blocker from collectReviewerBlockers must classify + // as coverage so the all-coverage set routes exclusively to test-writer. + expect( + isTestCoverageReviewerFinding( + 'BLOCKING: test coverage missing for changed behavior (add a case to the relevant *.test.ts)', + ), + ).toBe(true) + expect( + isTestCoverageReviewerFinding('test coverage is insufficient'), + ).toBe(true) + expect(isTestCoverageReviewerFinding('TEST COVERAGE missing')).toBe(true) + expect( + isTestCoverageReviewerFinding( + 'BLOCKING: coverage gap: add a case to src/foo.test.ts for the new behavior', + ), + ).toBe(true) + expect( + isTestCoverageReviewerFinding('coverage missing; extend widget.test.tsx'), + ).toBe(true) + }) + + test('isTestCoverageReviewerFinding stays conservative for generic test/coverage mentions', () => { + // Generic requirements mentioning test(s)/tested or bare coverage must + // keep routing to repair-editor (status quo). + expect( + isTestCoverageReviewerFinding('BLOCKING: add tests for the parser'), + ).toBe(false) + expect( + isTestCoverageReviewerFinding('BLOCKING: this path is not tested'), + ).toBe(false) + expect( + isTestCoverageReviewerFinding( + 'BLOCKING: coverage of edge cases is unclear', + ), + ).toBe(false) + expect( + isTestCoverageReviewerFinding( + 'BLOCKING: update foo.test.ts to match the new API', + ), + ).toBe(false) + expect( + isTestCoverageReviewerFinding( + 'BLOCKING: fix the null dereference in parse()', + ), + ).toBe(false) + expect(isTestCoverageReviewerFinding('')).toBe(false) + }) + + test('isTestCoverageReviewerFinding rejects non-string inputs', () => { + expect( + isTestCoverageReviewerFinding(undefined as unknown as string), + ).toBe(false) + expect(isTestCoverageReviewerFinding(null as unknown as string)).toBe( + false, + ) + expect(isTestCoverageReviewerFinding(42 as unknown as string)).toBe(false) + expect(isTestCoverageReviewerFinding({} as unknown as string)).toBe(false) + }) }) diff --git a/agents/base2/base2.ts b/agents/base2/base2.ts index 132b023159..afbb1bd689 100644 --- a/agents/base2/base2.ts +++ b/agents/base2/base2.ts @@ -2928,6 +2928,18 @@ ${specialistRoutingSection} activeWorkState.reviewerProtocolRetryCount = 0 const blockers = collectReviewerBlockers(reviewerToolResult) if (blockers.length > 0) { + // Coverage-style findings (a missing/uncertain test-coverage gap) + // are not code-diagnostic repairs: repair-editor cannot author the + // missing assertions and would return an empty receipt, parking the + // gate in blocked. Route an ALL-coverage set exclusively to + // test-writer, which can author the tests. Mixed or code-only sets + // keep the repair-editor path and converge across iterations. + const allCoverageFindings = blockers.every( + isTestCoverageReviewerFinding, + ) + const repairAgentLabel = allCoverageFindings + ? 'Test-writer' + : 'Repair-editor' activeWorkState.reviewerRepairRoundCount = Number( activeWorkState.reviewerRepairRoundCount ?? 0, ) + 1 @@ -2954,7 +2966,7 @@ ${specialistRoutingSection} input: { role: 'user', content: [ - `Reviewer gate: ${reviewerAgentType} returned blocking feedback. The harness will send these exact findings to repair-editor:`, + `Reviewer gate: ${reviewerAgentType} returned blocking feedback. The harness will send these exact findings to ${allCoverageFindings ? 'test-writer' : 'repair-editor'}:`, '', ...blockers, '', @@ -2969,97 +2981,187 @@ ${specialistRoutingSection} activeWorkState.repairSessionId = reviewerRepairSessionId activeWorkState.requiredReviewerRevalidation = requiredReviewerAgentType activeWorkState.currentPhase = 'repair_loop' - activeWorkState.nextRequiredAction = - 'Repair-editor must address every open reviewer finding, then targeted validation and a fresh reviewer pass must run.' + activeWorkState.nextRequiredAction = allCoverageFindings + ? 'Test-writer must add coverage for every open reviewer finding, then targeted validation and a fresh reviewer pass must run.' + : 'Repair-editor must address every open reviewer finding, then targeted validation and a fresh reviewer pass must run.' const reviewerRepairResult = yield { toolName: 'spawn_agents', input: { agents: [ - { - agent_type: 'repair-editor', - handoff: { - schemaVersion: 1, - taskId: reviewerRepairSessionId, - role: 'repair-editor', - objective: - 'Resolve every open reviewer finding without unrelated changes. Read the current file contents before editing; conversational summaries are not source evidence.', - requirements: activeWorkState.openReviewerFindings.map( - ({ id, text }) => ({ id, text, required: true }), - ), - acceptanceCriteria: - activeWorkState.openReviewerFindings.map(({ id }) => ({ - id: `clear-${id}`, - behavior: `Finding ${id} is addressed in the live workspace.`, - verification: - 'Targeted validation passes and a fresh snapshot-bound reviewer clears the finding.', - })), - context: [], - invariants: [ - 'Read every target from the live filesystem before editing.', - 'Treat every finding ID as open until a fresh reviewer clears it.', - ], - nonGoals: [ - 'Unrelated diagnostics, refactors, or cleanup.', - ], - risks: [ - 'Reviewer findings may be stale if the workspace snapshot changed.', - ], - unknowns: [], - findings: activeWorkState.openReviewerFindings.map( - ({ id, text, files, snapshotFingerprint }) => ({ - id, - text, - files, - snapshotFingerprint, - }), - ), - permissions: { - readablePaths: repairEditorReadablePaths([ - ...pendingGateFiles, - ...activeWorkState.openReviewerFindings.flatMap( - (finding: { files?: string[] }) => - finding.files ?? [], + allCoverageFindings + ? { + agent_type: 'test-writer', + handoff: { + schemaVersion: 1, + taskId: reviewerRepairSessionId, + role: 'test-writer', + objective: + 'Author or extend tests that assert the changed behavior so the reviewer test-coverage dimension is satisfied. For each finding, add or extend a case in the relevant *.test.* file exercising the behavior-changing logic in the gate changed files. Do not modify production source unless strictly required to make a test observable.', + requirements: + activeWorkState.openReviewerFindings.map( + ({ id, text }) => ({ id, text, required: true }), + ), + acceptanceCriteria: + activeWorkState.openReviewerFindings.map( + ({ id }) => ({ + id: `clear-${id}`, + behavior: `Finding ${id} is addressed by new or extended test coverage.`, + verification: + 'Targeted validation passes and a fresh snapshot-bound reviewer clears the finding.', + }), + ), + context: [], + invariants: [ + 'Read every target from the live filesystem before editing.', + 'Treat every finding ID as open until a fresh reviewer clears it.', + ], + nonGoals: [ + 'Unrelated diagnostics, refactors, or cleanup.', + ], + risks: [ + 'Reviewer findings may be stale if the workspace snapshot changed.', + ], + unknowns: [], + findings: activeWorkState.openReviewerFindings.map( + ({ id, text, files, snapshotFingerprint }) => ({ + id, + text, + files, + snapshotFingerprint, + }), ), - ]), - writablePaths: Array.from( - new Set([ - ...pendingGateFiles, - ...activeWorkState.openReviewerFindings.flatMap( - (finding: { files?: string[] }) => - finding.files ?? [], + permissions: { + readablePaths: repairEditorReadablePaths([ + ...pendingGateFiles, + ...activeWorkState.openReviewerFindings.flatMap( + (finding: { files?: string[] }) => + finding.files ?? [], + ), + ]), + writablePaths: Array.from( + new Set([ + ...pendingGateFiles, + ...activeWorkState.openReviewerFindings.flatMap( + (finding: { files?: string[] }) => + finding.files ?? [], + ), + ]), ), - ]), - ), - allowedTools: [ - 'read_files', - 'read_outline', - 'edit_transaction', - ], + allowedTools: [ + 'read_files', + 'read_outline', + 'write_file', + 'str_replace', + 'set_output', + ], + }, + workspaceRevision: + mutableAgentState.workspaceState?.revision, + workspaceSnapshotId: + mutableAgentState.workspaceState?.snapshotId, + artifacts: [], + successCriteria: [ + 'Writer receipt reports changed test files covering the findings.', + ], + constraints: [ + 'Keep every edit within the pending gate file set.', + ], + }, + prompt: [ + 'Add or extend the test coverage that satisfies the blocking reviewer findings below.', + 'Treat every finding ID as open until a fresh reviewer clears it.', + 'Read the changed source and the relevant existing test file before editing.', + '', + ...activeWorkState.openReviewerFindings.map( + (finding) => `${finding.id}: ${finding.text}`, + ), + ].join('\n'), + } + : { + agent_type: 'repair-editor', + handoff: { + schemaVersion: 1, + taskId: reviewerRepairSessionId, + role: 'repair-editor', + objective: + 'Resolve every open reviewer finding without unrelated changes. Read the current file contents before editing; conversational summaries are not source evidence.', + requirements: activeWorkState.openReviewerFindings.map( + ({ id, text }) => ({ id, text, required: true }), + ), + acceptanceCriteria: + activeWorkState.openReviewerFindings.map(({ id }) => ({ + id: `clear-${id}`, + behavior: `Finding ${id} is addressed in the live workspace.`, + verification: + 'Targeted validation passes and a fresh snapshot-bound reviewer clears the finding.', + })), + context: [], + invariants: [ + 'Read every target from the live filesystem before editing.', + 'Treat every finding ID as open until a fresh reviewer clears it.', + ], + nonGoals: [ + 'Unrelated diagnostics, refactors, or cleanup.', + ], + risks: [ + 'Reviewer findings may be stale if the workspace snapshot changed.', + ], + unknowns: [], + findings: activeWorkState.openReviewerFindings.map( + ({ id, text, files, snapshotFingerprint }) => ({ + id, + text, + files, + snapshotFingerprint, + }), + ), + permissions: { + readablePaths: repairEditorReadablePaths([ + ...pendingGateFiles, + ...activeWorkState.openReviewerFindings.flatMap( + (finding: { files?: string[] }) => + finding.files ?? [], + ), + ]), + writablePaths: Array.from( + new Set([ + ...pendingGateFiles, + ...activeWorkState.openReviewerFindings.flatMap( + (finding: { files?: string[] }) => + finding.files ?? [], + ), + ]), + ), + allowedTools: [ + 'read_files', + 'read_outline', + 'edit_transaction', + ], + }, + workspaceRevision: + mutableAgentState.workspaceState?.revision, + workspaceSnapshotId: + mutableAgentState.workspaceState?.snapshotId, + artifacts: [], + successCriteria: [ + 'All finding IDs are cleared by a fresh reviewer receipt.', + ], + constraints: [ + 'Keep every edit within the pending gate file set.', + ], + }, + prompt: [ + 'Repair the blocking reviewer findings below.', + 'Treat every finding ID as open until a fresh reviewer clears it.', + 'Do not claim a finding is stale because unrelated tests or another task passed.', + 'Read every target from the live filesystem before editing.', + 'Keep unrelated diagnostics secondary to this finding set.', + '', + ...activeWorkState.openReviewerFindings.map( + (finding) => `${finding.id}: ${finding.text}`, + ), + ].join('\n'), }, - workspaceRevision: - mutableAgentState.workspaceState?.revision, - workspaceSnapshotId: - mutableAgentState.workspaceState?.snapshotId, - artifacts: [], - successCriteria: [ - 'All finding IDs are cleared by a fresh reviewer receipt.', - ], - constraints: [ - 'Keep every edit within the pending gate file set.', - ], - }, - prompt: [ - 'Repair the blocking reviewer findings below.', - 'Treat every finding ID as open until a fresh reviewer clears it.', - 'Do not claim a finding is stale because unrelated tests or another task passed.', - 'Read every target from the live filesystem before editing.', - 'Keep unrelated diagnostics secondary to this finding set.', - '', - ...activeWorkState.openReviewerFindings.map( - (finding) => `${finding.id}: ${finding.text}`, - ), - ].join('\n'), - }, ], }, } as any @@ -3069,8 +3171,8 @@ ${specialistRoutingSection} if (repairCrash) { activeWorkState.currentPhase = 'blocked' activeWorkState.nextRequiredAction = - 'Repair-editor failed while addressing reviewer findings. Inspect the failure before retrying.' - activeWorkState.latestWorkSummary = `Repair-editor failed: ${repairCrash}` + `${repairAgentLabel} failed while addressing reviewer findings. Inspect the failure before retrying.` + activeWorkState.latestWorkSummary = `${repairAgentLabel} failed: ${repairCrash}` markActiveWorkStateChanged() break } @@ -3099,7 +3201,7 @@ ${specialistRoutingSection} ) { activeWorkState.currentPhase = 'blocked' activeWorkState.nextRequiredAction = - 'Repair-editor did not return a completed receipt addressing every open reviewer finding.' + `${repairAgentLabel} did not return a completed receipt addressing every open reviewer finding.` activeWorkState.latestWorkSummary = 'Reviewer repair receipt was incomplete or missing.' markActiveWorkStateChanged() @@ -3125,7 +3227,7 @@ ${specialistRoutingSection} if (repairedSnapshotFingerprint === reviewSnapshotFingerprint) { activeWorkState.currentPhase = 'blocked' activeWorkState.nextRequiredAction = - 'Repair-editor made no snapshot-visible progress on the reviewer findings. Stop retrying and inspect the finding or handoff.' + `${repairAgentLabel} made no snapshot-visible progress on the reviewer findings. Stop retrying and inspect the finding or handoff.` activeWorkState.latestWorkSummary = 'Reviewer repair produced no workspace fingerprint change.' markActiveWorkStateChanged() @@ -3150,7 +3252,7 @@ ${specialistRoutingSection} activeWorkState.currentPhase = 'awaiting_review' activeWorkState.nextRequiredAction = '' activeWorkState.latestWorkSummary = - 'Repair-editor addressed reviewer findings; validation re-ran inline and a fresh reviewer pass is required.' + `${repairAgentLabel} addressed reviewer findings; validation re-ran inline and a fresh reviewer pass is required.` markActiveWorkStateChanged() emitGateTelemetry({ currentPhase: 'awaiting_review', @@ -3166,7 +3268,7 @@ ${specialistRoutingSection} activeWorkState.lastReviewerGateSkipReason = 'validation-hook-failures' activeWorkState.currentPhase = 'blocked' - activeWorkState.latestWorkSummary = `Repair-editor addressed reviewer findings but ${reFailures.length} validation failure(s) remain.` + activeWorkState.latestWorkSummary = `${repairAgentLabel} addressed reviewer findings but ${reFailures.length} validation failure(s) remain.` markActiveWorkStateChanged() emitGateTelemetry({ currentPhase: 'blocked', @@ -3181,7 +3283,7 @@ ${specialistRoutingSection} input: { role: 'user', content: [ - `Repair-editor addressed the reviewer findings but ${reFailures.length} validation failure(s) remain. Fix these before ending your turn:`, + `${repairAgentLabel} addressed the reviewer findings but ${reFailures.length} validation failure(s) remain. Fix these before ending your turn:`, '', ...reFailures, '', @@ -5409,6 +5511,18 @@ ${specialistRoutingSection} return remaining } + // Inline mirror of isTestCoverageReviewerFinding in + // agents/base2/gate-reviewer.ts (all-coverage → exclusive test-writer). + // Keep in sync: handleSteps is serialized, so this helper cannot depend + // on imports. + function isTestCoverageReviewerFinding(text: string): boolean { + if (typeof text !== 'string') return false + const t = text.toLowerCase() + if (t.includes('test coverage')) return true + if (t.includes('coverage') && /\.test\.[a-z0-9]+/.test(t)) return true + return false + } + function collectReviewerBlockers(toolResult: unknown): string[] { // First check for structured reviewer outputs (e.g. JSON with a // verdict field). When present and BLOCKING, surface findings as the diff --git a/agents/base2/gate-reviewer.ts b/agents/base2/gate-reviewer.ts index 5df4fb51a3..7070a7965b 100644 --- a/agents/base2/gate-reviewer.ts +++ b/agents/base2/gate-reviewer.ts @@ -103,6 +103,15 @@ export function stripReviewerPreamble(text: string): string { return remaining } +/** True when a blocker is a pure test-coverage gap (all-coverage sets route to test-writer). */ +export function isTestCoverageReviewerFinding(text: string): boolean { + if (typeof text !== 'string') return false + const t = text.toLowerCase() + if (t.includes('test coverage')) return true + if (t.includes('coverage') && /\.test\.[a-z0-9]+/.test(t)) return true + return false +} + export function collectReviewerBlockers(toolResult: unknown): string[] { // First check for structured reviewer outputs (e.g. JSON with a // verdict field). When present and BLOCKING, surface findings as the From 7591af0966e59277151b27c8d8123681a5fb58eb Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Fri, 24 Jul 2026 01:21:10 +0300 Subject: [PATCH 11/11] Clarify hooks/reviewer gate ownership and fix deep phase order Expand gateAwareness for re-arm ownership, targeted validation != gate, commit recovery, and pending-set authority. Align base2/base-deep tool-choice and validation bullets; document control-plane and request-flow gate boundaries. Move deep Phase 6 Final Review before Phase 7 Lessons, polish typos, and assert specialistRouting in snapshots. --- .../__tests__/quality-prompt-snapshot.test.ts | 34 ++++++++++++++++-- agents/base2/base-deep.ts | 36 +++++++++---------- agents/base2/base2.ts | 4 +-- agents/base2/quality-prompt-section.ts | 16 +++++---- docs/agents-and-tools.md | 8 ++--- docs/request-flow.md | 2 +- 6 files changed, 64 insertions(+), 36 deletions(-) diff --git a/agents/__tests__/quality-prompt-snapshot.test.ts b/agents/__tests__/quality-prompt-snapshot.test.ts index 1bd6001ce4..951e28ee0c 100644 --- a/agents/__tests__/quality-prompt-snapshot.test.ts +++ b/agents/__tests__/quality-prompt-snapshot.test.ts @@ -135,11 +135,37 @@ describe('shared craftsmanship prompt sections', () => { test('gateAwarenessSection contains the required gate-awareness topics (not byte-frozen)', () => { // gateAwarenessSection is advisory guidance that may evolve; only assert // topic coverage so future tightening does not silently drop the - // don't-double-spawn-code-reviewer guidance. + // don't-double-spawn-code-reviewer guidance and the four hooks-vs-gate + // clarifications (re-arm ownership, targeted validation ≠ gate, commit + // recovery, pending-set authority). expect(gateAwarenessSection).toContain('# Automated Validation & Review Gate') expect(gateAwarenessSection).toContain('code-reviewer') - expect(gateAwarenessSection).toContain('validation hooks') - expect(gateAwarenessSection).toContain('before finalization') + expect(gateAwarenessSection).toContain('run_file_change_hooks') + expect(gateAwarenessSection).toContain('finalization allowed when green') + // 1) Re-arm ownership: runtime-owned hooks→reviewer; no manual re-spawn + // for the same pending set; wait when awaiting_validation. + expect(gateAwarenessSection).toContain('Do not double-spawn code-reviewer') + expect(gateAwarenessSection).toContain('same pending set') + expect(gateAwarenessSection).toContain('awaiting_validation') + // 2) run_targeted_validation is NOT the gate (scoped evidence only). + expect(gateAwarenessSection).toContain('run_targeted_validation') + expect(gateAwarenessSection).toContain('is NOT the gate') + expect(gateAwarenessSection).toContain( + 'does not clear reviewer findings by itself', + ) + expect(gateAwarenessSection).toContain('does **not** unlock') + expect(gateAwarenessSection).toContain('Basher typechecks') + // 3) Commit recovery: git-committer only after gate green; re-arms; no tight-loop. + expect(gateAwarenessSection).toContain('git-committer') + expect(gateAwarenessSection).toContain('re-arms on every new edit') + expect(gateAwarenessSection).toContain('tight-loop') + expect(gateAwarenessSection).toContain('not available yet') + // 4) Pending-set authority: full pendingGateFiles set, not last-edited file. + expect(gateAwarenessSection).toContain('pendingGateFiles') + expect(gateAwarenessSection).toContain('full related set') + expect(gateAwarenessSection).toContain( + 'authoritative over conversational memory', + ) }) test('securityReviewSection contains the required security-review topics (not byte-frozen)', () => { @@ -181,6 +207,7 @@ describe('shared craftsmanship prompt sections', () => { expect(base2.systemPrompt).toContain(gateAwarenessSection) expect(base2.systemPrompt).toContain(gitDisciplineSection) expect(base2.systemPrompt).toContain(securityReviewSection) + expect(base2.systemPrompt).toContain(specialistRoutingSection) expect(baseDeep.systemPrompt).toContain(qualitySection) expect(baseDeep.systemPrompt).toContain(PLACEHOLDER.FRONTEND_SECTION) @@ -189,6 +216,7 @@ describe('shared craftsmanship prompt sections', () => { expect(baseDeep.systemPrompt).toContain(gateAwarenessSection) expect(baseDeep.systemPrompt).toContain(gitDisciplineSection) expect(baseDeep.systemPrompt).toContain(securityReviewSection) + expect(baseDeep.systemPrompt).toContain(specialistRoutingSection) // gitDisciplineSection is intentionally NOT interpolated into the editor — // the editor is for code editing, not git work; the git-committer agent diff --git a/agents/base2/base-deep.ts b/agents/base2/base-deep.ts index a2c84e7771..ea71fe9504 100644 --- a/agents/base2/base-deep.ts +++ b/agents/base2/base-deep.ts @@ -1,5 +1,3 @@ -import { buildArray } from '@codebuff/common/util/array' - import { PLACEHOLDER, type SecretAgentDefinition, @@ -31,7 +29,7 @@ function buildDeepSystemPrompt( noAskUser ? '' : ` -- **Ask the user about important decisions or guidance using the ask_user tool:** You should feel free to stop and ask the user for guidance if there's a an important decision to make or you need an important clarification or you're stuck and don't know what to try next. Use the ask_user tool to collaborate with the user to acheive the best possible result! Prefer to gather context first before asking questions in case you end up answering your own question.` +- **Ask the user about important decisions or guidance using the ask_user tool:** You should feel free to stop and ask the user for guidance if there's an important decision to make or you need an important clarification or you're stuck and don't know what to try next. Use the ask_user tool to collaborate with the user to achieve the best possible result! Prefer to gather context first before asking questions in case you end up answering your own question.` } - **Be careful about terminal commands:** Be careful about instructing subagents to run terminal commands that could be destructive or have effects that are hard to undo (e.g. git push, git commit, running any scripts -- especially ones that could alter production environments (!), installing packages globally, etc). Don't run any of these effectful commands unless the user explicitly asks you to. - **Validation is dependency-neutral:** A test, typecheck, lint, or build request authorizes only that validation command. Never prepend or append install/add/remove/update/sync/restore commands. If validation cannot start because dependencies are missing, report that exact blocker; use dependency-manager only after separate explicit user authorization. @@ -50,10 +48,10 @@ Use the spawn_agents tool to spawn specialized agents to help you complete the u - **Thinker delegation:** Spawn thinker only after enough context exists for complex architecture, design tradeoff, risk, debugging strategy, spec/plan critique, or repeated-failure reasoning. Do not use thinker as a substitute for reading files or for straightforward edits. - **Release/deployment flow:** Treat releases, deployments, publishing, migrations against shared environments, production-affecting scripts, git commits, and git pushes as high-impact actions. Do not run or ask subagents to run them unless the user explicitly requested that action in this task or confirms after you explain the exact command, target environment, and rollback/verification plan. When requested, follow the deterministic sequence: inspect worktree, fetch remote state/tags, decide rebase/merge with the user when non-fast-forward or conflicts appear, push, wait for CI/CD, trigger the release, verify artifact/tag/package publication, then sync and report local branch state. - **Plan artifact maintenance:** In PLAN mode create and maintain durable artifacts; in EXECUTE_PLAN keep STATUS.md and LESSONS.md current at phase boundaries, blocker discovery/resolution, validation/review results, and finalization. Use update_plan_status for incremental STATUS/LESSONS updates and create_plan for SPEC/PLAN rewrites or missing artifacts. Do not update plan artifacts for ordinary implementation mode unless the user requested plan/session work. -- **Tool choice:** Prefer dedicated tools over shell fallbacks: repository status and configured file-change hooks are runtime-owned and injected automatically; use read_files/read_outline/read_subtree/glob/list_directory/query_index for inspection, read_image for screenshots/images and rendered/exported visual artifacts (3D render frames, image/video exports, generated diagrams, and charts), edit_transaction with the narrowest edit type for project mutations, browser_use/codebuff_local_cli for visual smoke tests, and basher only for commands without a dedicated tool. +- **Tool choice:** Prefer dedicated tools over shell fallbacks: repository status and configured file-change hooks are runtime-owned and injected automatically; use read_files/read_outline/read_subtree/glob/list_directory/query_index for inspection, read_image for screenshots/images and rendered/exported visual artifacts (3D render frames, image/video exports, generated diagrams, and charts), edit_transaction with the narrowest edit type for project mutations, browser_use/codebuff_local_cli for visual smoke tests, and basher only for commands without a dedicated tool. \`run_targeted_validation\` is scoped evidence only — it never unlocks the gate/commit path; hooks + automated reviewer remain runtime-owned. - **Sequence agents properly:** Keep in mind dependencies when spawning different agents. Don't spawn agents in parallel that depend on each other. - **Parallel join discipline:** When spawning agents in parallel, wait for every required result before moving to the next dependent phase. A timeout, failed validation, or \`BLOCKING:\` reviewer/security finding blocks completion until repaired or explicitly scoped out. -- **Validation selection:** Validate every non-trivial or risky edit with the narrowest relevant typecheck/test/lint/build command or configured file-change hooks. Map changed paths to suites deterministically when possible: agents/base2/* -> agents typecheck plus prompt/gate tests or e2e subset when behavior changes; agents/* -> agents typecheck and relevant agent tests; packages/sdk/* -> SDK typecheck/tests; packages/agent-runtime/* -> runtime typecheck/tests; common/* -> common checks plus dependent package typechecks; cli/src/components/* or cli/src/hooks/* -> CLI typecheck plus CLI visual smoke; docs/prompt-only changes -> configured hooks or explicit skip reason. Skip validation only for docs/prompt-only changes, tiny low-risk edits, explicit no-validation modes, or when the user forbids it; state the skip reason. Validation failures/timeouts are blocking and must be repaired or explicitly scoped out. +- **Validation selection:** Validate every non-trivial or risky edit with the narrowest relevant typecheck/test/lint/build command or configured file-change hooks. Map changed paths to suites deterministically when possible: agents/base2/* -> agents typecheck plus prompt/gate tests or e2e subset when behavior changes; agents/* -> agents typecheck and relevant agent tests; packages/sdk/* -> SDK typecheck/tests; packages/agent-runtime/* -> runtime typecheck/tests; common/* -> common checks plus dependent package typechecks; cli/src/components/* or cli/src/hooks/* -> CLI typecheck plus CLI visual smoke; docs/prompt-only changes -> configured hooks or explicit skip reason. Skip validation only for docs/prompt-only changes, tiny low-risk edits, explicit no-validation modes, or when the user forbids it; state the skip reason. Validation failures/timeouts are blocking and must be repaired or explicitly scoped out. Green basher typechecks or \`run_targeted_validation\` are optional evidence only — never a substitute for the runtime hooks+reviewer gate. - **Reviewer selection:** Use the automated reviewer gate for edited code in default mode. Spawn code-reviewer manually only for user-requested extra review, advisory/pre-edit review, significant diffs outside the automated gate, or changed code whose risk warrants another perspective; spawn security-reviewer for auth, crypto, secrets, permissions, injection, sandboxing, path/process/network handling, supply-chain, or production-risk changes; spawn test-writer when behavior changes lack coverage; spawn debugger after repeated validation failure, runtime failure, or unclear crash behavior. Do not duplicate the same post-edit review manually. - **Validation/reviewer coordination:** It is fine to run validation bashers and reviewers in parallel only when the reviewer is asked for static code review that explicitly does not depend on validation output. Always wait for both. Treat the final decision as a join of both results: validation failure/timeout blocks completion even if review looks good, and reviewer \`BLOCKING:\` blocks completion even if validation passes. When the review needs validation results, run validation first and include the completed validation summary in the reviewer prompt. - For broad codebase questions or tasks where relevant files are not already obvious, call query_index early yourself to get indexed file candidates, then verify the best candidates, matchedSnippets, and relatedFiles with read_files/read_subtree and/or spawn file-picker/code-searcher agents as needed. Use graph modes when useful: search for ranked discovery, explain for ranking rationale, neighbors to expand around a known file, path to connect two known files, and commands to find package scripts, CI workflows, task runners, and validation docs. Do not rely on query_index alone for correctness. @@ -63,7 +61,7 @@ Use the spawn_agents tool to spawn specialized agents to help you complete the u - Spawn bashers for validation/test coverage after edits when validation is appropriate; if validation fails, repair the exact failure before broadening scope. - Spawn the debugger after repeated validation failures, runtime failures, or unclear crash behavior where focused diagnosis is needed. - Spawn code-reviewer/security-reviewer after meaningful edits when user scope or risk calls for review. Spawn doc-writer/test-writer when documentation or test coverage is required or directly implied by acceptance criteria. - - Spawn bashers sequentially if the second command depends on the the first. + - Spawn bashers sequentially if the second command depends on the first. - **No need to include context:** When prompting an agent, realize that many agents can already see the entire conversation history, so you can be brief in prompting them without needing to include context. - **Never spawn the context-pruner agent:** This agent is spawned automatically for you and you don't need to spawn it yourself. @@ -253,7 +251,15 @@ Thoroughly validate the changes: - For a CLI tool: run it with relevant arguments - For a library: write and run a small integration script - For config/infra changes: validate the configuration is correct -4. If E2E verification reveals issues, fix them and re-validate.${ +4. If E2E verification reveals issues, fix them and re-validate. + +## Phase 6 — Final Review + +The automated runtime gate handles the final validation and code review after all implementation and validation-driven edits are complete. Do not manually duplicate its post-edit review for the same file set. + +1. **Let the automated gate run last:** The runtime detects the final changed-file set, reruns configured validation hooks, and then spawns code-reviewer before finalization. +2. **If the reviewer returns BLOCKING:** Treat that finding as the controlling next action. Fix it, rerun the relevant Phase 5 validation, then let the final gate re-run. +3. **Optional advisory review:** Before the final gate, you MAY request a focused security/design/architecture review when a specific concern warrants it. Advisory approval never replaces the final gate.${ noLearning ? '' : ` @@ -288,23 +294,15 @@ Capture learnings for future sessions: b. If the thinker suggests valid improvements or new skill ideas, update the relevant files accordingly. c. After updating, you MUST spawn thinker again to re-critique and brainstorm further. d. Repeat until the thinker finds no new substantive improvements or skill ideas. Do NOT skip the re-critique — every revision must be verified.` - }${ + } + +Make sure to narrate to the user what you are doing and why you are doing it as you go along. Give a very short summary of what you accomplished at the end of your turn before suggesting followups.${ noAskUser ? '' : ` -${noLearning ? '1' : '4'}. After writing a user-visible completion summary, use suggest_followups to suggest ~3 next steps the user might want to take.` +After writing a user-visible completion summary, use suggest_followups to suggest ~3 next steps the user might want to take.` } -## Phase 6 — Final Review - -The automated runtime gate handles the final validation and code review after all implementation and validation-driven edits are complete. Do not manually duplicate its post-edit review for the same file set. - -1. **Let the automated gate run last:** The runtime detects the final changed-file set, reruns configured validation hooks, and then spawns code-reviewer before finalization. -2. **If the reviewer returns BLOCKING:** Treat that finding as the controlling next action. Fix it, rerun the relevant Phase 5 validation, then let the final gate re-run. -3. **Optional advisory review:** Before the final gate, you MAY request a focused security/design/architecture review when a specific concern warrants it. Advisory approval never replaces the final gate. - -Make sure to narrate to the user what you are doing and why you are doing it as you go along. Give a very short summary of what you accomplished at the end of your turn before suggesting followups. - ## Followup Requests If the full ${totalPhases}-phase workflow has already been completed in this conversation and the user is asking for a followup change (e.g. "also add X" or "tweak Y"), you do NOT need to repeat the entire workflow. Use your judgement to run only the phases that are relevant — for example, directly make the requested changes (Phase 4), validate them (Phase 5), and let the final review gate run (Phase 6). Skip the spec and plan phases if the request is a straightforward extension of the work already done.${noLearning ? '' : ' Still update LESSONS.md and skills if you learn anything new.'} diff --git a/agents/base2/base2.ts b/agents/base2/base2.ts index afbb1bd689..59f07cc472 100644 --- a/agents/base2/base2.ts +++ b/agents/base2/base2.ts @@ -267,11 +267,11 @@ Use the spawn_agents tool to spawn specialized agents to help you complete the u - **Thinker delegation:** Spawn thinker only after enough context exists for complex architecture, design tradeoff, risk, debugging strategy, spec/plan critique, or repeated-failure reasoning. Do not use thinker as a substitute for reading files or for straightforward edits. - **Release/deployment flow:** Treat releases, deployments, publishing, migrations against shared environments, production-affecting scripts, git commits, and git pushes as high-impact actions. Do not run or ask subagents to run them unless the user explicitly requested that action in this task or confirms after you explain the exact command, target environment, and rollback/verification plan. When requested, follow the deterministic sequence: inspect worktree, fetch remote state/tags, decide rebase/merge with the user when non-fast-forward or conflicts appear, push, wait for CI/CD, trigger the release, verify artifact/tag/package publication, then sync and report local branch state. - **Plan artifact maintenance:** In PLAN mode create and maintain durable artifacts; in EXECUTE_PLAN keep STATUS.md and LESSONS.md current at phase boundaries, blocker discovery/resolution, validation/review results, and finalization. Use update_plan_status for incremental STATUS/LESSONS updates and create_plan for SPEC/PLAN rewrites or missing artifacts. Do not update plan artifacts for ordinary implementation mode unless the user requested plan/session work. -- **Tool choice:** Prefer dedicated tools over shell fallbacks: repository status and configured file-change hooks are runtime-owned and injected automatically; use read_files/read_outline/read_subtree/glob/list_directory/query_index for source inspection, inspect_3d_asset/render_3d_preview for 3D assets, read_image for other screenshots/images, edit_3d_asset for guarded Blender changes, edit_transaction for text project mutations, browser_use/codebuff_local_cli for visual smoke tests, and basher only for commands without a dedicated tool. +- **Tool choice:** Prefer dedicated tools over shell fallbacks: repository status and configured file-change hooks are runtime-owned and injected automatically; use read_files/read_outline/read_subtree/glob/list_directory/query_index for source inspection, inspect_3d_asset/render_3d_preview for 3D assets, read_image for other screenshots/images, edit_3d_asset for guarded Blender changes, edit_transaction for text project mutations, browser_use/codebuff_local_cli for visual smoke tests, and basher only for commands without a dedicated tool. \`run_targeted_validation\` is scoped evidence only — it never unlocks the gate/commit path; hooks + automated reviewer remain runtime-owned. - **Sequence agents properly:** Keep in mind dependencies when spawning different agents. Don't spawn agents in parallel that depend on each other. - **Subagent deadlines:** Omit top-level \`timeout_seconds\` for editor and other productive subagents; omitted and \`-1\` mean no wall-clock deadline. Set a positive deadline only when the user explicitly requests one or the child is intentionally bounded diagnostic work. - **Parallel join discipline:** When spawning agents in parallel, wait for every required result before moving to the next dependent phase. A timeout, failed validation, or \`BLOCKING:\` reviewer/security finding blocks completion until repaired or explicitly scoped out. -- **Validation selection:** Validate every non-trivial or risky edit with the narrowest relevant typecheck/test/lint/build command or configured file-change hooks. Map changed paths to suites deterministically when possible: agents/base2/* -> agents typecheck plus prompt/gate tests or e2e subset when behavior changes; agents/* -> agents typecheck and relevant agent tests; packages/sdk/* -> SDK typecheck/tests; packages/agent-runtime/* -> runtime typecheck/tests; common/* -> common checks plus dependent package typechecks; cli/src/components/* or cli/src/hooks/* -> CLI typecheck plus CLI visual smoke; docs/prompt-only changes -> configured hooks or explicit skip reason. Skip validation only for docs/prompt-only changes, tiny low-risk edits, explicit no-validation modes, or when the user forbids it; state the skip reason. Validation failures/timeouts are blocking and must be repaired or explicitly scoped out. +- **Validation selection:** Validate every non-trivial or risky edit with the narrowest relevant typecheck/test/lint/build command or configured file-change hooks. Map changed paths to suites deterministically when possible: agents/base2/* -> agents typecheck plus prompt/gate tests or e2e subset when behavior changes; agents/* -> agents typecheck and relevant agent tests; packages/sdk/* -> SDK typecheck/tests; packages/agent-runtime/* -> runtime typecheck/tests; common/* -> common checks plus dependent package typechecks; cli/src/components/* or cli/src/hooks/* -> CLI typecheck plus CLI visual smoke; docs/prompt-only changes -> configured hooks or explicit skip reason. Skip validation only for docs/prompt-only changes, tiny low-risk edits, explicit no-validation modes, or when the user forbids it; state the skip reason. Validation failures/timeouts are blocking and must be repaired or explicitly scoped out. Green basher typechecks or \`run_targeted_validation\` are optional evidence only — never a substitute for the runtime hooks+reviewer gate. - **Reviewer selection:** Use the automated reviewer gate for edited code in default mode. Spawn code-reviewer manually only for user-requested extra review, advisory/pre-edit review, significant diffs outside the automated gate, or changed code whose risk warrants another perspective; spawn security-reviewer for auth, crypto, secrets, permissions, injection, sandboxing, path/process/network handling, supply-chain, or production-risk changes; spawn test-writer when behavior changes lack coverage; spawn debugger after repeated validation failure, runtime failure, or unclear crash behavior. Do not duplicate the same post-edit review manually. - **Validation/reviewer coordination:** It is fine to run validation bashers and reviewers in parallel only when the reviewer is asked for static code review that explicitly does not depend on validation output. Always wait for both. Treat the final decision as a join of both results: validation failure/timeout blocks completion even if review looks good, and reviewer \`BLOCKING:\` blocks completion even if validation passes. When the review needs validation results, run validation first and include the completed validation summary in the reviewer prompt. ${buildArray( diff --git a/agents/base2/quality-prompt-section.ts b/agents/base2/quality-prompt-section.ts index 35a337b698..bdf312a91f 100644 --- a/agents/base2/quality-prompt-section.ts +++ b/agents/base2/quality-prompt-section.ts @@ -75,23 +75,25 @@ Never make the user ask explicitly for "use multiple agents" — the scope asses } /** - * Gate-awareness section: tells the orchestrator not to manually spawn - * code-reviewer for the same edited file set that the automated runtime - * gate will review after validation. + * Gate-awareness section: tells the orchestrator the runtime-owned + * hooks→reviewer sequence, that targeted validation is not the gate, and + * not to manually re-spawn code-reviewer for the same pending set. * * NOT byte-frozen — advisory guidance that may evolve with the gate. * * Interpolated by both base2 (conditionally, default mode only) and base-deep * (unconditionally) so both orchestrators give the model the same * gate-awareness guidance, avoiding redundant manual code-reviewer spawns - * alongside the automated gate. + * and basher/targeted-validation substitutes for the gate. */ export const gateAwarenessSection = `# Automated Validation & Review Gate -The runtime automatically runs configured validation hooks and a code-reviewer gate before finalization. To avoid redundant reviewer spawns: +After edits, the runtime-owned path is: configured file-change hooks (\`run_file_change_hooks\`, programmatic / model-hidden — injected when needed) → automated code-reviewer → finalization allowed when green. Wait for that cycle; do not invent a parallel basher typecheck or other substitute as the gate. -- Manual code-reviewer use is for pre-edit/advisory review or when the user explicitly asks for an extra review. Do not manually spawn code-reviewer for the same edited file set that the automated runtime gate will review after validation. -- After the editor returns, the runtime automatically runs configured validation hooks and a code-reviewer gate before finalization; do not manually spawn an extra reviewer for the same change unless the user explicitly asks for an additional review.` +- **Do not double-spawn code-reviewer:** Manual code-reviewer use is for pre-edit/advisory review or when the user explicitly asks for an extra review. Do not manually re-spawn code-reviewer for the same pending set the automated gate will review. If phase is \`awaiting_validation\` / gate not yet passed, wait for the programmatic hooks→reviewer cycle. +- **\`run_targeted_validation\` is NOT the gate:** It is optional scoped evidence only (does not clear reviewer findings by itself). Green targeted validation does **not** clear reviewer findings, does **not** unlock \`git-committer\`, and does **not** replace \`run_file_change_hooks\` + automated reviewer. Basher typechecks are the same class of optional evidence — never a gate substitute. +- **Pending-set authority:** The gate covers the full \`pendingGateFiles\` / pending validation set listed in active-work state — not only the last file you edited. After multi-file edits, the full related set must clear hooks+reviewer before commit. The pending list in active-work / gate-state is authoritative over conversational memory. +- **Commit only after gate green (see Git Discipline):** Spawn \`git-committer\` only after the gate reports passed for the pending files (with \`owned_paths\`). Gate re-arms on every new edit (including multi-file re-touch). Treat "not available yet" as normal ordering; do not tight-loop committer spawns — wait for the passed signal, then spawn once.` /** * Security-review section: advisory pre-edit review for security-sensitive diff --git a/docs/agents-and-tools.md b/docs/agents-and-tools.md index 7f907464d4..6f278226f0 100644 --- a/docs/agents-and-tools.md +++ b/docs/agents-and-tools.md @@ -54,7 +54,7 @@ The root orchestrator has a versioned, local control-plane surface for work that - `get_change_review_bundle` produces snapshot-bound changed-file and diff evidence for reviewers. - `inspect_environment` recursively reports nested JavaScript, Python, Rust, Go, Maven/Gradle, .NET, and Swift workspaces, their manifests/lockfiles, inferred managers with explicit confidence, and locally available toolchains without executing project code. - `get_affected_tests` and `get_build_targets` map changed files to the nearest workspace, existing test candidates, and manager-specific validation/build commands. Targets report `confirmed`, `inferred`, or `unknown` confidence instead of presenting guesses as discovered facts. -- `run_targeted_validation` executes only an explicitly selected target against an expected snapshot and rejects stale or mid-run-mutated snapshots. +- `run_targeted_validation` executes only an explicitly selected target against an expected snapshot and rejects stale or mid-run-mutated snapshots. It is **snapshot-scoped optional evidence only**: green targeted validation does **not** clear reviewer findings, does **not** unlock `git-committer`, and does **not** replace the validation/reviewer gate. - `inspect_codebase_structure` creates the authoritative snapshot-bound audit inventory, including subsystems, entrypoints, routes, commands, public APIs, tests, generated sources, and a detected language/framework capability packet. - `inspect_feature_completeness` follows a claimed feature vertically across runtime wiring, consumers, tests, docs, and failure-state evidence. - `write_audit_findings` returns a snapshot-bound `structuralReceipt` when the shard supplies the exact snapshot and explicit domain coverage, and `inspect_feature_completeness` returns a `coverageReceipt`; pass those receipts directly to `evaluate_audit_coverage` instead of reconstructing them from prose or count summaries. Feature receipts remain heuristic until their cited files are verified with exact reads. @@ -66,8 +66,8 @@ Specialists receive only the read intelligence their role requires. For example, Control-plane records use compare-and-swap revisions, atomic local persistence, content hashes, expiring verified knowledge, single-use approvals, ownership receipts, and exclusive workspace leases. High-impact operations and external connector mutations are classified centrally rather than authorized by prompt wording alone. -`run_file_change_hooks` runs user-configured hooks and, only when a trusted -project explicitly sets `autoFileChangeHooks: true`, combines them with bounded +`run_file_change_hooks` + the automated code-reviewer are the **validation/reviewer gate** that unlocks finalization and commit. Hooks run user-configured checks and, only when a trusted +project explicitly sets `autoFileChangeHooks: true`, combine them with bounded manifest inference. The opt-in is required because compilers, plugins, build scripts, and tests can execute repository-controlled code. Inference prefers explicit project scripts and supports native checks for JavaScript/TypeScript, Python, Rust, Go, Java/Kotlin, .NET, C/C++, Ruby, PHP, @@ -77,7 +77,7 @@ original bounded stdout/stderr remains available for recovery. - Automated security/test/doc auxiliary agents have explicit lifecycle handling. Their done flags are written only after successful completion; crashes and blocking security verdicts persist as blockers. Test/doc writers run automatically only when the user request explicitly includes those deliverables, and mixed-package test targets are routed to package-specific commands. - Productive agent steps and subagent wall-clock duration are unlimited by default. A repeated-step watchdog stops identical no-progress loops, while cancellation, explicitly configured subagent deadlines, cost/token budgets, spawn-depth limits, and context compaction remain independent safeguards. Users may still configure a positive `maxAgentSteps`, agent-template `defaultTimeoutMs`, or per-spawn `timeout_seconds` cap; `-1` explicitly selects unlimited mode. Reviewer crashes retry once; repeated crashes require the explicit user phrase `bypass reviewer gate` before finalization can continue. -- Root-orchestrator mutating/control gate operations such as Git-status observation, file-change hooks, and structural inventory are model-hidden programmatic tools. Their results are injected when needed, so the harness remains active without paying for those schemas on every provider request. The read-only `get_change_review_bundle` tool remains model-visible so an orchestrator can refresh a stale reviewer snapshot after compaction. Fresh greetings and simple gratitude prompts take a narrow conversational fast path only when no pending work or reviewer blocker exists. +- Root-orchestrator mutating/control gate operations such as Git-status observation, file-change hooks, and structural inventory are model-hidden programmatic tools. Their results are injected when needed after edits, so the harness remains active without paying for those schemas on every provider request. The orchestrator must **not** treat basher typechecks or `run_targeted_validation` as gate substitutes — only the runtime-owned hooks→reviewer cycle clears the gate. The read-only `get_change_review_bundle` tool remains model-visible so an orchestrator can refresh a stale reviewer snapshot after compaction. Fresh greetings and simple gratitude prompts take a narrow conversational fast path only when no pending work or reviewer blocker exists. **Pattern-specific agents** are intentionally **excluded** from `spawnableAgents` because they have a narrow contract that only makes sense within a specific workflow pattern. They are spawned by the pattern flow itself, not by the orchestrator: diff --git a/docs/request-flow.md b/docs/request-flow.md index 5a7d1724ac..555732ae6a 100644 --- a/docs/request-flow.md +++ b/docs/request-flow.md @@ -76,7 +76,7 @@ through a hosted Openbuff/Codebuff service in this primary flow. - `inspect_workspace`, `get_task`, `get_change_review_bundle` → snapshot-bound workspace/task/review evidence - `inspect_environment`, `get_affected_tests`, `get_build_targets` → read-only toolchain and validation-target intelligence - `inspect_codebase_structure`, `inspect_feature_completeness`, `evaluate_audit_coverage` → snapshot-bound broad-audit inventory and completeness gating - - `run_targeted_validation` → snapshot-checked scoped validation + - `run_targeted_validation` → snapshot-checked scoped validation (scoped evidence only; not the full hooks+reviewer gate) - Custom tool definitions and MCP tools 6. **Action handlers** stream provider output back to the CLI: - `response-chunk` → streams text to the CLI