steve/majordomo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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
Conversation 4 Commits 3 Files Changed 2
+24 -14
2 changed files with 24 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 1bbbdaa1e5 - Show all commits
agent/finalize.go
+14 -4
View File
@@ -52,7 +52,7 @@ func finalOutput(msgs []llm.Message, terminal string) string {
case isCitationsOnly(terminal): case isCitationsOnly(terminal):
mode = modeCitations mode = modeCitations
case isWeakFinal(terminal): case isWeakFinal(terminal):
gitea-actions commented 2026-08-06 00:27:21 +00:00
Review
Copy Link
Copy Source

⚪ 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 mode = modeBackRef
case isSummaryCloser(terminal): case isSummaryCloser(terminal):
mode = modeSummary mode = modeSummary
default: default:
@@ -128,15 +128,17 @@ var backRefRe = regexp.MustCompile(`(?i)(already answered|see above|as (i )?(sai
// behavior (fail closed). Assembled from named fragments so the alternations // behavior (fail closed). Assembled from named fragments so the alternations
// stay legible and extendable. // stay legible and extendable.
const ( const (
gitea-actions commented 2026-08-06 00:27:21 +00:00
Review
Copy Link
Copy Source

🟡 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 summaryPreface = `((done|all set|ok(ay)?)[\s,.!:—-]+)?` // optional "Done —" style opener
summaryNouns = `(citations?|sources?|references?|claims?)` summaryNouns = `(citations?|sources?|references?|claims?)`
// "all" appears here AND in summaryArticle on purpose: as a quantifier
// between noun and verb ("Citations all logged.") and as a determiner
// before the noun ("All claims cited.", "Logged all the citations.").
summaryCopulas = `((are|were|have\s+been|all)\s+)*` summaryCopulas = `((are|were|have\s+been|all)\s+)*`
summaryVerbs = `(logged|recorded|cited|saved|noted|captured|filed)` summaryVerbs = `(logged|recorded|cited|saved|noted|captured|filed)`
summaryArticle = `((all|the)\s+)*` // star, not ?: "Logged all the citations." summaryArticle = `((all|the)\s+)*` // star, not ?: "Logged all the citations."
) )
gitea-actions commented 2026-08-06 00:27:21 +00:00
Review
Copy Link
Copy Source

🟠 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>
var summaryCloserRe = regexp.MustCompile(`(?i)^` + summaryLead + summaryPreface + var summaryCloserRe = regexp.MustCompile(`(?i)^` + leadingMarkers + summaryPreface +
`(` + summaryArticle + summaryNouns + `\s+` + summaryCopulas + summaryVerbs + `(` + summaryArticle + summaryNouns + `\s+` + summaryCopulas + summaryVerbs +
`|logged\s+` + summaryArticle + summaryNouns + `)[.!]`) `|logged\s+` + summaryArticle + summaryNouns + `)[.!]`)
@@ -153,7 +155,15 @@ var preambleRe = regexp.MustCompile(`(?i)^(let me|let'?s|i'?ll|i will|first[, ]|
// the colon/dash separator. Anchored at ^ so a normal answer that merely // the colon/dash separator. Anchored at ^ so a normal answer that merely
// mentions "sources" mid-sentence, or ends with a "Sources:" section AFTER its // mentions "sources" mid-sentence, or ends with a "Sources:" section AFTER its
// prose, is never matched. // prose, is never matched.
var citationLabelRe = regexp.MustCompile(`(?i)^[\s>#*_+-]*(sources?|references?|citations?|works cited|further reading)\b[\s*_]*[:：\-—]`) var citationLabelRe = regexp.MustCompile(`(?i)^` + leadingMarkers +
`(sources?|references?|citations?|works cited|further reading)\b[\s*_]*[:：\-—]`)
// leadingMarkers tolerates markdown noise before a label: emphasis (*, _),
// list (-, +, *), block-quote (>), and ATX-heading (#) markers, with their
// whitespace. Shared by citationLabelRe and summaryCloserRe so the two
// classifiers cannot drift apart (the first draft of the summary class
// dropped '+' by hand-copying this set).
const leadingMarkers = `[\s>#*_+-]*`
// linkRe matches a whole markdown link "[label](url)" or a bare URL. Used both // linkRe matches a whole markdown link "[label](url)" or a bare URL. Used both
// to require that a citations terminal carries at least one link and to strip // to require that a citations terminal carries at least one link and to strip
agent/finalize_test.go
+10 -10
View File
1
 @@ -354,11 +354,11 @@ func TestFinalOutput(t *testing.T) {
want: b3cb9ee9Closer, want: b3cb9ee9Closer,
}, },
{ {
// Gadfly (opus, correctness): the modeSummary scan must stop at the // The modeSummary scan must stop at the most recent user message.
gitea-actions commented 2026-08-06 00:27:21 +00:00
Outdated
Review
Copy Link
Copy Source

🟡 External review reference embedded in permanent test comment

maintainability · flagged by 1 model

  • agent/finalize_test.go:357,391 — Test-case comments embed external review references (// Gadfly (opus, correctness):, // Gadfly (opus, error-handling):). These will be meaningless noise to future maintainers who don't have access to that review context. Replace them with plain prose describing the invariant being guarded.

🪰 Gadfly · advisory

🟡 **External review reference embedded in permanent test comment** _maintainability · flagged by 1 model_ - `agent/finalize_test.go:357,391` — Test-case comments embed external review references (`// Gadfly (opus, correctness):`, `// Gadfly (opus, error-handling):`). These will be meaningless noise to future maintainers who don't have access to that review context. Replace them with plain prose describing the invariant being guarded. <sub>🪰 Gadfly · advisory</sub>
// most recent user message. Here the current turn's answer sits in // Here the current turn's answer sits in the 1x-3x band (rejected
// the 1x-3x band (rejected by the ratio) while a dwarfing answer to // by the ratio) while a dwarfing answer to a DIFFERENT question
// a DIFFERENT question sits in history — resurrecting it would be // sits in history — resurrecting it would be strictly worse than
// strictly worse than keeping the closer. // keeping the closer.
name: "summary closer never resurrects a stale answer across the user boundary", name: "summary closer never resurrects a stale answer across the user boundary",
msgs: []llm.Message{ msgs: []llm.Message{
llm.UserText("earlier, unrelated question?"), llm.UserText("earlier, unrelated question?"),
@@ -388,11 +388,11 @@ func TestFinalOutput(t *testing.T) {
want: hugeAnswer, want: hugeAnswer,
}, },
{ {
// Gadfly (opus, error-handling): a closer matching BOTH the ack // A closer matching BOTH the ack shape and a back-reference
// shape and a back-reference carries no answer content, so the // carries no answer content, so the back-ref test must win and the
// back-ref test must win and the ordinary recovery bar apply — // ordinary recovery bar apply — under the summary bar this
// under the summary bar this ~106-byte terminal would demand a // ~106-byte terminal would demand a ~318-byte prior and wrongly
// ~318-byte prior and wrongly keep the closer over longAnswer. // keep the closer over longAnswer.
name: "back-reference wins over the summary ack when both match", name: "back-reference wins over the summary ack when both match",
msgs: []llm.Message{ msgs: []llm.Message{
llm.UserText("q?"), llm.UserText("q?"),