diff --git a/src/adapters/agenticLoop.test.ts b/src/adapters/agenticLoop.test.ts index 3179480a..b6f522be 100644 --- a/src/adapters/agenticLoop.test.ts +++ b/src/adapters/agenticLoop.test.ts @@ -251,6 +251,71 @@ describe('runAgenticLoop nudge budgets (INT-1925)', () => { }); }); +describe('runAgenticLoop finishValidator (AGT-4300)', () => { + it('rejects a would-be final answer and continues the SAME conversation instead of returning', async () => { + const calls: number[] = []; + let callApiInvocations = 0; + const callApi = async (messages: ChatMessage[]) => { + callApiInvocations++; + // Every call after the first sees the growing history — proof this is + // one continuous session, not a caller restarting a fresh conversation + // per attempt (the whole point of the fix: keep the warm KV-cache prefix). + calls.push(messages.length); + return finalResp(callApiInvocations === 1 ? 'thin answer' : 'faithful answer'); + }; + const finishValidator = vi.fn(async (finalText: string) => { + if (finalText === 'thin answer') return { ok: false as const, nudge: 'try harder' }; + return { ok: true as const }; + }); + + const result = await runAgenticLoop({ + prompt: 'draft this task', cwd: process.cwd(), model: 'test', callApi, + maxTurns: 10, webTools: false, + finishValidator, finishValidatorMaxRetries: 1, + }); + + expect(callApiInvocations).toBe(2); // one rejected, one accepted — no restart + expect(finishValidator).toHaveBeenCalledTimes(2); + expect(finishValidator).toHaveBeenNthCalledWith(1, 'thin answer', 1); + expect(finishValidator).toHaveBeenNthCalledWith(2, 'faithful answer', 2); + expect(result.text).toBe('faithful answer'); + // The second call's message array is longer than the first's — the nudge + // was appended as a new turn on the SAME messages array, not a fresh one. + expect(calls[1]).toBeGreaterThan(calls[0]); + }); + + it('gives up and returns the last candidate once finishValidatorMaxRetries is exhausted', async () => { + let callApiInvocations = 0; + const callApi = async () => { + callApiInvocations++; + return finalResp('always thin'); + }; + const finishValidator = vi.fn(async () => ({ ok: false as const, nudge: 'try harder' })); + + const result = await runAgenticLoop({ + prompt: 'draft this task', cwd: process.cwd(), model: 'test', callApi, + maxTurns: 10, webTools: false, + finishValidator, finishValidatorMaxRetries: 1, + }); + + // maxRetries=1 allows exactly one rejection before the loop stops asking + // the validator and returns the last text as-is, rather than looping + // forever on a model that never satisfies the gate. + expect(callApiInvocations).toBe(2); // one rejected round, one accepted-by-exhaustion round + expect(finishValidator).toHaveBeenCalledTimes(2); + expect(result.text).toBe('always thin'); + }); + + it('does not call finishValidator at all when unset (default off)', async () => { + const callApi = async () => finalResp('plain answer'); + const result = await runAgenticLoop({ + prompt: 'do a thing', cwd: process.cwd(), model: 'test', callApi, + maxTurns: 10, webTools: false, + }); + expect(result.text).toBe('plain answer'); + }); +}); + describe('runAgenticLoop warehouse discovery (AGT-4128)', () => { it('points every tool-loop worker at the warehouse index before it asks for local-only data', async () => { let firstMessages: ChatMessage[] = []; diff --git a/src/adapters/agenticLoop.ts b/src/adapters/agenticLoop.ts index 6e82f2cf..322c1120 100644 --- a/src/adapters/agenticLoop.ts +++ b/src/adapters/agenticLoop.ts @@ -12,7 +12,7 @@ import { WEB_TOOL_DEFINITIONS } from './webTools.js'; import { detectRateLimit, RateLimitError } from './rateLimitError.js'; import { isInfraError } from './errorClassification.js'; import { parseSearchReplaceBlocks, applyEditBlock, type EditFormat } from '../support/editParser.js'; -import type { CliRunResult } from './types.js'; +import type { CliRunResult, FinishValidation } from './types.js'; import type { ChatUsage } from './chatStream.js'; import { recordUsage, type UsageAttribution } from '../support/usageLedger.js'; import { COORDINATION_TOOL_DEFINITIONS, type CoordinationToolContext } from '../coordination/coordinationTools.js'; @@ -132,6 +132,20 @@ export interface AgenticLoopOptions { * 기본 0 (비활성) — 수정 없는 작업(진단·분석)도 정상이므로 옵트인. */ nudgeMaxOnNoEdit?: number; + /** + * Gate on the loop's finish itself, not just on whether it edited a file. + * Called with the model's would-be final answer before the loop returns; + * `{ ok: false, nudge }` appends the nudge as a new user turn and + * `continue`s the SAME conversation instead of returning. This is what + * lets a caller reject an insufficient answer (e.g. draftAnalyzer's hard + * gate) without restarting a fresh `spawnCli` call, which used to throw + * away the provider's warm KV-cache prefix on every retry — measured on + * vela: the retry's first call landed at 0.3% cache against 95%+ for the + * calls around it (AGT-4300). Bounded by `finishValidatorMaxRetries`. + */ + finishValidator?: (finalText: string, attempt: number) => FinishValidation | Promise; + /** Max times `finishValidator` may reject an answer before the loop gives up and returns it anyway. Default 0. */ + finishValidatorMaxRetries?: number; /** Verification-harness files for which edit/write are refused (see tools.ts ToolExecOptions) */ protectedFiles?: string[]; /** bash tool timeout — docker-based tests need minutes (default 30s) */ @@ -241,6 +255,8 @@ export async function runAgenticLoop(options: AgenticLoopOptions): Promise FinishValidation | Promise; + /** Max times `finishValidator` may reject an answer before the loop gives up and returns it anyway. Default 0. */ + finishValidatorMaxRetries?: number; } /** diff --git a/src/agents/draftAnalyzer.test.ts b/src/agents/draftAnalyzer.test.ts index fa5d02a9..4193472d 100644 --- a/src/agents/draftAnalyzer.test.ts +++ b/src/agents/draftAnalyzer.test.ts @@ -105,36 +105,41 @@ describe('runDraftAnalysis fallback', () => { ).rejects.toBeInstanceOf(RateLimitError); }); - it('drafter hard gate: retries on the same adapter when the brief is insufficient', async () => { + it('drafter hard gate: retries in the SAME spawnCli call when the brief is insufficient (AGT-4300)', async () => { + // The gate retry used to be a caller-side loop that called spawnCli again — + // a fresh conversation that threw away the just-warmed provider cache + // (measured on vela: the retry's first call landed at 0.3% cache against + // 95%+ around it). It now retries INSIDE one spawnCli call via + // finishValidator, which the real agenticLoop invokes with each would-be + // final answer; this mock plays that same role so the test exercises the + // actual wiring (finishValidatorMaxRetries, the nudge, the attempt count) + // rather than asserting a call count that no longer means what it used to. vi.spyOn(adapterModule, 'getDefaultAdapterName').mockReturnValue('codex'); vi.spyOn(adapterModule, 'getAdapter').mockReturnValue(makeAdapter('codex')); - vi.spyOn(adapterModule, 'spawnCli') - // attempt 1: thin brief (no completionCriteria) → insufficient → retry - .mockResolvedValueOnce({ - exitCode: 0, - stdout: JSON.stringify({ - taskType: 'feature', - intentSummary: 'do the thing', - relevantFiles: [], - suggestedApproach: 'figure it out', - }), - stderr: '', - durationMs: 1, - } as CliRunResult) - // attempt 2: faithful brief with execution-grounded criteria → sufficient - .mockResolvedValueOnce({ - exitCode: 0, - stdout: JSON.stringify({ - taskType: 'feature', - intentSummary: 'Wire the resolver into the streaming path', - relevantFiles: ['src/streaming.ts'], - suggestedApproach: 'call resolve_turn_model from build_request', - completionCriteria: ['resolve_turn_model invoked from streaming.ts (call site cited)'], - }), - stderr: '', - durationMs: 1, - } as CliRunResult); + const thinBrief = JSON.stringify({ + taskType: 'feature', + intentSummary: 'do the thing', + relevantFiles: [], + suggestedApproach: 'figure it out', + }); + const faithfulBrief = JSON.stringify({ + taskType: 'feature', + intentSummary: 'Wire the resolver into the streaming path', + relevantFiles: ['src/streaming.ts'], + suggestedApproach: 'call resolve_turn_model from build_request', + completionCriteria: ['resolve_turn_model invoked from streaming.ts (call site cited)'], + }); + + const spawnCliSpy = vi.spyOn(adapterModule, 'spawnCli').mockImplementation(async (_adapter, options) => { + expect(options.finishValidatorMaxRetries).toBeGreaterThan(0); + const first = await options.finishValidator!(thinBrief, 1); + expect(first.ok).toBe(false); + expect(first.nudge).toBeTruthy(); + const second = await options.finishValidator!(faithfulBrief, 2); + expect(second.ok).toBe(true); + return { exitCode: 0, stdout: faithfulBrief, stderr: '', durationMs: 1 } as CliRunResult; + }); const result = await runDraftAnalysis({ taskTitle: 'Wire resolver', @@ -142,28 +147,74 @@ describe('runDraftAnalysis fallback', () => { projectPath: '/tmp/project', }); - expect(adapterModule.spawnCli).toHaveBeenCalledTimes(2); // gate retry on same adapter + expect(spawnCliSpy).toHaveBeenCalledTimes(1); // one conversation, not a fresh one per attempt expect(result.sufficient).toBe(true); expect(result.completionCriteria).toHaveLength(1); }); - it('drafter hard gate: marks insufficient when retries still yield a thin brief', async () => { + it('drafter hard gate: marks insufficient when the finish gate exhausts its retries on a thin brief', async () => { vi.spyOn(adapterModule, 'getDefaultAdapterName').mockReturnValue('codex'); vi.spyOn(adapterModule, 'getAdapter').mockReturnValue(makeAdapter('codex')); - // Always thin → never passes the gate. - vi.spyOn(adapterModule, 'spawnCli').mockResolvedValue({ - exitCode: 0, - stdout: JSON.stringify({ taskType: 'feature', intentSummary: 'x', relevantFiles: [], suggestedApproach: 'y' }), - stderr: '', - durationMs: 1, - } as CliRunResult); + + const thinBrief = JSON.stringify({ taskType: 'feature', intentSummary: 'x', relevantFiles: [], suggestedApproach: 'y' }); + + const spawnCliSpy = vi.spyOn(adapterModule, 'spawnCli').mockImplementation(async (_adapter, options) => { + const maxRetries = options.finishValidatorMaxRetries ?? 0; + let verdict = await options.finishValidator!(thinBrief, 1); + for (let attempt = 2; !verdict.ok && attempt <= maxRetries + 1; attempt += 1) { + verdict = await options.finishValidator!(thinBrief, attempt); + } + // Retries exhausted without an `ok` verdict — the real loop returns the + // last candidate text as-is rather than looping forever. + return { exitCode: 0, stdout: thinBrief, stderr: '', durationMs: 1 } as CliRunResult; + }); const result = await runDraftAnalysis({ taskTitle: 'T', taskDescription: 'D', projectPath: '/tmp/project' }); - expect(adapterModule.spawnCli).toHaveBeenCalledTimes(2); // exhausts gate retries + expect(spawnCliSpy).toHaveBeenCalledTimes(1); expect(result.sufficient).toBe(false); }); + it('falls back to one fresh retry when finishValidator never got its full quota (AGT-4300 layer-2 fix)', async () => { + // finishValidator only works on adapters routed through runAgenticLoop + // (openrouter/gpt/local/atlascloud), and even there a run that exhausts + // its turn budget while still exploring never reaches the no-tool-calls + // branch where finishValidator lives. Either way, the first spawnCli call + // here behaves like those cases: it never calls options.finishValidator + // at all (as codex/claude/cursor/codex-responses/cc-router would) and just + // returns a thin brief. Without this fallback, the hard gate would give + // this adapter fewer real chances than the two it is supposed to get. + vi.spyOn(adapterModule, 'getDefaultAdapterName').mockReturnValue('codex'); + vi.spyOn(adapterModule, 'getAdapter').mockReturnValue(makeAdapter('codex')); + + const thinBrief = JSON.stringify({ taskType: 'feature', intentSummary: 'x', relevantFiles: [], suggestedApproach: 'y' }); + const faithfulBrief = JSON.stringify({ + taskType: 'feature', + intentSummary: 'Wire the resolver into the streaming path', + relevantFiles: ['src/streaming.ts'], + suggestedApproach: 'call resolve_turn_model from build_request', + completionCriteria: ['resolve_turn_model invoked from streaming.ts (call site cited)'], + }); + + const spawnCliSpy = vi.spyOn(adapterModule, 'spawnCli') + // First call: never invokes options.finishValidator — simulates an + // adapter that doesn't route through runAgenticLoop at all. + .mockResolvedValueOnce({ exitCode: 0, stdout: thinBrief, stderr: '', durationMs: 1 } as CliRunResult) + // Fallback fresh retry: a faithful brief this time. + .mockResolvedValueOnce({ exitCode: 0, stdout: faithfulBrief, stderr: '', durationMs: 1 } as CliRunResult); + + const result = await runDraftAnalysis({ taskTitle: 'T', taskDescription: 'D', projectPath: '/tmp/project' }); + + expect(spawnCliSpy).toHaveBeenCalledTimes(2); + // The fallback call's prompt carries the same retry nudge the old + // per-attempt loop used to append, so weaker/non-loop adapters still get + // a stricter second try, not just a repeat of the first prompt. + const fallbackCallArgs = spawnCliSpy.mock.calls[1][1]; + expect(fallbackCallArgs.prompt).toContain('Your previous brief was insufficient'); + expect(result.sufficient).toBe(true); + expect(result.completionCriteria).toHaveLength(1); + }); + it('does not fallback on non-quota failures', async () => { vi.spyOn(adapterModule, 'getDefaultAdapterName').mockReturnValue('codex'); vi.spyOn(adapterModule, 'getAdapter').mockReturnValue(makeAdapter('codex')); diff --git a/src/agents/draftAnalyzer.ts b/src/agents/draftAnalyzer.ts index 4203e5e4..9f6cb4f0 100644 --- a/src/agents/draftAnalyzer.ts +++ b/src/agents/draftAnalyzer.ts @@ -679,42 +679,90 @@ export async function runDraftAnalysis(options: DraftAnalyzerOptions): Promise { + lastAttemptNumber = attempt; + const parsed = parseDraftResponse(finalText); + const sufficient = isDraftSufficient(parsed); + onLog?.(`[Draft] ${adapterName}(${resolvedModel}) attempt ${attempt}: type=${parsed.taskType}, files=${parsed.relevantFiles?.length ?? 0}, criteria=${parsed.completionCriteria?.length ?? 0}, sufficient=${sufficient}`); + if (sufficient) return { ok: true }; + onLog?.('[Draft] Brief insufficient — retrying with a stricter prompt'); + return { ok: false, nudge: DRAFT_RETRY_NUDGE }; + }, + }); + + haikuResult = parseDraftResponse(raw.stdout); + succeeded = true; + draftSufficient = isDraftSufficient(haikuResult); + if (lastAttemptNumber === 0) { + // finishValidator never fired (no-tool-calls path never reached, e.g. the + // maxTurns-exhaustion salvage answered instead) — log once so this attempt + // is still visible. + onLog?.(`[Draft] ${adapterName}(${resolvedModel}): type=${haikuResult.taskType}, files=${haikuResult.relevantFiles?.length ?? 0}, criteria=${haikuResult.completionCriteria?.length ?? 0}, sufficient=${draftSufficient}`); + } + if (draftSufficient) break outer; + + // Fallback for the in-session retry NOT actually happening — either the + // adapter doesn't route through runAgenticLoop at all (codex, claude, + // cursor, codex-responses, cc-router: subprocess/CLI-delegate or a + // shared loop this diff didn't wire finishValidator into), or the model + // spent its whole turn budget exploring before its first would-be final + // answer and the maxTurns-exhaustion salvage answered instead of the + // no-tool-calls branch where finishValidator lives — either way + // `lastAttemptNumber` never reaches DRAFT_MAX_ATTEMPTS, meaning the + // brief got fewer real chances than the hard gate is supposed to give + // it. Restore the old safety net with exactly one extra fresh call, + // rather than silently shipping a thin brief on adapters this diff + // can't make cache-friendly. (Found in layer-2 review, AGT-4300.) + if (lastAttemptNumber < DRAFT_MAX_ATTEMPTS) { + onLog?.(`[Draft] ${adapterName}(${resolvedModel}): in-session retry unavailable or exhausted by turn budget — one fresh retry`); + const fallbackRaw = await spawnCli(adapter, { + prompt: prompt + DRAFT_RETRY_NUDGE, + cwd: options.projectPath, + timeoutMs: draftTimeoutMs, model: resolvedModel, - maxTurns: budget.maxTurns, // size-adaptive: 3 ran out reading a real repo before emitting the brief (INT-2485) + maxTurns: budget.maxTurns, processContext: { taskId: options.taskId ?? options.taskTitle, stage: 'draft' }, }); - - haikuResult = parseDraftResponse(raw.stdout); - succeeded = true; - draftSufficient = isDraftSufficient(haikuResult); - onLog?.(`[Draft] ${adapterName}(${resolvedModel}) attempt ${attempt}: type=${haikuResult.taskType}, files=${haikuResult.relevantFiles?.length ?? 0}, criteria=${haikuResult.completionCriteria?.length ?? 0}, sufficient=${draftSufficient}`); + const fallbackResult = parseDraftResponse(fallbackRaw.stdout); + const fallbackSufficient = isDraftSufficient(fallbackResult); + onLog?.(`[Draft] ${adapterName}(${resolvedModel}) fresh retry: type=${fallbackResult.taskType}, files=${fallbackResult.relevantFiles?.length ?? 0}, criteria=${fallbackResult.completionCriteria?.length ?? 0}, sufficient=${fallbackSufficient}`); + haikuResult = fallbackResult; + draftSufficient = fallbackSufficient; if (draftSufficient) break outer; - if (attempt < DRAFT_MAX_ATTEMPTS) { - onLog?.('[Draft] Brief insufficient — retrying with a stricter prompt'); - } - } catch (err) { - // A typed rate limit must NOT be swallowed into a best-effort draft — it - // has to reach the pipeline so the scheduler pauses instead of the planner - // + worker continuing to hammer the exhausted provider. (INT-2521) - if (err instanceof RateLimitError) throw err; - lastError = err; - const errMsg = err instanceof Error ? err.message : String(err); - onLog?.(`[Draft] analysis failed (${adapterName}): ${errMsg}`); - - if (!isFallbackAttempt && isProviderQuotaError(errMsg) && adaptersToTry.length > 1) { - break; // break inner → try the fallback adapter - } - // Non-quota failure: stop entirely, continue pipeline with best-effort data. - break outer; } + } catch (err) { + // A typed rate limit must NOT be swallowed into a best-effort draft — it + // has to reach the pipeline so the scheduler pauses instead of the planner + // + worker continuing to hammer the exhausted provider. (INT-2521) + if (err instanceof RateLimitError) throw err; + lastError = err; + const errMsg = err instanceof Error ? err.message : String(err); + onLog?.(`[Draft] analysis failed (${adapterName}): ${errMsg}`); + + if (!isFallbackAttempt && isProviderQuotaError(errMsg) && adaptersToTry.length > 1) { + continue; // try the fallback adapter + } + // Non-quota failure: stop entirely, continue pipeline with best-effort data. + break outer; } // Got a response from this adapter (sufficient or best-effort after retries). // Only quota errors (which leave succeeded=false) cascade to the fallback