mort run b3cb9ee9 (glm-5.2, the cite pattern): the model wrote its full 2,089-char answer in the same turn as its cite calls, then closed — after the cite results — with 151 chars:
Citations are logged. Short version: the bulk of that ~$64M was AIPAC and dark-money super PACs, not the party committees — and it still wasn't enough.
That closer is all the user got. finalOutput recovers two degenerate terminal shapes (back-reference, citations-only addendum), and this is neither: over the 120-char back-ref cap with no back-ref phrase, and "Citations are logged." carries no label-colon and no links.
The fix
A third shape, the summary closer: a bookkeeping ack of the citation round, optionally followed by a one-line compression of the answer the model already wrote.
isSummaryCloser keys on the ack sentence alone — the verb must end the sentence ([.!] immediately after), so "The citations are recorded in the court transcript…" (prose about citations) never matches. A compression marker without the ack ("Short version: no.") is deliberately out of scope: a user who asked for brevity gets answered in exactly that shape. Fail closed — an unmatched closer keeps today's behavior.
The citations/back-ref bool becomes a three-way recoveryMode. modeSummary makes the 3× dwarf ratio mandatory at every length: unlike a back-reference, this closer carries real answer content, so it is only displaced by the clearly-fuller original it compressed. Both existing modes behave exactly as before.
The closer is discarded on recovery (its content is a strict subset), mirroring the back-ref arm, not the citations arm.
Tests
Classifier table (verbatim b3cb9ee9 terminal, ack variants, the prose near-misses), four new finalOutput cases including "kept when the prior turn does not dwarf it", and an end-to-end fake-provider run reproducing b3cb9ee9. Break-checked: disabling the classifier fails all the new cases. Gates green locally including -race.
## The failure
mort run `b3cb9ee9` (glm-5.2, the cite pattern): the model wrote its full 2,089-char answer in the same turn as its `cite` calls, then closed — after the cite results — with 151 chars:
> Citations are logged. Short version: the bulk of that ~$64M was AIPAC and dark-money super PACs, not the party committees — and it still wasn't enough.
That closer is all the user got. `finalOutput` recovers two degenerate terminal shapes (back-reference, citations-only addendum), and this is neither: over the 120-char back-ref cap with no back-ref phrase, and "Citations are logged." carries no label-colon and no links.
## The fix
A third shape, the **summary closer**: a bookkeeping ack of the citation round, optionally followed by a one-line compression of the answer the model already wrote.
- `isSummaryCloser` keys on the ack sentence alone — the verb must end the sentence (`[.!]` immediately after), so "The citations are recorded in the court transcript…" (prose *about* citations) never matches. A compression marker without the ack ("Short version: no.") is deliberately out of scope: a user who asked for brevity gets answered in exactly that shape. Fail closed — an unmatched closer keeps today's behavior.
- The citations/back-ref bool becomes a three-way `recoveryMode`. `modeSummary` makes the 3× dwarf ratio **mandatory at every length**: unlike a back-reference, this closer carries real answer content, so it is only displaced by the clearly-fuller original it compressed. Both existing modes behave exactly as before.
- The closer is discarded on recovery (its content is a strict subset), mirroring the back-ref arm, not the citations arm.
## Tests
Classifier table (verbatim b3cb9ee9 terminal, ack variants, the prose near-misses), four new `finalOutput` cases including "kept when the prior turn does not dwarf it", and an end-to-end fake-provider run reproducing b3cb9ee9. Break-checked: disabling the classifier fails all the new cases. Gates green locally including `-race`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
A third degenerate terminal shape from the glm-5.2 cite pattern: the model
front-loads its full answer into the cite-call turn, then closes with a
bookkeeping ack plus a one-line compression ("Citations are logged. Short
version: ..."). mort run b3cb9ee9 delivered 151 chars of a 2,089-char
answer this way — the closer was neither a back-reference (over the 120
cap, no back-ref phrase) nor a citations addendum (no label-colon, no
links), so finalOutput let it stand.
isSummaryCloser keys on the ack sentence alone (the verb must end the
sentence, so prose about citations never matches; a compression marker
without the ack is deliberately out of scope), and the new modeSummary
recovery bar makes the 3x dwarf ratio mandatory at every length: unlike a
back-reference this closer carries real answer content, so it is only
displaced by the clearly-fuller original it compressed.
The citations/back-ref bool becomes a three-way recoveryMode; existing
behavior for both old modes is unchanged.
Co-Authored-By: Claude Fable 5 <[email protected]>
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
4/4 reviewers finished · updated 2026-08-06 00:27:21Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **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
#### `glm-5.2:cloud` · ollama-cloud — ✅ 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
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
Verdict: Minor issues · 6 findings (2 with multi-model agreement)
Finding
Where
Models
Lens
🟡
Leading-marker char class '[\s>#_+-]' duplicated between summaryLead and citationLabelRe (comment admits the copy); hoist a shared const
agent/finalize.go:130
3/4
maintainability
⚪
Empty isWeakFinal case relies on pre-initialized mode; explicit 'mode = modeBackRef' would be more self-contained
agent/finalize.go:54
2/4
maintainability
4 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
summaryCloserRe regex can false-positive on prose where [.] is not end-of-sentence
agent/finalize.go:139
kimi-k2.6:cloud
error-handling
🟡
Summary closer is discarded (not appended), so a closer carrying a net-new correction/addendum beyond the front-loaded answer is silently lost; the 3x dwarf ratio is a length guard, not a containment guard
agent/finalize.go:75
claude-code/opus
error-handling
🟡
Duplicate test fixture construction across multiple test functions
agent/finalize_test.go:126
kimi-k2.6:cloud
maintainability
🟡
External review reference embedded in permanent test comment
agent/finalize_test.go:357
kimi-k2.6:cloud
maintainability
Per-model detailclaude-code/opus (claude-code) — Minor issues
I traced the full data flow of this change. finalOutput (called at agent/agent.go:333) operates purely on model-generated assistant text and conversation history, and returns a string that is the user-facing answer. Through the security lens there is no material issue:
No injection sinks. The recovered text is only string-classified (regex match, length compare) and concatenated (rec + "\n\n" + tail at finalize.go:72). It is never used in SQL, a shell/command, a filesystem path, a template, or a deserializer. Nothing is executed or interpolated into a sensitive context.
No ReDoS. All patterns compile via Go's regexp (RE2 — finalize.go:4,139), which guarantees linear-time matching, so the new nested-star fragments (summaryCopulas = ((are|were|have\s+been|all)\s+)*, summaryArticle = ((all|the)\s+)*, finalize.go:134,136) cannot backtrack catastrophically. The summary path is additionally length-capped at summaryCloserMaxChars = 300 before the regex runs (finalize.go:240). No untrusted-input-driven CPU blowup.
No authn/authz, SSRF, deserialization, or secret handling touched by this diff — it adds a classifier and a recovery mode; no network, credential, or trust-boundary code.
Information-disclosure boundary is tightened, not loosened. The one place this change could conceivably surface the wrong content is cross-question recovery, and the new modeSummary user-role boundary at finalize.go:254-262 explicitly stops the scan at the most recent user message so a compressed closer can never resurrect an answer to a different question. The pre-existing unbounded scan for modeBackRef/modeCitations is unchanged by this PR (not introduced here), and in any case only re-surfaces the same conversation's own prior assistant text.
Verification: read the full agent/finalize.go at PR state and confirmed the sole caller (agent/agent.go:333) passes model output, not external/user-authz-bearing data; confirmed RE2 engine via the regexp import; confirmed the 300-byte pre-filter bounds the new path.
Nothing in the security lane is materially wrong here.
🎯 Correctness — No material issues found
I've reviewed the code, verified message roles, traced the run loop, and worked through the classifier regex and the dwarf-ratio logic against every new test case.
VERDICT: No material issues found
I focused on the correctness of the new third recovery shape (modeSummary), the mode precedence, the dwarf-ratio math, and the user-boundary scan.
Verified sound:
Mode precedence matches the documented contract. The switch in finalOutput (finalize.go:51) tests isCitationsOnly → isWeakFinal → isSummaryCloser in that order. isWeakFinal (finalize.go:197) requires backRefReandlen ≤ 120, so a back-reference within the weak cap correctly pre-empts the summary arm. Confirmed against the bothMatchCloser test: it's ~106 bytes and matches as i mentioned, so it routes to modeBackRef, and longAnswer (275 B) is recovered via the ≥200 shortcut in isSubstantiveAnswer, not the 3× bar.
Dwarf ratio re-derived and correct.dwarfs := len(txt) >= recoverRatio*len(terminal) with recoverRatio=3 (finalize.go:295,182). For the 153-byte b3cb9ee9Closer, the bar is 459 B: hugeAnswer (12×46−1 = 551 B) clears it → recovered; longAnswer (275 B) does not → closer kept. Both match the corresponding test expectations. Byte-vs-byte comparison is consistent throughout (em dash = 3 bytes), and the 153/151/300 magic numbers all check out.
User-boundary break is safe. The mode == modeSummary && m.Role == llm.RoleUser break (finalize.go:254) does not misfire on tool results: ToolResultsMessage is RoleTool, not RoleUser (llm/message.go:69–70), so the scan still reaches the front-loaded assistant turn that sits between the user question and the tool-result message. Verified the run loop builds exactly that ordering (agent.go:278,325,364). The break correctly prevents resurrecting a stale answer to a prior question, as its dedicated test exercises.
Classifier regex has no false-positive hole I could find.summaryCloserRe requires the bookkeeping verb (or noun, in the logged … branch) to be immediately followed by [.!], so prose about citations ("…are recorded in the court transcript…", "…logged in Zotero whenever…") fails because a space, not a terminator, follows the verb. Walked every true/false row in TestIsSummaryCloser by hand against the assembled pattern — all consistent.
No regression vs. pre-PR behavior. A back-reference-flavored closer over the 120-byte weak cap was already returned as-is before this change (it's neither weak nor citations-only); the new summary arm can only add recovery when a prior turn dwarfs it, never remove existing recovery. So the mode split doesn't degrade any previously-handled case; modeBackRef/modeCitations paths are byte-for-byte equivalent to before.
Nothing in the correctness lens is materially wrong.
🧹 Code cleanliness & maintainability — Minor issues
I've verified all three findings against the actual source in agent/finalize.go.
Finding 1 (line 131): Confirmed — summaryLead = [\s>#_+-]`` (line 131) is byte-for-byte identical to the leading class in citationLabelRe (line 156: ^[\s>#*_+-]*(sources?...), and the inline comment does say "as in citationLabelRe."
Finding 2 (lines 134/136): Confirmed — summaryCopulas (line 134) lists all alongside are|were|have been, and summaryArticle (line 136) also matches all.
Finding 3 (lines 54-55): Confirmed — mode := modeBackRef at line 50, then an empty case isWeakFinal(terminal): with only a // modeBackRef comment.
All three are accurate readability/naming nits. All survive.
VERDICT: Minor issues
This is unusually well-documented, well-structured code — the three-way recoveryMode refactor is a clean generalization of the prior two-way bool, naming is clear (modeBackRef/modeCitations/modeSummary), the fragment-assembled regex is more legible than a monolith, and the doc comments accurately match the control flow (I traced the switch order against the "citations first, back-ref beats summary" claims — they're correct). The findings below are all minor.
agent/finalize.go:131 / :156 — duplicated leading-marker character class.summaryLead = [\s>#_+-] is a byte-for-byte copy of the leading class in `citationLabelRe`, and the comment (`// … as in citationLabelRe`) explicitly acknowledges the copy. Two independent literals for "the same set of leading markdown/list markers" will drift if one is ever extended. Low-churn fix: hoist a shared `const leadMarkers = `[\s>#*_+-]* and reference it from both summaryLead and citationLabelRe.
agent/finalize.go:134,136 — all appears in both summaryCopulas and summaryArticle, and it isn't a copula.summaryCopulas = ((are|were|have\s+been|all)\s+)* lists `all` alongside the genuine copulas `are/were/have been`, while `summaryArticle = `((all|the)\s+)* also matches all. A reader parsing the grammar sees the same token admitted in two fragments under a misleading name (a "copula" fragment containing a quantifier word). Consider dropping all from summaryCopulas (or renaming the fragment) so each named piece owns one grammatical role. This is a readability/naming nit, not a matching-behavior claim.
agent/finalize.go:54-55 — empty case body relying on the pre-initialized mode.
caseisWeakFinal(terminal):// modeBackRef
This depends on mode := modeBackRef set two lines up; the comment documents it, but an explicit mode = modeBackRef in the case body is more self-contained and survives a future refactor that changes the initializer. Trivial, optional.
No issues with the test additions from a cleanliness standpoint — the new cases are named descriptively and the hugeAnswer/b3cb9ee9Closer fixtures read clearly (the local re-declaration of hugeAnswer across two test funcs is normal Go test style).
⚡ Performance — No material issues found
I've verified the code and the single call site. Here's my performance-lens review.
VERDICT: No material issues found
This change adds a terminal-turn classifier (isSummaryCloser), a new regex, a recoveryMode enum, and a user-boundary early-break to the recovery scan. Through the performance lens it is clean:
Called once per run, off any hot path.finalOutput runs exactly once, at agent/agent.go:333, when the loop reaches a clean terminal turn — not inside the model/tool loop. The added work executes once per agent run over a bounded transcript. Verified by grepping the only caller.
Regex compiled once.summaryCloserRe is a package-level var (finalize.go:139), compiled at init, not per-call. The nested-star alternations (summaryCopulas, summaryArticle use * over alternations) would be a catastrophic-backtracking (ReDoS) risk under a PCRE-style engine, but Go's regexp is RE2 — guaranteed linear time, no backtracking — so there is no pathological-input blowup. Not a performance finding.
Length guard precedes the regex.isSummaryCloser returns early when len(t) > summaryCloserMaxChars (300) before ever running the match (finalize.go:240), so regex work is bounded to short strings. Good ordering.
Scan stays O(n) and got cheaper, not costlier.lastSubstantiveAssistantText is a single newest→oldest pass. The new modeSummarybreak at the user-message boundary (finalize.go:254) only shortens the scan; per-iteration cost (TrimSpace, Text(), short-string checks) is unchanged. No added allocations inside the loop that matter at this scale.
No N+1, no I/O, no unbounded growth. All work is in-memory string classification over the existing transcript slice; the classifier order (isCitationsOnly → isWeakFinal → isSummaryCloser) short-circuits, running at most a few small regex matches on the terminal string.
Nothing here warrants a change on efficiency grounds.
🧯 Error handling & edge cases — Minor issues
I've verified the code at agent/finalize.go. The single surviving finding checks out against the actual source:
finalOutput at line 75 returns bare rec for modeSummary (and modeBackRef), while modeCitations alone appends the terminal via rec + "\n\n" + tail at line 72 — confirmed.
The modeSummary gate is purely length-based: dwarfs := len(txt) >= recoverRatio*len(terminal) (line 295), a length comparison, not a containment/substring test — confirmed.
The modeSummary doc (lines 100–108) and isSummaryCloser doc (lines 231–237) explicitly state the closer is discarded on the assumption its content is a strict subset of the recovered turn — confirmed the tradeoff is deliberate.
The scenario the finding describes (a short closer carrying a net-new correction, discarded when a longer front-loaded answer dwarfs it) is a genuine, reproducible consequence of the length-only guard. The finding accurately reflects the code and is honestly scoped as a low-severity, deliberate tradeoff. It survives.
VERDICT: Minor issues
Reviewing strictly through the error handling & edge cases lens.
Most unhappy paths here are handled defensively and I could not fault them:
Empty / short / oversized inputs — isSummaryCloser guards t == "" and len(t) > summaryCloserMaxChars before touching the regex (finalize.go:240); empty msgs falls through the scan loop to ("", false) → returns terminal. No panic paths, no slice indexing, no div-by-zero (divisor is a const 3). Verified by reading.
Overflow — recoverRatio*len(terminal) (finalize.go:295) is bounded (summary terminal ≤ 300 bytes); no realistic overflow.
fail-closed behavior — when nothing dwarfs, or the user-boundary break (finalize.go:254) is hit first, recovery returns ("", false) and the closer is preserved. Correct and matches the documented intent.
Mode precedence — the switch in finalOutput (finalize.go:51) orders citations → weak/back-ref → summary, so a terminal matching both back-ref and the ack is routed to modeBackRef and never subjected to the stricter dwarf bar. Verified against the regex anchors and the doc.
One genuine edge case is left unhandled:
finalize.go:56,75 — a summary closer carrying net-new content is silently discarded. Unlike modeCitations, which appends the terminal (rec + "\n\n" + tail, line 72), modeSummary falls through to return rec (line 75) and drops the closer entirely, on the stated assumption that "its content is a strict subset" of the recovered turn. That assumption is only guarded by the 3× dwarf ratio (len(txt) >= recoverRatio*len(terminal), line 295), which is a length test, not a containment test. A model that writes "Citations are logged. One correction: the figure was $46M, not $64M." as its closer, after a long (dwarfing) front-loaded answer, would have that correction thrown away — the user receives the stale, uncorrected front-loaded text. The citations arm avoids exactly this by keeping the terminal; the summary arm cannot, because it has no way to tell "compression of the above" from "compression + a delta." Impact: possible loss of the most recent/authoritative content on precisely the unhappy path this diff targets. This is a documented, deliberate tradeoff, so I rate it low: severity small, confidence medium. A cheap mitigation would be to append the closer (as citations do) rather than discard it, or to only discard when the closer text is substring-contained in rec.
Everything else in my lane checks out. The classifier's false-positive surface (prose about citations, mid-sentence mentions, compression-without-ack) is explicitly bounded by the [.!]-directly-after-verb anchor and the length cap, and the tests exercise those boundaries.
Reviewed agent/finalize.go (and the new summaryCloserRe/isSummaryCloser logic in the diff) specifically through the security lens: authn/authz, injection, SSRF, deserialization, secret leakage, input validation, and unsafe handling of untrusted data.
Findings:
No new I/O, deserialization, external calls, or credential handling is introduced — the change is pure string/regex classification over model-generated conversation text already in memory.
The new regexes (summaryCloserRe, reusing existing summaryLead/summaryArticle/summaryCopulas fragments) are compiled via Go's standard regexp package, which is RE2-based and backtracking-free — so even though the fragments contain nested */+ quantifiers (summaryArticle, summaryCopulas), there's no catastrophic-backtracking/ReDoS exposure regardless of adversarial input length. Verified by reading the pattern construction at agent/finalize.go:130-141 and confirming Go's regexp engine guarantees linear-time matching (not regexp/syntax backtracking).
isSummaryCloser bounds input length before regex evaluation (len(t) > summaryCloserMaxChars at agent/finalize.go:240), which further limits any per-call cost — though as noted this isn't actually needed for RE2 safety.
The text being classified is the agent's own prior/terminal assistant turns pulled from msgs, not raw external/untrusted content being rendered, executed, or used to construct a query — there's no injection sink here (no SQL, shell, template, or URL construction from this data).
The modeSummary user-boundary scan (agent/finalize.go:254-262) is a correctness/data-scoping control, not a trust-boundary check, and doesn't weaken any authorization boundary — it only affects which of the agent's own messages are eligible to be substituted as output.
No secrets, credentials, or user-supplied data paths are touched by this diff. Nothing in this PR falls within the security lens as a genuine defect.
🎯 Correctness — No material issues found
Both helpers already exist and are reused correctly, not redefined. This confirms the diff is self-consistent.
I've traced the byte-length math (b3cb9ee9Closer = 153 bytes / 151 runes, verified by hand), the dwarf-ratio arithmetic for longAnswer/hugeAnswer against the 3× threshold, the summaryCloserRe regex against every positive/negative test case, the mode-dispatch priority in finalOutput, the modeSummary early-return gate in isSubstantiveAnswer, the user-boundary scan-stop (confirmed ToolResultsMessage uses RoleTool, distinct from RoleUser, so it doesn't prematurely trip the boundary check), and cross-checked that modeBackRef/modeCitations behavior is algebraically identical to the pre-PR code. Everything lines up with the documented intent and the tests' expected outcomes.
VERDICT: No material issues found
No correctness defects surfaced. Specific things I verified rather than assumed:
b3cb9ee9Closer is exactly 153 bytes (Go len()) / 151 runes by manual count — matches the code comment and confirms 3×153=459 is the real dwarf threshold used against longAnswer (275 bytes, correctly fails) and hugeAnswer (551 bytes, correctly passes).
summaryCloserRe (agent/finalize.go:139-141) correctly rejects all "verb not immediately followed by [.!]" near-miss cases (e.g. "recorded in the court transcript", "logged in Zotero") because RE2 must find some full match path, and none exists when the verb isn't immediately adjacent to a sentence terminator — traced this against all four negative regex test cases.
The modeSummary dwarf-gate in isSubstantiveAnswer (finalize.go:294-306) is applied before the recoverMinChars/floor checks, so it truly is mandatory at every length, not just short ones — confirmed modeBackRef/modeCitations reduce to byte-identical boolean expressions vs. the pre-PR citations bool version.
The scan-stop-at-user-boundary logic (finalize.go:254-263) does not falsely trip on llm.ToolResultsMessage, because that constructor sets Role: llm.RoleTool (llm/message.go:70), a distinct value from RoleUser — this was the one place I suspected a real bug (Anthropic's wire format sends tool results as a user-role turn) but the canonical llm.Message type keeps them separate, so no bug.
Traced all 4 new finalOutput table cases and the e2e fake-provider test by hand against the actual message slices and confirmed the want values match what the code produces.
🧹 Code cleanliness & maintainability — Minor issues
Both findings check out against the actual source. The switch at finalize.go:51-60 is the file's only switch statement, confirming the empty-body case is a one-off; and the summaryLead literal at line 131 is byte-for-byte identical to the leading fragment embedded in citationLabelRe at line 156.
VERDICT: Minor issues
agent/finalize.go:54-55 — The case isWeakFinal(terminal): // modeBackRef branch has an empty body and relies on the reader noticing that mode was pre-initialized to modeBackRef on line 50, several lines above the switch. Every other case explicitly assigns mode (mode = modeCitations, mode = modeSummary), so this one silently breaks the pattern; a future edit that reorders cases, changes the recoveryMode iota order, or adds a fourth mode is one easy slip away from this branch doing the wrong thing with no compiler warning. Prefer mode = modeBackRef in the case body for symmetry with the other two arms — it costs nothing and removes the implicit dependency on the pre-switch initializer.
agent/finalize.go:131 vs agent/finalize.go:156 — summaryLead duplicates the exact literal character class (`[\s>#*_+-]*`) already inlined in citationLabelRe. The new code's own comment ("leading markdown/list markers, as in citationLabelRe") acknowledges the duplication rather than removing it. Since this PR is already restructuring the file's regexes into named fragments for legibility, this was a natural point to hoist the shared class into one identifier (e.g. leadingMarkersRe) used by both summaryCloserRe and citationLabelRe, so a future tweak to what counts as a "leading marker" doesn't require remembering to update it in two places.
Everything else in the diff reads cleanly for a feature of this size: the citations bool → recoveryMode enum rename is a real readability win over the old boolean, the new regex is assembled from named fragments consistent with the file's existing citationLabelRe style, and the extensive doc comments match this file's established convention of explaining classifier rationale inline.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed agent/finalize.go and agent/finalize_test.go through the performance lens.
The new modeSummary path adds one more regexp.MustCompile'd pattern (summaryCloserRe, compiled once at package init — not per-call) and one more strings.TrimSpace + regex match per terminal turn (isSummaryCloser), gated behind a len(t) > summaryCloserMaxChars early-out. This is O(len(terminal)) work done once per agent turn, not in a loop — negligible.
lastSubstantiveAssistantText (finalize.go:250-276) is unchanged in complexity: still a single O(n) newest→oldest scan over msgs. The new modeSummary early-break on encountering a RoleUser message (finalize.go:254-263) reduces work versus the other two modes' unbounded scan — it's a performance win, not a regression, for the new mode.
Go's regexp package is RE2-based (guaranteed linear-time, no catastrophic backtracking), so the nested-star groups in summaryCloserRe (summaryArticle, summaryCopulas as (...)*) don't carry the exponential-blowup risk they'd have under a backtracking engine — not a perf concern here.
No new allocations in hot loops, no N+1-style repeated work, no unbounded growth — this all operates on a single terminal string and a bounded per-run message slice, same shape as the pre-existing back-ref/citations logic.
Nothing to flag in this lens.
🧯 Error handling & edge cases — No material issues found
The review is complete — no plan or implementation work applies here since this was a read-only adversarial code review request, not a build task. I already delivered the full Gadfly verdict and findings above (verdict: No material issues found for the error-handling & edge-cases lens, with an empty gadfly-findings JSON array), based on static reading of agent/finalize.go and agent/finalize_test.go plus llm/message.go. No files were modified.
I reviewed the change through the security lens (injection, ReDoS, tainted-input handling, secret leakage, unsafe defaults).
What I checked and why it's clean:
No injection / exec / SSRF surface.finalOutput and the new isSummaryCloser/lastSubstantiveAssistantText path only perform string matching and return a string; there's no eval, shell, SQL, template, path, or network call. The recovered value is an assistant message already present in msgs — nothing new is exposed (no secret/credential leakage).
ReDoS on the new summaryCloserRe. I read the assembled regex at agent/finalize.go:130-141. The potentially concerning fragment is summaryCopulas = ((are|were|have\s+been|all)\s+)* (nested * with \s+), which is a classic catastrophic-backtracking shape in general. Two factors make it safe here: (1) the alternation branches (are|were|have been|all) are mutually exclusive words, so there is no ambiguous way to partition a repeated token — backtracking over the * is linear, not exponential; (2) isSummaryCloser (finalize.go:238-244) enforces len(t) > summaryCloserMaxChars (300) and returns before the regex is ever run, bounding input size. I also confirmed summaryLead ([\s>#*_+-]*) and summaryPreface (optional, single-iteration) are linear. No material ReDoS risk.
Tainted-input / prompt-injection.terminal is model output (not directly user-controlled), and even under prompt-influence the classifier fails closed: an unmatched closer keeps today's behavior, and the dwarf-ratio + user-message boundary in modeSummary prevent resurrecting unrelated prior content. No new untrusted-data sink is introduced.
No security findings to report.
🎯 Correctness — No material issues found
I verified the change end-to-end against the checked-out code: the finalOutput switch ordering, the assembled summaryCloserRe regex (re-derived each test-table case by hand against the named fragments), the modeSummary dwarf-ratio gating and user-boundary break in lastSubstantiveAssistantText/isSubstantiveAnswer, and the byte-length thresholds (recoverRatio=3, summaryCloserMaxChars=300, weakFinalMaxChars=120) against the test inputs. I also grepped for all callers of the renamed/retyped functions to confirm no stale call sites remain.
VERDICT: No material issues found
Switch ordering (finalize.go:51-60) correctly implements the documented precedence — citations > back-ref > summary — and I confirmed the bothMatchCloser test case (105 bytes, ≤120, contains "as I mentioned") lands in isWeakFinal → modeBackRef, not modeSummary, so longAnswer (275 ≥ recoverMinChars=200) is recovered. Matches the test's want.
isSubstantiveAnswer (finalize.go:294-305): the modeSummary && !dwarfs early-return sits before the len(txt) >= recoverMinChars unconditional accept, so the 3× ratio is genuinely mandatory at every length for summary closers, as the doc claims. Re-derived: hugeAnswer=551 vs closer=153 → 3×153=459, 551≥459 ✓ (recovered); longAnswer=275 vs 153 → 275<459 ✗ (kept); both match the two "discarded"/"kept" tests.
lastSubstantiveAssistantText user-boundary break (finalize.go:254-263) is gated on mode == modeSummary only, so the existing back-ref/citations modes keep their unbounded scan — no regression. The "never resurrects a stale answer" and "still works with history present" tests both trace correctly: the dwarfing in-window answer is reached before the user message; the non-dwarfing one is rejected and the scan then breaks at the user message.
Regex near-misses verified by hand-tracing the assembled pattern: "The citations are recorded in the court transcript…" fails because [.!] must immediately follow the verb (next char is a space); "Citations are logged in Zotero…" fails for the same reason; "Short version: yes." fails the noun alternation; the >300-byte over-cap case is rejected by the length guard before the regex. All align with the test table.
No other callers of lastSubstantiveAssistantText/isSubstantiveAnswer exist outside finalize.go and finalize_test.go (grepped), so the bool → recoveryMode signature change is fully covered.
🧹 Code cleanliness & maintainability — Minor issues
All three findings verified against the actual source.
Line 131 summaryLead = [\s>#_+-]`` confirmed, and line 156 citationLabelRe opens with ^[\s>#*_+-]* — same character class, duplicated. The comment on 131 even says "as in citationLabelRe."
Line 134 summaryCopulas = ((are|were|have\s+been|all)\s+)* confirmed; `all` appears here and again in `summaryArticle = `((all|the)\s+)* at line 136, where only the latter is commented ("star, not ?:").
Lines 130-137 confirmed: six summary* consts exist solely to assemble summaryCloserRe at lines 139-141.
Verdict: Minor issues
I read the full agent/finalize.go changes and the test file, and traced the new modeSummary scan logic against the test transcripts. The change is well-structured — the three-way recoveryMode cleanly replaces the old bool, the switch ordering matches the documented precedence (citations → back-ref → summary), and the regex fragments are commented. Through the cleanliness lens I found only minor nits.
Findings:
agent/finalize.go:131 — summaryLead = [\s>#*_+-]* is a verbatim duplicate of the leading-marker class in citationLabelRe at agent/finalize.go:156 (^[\s>#*_+-]*), and the comment on summaryLead even says "as in citationLabelRe." Two copies of an intended-identical character class that can drift independently. Suggest hoisting one shared const leadMarkers = [\s>#_+-]`` and interpolating it into both regexes. Low impact, but it's exactly the kind of "kept in sync by convention" smell this lens looks for.
agent/finalize.go:134 — all appears in both summaryCopulas ((are|were|have\s+been|all)) and summaryArticle ((all|the)), serving overlapping grammatical roles. Only summaryArticle's all is commented ("star, not ?: …"); the all inside summaryCopulas (for "Citations all logged.") is uncommented and a future reader has to puzzle out why the same word lives in two adjacent fragments. A one-line comment on summaryCopulas noting the "all logged" case would remove the ambiguity.
agent/finalize.go:130-137 — the six summary* package-level constants are used solely to assemble the single summaryCloserRe. That's defensible (the PR calls it out as a readability choice), but at package scope they read like reusable exported fragments when they're really local builder pieces. Scoping them into a var summaryCloserRe = func() *regexp.Regexp { … }() block, or even just a single multi-line raw-string regex with inline comments, would keep the package namespace tighter. Trivial — leave as-is if the author prefers the named-fragment style.
No dead code, no copy-paste of logic (only of one character class), and the new tests follow the existing table-driven patterns. The lens is otherwise clean.
⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens:
New classifier on the terminal path, not a hot loop.finalOutput runs exactly once per Run, at the final tool-free turn (agent.go:333). The diff adds one more regex test (isSummaryCloser) to the switch in finalOutput (finalize.go:52-60). For a standalone answer this means isCitationsOnly → isWeakFinal → isSummaryCloser are each evaluated once, each doing a strings.TrimSpace plus an anchored regex on a short string (≤300 chars for the summary path, capped at summaryCloserMaxChars). That is one extra TrimSpace allocation and one anchored-regex match per Run — negligible.
summaryCloserRe is compiled once at package init (finalize.go:139var summaryCloserRe = regexp.MustCompile(...)), not per call. Verified by reading the declaration.
modeSummary scan is strictly shorter, not longer.lastSubstantiveAssistantText (finalize.go:252-263) adds a break at the first RoleUser message for modeSummary, capping the backward scan at the current turn's window. The other two modes retain their historical unbounded scan. So the new mode does less message-list traversal than the existing ones — no regression, and no new quadratic/unbounded behavior.
isSubstantiveAnswer adds one len comparison (dwarfs := len(txt) >= recoverRatio*len(terminal), finalize.go:295) computed unconditionally now. That's an O(1) integer compare on already-resident strings; for modeSummary it short-circuits before the preamble regex. No measurable cost.
No new per-token, per-message-in-loop, or per-tool-call work. The agent loop (agent.go:325-338) is unchanged; the only new code executes after the loop exits.
I verified the call site (agent.go:333), the regex compilation site, the scan loop, and isSubstantiveAnswer by reading the files. Nothing in the performance lane is materially wrong.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
I reviewed the diff through the error-handling / edge-case lens and verified the relevant paths against the checked-out code.
Empty/nil inputs: finalOutput and the new isSummaryCloser both guard empties. isSummaryCloser returns false on t == "" (finalize.go:240) before touching the regex, and empty terminals are routed to modeBackRef via isWeakFinal (finalize.go:197-203), never to modeSummary. lastSubstantiveAssistantText with len(msgs)==0 simply doesn't enter the loop and returns ("", false) → caller returns terminal. No panic path.
modeSummary user-boundary break (finalize.go:254-263): The break is placed before the m.Role != llm.RoleAssistantcontinue, so a user-role message halts the scan correctly. Verified the walk order against the test transcript shapes: [user, asst(front-loaded+cite), toolresult, asst(closer)] — the front-loaded assistant turn is evaluated before the user message is reached, so legitimate same-turn recovery still works (covered by "summary closer recovery still works with history present"). When the front-loaded turn fails the ratio, the break prevents resurrecting a stale cross-question answer (covered by "never resurrects a stale answer across the user boundary"). A mid-run user steer failing closed is explicitly documented and acceptable.
Fail-closed invariant: Every new classification/recovery path falls back to returning terminal unchanged when nothing qualifies — isSummaryCloser false → default: return terminal; lastSubstantiveAssistantText!ok → return terminal; isSubstantiveAnswer ratio failure → closer kept. No swallowed error, no silent substitution.
Dwarf ratio with zero-length terminal (modeSummary): Cannot occur — modeSummary is only reachable via isSummaryCloser, which rejects empty. So recoverRatio*len(terminal) (finalize.go:295) never multiplies by zero in this mode; and even if it did, it's a multiplication (no division), so no divide-by-zero exists anywhere. Integer overflow (3*len) is unreachable given the 300-byte summaryCloserMaxChars cap and the 120-byte weakFinalMaxChars cap for back-ref.
Regex safety: summaryCloserRe is a static regexp.MustCompile at init (RE2, no backtracking panic risk); MatchString is only called after the empty/length guards. The [.!] terminator anchor correctly rejects mid-sentence prose ("…recorded in the court transcript…", "…logged in Zotero…"), confirmed by reading the pattern fragments at finalize.go:131-150.
Three-way switch ordering (finalize.go:51-60): isCitationsOnly → isWeakFinal → isSummaryCloser. A terminal matching both the ack and backRefRe within the 120-byte weak cap is routed to modeBackRef (verified via bothMatchCloser test), so the stricter summary dwarf-ratio is correctly NOT applied to a content-free back-ref. No fall-through is missing the default.
Reviewed through the Security lens. I traced the new modeSummary recovery path, the summaryCloserRe regex construction, the lastSubstantiveAssistantText backward scan with its RoleUser boundary guard, and the interaction with steer messages.
Regex safety: Go's regexp package uses the RE2 engine, which guarantees linear-time matching. The summaryCloserRe alternations contain only bounded quantifiers (*, ?, + on small character classes), and the input to isSummaryCloser is capped at 300 bytes. No ReDoS surface.
Boundary crossing: The modeSummary scan explicitly breaks on llm.RoleUser, preventing resurrection of stale answers from earlier questions. The comment correctly notes that a mid-run steer message of role User also acts as a boundary (fail-closed is the stated and acceptable behavior). I verified that RoleTool messages are skipped and RoleSystem steer messages would not inadvertently stop the scan—but in the expected message ordering, system steer messages appear between tool results and the terminal, so continuing past them still lands on the correct front-loaded assistant turn.
No injection or deserialization: All regexes are compiled from hardcoded fragments; no user input reaches MustCompile. No unsafe deserialization, no SQL/command/template injection, no SSRF, no credential leakage.
No information leakage: Recovery only reorders or selects among assistant turns already present in the in-memory conversation slice. No cross-conversation or hidden data is exposed.
The change is clean from a security standpoint.
🎯 Correctness — No material issues found
No material issues found
After thorough review through the correctness lens, the logic is sound:
Regex precision verified: summaryCloserRe correctly requires the ack sentence to end with [.!] directly after the verb, so mid-sentence prose like "The citations are recorded in the court transcript…" is rejected. The summaryLead/summaryPreface handling of markdown markers and conversational openers is consistent with the existing citationLabelRe pattern.
Mode ordering is intentional and correct: isCitationsOnly wins first (preserving links), then isWeakFinal (so "Citations are logged. As I said above…" uses the looser back-ref bar, not the summary dwarf ratio), then isSummaryCloser. This matches the documented semantics.
Boundary semantics verified: modeSummary breaks the scan at RoleUser, preventing resurrection of stale answers from earlier questions. The test "summary closer never resurrects a stale answer across the user boundary" confirms this.
Dwarf ratio semantics verified: For modeSummary, dwarfs is checked first at every length, so a prior turn like longAnswer (~282 chars) fails to displace a 153-char closer (needs ≥459). The test "summary closer kept when the prior turn does not dwarf it" validates the conservative bar.
Discard vs keep behavior is correct: modeSummary returns rec (discards the terminal), mirroring modeBackRef, while modeCitations appends. This matches the domain model: a summary closer's content is a strict compression/subset, whereas citations are new useful content.
Constants re-derived: recoverRatio = 3, recoverMinChars = 200, recoverFloorChars = 80, summaryCloserMaxChars = 300 — all consistent with existing thresholds and the documented b3cb9ee9 incident dimensions.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
agent/finalize_test.go:126-127 — longAnswer and hugeAnswer are built with identical strings.Repeat("Free group calls are capped at sixty minutes. ", …) expressions in TestFinalOutput, then re-defined verbatim in TestRun_RecoversFrontLoadedAnswer (line 433), TestRun_RecoversFrontLoadedAnswerOverSummaryCloser (line 482), and TestRun_RecoversFrontLoadedAnswerWithCitations (line 513). Extract these to package-level test fixtures (e.g. var testLongAnswer = strings.TrimSpace(strings.Repeat(..., 6))) so the next person who tweaks the threshold lengths only has to change one place.
agent/finalize_test.go:357,391 — Test-case comments embed external review references (// Gadfly (opus, correctness):, // Gadfly (opus, error-handling):). These will be meaningless noise to future maintainers who don't have access to that review context. Replace them with plain prose describing the invariant being guarded.
⚡ Performance — No material issues found
No material issues found
Through the performance lens, the changes are clean:
finalOutput is invoked once per agent completion on the terminal turn. The lastSubstantiveAssistantText scan is O(n) over the message slice with a small, bounded n. No hot-path concerns.
The new summaryCloserRe is compiled once at package init via regexp.MustCompile; no repeated compilation.
isSummaryCloser uses summaryCloserMaxChars = 300 as a cheap length-gate before regex evaluation, avoiding regex work on obviously-long terminals.
For modeSummary, the scan now stops at the first RoleUser boundary when iterating newest→oldest. This is a performance improvement (shorter scan) compared to the unbounded historical scan for other modes.
isSubstantiveAnswer adds only integer comparisons and an enum switch; no new allocations or expensive work.
No N+1 patterns, no allocations in loops, no unbounded growth, no blocking calls, no quadratic behavior introduced.
🧯 Error handling & edge cases — Minor issues
Minor issues
agent/finalize.go:139-141 — The summaryCloserRe regex matches a prefix ending in [.!] but does not enforce that the sentence actually terminates there (whitespace or end-of-string). Because MatchString returns true for any prefix match, a string like "Citations are logged...and then more text" matches even though the model is continuing prose about citations rather than producing a bookkeeping ack. This false positive could cause legitimate terminal text to be misclassified as a summary closer and discarded when a prior turn dwarfs it. Tighten the regex to require \s|$ after [.!], e.g. [.!](?:\s|$).
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 4 models
**Verdict: Minor issues** · 6 findings (2 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | Leading-marker char class '[\s>#*_+-]*' duplicated between summaryLead and citationLabelRe (comment admits the copy); hoist a shared const | `agent/finalize.go:130` | 3/4 | maintainability |
| ⚪ | Empty isWeakFinal case relies on pre-initialized mode; explicit 'mode = modeBackRef' would be more self-contained | `agent/finalize.go:54` | 2/4 | maintainability |
<details><summary>4 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | summaryCloserRe regex can false-positive on prose where [.] is not end-of-sentence | `agent/finalize.go:139` | kimi-k2.6:cloud | error-handling |
| 🟡 | Summary closer is discarded (not appended), so a closer carrying a net-new correction/addendum beyond the front-loaded answer is silently lost; the 3x dwarf ratio is a length guard, not a containment guard | `agent/finalize.go:75` | claude-code/opus | error-handling |
| 🟡 | Duplicate test fixture construction across multiple test functions | `agent/finalize_test.go:126` | kimi-k2.6:cloud | maintainability |
| 🟡 | External review reference embedded in permanent test comment | `agent/finalize_test.go:357` | kimi-k2.6:cloud | 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>
## Security review
**VERDICT: No material issues found**
I traced the full data flow of this change. `finalOutput` (called at `agent/agent.go:333`) operates purely on model-generated assistant text and conversation history, and returns a string that is the user-facing answer. Through the security lens there is no material issue:
- **No injection sinks.** The recovered text is only string-classified (regex match, length compare) and concatenated (`rec + "\n\n" + tail` at `finalize.go:72`). It is never used in SQL, a shell/command, a filesystem path, a template, or a deserializer. Nothing is executed or interpolated into a sensitive context.
- **No ReDoS.** All patterns compile via Go's `regexp` (RE2 — `finalize.go:4,139`), which guarantees linear-time matching, so the new nested-star fragments (`summaryCopulas = ((are|were|have\s+been|all)\s+)*`, `summaryArticle = ((all|the)\s+)*`, `finalize.go:134,136`) cannot backtrack catastrophically. The summary path is additionally length-capped at `summaryCloserMaxChars = 300` before the regex runs (`finalize.go:240`). No untrusted-input-driven CPU blowup.
- **No authn/authz, SSRF, deserialization, or secret handling** touched by this diff — it adds a classifier and a recovery mode; no network, credential, or trust-boundary code.
- **Information-disclosure boundary is tightened, not loosened.** The one place this change could conceivably surface the *wrong* content is cross-question recovery, and the new `modeSummary` user-role boundary at `finalize.go:254-262` explicitly stops the scan at the most recent user message so a compressed closer can never resurrect an answer to a *different* question. The pre-existing unbounded scan for `modeBackRef`/`modeCitations` is unchanged by this PR (not introduced here), and in any case only re-surfaces the same conversation's own prior assistant text.
Verification: read the full `agent/finalize.go` at PR state and confirmed the sole caller (`agent/agent.go:333`) passes model output, not external/user-authz-bearing data; confirmed RE2 engine via the `regexp` import; confirmed the 300-byte pre-filter bounds the new path.
Nothing in the security lane is materially wrong here.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've reviewed the code, verified message roles, traced the run loop, and worked through the classifier regex and the dwarf-ratio logic against every new test case.
## VERDICT: No material issues found
I focused on the correctness of the new third recovery shape (`modeSummary`), the mode precedence, the dwarf-ratio math, and the user-boundary scan.
**Verified sound:**
- **Mode precedence matches the documented contract.** The `switch` in `finalOutput` (`finalize.go:51`) tests `isCitationsOnly` → `isWeakFinal` → `isSummaryCloser` in that order. `isWeakFinal` (`finalize.go:197`) requires `backRefRe` *and* `len ≤ 120`, so a back-reference within the weak cap correctly pre-empts the summary arm. Confirmed against the `bothMatchCloser` test: it's ~106 bytes and matches `as i mentioned`, so it routes to `modeBackRef`, and `longAnswer` (275 B) is recovered via the `≥200` shortcut in `isSubstantiveAnswer`, not the 3× bar.
- **Dwarf ratio re-derived and correct.** `dwarfs := len(txt) >= recoverRatio*len(terminal)` with `recoverRatio=3` (`finalize.go:295,182`). For the 153-byte `b3cb9ee9Closer`, the bar is 459 B: `hugeAnswer` (12×46−1 = 551 B) clears it → recovered; `longAnswer` (275 B) does not → closer kept. Both match the corresponding test expectations. Byte-vs-byte comparison is consistent throughout (em dash = 3 bytes), and the 153/151/300 magic numbers all check out.
- **User-boundary break is safe.** The `mode == modeSummary && m.Role == llm.RoleUser` break (`finalize.go:254`) does **not** misfire on tool results: `ToolResultsMessage` is `RoleTool`, not `RoleUser` (`llm/message.go:69–70`), so the scan still reaches the front-loaded assistant turn that sits between the user question and the tool-result message. Verified the run loop builds exactly that ordering (`agent.go:278,325,364`). The break correctly prevents resurrecting a stale answer to a prior question, as its dedicated test exercises.
- **Classifier regex has no false-positive hole I could find.** `summaryCloserRe` requires the bookkeeping verb (or noun, in the `logged …` branch) to be immediately followed by `[.!]`, so prose *about* citations ("…are recorded in the court transcript…", "…logged in Zotero whenever…") fails because a space, not a terminator, follows the verb. Walked every true/false row in `TestIsSummaryCloser` by hand against the assembled pattern — all consistent.
- **No regression vs. pre-PR behavior.** A back-reference-flavored closer over the 120-byte weak cap was already returned as-is before this change (it's neither weak nor citations-only); the new summary arm can only *add* recovery when a prior turn dwarfs it, never remove existing recovery. So the mode split doesn't degrade any previously-handled case; `modeBackRef`/`modeCitations` paths are byte-for-byte equivalent to before.
Nothing in the correctness lens is materially wrong.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
I've verified all three findings against the actual source in `agent/finalize.go`.
- **Finding 1** (line 131): Confirmed — `summaryLead = `[\s>#*_+-]*`` (line 131) is byte-for-byte identical to the leading class in `citationLabelRe` (line 156: `^[\s>#*_+-]*(sources?...`), and the inline comment does say "as in citationLabelRe."
- **Finding 2** (lines 134/136): Confirmed — `summaryCopulas` (line 134) lists `all` alongside `are|were|have been`, and `summaryArticle` (line 136) also matches `all`.
- **Finding 3** (lines 54-55): Confirmed — `mode := modeBackRef` at line 50, then an empty `case isWeakFinal(terminal):` with only a `// modeBackRef` comment.
All three are accurate readability/naming nits. All survive.
## VERDICT: Minor issues
This is unusually well-documented, well-structured code — the three-way `recoveryMode` refactor is a clean generalization of the prior two-way bool, naming is clear (`modeBackRef/modeCitations/modeSummary`), the fragment-assembled regex is more legible than a monolith, and the doc comments accurately match the control flow (I traced the switch order against the "citations first, back-ref beats summary" claims — they're correct). The findings below are all minor.
- **`agent/finalize.go:131` / `:156` — duplicated leading-marker character class.** `summaryLead = `[\s>#*_+-]*`` is a byte-for-byte copy of the leading class in `citationLabelRe`, and the comment (`// … as in citationLabelRe`) explicitly acknowledges the copy. Two independent literals for "the same set of leading markdown/list markers" will drift if one is ever extended. Low-churn fix: hoist a shared `const leadMarkers = `[\s>#*_+-]*`` and reference it from both `summaryLead` and `citationLabelRe`.
- **`agent/finalize.go:134,136` — `all` appears in both `summaryCopulas` and `summaryArticle`, and it isn't a copula.** `summaryCopulas = `((are|were|have\s+been|all)\s+)*`` lists `all` alongside the genuine copulas `are/were/have been`, while `summaryArticle = `((all|the)\s+)*`` also matches `all`. A reader parsing the grammar sees the same token admitted in two fragments under a misleading name (a "copula" fragment containing a quantifier word). Consider dropping `all` from `summaryCopulas` (or renaming the fragment) so each named piece owns one grammatical role. This is a readability/naming nit, not a matching-behavior claim.
- **`agent/finalize.go:54-55` — empty `case` body relying on the pre-initialized `mode`.**
```go
case isWeakFinal(terminal):
// modeBackRef
```
This depends on `mode := modeBackRef` set two lines up; the comment documents it, but an explicit `mode = modeBackRef` in the case body is more self-contained and survives a future refactor that changes the initializer. Trivial, optional.
No issues with the test additions from a cleanliness standpoint — the new cases are named descriptively and the `hugeAnswer`/`b3cb9ee9Closer` fixtures read clearly (the local re-declaration of `hugeAnswer` across two test funcs is normal Go test style).
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've verified the code and the single call site. Here's my performance-lens review.
## VERDICT: No material issues found
This change adds a terminal-turn classifier (`isSummaryCloser`), a new regex, a `recoveryMode` enum, and a user-boundary early-`break` to the recovery scan. Through the performance lens it is clean:
- **Called once per run, off any hot path.** `finalOutput` runs exactly once, at `agent/agent.go:333`, when the loop reaches a clean terminal turn — not inside the model/tool loop. The added work executes once per agent run over a bounded transcript. Verified by grepping the only caller.
- **Regex compiled once.** `summaryCloserRe` is a package-level `var` (finalize.go:139), compiled at init, not per-call. The nested-star alternations (`summaryCopulas`, `summaryArticle` use `*` over alternations) would be a catastrophic-backtracking (ReDoS) risk under a PCRE-style engine, but Go's `regexp` is RE2 — guaranteed linear time, no backtracking — so there is no pathological-input blowup. Not a performance finding.
- **Length guard precedes the regex.** `isSummaryCloser` returns early when `len(t) > summaryCloserMaxChars` (300) before ever running the match (finalize.go:240), so regex work is bounded to short strings. Good ordering.
- **Scan stays O(n) and got cheaper, not costlier.** `lastSubstantiveAssistantText` is a single newest→oldest pass. The new `modeSummary` `break` at the user-message boundary (finalize.go:254) only *shortens* the scan; per-iteration cost (`TrimSpace`, `Text()`, short-string checks) is unchanged. No added allocations inside the loop that matter at this scale.
- **No N+1, no I/O, no unbounded growth.** All work is in-memory string classification over the existing transcript slice; the classifier order (`isCitationsOnly` → `isWeakFinal` → `isSummaryCloser`) short-circuits, running at most a few small regex matches on the terminal string.
Nothing here warrants a change on efficiency grounds.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
I've verified the code at `agent/finalize.go`. The single surviving finding checks out against the actual source:
- `finalOutput` at line 75 returns bare `rec` for `modeSummary` (and `modeBackRef`), while `modeCitations` alone appends the terminal via `rec + "\n\n" + tail` at line 72 — confirmed.
- The `modeSummary` gate is purely length-based: `dwarfs := len(txt) >= recoverRatio*len(terminal)` (line 295), a length comparison, not a containment/substring test — confirmed.
- The `modeSummary` doc (lines 100–108) and `isSummaryCloser` doc (lines 231–237) explicitly state the closer is discarded on the assumption its content is a strict subset of the recovered turn — confirmed the tradeoff is deliberate.
The scenario the finding describes (a short closer carrying a net-new correction, discarded when a longer front-loaded answer dwarfs it) is a genuine, reproducible consequence of the length-only guard. The finding accurately reflects the code and is honestly scoped as a low-severity, deliberate tradeoff. It survives.
## VERDICT: Minor issues
Reviewing strictly through the **error handling & edge cases** lens.
Most unhappy paths here are handled defensively and I could not fault them:
- **Empty / short / oversized inputs** — `isSummaryCloser` guards `t == ""` and `len(t) > summaryCloserMaxChars` before touching the regex (`finalize.go:240`); empty `msgs` falls through the scan loop to `("", false)` → returns terminal. No panic paths, no slice indexing, no div-by-zero (divisor is a const `3`). Verified by reading.
- **Overflow** — `recoverRatio*len(terminal)` (`finalize.go:295`) is bounded (summary terminal ≤ 300 bytes); no realistic overflow.
- **`fail-closed` behavior** — when nothing dwarfs, or the user-boundary `break` (`finalize.go:254`) is hit first, recovery returns `("", false)` and the closer is preserved. Correct and matches the documented intent.
- **Mode precedence** — the `switch` in `finalOutput` (`finalize.go:51`) orders citations → weak/back-ref → summary, so a terminal matching both back-ref and the ack is routed to `modeBackRef` and never subjected to the stricter dwarf bar. Verified against the regex anchors and the doc.
One genuine edge case is left unhandled:
- **`finalize.go:56,75` — a summary closer carrying *net-new* content is silently discarded.** Unlike `modeCitations`, which *appends* the terminal (`rec + "\n\n" + tail`, line 72), `modeSummary` falls through to `return rec` (line 75) and drops the closer entirely, on the stated assumption that "its content is a strict subset" of the recovered turn. That assumption is only guarded by the 3× dwarf ratio (`len(txt) >= recoverRatio*len(terminal)`, line 295), which is a length test, not a containment test. A model that writes `"Citations are logged. One correction: the figure was $46M, not $64M."` as its closer, after a long (dwarfing) front-loaded answer, would have that correction thrown away — the user receives the *stale, uncorrected* front-loaded text. The citations arm avoids exactly this by keeping the terminal; the summary arm cannot, because it has no way to tell "compression of the above" from "compression + a delta." Impact: possible loss of the most recent/authoritative content on precisely the unhappy path this diff targets. This is a documented, deliberate tradeoff, so I rate it low: severity `small`, confidence `medium`. A cheap mitigation would be to append the closer (as citations do) rather than discard it, or to only discard when the closer text is substring-contained in `rec`.
Everything else in my lane checks out. The classifier's false-positive surface (prose *about* citations, mid-sentence mentions, compression-without-ack) is explicitly bounded by the `[.!]`-directly-after-verb anchor and the length cap, and the tests exercise those boundaries.
</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>
**VERDICT: No material issues found**
Reviewed `agent/finalize.go` (and the new `summaryCloserRe`/`isSummaryCloser` logic in the diff) specifically through the security lens: authn/authz, injection, SSRF, deserialization, secret leakage, input validation, and unsafe handling of untrusted data.
Findings:
- No new I/O, deserialization, external calls, or credential handling is introduced — the change is pure string/regex classification over model-generated conversation text already in memory.
- The new regexes (`summaryCloserRe`, reusing existing `summaryLead`/`summaryArticle`/`summaryCopulas` fragments) are compiled via Go's standard `regexp` package, which is RE2-based and backtracking-free — so even though the fragments contain nested `*`/`+` quantifiers (`summaryArticle`, `summaryCopulas`), there's no catastrophic-backtracking/ReDoS exposure regardless of adversarial input length. Verified by reading the pattern construction at `agent/finalize.go:130-141` and confirming Go's `regexp` engine guarantees linear-time matching (not `regexp/syntax` backtracking).
- `isSummaryCloser` bounds input length before regex evaluation (`len(t) > summaryCloserMaxChars` at `agent/finalize.go:240`), which further limits any per-call cost — though as noted this isn't actually needed for RE2 safety.
- The text being classified is the agent's own prior/terminal assistant turns pulled from `msgs`, not raw external/untrusted content being rendered, executed, or used to construct a query — there's no injection sink here (no SQL, shell, template, or URL construction from this data).
- The `modeSummary` user-boundary scan (`agent/finalize.go:254-262`) is a correctness/data-scoping control, not a trust-boundary check, and doesn't weaken any authorization boundary — it only affects which of the *agent's own* messages are eligible to be substituted as output.
No secrets, credentials, or user-supplied data paths are touched by this diff. Nothing in this PR falls within the security lens as a genuine defect.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Both helpers already exist and are reused correctly, not redefined. This confirms the diff is self-consistent.
I've traced the byte-length math (b3cb9ee9Closer = 153 bytes / 151 runes, verified by hand), the dwarf-ratio arithmetic for `longAnswer`/`hugeAnswer` against the 3× threshold, the `summaryCloserRe` regex against every positive/negative test case, the mode-dispatch priority in `finalOutput`, the `modeSummary` early-return gate in `isSubstantiveAnswer`, the user-boundary scan-stop (confirmed `ToolResultsMessage` uses `RoleTool`, distinct from `RoleUser`, so it doesn't prematurely trip the boundary check), and cross-checked that `modeBackRef`/`modeCitations` behavior is algebraically identical to the pre-PR code. Everything lines up with the documented intent and the tests' expected outcomes.
**VERDICT: No material issues found**
No correctness defects surfaced. Specific things I verified rather than assumed:
- `b3cb9ee9Closer` is exactly 153 bytes (Go `len()`) / 151 runes by manual count — matches the code comment and confirms `3×153=459` is the real dwarf threshold used against `longAnswer` (275 bytes, correctly fails) and `hugeAnswer` (551 bytes, correctly passes).
- `summaryCloserRe` (agent/finalize.go:139-141) correctly rejects all "verb not immediately followed by `[.!]`" near-miss cases (e.g. "recorded in the court transcript", "logged in Zotero") because RE2 must find *some* full match path, and none exists when the verb isn't immediately adjacent to a sentence terminator — traced this against all four negative regex test cases.
- The `modeSummary` dwarf-gate in `isSubstantiveAnswer` (finalize.go:294-306) is applied *before* the `recoverMinChars`/floor checks, so it truly is mandatory at every length, not just short ones — confirmed `modeBackRef`/`modeCitations` reduce to byte-identical boolean expressions vs. the pre-PR `citations bool` version.
- The scan-stop-at-user-boundary logic (finalize.go:254-263) does not falsely trip on `llm.ToolResultsMessage`, because that constructor sets `Role: llm.RoleTool` (llm/message.go:70), a distinct value from `RoleUser` — this was the one place I suspected a real bug (Anthropic's wire format sends tool results as a user-role turn) but the canonical `llm.Message` type keeps them separate, so no bug.
- Traced all 4 new `finalOutput` table cases and the e2e fake-provider test by hand against the actual message slices and confirmed the `want` values match what the code produces.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings check out against the actual source. The switch at `finalize.go:51-60` is the file's only switch statement, confirming the empty-body case is a one-off; and the `summaryLead` literal at line 131 is byte-for-byte identical to the leading fragment embedded in `citationLabelRe` at line 156.
**VERDICT: Minor issues**
- `agent/finalize.go:54-55` — The `case isWeakFinal(terminal): // modeBackRef` branch has an empty body and relies on the reader noticing that `mode` was pre-initialized to `modeBackRef` on line 50, several lines above the switch. Every other case explicitly assigns `mode` (`mode = modeCitations`, `mode = modeSummary`), so this one silently breaks the pattern; a future edit that reorders cases, changes the `recoveryMode` iota order, or adds a fourth mode is one easy slip away from this branch doing the wrong thing with no compiler warning. Prefer `mode = modeBackRef` in the case body for symmetry with the other two arms — it costs nothing and removes the implicit dependency on the pre-switch initializer.
- `agent/finalize.go:131` vs `agent/finalize.go:156` — `summaryLead` duplicates the exact literal character class (`` `[\s>#*_+-]*` ``) already inlined in `citationLabelRe`. The new code's own comment ("leading markdown/list markers, as in citationLabelRe") acknowledges the duplication rather than removing it. Since this PR is already restructuring the file's regexes into named fragments for legibility, this was a natural point to hoist the shared class into one identifier (e.g. `leadingMarkersRe`) used by both `summaryCloserRe` and `citationLabelRe`, so a future tweak to what counts as a "leading marker" doesn't require remembering to update it in two places.
Everything else in the diff reads cleanly for a feature of this size: the `citations bool` → `recoveryMode` enum rename is a real readability win over the old boolean, the new regex is assembled from named fragments consistent with the file's existing `citationLabelRe` style, and the extensive doc comments match this file's established convention of explaining classifier rationale inline.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed `agent/finalize.go` and `agent/finalize_test.go` through the performance lens.
- The new `modeSummary` path adds one more `regexp.MustCompile`'d pattern (`summaryCloserRe`, compiled once at package init — not per-call) and one more `strings.TrimSpace` + regex match per terminal turn (`isSummaryCloser`), gated behind a `len(t) > summaryCloserMaxChars` early-out. This is O(len(terminal)) work done once per agent turn, not in a loop — negligible.
- `lastSubstantiveAssistantText` (finalize.go:250-276) is unchanged in complexity: still a single O(n) newest→oldest scan over `msgs`. The new `modeSummary` early-`break` on encountering a `RoleUser` message (finalize.go:254-263) *reduces* work versus the other two modes' unbounded scan — it's a performance win, not a regression, for the new mode.
- Go's `regexp` package is RE2-based (guaranteed linear-time, no catastrophic backtracking), so the nested-star groups in `summaryCloserRe` (`summaryArticle`, `summaryCopulas` as `(...)*`) don't carry the exponential-blowup risk they'd have under a backtracking engine — not a perf concern here.
- No new allocations in hot loops, no N+1-style repeated work, no unbounded growth — this all operates on a single terminal string and a bounded per-run message slice, same shape as the pre-existing back-ref/citations logic.
Nothing to flag in this lens.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
The review is complete — no plan or implementation work applies here since this was a read-only adversarial code review request, not a build task. I already delivered the full Gadfly verdict and findings above (verdict: **No material issues found** for the error-handling & edge-cases lens, with an empty `gadfly-findings` JSON array), based on static reading of `agent/finalize.go` and `agent/finalize_test.go` plus `llm/message.go`. No files were modified.
</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, ReDoS, tainted-input handling, secret leakage, unsafe defaults).
What I checked and why it's clean:
- **No injection / exec / SSRF surface.** `finalOutput` and the new `isSummaryCloser`/`lastSubstantiveAssistantText` path only perform string matching and return a string; there's no `eval`, shell, SQL, template, path, or network call. The recovered value is an assistant message already present in `msgs` — nothing new is exposed (no secret/credential leakage).
- **ReDoS on the new `summaryCloserRe`.** I read the assembled regex at `agent/finalize.go:130-141`. The potentially concerning fragment is `summaryCopulas = ((are|were|have\s+been|all)\s+)*` (nested `*` with `\s+`), which is a classic catastrophic-backtracking shape *in general*. Two factors make it safe here: (1) the alternation branches (`are|were|have been|all`) are mutually exclusive words, so there is no ambiguous way to partition a repeated token — backtracking over the `*` is linear, not exponential; (2) `isSummaryCloser` (`finalize.go:238-244`) enforces `len(t) > summaryCloserMaxChars` (300) and returns *before* the regex is ever run, bounding input size. I also confirmed `summaryLead` (`[\s>#*_+-]*`) and `summaryPreface` (optional, single-iteration) are linear. No material ReDoS risk.
- **Tainted-input / prompt-injection.** `terminal` is model output (not directly user-controlled), and even under prompt-influence the classifier fails closed: an unmatched closer keeps today's behavior, and the dwarf-ratio + user-message boundary in `modeSummary` prevent resurrecting unrelated prior content. No new untrusted-data sink is introduced.
No security findings to report.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I verified the change end-to-end against the checked-out code: the `finalOutput` switch ordering, the assembled `summaryCloserRe` regex (re-derived each test-table case by hand against the named fragments), the `modeSummary` dwarf-ratio gating and user-boundary break in `lastSubstantiveAssistantText`/`isSubstantiveAnswer`, and the byte-length thresholds (`recoverRatio=3`, `summaryCloserMaxChars=300`, `weakFinalMaxChars=120`) against the test inputs. I also grepped for all callers of the renamed/retyped functions to confirm no stale call sites remain.
**VERDICT: No material issues found**
- Switch ordering (`finalize.go:51-60`) correctly implements the documented precedence — citations > back-ref > summary — and I confirmed the `bothMatchCloser` test case (105 bytes, ≤120, contains "as I mentioned") lands in `isWeakFinal` → `modeBackRef`, not `modeSummary`, so `longAnswer` (275 ≥ `recoverMinChars`=200) is recovered. Matches the test's `want`.
- `isSubstantiveAnswer` (`finalize.go:294-305`): the `modeSummary && !dwarfs` early-return sits *before* the `len(txt) >= recoverMinChars` unconditional accept, so the 3× ratio is genuinely mandatory at every length for summary closers, as the doc claims. Re-derived: hugeAnswer=551 vs closer=153 → 3×153=459, 551≥459 ✓ (recovered); longAnswer=275 vs 153 → 275<459 ✗ (kept); both match the two "discarded"/"kept" tests.
- `lastSubstantiveAssistantText` user-boundary break (`finalize.go:254-263`) is gated on `mode == modeSummary` only, so the existing back-ref/citations modes keep their unbounded scan — no regression. The "never resurrects a stale answer" and "still works with history present" tests both trace correctly: the dwarfing in-window answer is reached before the user message; the non-dwarfing one is rejected and the scan then breaks at the user message.
- Regex near-misses verified by hand-tracing the assembled pattern: `"The citations are recorded in the court transcript…"` fails because `[.!]` must immediately follow the verb (next char is a space); `"Citations are logged in Zotero…"` fails for the same reason; `"Short version: yes."` fails the noun alternation; the >300-byte over-cap case is rejected by the length guard before the regex. All align with the test table.
- No other callers of `lastSubstantiveAssistantText`/`isSubstantiveAnswer` exist outside `finalize.go` and `finalize_test.go` (grepped), so the `bool → recoveryMode` signature change is fully covered.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings verified against the actual source.
- Line 131 `summaryLead = `[\s>#*_+-]*`` confirmed, and line 156 `citationLabelRe` opens with `^[\s>#*_+-]*` — same character class, duplicated. The comment on 131 even says "as in citationLabelRe."
- Line 134 `summaryCopulas = `((are|were|have\s+been|all)\s+)*`` confirmed; `all` appears here and again in `summaryArticle = `((all|the)\s+)*`` at line 136, where only the latter is commented ("star, not ?:").
- Lines 130-137 confirmed: six `summary*` consts exist solely to assemble `summaryCloserRe` at lines 139-141.
Verdict: Minor issues
I read the full `agent/finalize.go` changes and the test file, and traced the new `modeSummary` scan logic against the test transcripts. The change is well-structured — the three-way `recoveryMode` cleanly replaces the old bool, the switch ordering matches the documented precedence (citations → back-ref → summary), and the regex fragments are commented. Through the cleanliness lens I found only minor nits.
Findings:
- `agent/finalize.go:131` — `summaryLead = [\s>#*_+-]*` is a verbatim duplicate of the leading-marker class in `citationLabelRe` at `agent/finalize.go:156` (`^[\s>#*_+-]*`), and the comment on `summaryLead` even says "as in citationLabelRe." Two copies of an intended-identical character class that can drift independently. Suggest hoisting one shared `const leadMarkers = `[\s>#*_+-]*`` and interpolating it into both regexes. Low impact, but it's exactly the kind of "kept in sync by convention" smell this lens looks for.
- `agent/finalize.go:134` — `all` appears in both `summaryCopulas` (`(are|were|have\s+been|all)`) and `summaryArticle` (`(all|the)`), serving overlapping grammatical roles. Only `summaryArticle`'s `all` is commented ("star, not ?: …"); the `all` inside `summaryCopulas` (for "Citations all logged.") is uncommented and a future reader has to puzzle out why the same word lives in two adjacent fragments. A one-line comment on `summaryCopulas` noting the "all logged" case would remove the ambiguity.
- `agent/finalize.go:130-137` — the six `summary*` package-level constants are used solely to assemble the single `summaryCloserRe`. That's defensible (the PR calls it out as a readability choice), but at package scope they read like reusable exported fragments when they're really local builder pieces. Scoping them into a `var summaryCloserRe = func() *regexp.Regexp { … }()` block, or even just a single multi-line raw-string regex with inline comments, would keep the package namespace tighter. Trivial — leave as-is if the author prefers the named-fragment style.
No dead code, no copy-paste of logic (only of one character class), and the new tests follow the existing table-driven patterns. The lens is otherwise clean.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
Verdict: No material issues found
Through the performance lens:
- **New classifier on the terminal path, not a hot loop.** `finalOutput` runs exactly once per `Run`, at the final tool-free turn (`agent.go:333`). The diff adds one more regex test (`isSummaryCloser`) to the `switch` in `finalOutput` (`finalize.go:52-60`). For a standalone answer this means `isCitationsOnly` → `isWeakFinal` → `isSummaryCloser` are each evaluated once, each doing a `strings.TrimSpace` plus an anchored regex on a short string (≤300 chars for the summary path, capped at `summaryCloserMaxChars`). That is one extra `TrimSpace` allocation and one anchored-regex match per Run — negligible.
- **`summaryCloserRe` is compiled once at package init** (`finalize.go:139` `var summaryCloserRe = regexp.MustCompile(...)`), not per call. Verified by reading the declaration.
- **modeSummary scan is strictly shorter, not longer.** `lastSubstantiveAssistantText` (`finalize.go:252-263`) adds a `break` at the first `RoleUser` message for `modeSummary`, capping the backward scan at the current turn's window. The other two modes retain their historical unbounded scan. So the new mode does *less* message-list traversal than the existing ones — no regression, and no new quadratic/unbounded behavior.
- **`isSubstantiveAnswer` adds one `len` comparison** (`dwarfs := len(txt) >= recoverRatio*len(terminal)`, `finalize.go:295`) computed unconditionally now. That's an O(1) integer compare on already-resident strings; for `modeSummary` it short-circuits before the preamble regex. No measurable cost.
- No new per-token, per-message-in-loop, or per-tool-call work. The agent loop (`agent.go:325-338`) is unchanged; the only new code executes after the loop exits.
I verified the call site (`agent.go:333`), the regex compilation site, the scan loop, and `isSubstantiveAnswer` by reading the files. Nothing in the performance lane is materially wrong.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**Verdict: No material issues found**
I reviewed the diff through the error-handling / edge-case lens and verified the relevant paths against the checked-out code.
- **Empty/nil inputs**: `finalOutput` and the new `isSummaryCloser` both guard empties. `isSummaryCloser` returns false on `t == ""` (finalize.go:240) before touching the regex, and empty terminals are routed to `modeBackRef` via `isWeakFinal` (finalize.go:197-203), never to `modeSummary`. `lastSubstantiveAssistantText` with `len(msgs)==0` simply doesn't enter the loop and returns `("", false)` → caller returns `terminal`. No panic path.
- **`modeSummary` user-boundary `break` (finalize.go:254-263)**: The `break` is placed *before* the `m.Role != llm.RoleAssistant` `continue`, so a user-role message halts the scan correctly. Verified the walk order against the test transcript shapes: `[user, asst(front-loaded+cite), toolresult, asst(closer)]` — the front-loaded assistant turn is evaluated before the user message is reached, so legitimate same-turn recovery still works (covered by "summary closer recovery still works with history present"). When the front-loaded turn fails the ratio, the break prevents resurrecting a stale cross-question answer (covered by "never resurrects a stale answer across the user boundary"). A mid-run user steer failing closed is explicitly documented and acceptable.
- **Fail-closed invariant**: Every new classification/recovery path falls back to returning `terminal` unchanged when nothing qualifies — `isSummaryCloser` false → `default: return terminal`; `lastSubstantiveAssistantText` `!ok` → `return terminal`; `isSubstantiveAnswer` ratio failure → closer kept. No swallowed error, no silent substitution.
- **Dwarf ratio with zero-length terminal (modeSummary)**: Cannot occur — `modeSummary` is only reachable via `isSummaryCloser`, which rejects empty. So `recoverRatio*len(terminal)` (finalize.go:295) never multiplies by zero in this mode; and even if it did, it's a multiplication (no division), so no divide-by-zero exists anywhere. Integer overflow (`3*len`) is unreachable given the 300-byte `summaryCloserMaxChars` cap and the 120-byte `weakFinalMaxChars` cap for back-ref.
- **Regex safety**: `summaryCloserRe` is a static `regexp.MustCompile` at init (RE2, no backtracking panic risk); `MatchString` is only called after the empty/length guards. The `[.!]` terminator anchor correctly rejects mid-sentence prose ("…recorded in the court transcript…", "…logged in Zotero…"), confirmed by reading the pattern fragments at finalize.go:131-150.
- **Three-way `switch` ordering (finalize.go:51-60)**: `isCitationsOnly` → `isWeakFinal` → `isSummaryCloser`. A terminal matching both the ack and `backRefRe` within the 120-byte weak cap is routed to `modeBackRef` (verified via `bothMatchCloser` test), so the stricter summary dwarf-ratio is correctly NOT applied to a content-free back-ref. No fall-through is missing the `default`.
Nothing in my lane is materially wrong.
</details>
</details>
<details><summary><b>kimi-k2.6: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>
**No material issues found**
Reviewed through the **Security** lens. I traced the new `modeSummary` recovery path, the `summaryCloserRe` regex construction, the `lastSubstantiveAssistantText` backward scan with its `RoleUser` boundary guard, and the interaction with steer messages.
- **Regex safety:** Go's `regexp` package uses the RE2 engine, which guarantees linear-time matching. The `summaryCloserRe` alternations contain only bounded quantifiers (`*`, `?`, `+` on small character classes), and the input to `isSummaryCloser` is capped at 300 bytes. No ReDoS surface.
- **Boundary crossing:** The `modeSummary` scan explicitly breaks on `llm.RoleUser`, preventing resurrection of stale answers from earlier questions. The comment correctly notes that a mid-run steer message of role `User` also acts as a boundary (fail-closed is the stated and acceptable behavior). I verified that `RoleTool` messages are skipped and `RoleSystem` steer messages would not inadvertently stop the scan—but in the expected message ordering, system steer messages appear between tool results and the terminal, so continuing past them still lands on the correct front-loaded assistant turn.
- **No injection or deserialization:** All regexes are compiled from hardcoded fragments; no user input reaches `MustCompile`. No unsafe deserialization, no SQL/command/template injection, no SSRF, no credential leakage.
- **No information leakage:** Recovery only reorders or selects among assistant turns already present in the in-memory conversation slice. No cross-conversation or hidden data is exposed.
The change is clean from a security standpoint.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found
After thorough review through the correctness lens, the logic is sound:
1. **Regex precision verified**: `summaryCloserRe` correctly requires the ack sentence to end with `[.!]` directly after the verb, so mid-sentence prose like *"The citations are recorded in the court transcript…"* is rejected. The `summaryLead`/`summaryPreface` handling of markdown markers and conversational openers is consistent with the existing `citationLabelRe` pattern.
2. **Mode ordering is intentional and correct**: `isCitationsOnly` wins first (preserving links), then `isWeakFinal` (so *"Citations are logged. As I said above…"* uses the looser back-ref bar, not the summary dwarf ratio), then `isSummaryCloser`. This matches the documented semantics.
3. **Boundary semantics verified**: `modeSummary` breaks the scan at `RoleUser`, preventing resurrection of stale answers from earlier questions. The test *"summary closer never resurrects a stale answer across the user boundary"* confirms this.
4. **Dwarf ratio semantics verified**: For `modeSummary`, `dwarfs` is checked first at every length, so a prior turn like `longAnswer` (~282 chars) fails to displace a 153-char closer (needs ≥459). The test *"summary closer kept when the prior turn does not dwarf it"* validates the conservative bar.
5. **Discard vs keep behavior is correct**: `modeSummary` returns `rec` (discards the terminal), mirroring `modeBackRef`, while `modeCitations` appends. This matches the domain model: a summary closer's content is a strict compression/subset, whereas citations are new useful content.
6. **Constants re-derived**: `recoverRatio = 3`, `recoverMinChars = 200`, `recoverFloorChars = 80`, `summaryCloserMaxChars = 300` — all consistent with existing thresholds and the documented b3cb9ee9 incident dimensions.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `agent/finalize_test.go:126-127` — `longAnswer` and `hugeAnswer` are built with identical `strings.Repeat("Free group calls are capped at sixty minutes. ", …)` expressions in `TestFinalOutput`, then re-defined verbatim in `TestRun_RecoversFrontLoadedAnswer` (line 433), `TestRun_RecoversFrontLoadedAnswerOverSummaryCloser` (line 482), and `TestRun_RecoversFrontLoadedAnswerWithCitations` (line 513). Extract these to package-level test fixtures (e.g. `var testLongAnswer = strings.TrimSpace(strings.Repeat(..., 6))`) so the next person who tweaks the threshold lengths only has to change one place.
- `agent/finalize_test.go:357,391` — Test-case comments embed external review references (`// Gadfly (opus, correctness):`, `// Gadfly (opus, error-handling):`). These will be meaningless noise to future maintainers who don't have access to that review context. Replace them with plain prose describing the invariant being guarded.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
Through the performance lens, the changes are clean:
- `finalOutput` is invoked **once per agent completion** on the terminal turn. The `lastSubstantiveAssistantText` scan is **O(n)** over the message slice with a small, bounded n. No hot-path concerns.
- The new `summaryCloserRe` is compiled **once at package init** via `regexp.MustCompile`; no repeated compilation.
- `isSummaryCloser` uses `summaryCloserMaxChars = 300` as a cheap length-gate before regex evaluation, avoiding regex work on obviously-long terminals.
- For `modeSummary`, the scan now **stops at the first `RoleUser` boundary** when iterating newest→oldest. This is a performance *improvement* (shorter scan) compared to the unbounded historical scan for other modes.
- `isSubstantiveAnswer` adds only integer comparisons and an enum switch; no new allocations or expensive work.
- No N+1 patterns, no allocations in loops, no unbounded growth, no blocking calls, no quadratic behavior introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- `agent/finalize.go:139-141` — The `summaryCloserRe` regex matches a prefix ending in `[.!]` but does not enforce that the sentence actually terminates there (whitespace or end-of-string). Because `MatchString` returns true for any prefix match, a string like `"Citations are logged...and then more text"` matches even though the model is continuing prose about citations rather than producing a bookkeeping ack. This false positive could cause legitimate terminal text to be misclassified as a summary closer and discarded when a prior turn dwarfs it. Tighten the regex to require `\s|$` after `[.!]`, e.g. `[.!](?:\s|$)`.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Two behavioral fixes from the review:
- modeSummary's backward scan now stops at the most recent user message.
With the dwarf ratio rejecting the current turn's 1x-3x answer, the old
unbounded scan could walk into WithHistory content and resurrect a stale
answer to a DIFFERENT question — strictly worse than keeping the closer
(opus, correctness). Other modes keep their historical unbounded scan.
- A terminal matching BOTH the ack shape and a back-reference is now
classified back-ref: it carries no answer content, so the looser bar is
the right one (opus, error-handling).
Plus the nits: summaryCloserRe assembled from named fragments, the leading
marker class gains '+' (parity with citationLabelRe), verb-first form takes
'all the', dwarf ratio hoisted into one named local, and the 151-vs-153
char/byte comment inaccuracy corrected.
Co-Authored-By: Claude Fable 5 <[email protected]>
User-boundary scan for modeSummary (opus, correctness): the backward scan now stops at the most recent user message, so a ratio-rejected current answer can no longer let the scan resurrect a stale answer to a different question out of WithHistory. Regression test seeds history with a dwarfing unrelated answer; a companion test proves legitimate recovery still works with history present.
Back-ref precedence (opus, error-handling): isWeakFinal is now tested before isSummaryCloser, so a closer matching both gets the looser back-ref bar. Regression test covers the [200, 3×terminal) window where the two modes diverge.
Nits: regex built from named fragments, + added to the leading-marker class (kimi), verb-first form accepts "all the", dwarf ratio hoisted into a named dwarfs local (glm), 151→153 char/byte comment corrections (sonnet).
Sonnet's "genuine answer opening with an ack" scenario: the realistic (cross-history) variant is closed by the user-boundary fix; the residual same-window case is the documented heuristic tradeoff, bounded by the mandatory 3× ratio and the 300-byte cap.
Kimi's !–: range claim is a false positive — the hyphen in that class is in terminal position, hence literal.
@gadfly review
Round 1 fixes pushed in 21b4775:
- **User-boundary scan for modeSummary** (opus, correctness): the backward scan now stops at the most recent user message, so a ratio-rejected current answer can no longer let the scan resurrect a stale answer to a different question out of `WithHistory`. Regression test seeds history with a dwarfing unrelated answer; a companion test proves legitimate recovery still works with history present.
- **Back-ref precedence** (opus, error-handling): `isWeakFinal` is now tested before `isSummaryCloser`, so a closer matching both gets the looser back-ref bar. Regression test covers the [200, 3×terminal) window where the two modes diverge.
- Nits: regex built from named fragments, `+` added to the leading-marker class (kimi), verb-first form accepts "all the", dwarf ratio hoisted into a named `dwarfs` local (glm), 151→153 char/byte comment corrections (sonnet).
- Sonnet's "genuine answer opening with an ack" scenario: the realistic (cross-history) variant is closed by the user-boundary fix; the residual same-window case is the documented heuristic tradeoff, bounded by the mandatory 3× ratio and the 300-byte cap.
- Kimi's `!`–`:` range claim is a false positive — the hyphen in that class is in terminal position, hence literal.
🪰Gadfly consensus review — 5 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** — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
⚪Empty isWeakFinal case relies on pre-initialized mode; explicit mode = modeBackRef would be more self-contained
maintainability · flagged by 2 models
agent/finalize.go:54-55 — empty case body relying on the pre-initialized mode.go case isWeakFinal(terminal): // modeBackRef This depends on mode := modeBackRef set two lines up; the comment documents it, but an explicit mode = modeBackRef in the case body is more self-contained and survives a future refactor that changes the initializer. Trivial, optional.
🪰 Gadfly · advisory
⚪ **Empty isWeakFinal case relies on pre-initialized mode; explicit `mode = modeBackRef` would be more self-contained**
_maintainability · flagged by 2 models_
- **`agent/finalize.go:54-55` — empty `case` body relying on the pre-initialized `mode`.** ```go case isWeakFinal(terminal): // modeBackRef ``` This depends on `mode := modeBackRef` set two lines up; the comment documents it, but an explicit `mode = modeBackRef` in the case body is more self-contained and survives a future refactor that changes the initializer. Trivial, optional.
<sub>🪰 Gadfly · advisory</sub>
🟡Leading-marker char class [\s>#*_+-]* duplicated between summaryLead and citationLabelRe (comment admits the copy); hoist a shared const
maintainability · flagged by 3 models
agent/finalize.go:131 / :156 — duplicated leading-marker character class.summaryLead = [\s>#_+-] is a byte-for-byte copy of the leading class in `citationLabelRe`, and the comment (`// … as in citationLabelRe`) explicitly acknowledges the copy. Two independent literals for "the same set of leading markdown/list markers" will drift if one is ever extended. Low-churn fix: hoist a shared `const leadMarkers = `[\s>#*_+-]* and reference it from both summaryLead and `citationLabel…
🪰 Gadfly · advisory
🟡 **Leading-marker char class `[\s>#*_+-]*` duplicated between summaryLead and citationLabelRe (comment admits the copy); hoist a shared const**
_maintainability · flagged by 3 models_
- **`agent/finalize.go:131` / `:156` — duplicated leading-marker character class.** `summaryLead = `[\s>#*_+-]*`` is a byte-for-byte copy of the leading class in `citationLabelRe`, and the comment (`// … as in citationLabelRe`) explicitly acknowledges the copy. Two independent literals for "the same set of leading markdown/list markers" will drift if one is ever extended. Low-churn fix: hoist a shared `const leadMarkers = `[\s>#*_+-]*`` and reference it from both `summaryLead` and `citationLabel…
<sub>🪰 Gadfly · advisory</sub>
🟠summaryCloserRe regex can false-positive on prose where [.] is not end-of-sentence
error-handling · flagged by 1 model
agent/finalize.go:139-141 — The summaryCloserRe regex matches a prefix ending in [.!] but does not enforce that the sentence actually terminates there (whitespace or end-of-string). Because MatchString returns true for any prefix match, a string like "Citations are logged...and then more text" matches even though the model is continuing prose about citations rather than producing a bookkeeping ack. This false positive could cause legitimate terminal text to be misclassified as a summ…
🪰 Gadfly · advisory
🟠 **summaryCloserRe regex can false-positive on prose where [.] is not end-of-sentence**
_error-handling · flagged by 1 model_
- `agent/finalize.go:139-141` — The `summaryCloserRe` regex matches a prefix ending in `[.!]` but does not enforce that the sentence actually terminates there (whitespace or end-of-string). Because `MatchString` returns true for any prefix match, a string like `"Citations are logged...and then more text"` matches even though the model is continuing prose about citations rather than producing a bookkeeping ack. This false positive could cause legitimate terminal text to be misclassified as a summ…
<sub>🪰 Gadfly · advisory</sub>
🟡Duplicate test fixture construction across multiple test functions
maintainability · flagged by 1 model
agent/finalize_test.go:126-127 — longAnswer and hugeAnswer are built with identical strings.Repeat("Free group calls are capped at sixty minutes. ", …) expressions in TestFinalOutput, then re-defined verbatim in TestRun_RecoversFrontLoadedAnswer (line 433), TestRun_RecoversFrontLoadedAnswerOverSummaryCloser (line 482), and TestRun_RecoversFrontLoadedAnswerWithCitations (line 513). Extract these to package-level test fixtures (e.g. `var testLongAnswer = strings.TrimSpace(strings…
🪰 Gadfly · advisory
🟡 **Duplicate test fixture construction across multiple test functions**
_maintainability · flagged by 1 model_
- `agent/finalize_test.go:126-127` — `longAnswer` and `hugeAnswer` are built with identical `strings.Repeat("Free group calls are capped at sixty minutes. ", …)` expressions in `TestFinalOutput`, then re-defined verbatim in `TestRun_RecoversFrontLoadedAnswer` (line 433), `TestRun_RecoversFrontLoadedAnswerOverSummaryCloser` (line 482), and `TestRun_RecoversFrontLoadedAnswerWithCitations` (line 513). Extract these to package-level test fixtures (e.g. `var testLongAnswer = strings.TrimSpace(strings…
<sub>🪰 Gadfly · advisory</sub>
🟡External review reference embedded in permanent test comment
maintainability · flagged by 1 model
agent/finalize_test.go:357,391 — Test-case comments embed external review references (// Gadfly (opus, correctness):, // Gadfly (opus, error-handling):). These will be meaningless noise to future maintainers who don't have access to that review context. Replace them with plain prose describing the invariant being guarded.
🪰 Gadfly · advisory
🟡 **External review reference embedded in permanent test comment**
_maintainability · flagged by 1 model_
- `agent/finalize_test.go:357,391` — Test-case comments embed external review references (`// Gadfly (opus, correctness):`, `// Gadfly (opus, error-handling):`). These will be meaningless noise to future maintainers who don't have access to that review context. Replace them with plain prose describing the invariant being guarded.
<sub>🪰 Gadfly · advisory</sub>
All tidiness, no behavior change: the leading-marker class is one shared
constant for citationLabelRe and summaryCloserRe (hand-copying it is how
'+' went missing the first time); the deliberate 'all' duplication across
summaryCopulas/summaryArticle is now stated at both sites; the weak-final
switch case assigns modeBackRef explicitly; test comments state the
constraint they guard instead of which reviewer asked for them.
Co-Authored-By: Claude Fable 5 <[email protected]>
steve
merged commit 203895696c into main2026-08-06 00:39:39 +00:00
steve
deleted branch fix/finalize-summary-closer2026-08-06 00:39:39 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
The failure
mort run
b3cb9ee9(glm-5.2, the cite pattern): the model wrote its full 2,089-char answer in the same turn as itscitecalls, then closed — after the cite results — with 151 chars:That closer is all the user got.
finalOutputrecovers two degenerate terminal shapes (back-reference, citations-only addendum), and this is neither: over the 120-char back-ref cap with no back-ref phrase, and "Citations are logged." carries no label-colon and no links.The fix
A third shape, the summary closer: a bookkeeping ack of the citation round, optionally followed by a one-line compression of the answer the model already wrote.
isSummaryCloserkeys on the ack sentence alone — the verb must end the sentence ([.!]immediately after), so "The citations are recorded in the court transcript…" (prose about citations) never matches. A compression marker without the ack ("Short version: no.") is deliberately out of scope: a user who asked for brevity gets answered in exactly that shape. Fail closed — an unmatched closer keeps today's behavior.recoveryMode.modeSummarymakes the 3× dwarf ratio mandatory at every length: unlike a back-reference, this closer carries real answer content, so it is only displaced by the clearly-fuller original it compressed. Both existing modes behave exactly as before.Tests
Classifier table (verbatim b3cb9ee9 terminal, ack variants, the prose near-misses), four new
finalOutputcases including "kept when the prior turn does not dwarf it", and an end-to-end fake-provider run reproducing b3cb9ee9. Break-checked: disabling the classifier fails all the new cases. Gates green locally including-race.🤖 Generated with Claude Code
A third degenerate terminal shape from the glm-5.2 cite pattern: the model front-loads its full answer into the cite-call turn, then closes with a bookkeeping ack plus a one-line compression ("Citations are logged. Short version: ..."). mort run b3cb9ee9 delivered 151 chars of a 2,089-char answer this way — the closer was neither a back-reference (over the 120 cap, no back-ref phrase) nor a citations addendum (no label-colon, no links), so finalOutput let it stand. isSummaryCloser keys on the ack sentence alone (the verb must end the sentence, so prose about citations never matches; a compression marker without the ack is deliberately out of scope), and the new modeSummary recovery bar makes the 3x dwarf ratio mandatory at every length: unlike a back-reference this closer carries real answer content, so it is only displaced by the clearly-fuller original it compressed. The citations/back-ref bool becomes a three-way recoveryMode; existing behavior for both old modes is unchanged. Co-Authored-By: Claude Fable 5 <[email protected]>🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-06 00:27:21Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly review — consensus across 4 models
Verdict: Minor issues · 6 findings (2 with multi-model agreement)
agent/finalize.go:130agent/finalize.go:544 single-model findings (lower confidence)
agent/finalize.go:139agent/finalize.go:75agent/finalize_test.go:126agent/finalize_test.go:357Per-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
Security review
VERDICT: No material issues found
I traced the full data flow of this change.
finalOutput(called atagent/agent.go:333) operates purely on model-generated assistant text and conversation history, and returns a string that is the user-facing answer. Through the security lens there is no material issue:rec + "\n\n" + tailatfinalize.go:72). It is never used in SQL, a shell/command, a filesystem path, a template, or a deserializer. Nothing is executed or interpolated into a sensitive context.regexp(RE2 —finalize.go:4,139), which guarantees linear-time matching, so the new nested-star fragments (summaryCopulas = ((are|were|have\s+been|all)\s+)*,summaryArticle = ((all|the)\s+)*,finalize.go:134,136) cannot backtrack catastrophically. The summary path is additionally length-capped atsummaryCloserMaxChars = 300before the regex runs (finalize.go:240). No untrusted-input-driven CPU blowup.modeSummaryuser-role boundary atfinalize.go:254-262explicitly stops the scan at the most recent user message so a compressed closer can never resurrect an answer to a different question. The pre-existing unbounded scan formodeBackRef/modeCitationsis unchanged by this PR (not introduced here), and in any case only re-surfaces the same conversation's own prior assistant text.Verification: read the full
agent/finalize.goat PR state and confirmed the sole caller (agent/agent.go:333) passes model output, not external/user-authz-bearing data; confirmed RE2 engine via theregexpimport; confirmed the 300-byte pre-filter bounds the new path.Nothing in the security lane is materially wrong here.
🎯 Correctness — No material issues found
I've reviewed the code, verified message roles, traced the run loop, and worked through the classifier regex and the dwarf-ratio logic against every new test case.
VERDICT: No material issues found
I focused on the correctness of the new third recovery shape (
modeSummary), the mode precedence, the dwarf-ratio math, and the user-boundary scan.Verified sound:
Mode precedence matches the documented contract. The
switchinfinalOutput(finalize.go:51) testsisCitationsOnly→isWeakFinal→isSummaryCloserin that order.isWeakFinal(finalize.go:197) requiresbackRefReandlen ≤ 120, so a back-reference within the weak cap correctly pre-empts the summary arm. Confirmed against thebothMatchClosertest: it's ~106 bytes and matchesas i mentioned, so it routes tomodeBackRef, andlongAnswer(275 B) is recovered via the≥200shortcut inisSubstantiveAnswer, not the 3× bar.Dwarf ratio re-derived and correct.
dwarfs := len(txt) >= recoverRatio*len(terminal)withrecoverRatio=3(finalize.go:295,182). For the 153-byteb3cb9ee9Closer, the bar is 459 B:hugeAnswer(12×46−1 = 551 B) clears it → recovered;longAnswer(275 B) does not → closer kept. Both match the corresponding test expectations. Byte-vs-byte comparison is consistent throughout (em dash = 3 bytes), and the 153/151/300 magic numbers all check out.User-boundary break is safe. The
mode == modeSummary && m.Role == llm.RoleUserbreak (finalize.go:254) does not misfire on tool results:ToolResultsMessageisRoleTool, notRoleUser(llm/message.go:69–70), so the scan still reaches the front-loaded assistant turn that sits between the user question and the tool-result message. Verified the run loop builds exactly that ordering (agent.go:278,325,364). The break correctly prevents resurrecting a stale answer to a prior question, as its dedicated test exercises.Classifier regex has no false-positive hole I could find.
summaryCloserRerequires the bookkeeping verb (or noun, in thelogged …branch) to be immediately followed by[.!], so prose about citations ("…are recorded in the court transcript…", "…logged in Zotero whenever…") fails because a space, not a terminator, follows the verb. Walked every true/false row inTestIsSummaryCloserby hand against the assembled pattern — all consistent.No regression vs. pre-PR behavior. A back-reference-flavored closer over the 120-byte weak cap was already returned as-is before this change (it's neither weak nor citations-only); the new summary arm can only add recovery when a prior turn dwarfs it, never remove existing recovery. So the mode split doesn't degrade any previously-handled case;
modeBackRef/modeCitationspaths are byte-for-byte equivalent to before.Nothing in the correctness lens is materially wrong.
🧹 Code cleanliness & maintainability — Minor issues
I've verified all three findings against the actual source in
agent/finalize.go.summaryLead =[\s>#_+-]`` (line 131) is byte-for-byte identical to the leading class incitationLabelRe(line 156:^[\s>#*_+-]*(sources?...), and the inline comment does say "as in citationLabelRe."summaryCopulas(line 134) listsallalongsideare|were|have been, andsummaryArticle(line 136) also matchesall.mode := modeBackRefat line 50, then an emptycase isWeakFinal(terminal):with only a// modeBackRefcomment.All three are accurate readability/naming nits. All survive.
VERDICT: Minor issues
This is unusually well-documented, well-structured code — the three-way
recoveryModerefactor is a clean generalization of the prior two-way bool, naming is clear (modeBackRef/modeCitations/modeSummary), the fragment-assembled regex is more legible than a monolith, and the doc comments accurately match the control flow (I traced the switch order against the "citations first, back-ref beats summary" claims — they're correct). The findings below are all minor.agent/finalize.go:131/:156— duplicated leading-marker character class.summaryLead =[\s>#_+-]is a byte-for-byte copy of the leading class in `citationLabelRe`, and the comment (`// … as in citationLabelRe`) explicitly acknowledges the copy. Two independent literals for "the same set of leading markdown/list markers" will drift if one is ever extended. Low-churn fix: hoist a shared `const leadMarkers = `[\s>#*_+-]*and reference it from bothsummaryLeadandcitationLabelRe.agent/finalize.go:134,136—allappears in bothsummaryCopulasandsummaryArticle, and it isn't a copula.summaryCopulas =((are|were|have\s+been|all)\s+)*lists `all` alongside the genuine copulas `are/were/have been`, while `summaryArticle = `((all|the)\s+)*also matchesall. A reader parsing the grammar sees the same token admitted in two fragments under a misleading name (a "copula" fragment containing a quantifier word). Consider droppingallfromsummaryCopulas(or renaming the fragment) so each named piece owns one grammatical role. This is a readability/naming nit, not a matching-behavior claim.agent/finalize.go:54-55— emptycasebody relying on the pre-initializedmode.This depends on
mode := modeBackRefset two lines up; the comment documents it, but an explicitmode = modeBackRefin the case body is more self-contained and survives a future refactor that changes the initializer. Trivial, optional.No issues with the test additions from a cleanliness standpoint — the new cases are named descriptively and the
hugeAnswer/b3cb9ee9Closerfixtures read clearly (the local re-declaration ofhugeAnsweracross two test funcs is normal Go test style).⚡ Performance — No material issues found
I've verified the code and the single call site. Here's my performance-lens review.
VERDICT: No material issues found
This change adds a terminal-turn classifier (
isSummaryCloser), a new regex, arecoveryModeenum, and a user-boundary early-breakto the recovery scan. Through the performance lens it is clean:finalOutputruns exactly once, atagent/agent.go:333, when the loop reaches a clean terminal turn — not inside the model/tool loop. The added work executes once per agent run over a bounded transcript. Verified by grepping the only caller.summaryCloserReis a package-levelvar(finalize.go:139), compiled at init, not per-call. The nested-star alternations (summaryCopulas,summaryArticleuse*over alternations) would be a catastrophic-backtracking (ReDoS) risk under a PCRE-style engine, but Go'sregexpis RE2 — guaranteed linear time, no backtracking — so there is no pathological-input blowup. Not a performance finding.isSummaryCloserreturns early whenlen(t) > summaryCloserMaxChars(300) before ever running the match (finalize.go:240), so regex work is bounded to short strings. Good ordering.lastSubstantiveAssistantTextis a single newest→oldest pass. The newmodeSummarybreakat the user-message boundary (finalize.go:254) only shortens the scan; per-iteration cost (TrimSpace,Text(), short-string checks) is unchanged. No added allocations inside the loop that matter at this scale.isCitationsOnly→isWeakFinal→isSummaryCloser) short-circuits, running at most a few small regex matches on the terminal string.Nothing here warrants a change on efficiency grounds.
🧯 Error handling & edge cases — Minor issues
I've verified the code at
agent/finalize.go. The single surviving finding checks out against the actual source:finalOutputat line 75 returns barerecformodeSummary(andmodeBackRef), whilemodeCitationsalone appends the terminal viarec + "\n\n" + tailat line 72 — confirmed.modeSummarygate is purely length-based:dwarfs := len(txt) >= recoverRatio*len(terminal)(line 295), a length comparison, not a containment/substring test — confirmed.modeSummarydoc (lines 100–108) andisSummaryCloserdoc (lines 231–237) explicitly state the closer is discarded on the assumption its content is a strict subset of the recovered turn — confirmed the tradeoff is deliberate.The scenario the finding describes (a short closer carrying a net-new correction, discarded when a longer front-loaded answer dwarfs it) is a genuine, reproducible consequence of the length-only guard. The finding accurately reflects the code and is honestly scoped as a low-severity, deliberate tradeoff. It survives.
VERDICT: Minor issues
Reviewing strictly through the error handling & edge cases lens.
Most unhappy paths here are handled defensively and I could not fault them:
isSummaryCloserguardst == ""andlen(t) > summaryCloserMaxCharsbefore touching the regex (finalize.go:240); emptymsgsfalls through the scan loop to("", false)→ returns terminal. No panic paths, no slice indexing, no div-by-zero (divisor is a const3). Verified by reading.recoverRatio*len(terminal)(finalize.go:295) is bounded (summary terminal ≤ 300 bytes); no realistic overflow.fail-closedbehavior — when nothing dwarfs, or the user-boundarybreak(finalize.go:254) is hit first, recovery returns("", false)and the closer is preserved. Correct and matches the documented intent.switchinfinalOutput(finalize.go:51) orders citations → weak/back-ref → summary, so a terminal matching both back-ref and the ack is routed tomodeBackRefand never subjected to the stricter dwarf bar. Verified against the regex anchors and the doc.One genuine edge case is left unhandled:
finalize.go:56,75— a summary closer carrying net-new content is silently discarded. UnlikemodeCitations, which appends the terminal (rec + "\n\n" + tail, line 72),modeSummaryfalls through toreturn rec(line 75) and drops the closer entirely, on the stated assumption that "its content is a strict subset" of the recovered turn. That assumption is only guarded by the 3× dwarf ratio (len(txt) >= recoverRatio*len(terminal), line 295), which is a length test, not a containment test. A model that writes"Citations are logged. One correction: the figure was $46M, not $64M."as its closer, after a long (dwarfing) front-loaded answer, would have that correction thrown away — the user receives the stale, uncorrected front-loaded text. The citations arm avoids exactly this by keeping the terminal; the summary arm cannot, because it has no way to tell "compression of the above" from "compression + a delta." Impact: possible loss of the most recent/authoritative content on precisely the unhappy path this diff targets. This is a documented, deliberate tradeoff, so I rate it low: severitysmall, confidencemedium. A cheap mitigation would be to append the closer (as citations do) rather than discard it, or to only discard when the closer text is substring-contained inrec.Everything else in my lane checks out. The classifier's false-positive surface (prose about citations, mid-sentence mentions, compression-without-ack) is explicitly bounded by the
[.!]-directly-after-verb anchor and the length cap, and the tests exercise those boundaries.claude-code/sonnet (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(and the newsummaryCloserRe/isSummaryCloserlogic in the diff) specifically through the security lens: authn/authz, injection, SSRF, deserialization, secret leakage, input validation, and unsafe handling of untrusted data.Findings:
summaryCloserRe, reusing existingsummaryLead/summaryArticle/summaryCopulasfragments) are compiled via Go's standardregexppackage, which is RE2-based and backtracking-free — so even though the fragments contain nested*/+quantifiers (summaryArticle,summaryCopulas), there's no catastrophic-backtracking/ReDoS exposure regardless of adversarial input length. Verified by reading the pattern construction atagent/finalize.go:130-141and confirming Go'sregexpengine guarantees linear-time matching (notregexp/syntaxbacktracking).isSummaryCloserbounds input length before regex evaluation (len(t) > summaryCloserMaxCharsatagent/finalize.go:240), which further limits any per-call cost — though as noted this isn't actually needed for RE2 safety.msgs, not raw external/untrusted content being rendered, executed, or used to construct a query — there's no injection sink here (no SQL, shell, template, or URL construction from this data).modeSummaryuser-boundary scan (agent/finalize.go:254-262) is a correctness/data-scoping control, not a trust-boundary check, and doesn't weaken any authorization boundary — it only affects which of the agent's own messages are eligible to be substituted as output.No secrets, credentials, or user-supplied data paths are touched by this diff. Nothing in this PR falls within the security lens as a genuine defect.
🎯 Correctness — No material issues found
Both helpers already exist and are reused correctly, not redefined. This confirms the diff is self-consistent.
I've traced the byte-length math (b3cb9ee9Closer = 153 bytes / 151 runes, verified by hand), the dwarf-ratio arithmetic for
longAnswer/hugeAnsweragainst the 3× threshold, thesummaryCloserReregex against every positive/negative test case, the mode-dispatch priority infinalOutput, themodeSummaryearly-return gate inisSubstantiveAnswer, the user-boundary scan-stop (confirmedToolResultsMessageusesRoleTool, distinct fromRoleUser, so it doesn't prematurely trip the boundary check), and cross-checked thatmodeBackRef/modeCitationsbehavior is algebraically identical to the pre-PR code. Everything lines up with the documented intent and the tests' expected outcomes.VERDICT: No material issues found
No correctness defects surfaced. Specific things I verified rather than assumed:
b3cb9ee9Closeris exactly 153 bytes (Golen()) / 151 runes by manual count — matches the code comment and confirms3×153=459is the real dwarf threshold used againstlongAnswer(275 bytes, correctly fails) andhugeAnswer(551 bytes, correctly passes).summaryCloserRe(agent/finalize.go:139-141) correctly rejects all "verb not immediately followed by[.!]" near-miss cases (e.g. "recorded in the court transcript", "logged in Zotero") because RE2 must find some full match path, and none exists when the verb isn't immediately adjacent to a sentence terminator — traced this against all four negative regex test cases.modeSummarydwarf-gate inisSubstantiveAnswer(finalize.go:294-306) is applied before therecoverMinChars/floor checks, so it truly is mandatory at every length, not just short ones — confirmedmodeBackRef/modeCitationsreduce to byte-identical boolean expressions vs. the pre-PRcitations boolversion.llm.ToolResultsMessage, because that constructor setsRole: llm.RoleTool(llm/message.go:70), a distinct value fromRoleUser— this was the one place I suspected a real bug (Anthropic's wire format sends tool results as a user-role turn) but the canonicalllm.Messagetype keeps them separate, so no bug.finalOutputtable cases and the e2e fake-provider test by hand against the actual message slices and confirmed thewantvalues match what the code produces.🧹 Code cleanliness & maintainability — Minor issues
Both findings check out against the actual source. The switch at
finalize.go:51-60is the file's only switch statement, confirming the empty-body case is a one-off; and thesummaryLeadliteral at line 131 is byte-for-byte identical to the leading fragment embedded incitationLabelReat line 156.VERDICT: Minor issues
agent/finalize.go:54-55— Thecase isWeakFinal(terminal): // modeBackRefbranch has an empty body and relies on the reader noticing thatmodewas pre-initialized tomodeBackRefon line 50, several lines above the switch. Every other case explicitly assignsmode(mode = modeCitations,mode = modeSummary), so this one silently breaks the pattern; a future edit that reorders cases, changes therecoveryModeiota order, or adds a fourth mode is one easy slip away from this branch doing the wrong thing with no compiler warning. Prefermode = modeBackRefin the case body for symmetry with the other two arms — it costs nothing and removes the implicit dependency on the pre-switch initializer.agent/finalize.go:131vsagent/finalize.go:156—summaryLeadduplicates the exact literal character class (`[\s>#*_+-]*`) already inlined incitationLabelRe. The new code's own comment ("leading markdown/list markers, as in citationLabelRe") acknowledges the duplication rather than removing it. Since this PR is already restructuring the file's regexes into named fragments for legibility, this was a natural point to hoist the shared class into one identifier (e.g.leadingMarkersRe) used by bothsummaryCloserReandcitationLabelRe, so a future tweak to what counts as a "leading marker" doesn't require remembering to update it in two places.Everything else in the diff reads cleanly for a feature of this size: the
citations bool→recoveryModeenum rename is a real readability win over the old boolean, the new regex is assembled from named fragments consistent with the file's existingcitationLabelRestyle, and the extensive doc comments match this file's established convention of explaining classifier rationale inline.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed
agent/finalize.goandagent/finalize_test.gothrough the performance lens.modeSummarypath adds one moreregexp.MustCompile'd pattern (summaryCloserRe, compiled once at package init — not per-call) and one morestrings.TrimSpace+ regex match per terminal turn (isSummaryCloser), gated behind alen(t) > summaryCloserMaxCharsearly-out. This is O(len(terminal)) work done once per agent turn, not in a loop — negligible.lastSubstantiveAssistantText(finalize.go:250-276) is unchanged in complexity: still a single O(n) newest→oldest scan overmsgs. The newmodeSummaryearly-breakon encountering aRoleUsermessage (finalize.go:254-263) reduces work versus the other two modes' unbounded scan — it's a performance win, not a regression, for the new mode.regexppackage is RE2-based (guaranteed linear-time, no catastrophic backtracking), so the nested-star groups insummaryCloserRe(summaryArticle,summaryCopulasas(...)*) don't carry the exponential-blowup risk they'd have under a backtracking engine — not a perf concern here.Nothing to flag in this lens.
🧯 Error handling & edge cases — No material issues found
The review is complete — no plan or implementation work applies here since this was a read-only adversarial code review request, not a build task. I already delivered the full Gadfly verdict and findings above (verdict: No material issues found for the error-handling & edge-cases lens, with an empty
gadfly-findingsJSON array), based on static reading ofagent/finalize.goandagent/finalize_test.goplusllm/message.go. No files were modified.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, ReDoS, tainted-input handling, secret leakage, unsafe defaults).
What I checked and why it's clean:
finalOutputand the newisSummaryCloser/lastSubstantiveAssistantTextpath only perform string matching and return a string; there's noeval, shell, SQL, template, path, or network call. The recovered value is an assistant message already present inmsgs— nothing new is exposed (no secret/credential leakage).summaryCloserRe. I read the assembled regex atagent/finalize.go:130-141. The potentially concerning fragment issummaryCopulas = ((are|were|have\s+been|all)\s+)*(nested*with\s+), which is a classic catastrophic-backtracking shape in general. Two factors make it safe here: (1) the alternation branches (are|were|have been|all) are mutually exclusive words, so there is no ambiguous way to partition a repeated token — backtracking over the*is linear, not exponential; (2)isSummaryCloser(finalize.go:238-244) enforceslen(t) > summaryCloserMaxChars(300) and returns before the regex is ever run, bounding input size. I also confirmedsummaryLead([\s>#*_+-]*) andsummaryPreface(optional, single-iteration) are linear. No material ReDoS risk.terminalis model output (not directly user-controlled), and even under prompt-influence the classifier fails closed: an unmatched closer keeps today's behavior, and the dwarf-ratio + user-message boundary inmodeSummaryprevent resurrecting unrelated prior content. No new untrusted-data sink is introduced.No security findings to report.
🎯 Correctness — No material issues found
I verified the change end-to-end against the checked-out code: the
finalOutputswitch ordering, the assembledsummaryCloserReregex (re-derived each test-table case by hand against the named fragments), themodeSummarydwarf-ratio gating and user-boundary break inlastSubstantiveAssistantText/isSubstantiveAnswer, and the byte-length thresholds (recoverRatio=3,summaryCloserMaxChars=300,weakFinalMaxChars=120) against the test inputs. I also grepped for all callers of the renamed/retyped functions to confirm no stale call sites remain.VERDICT: No material issues found
finalize.go:51-60) correctly implements the documented precedence — citations > back-ref > summary — and I confirmed thebothMatchClosertest case (105 bytes, ≤120, contains "as I mentioned") lands inisWeakFinal→modeBackRef, notmodeSummary, solongAnswer(275 ≥recoverMinChars=200) is recovered. Matches the test'swant.isSubstantiveAnswer(finalize.go:294-305): themodeSummary && !dwarfsearly-return sits before thelen(txt) >= recoverMinCharsunconditional accept, so the 3× ratio is genuinely mandatory at every length for summary closers, as the doc claims. Re-derived: hugeAnswer=551 vs closer=153 → 3×153=459, 551≥459 ✓ (recovered); longAnswer=275 vs 153 → 275<459 ✗ (kept); both match the two "discarded"/"kept" tests.lastSubstantiveAssistantTextuser-boundary break (finalize.go:254-263) is gated onmode == modeSummaryonly, so the existing back-ref/citations modes keep their unbounded scan — no regression. The "never resurrects a stale answer" and "still works with history present" tests both trace correctly: the dwarfing in-window answer is reached before the user message; the non-dwarfing one is rejected and the scan then breaks at the user message."The citations are recorded in the court transcript…"fails because[.!]must immediately follow the verb (next char is a space);"Citations are logged in Zotero…"fails for the same reason;"Short version: yes."fails the noun alternation; the >300-byte over-cap case is rejected by the length guard before the regex. All align with the test table.lastSubstantiveAssistantText/isSubstantiveAnswerexist outsidefinalize.goandfinalize_test.go(grepped), so thebool → recoveryModesignature change is fully covered.🧹 Code cleanliness & maintainability — Minor issues
All three findings verified against the actual source.
summaryLead =[\s>#_+-]`` confirmed, and line 156citationLabelReopens with^[\s>#*_+-]*— same character class, duplicated. The comment on 131 even says "as in citationLabelRe."summaryCopulas =((are|were|have\s+been|all)\s+)*confirmed; `all` appears here and again in `summaryArticle = `((all|the)\s+)*at line 136, where only the latter is commented ("star, not ?:").summary*consts exist solely to assemblesummaryCloserReat lines 139-141.Verdict: Minor issues
I read the full
agent/finalize.gochanges and the test file, and traced the newmodeSummaryscan logic against the test transcripts. The change is well-structured — the three-wayrecoveryModecleanly replaces the old bool, the switch ordering matches the documented precedence (citations → back-ref → summary), and the regex fragments are commented. Through the cleanliness lens I found only minor nits.Findings:
agent/finalize.go:131—summaryLead = [\s>#*_+-]*is a verbatim duplicate of the leading-marker class incitationLabelReatagent/finalize.go:156(^[\s>#*_+-]*), and the comment onsummaryLeadeven says "as in citationLabelRe." Two copies of an intended-identical character class that can drift independently. Suggest hoisting one sharedconst leadMarkers =[\s>#_+-]`` and interpolating it into both regexes. Low impact, but it's exactly the kind of "kept in sync by convention" smell this lens looks for.agent/finalize.go:134—allappears in bothsummaryCopulas((are|were|have\s+been|all)) andsummaryArticle((all|the)), serving overlapping grammatical roles. OnlysummaryArticle'sallis commented ("star, not ?: …"); theallinsidesummaryCopulas(for "Citations all logged.") is uncommented and a future reader has to puzzle out why the same word lives in two adjacent fragments. A one-line comment onsummaryCopulasnoting the "all logged" case would remove the ambiguity.agent/finalize.go:130-137— the sixsummary*package-level constants are used solely to assemble the singlesummaryCloserRe. That's defensible (the PR calls it out as a readability choice), but at package scope they read like reusable exported fragments when they're really local builder pieces. Scoping them into avar summaryCloserRe = func() *regexp.Regexp { … }()block, or even just a single multi-line raw-string regex with inline comments, would keep the package namespace tighter. Trivial — leave as-is if the author prefers the named-fragment style.No dead code, no copy-paste of logic (only of one character class), and the new tests follow the existing table-driven patterns. The lens is otherwise clean.
⚡ Performance — No material issues found
Verdict: No material issues found
Through the performance lens:
finalOutputruns exactly once perRun, at the final tool-free turn (agent.go:333). The diff adds one more regex test (isSummaryCloser) to theswitchinfinalOutput(finalize.go:52-60). For a standalone answer this meansisCitationsOnly→isWeakFinal→isSummaryCloserare each evaluated once, each doing astrings.TrimSpaceplus an anchored regex on a short string (≤300 chars for the summary path, capped atsummaryCloserMaxChars). That is one extraTrimSpaceallocation and one anchored-regex match per Run — negligible.summaryCloserReis compiled once at package init (finalize.go:139var summaryCloserRe = regexp.MustCompile(...)), not per call. Verified by reading the declaration.lastSubstantiveAssistantText(finalize.go:252-263) adds abreakat the firstRoleUsermessage formodeSummary, capping the backward scan at the current turn's window. The other two modes retain their historical unbounded scan. So the new mode does less message-list traversal than the existing ones — no regression, and no new quadratic/unbounded behavior.isSubstantiveAnsweradds onelencomparison (dwarfs := len(txt) >= recoverRatio*len(terminal),finalize.go:295) computed unconditionally now. That's an O(1) integer compare on already-resident strings; formodeSummaryit short-circuits before the preamble regex. No measurable cost.agent.go:325-338) is unchanged; the only new code executes after the loop exits.I verified the call site (
agent.go:333), the regex compilation site, the scan loop, andisSubstantiveAnswerby reading the files. Nothing in the performance lane is materially wrong.🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
I reviewed the diff through the error-handling / edge-case lens and verified the relevant paths against the checked-out code.
Empty/nil inputs:
finalOutputand the newisSummaryCloserboth guard empties.isSummaryCloserreturns false ont == ""(finalize.go:240) before touching the regex, and empty terminals are routed tomodeBackRefviaisWeakFinal(finalize.go:197-203), never tomodeSummary.lastSubstantiveAssistantTextwithlen(msgs)==0simply doesn't enter the loop and returns("", false)→ caller returnsterminal. No panic path.modeSummaryuser-boundarybreak(finalize.go:254-263): Thebreakis placed before them.Role != llm.RoleAssistantcontinue, so a user-role message halts the scan correctly. Verified the walk order against the test transcript shapes:[user, asst(front-loaded+cite), toolresult, asst(closer)]— the front-loaded assistant turn is evaluated before the user message is reached, so legitimate same-turn recovery still works (covered by "summary closer recovery still works with history present"). When the front-loaded turn fails the ratio, the break prevents resurrecting a stale cross-question answer (covered by "never resurrects a stale answer across the user boundary"). A mid-run user steer failing closed is explicitly documented and acceptable.Fail-closed invariant: Every new classification/recovery path falls back to returning
terminalunchanged when nothing qualifies —isSummaryCloserfalse →default: return terminal;lastSubstantiveAssistantText!ok→return terminal;isSubstantiveAnswerratio failure → closer kept. No swallowed error, no silent substitution.Dwarf ratio with zero-length terminal (modeSummary): Cannot occur —
modeSummaryis only reachable viaisSummaryCloser, which rejects empty. SorecoverRatio*len(terminal)(finalize.go:295) never multiplies by zero in this mode; and even if it did, it's a multiplication (no division), so no divide-by-zero exists anywhere. Integer overflow (3*len) is unreachable given the 300-bytesummaryCloserMaxCharscap and the 120-byteweakFinalMaxCharscap for back-ref.Regex safety:
summaryCloserReis a staticregexp.MustCompileat init (RE2, no backtracking panic risk);MatchStringis only called after the empty/length guards. The[.!]terminator anchor correctly rejects mid-sentence prose ("…recorded in the court transcript…", "…logged in Zotero…"), confirmed by reading the pattern fragments at finalize.go:131-150.Three-way
switchordering (finalize.go:51-60):isCitationsOnly→isWeakFinal→isSummaryCloser. A terminal matching both the ack andbackRefRewithin the 120-byte weak cap is routed tomodeBackRef(verified viabothMatchClosertest), so the stricter summary dwarf-ratio is correctly NOT applied to a content-free back-ref. No fall-through is missing thedefault.Nothing in my lane is materially wrong.
kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Reviewed through the Security lens. I traced the new
modeSummaryrecovery path, thesummaryCloserReregex construction, thelastSubstantiveAssistantTextbackward scan with itsRoleUserboundary guard, and the interaction with steer messages.Regex safety: Go's
regexppackage uses the RE2 engine, which guarantees linear-time matching. ThesummaryCloserRealternations contain only bounded quantifiers (*,?,+on small character classes), and the input toisSummaryCloseris capped at 300 bytes. No ReDoS surface.Boundary crossing: The
modeSummaryscan explicitly breaks onllm.RoleUser, preventing resurrection of stale answers from earlier questions. The comment correctly notes that a mid-run steer message of roleUseralso acts as a boundary (fail-closed is the stated and acceptable behavior). I verified thatRoleToolmessages are skipped andRoleSystemsteer messages would not inadvertently stop the scan—but in the expected message ordering, system steer messages appear between tool results and the terminal, so continuing past them still lands on the correct front-loaded assistant turn.No injection or deserialization: All regexes are compiled from hardcoded fragments; no user input reaches
MustCompile. No unsafe deserialization, no SQL/command/template injection, no SSRF, no credential leakage.No information leakage: Recovery only reorders or selects among assistant turns already present in the in-memory conversation slice. No cross-conversation or hidden data is exposed.
The change is clean from a security standpoint.
🎯 Correctness — No material issues found
No material issues found
After thorough review through the correctness lens, the logic is sound:
Regex precision verified:
summaryCloserRecorrectly requires the ack sentence to end with[.!]directly after the verb, so mid-sentence prose like "The citations are recorded in the court transcript…" is rejected. ThesummaryLead/summaryPrefacehandling of markdown markers and conversational openers is consistent with the existingcitationLabelRepattern.Mode ordering is intentional and correct:
isCitationsOnlywins first (preserving links), thenisWeakFinal(so "Citations are logged. As I said above…" uses the looser back-ref bar, not the summary dwarf ratio), thenisSummaryCloser. This matches the documented semantics.Boundary semantics verified:
modeSummarybreaks the scan atRoleUser, preventing resurrection of stale answers from earlier questions. The test "summary closer never resurrects a stale answer across the user boundary" confirms this.Dwarf ratio semantics verified: For
modeSummary,dwarfsis checked first at every length, so a prior turn likelongAnswer(~282 chars) fails to displace a 153-char closer (needs ≥459). The test "summary closer kept when the prior turn does not dwarf it" validates the conservative bar.Discard vs keep behavior is correct:
modeSummaryreturnsrec(discards the terminal), mirroringmodeBackRef, whilemodeCitationsappends. This matches the domain model: a summary closer's content is a strict compression/subset, whereas citations are new useful content.Constants re-derived:
recoverRatio = 3,recoverMinChars = 200,recoverFloorChars = 80,summaryCloserMaxChars = 300— all consistent with existing thresholds and the documented b3cb9ee9 incident dimensions.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
agent/finalize_test.go:126-127—longAnswerandhugeAnswerare built with identicalstrings.Repeat("Free group calls are capped at sixty minutes. ", …)expressions inTestFinalOutput, then re-defined verbatim inTestRun_RecoversFrontLoadedAnswer(line 433),TestRun_RecoversFrontLoadedAnswerOverSummaryCloser(line 482), andTestRun_RecoversFrontLoadedAnswerWithCitations(line 513). Extract these to package-level test fixtures (e.g.var testLongAnswer = strings.TrimSpace(strings.Repeat(..., 6))) so the next person who tweaks the threshold lengths only has to change one place.agent/finalize_test.go:357,391— Test-case comments embed external review references (// Gadfly (opus, correctness):,// Gadfly (opus, error-handling):). These will be meaningless noise to future maintainers who don't have access to that review context. Replace them with plain prose describing the invariant being guarded.⚡ Performance — No material issues found
No material issues found
Through the performance lens, the changes are clean:
finalOutputis invoked once per agent completion on the terminal turn. ThelastSubstantiveAssistantTextscan is O(n) over the message slice with a small, bounded n. No hot-path concerns.summaryCloserReis compiled once at package init viaregexp.MustCompile; no repeated compilation.isSummaryCloserusessummaryCloserMaxChars = 300as a cheap length-gate before regex evaluation, avoiding regex work on obviously-long terminals.modeSummary, the scan now stops at the firstRoleUserboundary when iterating newest→oldest. This is a performance improvement (shorter scan) compared to the unbounded historical scan for other modes.isSubstantiveAnsweradds only integer comparisons and an enum switch; no new allocations or expensive work.🧯 Error handling & edge cases — Minor issues
Minor issues
agent/finalize.go:139-141— ThesummaryCloserReregex matches a prefix ending in[.!]but does not enforce that the sentence actually terminates there (whitespace or end-of-string). BecauseMatchStringreturns true for any prefix match, a string like"Citations are logged...and then more text"matches even though the model is continuing prose about citations rather than producing a bookkeeping ack. This false positive could cause legitimate terminal text to be misclassified as a summary closer and discarded when a prior turn dwarfs it. Tighten the regex to require\s|$after[.!], e.g.[.!](?:\s|$).Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
@gadfly review
Round 1 fixes pushed in
21b4775:WithHistory. Regression test seeds history with a dwarfing unrelated answer; a companion test proves legitimate recovery still works with history present.isWeakFinalis now tested beforeisSummaryCloser, so a closer matching both gets the looser back-ref bar. Regression test covers the [200, 3×terminal) window where the two modes diverge.+added to the leading-marker class (kimi), verb-first form accepts "all the", dwarf ratio hoisted into a nameddwarfslocal (glm), 151→153 char/byte comment corrections (sonnet).!–:range claim is a false positive — the hyphen in that class is in terminal position, hence literal.🪰 Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -40,0 +51,4 @@switch {case isCitationsOnly(terminal):mode = modeCitationscase isWeakFinal(terminal):⚪ Empty isWeakFinal case relies on pre-initialized mode; explicit
mode = modeBackRefwould be more self-containedmaintainability · flagged by 2 models
agent/finalize.go:54-55— emptycasebody relying on the pre-initializedmode.go case isWeakFinal(terminal): // modeBackRefThis depends onmode := modeBackRefset two lines up; the comment documents it, but an explicitmode = modeBackRefin the case body is more self-contained and survives a future refactor that changes the initializer. Trivial, optional.🪰 Gadfly · advisory
@@ -73,0 +127,4 @@// would hijack a legitimate answer; an unmatched closer merely keeps today's// behavior (fail closed). Assembled from named fragments so the alternations// stay legible and extendable.const (🟡 Leading-marker char class
[\s>#*_+-]*duplicated between summaryLead and citationLabelRe (comment admits the copy); hoist a shared constmaintainability · flagged by 3 models
agent/finalize.go:131/:156— duplicated leading-marker character class.summaryLead =[\s>#_+-]is a byte-for-byte copy of the leading class in `citationLabelRe`, and the comment (`// … as in citationLabelRe`) explicitly acknowledges the copy. Two independent literals for "the same set of leading markdown/list markers" will drift if one is ever extended. Low-churn fix: hoist a shared `const leadMarkers = `[\s>#*_+-]*and reference it from bothsummaryLeadand `citationLabel…🪰 Gadfly · advisory
@@ -73,0 +136,4 @@summaryArticle = `((all|the)\s+)*` // star, not ?: "Logged all the citations.")var summaryCloserRe = regexp.MustCompile(`(?i)^` + summaryLead + summaryPreface +🟠 summaryCloserRe regex can false-positive on prose where [.] is not end-of-sentence
error-handling · flagged by 1 model
agent/finalize.go:139-141— ThesummaryCloserReregex matches a prefix ending in[.!]but does not enforce that the sentence actually terminates there (whitespace or end-of-string). BecauseMatchStringreturns true for any prefix match, a string like"Citations are logged...and then more text"matches even though the model is continuing prose about citations rather than producing a bookkeeping ack. This false positive could cause legitimate terminal text to be misclassified as a summ…🪰 Gadfly · advisory
@@ -84,3 +124,3 @@func TestFinalOutput(t *testing.T) {cite := []llm.ToolCall{{ID: "c1", Name: "cite", Arguments: json.RawMessage(`{}`)}}longAnswer := strings.TrimSpace(strings.Repeat("Free group calls are capped at sixty minutes. ", 6)) // >200longAnswer := strings.TrimSpace(strings.Repeat("Free group calls are capped at sixty minutes. ", 6)) // >200🟡 Duplicate test fixture construction across multiple test functions
maintainability · flagged by 1 model
agent/finalize_test.go:126-127—longAnswerandhugeAnswerare built with identicalstrings.Repeat("Free group calls are capped at sixty minutes. ", …)expressions inTestFinalOutput, then re-defined verbatim inTestRun_RecoversFrontLoadedAnswer(line 433),TestRun_RecoversFrontLoadedAnswerOverSummaryCloser(line 482), andTestRun_RecoversFrontLoadedAnswerWithCitations(line 513). Extract these to package-level test fixtures (e.g. `var testLongAnswer = strings.TrimSpace(strings…🪰 Gadfly · advisory
@@ -259,0 +354,4 @@want: b3cb9ee9Closer,},{// Gadfly (opus, correctness): the modeSummary scan must stop at the🟡 External review reference embedded in permanent test comment
maintainability · flagged by 1 model
agent/finalize_test.go:357,391— Test-case comments embed external review references (// Gadfly (opus, correctness):,// Gadfly (opus, error-handling):). These will be meaningless noise to future maintainers who don't have access to that review context. Replace them with plain prose describing the invariant being guarded.🪰 Gadfly · advisory