fix(agent): recover front-loaded answer past a citations-only terminal turn #11
Reference in New Issue
Block a user
Delete Branch "fix/finalize-citations-only-terminal"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem (mort issue #1418)
finalOutputalready recovers a front-loaded answer when the terminal(no-tool-call) turn is empty or a short back-reference (
"(Already answered above.)"). But some open-weight models (observed:glm-5.2driving acitetool) front-load the full prose answer into an earlier tool-call turn and
close with a terminal turn that is only their sources:
That ~400-char list is neither empty nor a back-reference, so it was returned
verbatim and the real answer was discarded — the user got a bare sources
list; the good answer was only visible in the run trace.
Fix
Add
isCitationsOnly: a terminal that OPENS with a sources/citationsheading, carries a link, and — once the heading and links are stripped — is
dominated by that citation structure (little prose remains). Unlike a
back-reference, the citations are real content, so
finalOutputrecovers theprior substantive answer and appends the citations below it (deduped against
a front-loaded turn that already carried them).
Key robustness decisions (each has a regression test):
merely opens with
Source: … http://…keeps most of its text after strippingthe URL, so it is left as the answer (not demoted).
^-anchored heading so an answer that ends with aSources:section,or mentions "sources" mid-sentence, never matches.
concise (<200-char) front-loaded answer is still recovered against a long
citations list (the old ratio arm was structurally unsatisfiable there).
preambleRenow applies regardless of length, so a long planningnarration ("I'll look up …") is never recovered as the answer.
both (e.g.
References: as noted above, [x](url)) keeps its links.-,##,**) and a colonafter closing emphasis (
**Sources**:).Healthy terminal answers are unchanged; zero extra model calls (pure
transcript recovery). Applies to every agent + skill run via
agent.Run.Known limitation
A citations terminal whose sources are bare domains (no scheme, no markdown
link — e.g.
Sources: pcprice.watch, ebay.com) is intentionally out of scope:there's no reliable link signal, so it's left as-is rather than risk
misclassifying prose. In practice the
cite/front-load pattern emits realmarkdown links.
Tests
TestIsCitationsOnly— 19 cases (bold / ATX-heading / dash / back-ref-plus-linkspositives; source-led prose, bare-domain, prose-then-sources, midsentence
negatives).
TestFinalOutput— 4 new cases: concise-answer + long-citations recovery,source-led answer not hijacked, long-preamble skipped, dedup.
TestRun_RecoversFrontLoadedAnswerWithCitations— end-to-end #1418 repro.go test -race ./agent/green;go build ./...,go vet,gofmt -lclean.The heuristic was adversarially pre-reviewed (multi-lens: regression /
regex / control-flow / test-coverage, each finding independently verified) before
this PR; the surviving real findings above are what the design addresses.
Fixes mort #1418.
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
finalOutput already recovered a front-loaded answer when the terminal (no-tool-call) turn was empty or a short back-reference. But some models (glm-5.2 via a "cite" tool) front-load the prose answer into an earlier tool-call turn and close with a terminal turn that is ONLY their sources ("Sources: [pcprice.watch](url) (...), [ebay](url) (...)"). That list is neither empty nor a back-reference, so it was returned verbatim and the real answer was discarded (mort issue #1418: the user got a bare sources list; the good answer was only visible in the trace). Add isCitationsOnly: a terminal that OPENS with a sources/citations heading, carries a link, and is dominated by that citation structure (once heading+links are stripped, little prose remains). Unlike a back-reference the citations are real content, so finalOutput recovers the prior substantive answer and APPENDS the citations below it (deduped). The dominance check keeps a real prose answer that merely opens with "Source: ... http://..." from being misclassified; the ^ anchor keeps an answer that ends with a Sources section from matching; the citations branch is decoupled from the terminal-length ratio so a CONCISE front-loaded answer is still recovered; and preambleRe now applies regardless of length so planning narration is never recovered. Healthy terminal answers are unchanged; zero extra model calls. Tests: TestIsCitationsOnly (19 cases incl. bold/ATX/backref/prose negatives), new TestFinalOutput cases (concise+long-citations, source-led-not-hijacked, long-preamble-skipped, dedup), and end-to-end TestRun_RecoversFrontLoadedAnswerWithCitations. Full agent suite green (-race). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>🪰 Gadfly — live review status
6/6 reviewers finished · updated 2026-07-10 15:35:05Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ donedeepseek-v4-pro:cloud· ollama-cloud — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneqwen3.5:397b-cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 7 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -23,3 +36,3 @@// (the loop appends it before calling this); terminal is that message's text.func finalOutput(msgs []llm.Message, terminal string) string {if !isWeakFinal(terminal) {citations := isCitationsOnly(terminal)⚪ citations boolean threaded through three functions — mild boolean-parameter smell
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -32,0 +47,4 @@// Preserve the citations addendum below the recovered answer, unless the// recovered turn already carries it verbatim (guards against a duplicate// sources block when the front-loaded turn included its own citations).if tail := strings.TrimSpace(terminal); !strings.Contains(rec, tail) {🟡 citations dedup uses raw substring Contains(rec, tail); can drop or duplicate citations on minor text differences
correctness · flagged by 1 model
agent/finalize.go:50— citations dedup is a raw substringContains, which can both over- and under-trigger. Confirmed at lines 46-53:tail := strings.TrimSpace(terminal)is checked viastrings.Contains(rec, tail). If the recovered answer contains the exact sources string as a substring in a different context, the citations are silently dropped; conversely if the prior turn's citations differ by even trailing punctuation/whitespace from the terminal's, the block is appended twice. L…🪰 Gadfly · advisory
@@ -43,0 +71,4 @@// (** / __) plus whitespace between the label and the colon/dash separator.// Anchored at ^ so a normal answer that merely mentions "sources" mid-sentence,// or ends with a "Sources:" section AFTER its prose, is never matched.var citationLabelRe = regexp.MustCompile(`(?i)^[\s>#*_+-]*(sources?|references?|citations?|works cited|further reading)\b[\s*_]*[::\-—]`)⚪ citationLabelRe character class includes '+' without mentioning it in the comment
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -43,0 +81,4 @@// citationResidueCutset is trimmed from the ends of a citations terminal's// non-link remainder before measuring it — list bullets, separators, and the// short per-source annotations models add in parentheses.const citationResidueCutset = " \t\r\n,;.:|·•*_()[]—-"⚪ citationResidueCutset declared as standalone const instead of in the const block immediately below
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -92,0 +170,4 @@// accepted; a shorter one must clear a floor and, unless the terminal is a// citations addendum (not a rival answer, so its length is irrelevant), also// clearly dwarf the terminal.func isSubstantiveAnswer(txt, terminal string, citations bool) bool {🟠 preambleRe now rejects long front-loaded answers that open with common conversational phrases (Let me/Okay/I'll/Sure,), regressing back-ref and citations recovery
correctness, error-handling · flagged by 2 models
agent/finalize.go:173-176— preamble check now rejects legitimate long answers that open with conversational phrases (regression in the recovery path). Confirmed:isSubstantiveAnswer(lines 173-183) now runspreambleRe.MatchString(txt)before thelen(txt) >= recoverMinCharsshortcut, so the preamble rejection applies "regardless of length" — a change from the prior behavior where a ≥200-byte turn was accepted without a preamble test.preambleRe(line 64) is `(?i)^(let me|let'?s…🪰 Gadfly · advisory
@@ -39,0 +45,4 @@{"sources-md-links", "Sources: [pcprice.watch](https://pcprice.watch/x), [ebay](https://ebay.com/1).", true},{"lowercase-bare-url", "sources: see https://example.com/a", true},{"bold-label-colon-inside", "**Sources:** [a](https://a), [b](https://b)", true},{"bold-label-colon-outside", "**Sources**: [a](https://a), [b](https://b)", true}, // #4: colon after the bold⚪ // #N: annotations in TestIsCitationsOnly don't match table positions and reference an external review document
maintainability · flagged by 1 model
agent/finalize_test.go:48,:52,:55,:60,:63🪰 Gadfly · advisory
@@ -182,0 +313,4 @@// the recovered answer with the citations appended (not the bare sources list),// with no extra model call.func TestRun_RecoversFrontLoadedAnswerWithCitations(t *testing.T) {longAnswer := strings.TrimSpace(strings.Repeat("Free group calls are capped at sixty minutes. ", 6))⚪ longAnswer defined identically for the third time; a package-level var or helper would avoid the duplication
maintainability · flagged by 1 model
agent/finalize_test.go:316🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 6 models
Verdict: Minor issues · 7 findings (1 with multi-model agreement)
agent/finalize.go:1736 single-model findings (lower confidence)
agent/finalize.go:50agent/finalize.go:38agent/finalize.go:74agent/finalize.go:84agent/finalize_test.go:48agent/finalize_test.go:316Per-model detail
claude-code/opus (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed
agent/finalize.gothrough the security lens. This is a pure in-memory transcript-recovery change: it inspects already-received LLM output strings, matches them against static regexes, and returns a string that becomesresult.Output(verified atagent/agent.go:319). No new trust boundary is crossed.Security-relevant checks I performed:
citationLabelRe,linkRe) compile viaregexp.MustCompile, i.e. Go's RE2 engine, which guarantees linear-time matching with no catastrophic backtracking. Even the unbounded\S+/[^)]*/[^\]]*subexpressions are safe against adversarial model output under RE2. Verified:finalize.go:74,79.isCitationsOnlyextracts are only counted/measured and stripped for a length heuristic (finalize.go:134-139); they are never dereferenced, fetched, opened, or executed. No SSRF, no path/command/template injection is introduced. The recovered prior-turn text and appended citations flow straight back to the caller as an opaque string.lastSubstantiveAssistantText), but it only ever selects text the model already produced in this same run's transcript; it does not reach into tool results, system prompts, or credentials. No secret/credential leakage path.Nothing in the security lens is materially wrong here. (Any concerns about the correctness of the heuristic — e.g. mis-classifying prose, the
len(t)/divisorinteger-division edge — belong to the correctness lens and are out of scope for me.)🎯 Correctness — Minor issues
I've verified the finding against the actual code at
agent/finalize.go.Confirmed: The diff's "before" state of
isSubstantiveAnswerreturnedtrueunconditionally for anylen(txt) >= recoverMinChars(200) turn, with thepreambleRecheck applied only in the borderline< 200band. The "after" state (lines 173–184) movesif preambleRe.MatchString(txt) { return false }to the very top, ahead of the length shortcut.preambleRe(line 64) includesfirst[, ],sure[,. ], andokay[,. ], so a genuine ≥200-char enumerated answer opening with"First, "/"Sure, "/"Okay, "is now rejected during recovery where it was previously accepted. The consequence and line reference are accurate. Finding survives.VERDICT: Minor issues
The change is well-tested and the core citations heuristic is sound — the dominance check (
len(residue) <= len(t)/3) holds against the test strings, including the #1418 annotated-multi-source shape. Signatures are consistent with the sole caller, andterminalis correctly the last-appended message's text. One genuine behavioral concern:agent/finalize.go:174— preamble filter now discards genuine long answers that merely open with a preamble word. By movingif preambleRe.MatchString(txt) { return false }ahead of thelen(txt) >= recoverMinCharsshortcut, a front-loaded answer of ≥200 chars that legitimately begins with"First, "/"Sure, "/"Okay, "(all matched bypreambleRe,finalize.go:64) is now rejected during recovery, whereas before it was accepted unconditionally. Impact: when the terminal is weak/empty/citations and the real recovered answer starts with e.g."First, check that the service is running. Second, …", recovery now fails and the user gets the degenerate closer (bare sources list or"(see above)") instead of the real answer. This regresses the pre-existing back-reference path too, not just the new citations path."First, "is a plausible opener for a real enumerated answer, so the false-positive is reachable — though narrow (severity small). Suggested fix: tightenpreambleRe(e.g. drop/qualifyfirst), or keep the unconditional accept forlen(txt) >= recoverMinCharsand only gate the borderline band on preamble as before.🧹 Code cleanliness & maintainability — No material issues found
I've read the full implementation at
agent/finalize.go. Let me verify one detail about how the surrounding code (the existing conventions) names and structures similar helpers, which I already see in the same file (backRefRe,preambleRe,isWeakFinal).The diff is self-contained in
agent/finalize.go+ its test file, and I've confirmed the actual checked-out code matches the diff.Review — Lens: 🧹 Code cleanliness & maintainability
VERDICT: No material issues found
The change is clean and reads well through the maintainability lens. Verified by reading
agent/finalize.go:37-184:citationLabelRe,linkRe) mirror the establishedxxxRenaming and the block-comment-per-regex style already used bybackRefRe/preambleRe. New tuning constants sit in the sameconstblock with rationale comments, consistent withweakFinalMaxChars/recoverMinChars.isSubstantiveAnswerrefactor is a net readability gain. The old singlereturn A && B && Cexpression was reworked into early-return guards (finalize.go:173-184), and movingpreambleReto the top is both a behavior fix and clearer to read. No dead code left behind —recoverRatio,recoverFloorChars, andweakFinalMaxCharsall remain in use.citationLabelRecharacter class[\s>#*_+-]*against its doc comment (blockquote/ATX/emphasis/list markers) — accurate, not aspirational. The doc block onfinalOutputcorrectly describes the new two-shape recovery and the "citations tested first" ordering.linkRereused for both the presence check and the dominance measurement (finalize.go:135,138), rather than duplicated.Minor, non-blocking observations (not worth a code change on their own):
finalOutputthreads acitations booldown throughlastSubstantiveAssistantTextintoisSubstantiveAnswer— a flag-argument that toggles two behaviors (append-tail, skip-ratio). It's a mild smell, but it's tightly scoped to two private helpers and each is documented, so it stays readable. Acceptable as-is.terminalgetsstrings.TrimSpace'd in three spots (isCitationsOnly,finalOutputtail,lastSubstantiveAssistantText). Trivial redundancy; not worth restructuring.Nothing in my lens rises to a reportable defect.
⚡ Performance — No material issues found
VERDICT: No material issues found
I reviewed this change strictly through the Performance lens and verified it against the checked-out code.
Verification performed:
finalOutputis invoked exactly once per agent run, atagent/agent.go:319, immediately before the loop returns on the terminal (no-tool-call) turn. It is not on a hot path or inside any loop — confirmed by reading the surroundingRunloop (agent.go:290–324).backRefRe,preambleRe,citationLabelRe,linkRe) are package-levelregexp.MustCompilevars — compiled once at init, not per call (finalize.go:60,64,74,79). No per-invocation compilation.regexpis RE2 (linear-time, no backtracking), so even the alternation inlinkRe(\[[^\]]*\]\([^)]*\)|https?://\S+) and the char-class quantifiers incitationLabelRecannot exhibit catastrophic backtracking / quadratic blowup regardless of input.isCitationsOnly(finalize.go:129) does a bounded, constant number of string passes (TrimSpace, oneMatchString, twoReplaceAllString, oneTrim) over the terminal string, whose size is bounded by a single model turn. One-time work.lastSubstantiveAssistantText(finalize.go:146) is a single reverse O(n) scan overmsgsthat stops at the first substantive turn — no nested iteration, no N+1, no unbounded accumulation.m.Text()/TrimSpaceallocations are per-message on one pass, bounded by transcript length, executed once per run.On the common (healthy) path: the reordering now calls
isCitationsOnlybefore theisWeakFinalshort-circuit (finalize.go:38–39). For a normal terminal answer, this adds one^-anchoredcitationLabelRe.MatchStringthat fails immediately — a negligible, one-time cost. Not a regression worth flagging.The
rec + "\n\n" + tailconcatenation andstrings.Contains(rec, tail)dedup check (finalize.go:50–51) are single one-time operations on bounded strings — no concern.Nothing in this change introduces efficiency regressions, extra allocations on a hot path, N+1 behavior, or unbounded growth. The PR's "zero extra model calls" claim holds and the recovery is pure bounded transcript processing.
🧯 Error handling & edge cases — Minor issues
I've read the actual
finalize.go. Let me verify both findings against the real code.Finding 1 (line 174 — preamble check now vetoes any-length prior turn):
Confirmed. The diff moves
preambleRe.MatchString(txt)to the top ofisSubstantiveAnswer(line 174), before thelen(txt) >= recoverMinCharsshortcut (line 177). Pre-diff, a turn ≥200 chars returnedtrueimmediately and never hit the preamble check. Post-diff, a genuine long front-loaded answer opening withFirst,/Sure,/Okay,(all matched bypreambleReat line 64) is rejected, and when it's the only substantive prior turnfinalOutputfalls through toreturn terminal(line 44) — delivering the bare citations/back-reference. This is a real, code-verifiable behavior change from the prior version.First,/First of all,openers for enumerated answers are a plausible genuine shape. Keep.Finding 2 (line 139 — dominance ratio uses full
len(t)):The formula does use
len(t)(full terminal, including URLs) as the denominator at line 139 — that part is factually true. But the draft itself states it "did not construct a failing input" and marks it low-confidence/unverified. Working it through: for claim A, a "prose answer" only slips underlen(t)/3when a single URL dominates ~2/3 of the content, which means the prose residue is tiny — i.e. it isn't really a substantive prose answer being lost, and even if misclassified, with no recoverable prior turnfinalOutputreturns the terminal unchanged (no harm). It's a margins sensitivity note, not a confirmed defect. Per the gate, I cannot positively confirm a realistic failure. Drop.Below is the corrected review.
VERDICT: Minor issues
I reviewed the recovery/unhappy paths this diff introduces (
agent/finalize.go). No panics, swallowed errors, or missing cleanup — this is pure string logic,Message.Text()is nil-safe, empty/nilmsgsdegrades cleanly to "return terminal", and integer divisionlen(t)/citationDominatedDivisorcan't divide by zero (the divisor is a const3). One genuine edge-case regression.agent/finalize.go:174— preamble check now discards legitimate front-loaded answers that merely open with an enumeration/politeness word. The diff hoistspreambleRe.MatchString(txt)ahead of thelen(txt) >= recoverMinCharsshortcut, so it now applies to answers of any length.preambleRe(finalize.go:64) matches^(… |first[, ]|sure[,. ]|okay[,. ]|checking …). A genuine, 200+ char front-loaded answer such as"Sure, the total is $2,700 based on sold listings…"or"First of all, the price…"now matches and is rejected during recovery. When that is the only substantive prior turn,lastSubstantiveAssistantTextreturnsfalseandfinalOutputfalls through toreturn terminal(finalize.go:43-44) — i.e. the bare citations/back-reference list is delivered and the real answer is lost, which is exactly the #1418 failure this PR set out to fix, re-triggered for that phrasing. Before the diff these long turns bypassed the preamble check entirely. Verified by reading the pre/post control flow and the regex. Suggested fix: keep the length shortcut ahead of the preamble check for genuinely long turns (a real preamble is short — cap the preamble veto with a length bound the wayisWeakFinalbounds back-references), or tightenpreambleReso enumeration/politeness openers followed by substantive content don't veto.claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
The PR introduces regex-based text classification (
citationLabelRe,linkRe) and string-selection logic for picking which transcript turn to surface as the agent's final output. Through the security lens:Regex safety: Go's
regexppackage uses RE2 semantics — linear-time guarantees regardless of input.citationLabelReis^-anchored (limits backtracking to the start of the string) and uses only simple character classes and bounded alternations.linkReuses negated character classes ([^\]]*,[^)]*) with no nested quantifiers. ReDoS is structurally impossible here.No new external-input surface:
finalOutput/isCitationsOnlyoperate on strings already present in the message history (msgs). The PR does not add any new I/O, network calls, file reads, or deserialization paths. Tool results that flow intomsgsare an existing design characteristic unmodified by this PR.No URL fetching:
linkReidentifies markdown links and bare URLs for dominance-ratio calculation and deduplication; it never fetches them. No SSRF risk.No injection vectors: The function concatenates transcript strings into a returned
stringvalue. Nothing is executed, templated, or sent to a shell, database, or downstream system.Credential / secret leakage: None introduced. The logic selects among existing assistant turns — it doesn't access configuration, environment, or credential stores.
Authorization:
lastSubstantiveAssistantTextonly scans assistant-role messages (llm.RoleAssistant). No role boundary is crossed by the new citations path.🎯 Correctness — No material issues found
The draft's sole finding turns on whether
[+-]inside[\s>#*_+-]forms a range. I need to check the exact position of-in that character class.The character class is
[\s>#*_+-]. Tracing left-to-right:\s,>,#,*,_,+,-,]. The-is the last character before the closing]. In Go's regexp engine (RE2), a-immediately before]is always a literal, not a range operator. So[+-]here means literal+and literal-— no range, no incidental,match.The draft reviewer analyzed
[+-]in isolation without accounting for its position at the end of the full class, which changes the semantics. The finding does not survive verification.No material issues found
All other items in the draft were marked clean by the original reviewer and are confirmed correct by inspection of the diff:
isSubstantiveAnswer(finalize.go:163–175): preamble check fires before therecoverMinCharsshortcut, closing the regression.isCitationsOnlydominance check: thelen(residue) <= len(t)/citationDominatedDivisorratio correctly separates bare citation lists from prose answers that open with "Source:".strings.Contains(rec, tail)): logic confirmed sound.lastSubstantiveAssistantTextscan direction and skip conditions are correct.citationsflag routing (append vs discard) matches the described contract.🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual source. Here is the corrected review:
Minor issues
The code is well-structured and the refactoring is clean. Two small maintainability findings:
1. Stale
// #N:numbering annotations inTestIsCitationsOnlyagent/finalize_test.go:48,:52,:55,:60,:63Five test-case comments carry labels (
// #4:,// #8:,// #13:,// #5:,// #7:) that reference an external pre-review discussion numbering scheme, not their 1-indexed position in the 19-case table. Two coincidentally match their table index (#4 at position 4, #8 at position 8); the rest don't (#13 is the 11th case, #5 is the 15th, #7 is the 18th). A reader seeing// #13:on the 11th case will hunt for context and find nothing in the repo.Fix: drop the number prefix and keep only the descriptive text — e.g.
// colon after the boldinstead of// #4: colon after the bold.2. Third identical copy of
longAnswerinTestRun_RecoversFrontLoadedAnswerWithCitationsagent/finalize_test.go:316This exact expression already appears at line 86 (inside
TestFinalOutput) and line 267 (insideTestRun_RecoversFrontLoadedAnswer). The PR adds a third local copy rather than extracting a package-levelvaror a one-liner helper. Trivial to fix and keeps future test updates from drifting.⚡ Performance — No material issues found
No material issues found
This is post-processing applied once per agent run at termination, not on any hot path. The changes introduce no loops over the call site, no new allocations in repetitive code, and no blocking work.
backRefRe,preambleRe,citationLabelRe,linkRe) are compiled once at package init — correct.isCitationsOnlyruns two match→replace pairs on a single terminal string (verified:citationLabelRematched then replaced,linkRematched then replaced). A singleFindStringIndex+slice could halve the regex work, but the terminal string is a short agent output turn (hundreds of chars). This is micro-optimization territory on a once-per-run call.lastSubstantiveAssistantTextscans messages newest→oldest, callingm.Text()only after the role guard (line 150).m.Text()builds a string viastrings.Builderover the parts slice — one allocation per assistant turn, but there are at most tens of turns in any typical run, and this is only reached when the terminal is degenerate.strings.Contains(rec, tail)at line 50 is O(|rec|·|tail|) but both strings are bounded agent-output text (hundreds of bytes); no practical concern.Nothing in the added code introduces N+1 calls, unbounded growth, work moved into a hot loop, or blocking on a hot path. The change is architecturally a pure post-run string-analysis pass.
🧯 Error handling & edge cases — No material issues found
I've read the full implementation and tests. One thing to manually verify: whether the
strings.Trimof the citation residue leaves enough text in edge cases to correctly fail the dominance check for prose-led inputs.Let me trace through the
source-led-prose-answertest case mentally (the critical false-positive guard):Input:
"Source: According to https://cdc.gov the flu vaccine is 40-60% effective, and the CDC recommends annual vaccination for everyone over six months old."len(t)≈ 156citationLabelRestrips"Source: ", leaving body starting with"According to https://cdc.gov…"linkRestripshttps://cdc.gov, leaving"According to the flu vaccine is 40-60%…"(≈127 chars after trim)127 <= 156/3 = 52→ false → not citations-only ✓And for the annotated-multi-source shape (the real #1418 case), the residue is only per-source annotations like
"tracker), (sold), (retail), (asking"(~35 chars) againstlen(t)/3 ≈ 49→35 <= 49→ true ✓The dominance threshold correctly separates these two shapes.
Verdict: No material issues found
After reading
agent/finalize.goin full and tracing the key boundary cases:strings.TrimSpace;citationLabelRecan't match"", so empty strings short-circuit at line 131.msgsnil or empty —for i := len(msgs)-1; i >= 0doesn't execute;finalOutputfalls back toterminalon!ok.m.Text()on tool-result messages — returns""(noParts); thetxt == ""guard on line 154 skips these immediately, no risk.isCitationsOnly(line 139) —len(t)/3truncates, but any input that passes both the label regex and the link check is at minimum ~20 chars, so the divisor never magnifies to a meaningfully wrong threshold.tailcannot be empty here becauseisCitationsOnlyreturningtruerequirestto be non-empty and match a substantial regex.recoverRatio*len(terminal)withlen(terminal)==0— correctly degenerates to "always true for floor-clearing turns," which is right: against an empty terminal, any 80+ char non-preamble is a better answer.isSubstantiveAnswerpreamble-before-length reordering — the PR intentionally moved the preamble guard before therecoverMinCharsshortcut; the"long preamble is skipped"test covers this path.The unhappy paths are all handled: nothing better to recover →
terminalreturned as-is; citations terminal with only a preamble prior →sourcesreturned as-is; dedup guard prevents double-appending when the recovered turn already embeds the citations.deepseek-v4-pro:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
This change is a pure text-processing heuristic operating on model-generated transcript text. Through the security lens:
result.Outputis consumed downstream only via safefmt.Printf("%s", ...),fmt.Println, andstrings.Contains— no SQL, command, path, or template construction from the recovered text. Verified by grepping all.Outputconsumers across the repo.citationLabelRe,linkRe,backRefRe,preambleRe) use only anchored patterns, simple alternations, and negated character classes — no nested quantifiers that could cause catastrophic backtracking. Verified by reading the compiled patterns atfinalize.go:60-79.terminalparameter comes fromresp.Text()(model output), andmsgsis the conversation transcript. The code rearranges model-generated text and returns it asresult.Output— it never treats this text as trusted for any security-sensitive operation.The worst-case "abuse" would be a model emitting text that coincidentally matches the citations heuristic, causing a prior turn to be recovered instead — but both are model-generated text from the same conversation, so this is a UX concern, not a security one.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 4: context deadline exceeded
🧹 Code cleanliness & maintainability — Minor issues
All three findings are confirmed against the actual source. Here is the verified review.
VERDICT: Minor issues
citationResidueCutsetis a standaloneconstseparated from theconstblock below it (finalize.go:84vsfinalize.go:86-104). The file has twoconstdeclarations back-to-back: a single-constantconstfor the cutset, then aconst (...)block for the numeric thresholds. The cutset logically belongs with the other constants (same function, same package, same domain). Moving it into the block would make the declarations easier to scan.Boolean tramp-data through three functions (
finalize.go:38,42,146,157,173):citationsis computed infinalOutput, passed tolastSubstantiveAssistantText, and then passed again toisSubstantiveAnswerwhere it gates the ratio check. Each function's signature now carries acitations boolparameter whose meaning is "the terminal is a citations addendum, not a rival answer." This is a mild form of the "boolean parameter" smell — the parameter controls two distinct behaviors in two different functions (skip ratio in one, trigger append+dedup in the other). For three private functions in a single file it's acceptable, but if this pattern grows it would be cleaner to use a small strategy struct or an enum rather than threading booleans.citationLabelRecharacter class[\s>#*_+-]includes+(finalize.go:74). The comment says the class tolerates "leading markdown emphasis / list / block-quote / ATX-heading markers." The+is a valid markdown list marker (+ item), so it's intentional, but it's the least common of the three list styles and isn't mentioned in the comment. Adding+to the comment would help a reader.⚡ Performance — No material issues found
Verdict: No material issues found
The change adds a small amount of per-invocation work — a few regex matches/replaces on short terminal strings, one
strings.Containson the recovered answer, and some string concatenations — all insidefinalOutput, which is called exactly once per agent run (at the terminal turn). An agent run is dominated by LLM API calls taking seconds; the added CPU work here is microseconds. No N+1 queries, no hot-loop allocations, no unbounded growth, no blocking calls, and no quadratic behavior.The one structural shift worth noting (but not flagging as a problem):
preambleRe.MatchStringnow runs on every candidate inisSubstantiveAnswer, including turns ≥200 chars that previously skipped it. The regex is simple, the scan is at most a handful of messages, and the correctness win (not recovering a long planning preamble as the answer) justifies the negligible cost.🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
Through the 🧯 Error handling & edge cases lens, the change is clean. I verified the full files (not just the diff), traced every function's unhappy paths, and checked all regex edge cases against the test cases. Here's what I confirmed:
isCitationsOnly("")returns false (no label match);finalOutputwith nilmsgsfalls through toreturn terminalbecauselastSubstantiveAssistantTextiterates zero times and returns("", false). Both paths are safe.isCitationsOnlytrims first, label match fails, falls through toisWeakFinal(existing behavior, unchanged).len(t)/citationDominatedDivisor— the shortest possible positive case ("Sources: [a](https://a)") is ~26 chars, so the quotient is ≥8. No division-by-zero or meaningful truncation risk.regexp.MustCompilecalls are package-levelvardeclarations; the tests passing confirms they compile.\sinside[]is valid in Go's RE2 dialect (verified against Goregexp/syntaxdocs). The-at the end of[\s>#*_+-]is safely literal. Unicode characters incitationResidueCutset(·•—) are valid Go source and correctly handled bystrings.Trim.preambleRebefore length check: The old code would accept a ≥200-char preamble as an answer (thepreambleRecheck was only in the floor/ratio branch). The new code checkspreambleRefirst — this fixes a real bug where a long planning narration could be recovered. Verified atfinalize.go:174.strings.Contains: Bothrecandtailare trimmed before comparison. False negatives (different whitespace) are cosmetic at worst — the citations get duplicated rather than lost. Conservative.The only thing that gave me pause was a theoretical false-positive in
isCitationsOnlyfor extremely short prose that opens with "Source:" and contains a link — e.g."Source: Per [this](https://a.com), it's $5."(42 chars, residue 14,42/3=14,14≤14→ true). But the practical impact is nil: either no prior answer qualifies for recovery (floor is 80 chars), so the terminal is returned as-is, or a real prior answer exists and appending this as "citations" is reasonable. The dominance divisor of 3 is a well-chosen threshold; the test case"source-led-prose-answer"at 160 chars correctly returns false.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
I reviewed the change through the security lens (injection, SSRF, unsafe deserialization, secret leakage, input validation, untrusted-data handling, ReDoS).
linkReare never fetched or resolved; they are only copied as text into the returned string.rec + "\n\n" + tailis plain string concatenation with no interpolation/escaping concern.linkRe = \[[^\]]*\]\([^)]*\)|https?://\S+uses only negated-character-class quantifiers and\S+— linear, no nested quantifiers, no catastrophic backtracking.citationLabelReis^-anchored with a single bounded[\s>#*_+-]*and a simple alternation — also linear. Both are applied to terminal strings (small), and even adversarial long inputs would be O(n). No ReDoS risk.tailis just the model's own citation text.I verified by reading
agent/finalize.goin full (the entire finalOutput/isCitationsOnly/lastSubstantiveAssistantText/isSubstantiveAnswer path) and confirming there are no imports or calls into network, file, exec, or deserialization sinks from this code.🎯 Correctness — Minor issues
Verdict: Minor issues found
I verified the two findings against the actual source in
agent/finalize.go(andfinalize_test.go), re-deriving the regexes and the control flow.Findings
agent/finalize.go:173-176— preamble check now rejects legitimate long answers that open with conversational phrases (regression in the recovery path).Confirmed:
isSubstantiveAnswer(lines 173-183) now runspreambleRe.MatchString(txt)before thelen(txt) >= recoverMinCharsshortcut, so the preamble rejection applies "regardless of length" — a change from the prior behavior where a ≥200-byte turn was accepted without a preamble test.preambleRe(line 64) is(?i)^(let me|let'?s|i'?ll|i will|first[, ]|sure[,. ]|okay[,. ]|on it|checking). These tokens match real answer openers, not only planning narrations — e.g."Let me be clear: the limit is 60 minutes…"(let me),"I'll start with the bottom line: …"(i'?ll),"Okay, the answer is X…"(okay[,. ]),"Sure, here's the answer: …"(sure[,. ]). Any such front-loaded turn ≥200 bytes is now skipped bylastSubstantiveAssistantText, sofinalOutputfalls back to the degenerate terminal — potentially leaving the user with"(Already answered above.)"or a bare sources list instead of the real answer sitting in the transcript. This affects both the back-reference path and the citations path. The test suite only exercises the intended case (longPreamble= a genuine planning narration); there is no negative test for a long answer that opens with one of these tokens, so the regression is unguarded. (Note: the draft's"Sure!"example does not actually trigger the regex, since!is not in the[,. ]class — but"Sure,"/"Okay,"/"Let me"/"I'll"openers do.) Suggested fix: narrowpreambleReto intent-announcing planning phrasing, or only apply the preamble rejection when the turn carries tool calls, rather than blanket-rejecting every turn that opens with these tokens.agent/finalize.go:50— citations dedup is a raw substringContains, which can both over- and under-trigger. Confirmed at lines 46-53:tail := strings.TrimSpace(terminal)is checked viastrings.Contains(rec, tail). If the recovered answer contains the exact sources string as a substring in a different context, the citations are silently dropped; conversely if the prior turn's citations differ by even trailing punctuation/whitespace from the terminal's, the block is appended twice. Low impact and explicitly a best-effort guard, but it is not a structural (e.g. line/block) equality check. Not blocking.🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
isCitationsOnly/isSubstantiveAnsweradditions follow the existing file's idioms (package-levelregexp.MustCompilevars next to named threshold consts with doc comments,strings.TrimSpacenormalization at entry). Naming is clear and the comments explain why, not just what.finalOutputcontrol flow reads cleanly: thecitationsflag threads throughlastSubstantiveAssistantText→isSubstantiveAnsweras a single bool with a documented meaning ("terminal is a citations addendum, not a rival answer"), avoiding a second parallel path.finalize.go:50is localized and commented; thestrings.Contains(rec, tail)guard is a reasonable low-churn approach for the stated duplicate-block case.linkReis reused for both the link-presence check and the residue strip (finalize.go:135,:138);citationResidueCutsetis a single shared cutset.finalize_test.goare well-structured (table-driven, named, with inline rationale comments tying each case to a design decision), consistent with the pre-existingTestIsWeakFinal/TestFinalOutputpatterns.⚡ Performance — No material issues found
VERDICT: No material issues found
agent/finalize.gothrough the performance lens.finalOutputand its helpers (isCitationsOnly,lastSubstantiveAssistantText,isSubstantiveAnswer) execute once per agent run on the terminal turn — they are not on a hot loop or per-request path. Verified atagent/finalize.go:37-55.citationLabelRe,linkRe,backRefRe,preambleRe) are package-level compiled-once vars (agent/finalize.go:60,64,74,79); no per-callregexp.MustCompile.isCitationsOnlydoes up to 4 regex passes (MatchString+ twoReplaceAllString+MatchString) plus astrings.Trimover the terminal string — but the input is the single terminal message (typically a few hundred chars), so the allocations fromReplaceAllStringare trivial and one-shot. Not material.preambleRe.MatchStringcheck (agent/finalize.go:173-176) now runs on every candidate assistant turn instead of only the borderline band, butlastSubstantiveAssistantTextscans the transcript once per run and the regex match is on a single message's text. Negligible.strings.Contains(rec, tail)dedup is a single substring scan over the recovered answer, one-shot.🧯 Error handling & edge cases — Minor issues
The finding is confirmed against the actual code. The
preambleReat line 64 is(?i)^(let me|let'?s|i'?ll|i will|first[, ]|sure[,. ]|okay[,. ]|on it|checking), which matches common conversational answer openers ("Sure,", "Okay,", "First,", "Let me"). The PR moves this check to before thelen(txt) >= recoverMinCharsshortcut at lines 173-176, so a ≥200-byte substantive answer that opens with one of these phrases is now rejected byisSubstantiveAnsweron the recovery path — whereas before this PR it was recovered via the length shortcut. This is a real behavioral regression for that input shape, only on the recovery path, and unguarded by the test suite.No other findings to verify (the draft's other bullets were confirmations of soundness, not issues).
Verdict: Minor issues found
agent/finalize.go:64,173-176— Moving thepreambleRecheck before thelen(txt) >= recoverMinCharsshortcut (the PR's fix for long planning narration) now also rejects legitimate front-loaded answers that merely open with a conversational filler the regex matches.preambleReis(?i)^(let me|let'?s|i'?ll|i will|first[, ]|sure[,. ]|okay[,. ]|on it|checking), which matches common answer openers like"Sure, the limit is 60 minutes …","Okay, here's the result: …","First, the cap is 60 minutes …", or"Let me explain…". Previously a ≥200-byte such answer was recovered via the length shortcut (preamble was only checked on the floor path); nowisSubstantiveAnswerreturns false for it,lastSubstantiveAssistantTextfinds nothing, andfinalOutputreturns the degenerate terminal (a bareSources:list or(see above)) — i.e. the exact "real answer discarded" failure this PR is meant to fix, for a different input shape. This only bites on the recovery path (when the terminal is weak or citations-only), and it is a deliberate heuristic tradeoff enforced by the newlong preamble is skippedtest; but the test suite has no case for a preamble-phrase-led substantive answer being recovered, so the regression is unguarded. Suggested mitigation: narrowpreambleReto planning-style phrasing (e.g. require an action verb / ellipsis context:"let me (search|check|look|find|gather|try)","i'll (look|search|check|find)") rather than baresure/okay/first/let me, so conversational answer openers are not misclassified as planning narration.kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Through a security lens this change is clean.
backRefRe,preambleRe,citationLabelRe,linkRe) are compiled withregexp.MustCompileand use only RE2-safe constructs (no back-references, no look-around, no nested quantifiers). Go’s RE2 engine bounds execution time linearly with input size, so short LLM terminal strings cannot be used for ReDoS.finalOutputonly selects and concatenates existing assistant text already present in the transcript. It does not parse URLs, execute links, deserialize untrusted data, or invoke external systems.lastSubstantiveAssistantTextskips non-assistant roles, so recovering an earlier turn cannot leak system prompts or tool results that were not already emitted as assistant-visible text.strings.Contains(rec, tail)is a passive check; it does not transform or evaluate the content, so there is no injection path there.The PR is a pure transcript-reordering heuristic with no security-relevant changes to trust boundaries, input parsing, or secret handling.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 7: context deadline exceeded
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The change is clean and well-structured for its lane. The
finalOutputcontrol flow is clear (citations test → weak-final test → recovery → optional append), the newisCitationsOnlyfunction is cohesive with a single responsibility, and the constants/regexes are documented with explicit rationale. Tests are table-driven and comprehensive.⚡ Performance — No material issues found
No material issues found
The change is confined to transcript-recovery heuristics in
finalOutputand introduces no new allocations on hot paths, no loops over unbounded data, and no extra model calls.Verified details:
isCitationsOnlyoperates on the single terminal string (typically <1 KiB) and is called once perRun. It does two regex matches and twoReplaceAllStringpasses on that short input — negligible overhead.agent/agent.go:319confirmsfinalOutputis only invoked at loop termination.lastSubstantiveAssistantTextalready scannedmsgsnewest→oldest before this PR; the only difference is an extra boolean parameter forwarded toisSubstantiveAnswer. The loop is bounded by conversation length (maxSteps, default small).isSubstantiveAnswernow evaluatespreambleRe.MatchString(txt)before the length shortcuts. This means a non-preamble message ≥recoverMinChars(200) incurs one extra regex evaluation. The regex is anchored at^with a small alternation — confirmed by readingagent/finalize.go:64— so the cost is O(prefix) and trivial even for long messages.strings.Contains(rec, tail)infinalOutput(line 50) is a single substring search on the recovered answer, executed at most once per run.No new quadratic patterns (e.g., nested scans over messages × messages or repeated
m.Text()calls for the same message).🧯 Error handling & edge cases — No material issues found
No material issues found.
qwen3.5:397b-cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
After reviewing the change through the security lens:
Analysis of untrusted data handling:
terminalstring comes from LLM model output (resp.Text()atagent/agent.go:319), which is untrusted inputRegex patterns examined for ReDoS risk:
linkRe(\[[^\]]*\]\([^)]*\)|https?://\S+) — uses negated character classes, no nested quantifiers; safecitationLabelRe(^[\s>#*_+-]*(sources?|...)...) — linear scan with alternation on short literals; safePotential concerns evaluated:
citetool, outside this lens's scope.Verified by reading:
agent/finalize.go:37-54—finalOutputlogic confirmed as text classification/passthrough onlyagent/finalize.go:129-140—isCitationsOnlyconfirmed to only match/measure, not fetch or execute URLsagent/agent.go:319— confirmed input source is model response text🎯 Correctness — No material issues found
Verdict: No material issues found
After thorough review through the Correctness lens, I verified the logic by reading the full implementation and tracing through the key functions:
Verification performed:
agent/finalize.go(lines 1-184) to verify the complete implementationagent/finalize_test.go(lines 1-341) to verify test cases match the logicisCitationsOnly()with multiple test inputs to verify the dominance check mathisSubstantiveAnswer()control flow: preamble check → length≥200 shortcut → length<80 rejection → ratio/citations checkcitationLabelRe,linkRe,preambleRe,backRefRe) match their documented behaviorcitationDominatedDivisor=3threshold correctly separates bare citation lists from prose-with-linksKey logic verified:
isCitationsOnlydominance check: For"Sources: [a](url), [b](url).", after stripping label and links, residue is minimal (whitespace/punctuation only), correctly returningtrue. For"Source: According to https://cdc.gov the flu vaccine...", residue is substantial prose, correctly returningfalse.isSubstantiveAnswerpreamble-first ordering: The preamble check now correctly applies BEFORE therecoverMinCharsshortcut, preventing long planning narrations from being recovered as answers.Citations mode bypasses ratio check: When
citations=true, thelen(txt) >= recoverRatio*len(terminal)requirement is skipped, allowing concise answers (<200 chars) to be recovered against long citation terminals.Deduplication via
strings.Contains: When the recovered turn already contains the citations verbatim, they are not appended twice.Bare domains intentionally excluded: The
linkRepattern requireshttp://orhttps://, so bare domains likepcprice.watchdon't match, correctly returningfalseper the documented limitation.All test cases align with the implementation logic. No logic bugs, incorrect behavior, or semantic errors found.
🧹 Code cleanliness & maintainability — No material issues found
Verdict: No material issues found
After reviewing the changes through the code cleanliness & maintainability lens:
isCitationsOnly, updatedisSubstantiveAnswer) are purposeful and not redundant.citationLabelRe,linkRe,citationResidueCutset,citationDominatedDivisorare self-documenting with thorough comments explaining their role.finalOutputgrew but remains readable; the control flow (citations check → weak check → recovery → append-if-citations) is logical and well-commented.weakFinalMaxChars,recoverMinChars,citationDominatedDivisor, etc.) have comments explaining their purpose and derivation.TestIsCitationsOnlyand 11 forTestFinalOutputcover edge cases without redundancy.citations boolparameter passed throughlastSubstantiveAssistantText→isSubstantiveAnsweris a clean way to thread the recovery mode context.Verified by: Reading
agent/finalize.go:1-184andagent/finalize_test.go:1-341in full; confirmed no unchecked symbols, no structural issues, and test cases align with the implementation.⚡ Performance — No material issues found
No material issues found
I've reviewed this change through the Performance lens and verified the code with repository tools. The changes are computationally lightweight:
isCitationsOnly()— Adds 2 regex matches and 2 regex replacements on the terminal string (typically <1KB). Regexes are pre-compiled at package init. O(n) work, called once per agent run.isSubstantiveAnswer()reorder — The preamble regex check now runs first instead of last. This is actually a micro-improvement for preamble cases (fast fail) and a negligible cost for non-preamble short texts (anchored^regex fails fast).lastSubstantiveAssistantText()loop — Still iterates messages newest→oldest, callingisSubstantiveAnswerper message. Whencitations=true, the ratio check (len(txt) >= recoverRatio*len(terminal)) is skipped, slightly reducing work per iteration.Dedup check —
strings.Contains(rec, tail)is O(n*m) but runs at most once per run on small strings.No N+1 patterns, no unbounded growth, no hot-loop allocations, no blocking calls added. The heuristic work is bounded and trivial relative to the LLM call costs it guards.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 10: context deadline exceeded
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
- isSubstantiveAnswer: restore the original "≥200 chars ⇒ real answer" unconditional accept (the preamble filter now applies only in the borderline 80–200 band). Applying preambleRe regardless of length regressed the recovery path: a long front-loaded answer opening with a conversational word ("Sure,", "Let me explain:", "First,") was wrongly discarded (flagged by glm-5.2 + claude-code/opus). The NVD-style planning-narration case is already handled by the citation-dominance check, not this filter. - citations dedup: compare with <url> angle-bracket wrappers stripped, so a front-loaded turn that listed the same sources unwrapped still suppresses the duplicate block. - doc: spell out the citationLabelRe leading class members (+ list markers); drop the stale "gated by preamble" note on recoverMinChars. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>