fix(agent): recover the answer behind a bare "above" pointer #28

Merged
steve merged 10 commits from fix/backref-above-closer into main 2026-08-22 04:53:04 +00:00
Owner

Fixes the harness half of mort issue #1611.

What happened

mort run 8eea3e82-b61b-4174-9750-9aa2f4bde4d4 (ollama-cloud/glm-5.2:cloud) front-loaded a 2,245-char analysis into its cite-call turn (step #8, finish=tool_calls), then closed with 220 bytes:

Done — that's the full chain above. Short version: it's not one incident, it's the confluence of the Iran war, the Epstein files, and three ex-allies now openly plotting a third-party movement that finally set him off.

The user got the 220 bytes and a pointer at a "chain above" that was never posted — the analysis lived only in the transcript.

Why the existing recovery missed it

finalOutput already had three shapes for exactly this pathology. The closer matched none of them:

classifier verdict why
isCitationsOnly false no sources heading
isWeakFinal false 220 bytes > the 120-byte weak cap; backRefRe has no phrase for "the full chain above"
isSummaryCloser false no Citations are logged.-style ack to open on

So finalOutput returned the terminal verbatim. Reproduced before touching anything:

len=220  isWeakFinal=false  isCitationsOnly=false  isSummaryCloser=false
finalOutput passthrough=true

The fix — one shared signal, not a fourth shape

Adding a fourth phrase list is what produced the third one. Two of the three shapes are really one signal: the terminal defers — it tells us the answer is somewhere the user cannot see. They differ only in whether the terminal also carries content of its own (bare back-reference → discard; back-reference + compression → discard under a strict bar). That signal is now isBackRef, shared by both, so a new phrasing is added once and is covered in the bare and the "+ compression" variant at the same time.

Its open-ended half is aboveRefRe: a deictic above, separated from the preposition by what follows the word.

  • deictic — ends its clause: that's the full chain above. · as shown above, · a line that just ends in above
  • preposition — always continues into a noun phrase: above 100°C · above the fold · above all, …

pointsAbove additionally requires the reference inside the terminal's first backRefHeadChars (120) bytes: with almost no text before it in this message, the reference cannot be pointing at the message's own content, so it must point at a turn the user never saw. That is the same guard as weakFinalMaxChars, expressed as an offset — needed because a summary closer carries its compression after the pointer and so is not itself short.

isSummaryCloser now opens on either the citations ack or pointsAbove. Recovery is unchanged: the mandatory dwarf ratio (prior turn ≥ 3×) and the user-message scan boundary still gate it, so a closer only loses to a prior turn in the same user turn that is clearly the fuller original.

Tests

New: TestPointsAbove (deictic vs. prepositional split, both ends of the offset bound), deictic cases in TestIsWeakFinal / TestIsSummaryCloser, three TestFinalOutput cases (recover · ratio-too-low keeps the closer · prepositional above survives a dwarfing prior turn), and TestRun_RecoversFrontLoadedAnswerOverAboveRefCloser end-to-end with the verbatim closer. No existing expectation changed.

Break-checked — four mutations, each killed by a specific named test, unmutated control survives:

mutation killed by
isSummaryCloser back to citations-ack only TestIsSummaryCloser/1611-verbatim, TestFinalOutput/above-pointer_closer_discarded…, TestRun_RecoversFrontLoadedAnswerOverAboveRefCloser
isWeakFinal back to the fixed phrase list TestIsWeakFinal/bare-above-pointer, /above-pointer-in-parens
aboveRefRe matches any above TestPointsAbove/above-a-number, /above-the-fold, /above-all, /above-average, TestFinalOutput/prepositional_above…
pointsAbove drops the offset bound TestPointsAbove/late-reference-not-a-pointer

go build ./... · go vet ./... · gofmt -l . empty · go test ./... -count=1 -race all green.

The mort-side half of #1611 (recorded cite rows never reaching the Discord reply) is a separate mort PR.

Fixes the harness half of mort issue [#1611](https://gitea.stevedudenhoeffer.com/steve/mort/issues/1611). ## What happened mort run `8eea3e82-b61b-4174-9750-9aa2f4bde4d4` (`ollama-cloud/glm-5.2:cloud`) front-loaded a **2,245-char analysis** into its cite-call turn (step #8, `finish=tool_calls`), then closed with **220 bytes**: > Done — that's the full chain above. Short version: it's not one incident, it's the confluence of the Iran war, the Epstein files, and three ex-allies now openly plotting a third-party movement that finally set him off. The user got the 220 bytes and a pointer at a "chain above" that was never posted — the analysis lived only in the transcript. ## Why the existing recovery missed it `finalOutput` already had three shapes for exactly this pathology. The closer matched **none** of them: | classifier | verdict | why | |---|---|---| | `isCitationsOnly` | false | no sources heading | | `isWeakFinal` | false | 220 bytes > the 120-byte weak cap; `backRefRe` has no phrase for "the full chain above" | | `isSummaryCloser` | false | no `Citations are logged.`-style ack to open on | So `finalOutput` returned the terminal verbatim. Reproduced before touching anything: ``` len=220 isWeakFinal=false isCitationsOnly=false isSummaryCloser=false finalOutput passthrough=true ``` ## The fix — one shared signal, not a fourth shape Adding a fourth phrase list is what produced the third one. Two of the three shapes are really **one signal**: the terminal *defers* — it tells us the answer is somewhere the user cannot see. They differ only in whether the terminal also carries content of its own (bare back-reference → discard; back-reference + compression → discard under a strict bar). That signal is now `isBackRef`, shared by both, so a new phrasing is added **once** and is covered in the bare *and* the "+ compression" variant at the same time. Its open-ended half is `aboveRefRe`: a **deictic** `above`, separated from the preposition by what follows the word. - deictic — ends its clause: `that's the full chain above.` · `as shown above,` · a line that just ends in `above` - preposition — always continues into a noun phrase: `above 100°C` · `above the fold` · `above all, …` `pointsAbove` additionally requires the reference inside the terminal's first `backRefHeadChars` (120) bytes: with almost no text before it in *this* message, the reference cannot be pointing at the message's own content, so it must point at a turn the user never saw. That is the same guard as `weakFinalMaxChars`, expressed as an offset — needed because a summary closer carries its compression *after* the pointer and so is not itself short. `isSummaryCloser` now opens on either the citations ack **or** `pointsAbove`. **Recovery is unchanged**: the mandatory dwarf ratio (prior turn ≥ 3×) and the user-message scan boundary still gate it, so a closer only loses to a prior turn in the same user turn that is clearly the fuller original. ## Tests New: `TestPointsAbove` (deictic vs. prepositional split, both ends of the offset bound), deictic cases in `TestIsWeakFinal` / `TestIsSummaryCloser`, three `TestFinalOutput` cases (recover · ratio-too-low keeps the closer · prepositional `above` survives a dwarfing prior turn), and `TestRun_RecoversFrontLoadedAnswerOverAboveRefCloser` end-to-end with the verbatim closer. No existing expectation changed. **Break-checked** — four mutations, each killed by a specific named test, unmutated control survives: | mutation | killed by | |---|---| | `isSummaryCloser` back to citations-ack only | `TestIsSummaryCloser/1611-verbatim`, `TestFinalOutput/above-pointer_closer_discarded…`, `TestRun_RecoversFrontLoadedAnswerOverAboveRefCloser` | | `isWeakFinal` back to the fixed phrase list | `TestIsWeakFinal/bare-above-pointer`, `/above-pointer-in-parens` | | `aboveRefRe` matches any `above` | `TestPointsAbove/above-a-number`, `/above-the-fold`, `/above-all`, `/above-average`, `TestFinalOutput/prepositional_above…` | | `pointsAbove` drops the offset bound | `TestPointsAbove/late-reference-not-a-pointer` | `go build ./...` · `go vet ./...` · `gofmt -l .` empty · `go test ./... -count=1 -race` all green. The mort-side half of #1611 (recorded `cite` rows never reaching the Discord reply) is a separate mort PR.
steve added 1 commit 2026-08-22 03:20:40 +00:00
fix(agent): recover the answer behind a bare "above" pointer
Gadfly review (reusable) / review (pull_request) Failing after 1s
Adversarial Review (Gadfly) / review (pull_request) Failing after 2s
CI / Tidy (pull_request) Successful in 9m28s
CI / Build & Test (pull_request) Successful in 9m54s
bcba9667bd
mort issue #1611: run 8eea3e82 front-loaded a 2,245-char analysis into
its cite-call turn and closed with 220 bytes — "Done — that's the full
chain above. Short version: …". The user got the 220 bytes and a
pointer at a chain that was never posted.

finalOutput already had three shapes for this pathology, and the closer
matched none of them: too long for the weak-final cap (220 > 120), no
citations heading, and no "Citations are logged." ack to open the
summary-closer class. So it was delivered verbatim.

The three shapes were each a separate vocabulary of ack phrases, which
is why a fourth phrasing walked straight through. Two of them are really
one signal — the terminal DEFERS, telling us the answer is somewhere the
user cannot see — differing only in whether the terminal also carries
content of its own. That signal is now isBackRef, shared by both, so a
new phrasing is added once and covered in the bare and the
"+ compression" variant at the same time.

Its open-ended half is aboveRefRe: a DEICTIC "above", separated from the
preposition by what follows the word. The deictic use ends its clause
("that's the full chain above.", "as shown above,"); the preposition
always continues into a noun phrase ("above 100°C", "above the fold",
"above all, …"). pointsAbove additionally requires the reference in the
terminal's first 120 bytes — with almost no text before it in THIS
message, it cannot be pointing at the message's own content.

isSummaryCloser now opens on either the citations ack or pointsAbove.
Recovery is unchanged: the mandatory dwarf ratio and the user-message
scan boundary still gate it, so a closer only loses to a prior turn in
the same user turn that is clearly the fuller original.

Break-checked: reverting either classifier, dropping the clause-final
rule, or dropping the offset bound each kills a named test; the
unmutated control survives.
Author
Owner

@gadfly review

Focus areas:

  • aboveRefRe false positives. The deictic/prepositional split rests entirely on what follows above. Are there clause-final prepositional uses, or deictic uses that continue into a noun phrase, that this gets wrong?
  • The pointsAbove offset bound (120). Is "a reference in the first 120 bytes cannot point at this message's own content" actually sound, or can a terminal put real content before byte 120?
  • Widening isSummaryCloser. It now discards a ≤300-byte terminal whenever a same-turn prior assistant turn is ≥3× longer and the terminal opens with a deictic above. What legitimate final answer fits that profile and would be wrongly thrown away?
  • isBackRef is used by isWeakFinal but isSummaryCloser deliberately calls only pointsAbove. Is that asymmetry justified by the comment, or is it a seam that will drift?
@gadfly review Focus areas: - **`aboveRefRe` false positives.** The deictic/prepositional split rests entirely on what follows `above`. Are there clause-final prepositional uses, or deictic uses that continue into a noun phrase, that this gets wrong? - **The `pointsAbove` offset bound (120).** Is "a reference in the first 120 bytes cannot point at this message's own content" actually sound, or can a terminal put real content before byte 120? - **Widening `isSummaryCloser`.** It now discards a ≤300-byte terminal whenever a same-turn prior assistant turn is ≥3× longer and the terminal opens with a deictic `above`. What legitimate final answer fits that profile and would be wrongly thrown away? - **`isBackRef` is used by `isWeakFinal` but `isSummaryCloser` deliberately calls only `pointsAbove`.** Is that asymmetry justified by the comment, or is it a seam that will drift?
Author
Owner

The first round never ran — the reusable-workflow pin hardcoded a reviewer image tag (sha-b37cd09) that was never published, so run 7056 died in ~1s on failed to resolve reference … not found. Fixed on main in #29 (repin to @8adeeea, which resolves the tag per run). Retrying.

@gadfly review

Focus areas:

  • aboveRefRe false positives. The deictic/prepositional split rests entirely on what follows above. Are there clause-final prepositional uses, or deictic uses that continue into a noun phrase, that this gets wrong?
  • The pointsAbove offset bound (120). Is "a reference in the first 120 bytes cannot point at this message's own content" actually sound, or can a terminal put real content before byte 120?
  • Widening isSummaryCloser. It now discards a ≤300-byte terminal whenever a same-turn prior assistant turn is ≥3× longer and the terminal opens with a deictic above. What legitimate final answer fits that profile and would be wrongly thrown away?
  • isBackRef is used by isWeakFinal but isSummaryCloser deliberately calls only pointsAbove. Is that asymmetry justified by the comment, or is it a seam that will drift?
The first round never ran — the reusable-workflow pin hardcoded a reviewer image tag (`sha-b37cd09`) that was never published, so run 7056 died in ~1s on `failed to resolve reference … not found`. Fixed on main in #29 (repin to `@8adeeea`, which resolves the tag per run). Retrying. @gadfly review Focus areas: - **`aboveRefRe` false positives.** The deictic/prepositional split rests entirely on what follows `above`. Are there clause-final prepositional uses, or deictic uses that continue into a noun phrase, that this gets wrong? - **The `pointsAbove` offset bound (120).** Is "a reference in the first 120 bytes cannot point at this message's own content" actually sound, or can a terminal put real content before byte 120? - **Widening `isSummaryCloser`.** It now discards a ≤300-byte terminal whenever a same-turn prior assistant turn is ≥3× longer and the terminal opens with a deictic `above`. What legitimate final answer fits that profile and would be wrongly thrown away? - **`isBackRef` is used by `isWeakFinal` but `isSummaryCloser` deliberately calls only `pointsAbove`.** Is that asymmetry justified by the comment, or is it a seam that will drift?

🪰 Gadfly — live review status

0/4 reviewers finished · updated 2026-08-22 04:48:19Z

claude-code/opus · claude-code — 4/5 lenses

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

claude-code/sonnet · claude-code — 2/5 lenses

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

glm-5.2:cloud · ollama-cloud — 4/5 lenses

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

kimi-k2.6:cloud · ollama-cloud — 2/5 lenses

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

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 0/4 reviewers finished · updated 2026-08-22 04:48:19Z #### `claude-code/opus` · claude-code — ⏳ 4/5 lenses - ✅ **security** — No material issues found - 🔄 **correctness** — running - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `claude-code/sonnet` · claude-code — ⏳ 2/5 lenses - ✅ **security** — No material issues found - 🔄 **correctness** — running - 🔄 **maintainability** — running - ✅ **performance** — No material issues found - 🔄 **error-handling** — running #### `glm-5.2:cloud` · ollama-cloud — ⏳ 4/5 lenses - ✅ **security** — No material issues found - 🔄 **correctness** — running - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `kimi-k2.6:cloud` · ollama-cloud — ⏳ 2/5 lenses - 🔄 **security** — running - 🔄 **correctness** — running - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - 🔄 **error-handling** — running <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>

🪰 Gadfly review — consensus across 4 models

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

Finding Where Models Lens
🟡 First-person authorial narrative in permanent code comment ('every case I had written') violates project comment conventions agent/finalize.go:205 2/4 maintainability
4 single-model findings (lower confidence)
Finding Where Model Lens
🟠 em dash in clauseBoundaryChars makes start=k+1 land mid-rune, so a bare "Done — … above." closer defeats bareRemainderRe and is not recovered agent/finalize.go:192 claude-code/opus correctness, error-handling
Rationale for not sharing backRefRe with isSummaryCloser is duplicated verbatim in pointsAbove and isSummaryCloser comments agent/finalize.go:149 claude-code/sonnet maintainability
backRefHeadChars is named for the backRef family but is applied only to the deictic aboveRefRe offset test, never to backRefRe agent/finalize.go:316 claude-code/opus maintainability
analysis1611 is a function rather than a var — inconsistent with other test fixtures and recomputes on each call agent/finalize_test.go:76 claude-code/sonnet maintainability
Per-model detail
claude-code/opus (claude-code) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verified against the checked-out code. This change is pure text classification over model-generated transcript text — regex predicates deciding which prior turn's text to surface as the final answer. Assessing it strictly through the Security lens:

VERDICT: No material issues found

  • No ReDoS. The one construct that would concern a security reviewer is the nested quantifier in bareRemainderRe (agent/finalize.go:217): ^( + fillerWords + fillerSep + *)*$ — a classic (x*)* shape that causes catastrophic backtracking in PCRE-style engines. But this is Go's regexp (RE2, imported at finalize.go:4), which guarantees linear-time matching with no backtracking. I confirmed all new patterns (aboveRefRe, bareRemainderRe, compressionMarkerRe) compile under the same RE2 engine, so even fully attacker-controlled model output cannot force super-linear matching. Not exploitable.
  • No new taint sink. The recovered text is a prior assistant turn from the same transcript, returned in place of the terminal turn. It is the same trust level and the same destination (user-facing output) as the text it replaces — no new execution, no shell/SQL/path/template interpolation, no deserialization. bareAbovePointer does only slicing/trimming on the string (finalize.go:186-197); strings.LastIndexAny/strings.Trim are byte-safe against arbitrary UTF-8.
  • No authz/secret/SSRF surface. Nothing here touches credentials, network egress, or access decisions.

Everything I flag-checked resolved to "safe under RE2 / no sink," so I'm not manufacturing a finding. Nothing material in the security lane.

🎯 Correctness — Minor issues

I've confirmed the code matches the draft's claims exactly. Let me trace the core reasoning.

The bug (confirmed):

  • clauseBoundaryChars = ".!?\n,;:—" (line 207) includes the em dash (UTF-8 0xE2 0x80 0x94).
  • strings.LastIndexAny(t[:loc[0]], clauseBoundaryChars) (line 192) returns the byte index of the first byte of a matched rune. When the em dash is the last boundary char, k points at 0xE2.
  • start = k + 1 (line 193) lands on the em dash's second byte, mid-rune.

Tracing bareAbovePointer("Done — the full chain above."):

  • t[:loc[0]] = "Done — the full chain "; the only boundary char is the em dash at byte 5, so k=5, start=6.
  • t[:6] = "Done \xe2" (trailing lone lead byte).
  • rest = "Done \xe2" + " " + "". strings.Trim(rest, pointerResidueCutset)"Done \xe2": the \xe2 decodes as RuneError (U+FFFD), which is not in the cutset (the cutset's is the full 3-byte rune, not a lone byte), so it's not trimmed.
  • bareRemainderRe = ^((done|all set|ok(ay)?)[\s,.!:—-]*)*$"Done" + space matches, but the trailing \xe2/RuneError matches neither \s nor the in the separator class → no match → returns false.

So isWeakFinal returns false and backRefRe also misses ("the full chain above" is not one of its fixed phrases). With a rune-correct offset (start = k + utf8.RuneLen('—') = 8), rest trims to "Done" → matches the done filler → true. The byte math defeats the explicitly-designed em-dash handling (comments at lines 138, 180, 199–206).

I verified the sibling cases too: the comma path ("Ship Tuesday, as shown above.") uses a 1-byte comma and is unaffected; the em-dash-before-a-real-answer case ("Ship Tuesday — as shown above.") is accidentally still correct (the corrupted remainder isn't filler, so it's correctly not-bare). The bug only produces a wrong result — a false negative — when the remainder after an em dash should be recognized as filler. The added test {"bare-pointer-with-filler-opener", "Done. See the chain above.", true} uses a period, so this exact separator is untested and the gate stays green.

Finding is CONFIRMED. No new findings to add.

VERDICT: Minor issues

Finding — bareAbovePointer slices mid-rune on an em-dash clause boundary (agent/finalize.go:193, with clauseBoundaryChars at :207)

clauseBoundaryChars (agent/finalize.go:207) deliberately includes the em dash , a 3-byte UTF-8 rune (0xE2 0x80 0x94), but the clause is cut at a byte offset:

if k := strings.LastIndexAny(t[:loc[0]], clauseBoundaryChars); k >= 0 {
    start = k + 1   // k is the byte index of the em dash's FIRST byte
}
rest := strings.TrimSpace(t[:start]) + " " + strings.TrimSpace(t[loc[1]:])

strings.LastIndexAny returns the byte index of the em dash's first byte (0xE2), so start = k + 1 lands in the middle of the rune and t[:start] ends with a lone 0xE2 continuation byte. That stray byte decodes as RuneError, which is in neither the whitespace set nor pointerResidueCutset, so it survives TrimSpace and strings.Trim, and then fails bareRemainderRe (the filler classes don't match RuneError).

Concrete failure: a short terminal whose pointer clause is introduced by an em-dash filler opener — e.g. "Done — the full chain above.":

  • Not caught by backRefRe ("the full chain above" isn't one of its fixed phrases).
  • bareAbovePointer computes rest = "Done \xe2", which fails bareRemainderRe → returns false.
  • So isWeakFinal returns false, the closer is delivered verbatim, and the front-loaded answer is not recovered.

With a rune-correct offset, rest would be "Done" → matches the done filler → true. The intended behavior — the authors explicitly designed for the em dash to stay (:138) and for a "Done —" throat-clear to read as filler (:180) — is defeated by the byte math. This exact separator is untested: the added bare-pointer-with-filler-opener case uses a period ("Done. See the chain above."), not an em dash, so the gate is green while the case the comments call out is broken.

The em dash works correctly in summaryPreface/compressionMarkerRe because those are rune-aware regexes, and issue #1611's actual 220-byte closer is recovered via isSummaryCloser (which never calls bareAbovePointer), so the headline PR case is unaffected — this is a gap in the short-terminal isWeakFinal path only.

Fix: advance start past the whole rune — decode the rune at k and use start = k + utf8.RuneLen(r) — and add an em-dash-before-separator case to TestIsWeakFinal (e.g. "Done — the full chain above." → true) to pin it.

🧹 Code cleanliness & maintainability — Minor issues

Both findings verify against the actual code:

  • Finding 1 (line 204-206): The clauseBoundaryChars comment does end with the review-history meta-narrative ("That is the same defect… it survived a round of review because every case I had written put the answer AFTER the pointer"). Confirmed verbatim at finalize.go:204-206. Accurate trivial observation.
  • Finding 2 (line 316-324): backRefHeadChars is defined at line 324 and its only production use is finalize.go:163 inside aboveRefLoc, which matches on aboveRefRe (the deictic form) — never on backRefRe. Grep confirms no other reference. Accurate trivial observation.

Both survive. The verdict stands.

VERDICT: Minor issues

The change is well-structured for its lens: aboveRefLoc is correctly shared between pointsAbove and bareAbovePointer (verified at finalize.go:156,187), fillerWords/fillerSep are extracted so summaryPreface and bareRemainderRe can't drift (verified finalize.go:217,264), and backRefHeadChars is defined by reference to weakFinalMaxChars with a test pinning the equality. No dead code — every new symbol (aboveRefRe, pointsAbove, aboveRefLoc, bareAbovePointer, clauseBoundaryChars, pointerResidueCutset, bareRemainderRe, compressionMarkerRe) is reachable. The decision not to fold backRefRe into the shared pointsAbove path is explained and defensible.

Findings:

  • agent/finalize.go:204-206 — meta-narrative comment is noise, not documentation. The clauseBoundaryChars comment ends with "That is the same defect as treating a pointer as disposable because something followed it — mirrored, and it survived a round of review because every case I had written put the answer AFTER the pointer." This documents the author's review history rather than the code's behavior; a future maintainer gets nothing actionable from it and has to parse the self-reference. The preceding sentence ("the answer can share the reference's SENTENCE… cutting back only to the previous full stop swallowed the decision") already fully justifies why commas/semicolons/colons/dashes are in the set. Suggest deleting the trailing "That is the same defect…" clause. Severity: trivial.

  • agent/finalize.go:316-324backRefHeadChars name doesn't match its sole use. The constant is named for the backRef family but is applied only to the deictic aboveRefRe offset test in aboveRefLoc (finalize.go:163); it is never applied to backRefRe. A maintainer tracing backRefRe's bounds won't find this offset near it, and one reading this constant may assume it gates backRefRe. The doc comment does clarify ("a deictic 'above'"), which softens it, and "back-reference" is the umbrella family name, so this is defensible — but aboveRefHeadChars would read truer to the single call site. Severity: trivial; raising as an observation, not a blocker.

Everything else in my lane checks out. The comment volume is high but consistent with the file's established style (issue/run references, // Why:-style rationale), so I'm not flagging it.

Performance — No material issues found

VERDICT: No material issues found

I reviewed this change strictly through the performance lens and verified the call paths against the checked-out code.

Why there's nothing material here:

  • Runs once per agent run, not on a hot path. finalOutput is invoked exactly once, at agent/agent.go:333, only when the loop reaches a terminal turn with no tool calls (if len(resp.ToolCalls) == 0). None of the new predicates (pointsAbove, bareAbovePointer, isSummaryCloser, isWeakFinal) run per loop-iteration. Verified by reading the call site and grepping all callers.

  • All new regexes are compiled once at package init. aboveRefRe, bareRemainderRe, and compressionMarkerRe are package-level var … = regexp.MustCompile(...) (finalize.go:141,217,240) — no per-call compilation.

  • Every regex runs on a bounded, short string. bareAbovePointer is only reached via Go short-circuit under len(t) <= weakFinalMaxChars (120) in isWeakFinal (finalize.go:364); pointsAbove/compressionMarkerRe only run after isSummaryCloser rejects anything > summaryCloserMaxChars (300) at finalize.go:420. So the matchers never scan large payloads.

  • No ReDoS / catastrophic backtracking. bareRemainderRe uses a nested quantifier ^(fillerWords fillerSep*)*$ (finalize.go:217) — the classic exponential shape in a backtracking engine — but Go's regexp is RE2 (linear-time, no backtracking), and the input is capped at 120 bytes regardless. Not a concern.

  • String ops in bareAbovePointer are trivial. A LastIndexAny, two TrimSpace, one concat, and a Trim (finalize.go:191-196) allocate a couple of small (≤120-byte) strings once per run — negligible.

  • lastSubstantiveAssistantText is a single O(#messages) reverse scan with one preambleRe match per assistant turn on trimmed text (finalize.go:433-455); unchanged in complexity and run once. No N+1, no quadratic behavior, no unbounded growth.

Nothing in this diff introduces an efficiency regression worth flagging.

🧯 Error handling & edge cases — Minor issues

I've confirmed the code matches the diff exactly, and independently traced the byte handling. The empirical run was blocked by plan-mode sandboxing, but the manual trace against the verified source is decisive:

For "Done — that's the full chain above." (em dash = U+2014, bytes E2 80 94):

  • aboveRefLoc matches "above."; t[:loc[0]] = "Done — that's the full chain ".
  • The only clauseBoundaryChars rune present is (the ' in "that's" isn't in the set). strings.LastIndexAny returns the start byte of the em dash (documented stdlib behavior), so k points at 0xE2.
  • start = k + 1 lands on the 0x80 continuation byte → t[:start] = "Done \xe2", keeping a stray lead byte.
  • TrimSpace/Trim leave the 0xE2 (it decodes to RuneError, absent from both cutsets), so bareRemainderRe (^((done|all set|ok(ay)?)[\s,.!:—-]*)*$) fails on the RuneErrorfalse.

Correct handling (advancing past the whole rune, start = k + 3) yields "Done —" → trims to "Done" → matches → true. The single-byte boundary variant ("Done, …", "Done. …") works. No test exercises an em-dash boundary before a bare pointer (closer1611 uses one but is 220 bytes, routed to isSummaryCloser, not bareAbovePointer). The bareAbovePointer doc comment explicitly names the "Done —" throat-clearing opener as filler it should strip, so this is squarely in the fix's design scope. Finding confirmed.


VERDICT: Minor issues

agent/finalize.go:192 — em dash in clauseBoundaryChars makes start = k+1 land mid-rune, so a short bare "Done — … above." closer is not recovered.

strings.LastIndexAny returns the byte index of the em dash's first byte (0xE2); start = k + 1 then points at the 0x80 continuation byte, leaving a stray 0xE2 in the t[:start] prefix. Neither TrimSpace nor Trim(pointerResidueCutset) removes it (it decodes to RuneError, which is in neither cutset), so bareRemainderRe sees non-filler junk and returns false.

Trace for bareAbovePointer("Done — that's the full chain above."):

t[:loc0] = "Done — that's the full chain "
k        = byte index of '—' (0xE2)        via LastIndexAny
start    = k+1                              → mid-rune (0x80)
t[:start]= "Done \xe2"                      → stray continuation byte kept
rest     = "Done \xe2"                      → bareRemainderRe fails
⇒ false     (correct handling ⇒ true)

The em dash is the one boundary char the design explicitly wants to cut on for the "Done —" filler opener the comment at finalize.go:180 calls out, yet it is the exact case that breaks. A short (<120 byte) bare em-dash-opener closer therefore defeats bareAbovePointer, isWeakFinal returns false, and finalOutput delivers the bare pointer verbatim instead of recovering the prior answer.

  • Impact: fails safe — it never discards real content, it only misses a recovery it was meant to make; hence minor, not blocking. But it silently drops a case in the fix's stated scope, and no test covers it (closer1611's em dash is 220 bytes and routes to isSummaryCloser, not bareAbovePointer).
  • Fix: advance past the whole rune — _, size := utf8.DecodeRuneInString(t[k:]); start = k + size (add unicode/utf8) — or drop the em dash from clauseBoundaryChars.

Everything else in the diff checks out: aboveRefLoc handles empty/no-match (FindStringIndex → nil → false); the loc[0] > backRefHeadChars bound is correct; the new MustCompile patterns are RE2 (no catastrophic backtracking) and the nested (fillerWords fillerSep*)* can't spin on empty since fillerWords is non-empty; and the mid-rune slice can neither panic (Go slicing is byte-addressable) nor cause an incorrect discard (it only biases bareAbovePointer toward false).

claude-code/sonnet (claude-code) — Minor issues

Verdict: Minor issues · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

The PR adds text-classification logic (pointsAbove, bareAbovePointer, aboveRefRe, compressionMarkerRe) that operates entirely on LLM response strings — selecting which part of the conversation transcript to return to the caller. I traced the untrusted input (model output) through every new code path:

  • Injection / unsafe eval: no SQL, shell, template, or path operations anywhere in the new code; pure string matching and slicing.
  • ReDoS: all patterns use regexp.MustCompile against Go's RE2 engine (linear-time guaranteed). The heaviest new pattern, bareRemainderRe, has a (group*)* structure that would be catastrophic in a backtracking engine but is trivially safe in RE2. Inputs are additionally bounded by weakFinalMaxChars = 120 and summaryCloserMaxChars = 300.
  • Slice panics from untrusted input: aboveRefLoc only returns a non-nil result when the match offset is <= backRefHeadChars (line 163). bareAbovePointer then slices at t[:loc[0]] and t[loc[1]:] — both within the string's bounds by construction. strings.LastIndexAny returns a valid byte index or -1; the k >= 0 guard on line 192 is correct.
  • Credential / secret exposure: nothing in the changed paths touches auth tokens, API keys, or any other credential material.
  • Data flow out: finalOutput returns a plain string; no downstream security-sensitive sink (HTTP client, exec, template) is involved within the scope of these changes.

One observation outside correctness scope: clauseBoundaryChars (line 207) contains (U+2014, 3 UTF-8 bytes). strings.LastIndexAny returns the byte position of that rune's first byte, so start = k + 1 (line 193) can land inside the em-dash sequence and create an invalid-UTF-8 slice. This is a correctness edge case, not a security issue — the invalid bytes cause bareRemainderRe to not match, making bareAbovePointer return false (conservative/safe failure that keeps the terminal rather than discarding it).

🎯 Correctness⚠️ could not complete

⚠️ This reviewer failed to complete: claude -p context deadline exceeded

🧹 Code cleanliness & maintainability — Minor issues

All three findings are confirmed against the actual code. Here is the corrected review:


Minor issues

Three maintainability findings, all small. Nothing blocking.


1. First-person authorial voice in a permanent code commentagent/finalize.go:205-206

// That is the same defect as treating a pointer as disposable because
// something followed it — mirrored, and it survived a round of review because
// every case I had written put the answer AFTER the pointer.

"every case I had written" is a debugging diary entry, not an invariant. The project's own CLAUDE.md is explicit: "Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves." The useful reasoning (commas/semicolons are clause boundaries because an answer can share the pointer's sentence) is already in the lines before this. The "I had written" sentence should be dropped.


2. Identical rationale stated in two placesagent/finalize.go:149-155 and agent/finalize.go:413-417

Both the pointsAbove doc-comment and the isSummaryCloser doc-comment explain, in nearly identical prose, why backRefRe is not shared with isSummaryCloser ("those fixed phrases are matched anywhere in the text, which is only safe under isWeakFinal's 120-byte cap"). When the cap changes or the reasoning evolves, there are two places to keep in sync. One copy should defer to the other with a cross-reference, or the explanation should live in only one of the two.


3. analysis1611() is a function rather than a varagent/finalize_test.go:76-78

func analysis1611() string {
    return strings.TrimSpace(strings.Repeat("The break was the Iran strikes, ...", 12))
}

The comment says "shared by the finalOutput table and the end-to-end Run test so the two cannot drift apart" — the right goal, but the wrong form. closer1611 and b3cb9ee9Closer are const; analysis1611 can't be a const (runtime call), but it should be a var:

var analysis1611 = strings.TrimSpace(strings.Repeat("The break was the Iran strikes, ...", 12))

A function form hides that the value is deterministic and recomputes on every call; a var is Go idiom for a once-initialized test fixture.


Performance — No material issues found

No material issues found

The new code adds regex-based classification logic to a function called at most once per agent terminal turn, on input bounded to ~300 bytes. Running the numbers:

  • All five new/modified regexes (aboveRefRe, bareRemainderRe, compressionMarkerRe, updated summaryCloserRe, and backRefRe) are compiled once at package init via var … = regexp.MustCompile(…) — no per-call compilation cost.
  • aboveRefLoc can be called twice per finalOutput invocation when isWeakFinal falls through to isSummaryCloser (once via bareAbovePointer, once via pointsAbove), but on ≤300-byte strings this is two fast RE2 searches — not measurable.
  • Go's regexp package uses RE2 semantics (linear-time guaranteed), so the (fillerWords fillerSep*)* construction in bareRemainderRe cannot catastrophically backtrack.
  • finalOutput is not a hot loop — it fires once per terminal turn, not per token or per message in the history.

Nothing in this PR introduces N+1 patterns, unnecessary allocations, unbounded growth, or work in a hot path. The implementation is proportionate to the task.

🧯 Error handling & edge cases⚠️ could not complete

⚠️ This reviewer failed to complete: claude -p context deadline exceeded

glm-5.2:cloud (ollama-cloud) — No material issues found

Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

I reviewed this change through the security lens (authn/authz, injection, SSRF, unsafe deserialization, secret leakage, input validation, untrusted-data handling) and verified the actual code paths.

What I checked and why it's clean for this lens:

  • Tainted-input surface. The new predicates (pointsAbove, aboveAbovePointer, aboveRefLoc, compressionMarkerRe matching) only ever run on the model's own terminal-turn text via finalOutputisWeakFinal/isSummaryCloser (agent/finalize.go:56-83, 359-425). There is no user-controlled string, path, SQL, shell, URL, or serialized blob flowing into any of these. The terminal text is llm.Message text produced by the model — not an attacker-controlled channel. So the injection/SSRF/path-traversal axes don't apply.

  • Secret/credential leakage via recovery. The new behavior can discard a closer and surface a prior assistant turn instead. I traced lastSubstantiveAssistantText (agent/finalize.go:431-457) and isSubstantiveAnswer (:475-487): the recovered turn is always an assistant message already addressed to the user in the same conversation, just front-loaded into an earlier tool-call turn. Surfacing it is the explicit design intent and exposes no secret not already destined for the user — not a regression in leakage posture.

  • ReDoS / DoS on untrusted-ish text. Since the inputs are model outputs (which can be long/malformed), I checked the new regexes for catastrophic backtracking:

    • aboveRefRe (:141): (?i)\babove\b[ \t]*([...]) — linear, no nested quantifiers.
    • compressionMarkerRe (:240): alternation with bounded \s* — linear.
    • bareRemainderRe (:217): (?i)^((done|all set|ok(ay)?)[\s,.!:—-]*)*$ — has nested quantifiers, so I verified the ambiguity is impossible: fillerSep is [\s,.!:—-] (whitespace/punctuation only), none of which can begin a fillerWord (which all start with letters d/a/o). Partitioning is therefore deterministic — no exponential backtracking. Safe.

I did not find any authz, injection, deserialization, or secret-handling defect introduced by this diff.

🎯 Correctness — No material issues found

Verdict: No material issues found

I reviewed the change through the correctness lens, re-deriving the constants, byte thresholds, and regex behavior from first principles and checking them against the checked-out code (agent/finalize.go and agent/finalize_test.go).

Verified items:

  • aboveRefRe / offset bound (finalize.go:141,163). Re-derived the boundary test arithmetic: strings.Repeat("x",119)+" above." puts above at index 120 (loc[0]==120, allowed since the check is > backRefHeadChars), and Repeat("x",120)+" above." puts it at 121 (rejected). Matches the table's expected true/false. The hyphen exclusion is correct — \b holds between e and -, so a literal - in the class would have made above-average deictic; excluding it makes those cases fail as intended.
  • bareAbovePointer clause cutting (finalize.go:186-197). Traced "Ship Tuesday, as shown above.": LastIndexAny finds the comma, start lands after it, remainder becomes "Ship Tuesday" → not filler → not bare → isWeakFinal=false, preserving the answer. Traced "That's the full chain above.": no boundary → start=0, remainder empty → matches bareRemainderRe → bare. Both match the test expectations.
  • Additive, no regression (finalize.go:364,423-424). isWeakFinal is backRefRe.MatchString(t) || bareAbovePointer(t) and isSummaryCloser is summaryCloserRe.MatchString(t) || (pointsAbove && compressionMarker). Both are strict OR-additions over the existing predicates, so no previously-discarded/kept classification flips the wrong way. Cases the old backRefRe caught (e.g. "as noted above") still return true via backRefRe.
  • Dwarf-ratio arithmetic for the 1611 tests. Recomputed byte lengths by hand: closer1611 = 220 bytes (the adds 2 vs its rune count; I verified the rune count is 218, giving 220). analysis1611() = 56 bytes/unit × 12 − 1 trailing space = 671 bytes. recoverRatio*len(terminal) = 3*220 = 660, and 671 ≥ 660dwarfs=true ⇒ recovery returns the analysis (matches want: analysis). The "kept when prior does not dwarf" test uses longAnswer ≈ 275 bytes < 660 ⇒ dwarfs=false ⇒ closer kept (matches want: closer1611). Both consistent.
  • compressionMarkerRe sentence-anchor (finalize.go:240). Traced "Given the analysis above, the bottom line is …": bottom line is preceded by "the ", not by ^|[.!?:;—]\s*|\n\s*, so no match ⇒ isSummaryCloser=false (matches expectation). "That's the chain above: in short, …": : + space precedes in short ⇒ match ⇒ true (matches).
  • modeSummary scan boundary (finalize.go:435-444). The new pointsAbove-based opener still routes through modeSummary, which stops at the most recent user message and requires the 3× dwarf ratio — so the new opener inherits the same guard the PR description claims. No unbounded-backwards regression.

One trivial observation (not a production correctness bug): the above-pointer closer discarded… test's dwarf margin is ~11 bytes (671 vs 660). It passes and the literals are intentional per the test comments, but resizing the repeated phrase without re-checking the ratio could silently flip it. This is a test-maintenance fragility, not a defect in finalize.go.

No blocking or minor correctness issues found in the production change.

🧹 Code cleanliness & maintainability — No material issues found

Reviewed agent/finalize.go (lines 118–425) and the test additions against the actual checked-out code. I traced each new symbol's usage, the shared fillerWords/fillerSep dedup, the aboveRefLoc primitive, and the character-class constants.

Verdict: No material issues found

The change is, from a cleanliness standpoint, unusually careful: it introduces a single shared primitive (aboveRefLoc) used by both pointsAbove and bareAbovePointer precisely so they cannot disagree, explicitly de-duplicates fillerWords/fillerSep across bareRemainderRe and summaryPreface (with a comment on why the quantifier differs), and defines backRefHeadChars = weakFinalMaxChars by reference rather than repeating the literal. Every new symbol is used; there is no dead code.

One trivial, non-blocking observation (low confidence that it is even worth acting on):

  • agent/finalize.go:207,212,263 — there are now three closely-related punctuation character-class string literals that partially overlap in membership but serve different roles: clauseBoundaryChars (.!?\n,;:—), pointerResidueCutset (a trim set), and fillerSep ([\s,.!:—-]). They are conceptually distinct (clause termination vs. edge-trimming vs. inter-token separation), so unifying them would be wrong, but a reader has to hold three "which punctuation is in which set" maps in their head, and the sets differ in subtle ways (e.g. fillerSep includes the ASCII hyphen and \s; clauseBoundaryChars includes \n explicitly but not \s). Not worth changing — flagging only for completeness; the per-constant comments already justify each membership.

I verified by reading finalize.go lines 118–425 and grepping for all uses of the new symbols; each resolves to a real definition and call site.

Performance — No material issues found

Verdict: No material issues found

  • Reviewed agent/finalize.go:56-240 through the performance lens. finalOutput runs once per agent loop termination (a terminal, tool-free turn), not on a hot path; none of the new predicates are called in loops or per-message.
  • All new regexes (aboveRefRe, compressionMarkerRe, bareRemainderRe) are package-level regexp.MustCompile vars (compile-once), not recompiled per call.
  • The only redundant work is aboveRefRe.FindStringIndex potentially running twice on the same terminal string (once via isWeakFinalbareAbovePointeraboveRefLoc, once via isSummaryCloserpointsAboveaboveRefLoc). But finalOutput's switch short-circuits: isWeakFinal is evaluated before isSummaryCloser, and when isWeakFinal is true the closer branch is skipped. The double-run only occurs in the narrow case where isWeakFinal is false and isSummaryCloser is true, over a string capped at summaryCloserMaxChars (300 bytes). Cost is negligible — not a material regression.
  • bareAbovePointer's strings.LastIndexAny/TrimSpace/Trim operate on ≤120-byte terminals (gated by weakFinalMaxChars). No quadratic or unbounded behavior.
  • No N+1, no per-iteration allocations, no missing pagination. Nothing material in the performance lane.
🧯 Error handling & edge cases — No material issues found

I verified the core logic by reading agent/finalize.go lines 100-487 and tracing the new functions through boundary inputs (empty string, loc[0]==0, loc[1]==len(t), CRLF, end-of-string, hyphenated compounds, mid-sentence markers). All unhappy paths are handled.

VERDICT: No material issues found

  • Empty/zero-length inputs: pointsAbove("")aboveRefRe.FindStringIndex returns nilaboveRefLoc returns nil → safe. bareAbovePointer("") returns false at the loc == nil guard. isSummaryCloser("") / isWeakFinal("") short-circuit on t == "". Verified at finalize.go:156-167, 186-197, 359-365, 418-425.
  • Slice-bounds / nil-deref: FindStringIndex is the only source of loc; the loc == nil check precedes every loc[0]/loc[1] use. t[:loc[0]] is safe even when loc[0]==0 (yields ""); t[loc[1]:] is safe even when loc[1]==len(t) (yields ""). strings.LastIndexAny("", clauseBoundaryChars) returns -1, handled by start = 0. Verified finalize.go:161-167, 191-194.
  • No integer/overflow concerns: backRefHeadChars/weakFinalMaxChars are small const int; the only arithmetic is loc[0] > backRefHeadChars and recoverRatio*len(terminal) in pre-existing code. No new overflow surface.
  • Boundary/off-by-one: The offset bound is loc[0] > backRefHeadChars (i.e. <= allowed), and the test suite pins exactly index 120 (allowed) vs 121 (rejected) with literal lengths, so the fence is correct and tested. Verified against TestPointsAbove/reference-exactly-at-the-bound and reference-one-past-the-bound.
  • Regex terminator class edge cases: hyphen deliberately excluded from aboveRefRe's class (placed nowhere; - not in [.,;:!?)\]"'…—]), so above-average/above-board don't match deictic; $ without (?m) in Go matches end-of-text, but line-final above is covered via the explicit \r/\n alternatives, so CRLF/trailing-newline cases don't silently drop. pointerResidueCutset and fillerSep both place - at the end of their character classes (literal, no accidental range). Verified finalize.go:141, 207, 212, 263.
  • Compression-marker-without-pointer: isSummaryCloser requires BOTH pointsAbove(t) AND compressionMarkerRe.MatchString(t), so a bare Short version: no. (no pointer) is not misclassified — fails closed as documented. Verified finalize.go:423-424.

No swallowed errors, missing cleanup, or panics-on-bad-input in the diff. The new code is purely string-classification with nil-safe regex results and bounded slicing.

kimi-k2.6:cloud (ollama-cloud) — No material issues found

Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

From the 🔒 Security lens, the PR introduces no material security issues. All regex patterns are compiled from static, attacker-uncontrolled strings at package init time, so there is no regex-injection vector. Go's regexp package uses the RE2 engine, which guarantees linear-time matching and rules out ReDoS. The recovery logic preserves its existing scan boundary: modeSummary stops at the first user-role message (preventing cross-question information leakage), and the new pointsAbove check is offset-bounded to the same 120-byte guard as weakFinalMaxChars, maintaining the invariant that a classified back-reference must point outside the current terminal message. No secrets, credentials, or unsafe deserialization are involved.

🎯 Correctness — No material issues found

Verdict: No material issues found

The change introduces a carefully-scoped deictic/prepositional distinction for "above" and wires it into the existing recovery pipeline. I scrutinized the regexes, the clause-cutting logic, the offset bound, the interaction with compressionMarkerRe, and the end-to-end test paths. The reasoning in comments and tests matches the implementation.

Specifically verified:

  • aboveRefRe correctly excludes the ASCII hyphen so hyphenated compounds (above-average, above-mentioned) are not misread as deictic, while clause-final uses (above., above,, above\n) do match.
  • bareAbovePointer cuts the pointer’s clause (bounded by clauseBoundaryChars) and tests the remainder against bareRemainderRe; mixed terminals (Ship Tuesday, as shown above.) correctly survive.
  • pointsAbove is offset-bounded to backRefHeadChars (120), anchoring the reference to the terminal’s opening so it cannot be pointing at content inside the same long message.
  • isSummaryCloser requires both pointsAbove and compressionMarkerRe; a pointer followed by a new conclusion (I recommend option B…) correctly fails because there is no compression marker.
  • finalOutput ordering preserves the existing precedence: citations-only wins, then weak/back-ref, then summary closer, so a terminal matching both ack and back-ref still uses the lower back-ref recovery bar.
  • The dwarf ratio (≥3×) still gates modeSummary recovery; the #1611 end-to-end test uses a 672-byte front-loaded answer vs. a 220-byte closer, clearing the bar.

No logic bugs or semantic misclassifications were found.

🧹 Code cleanliness & maintainability — No material issues found

Verdict: No material issues found

I scrutinized the PR through the maintainability lens — readability, structure, deduplication, naming, function size, and consistency with surrounding patterns. After reading the implementation (agent/finalize.go) and tests (agent/finalize_test.go), I found the change to be well-structured:

  • Good deduplication: fillerWords and fillerSep are shared between bareRemainderRe and summaryPreface, preventing the two classifier regexes from drifting. backRefHeadChars is defined as weakFinalMaxChars rather than a repeated literal, keeping the two guards coupled.
  • Clear separation of concerns: pointsAbove, bareAbovePointer, aboveRefLoc, and compressionMarkerRe each have a single, documented responsibility. The isSummaryCloser / isWeakFinal split is explained at length and the tests pin the boundary behavior.
  • Reasonable function size: No new function exceeds ~10 lines; bareAbovePointer and aboveRefLoc are compact and focused.
  • Consistent naming and patterns: The new identifiers follow the existing *Re / *Chars / *Cutset conventions already used in the file. Table-driven tests continue the established style.

The extensive inline documentation is atypical in volume, but that is a stylistic choice rather than a maintainability defect, and the comments do explain why each regex is shaped the way it is — which matters for code that is essentially a curated list of model heuristics.

Performance — No material issues found

Verdict: No material issues found

From the Performance lens, the new regexes (aboveRefRe, bareRemainderRe, compressionMarkerRe) are compiled once at package init and matched only during finalOutput, which runs a single time at the end of an agent run on strings bounded to ≤300 bytes. The added string slicing/trimming in bareAbovePointer operates on strings capped at weakFinalMaxChars (120 bytes). Scanning in lastSubstantiveAssistantText is linear in message count with no nested loops. No hot-path work, no unbounded growth, no quadratic behavior, and no allocations that would be material at these sizes.

🧯 Error handling & edge cases — No material issues found

Verdict: No material issues found

After walking through finalize.go and its new aboveRefRe/pointsAbove/bareAbovePointer machinery with the error-handling lens, the code is notably careful about the edge cases it introduces:

  • nil/empty safety: aboveRefLoc correctly short-circuits on loc == nil; isWeakFinal and isSummaryCloser both guard on t == "" before calling the new predicates.
  • offset bound is strict and tested: backRefHeadChars is exactly weakFinalMaxChars (120), and the test table explicitly pins both the constant equality and the literal-length boundary cases (119 + " above." → true, 120 + " above." → false). No off-by-one drift.
  • clause cutting preserves preceding answer content: strings.LastIndexAny(t[:loc[0]], clauseBoundaryChars) finds the nearest boundary before the pointer, so a decision that shares the pointer’s sentence ("Ship Tuesday, as shown above.") is correctly kept. The test suite covers this explicitly.
  • remainder check is conservative: bareRemainderRe only accepts empty/filler remainders after cutting the pointer clause, so mixed terminals ("That's the chain above. Ship Tuesday.") survive. Verified against the test table.
  • hyphen vs. em-dash handling is deliberate: ASCII - is excluded from aboveRefRe’s terminator set (preventing "above-average" from reading as deictic) while stays; covered by the hyphenated-compound test cases.
  • deictic vs. prepositional split is enforced: aboveRefRe only matches when "above" is followed by clause-final punctuation or line-end, so prepositional continuations ("above 100 degrees", "above the fold", "above all") are rejected. Verified by TestPointsAbove.
  • no swallowed errors or missing cleanup: the new functions are pure predicates with no resource acquisition; no defer or rollback is needed.

The only potential worry I chased was whether bareAbovePointer could misfire when the pointer sentence carries real content before it with no clause boundary (e.g., "The answer is above. Done."). In that scenario the entire sentence is the pointer clause; the text before the pointer is part of that clause and is correctly treated as pointer text, not independent answer content. The behavior is consistent with the design goal of treating a bare pointer sentence as disposable.

No material issues found in the error-handling & edge-cases lane.

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** · 5 findings (1 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟡 | First-person authorial narrative in permanent code comment ('every case I had written') violates project comment conventions | `agent/finalize.go:205` | 2/4 | maintainability | <details><summary>4 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | em dash in clauseBoundaryChars makes start=k+1 land mid-rune, so a bare "Done — … above." closer defeats bareRemainderRe and is not recovered | `agent/finalize.go:192` | claude-code/opus | correctness, error-handling | | ⚪ | Rationale for not sharing backRefRe with isSummaryCloser is duplicated verbatim in pointsAbove and isSummaryCloser comments | `agent/finalize.go:149` | claude-code/sonnet | maintainability | | ⚪ | backRefHeadChars is named for the backRef family but is applied only to the deictic aboveRefRe offset test, never to backRefRe | `agent/finalize.go:316` | claude-code/opus | maintainability | | ⚪ | analysis1611 is a function rather than a var — inconsistent with other test fixtures and recomputes on each call | `agent/finalize_test.go:76` | claude-code/sonnet | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/opus</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Verified against the checked-out code. This change is pure text classification over model-generated transcript text — regex predicates deciding which prior turn's text to surface as the final answer. Assessing it strictly through the Security lens: **VERDICT: No material issues found** - **No ReDoS.** The one construct that would concern a security reviewer is the nested quantifier in `bareRemainderRe` (`agent/finalize.go:217`): `^(` + fillerWords + fillerSep + `*)*$` — a classic `(x*)*` shape that causes catastrophic backtracking in PCRE-style engines. But this is Go's `regexp` (RE2, imported at `finalize.go:4`), which guarantees linear-time matching with no backtracking. I confirmed all new patterns (`aboveRefRe`, `bareRemainderRe`, `compressionMarkerRe`) compile under the same RE2 engine, so even fully attacker-controlled model output cannot force super-linear matching. Not exploitable. - **No new taint sink.** The recovered text is a *prior assistant turn* from the same transcript, returned in place of the terminal turn. It is the same trust level and the same destination (user-facing output) as the text it replaces — no new execution, no shell/SQL/path/template interpolation, no deserialization. `bareAbovePointer` does only slicing/trimming on the string (`finalize.go:186-197`); `strings.LastIndexAny`/`strings.Trim` are byte-safe against arbitrary UTF-8. - **No authz/secret/SSRF surface.** Nothing here touches credentials, network egress, or access decisions. Everything I flag-checked resolved to "safe under RE2 / no sink," so I'm not manufacturing a finding. Nothing material in the security lane. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> I've confirmed the code matches the draft's claims exactly. Let me trace the core reasoning. **The bug (confirmed):** - `clauseBoundaryChars = ".!?\n,;:—"` (line 207) includes the em dash `—` (UTF-8 `0xE2 0x80 0x94`). - `strings.LastIndexAny(t[:loc[0]], clauseBoundaryChars)` (line 192) returns the byte index of the **first byte** of a matched rune. When the em dash is the last boundary char, `k` points at `0xE2`. - `start = k + 1` (line 193) lands on the em dash's second byte, mid-rune. Tracing `bareAbovePointer("Done — the full chain above.")`: - `t[:loc[0]]` = `"Done — the full chain "`; the only boundary char is the em dash at byte 5, so `k=5`, `start=6`. - `t[:6]` = `"Done \xe2"` (trailing lone lead byte). - `rest` = `"Done \xe2"` + `" "` + `""`. `strings.Trim(rest, pointerResidueCutset)` → `"Done \xe2"`: the `\xe2` decodes as `RuneError` (U+FFFD), which is not in the cutset (the cutset's `—` is the full 3-byte rune, not a lone byte), so it's not trimmed. - `bareRemainderRe` = `^((done|all set|ok(ay)?)[\s,.!:—-]*)*$` — `"Done"` + space matches, but the trailing `\xe2`/RuneError matches neither `\s` nor the `—` in the separator class → **no match → returns false**. So `isWeakFinal` returns false and `backRefRe` also misses ("the full chain above" is not one of its fixed phrases). With a rune-correct offset (`start = k + utf8.RuneLen('—') = 8`), `rest` trims to `"Done"` → matches the `done` filler → true. The byte math defeats the explicitly-designed em-dash handling (comments at lines 138, 180, 199–206). I verified the sibling cases too: the comma path (`"Ship Tuesday, as shown above."`) uses a 1-byte comma and is unaffected; the em-dash-before-a-real-answer case (`"Ship Tuesday — as shown above."`) is *accidentally* still correct (the corrupted remainder isn't filler, so it's correctly not-bare). The bug only produces a wrong result — a false negative — when the remainder after an em dash *should* be recognized as filler. The added test `{"bare-pointer-with-filler-opener", "Done. See the chain above.", true}` uses a period, so this exact separator is untested and the gate stays green. Finding is CONFIRMED. No new findings to add. ## VERDICT: Minor issues ### Finding — `bareAbovePointer` slices mid-rune on an em-dash clause boundary (`agent/finalize.go:193`, with `clauseBoundaryChars` at `:207`) `clauseBoundaryChars` (`agent/finalize.go:207`) deliberately includes the em dash `—`, a 3-byte UTF-8 rune (`0xE2 0x80 0x94`), but the clause is cut at a **byte** offset: ```go if k := strings.LastIndexAny(t[:loc[0]], clauseBoundaryChars); k >= 0 { start = k + 1 // k is the byte index of the em dash's FIRST byte } rest := strings.TrimSpace(t[:start]) + " " + strings.TrimSpace(t[loc[1]:]) ``` `strings.LastIndexAny` returns the byte index of the em dash's first byte (`0xE2`), so `start = k + 1` lands in the middle of the rune and `t[:start]` ends with a lone `0xE2` continuation byte. That stray byte decodes as `RuneError`, which is in neither the whitespace set nor `pointerResidueCutset`, so it survives `TrimSpace` and `strings.Trim`, and then fails `bareRemainderRe` (the filler classes don't match `RuneError`). **Concrete failure:** a short terminal whose pointer clause is introduced by an em-dash filler opener — e.g. `"Done — the full chain above."`: - Not caught by `backRefRe` ("the full chain above" isn't one of its fixed phrases). - `bareAbovePointer` computes `rest = "Done \xe2"`, which fails `bareRemainderRe` → returns **false**. - So `isWeakFinal` returns **false**, the closer is delivered verbatim, and the front-loaded answer is **not** recovered. With a rune-correct offset, `rest` would be `"Done"` → matches the `done` filler → **true**. The intended behavior — the authors explicitly designed for the em dash to stay (`:138`) and for a `"Done —"` throat-clear to read as filler (`:180`) — is defeated by the byte math. This exact separator is untested: the added `bare-pointer-with-filler-opener` case uses a period (`"Done. See the chain above."`), not an em dash, so the gate is green while the case the comments call out is broken. The em dash works correctly in `summaryPreface`/`compressionMarkerRe` because those are rune-aware regexes, and issue #1611's actual 220-byte closer is recovered via `isSummaryCloser` (which never calls `bareAbovePointer`), so the headline PR case is unaffected — this is a gap in the short-terminal `isWeakFinal` path only. **Fix:** advance `start` past the whole rune — decode the rune at `k` and use `start = k + utf8.RuneLen(r)` — and add an em-dash-before-separator case to `TestIsWeakFinal` (e.g. `"Done — the full chain above." → true`) to pin it. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Both findings verify against the actual code: - **Finding 1** (line 204-206): The `clauseBoundaryChars` comment does end with the review-history meta-narrative ("That is the same defect… it survived a round of review because every case I had written put the answer AFTER the pointer"). Confirmed verbatim at `finalize.go:204-206`. Accurate trivial observation. - **Finding 2** (line 316-324): `backRefHeadChars` is defined at line 324 and its only production use is `finalize.go:163` inside `aboveRefLoc`, which matches on `aboveRefRe` (the deictic form) — never on `backRefRe`. Grep confirms no other reference. Accurate trivial observation. Both survive. The verdict stands. ## VERDICT: Minor issues The change is well-structured for its lens: `aboveRefLoc` is correctly shared between `pointsAbove` and `bareAbovePointer` (verified at `finalize.go:156,187`), `fillerWords`/`fillerSep` are extracted so `summaryPreface` and `bareRemainderRe` can't drift (verified `finalize.go:217,264`), and `backRefHeadChars` is defined by reference to `weakFinalMaxChars` with a test pinning the equality. No dead code — every new symbol (`aboveRefRe`, `pointsAbove`, `aboveRefLoc`, `bareAbovePointer`, `clauseBoundaryChars`, `pointerResidueCutset`, `bareRemainderRe`, `compressionMarkerRe`) is reachable. The decision *not* to fold `backRefRe` into the shared `pointsAbove` path is explained and defensible. Findings: - **`agent/finalize.go:204-206` — meta-narrative comment is noise, not documentation.** The `clauseBoundaryChars` comment ends with "That is the same defect as treating a pointer as disposable because something followed it — mirrored, and it survived a round of review because every case I had written put the answer AFTER the pointer." This documents the author's review history rather than the code's behavior; a future maintainer gets nothing actionable from it and has to parse the self-reference. The preceding sentence ("the answer can share the reference's SENTENCE… cutting back only to the previous full stop swallowed the decision") already fully justifies why commas/semicolons/colons/dashes are in the set. Suggest deleting the trailing "That is the same defect…" clause. Severity: trivial. - **`agent/finalize.go:316-324` — `backRefHeadChars` name doesn't match its sole use.** The constant is named for the `backRef` family but is applied *only* to the deictic `aboveRefRe` offset test in `aboveRefLoc` (`finalize.go:163`); it is never applied to `backRefRe`. A maintainer tracing `backRefRe`'s bounds won't find this offset near it, and one reading this constant may assume it gates `backRefRe`. The doc comment does clarify ("a deictic 'above'"), which softens it, and "back-reference" is the umbrella family name, so this is defensible — but `aboveRefHeadChars` would read truer to the single call site. Severity: trivial; raising as an observation, not a blocker. Everything else in my lane checks out. The comment volume is high but consistent with the file's established style (issue/run references, `// Why:`-style rationale), so I'm not flagging it. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> ## VERDICT: No material issues found I reviewed this change strictly through the **performance** lens and verified the call paths against the checked-out code. **Why there's nothing material here:** - **Runs once per agent run, not on a hot path.** `finalOutput` is invoked exactly once, at `agent/agent.go:333`, only when the loop reaches a terminal turn with no tool calls (`if len(resp.ToolCalls) == 0`). None of the new predicates (`pointsAbove`, `bareAbovePointer`, `isSummaryCloser`, `isWeakFinal`) run per loop-iteration. Verified by reading the call site and grepping all callers. - **All new regexes are compiled once at package init.** `aboveRefRe`, `bareRemainderRe`, and `compressionMarkerRe` are package-level `var … = regexp.MustCompile(...)` (`finalize.go:141,217,240`) — no per-call compilation. - **Every regex runs on a bounded, short string.** `bareAbovePointer` is only reached via Go short-circuit under `len(t) <= weakFinalMaxChars` (120) in `isWeakFinal` (`finalize.go:364`); `pointsAbove`/`compressionMarkerRe` only run after `isSummaryCloser` rejects anything `> summaryCloserMaxChars` (300) at `finalize.go:420`. So the matchers never scan large payloads. - **No ReDoS / catastrophic backtracking.** `bareRemainderRe` uses a nested quantifier `^(fillerWords fillerSep*)*$` (`finalize.go:217`) — the classic exponential shape in a backtracking engine — but Go's `regexp` is RE2 (linear-time, no backtracking), and the input is capped at 120 bytes regardless. Not a concern. - **String ops in `bareAbovePointer` are trivial.** A `LastIndexAny`, two `TrimSpace`, one concat, and a `Trim` (`finalize.go:191-196`) allocate a couple of small (≤120-byte) strings once per run — negligible. - **`lastSubstantiveAssistantText`** is a single O(#messages) reverse scan with one `preambleRe` match per assistant turn on trimmed text (`finalize.go:433-455`); unchanged in complexity and run once. No N+1, no quadratic behavior, no unbounded growth. Nothing in this diff introduces an efficiency regression worth flagging. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> I've confirmed the code matches the diff exactly, and independently traced the byte handling. The empirical run was blocked by plan-mode sandboxing, but the manual trace against the verified source is decisive: For `"Done — that's the full chain above."` (em dash = U+2014, bytes `E2 80 94`): - `aboveRefLoc` matches `"above."`; `t[:loc[0]]` = `"Done — that's the full chain "`. - The only `clauseBoundaryChars` rune present is `—` (the `'` in "that's" isn't in the set). `strings.LastIndexAny` returns the **start byte** of the em dash (documented stdlib behavior), so `k` points at `0xE2`. - `start = k + 1` lands on the `0x80` continuation byte → `t[:start]` = `"Done \xe2"`, keeping a stray lead byte. - `TrimSpace`/`Trim` leave the `0xE2` (it decodes to `RuneError`, absent from both cutsets), so `bareRemainderRe` (`^((done|all set|ok(ay)?)[\s,.!:—-]*)*$`) fails on the `RuneError` → `false`. Correct handling (advancing past the whole rune, `start = k + 3`) yields `"Done —"` → trims to `"Done"` → matches → `true`. The single-byte boundary variant (`"Done, …"`, `"Done. …"`) works. No test exercises an em-dash boundary before a *bare* pointer (`closer1611` uses one but is 220 bytes, routed to `isSummaryCloser`, not `bareAbovePointer`). The `bareAbovePointer` doc comment explicitly names the `"Done —"` throat-clearing opener as filler it should strip, so this is squarely in the fix's design scope. Finding confirmed. --- ## VERDICT: Minor issues **`agent/finalize.go:192` — em dash in `clauseBoundaryChars` makes `start = k+1` land mid-rune, so a short bare "Done — … above." closer is not recovered.** `strings.LastIndexAny` returns the byte index of the em dash's first byte (`0xE2`); `start = k + 1` then points at the `0x80` continuation byte, leaving a stray `0xE2` in the `t[:start]` prefix. Neither `TrimSpace` nor `Trim(pointerResidueCutset)` removes it (it decodes to `RuneError`, which is in neither cutset), so `bareRemainderRe` sees non-filler junk and returns `false`. Trace for `bareAbovePointer("Done — that's the full chain above.")`: ``` t[:loc0] = "Done — that's the full chain " k = byte index of '—' (0xE2) via LastIndexAny start = k+1 → mid-rune (0x80) t[:start]= "Done \xe2" → stray continuation byte kept rest = "Done \xe2" → bareRemainderRe fails ⇒ false (correct handling ⇒ true) ``` The em dash is the one boundary char the design explicitly wants to cut on for the `"Done —"` filler opener the comment at `finalize.go:180` calls out, yet it is the exact case that breaks. A short (<120 byte) bare em-dash-opener closer therefore defeats `bareAbovePointer`, `isWeakFinal` returns `false`, and `finalOutput` delivers the bare pointer verbatim instead of recovering the prior answer. - Impact: **fails safe** — it never discards real content, it only *misses* a recovery it was meant to make; hence minor, not blocking. But it silently drops a case in the fix's stated scope, and no test covers it (`closer1611`'s em dash is 220 bytes and routes to `isSummaryCloser`, not `bareAbovePointer`). - Fix: advance past the whole rune — `_, size := utf8.DecodeRuneInString(t[k:]); start = k + size` (add `unicode/utf8`) — or drop the em dash from `clauseBoundaryChars`. Everything else in the diff checks out: `aboveRefLoc` handles empty/no-match (`FindStringIndex` → nil → false); the `loc[0] > backRefHeadChars` bound is correct; the new `MustCompile` patterns are RE2 (no catastrophic backtracking) and the nested `(fillerWords fillerSep*)*` can't spin on empty since `fillerWords` is non-empty; and the mid-rune slice can neither panic (Go slicing is byte-addressable) nor cause an incorrect discard (it only biases `bareAbovePointer` toward `false`). </details> </details> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues · ⚠️ 2/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** The PR adds text-classification logic (`pointsAbove`, `bareAbovePointer`, `aboveRefRe`, `compressionMarkerRe`) that operates entirely on LLM response strings — selecting which part of the conversation transcript to return to the caller. I traced the untrusted input (model output) through every new code path: - **Injection / unsafe eval**: no SQL, shell, template, or path operations anywhere in the new code; pure string matching and slicing. - **ReDoS**: all patterns use `regexp.MustCompile` against Go's RE2 engine (linear-time guaranteed). The heaviest new pattern, `bareRemainderRe`, has a `(group*)*` structure that would be catastrophic in a backtracking engine but is trivially safe in RE2. Inputs are additionally bounded by `weakFinalMaxChars = 120` and `summaryCloserMaxChars = 300`. - **Slice panics from untrusted input**: `aboveRefLoc` only returns a non-nil result when the match offset is `<= backRefHeadChars` (line 163). `bareAbovePointer` then slices at `t[:loc[0]]` and `t[loc[1]:]` — both within the string's bounds by construction. `strings.LastIndexAny` returns a valid byte index or -1; the `k >= 0` guard on line 192 is correct. - **Credential / secret exposure**: nothing in the changed paths touches auth tokens, API keys, or any other credential material. - **Data flow out**: `finalOutput` returns a plain string; no downstream security-sensitive sink (HTTP client, exec, template) is involved within the scope of these changes. One observation outside correctness scope: `clauseBoundaryChars` (line 207) contains `—` (U+2014, 3 UTF-8 bytes). `strings.LastIndexAny` returns the byte position of that rune's first byte, so `start = k + 1` (line 193) can land inside the em-dash sequence and create an invalid-UTF-8 slice. This is a correctness edge case, not a security issue — the invalid bytes cause `bareRemainderRe` to not match, making `bareAbovePointer` return `false` (conservative/safe failure that keeps the terminal rather than discarding it). </details> <details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: claude -p context deadline exceeded </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All three findings are confirmed against the actual code. Here is the corrected review: --- **Minor issues** Three maintainability findings, all small. Nothing blocking. --- **1. First-person authorial voice in a permanent code comment** — `agent/finalize.go:205-206` ```go // That is the same defect as treating a pointer as disposable because // something followed it — mirrored, and it survived a round of review because // every case I had written put the answer AFTER the pointer. ``` "every case I had written" is a debugging diary entry, not an invariant. The project's own `CLAUDE.md` is explicit: *"Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves."* The useful reasoning (commas/semicolons are clause boundaries because an answer can share the pointer's sentence) is already in the lines before this. The "I had written" sentence should be dropped. --- **2. Identical rationale stated in two places** — `agent/finalize.go:149-155` and `agent/finalize.go:413-417` Both the `pointsAbove` doc-comment and the `isSummaryCloser` doc-comment explain, in nearly identical prose, why `backRefRe` is not shared with `isSummaryCloser` ("those fixed phrases are matched anywhere in the text, which is only safe under isWeakFinal's 120-byte cap"). When the cap changes or the reasoning evolves, there are two places to keep in sync. One copy should defer to the other with a cross-reference, or the explanation should live in only one of the two. --- **3. `analysis1611()` is a function rather than a `var`** — `agent/finalize_test.go:76-78` ```go func analysis1611() string { return strings.TrimSpace(strings.Repeat("The break was the Iran strikes, ...", 12)) } ``` The comment says "shared by the finalOutput table and the end-to-end Run test so the two cannot drift apart" — the right goal, but the wrong form. `closer1611` and `b3cb9ee9Closer` are `const`; `analysis1611` can't be a const (runtime call), but it should be a `var`: ```go var analysis1611 = strings.TrimSpace(strings.Repeat("The break was the Iran strikes, ...", 12)) ``` A function form hides that the value is deterministic and recomputes on every call; a `var` is Go idiom for a once-initialized test fixture. --- </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** The new code adds regex-based classification logic to a function called at most once per agent terminal turn, on input bounded to ~300 bytes. Running the numbers: - All five new/modified regexes (`aboveRefRe`, `bareRemainderRe`, `compressionMarkerRe`, updated `summaryCloserRe`, and `backRefRe`) are compiled **once at package init** via `var … = regexp.MustCompile(…)` — no per-call compilation cost. - `aboveRefLoc` can be called twice per `finalOutput` invocation when `isWeakFinal` falls through to `isSummaryCloser` (once via `bareAbovePointer`, once via `pointsAbove`), but on ≤300-byte strings this is two fast RE2 searches — not measurable. - Go's regexp package uses RE2 semantics (linear-time guaranteed), so the `(fillerWords fillerSep*)*` construction in `bareRemainderRe` cannot catastrophically backtrack. - `finalOutput` is not a hot loop — it fires once per terminal turn, not per token or per message in the history. Nothing in this PR introduces N+1 patterns, unnecessary allocations, unbounded growth, or work in a hot path. The implementation is proportionate to the task. </details> <details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: claude -p context deadline exceeded </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> No material issues found I reviewed this change through the security lens (authn/authz, injection, SSRF, unsafe deserialization, secret leakage, input validation, untrusted-data handling) and verified the actual code paths. What I checked and why it's clean for this lens: - **Tainted-input surface.** The new predicates (`pointsAbove`, `aboveAbovePointer`, `aboveRefLoc`, `compressionMarkerRe` matching) only ever run on the model's own terminal-turn text via `finalOutput` → `isWeakFinal`/`isSummaryCloser` (`agent/finalize.go:56-83`, `359-425`). There is no user-controlled string, path, SQL, shell, URL, or serialized blob flowing into any of these. The terminal text is `llm.Message` text produced by the model — not an attacker-controlled channel. So the injection/SSRF/path-traversal axes don't apply. - **Secret/credential leakage via recovery.** The new behavior can discard a closer and surface a *prior* assistant turn instead. I traced `lastSubstantiveAssistantText` (`agent/finalize.go:431-457`) and `isSubstantiveAnswer` (`:475-487`): the recovered turn is always an assistant message already addressed to the user in the same conversation, just front-loaded into an earlier tool-call turn. Surfacing it is the explicit design intent and exposes no secret not already destined for the user — not a regression in leakage posture. - **ReDoS / DoS on untrusted-ish text.** Since the inputs are model outputs (which can be long/malformed), I checked the new regexes for catastrophic backtracking: - `aboveRefRe` (`:141`): `(?i)\babove\b[ \t]*([...])` — linear, no nested quantifiers. - `compressionMarkerRe` (`:240`): alternation with bounded `\s*` — linear. - `bareRemainderRe` (`:217`): `(?i)^((done|all set|ok(ay)?)[\s,.!:—-]*)*$` — has nested quantifiers, so I verified the ambiguity is impossible: `fillerSep` is `[\s,.!:—-]` (whitespace/punctuation only), none of which can begin a `fillerWord` (which all start with letters d/a/o). Partitioning is therefore deterministic — no exponential backtracking. Safe. I did not find any authz, injection, deserialization, or secret-handling defect introduced by this diff. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **Verdict: No material issues found** I reviewed the change through the correctness lens, re-deriving the constants, byte thresholds, and regex behavior from first principles and checking them against the checked-out code (`agent/finalize.go` and `agent/finalize_test.go`). Verified items: - **`aboveRefRe` / offset bound (`finalize.go:141,163`).** Re-derived the boundary test arithmetic: `strings.Repeat("x",119)+" above."` puts `above` at index 120 (`loc[0]==120`, allowed since the check is `> backRefHeadChars`), and `Repeat("x",120)+" above."` puts it at 121 (rejected). Matches the table's expected `true`/`false`. The hyphen exclusion is correct — `\b` holds between `e` and `-`, so a literal `-` in the class would have made `above-average` deictic; excluding it makes those cases fail as intended. - **`bareAbovePointer` clause cutting (`finalize.go:186-197`).** Traced `"Ship Tuesday, as shown above."`: `LastIndexAny` finds the comma, `start` lands after it, remainder becomes `"Ship Tuesday"` → not filler → not bare → `isWeakFinal=false`, preserving the answer. Traced `"That's the full chain above."`: no boundary → `start=0`, remainder empty → matches `bareRemainderRe` → bare. Both match the test expectations. - **Additive, no regression (`finalize.go:364,423-424`).** `isWeakFinal` is `backRefRe.MatchString(t) || bareAbovePointer(t)` and `isSummaryCloser` is `summaryCloserRe.MatchString(t) || (pointsAbove && compressionMarker)`. Both are strict OR-additions over the existing predicates, so no previously-discarded/kept classification flips the wrong way. Cases the old `backRefRe` caught (e.g. `"as noted above"`) still return `true` via `backRefRe`. - **Dwarf-ratio arithmetic for the 1611 tests.** Recomputed byte lengths by hand: `closer1611` = 220 bytes (the `—` adds 2 vs its rune count; I verified the rune count is 218, giving 220). `analysis1611()` = 56 bytes/unit × 12 − 1 trailing space = 671 bytes. `recoverRatio*len(terminal) = 3*220 = 660`, and `671 ≥ 660` ⇒ `dwarfs=true` ⇒ recovery returns the analysis (matches `want: analysis`). The "kept when prior does not dwarf" test uses `longAnswer` ≈ 275 bytes < 660 ⇒ `dwarfs=false` ⇒ closer kept (matches `want: closer1611`). Both consistent. - **`compressionMarkerRe` sentence-anchor (`finalize.go:240`).** Traced `"Given the analysis above, the bottom line is …"`: `bottom line` is preceded by `"the "`, not by `^|[.!?:;—]\s*|\n\s*`, so no match ⇒ `isSummaryCloser=false` (matches expectation). `"That's the chain above: in short, …"`: `:` + space precedes `in short` ⇒ match ⇒ true (matches). - **`modeSummary` scan boundary (`finalize.go:435-444`).** The new `pointsAbove`-based opener still routes through `modeSummary`, which stops at the most recent user message and requires the 3× dwarf ratio — so the new opener inherits the same guard the PR description claims. No unbounded-backwards regression. One trivial observation (not a production correctness bug): the `above-pointer closer discarded…` test's dwarf margin is ~11 bytes (671 vs 660). It passes and the literals are intentional per the test comments, but resizing the repeated phrase without re-checking the ratio could silently flip it. This is a test-maintenance fragility, not a defect in `finalize.go`. No blocking or minor correctness issues found in the production change. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> Reviewed `agent/finalize.go` (lines 118–425) and the test additions against the actual checked-out code. I traced each new symbol's usage, the shared `fillerWords`/`fillerSep` dedup, the `aboveRefLoc` primitive, and the character-class constants. ## Verdict: No material issues found The change is, from a cleanliness standpoint, unusually careful: it introduces a single shared primitive (`aboveRefLoc`) used by both `pointsAbove` and `bareAbovePointer` precisely so they cannot disagree, explicitly de-duplicates `fillerWords`/`fillerSep` across `bareRemainderRe` and `summaryPreface` (with a comment on why the quantifier differs), and defines `backRefHeadChars = weakFinalMaxChars` by reference rather than repeating the literal. Every new symbol is used; there is no dead code. One trivial, non-blocking observation (low confidence that it is even worth acting on): - `agent/finalize.go:207,212,263` — there are now three closely-related punctuation character-class string literals that partially overlap in membership but serve different roles: `clauseBoundaryChars` (`.!?\n,;:—`), `pointerResidueCutset` (a trim set), and `fillerSep` (`[\s,.!:—-]`). They are conceptually distinct (clause termination vs. edge-trimming vs. inter-token separation), so unifying them would be wrong, but a reader has to hold three "which punctuation is in which set" maps in their head, and the sets differ in subtle ways (e.g. `fillerSep` includes the ASCII hyphen and `\s`; `clauseBoundaryChars` includes `\n` explicitly but not `\s`). Not worth changing — flagging only for completeness; the per-constant comments already justify each membership. I verified by reading `finalize.go` lines 118–425 and grepping for all uses of the new symbols; each resolves to a real definition and call site. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> Verdict: No material issues found - Reviewed `agent/finalize.go:56-240` through the performance lens. `finalOutput` runs once per agent loop termination (a terminal, tool-free turn), not on a hot path; none of the new predicates are called in loops or per-message. - All new regexes (`aboveRefRe`, `compressionMarkerRe`, `bareRemainderRe`) are package-level `regexp.MustCompile` vars (compile-once), not recompiled per call. - The only redundant work is `aboveRefRe.FindStringIndex` potentially running twice on the same terminal string (once via `isWeakFinal`→`bareAbovePointer`→`aboveRefLoc`, once via `isSummaryCloser`→`pointsAbove`→`aboveRefLoc`). But `finalOutput`'s `switch` short-circuits: `isWeakFinal` is evaluated before `isSummaryCloser`, and when `isWeakFinal` is true the closer branch is skipped. The double-run only occurs in the narrow case where `isWeakFinal` is false and `isSummaryCloser` is true, over a string capped at `summaryCloserMaxChars` (300 bytes). Cost is negligible — not a material regression. - `bareAbovePointer`'s `strings.LastIndexAny`/`TrimSpace`/`Trim` operate on ≤120-byte terminals (gated by `weakFinalMaxChars`). No quadratic or unbounded behavior. - No N+1, no per-iteration allocations, no missing pagination. Nothing material in the performance lane. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> I verified the core logic by reading `agent/finalize.go` lines 100-487 and tracing the new functions through boundary inputs (empty string, `loc[0]==0`, `loc[1]==len(t)`, CRLF, end-of-string, hyphenated compounds, mid-sentence markers). All unhappy paths are handled. **VERDICT: No material issues found** - **Empty/zero-length inputs**: `pointsAbove("")` → `aboveRefRe.FindStringIndex` returns `nil` → `aboveRefLoc` returns `nil` → safe. `bareAbovePointer("")` returns false at the `loc == nil` guard. `isSummaryCloser("")` / `isWeakFinal("")` short-circuit on `t == ""`. Verified at finalize.go:156-167, 186-197, 359-365, 418-425. - **Slice-bounds / nil-deref**: `FindStringIndex` is the only source of `loc`; the `loc == nil` check precedes every `loc[0]`/`loc[1]` use. `t[:loc[0]]` is safe even when `loc[0]==0` (yields `""`); `t[loc[1]:]` is safe even when `loc[1]==len(t)` (yields `""`). `strings.LastIndexAny("", clauseBoundaryChars)` returns `-1`, handled by `start = 0`. Verified finalize.go:161-167, 191-194. - **No integer/overflow concerns**: `backRefHeadChars`/`weakFinalMaxChars` are small `const int`; the only arithmetic is `loc[0] > backRefHeadChars` and `recoverRatio*len(terminal)` in pre-existing code. No new overflow surface. - **Boundary/off-by-one**: The offset bound is `loc[0] > backRefHeadChars` (i.e. `<=` allowed), and the test suite pins exactly index 120 (allowed) vs 121 (rejected) with literal lengths, so the fence is correct and tested. Verified against `TestPointsAbove/reference-exactly-at-the-bound` and `reference-one-past-the-bound`. - **Regex terminator class edge cases**: hyphen deliberately excluded from `aboveRefRe`'s class (placed nowhere; `-` not in `[.,;:!?)\]"'…—]`), so `above-average`/`above-board` don't match deictic; `$` without `(?m)` in Go matches end-of-text, but line-final `above` is covered via the explicit `\r`/`\n` alternatives, so CRLF/trailing-newline cases don't silently drop. `pointerResidueCutset` and `fillerSep` both place `-` at the end of their character classes (literal, no accidental range). Verified finalize.go:141, 207, 212, 263. - **Compression-marker-without-pointer**: `isSummaryCloser` requires BOTH `pointsAbove(t)` AND `compressionMarkerRe.MatchString(t)`, so a bare `Short version: no.` (no pointer) is not misclassified — fails closed as documented. Verified finalize.go:423-424. No swallowed errors, missing cleanup, or panics-on-bad-input in the diff. The new code is purely string-classification with nil-safe regex results and bounded slicing. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **Verdict: No material issues found** From the 🔒 Security lens, the PR introduces no material security issues. All regex patterns are compiled from static, attacker-uncontrolled strings at package init time, so there is no regex-injection vector. Go's `regexp` package uses the RE2 engine, which guarantees linear-time matching and rules out ReDoS. The recovery logic preserves its existing scan boundary: `modeSummary` stops at the first user-role message (preventing cross-question information leakage), and the new `pointsAbove` check is offset-bounded to the same 120-byte guard as `weakFinalMaxChars`, maintaining the invariant that a classified back-reference must point outside the current terminal message. No secrets, credentials, or unsafe deserialization are involved. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **Verdict:** No material issues found The change introduces a carefully-scoped deictic/prepositional distinction for `"above"` and wires it into the existing recovery pipeline. I scrutinized the regexes, the clause-cutting logic, the offset bound, the interaction with `compressionMarkerRe`, and the end-to-end test paths. The reasoning in comments and tests matches the implementation. Specifically verified: - `aboveRefRe` correctly excludes the ASCII hyphen so hyphenated compounds (`above-average`, `above-mentioned`) are not misread as deictic, while clause-final uses (`above.`, `above,`, `above\n`) do match. - `bareAbovePointer` cuts the pointer’s clause (bounded by `clauseBoundaryChars`) and tests the remainder against `bareRemainderRe`; mixed terminals (`Ship Tuesday, as shown above.`) correctly survive. - `pointsAbove` is offset-bounded to `backRefHeadChars` (120), anchoring the reference to the terminal’s opening so it cannot be pointing at content inside the same long message. - `isSummaryCloser` requires both `pointsAbove` and `compressionMarkerRe`; a pointer followed by a *new conclusion* (`I recommend option B…`) correctly fails because there is no compression marker. - `finalOutput` ordering preserves the existing precedence: citations-only wins, then weak/back-ref, then summary closer, so a terminal matching both ack and back-ref still uses the lower back-ref recovery bar. - The dwarf ratio (`≥3×`) still gates `modeSummary` recovery; the `#1611` end-to-end test uses a 672-byte front-loaded answer vs. a 220-byte closer, clearing the bar. No logic bugs or semantic misclassifications were found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> **Verdict:** No material issues found I scrutinized the PR through the maintainability lens — readability, structure, deduplication, naming, function size, and consistency with surrounding patterns. After reading the implementation (`agent/finalize.go`) and tests (`agent/finalize_test.go`), I found the change to be well-structured: - **Good deduplication:** `fillerWords` and `fillerSep` are shared between `bareRemainderRe` and `summaryPreface`, preventing the two classifier regexes from drifting. `backRefHeadChars` is defined as `weakFinalMaxChars` rather than a repeated literal, keeping the two guards coupled. - **Clear separation of concerns:** `pointsAbove`, `bareAbovePointer`, `aboveRefLoc`, and `compressionMarkerRe` each have a single, documented responsibility. The `isSummaryCloser` / `isWeakFinal` split is explained at length and the tests pin the boundary behavior. - **Reasonable function size:** No new function exceeds ~10 lines; `bareAbovePointer` and `aboveRefLoc` are compact and focused. - **Consistent naming and patterns:** The new identifiers follow the existing `*Re` / `*Chars` / `*Cutset` conventions already used in the file. Table-driven tests continue the established style. The extensive inline documentation is atypical in volume, but that is a stylistic choice rather than a maintainability defect, and the comments do explain *why* each regex is shaped the way it is — which matters for code that is essentially a curated list of model heuristics. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **Verdict:** No material issues found From the Performance lens, the new regexes (`aboveRefRe`, `bareRemainderRe`, `compressionMarkerRe`) are compiled once at package init and matched only during `finalOutput`, which runs a single time at the end of an agent run on strings bounded to ≤300 bytes. The added string slicing/trimming in `bareAbovePointer` operates on strings capped at `weakFinalMaxChars` (120 bytes). Scanning in `lastSubstantiveAssistantText` is linear in message count with no nested loops. No hot-path work, no unbounded growth, no quadratic behavior, and no allocations that would be material at these sizes. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **Verdict:** No material issues found After walking through `finalize.go` and its new `aboveRefRe`/`pointsAbove`/`bareAbovePointer` machinery with the error-handling lens, the code is notably careful about the edge cases it introduces: - **nil/empty safety:** `aboveRefLoc` correctly short-circuits on `loc == nil`; `isWeakFinal` and `isSummaryCloser` both guard on `t == ""` before calling the new predicates. - **offset bound is strict and tested:** `backRefHeadChars` is exactly `weakFinalMaxChars` (120), and the test table explicitly pins both the constant equality and the literal-length boundary cases (119 + " above." → true, 120 + " above." → false). No off-by-one drift. - **clause cutting preserves preceding answer content:** `strings.LastIndexAny(t[:loc[0]], clauseBoundaryChars)` finds the nearest boundary before the pointer, so a decision that shares the pointer’s sentence (`"Ship Tuesday, as shown above."`) is correctly kept. The test suite covers this explicitly. - **remainder check is conservative:** `bareRemainderRe` only accepts empty/filler remainders after cutting the pointer clause, so mixed terminals (`"That's the chain above. Ship Tuesday."`) survive. Verified against the test table. - **hyphen vs. em-dash handling is deliberate:** ASCII `-` is excluded from `aboveRefRe`’s terminator set (preventing `"above-average"` from reading as deictic) while `—` stays; covered by the hyphenated-compound test cases. - **deictic vs. prepositional split is enforced:** `aboveRefRe` only matches when `"above"` is followed by clause-final punctuation or line-end, so prepositional continuations (`"above 100 degrees"`, `"above the fold"`, `"above all"`) are rejected. Verified by `TestPointsAbove`. - **no swallowed errors or missing cleanup:** the new functions are pure predicates with no resource acquisition; no `defer` or rollback is needed. The only potential worry I chased was whether `bareAbovePointer` could misfire when the pointer sentence carries real content before it with no clause boundary (e.g., `"The answer is above. Done."`). In that scenario the entire sentence is the pointer clause; the text before the pointer is part of that clause and is correctly treated as pointer text, not independent answer content. The behavior is consistent with the design goal of treating a bare pointer sentence as disposable. No material issues found in the error-handling & edge-cases lane. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-08-22 03:41:54 +00:00
fix(agent): gadfly round 1 — the hyphen made "above-board" a back-reference
CI / Build & Test (pull_request) Successful in 10m43s
CI / Tidy (pull_request) Successful in 9m25s
f97c2b78c2
Three of four reviewers independently found the same defect: `\b` holds
between "above" and "-", so the literal hyphen in aboveRefRe's terminator
class made every hyphenated compound clause-final. "above-average",
"above-board", "above-ground" all read as deictic pointers, putting a
legitimate short closer at risk of being discarded. My own test used the
SPACE-separated "above average" — it tested the neighbour, not the named
path, which is exactly why the class survived a round of review. Hyphen
dropped from the class; the four compounds are now table cases.

Two more findings, both real:

- backRefHeadChars and weakFinalMaxChars were two unlinked 120 literals
  that the comment called "the same guard". Now defined by reference,
  with a test pinning the identity.
- isBackRef claimed to be the shared extension point for both shapes but
  had a single caller — isSummaryCloser deliberately uses pointsAbove
  directly, because backRefRe's fixed phrases match ANYWHERE and are only
  safe under the 120-byte weak cap. Folding the two together for
  tidiness would widen the gate, not deduplicate it. Helper deleted, the
  doc moved onto pointsAbove where the real sharing is, and it now says
  why backRefRe is not shared.

Also shared the front-loaded-analysis fixture between the table and the
end-to-end test (kimi), and de-hollowed the offset-bound cases: they were
sized as backRefHeadChars±n, so they moved with the constant they were
meant to pin — a break-check that widened the bound to 100000 sailed
through. Literal lengths now, plus an explicit identity assertion.

Break-check: six mutations, each killed by a named test; control survives.
steve added 1 commit 2026-08-22 03:44:59 +00:00
test(agent): gadfly round 1b — pin the exact bound and the bar asymmetry
CI / Build & Test (pull_request) Successful in 10m49s
CI / Tidy (pull_request) Successful in 9m23s
9534e442fb
Sonnet's three findings, all on the tests rather than the rule:

- The "at the bound" case sat at index 115 (and 101 after the first
  round), never at 120 itself, so <= vs < was never exercised. Both sides
  of the boundary are now pinned — index 120 accepted, 121 rejected — and
  an off-by-one mutation is killed by the new case.
- A SHORT (<=120-byte) deictic closer takes the modeBackRef bar, not the
  summary closer's mandatory dwarf ratio, even though it can carry a
  scrap of answer content. That asymmetry is deliberate and pre-existing
  (a "see above" closer has always had it): inside 120 bytes there is no
  room for both a pointer and a real answer. Pinned with a case sized so
  the two bars actually DISAGREE — 3x98 = 294 > the 275-byte prior turn,
  so the summary bar would reject what the back-ref bar accepts. The
  first draft of this fixture was 83 bytes and passed under either bar,
  proving nothing.
- The isBackRef doc finding was sonnet's fourth independent report of it;
  already fixed in the previous commit.

Break-check now runs eight mutations, including the off-by-one and a
swap of the classifier ordering; each is killed by a named test and the
control survives.
Author
Owner

Round 1: 10 findings, all 10 graded real, all 10 addressed in f97c2b7 + 9534e44. Nothing dismissed.

The one that mattered — found independently by three models on three different lenses — is a genuine bug I shipped: \b holds between above and -, so the literal hyphen in aboveRefRe's terminator class made every hyphenated compound clause-final. Reproduced before fixing:

pointsAbove("Turnout was above-average in three counties.") = true
pointsAbove("The deal was above-board from the start.")     = true

My own table had {"above-average", "Turnout was above average in three counties.", false} — the space-separated form. It tested the neighbour, not the named path, which is exactly why the defect survived my break-check. Hyphen dropped; four compounds are now regression cases.

The rest:

finding resolution
two unlinked 120 literals the comment called "the same guard" backRefHeadChars = weakFinalMaxChars, plus a test pinning the identity
isBackRef doc claims a shared extension point, has one caller (×4 models) helper deleted, not given a second caller — see below
duplicated analysis1611 fixture extracted to one function
reference-at-the-bound tests 115, not 120 both sides of the exact bound pinned with literal lengths
short deictic closer takes the back-ref bar, not the dwarf ratio deliberate + pre-existing; now pinned by a test

On isBackRef: the tempting fix is to give it its second caller. That would be wrong. backRefRe's fixed phrases match anywhere in the text, which is only safe under isWeakFinal's 120-byte cap; handing them to isSummaryCloser would let an unanchored phrase fire across 300 bytes — widening the gate, not deduplicating it. The genuinely shared half is pointsAbove (offset-bounded, so it stays anchored), which both classifiers call. Helper deleted, doc moved onto pointsAbove, and it now states why backRefRe is excluded.

Two of your findings also exposed hollow tests of my own, which is the more valuable half:

  • reference-at-the-bound was written as backRefHeadChars-6, so it moved with the constant it was meant to pin. A mutation widening the bound to 100000 sailed straight through it. Literal lengths now.
  • My first shortAbovePointer fixture was 83 bytes — 3×83 = 249 < 275, so it passed under either bar and proved nothing about the asymmetry it was named for. Resized to 98 bytes (3×98 = 294 > 275) so the two bars genuinely disagree.

Break-check is now 8 mutations — including the off-by-one (<=<) and a swap of the classifier ordering — each killed by a specific named test, unmutated control survives.

@gadfly review

Focus areas for this round:

  • The narrowed aboveRefRe. With the ASCII hyphen gone, is any legitimate deictic form now missed in a way that matters — and is the em dash still safe to keep in the class?
  • backRefHeadChars = weakFinalMaxChars. Is tying them genuinely one rule, or have I coupled two things that should be able to move independently?
  • The new bound cases use literal 119/120 plus a weakFinalMaxChars != 120 guard. Does that pin the behaviour, or just make a future retune noisy?
  • Anything hollow left in the tests. Two fixtures this round turned out to prove nothing; assume there are more.
Round 1: **10 findings, all 10 graded real, all 10 addressed** in `f97c2b7` + `9534e44`. Nothing dismissed. The one that mattered — found independently by **three** models on three different lenses — is a genuine bug I shipped: `\b` holds between `above` and `-`, so the literal hyphen in `aboveRefRe`'s terminator class made every hyphenated compound clause-final. Reproduced before fixing: ``` pointsAbove("Turnout was above-average in three counties.") = true pointsAbove("The deal was above-board from the start.") = true ``` My own table had `{"above-average", "Turnout was above average in three counties.", false}` — the **space-separated** form. It tested the neighbour, not the named path, which is exactly why the defect survived my break-check. Hyphen dropped; four compounds are now regression cases. The rest: | finding | resolution | |---|---| | two unlinked `120` literals the comment called "the same guard" | `backRefHeadChars = weakFinalMaxChars`, plus a test pinning the identity | | `isBackRef` doc claims a shared extension point, has one caller (×4 models) | helper **deleted**, not given a second caller — see below | | duplicated `analysis1611` fixture | extracted to one function | | `reference-at-the-bound` tests 115, not 120 | both sides of the exact bound pinned with literal lengths | | short deictic closer takes the back-ref bar, not the dwarf ratio | deliberate + pre-existing; now pinned by a test | On `isBackRef`: the tempting fix is to give it its second caller. That would be wrong. `backRefRe`'s fixed phrases match **anywhere** in the text, which is only safe under `isWeakFinal`'s 120-byte cap; handing them to `isSummaryCloser` would let an unanchored phrase fire across 300 bytes — widening the gate, not deduplicating it. The genuinely shared half is `pointsAbove` (offset-bounded, so it stays anchored), which both classifiers call. Helper deleted, doc moved onto `pointsAbove`, and it now states why `backRefRe` is excluded. Two of your findings also exposed **hollow tests of my own**, which is the more valuable half: - `reference-at-the-bound` was written as `backRefHeadChars-6`, so it moved with the constant it was meant to pin. A mutation widening the bound to `100000` sailed straight through it. Literal lengths now. - My first `shortAbovePointer` fixture was 83 bytes — `3×83 = 249 < 275`, so it passed under *either* bar and proved nothing about the asymmetry it was named for. Resized to 98 bytes (`3×98 = 294 > 275`) so the two bars genuinely disagree. Break-check is now 8 mutations — including the off-by-one (`<=` → `<`) and a swap of the classifier ordering — each killed by a specific named test, unmutated control survives. @gadfly review Focus areas for this round: - **The narrowed `aboveRefRe`.** With the ASCII hyphen gone, is any *legitimate* deictic form now missed in a way that matters — and is the em dash still safe to keep in the class? - **`backRefHeadChars = weakFinalMaxChars`.** Is tying them genuinely one rule, or have I coupled two things that should be able to move independently? - **The new bound cases use literal 119/120 plus a `weakFinalMaxChars != 120` guard.** Does that pin the behaviour, or just make a future retune noisy? - **Anything hollow left in the tests.** Two fixtures this round turned out to prove nothing; assume there are more.
steve added 1 commit 2026-08-22 03:52:31 +00:00
docs(agent): drop the dangling isBackRef reference
CI / Build & Test (pull_request) Successful in 9m49s
CI / Tidy (pull_request) Successful in 9m26s
7dccb233ad
Round 2 (glm): the isSummaryCloser comment still explained itself in
terms of "the whole isBackRef class" — a symbol deleted in f97c2b7. It
named the right distinction with the wrong noun. Reworded to name
backRefRe directly; no isBackRef occurrences remain in the package.
steve added 1 commit 2026-08-22 04:03:22 +00:00
fix(agent): gadfly round 2 — a pointer is not a compression
CI / Build & Test (pull_request) Successful in 9m47s
CI / Tidy (pull_request) Successful in 9m22s
3f1c016e74
Four findings, all real.

**A deictic closer now needs a compression marker too** (opus,
correctness). The pointsAbove branch of isSummaryCloser required only a
back-reference, so a terminal like "Given the analysis above, I
recommend option B because X" — a pointer followed by a CONCLUSION the
earlier turn never contained — would be discarded in favour of that
turn, throwing away the answer. The ack shape does not have this problem
because "Citations are logged." carries nothing; a bare pointer does not
carry that guarantee.

Both halves are now required: the pointer says the full answer is
elsewhere, and compressionMarkerRe ("Short version:", "TL;DR", "In
short") is the model saying what sits beside it is a condensation rather
than new reasoning. #1611's closer has both. A marker without a pointer
stays out of scope for the reason already documented — a user who asked
for brevity gets exactly that shape. Unmatched closers keep today's
behaviour, so the narrowing fails closed.

**CRLF** (opus, error-handling): "above\r\n" was not clause-final, so a
CRLF transcript quietly lost every line-final deictic. CR joins LF in the
terminator set.

**A comment wrapped mid-phrase** so that "// -style" read as a list
marker (sonnet) — reflowed as part of rewriting that doc block.

**Process provenance in a test comment** (sonnet): "(gadfly, 3 models)"
is an execution log, not an invariant. The lesson survives, the
attribution does not.

Break-check is ten mutations now — dropping CR and dropping the
compression requirement are each killed by their own named cases — with
the control surviving.
Author
Owner

Round 2: 4 findings, all 4 real, all 4 fixed in 3f1c016.

The one that changes the design

A pointer is not a compression (opus, correctness). The pointsAbove branch of isSummaryCloser required only a back-reference, which is not enough. This shape

Given the analysis above, I recommend option B because X.

points backwards and then states a conclusion the earlier turn never contained — and the rule as written would have discarded it in favour of that turn, throwing away the actual answer. The citations-ack shape does not have this failure mode because Citations are logged. carries nothing of its own; a bare deictic pointer carries no such guarantee. I had weighed this hazard and under-weighted it.

Both halves are now required: the pointer says the full answer is elsewhere, and compressionMarkerRe (Short version:, TL;DR, In short, Bottom line) is the model saying that what sits beside the pointer is a condensation rather than new reasoning. #1611's closer has both. A marker without a pointer stays out of scope for the reason already documented — a user who asked for brevity is answered with exactly that shape. Unmatched closers keep today's behaviour, so the narrowing fails closed.

Three "pointer + new conclusion" cases (recommendation, decision, new caveat) and an end-to-end finalOutput case now pin it, and dropping the marker requirement is break-check mutation M10.

The rest

finding resolution
CRLF: above\r\n was not clause-final (opus) CR joins LF in the terminator set — two CRLF cases + mutation M9. It fails closed as you say, but silently skipping recovery is the exact failure this change exists to stop
comment wrapped so // -style reads as a list marker (sonnet) reflowed while rewriting that block
(gadfly, 3 models) provenance in a test comment (sonnet) dropped — that is an execution log, and this project's doc policy says a hard-won lesson becomes an invariant, not a diary entry. The lesson (the space-separated cases are a different character, and one passed while the other was broken) stays

Break-check is now 10 mutations, each killed by a specific named test, unmutated control survives.

Running total across both rounds: 14 findings, 14 graded real, 14 fixed. Nothing dismissed. Two of them exposed hollow tests of mine rather than product bugs, which is the half I would not have found alone.

@gadfly review

Focus areas for this round:

  • compressionMarkerRe. Is the marker list missing a phrasing an open-weight model actually uses to introduce a TL;DR — and can any of the listed phrases appear mid-answer in a way that now makes a legitimate closer disposable?
  • The && I just added. Requiring both halves narrows recovery. Is there a real front-loading shape — the thing this PR exists to fix — that no longer recovers because the model pointed backwards without announcing a summary?
  • The terminator set after CR. Anything else that ends a clause in model output and is still missing: a form feed, a unicode line separator, a right-single-quote as an apostrophe?
  • Hollow tests. Three of my fixtures across these rounds have turned out to prove nothing. Assume the count is not three.
Round 2: **4 findings, all 4 real, all 4 fixed** in `3f1c016`. ### The one that changes the design **A pointer is not a compression** (opus, correctness). The `pointsAbove` branch of `isSummaryCloser` required only a back-reference, which is not enough. This shape > Given the analysis above, I recommend option B because X. points backwards and then states a **conclusion the earlier turn never contained** — and the rule as written would have discarded it in favour of that turn, throwing away the actual answer. The citations-ack shape does not have this failure mode because `Citations are logged.` carries nothing of its own; a bare deictic pointer carries no such guarantee. I had weighed this hazard and under-weighted it. Both halves are now required: the pointer says the full answer is elsewhere, and `compressionMarkerRe` (`Short version:`, `TL;DR`, `In short`, `Bottom line`) is the model saying that what sits beside the pointer is a **condensation rather than new reasoning**. #1611's closer has both. A marker without a pointer stays out of scope for the reason already documented — a user who asked for brevity is answered with exactly that shape. Unmatched closers keep today's behaviour, so the narrowing fails closed. Three "pointer + new conclusion" cases (recommendation, decision, new caveat) and an end-to-end `finalOutput` case now pin it, and dropping the marker requirement is break-check mutation M10. ### The rest | finding | resolution | |---|---| | CRLF: `above\r\n` was not clause-final (opus) | CR joins LF in the terminator set — two CRLF cases + mutation M9. It fails closed as you say, but *silently skipping recovery* is the exact failure this change exists to stop | | comment wrapped so `// -style` reads as a list marker (sonnet) | reflowed while rewriting that block | | `(gadfly, 3 models)` provenance in a test comment (sonnet) | dropped — that is an execution log, and this project's doc policy says a hard-won lesson becomes an invariant, not a diary entry. The lesson (the space-separated cases are a *different character*, and one passed while the other was broken) stays | Break-check is now **10 mutations**, each killed by a specific named test, unmutated control survives. Running total across both rounds: **14 findings, 14 graded real, 14 fixed.** Nothing dismissed. Two of them exposed hollow tests of mine rather than product bugs, which is the half I would not have found alone. @gadfly review Focus areas for this round: - **`compressionMarkerRe`.** Is the marker list missing a phrasing an open-weight model actually uses to introduce a TL;DR — and can any of the listed phrases appear mid-answer in a way that now makes a legitimate closer disposable? - **The `&&` I just added.** Requiring both halves narrows recovery. Is there a *real* front-loading shape — the thing this PR exists to fix — that no longer recovers because the model pointed backwards without announcing a summary? - **The terminator set after CR.** Anything else that ends a clause in model output and is still missing: a form feed, a unicode line separator, a right-single-quote as an apostrophe? - **Hollow tests.** Three of my fixtures across these rounds have turned out to prove nothing. Assume the count is not three.
steve added 1 commit 2026-08-22 04:16:49 +00:00
fix(agent): gadfly round 3 — the pointer must be all there is
CI / Build & Test (pull_request) Successful in 9m48s
CI / Tidy (pull_request) Successful in 9m22s
1756910ef0
Round 2 fixed this class on isSummaryCloser and left its twin standing
on isWeakFinal. Three of the four findings are that twin, and every one
of them reproduced:

    "That's the chain above. Ship Tuesday."          weak=true
    "See the summary above. Option B wins."          weak=true
    "Anything above 100 boils. See the note above."  weak=true
    "Anything above, say, 40 degrees is a problem."  weak=true

Each of those is 37-58 bytes with the answer sitting right next to the
pointer, and each was disposable — a >=200-byte earlier turn replaced it
and "Ship Tuesday" went in the bin. pointsAbove asks whether a pointer is
PRESENT; disposability needs it to be ALL THERE IS.

bareAbovePointer cuts the reference's own clause — from the end of the
previous sentence through the match — and requires what remains to be
filler: nothing, punctuation, or a throat-clearing "Done —". Cutting the
clause rather than testing position is what makes the other two shapes
safe for free: a mixed terminal keeps its first sentence, and a
comparative "above," with an interjection keeps the rest of its own.

The fourth finding (kimi) is the same idea one level up:
compressionMarkerRe matched anywhere, so "Given the analysis above, the
bottom line is that we need a different vendor" read as an announced
summary when it is a conclusion. The marker must now OPEN a sentence.

Both classes now enforce one rule from opposite ends: a terminal is
disposable only when it carries no answer of its own — proved in
isWeakFinal by nothing standing beside the pointer, and in
isSummaryCloser by the model declaring what stands beside it a
compression.

One of my own round-1b fixtures had to change: "…is above, so there is
no point repeating it" is a pure pointer to a human, but prose after the
reference is indistinguishable from an answer, so the rule correctly
stops treating it as disposable. Replaced with a genuinely bare 96-byte
pointer, still sized so the two recovery bars disagree about it.

Break-check: thirteen mutations, each killed by a named test, control
survives. M12 initially "passed" by failing to compile — reformulated so
it builds, and TestIsWeakFinal/prepositional-then-deictic-with-content
kills it properly.
Author
Owner

Round 3: 4 findings, all 4 real, all 4 fixed in 1756910. This was the best round — and the most embarrassing.

Three of the four are round 2's finding standing on the other branch. I fixed isSummaryCloser and left its twin on isWeakFinal. Reproduced before touching anything:

"That's the chain above. Ship Tuesday."                      weak=true
"See the summary above. Option B wins."                      weak=true
"Anything above 100 boils. See the note above."              weak=true
"Anything above, say, 40 degrees is a problem for the pump." weak=true

Every one of those is 37–58 bytes with the answer sitting right next to the pointer, and every one was disposable — a ≥200-byte earlier turn replaced it and Ship Tuesday went in the bin. pointsAbove asks whether a pointer is present; disposability needs it to be all there is.

bareAbovePointer cuts the reference's own clause — from the end of the previous sentence through the match — and requires what remains to be filler: nothing, punctuation, or a throat-clearing Done —. Cutting the clause rather than testing the match position is what makes the other two shapes safe for free:

  • Anything above 100 boils. See the note above. keeps its first sentence in the remainder → not bare;
  • Anything above, say, 40 degrees… keeps the rest of its own sentence → not bare. That is why the comma stays in the terminator set: dropping it would also lose the genuinely deictic As shown above, ….

The fourth (kimi) is the same idea one level up: compressionMarkerRe matched anywhere, so Given the analysis above, the bottom line is that we need a different vendor read as an announced summary when it is a conclusion. The marker must now open a sentence.

Both classes now enforce one rule from opposite ends: a terminal is disposable only when it carries no answer of its own — proved in isWeakFinal by nothing standing beside the pointer, and in isSummaryCloser by the model declaring what stands beside it a compression.

Two things I had to correct in my own work

  • A fixture of mine had to change. My round-1b shortAbovePointer ended …is above, so there is no point repeating it — a pure pointer to a human, but prose after the reference is indistinguishable from an answer, so the new rule correctly stopped treating it as disposable. I did not weaken the rule to keep the fixture; I replaced the fixture with a genuinely bare 96-byte pointer, still sized so the two recovery bars disagree about it.
  • A break-check mutation was passing by not compiling. M12 (drop the prefix from the remainder) left an unused variable, so go test failed to build and the harness scored it KILLED. That is a false green in the tool I use to trust everything else. Reformulated so it builds; TestIsWeakFinal/prepositional-then-deictic-with-content kills it properly.

Break-check is now 13 mutations, each killed by a specific named test, unmutated control survives.

Running total: 18 findings across three rounds, 18 graded real, 18 fixed. Nothing dismissed, and the severity went up in round 3, not down — which is exactly why I did not merge after round 2.

@gadfly review

Focus for this round:

  • Is the twin gone, or is there a third instance? Round 2 fixed one branch of "a pointer does not make the text beside it disposable", round 3 fixed the other. modeCitations still recovers with no such check — is that a third instance, or genuinely different because a sources list carries no prose?
  • bareAbovePointer's clause cut. LastIndexAny(t[:loc[0]], ".!?\n") for the start. What breaks it — an abbreviation, a decimal, a numbered list, a URL with a dot before the pointer?
  • bareRemainderRe and fillerWords. The remainder must be only filler. Is there a real closer whose leftovers are meaningless but not in that three-word list?
  • Hollow tests. Five of my fixtures across this PR and its mort sibling have now turned out to prove nothing, and one break-check mutation was scoring itself green on a compile error. Assume there are more.
Round 3: **4 findings, all 4 real, all 4 fixed** in `1756910`. This was the best round — and the most embarrassing. Three of the four are **round 2's finding standing on the other branch.** I fixed `isSummaryCloser` and left its twin on `isWeakFinal`. Reproduced before touching anything: ``` "That's the chain above. Ship Tuesday." weak=true "See the summary above. Option B wins." weak=true "Anything above 100 boils. See the note above." weak=true "Anything above, say, 40 degrees is a problem for the pump." weak=true ``` Every one of those is 37–58 bytes with the answer sitting right next to the pointer, and every one was disposable — a ≥200-byte earlier turn replaced it and *Ship Tuesday* went in the bin. `pointsAbove` asks whether a pointer is **present**; disposability needs it to be **all there is**. `bareAbovePointer` cuts the reference's own clause — from the end of the previous sentence through the match — and requires what remains to be filler: nothing, punctuation, or a throat-clearing `Done —`. Cutting the **clause** rather than testing the match position is what makes the other two shapes safe for free: - `Anything above 100 boils. See the note above.` keeps its first sentence in the remainder → not bare; - `Anything above, say, 40 degrees…` keeps the rest of its own sentence → not bare. That is why the comma stays in the terminator set: dropping it would also lose the genuinely deictic `As shown above, …`. The fourth (kimi) is the same idea one level up: `compressionMarkerRe` matched **anywhere**, so `Given the analysis above, the bottom line is that we need a different vendor` read as an announced summary when it is a conclusion. The marker must now **open a sentence**. Both classes now enforce one rule from opposite ends: **a terminal is disposable only when it carries no answer of its own** — proved in `isWeakFinal` by nothing standing beside the pointer, and in `isSummaryCloser` by the model declaring what stands beside it a compression. ### Two things I had to correct in my own work - **A fixture of mine had to change.** My round-1b `shortAbovePointer` ended `…is above, so there is no point repeating it` — a pure pointer to a human, but prose after the reference is indistinguishable from an answer, so the new rule correctly stopped treating it as disposable. I did **not** weaken the rule to keep the fixture; I replaced the fixture with a genuinely bare 96-byte pointer, still sized so the two recovery bars disagree about it. - **A break-check mutation was passing by not compiling.** M12 (drop the prefix from the remainder) left an unused variable, so `go test` failed to build and the harness scored it KILLED. That is a false green in the tool I use to trust everything else. Reformulated so it builds; `TestIsWeakFinal/prepositional-then-deictic-with-content` kills it properly. Break-check is now **13 mutations**, each killed by a specific named test, unmutated control survives. Running total: **18 findings across three rounds, 18 graded real, 18 fixed.** Nothing dismissed, and the severity went *up* in round 3, not down — which is exactly why I did not merge after round 2. @gadfly review Focus for this round: - **Is the twin gone, or is there a third instance?** Round 2 fixed one branch of "a pointer does not make the text beside it disposable", round 3 fixed the other. `modeCitations` still recovers with no such check — is that a third instance, or genuinely different because a sources list carries no prose? - **`bareAbovePointer`'s clause cut.** `LastIndexAny(t[:loc[0]], ".!?\n")` for the start. What breaks it — an abbreviation, a decimal, a numbered list, a URL with a dot before the pointer? - **`bareRemainderRe` and `fillerWords`.** The remainder must be *only* filler. Is there a real closer whose leftovers are meaningless but not in that three-word list? - **Hollow tests.** Five of my fixtures across this PR and its mort sibling have now turned out to prove nothing, and one break-check mutation was scoring itself green on a compile error. Assume there are more.
steve added 1 commit 2026-08-22 04:28:56 +00:00
fix(agent): gadfly round 4 — the answer can come BEFORE the pointer
CI / Build & Test (pull_request) Successful in 9m46s
CI / Tidy (pull_request) Successful in 9m22s
184627c570
My round-3 fix has the same defect it fixed, mirrored. bareAbovePointer
cut back to the previous FULL STOP, not the previous clause, so anything
sharing the reference's sentence ahead of it was swallowed:

    "Ship Tuesday, as shown above."                  bare=true
    "OK. The verdict is guilty, as detailed above."  bare=true
    "The answer is sixty minutes, as computed above." bare=true

All three read as pure pointers and were replaced by an earlier turn.
The comment called it a clause cut; the code cut a sentence. It survived
a round because every case I had written put the answer AFTER the
pointer — I fixed the direction I had thought of and tested only that.
clauseBoundaryChars now includes , ; : and the em dash.

Three more, all real:

- pointsAbove and bareAbovePointer each did their own FindStringIndex
  plus offset check, so the two could disagree about what a pointer is.
  One aboveRefLoc now answers that.
- fillerWords was extracted to stop drift while its separator class was
  still copy-pasted beside it — the same defect one token over. fillerSep
  shares it too.
- A missing blank comment line ran two paragraphs together.

The padded 96-byte "bare pointer" fixture is gone rather than bent a
third time. Its premise does not survive the rule: a genuinely bare
pointer is its own clause and nothing else, so it is SHORT, and the two
recovery bars cannot disagree about a short terminal. I had twice
reshaped that string to keep a case alive whose scenario the code no
longer admits. The modeBackRef path stays covered by the bare-pointer
cases in TestIsWeakFinal and the "(Already answered above.)" end-to-end.

Break-check: fifteen mutations including a regression to the sentence
cut, each killed by a named test; the harness now also fails a mutation
that only breaks the build.
Author
Owner

Round 4: 6 findings, all 6 real, 5 fixed and 1 accepted with reasoning, in 184627c.

The answer can come BEFORE the pointer

Two findings (opus, on two lenses) are a real bug in the fix I shipped last round — the same defect, mirrored. bareAbovePointer cut back to the previous full stop, not the previous clause, so anything sharing the reference's sentence ahead of it was swallowed:

"Ship Tuesday, as shown above."                   bare=true
"OK. The verdict is guilty, as detailed above."   bare=true
"The answer is sixty minutes, as computed above." bare=true

Every one of those read as a pure pointer and was replaced by an earlier turn. My comment called it a clause cut; the code cut a sentence. It survived a round because every case I wrote put the answer after the pointer — I fixed the direction I had thought of and then tested only that direction. clauseBoundaryChars now includes , ; : and the em dash.

The second finding's example is the sharper one: the filler opener OK. supplies the very full stop the cut anchors on, so a throat-clearing word is what loses you the verdict.

The rest

finding resolution
pointsAbove / bareAbovePointer each ran their own match + offset test one aboveRefLoc. Worth more than tidiness: bareAbovePointer is meant to be a stricter pointsAbove, which only works if they start from the same match
fillerSep copy-pasted beside the fillerWords I extracted to stop drift shared — the same defect one token over, inside the change meant to prevent it
two comment paragraphs run together blank line added

Accepted, not fixed (glm): a pointer plus a sentence-opening marker introducing genuinely new content is still misreadable. At that point four conditions have all held — a deictic pointer in the opening 120 bytes, a sentence-opening compression marker, a terminal under 300 bytes, and a same-turn prior turn ≥3× longer — and the marker is the model's own declaration that what follows is a condensation. No lexical test separates "declared a summary and wrote one" from "declared a summary and wrote something else"; refusing to trust the declaration at all is what reverts #1611. The mandatory dwarf ratio is the backstop.

A fixture I stopped bending

The padded 96-byte "bare pointer" is deleted, not reshaped a third time. Its premise does not survive the rule: a genuinely bare pointer is its own clause and nothing else, so it is short, and the two recovery bars cannot disagree about a short terminal. I had twice rewritten that string to keep alive a case whose scenario the code no longer admits — which is a test steering the code rather than the other way round. The modeBackRef path stays covered by the bare-pointer cases in TestIsWeakFinal and the (Already answered above.) end-to-end.

Break-check: 15 mutations, including a regression to the sentence cut and a widening of the shared offset bound, each killed by a named test. The harness now also fails a mutation that is "killed" only by breaking the build.

Running total: 24 findings across four rounds, 24 real, 23 fixed, 1 accepted with reasoning. Rounds 3 and 4 each found a genuine behaviour bug — including one in the previous round's fix — so I am running another.

@gadfly review

Focus:

  • Is the mirror gone, or is there a third orientation? Round 3 caught "answer after the pointer", round 4 caught "answer before it, same sentence". What about across a newline, inside a list item, or a pointer in the middle of three clauses?
  • clauseBoundaryChars now cuts at , ; : . Does that make the cut too narrow anywhere — a genuinely bare pointer that contains one of those and is now judged non-bare, so a front-loaded answer stops being recovered? That direction fails closed but it is the failure this PR exists to fix.
  • Anything still hollow. Six of my fixtures across this PR and its mort sibling have now proved nothing, and I have deleted one rather than fix it. Assume there are more.
Round 4: **6 findings, all 6 real, 5 fixed and 1 accepted with reasoning**, in `184627c`. ### The answer can come BEFORE the pointer Two findings (opus, on two lenses) are **a real bug in the fix I shipped last round** — the same defect, mirrored. `bareAbovePointer` cut back to the previous **full stop**, not the previous clause, so anything sharing the reference's sentence *ahead* of it was swallowed: ``` "Ship Tuesday, as shown above." bare=true "OK. The verdict is guilty, as detailed above." bare=true "The answer is sixty minutes, as computed above." bare=true ``` Every one of those read as a pure pointer and was replaced by an earlier turn. My comment called it a *clause* cut; the code cut a *sentence*. It survived a round because **every case I wrote put the answer after the pointer** — I fixed the direction I had thought of and then tested only that direction. `clauseBoundaryChars` now includes `,` `;` `:` and the em dash. The second finding's example is the sharper one: the filler opener `OK.` supplies the very full stop the cut anchors on, so a throat-clearing word is what loses you the verdict. ### The rest | finding | resolution | |---|---| | `pointsAbove` / `bareAbovePointer` each ran their own match + offset test | one `aboveRefLoc`. Worth more than tidiness: `bareAbovePointer` is meant to be a *stricter* `pointsAbove`, which only works if they start from the same match | | `fillerSep` copy-pasted beside the `fillerWords` I extracted to stop drift | shared — the same defect one token over, inside the change meant to prevent it | | two comment paragraphs run together | blank line added | **Accepted, not fixed** (glm): a pointer plus a sentence-opening marker introducing genuinely *new* content is still misreadable. At that point four conditions have all held — a deictic pointer in the opening 120 bytes, a sentence-opening compression marker, a terminal under 300 bytes, and a same-turn prior turn ≥3× longer — and the marker is the model's own declaration that what follows is a condensation. No lexical test separates "declared a summary and wrote one" from "declared a summary and wrote something else"; refusing to trust the declaration at all is what reverts #1611. The mandatory dwarf ratio is the backstop. ### A fixture I stopped bending The padded 96-byte "bare pointer" is **deleted**, not reshaped a third time. Its premise does not survive the rule: a genuinely bare pointer is its own clause and nothing else, so it is *short*, and the two recovery bars cannot disagree about a short terminal. I had twice rewritten that string to keep alive a case whose scenario the code no longer admits — which is a test steering the code rather than the other way round. The `modeBackRef` path stays covered by the bare-pointer cases in `TestIsWeakFinal` and the `(Already answered above.)` end-to-end. Break-check: **15 mutations**, including a regression to the sentence cut and a widening of the shared offset bound, each killed by a named test. The harness now also fails a mutation that is "killed" only by breaking the build. Running total: **24 findings across four rounds, 24 real, 23 fixed, 1 accepted with reasoning.** Rounds 3 and 4 each found a genuine behaviour bug — including one in the previous round's fix — so I am running another. @gadfly review Focus: - **Is the mirror gone, or is there a third orientation?** Round 3 caught "answer after the pointer", round 4 caught "answer before it, same sentence". What about across a newline, inside a list item, or a pointer in the middle of three clauses? - **`clauseBoundaryChars` now cuts at `,` `;` `:` `—`.** Does that make the cut too *narrow* anywhere — a genuinely bare pointer that contains one of those and is now judged non-bare, so a front-loaded answer stops being recovered? That direction fails closed but it is the failure this PR exists to fix. - **Anything still hollow.** Six of my fixtures across this PR and its mort sibling have now proved nothing, and I have deleted one rather than fix it. Assume there are more.
steve added 1 commit 2026-08-22 04:32:47 +00:00
test(agent): enumerate the orientations instead of patching them one by one
CI / Build & Test (pull_request) Successful in 10m3s
CI / Tidy (pull_request) Successful in 9m21s
285789c89b
Three consecutive rounds found the same rule broken in a different
orientation — content after the pointer, then content before it in the
same sentence — because each round's cases only covered the direction
that round was about, and each fix was then tested only in the direction
I had just thought of. A fourth patch was not the answer; the missing
thing was a harness.

TestBareAbovePointerOrientations crosses every pointer form with every
position content can occupy: alone, before in the same sentence, before
in its own sentence, on its own line, as a list item, after in each of
those, and behind a filler opener. 36 cells, one assertion — bare IFF
there is no content — and each cell first asserts the input really is a
pointer, so a mistyped fixture fails loudly instead of passing vacuously.

It earned itself immediately: three cells failed on the first run, and
the bug was mine and shipped. clauseBoundaryChars gained the em dash last
round, and the cut did start = k + 1 — but LastIndexAny returns the BYTE
index of the boundary rune, and an em dash is three bytes. "Done — as
shown above." sliced mid-rune, left a stray continuation byte in the
remainder that no trim removes, and a genuinely bare pointer stopped
being recovered. Now advances by the rune's width.

Break-check: sixteen mutations, each killed by a named test, control
survives. Two had to be reformulated after the harness started failing
mutations that were only "killed" by breaking the build — including the
one for this very fix.
steve added 1 commit 2026-08-22 04:37:50 +00:00
test(agent): the orientation matrix iterated maps, so its 36 cells shuffled
CI / Build & Test (pull_request) Successful in 9m51s
CI / Tidy (pull_request) Successful in 9m26s
bb2196ecb2
Caught on the sibling mort PR (gadfly/sonnet) and the same mistake was
here: map iteration is randomised, so the matrix printed its cells in a
different order every run and two failing runs could not be diffed. The
"which placements are bare" lookup was a second map keyed by name, which
also let a placement and its expectation drift apart by a typo.

Both are now one slice of {name, bare, build}, so the expectation sits
on the row it belongs to and the order is fixed.
gitea-actions bot reviewed 2026-08-22 04:40:17 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 4 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** — 4 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -116,0 +146,4 @@
// almost no text before it in THIS message, the reference cannot be pointing
// at the message's own content, so it must point at a turn the user never saw
// (the harness delivers only the final turn).
//

Rationale for not sharing backRefRe with isSummaryCloser is duplicated verbatim in pointsAbove and isSummaryCloser comments

maintainability · flagged by 1 model

2. Identical rationale stated in two placesagent/finalize.go:149-155 and agent/finalize.go:413-417

🪰 Gadfly · advisory

⚪ **Rationale for not sharing backRefRe with isSummaryCloser is duplicated verbatim in pointsAbove and isSummaryCloser comments** _maintainability · flagged by 1 model_ **2. Identical rationale stated in two places** — `agent/finalize.go:149-155` and `agent/finalize.go:413-417` <sub>🪰 Gadfly · advisory</sub>
@@ -116,0 +189,4 @@
if loc == nil {
return false
}
start := 0

🟠 em dash in clauseBoundaryChars makes start=k+1 land mid-rune, so a bare "Done — … above." closer defeats bareRemainderRe and is not recovered

correctness, error-handling · flagged by 1 model

agent/finalize.go:192 — em dash in clauseBoundaryChars makes start = k+1 land mid-rune, so a short bare "Done — … above." closer is not recovered.

🪰 Gadfly · advisory

🟠 **em dash in clauseBoundaryChars makes start=k+1 land mid-rune, so a bare "Done — … above." closer defeats bareRemainderRe and is not recovered** _correctness, error-handling · flagged by 1 model_ **`agent/finalize.go:192` — em dash in `clauseBoundaryChars` makes `start = k+1` land mid-rune, so a short bare "Done — … above." closer is not recovered.** <sub>🪰 Gadfly · advisory</sub>
@@ -116,0 +202,4 @@
return bareRemainderRe.MatchString(strings.Trim(rest, pointerResidueCutset))
}
// clauseBoundaryChars ends the clause the reference belongs to. Commas,

🟡 First-person authorial narrative in permanent code comment ('every case I had written') violates project comment conventions

maintainability · flagged by 2 models

1. First-person authorial voice in a permanent code commentagent/finalize.go:205-206

🪰 Gadfly · advisory

🟡 **First-person authorial narrative in permanent code comment ('every case I had written') violates project comment conventions** _maintainability · flagged by 2 models_ **1. First-person authorial voice in a permanent code comment** — `agent/finalize.go:205-206` <sub>🪰 Gadfly · advisory</sub>
@@ -39,0 +73,4 @@
// analysis1611 stands in for that run's front-loaded analysis: long enough to
// dwarf closer1611 (>3x its 220 bytes). Shared by the finalOutput table and
// the end-to-end Run test so the two cannot drift apart.
func analysis1611() string {

analysis1611 is a function rather than a var — inconsistent with other test fixtures and recomputes on each call

maintainability · flagged by 1 model

3. analysis1611() is a function rather than a varagent/finalize_test.go:76-78

🪰 Gadfly · advisory

⚪ **analysis1611 is a function rather than a var — inconsistent with other test fixtures and recomputes on each call** _maintainability · flagged by 1 model_ **3. `analysis1611()` is a function rather than a `var`** — `agent/finalize_test.go:76-78` <sub>🪰 Gadfly · advisory</sub>
steve added 1 commit 2026-08-22 04:42:25 +00:00
fix(agent): "In short supply" is not a short version
CI / Build & Test (pull_request) Successful in 10m17s
CI / Tidy (pull_request) Successful in 9m26s
7fd239c8cd
Round 5. Two of the eight findings are the em-dash mid-rune slice, which
the orientation matrix had already caught and fixed in 285789c before
this round returned — the harness doing the reviewers' job, which is the
point of it.

Of the rest:

**A marker phrase can open a sentence and still be prose** (kimi).
"That's the chain above. In short supply of alternatives, we went with
B." opens a sentence with "In short" and announces nothing. The marker
must now be followed by the delimiter a model puts after a real
announcement — the colon in "Short version:", the comma in "In short,".
No delimiter means no match, which keeps the terminal: fails closed.

Adding that made the sentence-opening anchor look redundant — a
break-check that removed it survived, because every mid-sentence case in
the table was by then rejected for want of a delimiter. It is not
redundant: "…, and in short, we went with B" is a clause continuation
with a comma. The missing thing was a case for what the anchor alone
does, not the anchor.

**backRefHeadChars was named for the wrong family** (opus; sonnet raised
the same constant from the other side). It is applied only to the
deictic offset test, never to backRefRe. Renamed abovePointerHeadChars.
It stays defined as `= weakFinalMaxChars` rather than inlined, because an
earlier round asked for exactly that linkage and the reason still holds.

**The clause-boundary comment narrated my review history** (opus) — the
same class of finding I took two rounds ago and then reintroduced one
comment over. Rewritten as behaviour, pointing at the matrix.

**analysis1611 was a function returning a constant string** (sonnet) —
now a var.

Accepted, not fixed: a comparative "above," plus a sentence-opening
marker can still reach isSummaryCloser. That needs the comparative false
positive, a real marker, a terminal under 300 bytes, and a same-turn
prior at least 3x longer, all at once; the dwarf ratio is the backstop
and no lexical test separates the comparative from the deictic.

Break-check: seventeen mutations, each killed by a named test.
Author
Owner

Round 5: 8 findings, all 8 real — 7 fixed, 1 accepted with reasoning. In 7fd239c.

The harness got there first

Two of the eight (ec52323c, 54f481d5) are the em-dash mid-rune slice. Already fixed in 285789c, before this round returned — found by the orientation matrix I added at the top of the round, which failed three cells on its first ever run for exactly that reason. LastIndexAny returns the byte index of the boundary rune, an em dash is three bytes, and start = k+1 sliced into the middle of it.

That is the matrix earning its place on day one, and it is the answer to the question I asked in round 4 — the reason this class kept coming back was never a missing patch.

"In short supply" is not a short version

(kimi) I anchored the marker to a sentence opening last round and assumed that settled it. It does not: That's the chain above. In short supply of alternatives, we went with B. opens a sentence with In short and announces nothing. The marker must now also be followed by the delimiter a model actually puts after a real announcement — the colon in Short version:, the comma in In short,. No delimiter, no match, terminal kept.

Fixing that immediately exposed a gap in my own table: a break-check that removed the sentence anchor started surviving, because by then every mid-sentence case was being rejected for want of a delimiter instead. The anchor is not redundant — …, and in short, we went with B is a clause continuation with a comma — I simply had no case for what the anchor does on its own. Added.

The rest

finding resolution
backRefHeadChars named for the wrong family (opus) renamed abovePointerHeadChars — it governs the deictic offset only, never backRefRe
same constant, "prefer inlining" (sonnet) half declined: naming fixed, alias kept. Round 1 of this review asked for exactly that linkage — two unlinked 120s the comment called "the same guard" — and a test pins the equality. Undoing it trades an invariant for a style preference
clause-boundary comment narrates my review history (opus) rewritten as behaviour. This is the finding I accepted two rounds ago about a test comment and then reintroduced one comment over
analysis1611 was a func returning a constant (sonnet) now a var

Accepted, not fixed (sonnet): a comparative above, plus a sentence-opening marker can still reach isSummaryCloser. isWeakFinal is protected — bareAbovePointer keeps the rest of the comparative's clause, so it is not bare — but requiring that in isSummaryCloser would reject #1611's own closer. It needs the comparative, a real delimited marker, a terminal under 300 bytes, and a same-turn prior ≥3× longer, all at once, with the dwarf ratio as backstop. No lexical test separates a comparative above, from a deictic one, and dropping the comma would lose the genuinely deictic As shown above, ….

Break-check: 17 mutations, each killed by a named test, control survives.

Running total: 32 findings across five rounds — 32 real, 30 fixed, 2 accepted with recorded reasoning. Zero false positives in this repo.

@gadfly review

Last round unless something behavioural turns up. Focus only on:

  • compressionMarkerRe, third revision. Anchor plus delimiter. Is there a real announced-summary shape it now misses, or a prose shape it still catches?
  • Anything behavioural anywhere else. Naming and comments are settled — please don't spend the round on them.
Round 5: **8 findings, all 8 real — 7 fixed, 1 accepted with reasoning.** In `7fd239c`. ### The harness got there first Two of the eight (`ec52323c`, `54f481d5`) are the em-dash mid-rune slice. **Already fixed in `285789c`, before this round returned** — found by the orientation matrix I added at the top of the round, which failed three cells on its first ever run for exactly that reason. `LastIndexAny` returns the *byte* index of the boundary rune, an em dash is three bytes, and `start = k+1` sliced into the middle of it. That is the matrix earning its place on day one, and it is the answer to the question I asked in round 4 — the reason this class kept coming back was never a missing patch. ### "In short supply" is not a short version **(kimi)** I anchored the marker to a sentence opening last round and assumed that settled it. It does not: `That's the chain above. In short supply of alternatives, we went with B.` opens a sentence with `In short` and announces nothing. The marker must now also be followed by the **delimiter** a model actually puts after a real announcement — the colon in `Short version:`, the comma in `In short,`. No delimiter, no match, terminal kept. Fixing that immediately exposed a gap in my own table: a break-check that **removed the sentence anchor started surviving**, because by then every mid-sentence case was being rejected for want of a delimiter instead. The anchor is not redundant — `…, and in short, we went with B` is a clause continuation *with* a comma — I simply had no case for what the anchor does on its own. Added. ### The rest | finding | resolution | |---|---| | `backRefHeadChars` named for the wrong family (opus) | renamed `abovePointerHeadChars` — it governs the deictic offset only, never `backRefRe` | | same constant, "prefer inlining" (sonnet) | **half declined**: naming fixed, alias kept. Round 1 of this review asked for exactly that linkage — two unlinked `120`s the comment called "the same guard" — and a test pins the equality. Undoing it trades an invariant for a style preference | | clause-boundary comment narrates my review history (opus) | rewritten as behaviour. This is the finding I accepted two rounds ago about a *test* comment and then reintroduced one comment over | | `analysis1611` was a func returning a constant (sonnet) | now a var | **Accepted, not fixed** (sonnet): a comparative `above,` plus a sentence-opening marker can still reach `isSummaryCloser`. `isWeakFinal` is protected — `bareAbovePointer` keeps the rest of the comparative's clause, so it is not bare — but requiring that in `isSummaryCloser` would reject #1611's own closer. It needs the comparative, a real delimited marker, a terminal under 300 bytes, and a same-turn prior ≥3× longer, all at once, with the dwarf ratio as backstop. No lexical test separates a comparative `above,` from a deictic one, and dropping the comma would lose the genuinely deictic `As shown above, …`. Break-check: **17 mutations**, each killed by a named test, control survives. Running total: **32 findings across five rounds — 32 real, 30 fixed, 2 accepted with recorded reasoning.** Zero false positives in this repo. @gadfly review Last round unless something behavioural turns up. Focus only on: - **`compressionMarkerRe`, third revision.** Anchor plus delimiter. Is there a real announced-summary shape it now misses, or a prose shape it still catches? - **Anything behavioural anywhere else.** Naming and comments are settled — please don't spend the round on them.
Author
Owner

Round 6: 1 finding, real, already fixed. analysis1611 as a func rather than a var — the same reviewer's round-5 finding, landed in 7fd239c while this round was running. A re-report against the older head, not new signal.

Final tally: 33 findings across six rounds — 33 real, 31 fixed, 2 accepted with recorded reasoning, zero false positives in this repo.

Six of them were genuine behaviour bugs, and three were in my own fixes from the round before:

round the bug
1 \b holds between above and -, so every hyphenated compound (above-board, above-average) read as a deictic pointer. Found by 3 models independently; my own test used the space-separated form
2 a pointer alone made a terminal disposable, so Given the analysis above, I recommend option B lost the recommendation
3 same class, other branch — That's the chain above. Ship Tuesday. lost the decision
4 same class, mirrored — Ship Tuesday, as shown above. lost it too, because the "clause cut" was a sentence cut
em dash is 3 bytes and start = k+1 sliced mid-rune. Caught by the orientation matrix, not by review
5 In short supply of alternatives opens a sentence with a marker phrase and announces nothing

The pattern is the point: rounds 2, 3 and 4 were one rule broken in three orientations, each surviving because the round's own cases only covered the direction that round was about. The fix was not a fourth patch — it was TestBareAbovePointerOrientations, 36 cells crossing every pointer form with every position content can occupy. It failed three cells on its first run and found the em-dash bug before round 5 reported it.

The review also caught six hollow tests of mine, which is the half I could not have found alone: a bound case sized from the constant it was meant to pin, a fixture too short to distinguish the two code paths it was named for, two break-check mutations scoring green purely by failing to compile, and a fixture I bent twice before admitting its premise was unreachable and deleting it. The break-check now fails any mutation that is "killed" only by the build.

Merging. Thanks — this was worth every round.

Round 6: **1 finding, real, already fixed.** `analysis1611` as a func rather than a var — the same reviewer's round-5 finding, landed in `7fd239c` while this round was running. A re-report against the older head, not new signal. **Final tally: 33 findings across six rounds — 33 real, 31 fixed, 2 accepted with recorded reasoning, zero false positives in this repo.** Six of them were genuine behaviour bugs, and **three were in my own fixes from the round before**: | round | the bug | |---|---| | 1 | `\b` holds between `above` and `-`, so every hyphenated compound (`above-board`, `above-average`) read as a deictic pointer. Found by 3 models independently; my own test used the *space-separated* form | | 2 | a pointer alone made a terminal disposable, so `Given the analysis above, I recommend option B` lost the recommendation | | 3 | same class, other branch — `That's the chain above. Ship Tuesday.` lost the decision | | 4 | same class, mirrored — `Ship Tuesday, as shown above.` lost it too, because the "clause cut" was a *sentence* cut | | — | em dash is 3 bytes and `start = k+1` sliced mid-rune. **Caught by the orientation matrix, not by review** | | 5 | `In short supply of alternatives` opens a sentence with a marker phrase and announces nothing | The pattern is the point: rounds 2, 3 and 4 were one rule broken in three orientations, each surviving because the round's own cases only covered the direction that round was about. The fix was not a fourth patch — it was `TestBareAbovePointerOrientations`, 36 cells crossing every pointer form with every position content can occupy. It failed three cells on its first run and found the em-dash bug before round 5 reported it. The review also caught **six hollow tests of mine**, which is the half I could not have found alone: a bound case sized from the constant it was meant to pin, a fixture too short to distinguish the two code paths it was named for, two break-check mutations scoring green purely by failing to compile, and a fixture I bent twice before admitting its premise was unreachable and deleting it. The break-check now fails any mutation that is "killed" only by the build. Merging. Thanks — this was worth every round.
steve merged commit faf9e3f5b8 into main 2026-08-22 04:53:04 +00:00
steve deleted branch fix/backref-above-closer 2026-08-22 04:53:04 +00:00
Author
Owner

Correction to the merge commit message: it says CI "hung" in actions/setup-go's cache-save post step. Run 7090 in fact completed greenCI / Build & Test: success, CI / Tidy: success — seconds after the merge went through. It was slow (about 15 minutes tarring the Go cache), not stuck.

The merge was still made on the right evidence and the outcome is the same, but "hung" is wrong in the permanent record, so: the check passed on its own.

Correction to the merge commit message: it says CI "hung" in `actions/setup-go`'s cache-save post step. Run 7090 in fact **completed green** — `CI / Build & Test: success`, `CI / Tidy: success` — seconds after the merge went through. It was slow (about 15 minutes tarring the Go cache), not stuck. The merge was still made on the right evidence and the outcome is the same, but "hung" is wrong in the permanent record, so: the check passed on its own.
Sign in to join this conversation.
No Reviewers
No labels
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: steve/majordomo#28