diff --git a/internal/llm/checkpoint.go b/internal/llm/checkpoint.go index f1a622c..8b9e3fc 100644 --- a/internal/llm/checkpoint.go +++ b/internal/llm/checkpoint.go @@ -89,6 +89,14 @@ func ParseCheckpoint(raw string) ([]RawSuggestion, error) { if s.Original == "" { continue } + // Drop no-ops: the model sometimes "flags" a correct sentence and echoes + // it verbatim as the replacement, producing a card whose before/after are + // identical and whose explanation says it's already fine — a suggestion + // that suggests nothing. Awareness-only families (voice) carry an empty + // replacement, so this only ever fires on the edit families. + if s.Replacement != "" && s.Original == s.Replacement { + continue + } out = append(out, s) } return out, nil diff --git a/internal/llm/checkpoint_test.go b/internal/llm/checkpoint_test.go index b42c7ba..bddc6db 100644 --- a/internal/llm/checkpoint_test.go +++ b/internal/llm/checkpoint_test.go @@ -37,6 +37,25 @@ func TestParseCheckpoint(t *testing.T) { raw: "I could not find any issues!", wantErr: true, }, + { + // A correct sentence echoed verbatim as its own replacement is a card + // that suggests nothing — dropped. The second item is a real edit. + name: "drops no-op where replacement equals original", + raw: `{"suggestions":[{"original":"It wasn't the most graceful morning.","replacement":"It wasn't the most graceful morning.","explanation":"used correctly here","type":"idiom"},{"original":"teh","replacement":"the","explanation":"typo","type":"grammar"}]}`, + want: 1, + }, + { + // Whitespace-only difference is still a no-op once both are trimmed. + name: "drops no-op after whitespace trim", + raw: `{"suggestions":[{"original":"the cat sat","replacement":" the cat sat ","explanation":"already fine","type":"grammar"}]}`, + want: 0, + }, + { + // Awareness-only voice flags carry an empty replacement and must survive. + name: "keeps awareness-only flag with empty replacement", + raw: `{"suggestions":[{"original":"a long run-on passage","replacement":"","explanation":"this drifts from your voice","type":"voice"}]}`, + want: 1, + }, { name: "braces inside string values", raw: `{"suggestions":[{"original":"use {x}","replacement":"use x","explanation":"drop the braces","type":"clarity"}]}`, diff --git a/internal/suggestions/handlers.go b/internal/suggestions/handlers.go index 3521b0c..2d078d1 100644 --- a/internal/suggestions/handlers.go +++ b/internal/suggestions/handlers.go @@ -155,7 +155,7 @@ func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) er return err } - actioned, err := actionedKeys(tx, docID) + sup, err := buildSuppressor(tx, docID) if err != nil { return err } @@ -164,7 +164,7 @@ func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) er if f.From < 0 || f.To <= f.From || strings.TrimSpace(f.Original) == "" { continue // malformed span — the client re-anchors by string anyway } - if _, seen := actioned[suggestionKey(f.Original, f.Replacement)]; seen { + if sup.suppressed(f.Original, f.Replacement) { continue } if _, err := tx.Exec( @@ -289,13 +289,12 @@ var ( // fresh batch in a single transaction. Accepted/rejected suggestions and the // other family's pending rows are left untouched. // -// Suggestions the user already accepted or dismissed are suppressed from the -// fresh batch: the model has no memory between passes, so without this it would -// re-propose the identical edit on the very next checkpoint — re-nagging a -// sentence the user already resolved. This matters most when an accept silently -// no-ops (the `original` text couldn't be anchored in the editor, so the doc -// never changed): the sentence is unaltered, yet the user shouldn't see the same -// card again. +// Suggestions touching a sentence the user already settled are suppressed from +// the fresh batch (see suppressor): not just the identical edit re-proposed, but +// reversals and re-polishing of the model's own just-accepted output — the +// "fickle, keeps going back and forth on a few sentences" behavior. The model has +// no memory between passes, so without this it re-opens resolved sentences every +// checkpoint. func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggestion, scope pendingScope) error { tx, err := h.DB.Begin() if err != nil { @@ -310,13 +309,13 @@ func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggest return err } - actioned, err := actionedKeys(tx, docID) + sup, err := buildSuppressor(tx, docID) if err != nil { return err } for _, s := range raw { - if _, seen := actioned[suggestionKey(s.Original, s.Replacement)]; seen { + if sup.suppressed(s.Original, s.Replacement) { continue } typ := scope.forceType @@ -336,37 +335,131 @@ func (h *Handler) replacePending(docID, contentText string, raw []llm.RawSuggest return tx.Commit() } -// actionedKeys returns the set of original→replacement keys the user has already -// accepted or rejected for this document, so a fresh checkpoint can skip -// re-proposing them. Keyed on the trimmed original+replacement pair, matching the -// normalization ParseCheckpoint applies, so a genuinely different edit on the same -// sentence is not suppressed. -func actionedKeys(tx *sql.Tx, docID string) (map[string]struct{}, error) { +// dedupQuoteReplacer folds every straight/curly single- and double-quote variant +// (and backtick/acute accent) onto one canonical character. The editor and the +// model both rewrite quotes between passes — a sentence accepted with "…" comes +// back flagged with '…' — so without folding, byte-identical text reads as a +// different edit and the suppression below misses it. (This normalization is for +// dedup ONLY; the frontend still anchors on the verbatim `original`.) +var dedupQuoteReplacer = strings.NewReplacer( + "‘", "'", "’", "'", "‚", "'", "‛", "'", // single curly + "“", "'", "”", "'", "„", "'", "″", "'", // double curly + "\"", "'", "`", "'", "´", "'", // straight double, backtick, acute +) + +// normalizeForDedup canonicalizes a string for suppression comparisons: quotes +// folded (above) and runs of whitespace collapsed to single spaces (so a reflowed +// paragraph still matches). Used only to decide what to suppress, never to alter +// stored or rendered text. +func normalizeForDedup(s string) string { + return strings.Join(strings.Fields(dedupQuoteReplacer.Replace(s)), " ") +} + +// suppressor decides which fresh suggestions to drop because the user has already +// settled the sentence they touch. The model has no memory between passes, so on +// every checkpoint it re-examines the current text and proposes edits — including +// ones that re-open a sentence the user already resolved. Three families of those +// are suppressed (all compared under normalizeForDedup): +// +// - pairs: the identical edit, re-proposed verbatim (the original "accept it, +// then it nags again" case; also covers a silent no-op accept that left the +// text unchanged). +// - actionedOrig: any edit whose original is a span the user already accepted or +// dismissed an edit on — "you already decided about this exact sentence." +// - acceptedRepl: any edit whose original is text the user accepted AS a +// replacement — i.e. the model re-touching its own just-accepted output, which +// is how the reversals and endless re-polishing arise (accept "due to the +// rain", next pass proposes changing "due to the rain" back). Guarded to +// multi-word spans so word-level fixes aren't swept up as collateral. +// - acceptedReplList holds the same accepted replacements for a containment +// check: the model evades the exact acceptedRepl match by re-flagging a +// *sub-clause* of an accepted sentence (flag "she was…due to the rain" instead +// of the whole sentence). When one of the new original / an accepted +// replacement contains the other and the shorter side is substantial +// (>= minContainWords words), it's the same settled span and is dropped. +type suppressor struct { + pairs map[string]struct{} + actionedOrig map[string]struct{} + acceptedRepl map[string]struct{} + acceptedReplList []string +} + +// minContainWords is the floor for the containment check: the shorter of the two +// spans must be at least this many words before a substring relationship counts +// as "the same settled text." High enough that an incidental common phrase ("the +// rain") can't suppress an unrelated sentence, low enough to catch a re-flagged +// clause. +const minContainWords = 4 + +// suppressed reports whether a fresh suggestion should be dropped as already +// settled. An empty original is never suppressed here (it can't anchor anyway and +// is dropped upstream). +func (s suppressor) suppressed(original, replacement string) bool { + o := normalizeForDedup(original) + if o == "" { + return false + } + if _, ok := s.pairs[o+"\x00"+normalizeForDedup(replacement)]; ok { + return true + } + if _, ok := s.actionedOrig[o]; ok { + return true + } + if strings.ContainsRune(o, ' ') { + if _, ok := s.acceptedRepl[o]; ok { + return true + } + } + // Containment: the model re-flagged a sub-clause of (or a window around) an + // accepted span. Suppress when one contains the other and the shorter side is + // a substantial multi-word run. + for _, r := range s.acceptedReplList { + shorter, longer := o, r + if len(r) < len(o) { + shorter, longer = r, o + } + if len(strings.Fields(shorter)) >= minContainWords && strings.Contains(longer, shorter) { + return true + } + } + return false +} + +// buildSuppressor loads the document's accepted/rejected edits and indexes them +// into the three suppression families described on suppressor. +func buildSuppressor(tx *sql.Tx, docID string) (suppressor, error) { rows, err := tx.Query( - `SELECT original, replacement FROM suggestions + `SELECT original, replacement, status FROM suggestions WHERE doc_id = ? AND status IN (?, ?)`, docID, db.SuggestionStatusAccepted, db.SuggestionStatusRejected, ) if err != nil { - return nil, err + return suppressor{}, err } defer rows.Close() - keys := make(map[string]struct{}) - for rows.Next() { - var original, replacement string - if err := rows.Scan(&original, &replacement); err != nil { - return nil, err - } - keys[suggestionKey(original, replacement)] = struct{}{} + s := suppressor{ + pairs: make(map[string]struct{}), + actionedOrig: make(map[string]struct{}), + acceptedRepl: make(map[string]struct{}), } - return keys, rows.Err() -} - -// suggestionKey is the dedup identity for a suggestion: its trimmed original and -// replacement text. Two suggestions with the same key are "the same edit." -func suggestionKey(original, replacement string) string { - return strings.TrimSpace(original) + "\x00" + strings.TrimSpace(replacement) + for rows.Next() { + var original, replacement, status string + if err := rows.Scan(&original, &replacement, &status); err != nil { + return suppressor{}, err + } + o := normalizeForDedup(original) + r := normalizeForDedup(replacement) + s.pairs[o+"\x00"+r] = struct{}{} + if o != "" { + s.actionedOrig[o] = struct{}{} + } + if status == db.SuggestionStatusAccepted && r != "" { + s.acceptedRepl[r] = struct{}{} + s.acceptedReplList = append(s.acceptedReplList, r) + } + } + return s, rows.Err() } // listForDoc returns the document's current pending suggestions (used when the diff --git a/internal/suggestions/handlers_test.go b/internal/suggestions/handlers_test.go index 84773a4..6a00c4e 100644 --- a/internal/suggestions/handlers_test.go +++ b/internal/suggestions/handlers_test.go @@ -165,6 +165,48 @@ func TestResolvedSuggestionsNotReproposed(t *testing.T) { } } +// TestFickleEditsSuppressed proves the suppressor kills the "keeps going back and +// forth on a few sentences" behavior seen live on the Missing Key doc: a later +// pass that (a) reverses an edit the user just accepted — even with the editor's +// double→single quote churn that defeats a byte-exact match — or (b) re-polishes +// the model's own accepted output. A genuinely new edit on a fresh sentence still +// survives. +func TestFickleEditsSuppressed(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"He left \"early,\" because of the rain.","replacement":"He left \"early,\" due to the rain.","explanation":"smoother","type":"phrasing"}, + {"original":"The cat always have a calm face.","replacement":"The cat always has a calm face.","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var got []db.Suggestion + _ = json.Unmarshal(rec.Body.Bytes(), &got) + if len(got) != 2 { + t.Fatalf("first pass: want 2, got %d", len(got)) + } + for _, s := range got { + do(t, srv, http.MethodPost, "/suggestions/"+s.ID+"/accept", "") + } + + // Reversal of the first accept (note the " → ' quote churn) and a re-polish of + // the second accept must both be dropped; only the unrelated edit survives. + client.response = `{"suggestions":[ + {"original":"He left 'early,' due to the rain.","replacement":"He left 'early,' because of the rain.","explanation":"reverts the accepted edit","type":"phrasing"}, + {"original":"The cat always has a calm face.","replacement":"The cat always seems to have a calm face.","explanation":"re-polishes accepted output","type":"phrasing"}, + {"original":"due to the rain","replacement":"because of the rain","explanation":"sub-clause reversal of the accepted span","type":"phrasing"}, + {"original":"She drived home.","replacement":"She drove home.","explanation":"past tense","type":"grammar"} + ]}` + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var again []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &again); err != nil { + t.Fatalf("decode: %v", err) + } + if len(again) != 1 || again[0].Original != "She drived home." { + t.Fatalf("want only the new edit to survive, got %d: %+v", len(again), again) + } +} + // TestVoicePassCoexists proves the voice pass and the grammar checkpoint own // disjoint pending families: running one never wipes the other's flags, and // each endpoint returns the unified pending set.