fix(agent): recover the front-loaded answer over a summary closer #25

Merged
steve merged 3 commits from fix/finalize-summary-closer into main 2026-08-06 00:39:39 +00:00
2 changed files with 107 additions and 19 deletions
Showing only changes of commit 21b4775d16 - Show all commits
+49 -17
View File
@@ -28,15 +28,18 @@ import (
// - a bookkeeping closer ("Citations are logged. Short version: …"): the // - a bookkeeping closer ("Citations are logged. Short version: …"): the
// model acknowledged the citation round and compressed the answer it had // model acknowledged the citation round and compressed the answer it had
// already written into a one-liner (mort run b3cb9ee9 — a 2,089-char answer // already written into a one-liner (mort run b3cb9ee9 — a 2,089-char answer
// shrank to 151 chars at delivery). The compression is strictly poorer than // shrank to a 153-byte closer at delivery). The compression is strictly
// the front-loaded answer, so recover the prior turn and DISCARD the // poorer than the front-loaded answer, so recover the prior turn and
// closer — but only when the prior turn clearly dwarfs it, because unlike a // DISCARD the closer — but only when the prior turn clearly dwarfs it,
// back-reference this closer DOES carry answer content (see modeSummary). // because unlike a back-reference this closer DOES carry answer content
// (see modeSummary).
// //
// A citations addendum is tested first and wins over the other two (a short // A citations addendum is tested first and wins over the other two (a short
// terminal can match more than one shape), so its links are never discarded; // terminal can match more than one shape), so its links are never discarded.
// the summary closer is tested before the plain weak-final test so the // The back-reference test wins over the summary-closer test: a terminal
// stricter recovery bar applies when both match. When the terminal text stands // matching both ("Citations are logged. As I said above…") carries no answer
// content of its own, so the looser back-ref recovery bar — not the summary
// closer's dwarf ratio — is the right one. When the terminal text stands
// on its own it is returned unchanged; when it is degenerate but nothing // on its own it is returned unchanged; when it is degenerate but nothing
// better can be recovered, it is returned as-is (a compressed answer still // better can be recovered, it is returned as-is (a compressed answer still
// beats nothing). // beats nothing).
@@ -48,9 +51,11 @@ func finalOutput(msgs []llm.Message, terminal string) string {
switch { switch {
case isCitationsOnly(terminal): case isCitationsOnly(terminal):
mode = modeCitations mode = modeCitations
case isWeakFinal(terminal):
Review

Empty isWeakFinal case relies on pre-initialized mode; explicit mode = modeBackRef would be more self-contained

maintainability · flagged by 2 models

  • agent/finalize.go:54-55 — empty case body relying on the pre-initialized mode. go case isWeakFinal(terminal): // modeBackRef This depends on mode := modeBackRef set two lines up; the comment documents it, but an explicit mode = modeBackRef in the case body is more self-contained and survives a future refactor that changes the initializer. Trivial, optional.

🪰 Gadfly · advisory

⚪ **Empty isWeakFinal case relies on pre-initialized mode; explicit `mode = modeBackRef` would be more self-contained** _maintainability · flagged by 2 models_ - **`agent/finalize.go:54-55` — empty `case` body relying on the pre-initialized `mode`.** ```go case isWeakFinal(terminal): // modeBackRef ``` This depends on `mode := modeBackRef` set two lines up; the comment documents it, but an explicit `mode = modeBackRef` in the case body is more self-contained and survives a future refactor that changes the initializer. Trivial, optional. <sub>🪰 Gadfly · advisory</sub>
// modeBackRef
case isSummaryCloser(terminal): case isSummaryCloser(terminal):
mode = modeSummary mode = modeSummary
case !isWeakFinal(terminal): default:
return terminal return terminal
} }
rec, ok := lastSubstantiveAssistantText(msgs, terminal, mode) rec, ok := lastSubstantiveAssistantText(msgs, terminal, mode)
@@ -95,8 +100,11 @@ const (
// modeSummary: the terminal acknowledges the citation round and may carry // modeSummary: the terminal acknowledges the citation round and may carry
// a short compression of the front-loaded answer. Unlike a back-reference // a short compression of the front-loaded answer. Unlike a back-reference
// it DOES contain answer content, so it is only replaced when a prior turn // it DOES contain answer content, so it is only replaced when a prior turn
// clearly dwarfs it — the ratio is mandatory at every length, and the // clearly dwarfs it — the ratio is mandatory at every length, the recovery
// closer is discarded (its content is a strict subset of what it replaced). // scan stops at the most recent user message (a compression can only be of
// THIS turn's answer; never resurrect one from an earlier question), and
// the closer is discarded (its content is a strict subset of what it
// replaced).
modeSummary modeSummary
) )
@@ -117,8 +125,20 @@ var backRefRe = regexp.MustCompile(`(?i)(already answered|see above|as (i )?(sai
// ("Short version: no.") is deliberately out of scope — a user who asked for // ("Short version: no.") is deliberately out of scope — a user who asked for
// brevity would be answered with exactly that shape, and misclassifying it // brevity would be answered with exactly that shape, and misclassifying it
// would hijack a legitimate answer; an unmatched closer merely keeps today's // would hijack a legitimate answer; an unmatched closer merely keeps today's
// behavior (fail closed). // behavior (fail closed). Assembled from named fragments so the alternations
var summaryCloserRe = regexp.MustCompile(`(?i)^[\s>#*_-]*((done|all set|ok(ay)?)[\s,.!:—-]+)?(((all|the)\s+)?(citations?|sources?|references?|claims?)\s+((are|were|have\s+been|all)\s+)*(logged|recorded|cited|saved|noted|captured|filed)|logged\s+((all|the)\s+)*(citations?|sources?|references?))[.!]`) // stay legible and extendable.
const (
Review

🟡 Leading-marker char class [\s>#*_+-]* duplicated between summaryLead and citationLabelRe (comment admits the copy); hoist a shared const

maintainability · flagged by 3 models

  • agent/finalize.go:131 / :156 — duplicated leading-marker character class. summaryLead = [\s>#_+-] is a byte-for-byte copy of the leading class in `citationLabelRe`, and the comment (`// … as in citationLabelRe`) explicitly acknowledges the copy. Two independent literals for "the same set of leading markdown/list markers" will drift if one is ever extended. Low-churn fix: hoist a shared `const leadMarkers = `[\s>#*_+-]* and reference it from both summaryLead and `citationLabel…

🪰 Gadfly · advisory

🟡 **Leading-marker char class `[\s>#*_+-]*` duplicated between summaryLead and citationLabelRe (comment admits the copy); hoist a shared const** _maintainability · flagged by 3 models_ - **`agent/finalize.go:131` / `:156` — duplicated leading-marker character class.** `summaryLead = `[\s>#*_+-]*`` is a byte-for-byte copy of the leading class in `citationLabelRe`, and the comment (`// … as in citationLabelRe`) explicitly acknowledges the copy. Two independent literals for "the same set of leading markdown/list markers" will drift if one is ever extended. Low-churn fix: hoist a shared `const leadMarkers = `[\s>#*_+-]*`` and reference it from both `summaryLead` and `citationLabel… <sub>🪰 Gadfly · advisory</sub>
summaryLead = `[\s>#*_+-]*` // leading markdown/list markers, as in citationLabelRe
summaryPreface = `((done|all set|ok(ay)?)[\s,.!:—-]+)?` // optional "Done —" style opener
summaryNouns = `(citations?|sources?|references?|claims?)`
summaryCopulas = `((are|were|have\s+been|all)\s+)*`
summaryVerbs = `(logged|recorded|cited|saved|noted|captured|filed)`
summaryArticle = `((all|the)\s+)*` // star, not ?: "Logged all the citations."
)
var summaryCloserRe = regexp.MustCompile(`(?i)^` + summaryLead + summaryPreface +
Review

🟠 summaryCloserRe regex can false-positive on prose where [.] is not end-of-sentence

error-handling · flagged by 1 model

  • agent/finalize.go:139-141 — The summaryCloserRe regex matches a prefix ending in [.!] but does not enforce that the sentence actually terminates there (whitespace or end-of-string). Because MatchString returns true for any prefix match, a string like "Citations are logged...and then more text" matches even though the model is continuing prose about citations rather than producing a bookkeeping ack. This false positive could cause legitimate terminal text to be misclassified as a summ…

🪰 Gadfly · advisory

🟠 **summaryCloserRe regex can false-positive on prose where [.] is not end-of-sentence** _error-handling · flagged by 1 model_ - `agent/finalize.go:139-141` — The `summaryCloserRe` regex matches a prefix ending in `[.!]` but does not enforce that the sentence actually terminates there (whitespace or end-of-string). Because `MatchString` returns true for any prefix match, a string like `"Citations are logged...and then more text"` matches even though the model is continuing prose about citations rather than producing a bookkeeping ack. This false positive could cause legitimate terminal text to be misclassified as a summ… <sub>🪰 Gadfly · advisory</sub>
`(` + summaryArticle + summaryNouns + `\s+` + summaryCopulas + summaryVerbs +
`|logged\s+` + summaryArticle + summaryNouns + `)[.!]`)
// preambleRe matches intent-announcing prefixes ("Let me search...", "I'll // preambleRe matches intent-announcing prefixes ("Let me search...", "I'll
// check...") so a preamble is never mistaken for the answer during recovery. // check...") so a preamble is never mistaken for the answer during recovery.
@@ -165,9 +185,10 @@ const (
// "Source:" and cites a URL mid-sentence is not mistaken for a bare list. // "Source:" and cites a URL mid-sentence is not mistaken for a bare list.
citationDominatedDivisor = 3 citationDominatedDivisor = 3
// summaryCloserMaxChars bounds a summary closer: room for the ack sentence // summaryCloserMaxChars bounds a summary closer: room for the ack sentence
// plus a couple of compression sentences (the b3cb9ee9 closer was 151). // plus a couple of compression sentences (the b3cb9ee9 closer was 153
// Beyond this the "short version" is substantial enough that replacing it // bytes — Go len(), which is what every threshold here compares). Beyond
// risks losing content the front-loaded turn never had. // this the "short version" is substantial enough that replacing it risks
// losing content the front-loaded turn never had.
summaryCloserMaxChars = 300 summaryCloserMaxChars = 300
) )
@@ -230,6 +251,16 @@ func lastSubstantiveAssistantText(msgs []llm.Message, terminal string, mode reco
tt := strings.TrimSpace(terminal) tt := strings.TrimSpace(terminal)
for i := len(msgs) - 1; i >= 0; i-- { for i := len(msgs) - 1; i >= 0; i-- {
m := msgs[i] m := msgs[i]
if mode == modeSummary && m.Role == llm.RoleUser {
// A summary closer compresses THIS turn's front-loaded answer, so
// the scan must not cross into an earlier question: once the dwarf
// ratio has rejected the current turn's text, walking further back
// would resurrect a stale answer to a DIFFERENT question — strictly
// worse than keeping the closer. (A mid-run steer message is also a
// user-role boundary; recovery then fails closed, which is fine.)
// The other modes keep their historical unbounded scan.
break
}
if m.Role != llm.RoleAssistant { if m.Role != llm.RoleAssistant {
continue continue
} }
@@ -261,7 +292,8 @@ func lastSubstantiveAssistantText(msgs []llm.Message, terminal string, mode reco
// addendum is not a rival answer, so its length is irrelevant; a summary // addendum is not a rival answer, so its length is irrelevant; a summary
// closer already proved the ratio above). // closer already proved the ratio above).
func isSubstantiveAnswer(txt, terminal string, mode recoveryMode) bool { func isSubstantiveAnswer(txt, terminal string, mode recoveryMode) bool {
if mode == modeSummary && len(txt) < recoverRatio*len(terminal) { dwarfs := len(txt) >= recoverRatio*len(terminal)
if mode == modeSummary && !dwarfs {
return false return false
} }
if len(txt) >= recoverMinChars { if len(txt) >= recoverMinChars {
@@ -270,5 +302,5 @@ func isSubstantiveAnswer(txt, terminal string, mode recoveryMode) bool {
if len(txt) < recoverFloorChars || preambleRe.MatchString(txt) { if len(txt) < recoverFloorChars || preambleRe.MatchString(txt) {
return false return false
} }
return mode != modeBackRef || len(txt) >= recoverRatio*len(terminal) return mode != modeBackRef || dwarfs
} }
+58 -2
View File
@@ -73,8 +73,9 @@ func TestIsCitationsOnly(t *testing.T) {
} }
// b3cb9ee9Closer is the verbatim terminal turn from mort run b3cb9ee9: a // b3cb9ee9Closer is the verbatim terminal turn from mort run b3cb9ee9: a
// 2,089-char answer was front-loaded into the cite-call turn and this 151-char // 2,089-char answer was front-loaded into the cite-call turn and this 153-byte
// compression was all that got delivered. // compression (151 runes — the em dash is 3 bytes, and byte length is what the
// thresholds compare) was all that got delivered.
const b3cb9ee9Closer = "Citations are logged. Short version: the bulk of that ~$64M was AIPAC and dark-money super PACs, not the party committees — and it still wasn't enough." const b3cb9ee9Closer = "Citations are logged. Short version: the bulk of that ~$64M was AIPAC and dark-money super PACs, not the party committees — and it still wasn't enough."
func TestIsSummaryCloser(t *testing.T) { func TestIsSummaryCloser(t *testing.T) {
@@ -91,6 +92,8 @@ func TestIsSummaryCloser(t *testing.T) {
{"done-prefix", "Done — citations logged.", true}, {"done-prefix", "Done — citations logged.", true},
{"ack-then-tldr", "Sources have been recorded! TL;DR: the GPU was the bottleneck.", true}, {"ack-then-tldr", "Sources have been recorded! TL;DR: the GPU was the bottleneck.", true},
{"references-noted", "References noted. In short: yes, it ships Tuesday.", true}, {"references-noted", "References noted. In short: yes, it ships Tuesday.", true},
{"plus-list-marker", "+ Citations are logged.", true},
{"logged-all-the", "Logged all the citations.", true},
{"empty", "", false}, {"empty", "", false},
{"ack-continues-midsentence", "The citations are recorded in the court transcript, which shows the filing dates.", false}, {"ack-continues-midsentence", "The citations are recorded in the court transcript, which shows the filing dates.", false},
1
@@ -141,6 +144,9 @@ func TestFinalOutput(t *testing.T) {
// A >=200-byte real answer that merely OPENS with a conversational word // A >=200-byte real answer that merely OPENS with a conversational word
// ("Sure,"). The preamble filter must NOT veto it (gadfly regression guard). // ("Sure,"). The preamble filter must NOT veto it (gadfly regression guard).
longConversationalAnswer := "Sure, here's the rundown: it currently sells for about $2,700 used on eBay, typically $2,400 to $2,900 depending on condition and bundle, with the sealed Founders Edition commanding the top of that range while used AIB cards go a bit lower." longConversationalAnswer := "Sure, here's the rundown: it currently sells for about $2,700 used on eBay, typically $2,400 to $2,900 depending on condition and bundle, with the sealed Founders Edition commanding the top of that range while used AIB cards go a bit lower."
// Matches BOTH the summary ack and backRefRe, within the 120-byte weak cap,
// and long enough (>~92 bytes) that longAnswer would fail the summary bar.
bothMatchCloser := "Citations are logged. As I mentioned above, the full detail on the money sources is in my earlier message."
tests := []struct { tests := []struct {
name string name string
@@ -347,6 +353,56 @@ func TestFinalOutput(t *testing.T) {
terminal: b3cb9ee9Closer, terminal: b3cb9ee9Closer,
want: b3cb9ee9Closer, want: b3cb9ee9Closer,
}, },
{
// Gadfly (opus, correctness): the modeSummary scan must stop at the
// most recent user message. Here the current turn's answer sits in
// the 1x-3x band (rejected by the ratio) while a dwarfing answer to
// a DIFFERENT question sits in history — resurrecting it would be
// strictly worse than keeping the closer.
name: "summary closer never resurrects a stale answer across the user boundary",
msgs: []llm.Message{
llm.UserText("earlier, unrelated question?"),
asst(hugeAnswer),
llm.UserText("q?"),
asst(conciseAnswer, cite...),
llm.ToolResultsMessage(llm.ToolResult{ID: "c1", Name: "cite", Content: "ok"}),
asst(b3cb9ee9Closer),
},
terminal: b3cb9ee9Closer,
want: b3cb9ee9Closer,
},
{
// The boundary must not break the legitimate multi-turn case: the
// dwarfing front-loaded answer in THIS turn's window is recovered
// even with history behind it.
name: "summary closer recovery still works with history present",
msgs: []llm.Message{
llm.UserText("earlier, unrelated question?"),
asst(longAnswer),
llm.UserText("q?"),
asst(hugeAnswer, cite...),
llm.ToolResultsMessage(llm.ToolResult{ID: "c1", Name: "cite", Content: "ok"}),
asst(b3cb9ee9Closer),
},
terminal: b3cb9ee9Closer,
want: hugeAnswer,
},
{
// Gadfly (opus, error-handling): a closer matching BOTH the ack
// shape and a back-reference carries no answer content, so the
// back-ref test must win and the ordinary recovery bar apply —
// under the summary bar this ~106-byte terminal would demand a
// ~318-byte prior and wrongly keep the closer over longAnswer.
name: "back-reference wins over the summary ack when both match",
msgs: []llm.Message{
llm.UserText("q?"),
asst(longAnswer, cite...),
llm.ToolResultsMessage(llm.ToolResult{ID: "c1", Name: "cite", Content: "ok"}),
asst(bothMatchCloser),
},
terminal: bothMatchCloser,
want: longAnswer,
},
} }
for _, tc := range tests { for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) { t.Run(tc.name, func(t *testing.T) {