diff --git a/UX_REVIEW_2026-07-27.md b/UX_REVIEW_2026-07-27.md index 238d9d3..840c079 100644 --- a/UX_REVIEW_2026-07-27.md +++ b/UX_REVIEW_2026-07-27.md @@ -143,6 +143,81 @@ instability, doubles the pause after each accept, and burns qwen3.5 tokens. or position of any other card; re-check traffic after a one-sentence edit contains only that sentence's chunk; explanations are stable across rounds. +### 2 — DONE (fourth session). Server-side; the client needed nothing. + +Both halves shipped, and they turned out to be one idea. The root cause of +the vanish/reappear was structural: **every pass deleted its whole family +and re-inserted it**, so each round minted new row ids. The rail keys its +cards on `suggestion.id`, so a full remount was guaranteed — new id, new +`created_at` (hence the re-fired arrival chime), and a freshly-worded +explanation from a model that re-reasons every time it's asked. + +**Implemented:** + +- `chunk.go` — splits the document into sentences and hashes each. Newlines + always break; ASCII terminators need trailing whitespace (so `3.50` and + `Ms.` stay whole); `。!?` break outright, since Chinese runs sentences + together with no space and she writes both languages in one document. The + hash normalizes quotes and whitespace runs through the existing + `normalizeForDedup`, so the editor's constant quote rewriting and a + reflowed paragraph cost nothing. Identity is the hash, not the position — + insert a paragraph at the top and every sentence below keeps its cards. +- `reconcile.go` — passes now *reconcile* rather than replace. A row on a + sentence this pass didn't ask about is kept untouched; a row whose + sentence is gone is dropped; a row on a sentence that was re-read survives + only if the model proposed the same edit again, keeping its id, + `created_at` and its **original explanation**. Re-proposals are matched on + `(original, replacement)` normalized — not on type, so a re-labelled edit + keeps the label she's already reading. +- `checked_chunks` (migration `0014`) records which sentences a family has + read. The grammar checkpoint asks only about the difference. **When + nothing changed it doesn't call the model at all** — and doesn't consume + its rate-limit slot, so an idle check can't throttle the next real edit. +- The tone is folded into a sentence's hash, so switching doc type still + re-reads every line: the same sentence gets different advice as an + academic essay than as a journal entry, and cached advice was written for + the old register. +- `replaceMechanics` reconciles too. This mattered more than expected: the + rule pack fires 250 ms after a keystroke (item 3b), so it was re-minting + every local card's id several times a sentence. + +**Deliberately not done:** + +- Only the grammar checkpoint is chunked. Voice is a property of the + document as a whole — a sentence isn't inconsistent with itself — and the + collocation coach is a button she presses asking for a fresh read. Both + still read everything, but both now reconcile, so they keep their ids. +- No client change. With stable ids the existing code already does what the + item asked for: the rail keeps its card DOM, an expanded card survives a + re-check, and the chime (which keys on id) stops re-firing for advice she + is already reading. The one-card optimistic removal on accept was already + there. + +**Sentences the model can't be trusted to have read.** Two guards the plan +didn't anticipate, both found while writing the tests: a finding is +attributed to a sentence the model was *actually shown* before falling back +to the whole document (a short span like "the the" can occur twice, and +crediting the cached copy would drop it); and a cached row whose quoted span +no longer matches byte-for-byte is dropped *and* its sentence re-opened, +rather than caching advice the frontend can't anchor. + +**Verified on the running binary**, not just in tests — per the handoff's +own advice. Against a stand-in model server: three checks over a two- +sentence document, editing only the second. The model received exactly +`She goes to market yesterday.` and never saw the first sentence; the +untouched card kept its id and its first explanation across all three +rounds; the fixed sentence's card was dropped; the idle re-check made zero +model calls. Mechanics identity confirmed the same way (a finding kept its +row id while its span moved). + +Coverage: `chunk_test.go` (splitting, CJK, decimals, cosmetic churn) and +`stability_test.go` (untouched cards keep id + explanation, unchanged +document skips the model, deleted sentence drops its card, tone change +re-opens everything, mechanics rows keep identity). Two existing tests +changed contract deliberately — `TestFickleEditsSuppressed` and +`TestCollocationPassCoexists` both re-checked a document nobody had edited, +which is now a no-op; they edit the text between passes, as she always does. + ## 3. Perceived latency: mask the LLM round-trip Measured ~8–15 s from typing-stop to cards, with only a small "Checking…" @@ -338,6 +413,14 @@ reproducible) and item 5's original premise (re-scoped, much cheaper now). *(Third session: item 3b done — see the subsection under item 3. Item 3's incremental-surfacing half remains. Untouched: 2, 4, 6, 7, 8.)* +*(Fourth session: item 2 done — see the subsection under it. **Neither 3b +nor 2 is deployed yet**: both sit on unmerged topic branches +(`feat/instant-local-rules`, then `feat/stable-suggestions` stacked on it) +and `main` is still at `ba06d90`. Untouched: 4, 6, 7, 8. Item 3's +incremental-surfacing half is now cheap — the chunking it was waiting on +exists — but it needs streaming, which the current `/check` shape doesn't +do.)* + **Suggested next:** item 3b, the instant local rules layer — but it is **largely already built, in `main`**, and the item as written doesn't know that. Before writing any rules engine, read: diff --git a/internal/db/db.go b/internal/db/db.go index 8e91c31..b5ea7e1 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -497,6 +497,32 @@ CREATE INDEX idx_suggestions_resolved ON suggestions(status, resolved_at); stmt: ` ALTER TABLE suggestions ADD COLUMN source TEXT NOT NULL DEFAULT 'llm'; UPDATE suggestions SET source = 'local' WHERE type = 'mechanics'; +`, + }, + { + // Sentence-level identity, so a re-check stops regenerating the world. + // Every pass used to delete its whole family and re-insert it, which + // meant accepting one edit gave every other card a new id and a newly + // worded explanation — the rail visibly emptied and refilled, and the + // model was asked again about sentences nobody had touched. + // + // `chunk_hash` records which sentence a suggestion belongs to, and + // checked_chunks records which sentences a family has already read. A + // re-check then asks only about the difference and keeps the rest of + // the rows exactly as they are, id and wording included. + // + // Existing rows get '' — "sentence unknown", which reads as in-play, so + // they are simply reconciled on the next pass like any fresh finding. + name: "0014_suggestion_chunk_hash", + stmt: ` +ALTER TABLE suggestions ADD COLUMN chunk_hash TEXT NOT NULL DEFAULT ''; + +CREATE TABLE checked_chunks ( + doc_id TEXT NOT NULL REFERENCES documents(id) ON DELETE CASCADE, + family TEXT NOT NULL, + hash TEXT NOT NULL, + PRIMARY KEY (doc_id, family, hash) +); `, }, } diff --git a/internal/suggestions/chunk.go b/internal/suggestions/chunk.go new file mode 100644 index 0000000..611d9a2 --- /dev/null +++ b/internal/suggestions/chunk.go @@ -0,0 +1,145 @@ +package suggestions + +import ( + "crypto/sha256" + "encoding/hex" + "strings" + "unicode" +) + +// Chunking splits a document into sentence-sized units so a re-check can ask the +// model only about the sentences that actually changed. Accepting one edit used +// to re-run the whole document: every card vanished, came back with a new id and +// a freshly-worded explanation, and spans re-merged into different shapes. The +// sentences she didn't touch have nothing new to say about themselves, so their +// suggestions are simply kept (see reconcilePending). +// +// A chunk's identity is its hash, not its position — she inserts a paragraph at +// the top and every sentence below keeps its suggestions. + +// chunk is one sentence of the document, with the hash that identifies it. +type chunk struct { + text string + hash string +} + +// asciiTerminators end a sentence only when whitespace (or the end of the text) +// follows, so "3.5" and "Ms." don't split mid-word — a wrong split costs only a +// slightly smaller chunk, but a split inside a number would churn its hash on +// every keystroke around it. +const asciiTerminators = ".!?" + +// cjkTerminators end a sentence outright: Chinese runs sentences together with +// no space after 。, and she writes in both languages in one document. +const cjkTerminators = "。!?" + +// closers are swallowed into the sentence they close, so the quote mark travels +// with the sentence rather than opening the next one. +const closers = `)]}"'’”」』` + +// splitChunks divides text into sentences, dropping whitespace-only runs. +// Newlines always break a chunk, so a list or a line of dialogue is its own unit. +// +// `salt` distinguishes two *readings* of the same sentence. The grammar +// checkpoint's advice depends on the document's tone — the same line gets +// different notes as an academic essay than as a journal entry — so switching +// tone must re-open every sentence rather than serve back advice written for the +// old register. +func splitChunks(text, salt string) []chunk { + var out []chunk + runes := []rune(text) + start := 0 + add := func(end int) { + if s := string(runes[start:end]); strings.TrimSpace(s) != "" { + out = append(out, chunk{text: s, hash: hashChunk(s, salt)}) + } + start = end + } + + for i := 0; i < len(runes); i++ { + r := runes[i] + if r == '\n' { + add(i + 1) + continue + } + cjk := strings.ContainsRune(cjkTerminators, r) + if !cjk && !strings.ContainsRune(asciiTerminators, r) { + continue + } + // Swallow a run of terminators ("?!", "…") and any closing punctuation. + j := i + 1 + for j < len(runes) && (strings.ContainsRune(asciiTerminators+cjkTerminators+closers, runes[j])) { + j++ + } + if cjk || j >= len(runes) || unicode.IsSpace(runes[j]) { + add(j) + i = j - 1 + } + } + if start < len(runes) { + add(len(runes)) + } + return out +} + +// hashChunk identifies a sentence by its content under the same normalization +// the suppression logic uses: quote style and whitespace runs churn constantly +// (the editor rewrites quotes as she types, a paragraph reflows) and none of +// that changes what the sentence says, so none of it should cost a re-check. +func hashChunk(s, salt string) string { + sum := sha256.Sum256([]byte(salt + "\x00" + normalizeForDedup(s))) + return hex.EncodeToString(sum[:])[:16] +} + +// hashSet indexes chunks by hash — "is this sentence in the document?" +func hashSet(chunks []chunk) map[string]bool { + out := make(map[string]bool, len(chunks)) + for _, c := range chunks { + out[c.hash] = true + } + return out +} + +// changedChunks returns the chunks whose hash wasn't in the last checked set, +// in document order and deduplicated — a sentence repeated verbatim is one +// question, not two. +func changedChunks(chunks []chunk, checked map[string]bool) []chunk { + seen := make(map[string]bool, len(chunks)) + var out []chunk + for _, c := range chunks { + if checked[c.hash] || seen[c.hash] { + continue + } + seen[c.hash] = true + out = append(out, c) + } + return out +} + +// joinChunks renders a chunk set as the text to hand the model: one sentence per +// line, so two sentences pulled from opposite ends of the document don't read as +// one run-on. +func joinChunks(chunks []chunk) string { + parts := make([]string, 0, len(chunks)) + for _, c := range chunks { + parts = append(parts, strings.TrimSpace(c.text)) + } + return strings.Join(parts, "\n") +} + +// chunkFor names the sentence a suggestion belongs to: the first chunk whose +// text contains the flagged span. Returns "" when the span straddles a sentence +// boundary or the model paraphrased what it quoted — such a row is re-examined +// on every pass rather than cached, which is the safe direction. +func chunkFor(original string, chunks []chunk) string { + o := normalizeForDedup(original) + if o == "" { + return "" + } + for _, c := range chunks { + if strings.Contains(normalizeForDedup(c.text), o) { + return c.hash + } + } + return "" +} diff --git a/internal/suggestions/chunk_test.go b/internal/suggestions/chunk_test.go new file mode 100644 index 0000000..2faab5e --- /dev/null +++ b/internal/suggestions/chunk_test.go @@ -0,0 +1,118 @@ +package suggestions + +import ( + "strings" + "testing" +) + +func texts(chunks []chunk) []string { + out := make([]string, 0, len(chunks)) + for _, c := range chunks { + out = append(out, strings.TrimSpace(c.text)) + } + return out +} + +func TestSplitChunks(t *testing.T) { + cases := []struct { + name string + in string + want []string + }{ + { + name: "plain sentences", + in: "I has two apple. She go to market yesterday! Why?", + want: []string{"I has two apple.", "She go to market yesterday!", "Why?"}, + }, + { + // A decimal must not split, or the sentence's identity would churn + // while she types the number. + name: "decimals stay whole", + in: "It costs 3.50 today. Tomorrow, more.", + want: []string{"It costs 3.50 today.", "Tomorrow, more."}, + }, + { + name: "closing quote travels with its sentence", + in: `He said "early," and left. She stayed.`, + want: []string{`He said "early," and left.`, "She stayed."}, + }, + { + // Chinese runs sentences together with no space after 。 — she writes + // in both languages in one document. + name: "cjk terminators split without a space", + in: "我想说这句话。但是不知道用英语怎么说。", + want: []string{"我想说这句话。", "但是不知道用英语怎么说。"}, + }, + { + name: "newlines break chunks", + in: "A list item\nAnother item\n", + want: []string{"A list item", "Another item"}, + }, + { + name: "blank runs are dropped", + in: "\n\n \nOnly this.\n\n", + want: []string{"Only this."}, + }, + { + name: "trailing fragment is its own chunk", + in: "Done. Still writing", + want: []string{"Done.", "Still writing"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := texts(splitChunks(tc.in, "")) + if len(got) != len(tc.want) { + t.Fatalf("want %q, got %q", tc.want, got) + } + for i := range got { + if got[i] != tc.want[i] { + t.Fatalf("chunk %d: want %q, got %q", i, tc.want[i], got[i]) + } + } + }) + } +} + +// A sentence's identity survives the churn that doesn't change what it says: +// the editor rewrites quotes as she types, and a paragraph reflows. +func TestChunkIdentityIgnoresCosmeticChurn(t *testing.T) { + a := splitChunks(`She said "hello" softly.`, "") + b := splitChunks("She said “hello” softly.", "") + if len(a) != 1 || len(b) != 1 { + t.Fatalf("want one chunk each, got %d and %d", len(a), len(b)) + } + if a[0].hash != b[0].hash { + t.Fatalf("quote/whitespace churn changed the sentence's identity") + } + if same := splitChunks(`She said "hello" softly.`, "academic"); same[0].hash == a[0].hash { + t.Fatalf("a different tone must be a different reading of the sentence") + } +} + +func TestChangedChunksAndLookup(t *testing.T) { + chunks := splitChunks("One thing. Another thing. One thing.", "") + if len(chunks) != 3 { + t.Fatalf("want 3 chunks, got %d", len(chunks)) + } + + // A repeated sentence is one question, not two. + if got := changedChunks(chunks, nil); len(got) != 2 { + t.Fatalf("want 2 distinct changed chunks, got %d", len(got)) + } + + checked := hashSet(chunks[:1]) + changed := changedChunks(chunks, checked) + if len(changed) != 1 || strings.TrimSpace(changed[0].text) != "Another thing." { + t.Fatalf("want only the unread sentence, got %q", texts(changed)) + } + + if chunkFor("Another", chunks) != chunks[1].hash { + t.Fatalf("span was attributed to the wrong sentence") + } + // A span the document doesn't contain has no sentence, so it is never cached. + if chunkFor("nowhere in here", chunks) != "" { + t.Fatalf("unanchorable span should have no chunk") + } +} diff --git a/internal/suggestions/handlers.go b/internal/suggestions/handlers.go index bf1dfaa..fb9a1cc 100644 --- a/internal/suggestions/handlers.go +++ b/internal/suggestions/handlers.go @@ -160,16 +160,21 @@ func (h *Handler) mechanics(w http.ResponseWriter, r *http.Request) { httputil.WriteJSON(w, http.StatusOK, out) } -// replaceMechanics swaps the document's pending offline rows for the supplied -// findings in one transaction, leaving the LLM families and actioned rows -// untouched. Findings the user already accepted or dismissed are suppressed (the -// detector has no memory between runs), and malformed spans are skipped. +// replaceMechanics brings the document's pending offline rows in line with the +// supplied findings in one transaction, leaving the LLM families and actioned +// rows untouched. Findings the user already accepted or dismissed are suppressed +// (the detector has no memory between runs), and malformed spans are skipped. // -// The DELETE is scoped by *source*, not by type: the rule pack owns both the -// mechanics family and its share of the collocation family, and every run is a -// full recompute of the document, so everything it wrote last time goes. Scoping -// by type instead would strand offline collocations the current text no longer -// warrants — the one row nobody would ever replace. +// A finding the detector still reports keeps its existing row — same id, same +// created_at — and only its offsets move. This pass fires 250 ms after a +// keystroke, so deleting and re-inserting the family would hand every card a new +// identity several times a sentence: the rail would remount, a card expanded for +// Ask Petal would collapse under her, and the arrival chime would re-fire. +// +// The scope is *source*, not type: the rule pack owns both the mechanics family +// and its share of the collocation family, and every run is a full recompute of +// the document. Scoping by type instead would strand offline collocations the +// current text no longer warrants — the one row nobody would ever replace. func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) error { tx, err := h.DB.Begin() if err != nil { @@ -177,18 +182,18 @@ func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) er } defer tx.Rollback() - if _, err := tx.Exec( - `DELETE FROM suggestions WHERE doc_id = ? AND status = ? AND source = ?`, - docID, db.SuggestionStatusPending, db.SuggestionSourceLocal, - ); err != nil { + existing, err := loadPending(tx, docID, "source = '"+db.SuggestionSourceLocal+"'") + if err != nil { return err } + index := indexByEdit(existing) sup, err := buildSuppressor(tx, docID) if err != nil { return err } + kept := make(map[string]bool, len(existing)) for _, f := range findings { if f.From < 0 || f.To <= f.From || strings.TrimSpace(f.Original) == "" { continue // malformed span — the client re-anchors by string anyway @@ -196,16 +201,34 @@ func (h *Handler) replaceMechanics(docID string, findings []mechanicsFinding) er if sup.suppressed(f.Original, f.Replacement) { continue } + typ := localType(f.Type) + if row, ok := index.take(f.Original, f.Replacement, f.From); ok { + kept[row.id] = true + if err := reposition(tx, row, f.From, f.To, ""); err != nil { + return err + } + continue + } if _, err := tx.Exec( `INSERT INTO suggestions (doc_id, from_pos, to_pos, original, replacement, explanation, type, source) VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, docID, f.From, f.To, f.Original, f.Replacement, f.Explanation, - localType(f.Type), db.SuggestionSourceLocal, + typ, db.SuggestionSourceLocal, ); err != nil { return err } } + // Whatever the detector no longer reports, she has fixed. + for _, row := range existing { + if kept[row.id] { + continue + } + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, row.id); err != nil { + return err + } + } + return tx.Commit() } @@ -258,12 +281,65 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R return } - // Nothing to analyze on an empty document — skip the LLM round-trip. + // Nothing to analyze on an empty document — skip the LLM round-trip. The + // family's rows go with the text they were about. if strings.TrimSpace(contentText) == "" { - httputil.WriteJSON(w, http.StatusOK, []db.Suggestion{}) + if err := h.reconcilePending(docID, contentText, nil, scope, nil, nil, false); err != nil { + httputil.ServerError(w, err) + return + } + out, err := h.fetchPending(userID, docID) + if err != nil { + httputil.ServerError(w, err) + return + } + httputil.WriteJSON(w, http.StatusOK, out) return } + // Decide what to ask about before spending anything: a chunked pass asks only + // about the sentences that changed since it last read the document, and when + // none did it doesn't call the model at all — nor consume its rate-limit slot, + // so the next real edit isn't throttled by a check that had nothing to do. + // + // Only a chunked pass consults that record, so only it needs the tone folded + // into a sentence's identity. + salt := "" + if scope.chunked { + salt = tone + } + chunks := splitChunks(contentText, salt) + askText, fresh := contentText, chunks + if scope.chunked { + checked, err := h.checkedChunks(docID, scope.family) + if err != nil { + httputil.ServerError(w, err) + return + } + changed := changedChunks(chunks, checked) + if len(changed) == 0 { + // Every sentence has already been read. Drop the rows whose sentence is + // gone, keep the rest exactly as they are, and answer immediately. + if err := h.reconcilePending(docID, contentText, nil, scope, chunks, nil, false); err != nil { + httputil.ServerError(w, err) + return + } + out, err := h.fetchPending(userID, docID) + if err != nil { + httputil.ServerError(w, err) + return + } + httputil.WriteJSON(w, http.StatusOK, out) + return + } + // When every sentence is new — a first pass, a paste, a tone switch — hand + // over the document verbatim so the model reads it with its paragraphing + // intact. Otherwise send just the delta, one sentence per line. + if len(changed) < len(hashSet(chunks)) { + askText, fresh = joinChunks(changed), changed + } + } + ok, _, slotAt := limiter.Allow(docID) if !ok { // Throttled: return the existing pending set unchanged rather than an @@ -277,7 +353,7 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R return } - raw, err := run(r.Context(), h.Client, contentText, tone, llm.LangFor(pairLang)) + raw, err := run(r.Context(), h.Client, askText, tone, llm.LangFor(pairLang)) if err != nil { // Allow ran before the model call, so a failed pass would otherwise hold // the per-document slot for the full interval — stranding the frontend's @@ -287,7 +363,9 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R return } - if err := h.replacePending(docID, contentText, raw, scope); err != nil { + // A whole-document pass re-read everything, so every one of its rows is up for + // re-proposal; a chunked pass only puts the sentences it asked about in play. + if err := h.reconcilePending(docID, contentText, raw, scope, chunks, fresh, !scope.chunked); err != nil { httputil.ServerError(w, err) return } @@ -308,78 +386,49 @@ func (h *Handler) runPass(w http.ResponseWriter, r *http.Request, limiter *llm.R // inserts. The grammar checkpoint and voice pass each own a disjoint family, so // running one never disturbs the other's pending flags. type pendingScope struct { - deleteWhere string // extra WHERE clause scoping the DELETE to this family + deleteWhere string // extra WHERE clause scoping this pass to its own family forceType string // if set, every inserted row gets this type; else normalizeType + // family keys the sentences this pass has already read (see checked_chunks). + family string + // chunked passes re-read only the sentences that changed. True for the typing- + // cadence grammar checkpoint, which fires constantly and must feel still; + // false for the explicit whole-document passes, where she pressed a button + // asking for a fresh read of everything. + chunked bool } -// Every scope below is confined to source='llm'. The offline rule pack replaces -// its own rows wholesale on each edit (see replaceMechanics) and its findings +// Every scope below is confined to source='llm'. The offline rule pack owns its +// own rows and recomputes them on each edit (see replaceMechanics); its findings // must survive all three model passes — including the collocation coach, which // now shares the collocation family with it. var ( // grammarScope owns the grammar/phrasing/idiom/clarity flags — everything but // the other self-owned families (voice, collocation), which run on their own // cadence/pass and must survive a grammar checkpoint. Notably the offline pass - // writes its rows in the same /check request just before this DELETE runs, so - // the source clause is also what keeps them alive. - grammarScope = pendingScope{deleteWhere: "source = 'llm' AND type NOT IN ('voice','collocation')", forceType: ""} - // voiceScope owns the model's voice flags only. - voiceScope = pendingScope{deleteWhere: "source = 'llm' AND type = 'voice'", forceType: db.SuggestionTypeVoice} + // writes its rows in the same /check request just before this pass reconciles, + // so the source clause is also what keeps them alive. + grammarScope = pendingScope{ + deleteWhere: "source = 'llm' AND type NOT IN ('voice','collocation')", + family: "grammar", + chunked: true, + } + // voiceScope owns the model's voice flags only. Voice is a property of the + // document as a whole — a sentence isn't inconsistent with itself — so this + // pass always reads everything. + voiceScope = pendingScope{ + deleteWhere: "source = 'llm' AND type = 'voice'", + forceType: db.SuggestionTypeVoice, + family: "voice", + } // collocationScope owns the model's collocation flags only — the rule pack's // share of the same family is left standing. - collocationScope = pendingScope{deleteWhere: "source = 'llm' AND type = 'collocation'", forceType: db.SuggestionTypeCollocation} + collocationScope = pendingScope{ + deleteWhere: "source = 'llm' AND type = 'collocation'", + forceType: db.SuggestionTypeCollocation, + family: "collocation", + } ) -// replacePending swaps a document's pending suggestions within one family for a -// fresh batch in a single transaction. Accepted/rejected suggestions and the -// other family's pending rows are left untouched. -// -// 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 { - return err - } - defer tx.Rollback() - - if _, err := tx.Exec( - `DELETE FROM suggestions WHERE doc_id = ? AND status = ? AND `+scope.deleteWhere, - docID, db.SuggestionStatusPending, - ); err != nil { - return err - } - - sup, err := buildSuppressor(tx, docID) - if err != nil { - return err - } - - for _, s := range raw { - if sup.suppressed(s.Original, s.Replacement) { - continue - } - typ := scope.forceType - if typ == "" { - typ = normalizeType(s.Type) - } - from, to := locate(contentText, s.Original) - if _, err := tx.Exec( - `INSERT INTO suggestions (doc_id, from_pos, to_pos, original, replacement, explanation, type, source) - VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, - docID, from, to, s.Original, s.Replacement, s.Explanation, typ, db.SuggestionSourceLLM, - ); err != nil { - return err - } - } - - return tx.Commit() -} - // 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 diff --git a/internal/suggestions/handlers_test.go b/internal/suggestions/handlers_test.go index ca2a289..badb3ab 100644 --- a/internal/suggestions/handlers_test.go +++ b/internal/suggestions/handlers_test.go @@ -20,10 +20,17 @@ import ( type stubClient struct { response string calls int + // The full prompt of the most recent call, so a test can assert which + // sentences a chunked pass actually asked about. + lastPrompt string } -func (s *stubClient) Complete(_ context.Context, _ llm.CompletionRequest) (string, error) { +func (s *stubClient) Complete(_ context.Context, req llm.CompletionRequest) (string, error) { s.calls++ + s.lastPrompt = "" + for _, m := range req.Messages { + s.lastPrompt += m.Content + "\n" + } return s.response, nil } @@ -64,6 +71,17 @@ func newTestServer(t *testing.T, client llm.LLMClient) (http.Handler, string, *H return authed, docID, h } +// setDocText rewrites the seeded document, standing in for the writer editing. +// The grammar checkpoint only asks the model about sentences that changed since +// it last read the document, so a test that wants a second real pass has to +// change something first — as she always has. +func setDocText(t *testing.T, h *Handler, docID, text string) { + t.Helper() + if _, err := h.DB.Exec(`UPDATE documents SET content_text = ? WHERE id = ?`, text, docID); err != nil { + t.Fatalf("update doc text: %v", err) + } +} + func do(t *testing.T, srv http.Handler, method, path, body string) *httptest.ResponseRecorder { t.Helper() var r *http.Request @@ -183,6 +201,7 @@ func TestFickleEditsSuppressed(t *testing.T) { ]}`} srv, docID, h := newTestServer(t, client) h.Limit = llm.NewRateLimiter(0) + setDocText(t, h, docID, `He left "early," because of the rain. The cat always have a calm face.`) rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") var got []db.Suggestion @@ -194,6 +213,10 @@ func TestFickleEditsSuppressed(t *testing.T) { do(t, srv, http.MethodPost, "/suggestions/"+s.ID+"/accept", "") } + // Both edits are now in the document, which is what re-opens those sentences + // for a second reading. + setDocText(t, h, docID, `He left "early," due to the rain. The cat always has a calm face.`) + // 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":[ @@ -314,7 +337,9 @@ func TestCollocationPassCoexists(t *testing.T) { t.Fatalf("collocation response should carry all three families, got %+v", got) } - // A grammar checkpoint must NOT wipe the voice or collocation flags. + // A grammar checkpoint must NOT wipe the voice or collocation flags. She fixes + // the flagged sentence, so its own grammar row goes and nothing replaces it. + setDocText(t, h, docID, "I have two apples.") client.response = `{"suggestions":[]}` rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") if rec.Code != http.StatusOK { diff --git a/internal/suggestions/reconcile.go b/internal/suggestions/reconcile.go new file mode 100644 index 0000000..6048bd8 --- /dev/null +++ b/internal/suggestions/reconcile.go @@ -0,0 +1,293 @@ +package suggestions + +import ( + "database/sql" + + "gitea.parodia.dev/drwily/petal/internal/db" + "gitea.parodia.dev/drwily/petal/internal/llm" +) + +// Reconciliation replaces the old "delete the family, insert the new batch" +// shape of every pass. A suggestion the pass proposes again is the *same* +// suggestion: it keeps its row, and therefore its id, its created_at and — most +// visibly — the explanation it was first given. The model re-words its reasoning +// every time it is asked, so re-inserting meant one unchanged mistake carried +// three different explanations in a single sitting. +// +// The id is what the frontend keys its cards on, so a stable id is also what +// keeps the rail from emptying and refilling, a card from collapsing mid-read, +// and the arrival chime from re-firing for advice she has already seen. + +// pendingRow is the part of an existing pending suggestion reconciliation cares +// about. +type pendingRow struct { + id string + original string + replacement string + chunkHash string + from int +} + +// loadPending reads the pending rows a pass owns. `where` is the pass's own +// scoping clause (by source, and for the model passes by family) — the same +// fragment that used to scope its DELETE. +func loadPending(tx *sql.Tx, docID, where string) ([]pendingRow, error) { + rows, err := tx.Query( + `SELECT id, original, replacement, chunk_hash, from_pos FROM suggestions + WHERE doc_id = ? AND status = ? AND `+where, + docID, db.SuggestionStatusPending, + ) + if err != nil { + return nil, err + } + defer rows.Close() + + var out []pendingRow + for rows.Next() { + var r pendingRow + if err := rows.Scan(&r.id, &r.original, &r.replacement, &r.chunkHash, &r.from); err != nil { + return nil, err + } + out = append(out, r) + } + return out, rows.Err() +} + +// editKey identifies an edit by what it proposes, not where: "this exact change +// to this exact text". Normalized like the suppression comparisons, so the +// editor's quote rewriting and a reflowed paragraph don't read as a new edit. +func editKey(original, replacement string) string { + return normalizeForDedup(original) + "\x00" + normalizeForDedup(replacement) +} + +// editIndex matches freshly proposed edits against the rows already standing. +type editIndex struct { + rows []pendingRow + used []bool + byKey map[string][]int +} + +func indexByEdit(rows []pendingRow) *editIndex { + idx := &editIndex{rows: rows, used: make([]bool, len(rows)), byKey: map[string][]int{}} + for i, r := range rows { + k := editKey(r.original, r.replacement) + idx.byKey[k] = append(idx.byKey[k], i) + } + return idx +} + +// take claims the standing row for this edit, if there is one. When a document +// repeats the same mistake, `near` (the fresh span's start) picks the closest +// standing row, so two identical cards keep their own identities instead of +// trading them whenever the text between them grows. +func (i *editIndex) take(original, replacement string, near int) (pendingRow, bool) { + best, bestDist := -1, 0 + for _, n := range i.byKey[editKey(original, replacement)] { + if i.used[n] { + continue + } + d := i.rows[n].from - near + if d < 0 { + d = -d + } + if best < 0 || d < bestDist { + best, bestDist = n, d + } + } + if best < 0 { + return pendingRow{}, false + } + i.used[best] = true + return i.rows[best], true +} + +// reposition updates the advisory offsets (and the sentence a row belongs to) +// without touching anything the writer can see. The frontend re-anchors by +// string at render time, so these only matter for the local-vs-model span +// arbitration in dedupeSpans. +func reposition(tx *sql.Tx, row pendingRow, from, to int, chunkHash string) error { + if row.from == from && row.chunkHash == chunkHash { + return nil + } + _, err := tx.Exec( + `UPDATE suggestions SET from_pos = ?, to_pos = ?, chunk_hash = ? WHERE id = ?`, + from, to, chunkHash, row.id, + ) + return err +} + +// reconcilePending brings a model pass's family in line with what it just +// proposed, sentence by sentence: +// +// - A row on a sentence this pass didn't ask about is kept untouched — that +// is the whole point of chunking. Only its offsets are refreshed. +// - A row on a sentence that no longer exists in the document is dropped: she +// rewrote or deleted it. +// - A row on a sentence the pass *did* ask about survives only if the model +// proposed the same edit again, in which case it keeps its identity. +// +// `fresh` names the sentences the model was asked about (nil when it wasn't +// called at all). inPlayAll marks the whole-document passes — voice and the +// collocation coach — where every row is up for re-proposal because the model +// just re-read everything. +func (h *Handler) reconcilePending( + docID, contentText string, + raw []llm.RawSuggestion, + scope pendingScope, + chunks, fresh []chunk, + inPlayAll bool, +) error { + tx, err := h.DB.Begin() + if err != nil { + return err + } + defer tx.Rollback() + + existing, err := loadPending(tx, docID, scope.deleteWhere) + if err != nil { + return err + } + + present := hashSet(chunks) + asked := hashSet(fresh) + modelRan := inPlayAll || fresh != nil + + // Sentences to hand back to the model next time, because a row we were + // caching on them turned out to be unanchorable (see below). + reopen := map[string]bool{} + + var inPlay []pendingRow + for _, r := range existing { + switch { + // A row whose sentence we can't name is never cached — it is re-examined + // whenever the model speaks, and left alone when it doesn't. + case inPlayAll, r.chunkHash == "" && modelRan, asked[r.chunkHash]: + inPlay = append(inPlay, r) + case r.chunkHash != "" && !present[r.chunkHash]: + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, r.id); err != nil { + return err + } + default: + // Untouched sentence: keep the card exactly as she last saw it. + from, to := locate(contentText, r.original) + if from < 0 { + // The sentence is unchanged in substance but the quoted span no + // longer matches byte for byte — a quote mark the editor rewrote + // inside it, say. The frontend anchors by that string, so this card + // can't be shown; drop it and let the sentence be read again rather + // than cache advice nobody can see. + reopen[r.chunkHash] = true + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, r.id); err != nil { + return err + } + continue + } + if err := reposition(tx, r, from, to, r.chunkHash); err != nil { + return err + } + } + } + for h := range reopen { + delete(present, h) + } + + sup, err := buildSuppressor(tx, docID) + if err != nil { + return err + } + + index := indexByEdit(inPlay) + kept := make(map[string]bool, len(inPlay)) + for _, s := range raw { + if sup.suppressed(s.Original, s.Replacement) { + continue + } + from, to := locate(contentText, s.Original) + // Attribute the finding to a sentence the model was actually shown before + // falling back to the whole document: a short span ("the the") can occur in + // two sentences, and crediting it to the cached one would drop it as advice + // we already have. + hash := chunkFor(s.Original, fresh) + if hash == "" { + hash = chunkFor(s.Original, chunks) + } + // A sentence we didn't ask about already has whatever advice it deserves. + // The model can't normally quote one — it was only shown the delta — but if + // it wanders there anyway, the cached card stands rather than gaining a + // twin. + if !inPlayAll && hash != "" && present[hash] && !asked[hash] { + continue + } + if row, ok := index.take(s.Original, s.Replacement, from); ok { + kept[row.id] = true + if err := reposition(tx, row, from, to, hash); err != nil { + return err + } + continue + } + typ := scope.forceType + if typ == "" { + typ = normalizeType(s.Type) + } + if _, err := tx.Exec( + `INSERT INTO suggestions (doc_id, from_pos, to_pos, original, replacement, explanation, type, source, chunk_hash) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + docID, from, to, s.Original, s.Replacement, s.Explanation, typ, db.SuggestionSourceLLM, hash, + ); err != nil { + return err + } + } + + // Asked about and not proposed again: the model has changed its mind, or she + // has fixed it. + for _, r := range inPlay { + if kept[r.id] { + continue + } + if _, err := tx.Exec(`DELETE FROM suggestions WHERE id = ?`, r.id); err != nil { + return err + } + } + + // Record the sentences this family has now read. Every sentence still in the + // document has been read by *some* pass: the ones just asked about now, the + // rest in an earlier round. + if scope.chunked { + if _, err := tx.Exec( + `DELETE FROM checked_chunks WHERE doc_id = ? AND family = ?`, docID, scope.family, + ); err != nil { + return err + } + for h := range present { + if _, err := tx.Exec( + `INSERT INTO checked_chunks (doc_id, family, hash) VALUES (?, ?, ?)`, + docID, scope.family, h, + ); err != nil { + return err + } + } + } + + return tx.Commit() +} + +// checkedChunks loads the sentences a family read on its last pass. +func (h *Handler) checkedChunks(docID, family string) (map[string]bool, error) { + rows, err := h.DB.Query( + `SELECT hash FROM checked_chunks WHERE doc_id = ? AND family = ?`, docID, family, + ) + if err != nil { + return nil, err + } + defer rows.Close() + + out := map[string]bool{} + for rows.Next() { + var hash string + if err := rows.Scan(&hash); err != nil { + return nil, err + } + out[hash] = true + } + return out, rows.Err() +} diff --git a/internal/suggestions/stability_test.go b/internal/suggestions/stability_test.go new file mode 100644 index 0000000..20ffa90 --- /dev/null +++ b/internal/suggestions/stability_test.go @@ -0,0 +1,191 @@ +package suggestions + +import ( + "encoding/json" + "net/http" + "strings" + "testing" + + "gitea.parodia.dev/drwily/petal/internal/db" + "gitea.parodia.dev/drwily/petal/internal/llm" +) + +// byOriginal indexes a pending set by the text each card flags. +func byOriginal(in []db.Suggestion) map[string]db.Suggestion { + out := map[string]db.Suggestion{} + for _, s := range in { + out[s.Original] = s + } + return out +} + +// TestUntouchedSentencesKeepTheirCards is the heart of the stability work: she +// edits one sentence, and the cards on every other sentence stay exactly as they +// were — same id (so the rail keeps the card instead of remounting it), same +// explanation (the model re-words its reasoning every time it is asked, and one +// unchanged mistake used to carry three different explanations in a sitting). +// The model is only asked about the sentence that changed. +func TestUntouchedSentencesKeepTheirCards(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has two apple","replacement":"I have two apples","explanation":"first wording","type":"grammar"}, + {"original":"She go to market","replacement":"She goes to market","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + setDocText(t, h, docID, "I has two apple. She go to market yesterday.") + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var first []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &first); err != nil { + t.Fatalf("decode: %v", err) + } + if len(first) != 2 { + t.Fatalf("first pass: want 2, got %d: %+v", len(first), first) + } + kept := byOriginal(first)["I has two apple"] + + // She fixes only the second sentence. The model, asked again, re-words its + // reasoning about the first — which it must never get the chance to do. + setDocText(t, h, docID, "I has two apple. She goes to market yesterday.") + client.response = `{"suggestions":[ + {"original":"I has two apple","replacement":"I have two apples","explanation":"REWORDED","type":"grammar"} + ]}` + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var second []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &second); err != nil { + t.Fatalf("decode: %v", err) + } + + if strings.Contains(client.lastPrompt, "I has two apple") { + t.Fatalf("untouched sentence was sent to the model:\n%s", client.lastPrompt) + } + if !strings.Contains(client.lastPrompt, "She goes to market") { + t.Fatalf("edited sentence was not sent to the model:\n%s", client.lastPrompt) + } + + now := byOriginal(second)["I has two apple"] + if now.ID != kept.ID { + t.Fatalf("card was remounted: id %q became %q", kept.ID, now.ID) + } + if now.Explanation != "first wording" { + t.Fatalf("explanation drifted: %q", now.Explanation) + } + // The fixed sentence's card is gone, and the model's stray re-proposal for the + // cached sentence did not become a second card. + if len(second) != 1 { + t.Fatalf("want exactly one card left, got %d: %+v", len(second), second) + } +} + +// TestUnchangedDocumentSkipsTheModel proves a check with nothing new to read +// costs nothing: no model call, and every card left standing untouched. This is +// the doc-open and tone-less re-check path. +func TestUnchangedDocumentSkipsTheModel(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has","replacement":"I have","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 first []db.Suggestion + _ = json.Unmarshal(rec.Body.Bytes(), &first) + if len(first) != 1 || client.calls != 1 { + t.Fatalf("first pass: %d cards, %d calls", len(first), client.calls) + } + + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var second []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &second); err != nil { + t.Fatalf("decode: %v", err) + } + if client.calls != 1 { + t.Fatalf("re-checking an unedited document called the model %d times", client.calls) + } + if len(second) != 1 || second[0].ID != first[0].ID { + t.Fatalf("card did not survive an idle re-check: %+v", second) + } +} + +// TestDeletedSentenceDropsItsCard covers the other half of the skip path: she +// removes a flagged sentence outright, so nothing changed that the model could +// be asked about — but its card must still go. +func TestDeletedSentenceDropsItsCard(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has","replacement":"I have","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + + do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + setDocText(t, h, docID, "") + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + var got []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if len(got) != 0 { + t.Fatalf("card outlived its sentence: %+v", got) + } +} + +// TestToneChangeReopensEverySentence: the checkpoint's advice is written for the +// document's tone, so switching from a journal to an academic essay has to +// re-read sentences that haven't changed a character. +func TestToneChangeReopensEverySentence(t *testing.T) { + client := &stubClient{response: `{"suggestions":[ + {"original":"I has","replacement":"I have","explanation":"agreement","type":"grammar"} + ]}`} + srv, docID, h := newTestServer(t, client) + h.Limit = llm.NewRateLimiter(0) + + do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + if _, err := h.DB.Exec(`UPDATE documents SET tone = 'academic' WHERE id = ?`, docID); err != nil { + t.Fatalf("set tone: %v", err) + } + do(t, srv, http.MethodPost, "/docs/"+docID+"/check", "") + + if client.calls != 2 { + t.Fatalf("tone change did not re-read the document: %d model calls", client.calls) + } +} + +// TestMechanicsFindingsKeepTheirRows: the rule pack re-runs 250 ms after every +// keystroke. A finding it still reports must keep its row, or the rail would +// remount several times a sentence — collapsing a card she has open, and +// re-firing the arrival chime for advice she is already reading. +func TestMechanicsFindingsKeepTheirRows(t *testing.T) { + srv, docID, _ := newTestServer(t, &stubClient{response: `{"suggestions":[]}`}) + body := `{"findings":[ + {"from":0,"to":5,"original":"I has","replacement":"I have","explanation":"agreement"}, + {"from":6,"to":15,"original":"two apple","replacement":"two apples","explanation":"plural"} + ]}` + + rec := do(t, srv, http.MethodPost, "/docs/"+docID+"/mechanics", body) + var first []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &first); err != nil { + t.Fatalf("decode: %v", err) + } + if len(first) != 2 { + t.Fatalf("want 2 rows, got %d", len(first)) + } + + // She types elsewhere: same findings, shifted spans, one of them now fixed. + rec = do(t, srv, http.MethodPost, "/docs/"+docID+"/mechanics", `{"findings":[ + {"from":20,"to":25,"original":"I has","replacement":"I have","explanation":"agreement"} + ]}`) + var second []db.Suggestion + if err := json.Unmarshal(rec.Body.Bytes(), &second); err != nil { + t.Fatalf("decode: %v", err) + } + if len(second) != 1 { + t.Fatalf("want 1 row, got %d: %+v", len(second), second) + } + if second[0].ID != byOriginal(first)["I has"].ID { + t.Fatalf("surviving finding was given a new identity: %+v", second[0]) + } + if second[0].FromPos != 20 { + t.Fatalf("span did not follow the text: %+v", second[0]) + } +}