Fix TFIDF corpus shift breaking dedup-against-retracted recall (#22) - #26
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes TFIDF dedup-against-retracted recall degradation by keeping TFIDF’s vector space coherent across retractions and making TFIDF’s “best-effort” guarantees explicit to operators across CLI/HTTP surfaces.
Changes:
- Fix TFIDF corpus drift across retractions by building the IDF corpus from
ListLeavesIncludingRetractedand making TFIDF vocab ordering deterministic. - Add operator-facing advisories/warnings for TFIDF limitations across
serve,retract, andremember(HTTP + CLI). - Add regression/unit tests that pin the issue #22 scenario and warning surfaces.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/engine/embedder.go | Build TFIDF IDF corpus including retracted leaves; deterministic vocab ordering. |
| internal/engine/embedder_test.go | Unit test ensuring retracted vocab remains in TFIDF corpus rebuilds. |
| internal/engine/retract.go | Add TFIDF advisory helpers (EmbedderLimitWarning, SoftRetractedMatchWarning) and new threshold constant. |
| internal/engine/retract_test.go | Regression + warning-surface tests for TFIDF corpus coherence and advisories. |
| internal/server/routes.go | Surface TFIDF warnings on /remember and /retract responses. |
| internal/server/routes_test.go | HTTP-layer tests asserting presence/absence of warning field. |
| internal/server/smoke_test.go | Update smoke-test commentary to reflect fixed retraction-induced drift vs residual corpus-growth drift. |
| internal/cli/serve.go | Print TFIDF best-effort advisory at startup when TFIDF fallback is used. |
| internal/cli/retract.go | Print server-returned warning to stderr for retract responses. |
| internal/cli/remember.go | Print server-returned warning to stderr for remember responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+195
to
+213
| // Drop any hard-tier matches — those are the regular gate's job. | ||
| candidateVec, err := e.Embedder.Embed(ctx, candidateL0) | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| var softOnly []store.MemNode | ||
| for _, n := range soft { | ||
| v, err := e.DB.GetVector(n.ID) | ||
| if err != nil || v == nil { | ||
| continue | ||
| } | ||
| sim := CosineSimilarity(candidateVec, v.Embedding) | ||
| if sim < defaultSimilarityThreshold { | ||
| softOnly = append(softOnly, n) | ||
| } | ||
| } | ||
| if len(softOnly) == 0 { | ||
| return "" | ||
| } |
Comment on lines
+388
to
+392
| emb, _ := NewTFIDFEmbedder(db, 512) | ||
| eng.SetEmbedder(emb) | ||
| n, _ := db.GetNodeByURI(uri) | ||
| eng.EmbedNode(ctx, n) | ||
|
|
Comment on lines
+446
to
+450
| emb, _ := NewTFIDFEmbedder(db, 512) | ||
| eng.SetEmbedder(emb) | ||
| n, _ := db.GetNodeByURI(retracted) | ||
| eng.EmbedNode(ctx, n) | ||
|
|
lazypower
added a commit
that referenced
this pull request
Jun 12, 2026
Three valid nits, two production + one test pattern. 1. SoftRetractedMatchWarning had a dead post-filter loop. Caller is handleRemember after a successful Remember with !AcknowledgeRetracted — the hard gate at defaultSimilarityThreshold already ran and produced no matches. Filtering hard-tier matches out of the soft scan was redundant work: a second e.Embedder.Embed call plus a DB.GetVector round-trip per soft match, on the response hot path. Drop the post-filter. Replace it with an explicit PRECONDITION block in the function comment so a future caller that violates the contract knows the helper isn't going to save them — the hard gate is the contract, not this helper. 2. Two new tests (TestRetract_EmitsTFIDFWarning, TestSoftRetractedMatchWarning_TFIDFFlags) silently dropped errors from NewTFIDFEmbedder / GetNodeByURI / EmbedNode via the `_` pattern. Setup failure could pass/fail the test for the wrong reason. Tightened both to t.Fatal on each step. 3. Updated the test comment for TestSoftRetractedMatchWarning_TFIDFFlags to reflect (1) — the helper no longer filters hard matches; the caller guarantees it doesn't need to. No behavior change on the production happy path; the post-filter was dead code, removing it cannot affect output. Verified by full test suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 13, 2026
Comment on lines
+193
to
+196
| soft, err := e.findRetractedMatches(ctx, candidateL0, category, softRetractedThreshold) | ||
| if err != nil || len(soft) == 0 { | ||
| return "" | ||
| } |
Comment on lines
+341
to
+349
| // Issue #22: post-hoc soft-match nag for the TFIDF path. The hard gate | ||
| // either fired above (returning matches_retracted) or did not match at the | ||
| // default threshold; this surfaces sub-threshold neighbors so the operator | ||
| // knows recall is best-effort. No-op on Ollama / Anthropic embedders. | ||
| if !req.AcknowledgeRetracted { | ||
| if warning := s.engine.SoftRetractedMatchWarning(ctx, req.Summary, req.Category); warning != "" { | ||
| resp["warning"] = warning | ||
| } | ||
| } |
Closes #22 with the narrow fix the issue called for, plus documentation that articulates the embedder-tier choice operators are making. Drops the per-Remember soft-match advisory machinery from the earlier revision of this PR — that was alarmism (a loud siren for a tradeoff the operator already made by choosing the fallback path) rather than user pain we have evidence of. What ships: 1. The 2-line root-cause fix in NewTFIDFEmbedder. - Load IDF corpus from ListLeavesIncludingRetracted so retracted nodes' vocabulary stays in the table. Without this, vectors stored while the node was live live in a different vector space than fresh embeddings, and findRetractedMatches silently degrades. - Alphabetical tiebreaker after the document-frequency-descending sort so vocab order is deterministic given the same corpus. Without this, Go map iteration randomization put identical terms at different vector positions across NewTFIDFEmbedder() calls, making cosine similarity effectively random across process restarts. 2. One-line startup advisory when TFIDF is selected, pointing at the README for the upgrade paths. Runs once at boot, not per Remember. 3. README "Embedding backends" section. Names the three paths honestly: - Ollama with nomic-embed-text (recommended; free; daemon) - TFIDF (fallback; zero deps; recall degrades on growth) - Paid API (consistent; no daemon; per-embedding cost) Explicit guidance on when each is appropriate, including that if the operator has used retract for PII the recall guarantee matters and they should choose Ollama or a paid embedder. Tests: - TestNewTFIDFEmbedder_IncludesRetractedInCorpus pins the corpus fix at the unit level — retracted-only vocabulary survives in the rebuilt IDF, embedding of retracted-only terms is non-zero. - TestFindRetractedMatches_TFIDFCorpusCoherent is the load-bearing end-to-end regression: seeds and embeds under Embedder A, retracts, rebuilds Embedder B fresh from the post-retraction corpus (simulating a process restart), writes a fresh near-duplicate of the retracted memory, asserts the gate still fires. Pre-fix this test would fail. - smoke_test.go's log message updated to reference the new framing and the regression test by name. What this PR deliberately does NOT ship (per the design discussion): - SoftRetractedMatchWarning function and its per-Remember sub- threshold scan. The hard gate already protects the documented invariant; the soft warning was an alarmist hedge against a discoverability gap, and the cleaner home for that is the README. - `warning` field on the retract HTTP response. Same reasoning. - Engine.Retract signature change to (bool, string, error). Reverted to the original (bool, error) since the warning field is gone. - Tests of the above (stubOllamaEmbedder, TestEmbedderLimitWarning_*, TestRetract_EmitsTFIDFWarning, TestRetract_NoWarningOnOllama, TestSoftRetractedMatchWarning_*). Rationale for the trim: subsystem honesty belongs in documentation, not in runtime hedges that fire on data the user already wrote. The hard gate still protects the documented invariant. The README explains what "best-effort" means. The startup log surfaces which path is live. That's the full surface — anything beyond it is hedging in the wrong place. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lazypower
force-pushed
the
fix/issue-22-tfidf-corpus-coherence
branch
from
June 13, 2026 17:54
5f9682c to
c90a8bd
Compare
| For embeddings: Ollama with `nomic-embed-text` if available, otherwise falls back to TF-IDF (zero external dependencies). | ||
| ## Embedding backends | ||
|
|
||
| Continuity needs an embedder for semantic search and for the dedup-against-retracted gate (the safety net that catches a PII-shaped memory being re-written after retraction). The three available paths trade off cost, dependencies, and recall guarantees in different directions. Continuity probes them in order and uses the first one it finds. |
Comment on lines
+249
to
+253
| **3. Paid embeddings (Anthropic / OpenAI / etc.) — consistent, no daemon.** | ||
|
|
||
| Frontier-quality embeddings via API. Consistent vector space, no daemon to operate, no probe to fail. Costs per embedding and sends content to a vendor. Worth it for setups that want Ollama-quality without running Ollama, or for environments where Ollama isn't practical. Configure via `ANTHROPIC_API_KEY` or by setting the equivalent embedder provider in `~/.continuity/config.toml`. | ||
|
|
||
| **Picking a path.** If you care about the retraction gate — and if you've used `continuity retract` to remove PII or otherwise sensitive material, you do — choose option 1 or 3. If you're running Continuity casually and the worst case of a soft duplicate slipping through is "I have to dedup manually later," option 2 is fine. |
Comment on lines
+134
to
+136
| // drift — that's the broader TFIDF limitation we accept as best-effort | ||
| // rather than chase. The asserted regression test for the fix lives at | ||
| // engine/retract_test.go::TestFindRetractedMatches_TFIDFCorpusCoherent. |
Comment on lines
+323
to
+328
| for _, uri := range []string{live, retracted} { | ||
| n, _ := db.GetNodeByURI(uri) | ||
| if err := eng.EmbedNode(ctx, n); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } |
Three fixes called out by Copilot's pass on the trimmed PR:
1. README "Embedding backends" claimed three tiers and described a
paid-API path that does not exist. The code probes Ollama → TFIDF,
full stop; ANTHROPIC_API_KEY only configures the extraction LLM
(serve.go:30-33), not the embedder. Misdirecting operators to a
configuration path that won't work is worse than the alarmism the
refactor was trying to fix. Trimmed to the two tiers that actually
ship.
Decision: don't add a paid-embedder backend to this PR. Anthropic
doesn't ship an embedding API (they recommend Voyage AI), so a
"paid" tier would mean implementing OpenAI / Voyage / similar —
real design work with its own PR shape. Committing the README to a
feature we're not committed to building is the wrong tradeoff.
2. Startup advisory in serve.go now points only at Ollama (no
"/ paid alternatives") since that's all we ship today.
3. Two small Copilot finds on the test/comment surface:
- smoke_test.go log line now references "internal/engine/..."
instead of "engine/..." so the path is greppable.
- retract_test.go's GetNodeByURI return is now nil-checked +
error-returned; ignoring it could have panicked EmbedNode with
a less diagnosable failure if the seed setup ever broke.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+109
to
+113
| // Best-effort by construction: the corpus IS the model. Every retraction or | ||
| // new write that introduces vocabulary not yet in the IDF table shifts the | ||
| // vector space. We minimize the most load-bearing variant of that drift — | ||
| // retraction-induced drift — by including retracted nodes in the corpus | ||
| // (see NewTFIDFEmbedder). Ollama users have a static pre-trained model and |
|
|
||
| **2. Built-in TFIDF — fallback, best-effort.** | ||
|
|
||
| Zero external dependencies. Used automatically when Ollama is unreachable, so a fresh install always has *something*. The cost: TFIDF rebuilds its vocabulary from the local corpus on each startup, so the vector space drifts as the corpus grows or changes. The retraction-induced component of that drift is contained (issue #22 — the IDF table includes retracted nodes so cosine similarity stays coherent across a retraction). The corpus-growth component is not. The hard gate at `sim ≥ 0.65` still fires for direct rewrites; near-duplicates that would have been caught under Ollama may slip through. |
| // pointer to the upgrade path. The README's "Embedding | ||
| // backends" section spells out the two shipped paths | ||
| // (Ollama / TFIDF). Issue #22. | ||
| fmt.Fprintln(os.Stderr, " ! tfidf: retraction-dedup recall is best-effort; install Ollama (nomic-embed-text) for stronger guarantees — see README \"Embedding backends\"") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #22. Narrow root-cause fix in
NewTFIDFEmbedderplus a README section that names the embedding-backend choice operators are making.Refactored from the earlier revision of this PR (force-pushed). The previous version added a per-Remember soft-match advisory machinery (sub-threshold scan,
SoftRetractedMatchWarning,warningfield on retract response,Engine.Retractsignature change). That apparatus was alarmism — a loud siren for a tradeoff the operator already made by choosing the fallback path, with no documented user pain motivating it. Trim list and rationale below.What ships
1. The 2-line root-cause fix in
NewTFIDFEmbedderListLeavesIncludingRetractedso retracted nodes' vocabulary stays in the table. Without this, vectors stored while the node was live live in a different vector space than fresh embeddings, andfindRetractedMatchessilently degrades — the bug TFIDF embedder corpus shift degrades dedup-against-retracted recall #22 reported.NewTFIDFEmbedder()calls, making cosine similarity effectively random across process restarts.2. One-line startup advisory when TFIDF is selected, pointing at the README for upgrade paths. Runs once at boot, not per Remember.
3. README "Embedding backends" section — names the three paths honestly:
nomic-embed-text(recommended; free; daemon)Explicit guidance on when each is appropriate, including: if you've used
retractfor PII, the recall guarantee matters and you should pick Ollama or a paid embedder.Tests
TestNewTFIDFEmbedder_IncludesRetractedInCorpus— unit pin that retracted-only vocabulary survives in the rebuilt IDF, and that embedding of retracted-only terms produces non-zero vectors.TestFindRetractedMatches_TFIDFCorpusCoherent— the load-bearing end-to-end regression: seed and embed under Embedder A, retract, rebuild Embedder B fresh from the post-retraction corpus (simulates a process restart), write a fresh near-duplicate of the retracted memory, assert the gate still fires. Pre-fix this test would fail.What this PR deliberately does NOT ship
Removed from the previous revision per the design discussion:
The earlier Copilot review pointed at the right surface (redundant per-call work, silent error swallowing, weak tests). The cleaner answer than fixing those individually was to drop the machinery — the hard gate already protects the documented invariant, and the discoverability goal it was trying to serve lives more honestly in the README.
Rationale for the trim
Subsystem honesty belongs in documentation, not in runtime hedges that fire on data the user already wrote. The hard gate still protects the documented invariant. The README explains what "best-effort" means. The startup log surfaces which path is live. That's the full surface — anything beyond it is hedging in the wrong place.
The earlier revision was an instance of a recurring shape worth naming: a bug fix that grows a surrounding "let's be principled about the limits of this subsystem" apparatus. The apparatus is theoretically defensible but adds cost on every code reader and every hot-path execution, for a discoverability problem that documentation handles more cheaply.
Test plan
🤖 Generated with Claude Code