fix(agent): recover front-loaded answer past a citations-only terminal turn #11

Merged
steve merged 5 commits from fix/finalize-citations-only-terminal into main 2026-07-10 16:47:32 +00:00
Owner

Problem (mort issue #1418)

finalOutput already 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.2 driving a cite
tool) front-load the full prose answer into an earlier tool-call turn and
close with a terminal turn that is only their sources:

Sources: [pcprice.watch](<url>) (sold-median tracker), [eBay sold listing](<url>) ($2,770 sealed FE, Jul 5), [bestvaluegpu](<url>) (retail vs used), [resaleprices](<url>) (active asking range).

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/citations
heading, 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 finalOutput recovers the
prior 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):

  • Dominance check, not just "opens with a label." A real prose answer that
    merely opens with Source: … http://… keeps most of its text after stripping
    the URL, so it is left as the answer (not demoted).
  • ^-anchored heading so an answer that ends with a Sources: section,
    or mentions "sources" mid-sentence, never matches.
  • Citations recovery is decoupled from the terminal-length ratio, so a
    concise (<200-char) front-loaded answer is still recovered against a long
    citations list (the old ratio arm was structurally unsatisfiable there).
  • preambleRe now applies regardless of length, so a long planning
    narration ("I'll look up …") is never recovered as the answer.
  • Citations are tested before the back-reference test, so a terminal that is
    both (e.g. References: as noted above, [x](url)) keeps its links.
  • Label matching tolerates leading markdown (- , ## , **) and a colon
    after 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 real
markdown links.

Tests

  • TestIsCitationsOnly — 19 cases (bold / ATX-heading / dash / back-ref-plus-links
    positives; 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 -l clean.

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

## Problem (mort issue #1418) `finalOutput` already 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.2` driving a `cite` tool) front-load the full prose answer into an *earlier* tool-call turn and close with a terminal turn that is **only their sources**: ``` Sources: [pcprice.watch](<url>) (sold-median tracker), [eBay sold listing](<url>) ($2,770 sealed FE, Jul 5), [bestvaluegpu](<url>) (retail vs used), [resaleprices](<url>) (active asking range). ``` 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/citations heading, **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 `finalOutput` recovers the prior 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): - **Dominance check, not just "opens with a label."** A real prose answer that merely opens with `Source: … http://…` keeps most of its text after stripping the URL, so it is left as the answer (not demoted). - **`^`-anchored heading** so an answer that *ends* with a `Sources:` section, or mentions "sources" mid-sentence, never matches. - **Citations recovery is decoupled from the terminal-length ratio**, so a *concise* (<200-char) front-loaded answer is still recovered against a long citations list (the old ratio arm was structurally unsatisfiable there). - **`preambleRe` now applies regardless of length**, so a long planning narration ("I'll look up …") is never recovered as the answer. - **Citations are tested before the back-reference test**, so a terminal that is both (e.g. `References: as noted above, [x](url)`) keeps its links. - Label matching tolerates leading markdown (`- `, `## `, `**`) and a colon after 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 real markdown links. ## Tests - `TestIsCitationsOnly` — 19 cases (bold / ATX-heading / dash / back-ref-plus-links positives; 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 -l` clean. 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](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
steve added 3 commits 2026-07-10 15:20:53 +00:00
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>
test(agent): cover citations-only terminal recovery + edge cases (#1418)
CI / Tidy (pull_request) Successful in 9m28s
CI / Build & Test (pull_request) Successful in 10m19s
Adversarial Review (Gadfly) / review (pull_request) Successful in 14m12s
a868863d24
TestIsCitationsOnly (19 cases: bold/ATX/dash/backref positives; source-led
prose, bare-domain, prose-then-sources negatives), 4 new TestFinalOutput cases
(concise+long-citations, source-led-not-hijacked, long-preamble-skipped, dedup),
and end-to-end TestRun_RecoversFrontLoadedAnswerWithCitations.

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 — done

  • security — No material issues found
  • correctness — Minor issues
  • maintainability — No material issues found
  • performance — No material issues found
  • error-handling — Minor issues

claude-code/sonnet · claude-code — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — No material issues found

deepseek-v4-pro:cloud · ollama-cloud — done

  • security — No material issues found
  • ⚠️ correctness — could not complete
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — No material issues found

glm-5.2:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — Minor issues
  • maintainability — No material issues found
  • performance — No material issues found
  • error-handling — Minor issues

kimi-k2.6:cloud · ollama-cloud — done

  • security — No material issues found
  • ⚠️ correctness — could not complete
  • maintainability — No material issues found
  • performance — No material issues found
  • error-handling — No material issues found

qwen3.5:397b-cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — No material issues found
  • performance — No material issues found
  • ⚠️ error-handling — could not complete

Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

<!-- gadfly-status-board --> ## 🪰 Gadfly — live review status 6/6 reviewers finished · updated 2026-07-10 15:35:05Z #### `claude-code/opus` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `deepseek-v4-pro:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ⚠️ **correctness** — could not complete - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ⚠️ **correctness** — could not complete - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `qwen3.5:397b-cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ⚠️ **error-handling** — could not complete <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-10 15:35:05 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 7 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 7 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -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

⚪ **citations boolean threaded through three functions — mild boolean-parameter smell** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -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 substring Contains, which can both over- and under-trigger. Confirmed at lines 46-53: tail := strings.TrimSpace(terminal) is checked via strings.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

🟡 **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 substring `Contains`, which can both over- and under-trigger.** Confirmed at lines 46-53: `tail := strings.TrimSpace(terminal)` is checked via `strings.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… <sub>🪰 Gadfly · advisory</sub>
@@ -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

⚪ **citationLabelRe character class includes '+' without mentioning it in the comment** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -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

⚪ **citationResidueCutset declared as standalone const instead of in the const block immediately below** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -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 runs preambleRe.MatchString(txt) before the len(txt) >= recoverMinChars shortcut, 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

🟠 **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 runs `preambleRe.MatchString(txt)` *before* the `len(txt) >= recoverMinChars` shortcut, 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… <sub>🪰 Gadfly · advisory</sub>
@@ -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

⚪ **// #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` <sub>🪰 Gadfly · advisory</sub>
@@ -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

⚪ **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` <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 6 models

Verdict: Minor issues · 7 findings (1 with multi-model agreement)

Finding Where Models Lens
🟠 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 agent/finalize.go:173 2/6 correctness, error-handling
6 single-model findings (lower confidence)
Finding Where Model Lens
🟡 citations dedup uses raw substring Contains(rec, tail); can drop or duplicate citations on minor text differences agent/finalize.go:50 glm-5.2:cloud correctness
citations boolean threaded through three functions — mild boolean-parameter smell agent/finalize.go:38 deepseek-v4-pro:cloud maintainability
citationLabelRe character class includes '+' without mentioning it in the comment agent/finalize.go:74 deepseek-v4-pro:cloud maintainability
citationResidueCutset declared as standalone const instead of in the const block immediately below agent/finalize.go:84 deepseek-v4-pro:cloud maintainability
// #N: annotations in TestIsCitationsOnly don't match table positions and reference an external review document agent/finalize_test.go:48 claude-code/sonnet maintainability
longAnswer defined identically for the third time; a package-level var or helper would avoid the duplication agent/finalize_test.go:316 claude-code/sonnet maintainability
Per-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.go through 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 becomes result.Output (verified at agent/agent.go:319). No new trust boundary is crossed.

Security-relevant checks I performed:

  • ReDoS / regex safety — All new patterns (citationLabelRe, linkRe) compile via regexp.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.
  • Tainted-data sink — The "links" isCitationsOnly extracts 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.
  • Data exposure — Recovery can now surface an earlier assistant turn instead of the terminal turn (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.
  • No auth/deserialization surface — no serialization, no auth decisions, no untrusted structural parsing.

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)/divisor integer-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 isSubstantiveAnswer returned true unconditionally for any len(txt) >= recoverMinChars (200) turn, with the preambleRe check applied only in the borderline < 200 band. The "after" state (lines 173–184) moves if preambleRe.MatchString(txt) { return false } to the very top, ahead of the length shortcut. preambleRe (line 64) includes first[, ], sure[,. ], and okay[,. ], 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, and terminal is 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 moving if preambleRe.MatchString(txt) { return false } ahead of the len(txt) >= recoverMinChars shortcut, a front-loaded answer of ≥200 chars that legitimately begins with "First, " / "Sure, " / "Okay, " (all matched by preambleRe, 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: tighten preambleRe (e.g. drop/qualify first), or keep the unconditional accept for len(txt) >= recoverMinChars and 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:

  • Follows existing patterns. New regexes (citationLabelRe, linkRe) mirror the established xxxRe naming and the block-comment-per-regex style already used by backRefRe/preambleRe. New tuning constants sit in the same const block with rationale comments, consistent with weakFinalMaxChars/recoverMinChars.
  • The isSubstantiveAnswer refactor is a net readability gain. The old single return A && B && C expression was reworked into early-return guards (finalize.go:173-184), and moving preambleRe to the top is both a behavior fix and clearer to read. No dead code left behind — recoverRatio, recoverFloorChars, and weakFinalMaxChars all remain in use.
  • Comments match the code. I checked the citationLabelRe character class [\s>#*_+-]* against its doc comment (blockquote/ATX/emphasis/list markers) — accurate, not aspirational. The doc block on finalOutput correctly describes the new two-shape recovery and the "citations tested first" ordering.
  • No copy-paste duplication. The link-stripping logic is centralized in one linkRe reused 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):

  • finalOutput threads a citations bool down through lastSubstantiveAssistantText into isSubstantiveAnswer — 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.
  • terminal gets strings.TrimSpace'd in three spots (isCitationsOnly, finalOutput tail, 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:

  • finalOutput is invoked exactly once per agent run, at agent/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 surrounding Run loop (agent.go:290–324).
  • All four regexes (backRefRe, preambleRe, citationLabelRe, linkRe) are package-level regexp.MustCompile vars — compiled once at init, not per call (finalize.go:60,64,74,79). No per-invocation compilation.
  • Go's regexp is RE2 (linear-time, no backtracking), so even the alternation in linkRe (\[[^\]]*\]\([^)]*\)|https?://\S+) and the char-class quantifiers in citationLabelRe cannot exhibit catastrophic backtracking / quadratic blowup regardless of input.
  • isCitationsOnly (finalize.go:129) does a bounded, constant number of string passes (TrimSpace, one MatchString, two ReplaceAllString, one Trim) 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 over msgs that stops at the first substantive turn — no nested iteration, no N+1, no unbounded accumulation. m.Text()/TrimSpace allocations are per-message on one pass, bounded by transcript length, executed once per run.

On the common (healthy) path: the reordering now calls isCitationsOnly before the isWeakFinal short-circuit (finalize.go:38–39). For a normal terminal answer, this adds one ^-anchored citationLabelRe.MatchString that fails immediately — a negligible, one-time cost. Not a regression worth flagging.

The rec + "\n\n" + tail concatenation and strings.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 of isSubstantiveAnswer (line 174), before the len(txt) >= recoverMinChars shortcut (line 177). Pre-diff, a turn ≥200 chars returned true immediately and never hit the preamble check. Post-diff, a genuine long front-loaded answer opening with First, / Sure, / Okay, (all matched by preambleRe at line 64) is rejected, and when it's the only substantive prior turn finalOutput falls through to return 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 under len(t)/3 when 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 turn finalOutput returns 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/nil msgs degrades cleanly to "return terminal", and integer division len(t)/citationDominatedDivisor can't divide by zero (the divisor is a const 3). 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 hoists preambleRe.MatchString(txt) ahead of the len(txt) >= recoverMinChars shortcut, 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, lastSubstantiveAssistantText returns false and finalOutput falls through to return 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 way isWeakFinal bounds back-references), or tighten preambleRe so 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 regexp package uses RE2 semantics — linear-time guarantees regardless of input. citationLabelRe is ^-anchored (limits backtracking to the start of the string) and uses only simple character classes and bounded alternations. linkRe uses negated character classes ([^\]]*, [^)]*) with no nested quantifiers. ReDoS is structurally impossible here.

  • No new external-input surface: finalOutput / isCitationsOnly operate 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 into msgs are an existing design characteristic unmodified by this PR.

  • No URL fetching: linkRe identifies 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 string value. 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: lastSubstantiveAssistantText only 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:

  • Preamble-before-length ordering in isSubstantiveAnswer (finalize.go:163–175): preamble check fires before the recoverMinChars shortcut, closing the regression.
  • isCitationsOnly dominance check: the len(residue) <= len(t)/citationDominatedDivisor ratio correctly separates bare citation lists from prose answers that open with "Source:".
  • Dedup guard (strings.Contains(rec, tail)): logic confirmed sound.
  • lastSubstantiveAssistantText scan direction and skip conditions are correct.
  • citations flag 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 in TestIsCitationsOnly

agent/finalize_test.go:48, :52, :55, :60, :63

Five 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 bold instead of // #4: colon after the bold.


2. Third identical copy of longAnswer in TestRun_RecoversFrontLoadedAnswerWithCitations

agent/finalize_test.go:316

longAnswer := strings.TrimSpace(strings.Repeat("Free group calls are capped at sixty minutes. ", 6))

This exact expression already appears at line 86 (inside TestFinalOutput) and line 267 (inside TestRun_RecoversFrontLoadedAnswer). The PR adds a third local copy rather than extracting a package-level var or 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.

  • All four regex patterns (backRefRe, preambleRe, citationLabelRe, linkRe) are compiled once at package init — correct.
  • isCitationsOnly runs two match→replace pairs on a single terminal string (verified: citationLabelRe matched then replaced, linkRe matched then replaced). A single FindStringIndex+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.
  • lastSubstantiveAssistantText scans messages newest→oldest, calling m.Text() only after the role guard (line 150). m.Text() builds a string via strings.Builder over 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.Trim of 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-answer test 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) ≈ 156
  • citationLabelRe strips "Source: ", leaving body starting with "According to https://cdc.gov…"
  • linkRe strips https://cdc.gov, leaving "According to the flu vaccine is 40-60%…" (≈127 chars after trim)
  • 127 <= 156/3 = 52false → 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) against len(t)/3 ≈ 4935 <= 49true

The dominance threshold correctly separates these two shapes.


Verdict: No material issues found

After reading agent/finalize.go in full and tracing the key boundary cases:

  • Empty/whitespace inputs — every function entry calls strings.TrimSpace; citationLabelRe can't match "", so empty strings short-circuit at line 131.
  • msgs nil or emptyfor i := len(msgs)-1; i >= 0 doesn't execute; finalOutput falls back to terminal on !ok.
  • m.Text() on tool-result messages — returns "" (no Parts); the txt == "" guard on line 154 skips these immediately, no risk.
  • Integer division in isCitationsOnly (line 139)len(t)/3 truncates, 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.
  • Dedup check with TrimSpace'd terminal (line 50)tail cannot be empty here because isCitationsOnly returning true requires t to be non-empty and match a substantial regex.
  • recoverRatio*len(terminal) with len(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.
  • isSubstantiveAnswer preamble-before-length reordering — the PR intentionally moved the preamble guard before the recoverMinChars shortcut; the "long preamble is skipped" test covers this path.
  • Scan loop never short-circuits on non-qualifying intermediate turns — this is intentional and tested by the same case; the function returns the first qualifying turn found, skipping over short or preamble turns.

The unhappy paths are all handled: nothing better to recover → terminal returned as-is; citations terminal with only a preamble prior → sources returned 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:

  • No injection surface: result.Output is consumed downstream only via safe fmt.Printf("%s", ...), fmt.Println, and strings.Contains — no SQL, command, path, or template construction from the recovered text. Verified by grepping all .Output consumers across the repo.
  • No ReDoS risk: All four regexes (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 at finalize.go:60-79.
  • No untrusted data crossing a trust boundary: The terminal parameter comes from resp.Text() (model output), and msgs is the conversation transcript. The code rearranges model-generated text and returns it as result.Output — it never treats this text as trusted for any security-sensitive operation.
  • No secret/credential exposure: The code touches no secrets, keys, or credentials.
  • No insecure default: The new path activates only when the terminal matches a specific citations pattern; healthy terminals are returned unchanged (line 39-40).

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

  • citationResidueCutset is a standalone const separated from the const block below it (finalize.go:84 vs finalize.go:86-104). The file has two const declarations back-to-back: a single-constant const for the cutset, then a const (...) 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): citations is computed in finalOutput, passed to lastSubstantiveAssistantText, and then passed again to isSubstantiveAnswer where it gates the ratio check. Each function's signature now carries a citations bool parameter 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.

  • citationLabelRe character 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.Contains on the recovered answer, and some string concatenations — all inside finalOutput, 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.MatchString now runs on every candidate in isSubstantiveAnswer, 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:

  • Nil/empty inputs: isCitationsOnly("") returns false (no label match); finalOutput with nil msgs falls through to return terminal because lastSubstantiveAssistantText iterates zero times and returns ("", false). Both paths are safe.
  • Whitespace-only terminal: isCitationsOnly trims first, label match fails, falls through to isWeakFinal (existing behavior, unchanged).
  • Integer division: 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.
  • Regex safety: All regexp.MustCompile calls are package-level var declarations; the tests passing confirms they compile. \s inside [] is valid in Go's RE2 dialect (verified against Go regexp/syntax docs). The - at the end of [\s>#*_+-] is safely literal. Unicode characters in citationResidueCutset (·•—) are valid Go source and correctly handled by strings.Trim.
  • preambleRe before length check: The old code would accept a ≥200-char preamble as an answer (the preambleRe check was only in the floor/ratio branch). The new code checks preambleRe first — this fixes a real bug where a long planning narration could be recovered. Verified at finalize.go:174.
  • Dedup strings.Contains: Both rec and tail are trimmed before comparison. False negatives (different whitespace) are cosmetic at worst — the citations get duplicated rather than lost. Conservative.
  • No panics, no missing cleanup: All new code is pure functions with no resource acquisition.

The only thing that gave me pause was a theoretical false-positive in isCitationsOnly for 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).

  • The change operates entirely on model-generated transcript text and rewrites the user-facing string output. It performs no network fetch, no shell/command execution, no file/path access, no template evaluation, and no deserialization — so there is no SSRF, command-injection, path-traversal, or deserialization surface introduced. The "links" matched by linkRe are never fetched or resolved; they are only copied as text into the returned string.
  • The taint source (model output) is the same trust level as the existing terminal text that would already be returned verbatim, so recovery does not elevate any untrusted input into a more dangerous sink. Appending the citations addendum via rec + "\n\n" + tail is plain string concatenation with no interpolation/escaping concern.
  • Regex DoS on untrusted input: I checked both new regexes. linkRe = \[[^\]]*\]\([^)]*\)|https?://\S+ uses only negated-character-class quantifiers and \S+ — linear, no nested quantifiers, no catastrophic backtracking. citationLabelRe is ^-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.
  • No secrets/credentials are read, logged, or leaked; the appended tail is just the model's own citation text.

I verified by reading agent/finalize.go in 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 (and finalize_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 runs preambleRe.MatchString(txt) before the len(txt) >= recoverMinChars shortcut, 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 by lastSubstantiveAssistantText, so finalOutput falls 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: narrow preambleRe to 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 substring Contains, which can both over- and under-trigger. Confirmed at lines 46-53: tail := strings.TrimSpace(terminal) is checked via strings.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

  • The new isCitationsOnly/isSubstantiveAnswer additions follow the existing file's idioms (package-level regexp.MustCompile vars next to named threshold consts with doc comments, strings.TrimSpace normalization at entry). Naming is clear and the comments explain why, not just what.
  • The finalOutput control flow reads cleanly: the citations flag threads through lastSubstantiveAssistantTextisSubstantiveAnswer as a single bool with a documented meaning ("terminal is a citations addendum, not a rival answer"), avoiding a second parallel path.
  • Dedup logic at finalize.go:50 is localized and commented; the strings.Contains(rec, tail) guard is a reasonable low-churn approach for the stated duplicate-block case.
  • No dead/duplicated code introduced: linkRe is reused for both the link-presence check and the residue strip (finalize.go:135, :138); citationResidueCutset is a single shared cutset.
  • Test cases in finalize_test.go are well-structured (table-driven, named, with inline rationale comments tying each case to a design decision), consistent with the pre-existing TestIsWeakFinal/TestFinalOutput patterns.
Performance — No material issues found

VERDICT: No material issues found

  • Reviewed agent/finalize.go through the performance lens. finalOutput and 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 at agent/finalize.go:37-55.
  • All regexes (citationLabelRe, linkRe, backRefRe, preambleRe) are package-level compiled-once vars (agent/finalize.go:60,64,74,79); no per-call regexp.MustCompile.
  • isCitationsOnly does up to 4 regex passes (MatchString + two ReplaceAllString + MatchString) plus a strings.Trim over the terminal string — but the input is the single terminal message (typically a few hundred chars), so the allocations from ReplaceAllString are trivial and one-shot. Not material.
  • The moved preambleRe.MatchString check (agent/finalize.go:173-176) now runs on every candidate assistant turn instead of only the borderline band, but lastSubstantiveAssistantText scans the transcript once per run and the regex match is on a single message's text. Negligible.
  • No N+1, unbounded growth, missing limits, or quadratic behavior introduced. The 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 preambleRe at 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 the len(txt) >= recoverMinChars shortcut at lines 173-176, so a ≥200-byte substantive answer that opens with one of these phrases is now rejected by isSubstantiveAnswer on 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 the preambleRe check before the len(txt) >= recoverMinChars shortcut (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. preambleRe is (?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); now isSubstantiveAnswer returns false for it, lastSubstantiveAssistantText finds nothing, and finalOutput returns the degenerate terminal (a bare Sources: 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 new long preamble is skipped test; but the test suite has no case for a preamble-phrase-led substantive answer being recovered, so the regression is unguarded. Suggested mitigation: narrow preambleRe to 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 bare sure/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.

  • Regex safety: All four patterns (backRefRe, preambleRe, citationLabelRe, linkRe) are compiled with regexp.MustCompile and 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.
  • No new injection surface: finalOutput only 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.
  • No authz/authn or secrecy impact: lastSubstantiveAssistantText skips non-assistant roles, so recovering an earlier turn cannot leak system prompts or tool results that were not already emitted as assistant-visible text.
  • Deduplication is substring-only: 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 finalOutput control flow is clear (citations test → weak-final test → recovery → optional append), the new isCitationsOnly function 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 finalOutput and introduces no new allocations on hot paths, no loops over unbounded data, and no extra model calls.

Verified details:

  • isCitationsOnly operates on the single terminal string (typically <1 KiB) and is called once per Run. It does two regex matches and two ReplaceAllString passes on that short input — negligible overhead. agent/agent.go:319 confirms finalOutput is only invoked at loop termination.

  • lastSubstantiveAssistantText already scanned msgs newest→oldest before this PR; the only difference is an extra boolean parameter forwarded to isSubstantiveAnswer. The loop is bounded by conversation length (maxSteps, default small).

  • isSubstantiveAnswer now evaluates preambleRe.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 reading agent/finalize.go:64 — so the cost is O(prefix) and trivial even for long messages.

  • strings.Contains(rec, tail) in finalOutput (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:

  • The terminal string comes from LLM model output (resp.Text() at agent/agent.go:319), which is untrusted input
  • The code performs text classification and passthrough — URLs are preserved as plain text, not fetched or executed within this function
  • No SQL, command injection, or deserialization vectors exist in this text-processing code

Regex patterns examined for ReDoS risk:

  • linkRe (\[[^\]]*\]\([^)]*\)|https?://\S+) — uses negated character classes, no nested quantifiers; safe
  • citationLabelRe (^[\s>#*_+-]*(sources?|...)...) — linear scan with alternation on short literals; safe

Potential concerns evaluated:

  • SSRF: URLs from the model are preserved in output text but not fetched by this code. Any fetching occurs in the separate cite tool, outside this lens's scope.
  • Output injection: The terminal text is returned as-is. If downstream consumers render this in HTML without escaping, that's a separate layer's responsibility.
  • No authn/authz, secrets, or credential handling in this change

Verified by reading:

  • agent/finalize.go:37-54finalOutput logic confirmed as text classification/passthrough only
  • agent/finalize.go:129-140isCitationsOnly confirmed to only match/measure, not fetch or execute URLs
  • agent/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:

  • Read agent/finalize.go (lines 1-184) to verify the complete implementation
  • Read agent/finalize_test.go (lines 1-341) to verify test cases match the logic
  • Manually traced isCitationsOnly() with multiple test inputs to verify the dominance check math
  • Verified isSubstantiveAnswer() control flow: preamble check → length≥200 shortcut → length<80 rejection → ratio/citations check
  • Verified regex patterns (citationLabelRe, linkRe, preambleRe, backRefRe) match their documented behavior
  • Verified the citationDominatedDivisor=3 threshold correctly separates bare citation lists from prose-with-links

Key logic verified:

  1. isCitationsOnly dominance check: For "Sources: [a](url), [b](url).", after stripping label and links, residue is minimal (whitespace/punctuation only), correctly returning true. For "Source: According to https://cdc.gov the flu vaccine...", residue is substantial prose, correctly returning false.

  2. isSubstantiveAnswer preamble-first ordering: The preamble check now correctly applies BEFORE the recoverMinChars shortcut, preventing long planning narrations from being recovered as answers.

  3. Citations mode bypasses ratio check: When citations=true, the len(txt) >= recoverRatio*len(terminal) requirement is skipped, allowing concise answers (<200 chars) to be recovered against long citation terminals.

  4. Deduplication via strings.Contains: When the recovered turn already contains the citations verbatim, they are not appended twice.

  5. Bare domains intentionally excluded: The linkRe pattern requires http:// or https://, so bare domains like pcprice.watch don't match, correctly returning false per 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:

  • No dead/duplicated code: New functions (isCitationsOnly, updated isSubstantiveAnswer) are purposeful and not redundant.
  • Naming is clear: citationLabelRe, linkRe, citationResidueCutset, citationDominatedDivisor are self-documenting with thorough comments explaining their role.
  • Function structure is sound: finalOutput grew but remains readable; the control flow (citations check → weak check → recovery → append-if-citations) is logical and well-commented.
  • Constants are documented: All magic numbers (weakFinalMaxChars, recoverMinChars, citationDominatedDivisor, etc.) have comments explaining their purpose and derivation.
  • Test coverage follows patterns: Table-driven tests match the existing style in the file; 19 cases for TestIsCitationsOnly and 11 for TestFinalOutput cover edge cases without redundancy.
  • No leaky abstractions: The citations bool parameter passed through lastSubstantiveAssistantTextisSubstantiveAnswer is a clean way to thread the recovery mode context.

Verified by: Reading agent/finalize.go:1-184 and agent/finalize_test.go:1-341 in 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:

  1. 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.

  2. 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).

  3. lastSubstantiveAssistantText() loop — Still iterates messages newest→oldest, calling isSubstantiveAnswer per message. When citations=true, the ratio check (len(txt) >= recoverRatio*len(terminal)) is skipped, slightly reducing work per iteration.

  4. Dedup checkstrings.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.

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 6 models **Verdict: Minor issues** · 7 findings (1 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟠 | 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 | `agent/finalize.go:173` | 2/6 | correctness, error-handling | <details><summary>6 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟡 | citations dedup uses raw substring Contains(rec, tail); can drop or duplicate citations on minor text differences | `agent/finalize.go:50` | glm-5.2:cloud | correctness | | ⚪ | citations boolean threaded through three functions — mild boolean-parameter smell | `agent/finalize.go:38` | deepseek-v4-pro:cloud | maintainability | | ⚪ | citationLabelRe character class includes '+' without mentioning it in the comment | `agent/finalize.go:74` | deepseek-v4-pro:cloud | maintainability | | ⚪ | citationResidueCutset declared as standalone const instead of in the const block immediately below | `agent/finalize.go:84` | deepseek-v4-pro:cloud | maintainability | | ⚪ | // #N: annotations in TestIsCitationsOnly don't match table positions and reference an external review document | `agent/finalize_test.go:48` | claude-code/sonnet | maintainability | | ⚪ | longAnswer defined identically for the third time; a package-level var or helper would avoid the duplication | `agent/finalize_test.go:316` | claude-code/sonnet | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/opus</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> ## VERDICT: No material issues found Reviewed `agent/finalize.go` through 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 becomes `result.Output` (verified at `agent/agent.go:319`). No new trust boundary is crossed. Security-relevant checks I performed: - **ReDoS / regex safety** — All new patterns (`citationLabelRe`, `linkRe`) compile via `regexp.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`. - **Tainted-data sink** — The "links" `isCitationsOnly` extracts 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. - **Data exposure** — Recovery can now surface an *earlier* assistant turn instead of the terminal turn (`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. - **No auth/deserialization surface** — no serialization, no auth decisions, no untrusted structural parsing. 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)/divisor` integer-division edge — belong to the correctness lens and are out of scope for me.) </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> I've verified the finding against the actual code at `agent/finalize.go`. **Confirmed:** The diff's "before" state of `isSubstantiveAnswer` returned `true` unconditionally for any `len(txt) >= recoverMinChars` (200) turn, with the `preambleRe` check applied only in the borderline `< 200` band. The "after" state (lines 173–184) moves `if preambleRe.MatchString(txt) { return false }` to the very top, ahead of the length shortcut. `preambleRe` (line 64) includes `first[, ]`, `sure[,. ]`, and `okay[,. ]`, 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, and `terminal` is 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 moving `if preambleRe.MatchString(txt) { return false }` ahead of the `len(txt) >= recoverMinChars` shortcut, a front-loaded answer of ≥200 chars that legitimately begins with `"First, "` / `"Sure, "` / `"Okay, "` (all matched by `preambleRe`, `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: tighten `preambleRe` (e.g. drop/qualify `first`), or keep the unconditional accept for `len(txt) >= recoverMinChars` and only gate the borderline band on preamble as before. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> 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`: - **Follows existing patterns.** New regexes (`citationLabelRe`, `linkRe`) mirror the established `xxxRe` naming and the block-comment-per-regex style already used by `backRefRe`/`preambleRe`. New tuning constants sit in the same `const` block with rationale comments, consistent with `weakFinalMaxChars`/`recoverMinChars`. - **The `isSubstantiveAnswer` refactor is a net readability gain.** The old single `return A && B && C` expression was reworked into early-return guards (`finalize.go:173-184`), and moving `preambleRe` to the top is both a behavior fix and clearer to read. No dead code left behind — `recoverRatio`, `recoverFloorChars`, and `weakFinalMaxChars` all remain in use. - **Comments match the code.** I checked the `citationLabelRe` character class `[\s>#*_+-]*` against its doc comment (blockquote/ATX/emphasis/list markers) — accurate, not aspirational. The doc block on `finalOutput` correctly describes the new two-shape recovery and the "citations tested first" ordering. - **No copy-paste duplication.** The link-stripping logic is centralized in one `linkRe` reused 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): - `finalOutput` threads a `citations bool` down through `lastSubstantiveAssistantText` into `isSubstantiveAnswer` — 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. - `terminal` gets `strings.TrimSpace`'d in three spots (`isCitationsOnly`, `finalOutput` tail, `lastSubstantiveAssistantText`). Trivial redundancy; not worth restructuring. Nothing in my lens rises to a reportable defect. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> ## VERDICT: No material issues found I reviewed this change strictly through the **Performance** lens and verified it against the checked-out code. **Verification performed:** - `finalOutput` is invoked exactly once per agent run, at `agent/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 surrounding `Run` loop (`agent.go:290–324`). - All four regexes (`backRefRe`, `preambleRe`, `citationLabelRe`, `linkRe`) are package-level `regexp.MustCompile` vars — compiled once at init, not per call (`finalize.go:60,64,74,79`). No per-invocation compilation. - Go's `regexp` is RE2 (linear-time, no backtracking), so even the alternation in `linkRe` (`\[[^\]]*\]\([^)]*\)|https?://\S+`) and the char-class quantifiers in `citationLabelRe` cannot exhibit catastrophic backtracking / quadratic blowup regardless of input. - `isCitationsOnly` (`finalize.go:129`) does a bounded, constant number of string passes (`TrimSpace`, one `MatchString`, two `ReplaceAllString`, one `Trim`) 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 over `msgs` that stops at the first substantive turn — no nested iteration, no N+1, no unbounded accumulation. `m.Text()`/`TrimSpace` allocations are per-message on one pass, bounded by transcript length, executed once per run. **On the common (healthy) path:** the reordering now calls `isCitationsOnly` *before* the `isWeakFinal` short-circuit (`finalize.go:38–39`). For a normal terminal answer, this adds one `^`-anchored `citationLabelRe.MatchString` that fails immediately — a negligible, one-time cost. Not a regression worth flagging. The `rec + "\n\n" + tail` concatenation and `strings.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. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> 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 of `isSubstantiveAnswer` (line 174), *before* the `len(txt) >= recoverMinChars` shortcut (line 177). Pre-diff, a turn ≥200 chars returned `true` immediately and never hit the preamble check. Post-diff, a genuine long front-loaded answer opening with `First, ` / `Sure, ` / `Okay, ` (all matched by `preambleRe` at line 64) is rejected, and when it's the only substantive prior turn `finalOutput` falls through to `return 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 under `len(t)/3` when 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 turn `finalOutput` returns 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/nil `msgs` degrades cleanly to "return terminal", and integer division `len(t)/citationDominatedDivisor` can't divide by zero (the divisor is a const `3`). 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 hoists `preambleRe.MatchString(txt)` ahead of the `len(txt) >= recoverMinChars` shortcut, 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, `lastSubstantiveAssistantText` returns `false` and `finalOutput` falls through to `return 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 way `isWeakFinal` bounds back-references), or tighten `preambleRe` so enumeration/politeness openers followed by substantive content don't veto. </details> </details> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> 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 `regexp` package uses RE2 semantics — linear-time guarantees regardless of input. `citationLabelRe` is `^`-anchored (limits backtracking to the start of the string) and uses only simple character classes and bounded alternations. `linkRe` uses negated character classes (`[^\]]*`, `[^)]*`) with no nested quantifiers. ReDoS is structurally impossible here. - **No new external-input surface**: `finalOutput` / `isCitationsOnly` operate 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 into `msgs` are an existing design characteristic unmodified by this PR. - **No URL fetching**: `linkRe` identifies 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 `string` value. 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**: `lastSubstantiveAssistantText` only scans assistant-role messages (`llm.RoleAssistant`). No role boundary is crossed by the new citations path. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> 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: - Preamble-before-length ordering in `isSubstantiveAnswer` (`finalize.go:163–175`): preamble check fires before the `recoverMinChars` shortcut, closing the regression. - `isCitationsOnly` dominance check: the `len(residue) <= len(t)/citationDominatedDivisor` ratio correctly separates bare citation lists from prose answers that open with "Source:". - Dedup guard (`strings.Contains(rec, tail)`): logic confirmed sound. - `lastSubstantiveAssistantText` scan direction and skip conditions are correct. - `citations` flag routing (append vs discard) matches the described contract. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> 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 in `TestIsCitationsOnly`** `agent/finalize_test.go:48`, `:52`, `:55`, `:60`, `:63` Five 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 bold` instead of `// #4: colon after the bold`. --- **2. Third identical copy of `longAnswer` in `TestRun_RecoversFrontLoadedAnswerWithCitations`** `agent/finalize_test.go:316` ```go longAnswer := strings.TrimSpace(strings.Repeat("Free group calls are capped at sixty minutes. ", 6)) ``` This exact expression already appears at line 86 (inside `TestFinalOutput`) and line 267 (inside `TestRun_RecoversFrontLoadedAnswer`). The PR adds a third local copy rather than extracting a package-level `var` or a one-liner helper. Trivial to fix and keeps future test updates from drifting. --- </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **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. - All four regex patterns (`backRefRe`, `preambleRe`, `citationLabelRe`, `linkRe`) are compiled once at package init — correct. - `isCitationsOnly` runs two match→replace pairs on a single terminal string (verified: `citationLabelRe` matched then replaced, `linkRe` matched then replaced). A single `FindStringIndex`+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. - `lastSubstantiveAssistantText` scans messages newest→oldest, calling `m.Text()` only after the role guard (line 150). `m.Text()` builds a string via `strings.Builder` over 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. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> I've read the full implementation and tests. One thing to manually verify: whether the `strings.Trim` of 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-answer` test 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)` ≈ 156 - `citationLabelRe` strips `"Source: "`, leaving body starting with `"According to https://cdc.gov…"` - `linkRe` strips `https://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) against `len(t)/3 ≈ 49` → `35 <= 49` → **true** ✓ The dominance threshold correctly separates these two shapes. --- **Verdict: No material issues found** After reading `agent/finalize.go` in full and tracing the key boundary cases: - **Empty/whitespace inputs** — every function entry calls `strings.TrimSpace`; `citationLabelRe` can't match `""`, so empty strings short-circuit at line 131. - **`msgs` nil or empty** — `for i := len(msgs)-1; i >= 0` doesn't execute; `finalOutput` falls back to `terminal` on `!ok`. - **`m.Text()` on tool-result messages** — returns `""` (no `Parts`); the `txt == ""` guard on line 154 skips these immediately, no risk. - **Integer division in `isCitationsOnly` (line 139)** — `len(t)/3` truncates, 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. - **Dedup check with TrimSpace'd terminal (line 50)** — `tail` cannot be empty here because `isCitationsOnly` returning `true` requires `t` to be non-empty and match a substantial regex. - **`recoverRatio*len(terminal)` with `len(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. - **`isSubstantiveAnswer` preamble-before-length reordering** — the PR intentionally moved the preamble guard before the `recoverMinChars` shortcut; the `"long preamble is skipped"` test covers this path. - **Scan loop never short-circuits on non-qualifying intermediate turns** — this is intentional and tested by the same case; the function returns the first *qualifying* turn found, skipping over short or preamble turns. The unhappy paths are all handled: nothing better to recover → `terminal` returned as-is; citations terminal with only a preamble prior → `sources` returned as-is; dedup guard prevents double-appending when the recovered turn already embeds the citations. </details> </details> <details><summary><b>deepseek-v4-pro:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> ## Verdict: No material issues found This change is a pure text-processing heuristic operating on model-generated transcript text. Through the security lens: - **No injection surface**: `result.Output` is consumed downstream only via safe `fmt.Printf("%s", ...)`, `fmt.Println`, and `strings.Contains` — no SQL, command, path, or template construction from the recovered text. Verified by grepping all `.Output` consumers across the repo. - **No ReDoS risk**: All four regexes (`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 at `finalize.go:60-79`. - **No untrusted data crossing a trust boundary**: The `terminal` parameter comes from `resp.Text()` (model output), and `msgs` is the conversation transcript. The code rearranges model-generated text and returns it as `result.Output` — it never treats this text as trusted for any security-sensitive operation. - **No secret/credential exposure**: The code touches no secrets, keys, or credentials. - **No insecure default**: The new path activates only when the terminal matches a specific citations pattern; healthy terminals are returned unchanged (line 39-40). 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. </details> <details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 4: context deadline exceeded </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All three findings are confirmed against the actual source. Here is the verified review. --- **VERDICT: Minor issues** - **`citationResidueCutset` is a standalone `const` separated from the `const` block below it** (`finalize.go:84` vs `finalize.go:86-104`). The file has two `const` declarations back-to-back: a single-constant `const` for the cutset, then a `const (...)` 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`): `citations` is computed in `finalOutput`, passed to `lastSubstantiveAssistantText`, and then passed again to `isSubstantiveAnswer` where it gates the ratio check. Each function's signature now carries a `citations bool` parameter 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. - **`citationLabelRe` character 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. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> ## 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.Contains` on the recovered answer, and some string concatenations — all inside `finalOutput`, 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.MatchString` now runs on *every* candidate in `isSubstantiveAnswer`, 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. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> ## 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: - **Nil/empty inputs**: `isCitationsOnly("")` returns false (no label match); `finalOutput` with nil `msgs` falls through to `return terminal` because `lastSubstantiveAssistantText` iterates zero times and returns `("", false)`. Both paths are safe. - **Whitespace-only terminal**: `isCitationsOnly` trims first, label match fails, falls through to `isWeakFinal` (existing behavior, unchanged). - **Integer division**: `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. - **Regex safety**: All `regexp.MustCompile` calls are package-level `var` declarations; the tests passing confirms they compile. `\s` inside `[]` is valid in Go's RE2 dialect (verified against Go `regexp/syntax` docs). The `-` at the end of `[\s>#*_+-]` is safely literal. Unicode characters in `citationResidueCutset` (`·•—`) are valid Go source and correctly handled by `strings.Trim`. - **`preambleRe` before length check**: The old code would accept a ≥200-char preamble as an answer (the `preambleRe` check was only in the floor/ratio branch). The new code checks `preambleRe` first — this fixes a real bug where a long planning narration could be recovered. Verified at `finalize.go:174`. - **Dedup `strings.Contains`**: Both `rec` and `tail` are trimmed before comparison. False negatives (different whitespace) are cosmetic at worst — the citations get duplicated rather than lost. Conservative. - **No panics, no missing cleanup**: All new code is pure functions with no resource acquisition. The only thing that gave me pause was a theoretical false-positive in `isCitationsOnly` for 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. </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> 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). - The change operates entirely on model-generated transcript text and rewrites the user-facing string output. It performs no network fetch, no shell/command execution, no file/path access, no template evaluation, and no deserialization — so there is no SSRF, command-injection, path-traversal, or deserialization surface introduced. The "links" matched by `linkRe` are never fetched or resolved; they are only copied as text into the returned string. - The taint source (model output) is the same trust level as the existing terminal text that would already be returned verbatim, so recovery does not elevate any untrusted input into a more dangerous sink. Appending the citations addendum via `rec + "\n\n" + tail` is plain string concatenation with no interpolation/escaping concern. - Regex DoS on untrusted input: I checked both new regexes. `linkRe = \[[^\]]*\]\([^)]*\)|https?://\S+` uses only negated-character-class quantifiers and `\S+` — linear, no nested quantifiers, no catastrophic backtracking. `citationLabelRe` is `^`-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. - No secrets/credentials are read, logged, or leaked; the appended `tail` is just the model's own citation text. I verified by reading `agent/finalize.go` in 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. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> ## Verdict: Minor issues found I verified the two findings against the actual source in `agent/finalize.go` (and `finalize_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 runs `preambleRe.MatchString(txt)` *before* the `len(txt) >= recoverMinChars` shortcut, 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 by `lastSubstantiveAssistantText`, so `finalOutput` falls 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: narrow `preambleRe` to 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 substring `Contains`, which can both over- and under-trigger.** Confirmed at lines 46-53: `tail := strings.TrimSpace(terminal)` is checked via `strings.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. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> VERDICT: No material issues found - The new `isCitationsOnly`/`isSubstantiveAnswer` additions follow the existing file's idioms (package-level `regexp.MustCompile` vars next to named threshold consts with doc comments, `strings.TrimSpace` normalization at entry). Naming is clear and the comments explain *why*, not just *what*. - The `finalOutput` control flow reads cleanly: the `citations` flag threads through `lastSubstantiveAssistantText` → `isSubstantiveAnswer` as a single bool with a documented meaning ("terminal is a citations addendum, not a rival answer"), avoiding a second parallel path. - Dedup logic at `finalize.go:50` is localized and commented; the `strings.Contains(rec, tail)` guard is a reasonable low-churn approach for the stated duplicate-block case. - No dead/duplicated code introduced: `linkRe` is reused for both the link-presence check and the residue strip (`finalize.go:135`, `:138`); `citationResidueCutset` is a single shared cutset. - Test cases in `finalize_test.go` are well-structured (table-driven, named, with inline rationale comments tying each case to a design decision), consistent with the pre-existing `TestIsWeakFinal`/`TestFinalOutput` patterns. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> VERDICT: No material issues found - Reviewed `agent/finalize.go` through the performance lens. `finalOutput` and 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 at `agent/finalize.go:37-55`. - All regexes (`citationLabelRe`, `linkRe`, `backRefRe`, `preambleRe`) are package-level compiled-once vars (`agent/finalize.go:60,64,74,79`); no per-call `regexp.MustCompile`. - `isCitationsOnly` does up to 4 regex passes (`MatchString` + two `ReplaceAllString` + `MatchString`) plus a `strings.Trim` over the terminal string — but the input is the single terminal message (typically a few hundred chars), so the allocations from `ReplaceAllString` are trivial and one-shot. Not material. - The moved `preambleRe.MatchString` check (`agent/finalize.go:173-176`) now runs on every candidate assistant turn instead of only the borderline band, but `lastSubstantiveAssistantText` scans the transcript once per run and the regex match is on a single message's text. Negligible. - No N+1, unbounded growth, missing limits, or quadratic behavior introduced. The `strings.Contains(rec, tail)` dedup is a single substring scan over the recovered answer, one-shot. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> The finding is confirmed against the actual code. The `preambleRe` at 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** the `len(txt) >= recoverMinChars` shortcut at lines 173-176, so a ≥200-byte substantive answer that opens with one of these phrases is now rejected by `isSubstantiveAnswer` on 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 the `preambleRe` check **before** the `len(txt) >= recoverMinChars` shortcut (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. `preambleRe` is `(?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); now `isSubstantiveAnswer` returns false for it, `lastSubstantiveAssistantText` finds nothing, and `finalOutput` returns the degenerate terminal (a bare `Sources:` 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 new `long preamble is skipped` test; but the test suite has no case for a preamble-phrase-led *substantive* answer being recovered, so the regression is unguarded. Suggested mitigation: narrow `preambleRe` to 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 bare `sure`/`okay`/`first`/`let me`, so conversational answer openers are not misclassified as planning narration. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** Through a security lens this change is clean. - **Regex safety**: All four patterns (`backRefRe`, `preambleRe`, `citationLabelRe`, `linkRe`) are compiled with `regexp.MustCompile` and 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. - **No new injection surface**: `finalOutput` only 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. - **No authz/authn or secrecy impact**: `lastSubstantiveAssistantText` skips non-assistant roles, so recovering an earlier turn cannot leak system prompts or tool results that were not already emitted as assistant-visible text. - **Deduplication is substring-only**: `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. </details> <details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 7: context deadline exceeded </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> No material issues found The change is clean and well-structured for its lane. The `finalOutput` control flow is clear (citations test → weak-final test → recovery → optional append), the new `isCitationsOnly` function is cohesive with a single responsibility, and the constants/regexes are documented with explicit rationale. Tests are table-driven and comprehensive. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** The change is confined to transcript-recovery heuristics in `finalOutput` and introduces no new allocations on hot paths, no loops over unbounded data, and no extra model calls. Verified details: - `isCitationsOnly` operates on the single terminal string (typically <1 KiB) and is called **once** per `Run`. It does two regex matches and two `ReplaceAllString` passes on that short input — negligible overhead. `agent/agent.go:319` confirms `finalOutput` is only invoked at loop termination. - `lastSubstantiveAssistantText` already scanned `msgs` newest→oldest before this PR; the only difference is an extra boolean parameter forwarded to `isSubstantiveAnswer`. The loop is bounded by conversation length (`maxSteps`, default small). - `isSubstantiveAnswer` now evaluates `preambleRe.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 reading `agent/finalize.go:64` — so the cost is O(prefix) and trivial even for long messages. - `strings.Contains(rec, tail)` in `finalOutput` (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). </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> No material issues found. </details> </details> <details><summary><b>qwen3.5:397b-cloud</b> (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> ## VERDICT: No material issues found After reviewing the change through the security lens: **Analysis of untrusted data handling:** - The `terminal` string comes from LLM model output (`resp.Text()` at `agent/agent.go:319`), which is untrusted input - The code performs text classification and passthrough — URLs are preserved as plain text, not fetched or executed within this function - No SQL, command injection, or deserialization vectors exist in this text-processing code **Regex patterns examined for ReDoS risk:** - `linkRe` (`\[[^\]]*\]\([^)]*\)|https?://\S+`) — uses negated character classes, no nested quantifiers; safe - `citationLabelRe` (`^[\s>#*_+-]*(sources?|...)...`) — linear scan with alternation on short literals; safe **Potential concerns evaluated:** - **SSRF**: URLs from the model are preserved in output text but not fetched by this code. Any fetching occurs in the separate `cite` tool, outside this lens's scope. - **Output injection**: The terminal text is returned as-is. If downstream consumers render this in HTML without escaping, that's a separate layer's responsibility. - **No authn/authz, secrets, or credential handling** in this change **Verified by reading:** - `agent/finalize.go:37-54` — `finalOutput` logic confirmed as text classification/passthrough only - `agent/finalize.go:129-140` — `isCitationsOnly` confirmed to only match/measure, not fetch or execute URLs - `agent/agent.go:319` — confirmed input source is model response text </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> ## 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:** - Read `agent/finalize.go` (lines 1-184) to verify the complete implementation - Read `agent/finalize_test.go` (lines 1-341) to verify test cases match the logic - Manually traced `isCitationsOnly()` with multiple test inputs to verify the dominance check math - Verified `isSubstantiveAnswer()` control flow: preamble check → length≥200 shortcut → length<80 rejection → ratio/citations check - Verified regex patterns (`citationLabelRe`, `linkRe`, `preambleRe`, `backRefRe`) match their documented behavior - Verified the `citationDominatedDivisor=3` threshold correctly separates bare citation lists from prose-with-links **Key logic verified:** 1. **`isCitationsOnly` dominance check**: For `"Sources: [a](url), [b](url)."`, after stripping label and links, residue is minimal (whitespace/punctuation only), correctly returning `true`. For `"Source: According to https://cdc.gov the flu vaccine..."`, residue is substantial prose, correctly returning `false`. 2. **`isSubstantiveAnswer` preamble-first ordering**: The preamble check now correctly applies BEFORE the `recoverMinChars` shortcut, preventing long planning narrations from being recovered as answers. 3. **Citations mode bypasses ratio check**: When `citations=true`, the `len(txt) >= recoverRatio*len(terminal)` requirement is skipped, allowing concise answers (<200 chars) to be recovered against long citation terminals. 4. **Deduplication via `strings.Contains`**: When the recovered turn already contains the citations verbatim, they are not appended twice. 5. **Bare domains intentionally excluded**: The `linkRe` pattern requires `http://` or `https://`, so bare domains like `pcprice.watch` don't match, correctly returning `false` per the documented limitation. All test cases align with the implementation logic. No logic bugs, incorrect behavior, or semantic errors found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> ## Verdict: No material issues found After reviewing the changes through the code cleanliness & maintainability lens: - **No dead/duplicated code**: New functions (`isCitationsOnly`, updated `isSubstantiveAnswer`) are purposeful and not redundant. - **Naming is clear**: `citationLabelRe`, `linkRe`, `citationResidueCutset`, `citationDominatedDivisor` are self-documenting with thorough comments explaining their role. - **Function structure is sound**: `finalOutput` grew but remains readable; the control flow (citations check → weak check → recovery → append-if-citations) is logical and well-commented. - **Constants are documented**: All magic numbers (`weakFinalMaxChars`, `recoverMinChars`, `citationDominatedDivisor`, etc.) have comments explaining their purpose and derivation. - **Test coverage follows patterns**: Table-driven tests match the existing style in the file; 19 cases for `TestIsCitationsOnly` and 11 for `TestFinalOutput` cover edge cases without redundancy. - **No leaky abstractions**: The `citations bool` parameter passed through `lastSubstantiveAssistantText` → `isSubstantiveAnswer` is a clean way to thread the recovery mode context. **Verified by**: Reading `agent/finalize.go:1-184` and `agent/finalize_test.go:1-341` in full; confirmed no unchecked symbols, no structural issues, and test cases align with the implementation. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **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: 1. **`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. 2. **`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). 3. **`lastSubstantiveAssistantText()` loop** — Still iterates messages newest→oldest, calling `isSubstantiveAnswer` per message. When `citations=true`, the ratio check (`len(txt) >= recoverRatio*len(terminal)`) is **skipped**, slightly reducing work per iteration. 4. **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. </details> <details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 10: context deadline exceeded </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-10 16:31:27 +00:00
fix(agent): address gadfly — don't preamble-veto long answers; dedup ignores <url> wrappers
CI / Build & Test (pull_request) Failing after 5m40s
CI / Tidy (pull_request) Successful in 9m27s
88b94576af
- 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>
steve added 1 commit 2026-07-10 16:33:32 +00:00
test(agent): guard the no-preamble-veto-for-long-answers case; reword comments (gadfly)
CI / Build & Test (pull_request) Successful in 10m24s
CI / Tidy (pull_request) Successful in 9m31s
440bff7bdd
Add "long answer opening with a conversational word is still recovered"
(regression guard); retarget the skipped-preamble case to the borderline band;
drop the external "#N" annotations in favor of self-explanatory comments.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
steve merged commit 6abb399f5b into main 2026-07-10 16:47:32 +00:00
steve deleted branch fix/finalize-citations-only-terminal 2026-07-10 16:47:32 +00:00
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: steve/majordomo#11