Merge feat/calm-suggestions: drop no-op suggestions + suppress fickle re-edits

Claude-Session: https://claude.ai/code/session_016Yr6jELuRc7hyzYLccQKZd

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

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"}]}`,

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{})
+	s := suppressor{
+		pairs:        make(map[string]struct{}),
+		actionedOrig: make(map[string]struct{}),
+		acceptedRepl: make(map[string]struct{}),
+	}
 	for rows.Next() {
-		var original, replacement string
-		if err := rows.Scan(&original, &replacement); err != nil {
-			return nil, err
+		var original, replacement, status string
+		if err := rows.Scan(&original, &replacement, &status); err != nil {
+			return suppressor{}, err
 		}
-		keys[suggestionKey(original, replacement)] = struct{}{}
+		o := normalizeForDedup(original)
+		r := normalizeForDedup(replacement)
+		s.pairs[o+"\x00"+r] = struct{}{}
+		if o != "" {
+			s.actionedOrig[o] = struct{}{}
 		}
-	return keys, rows.Err()
+		if status == db.SuggestionStatusAccepted && r != "" {
+			s.acceptedRepl[r] = struct{}{}
+			s.acceptedReplList = append(s.acceptedReplList, r)
 		}
-
-// 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)
+	}
+	return s, rows.Err()
 }
 
 // listForDoc returns the document's current pending suggestions (used when the

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.