fix(draft-cache): retry the hard gate in the same conversation instead of a fresh spawnCli call (AGT-4300) - #621
Merged
Merged
Conversation
…d of a fresh spawnCli call (AGT-4300) draftAnalyzer's hard gate used to retry an insufficient draft brief by calling spawnCli AGAIN with a stricter prompt -- a structurally fresh conversation. Measured on vela post-fingerprint-fix: that retry's first call lands at 0.3% prompt-cache hit against 95%+ for the calls around it, reproducibly (4/4 in a small sample), correlated exactly with the attempt boundary. Root cause of why a fresh conversation loses cache on this provider wasn't conclusively pinned (isolated reproductions of "fresh conversation, identical/prefix-matching content" all cached near-perfectly), so this sidesteps the mechanism instead of chasing it: retry in place. Adds `finishValidator`/`finishValidatorMaxRetries` to CliRunOptions and the shared agenticLoop.ts (openrouter/gpt/local/atlascloud). When the loop is about to return a final answer with no more tool calls, the validator gets a look; rejecting appends a nudge as a new user turn and continues the SAME messages array instead of the caller starting over, modeled directly on the existing no-edit-guard pattern (nudgeMaxOnNoEdit). draftAnalyzer.ts's DRAFT_MAX_ATTEMPTS loop is replaced by one spawnCli call using this hook. Layer-2 review caught a real gap in the first draft of this fix: the mechanism only works for adapters routed through runAgenticLoop, so codex/claude/cursor/codex-responses/cc-router (and the loop's own turn-budget-exhaustion salvage path, which bypasses the no-tool-calls branch entirely) would have silently gotten ZERO retries instead of a colder one. Fixed with an explicit fallback: if the validator never reached its full DRAFT_MAX_ATTEMPTS quota (tracked via the attempt number it's called with), draftAnalyzer does exactly one more old-style fresh retry, restoring the historical guarantee universally. Round-2 re-review confirmed no findings survive. TSC=0, LINT=0, BUILD=0, full vitest suite green (one unrelated fileLock.test.ts flake confirmed by isolated rerun, not touched by this diff). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
draftAnalyzer.ts's hard-gate retry (thin brief → retry) used to callspawnCliagain — a structurally new conversation. Measured on vela post-fingerprint-fix: that retry's first call lands at 0.3% cache against 95%+ around it, reproducibly.finishValidator/finishValidatorMaxRetriestoCliRunOptionsand the sharedagenticLoop.ts(openrouter/gpt/local/atlascloud), modeled on the existing no-edit-guard pattern: rejecting a would-be final answer appends a nudge as a new turn on the SAME conversation instead of the caller restarting.draftAnalyzer.ts'sDRAFT_MAX_ATTEMPTSloop replaced by onespawnClicall using this hook.runAgenticLoop, so codex/claude/cursor/codex-responses/cc-router (and the loop's own turn-budget-exhaustion salvage path) would have silently gotten zero retries. Fixed with an explicit fallback that restores the historical "every adapter getsDRAFT_MAX_ATTEMPTSreal chances" guarantee. Round-2 re-review found no further issues.Test plan
npx tsc --noEmit/ lint / build / full vitest suite green (one unrelatedfileLock.test.tsflake confirmed by isolated rerun)draftstage cache rate (part of the standing 80%-cache goal)🤖 Generated with Claude Code