feat(dedup): audit-only aelf doctor dedup (#197 R1) - #376
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 52 minutes and 51 seconds.Comment |
Reviewer's GuideImplements an audit-only near-duplicate detection module ( Sequence diagram for aelf doctor dedup audit flowsequenceDiagram
actor User
participant CLI as aelf_doctor
participant Doctor as _cmd_doctor
participant DedupCmd as _cmd_doctor_dedup
participant Config as load_dedup_config
participant Store as MemoryStore
participant Dedup as dedup_audit
participant Formatter as format_audit_report
participant Out as out
User->>CLI: run aelf doctor --dedup [--dedup-*]
CLI->>Doctor: _cmd_doctor(args,out)
Doctor->>DedupCmd: _cmd_doctor_dedup(args,out)
DedupCmd->>Config: load_dedup_config()
Config-->>DedupCmd: DedupConfig(defaults_or_toml_values)
DedupCmd->>DedupCmd: apply CLI overrides to DedupConfig
DedupCmd->>Store: _open_store()
Store-->>DedupCmd: MemoryStore
DedupCmd->>Dedup: dedup_audit(store,jaccard_min,levenshtein_min,max_candidate_pairs)
Dedup-->>DedupCmd: DedupAuditReport
DedupCmd->>Store: close()
DedupCmd->>Formatter: format_audit_report(report)
Formatter-->>DedupCmd: text_report
DedupCmd->>Out: print(text_report)
DedupCmd-->>Doctor: exit_code 0 or 1
Doctor-->>CLI: exit_code
CLI-->>User: dedup audit output
Class diagram for dedup audit types and helpersclassDiagram
class DuplicatePair {
+str belief_a_id
+str belief_b_id
+float jaccard_score
+float levenshtein_score
}
class DuplicateCluster {
+str representative_id
+tuple~str~ member_ids
}
class DedupAuditReport {
+int n_beliefs_scanned
+int n_candidate_pairs
+int n_duplicate_pairs
+int n_clusters
+bool truncated
+tuple~DuplicatePair~ pairs
+tuple~DuplicateCluster~ clusters
}
class DedupConfig {
+float jaccard_min
+float levenshtein_min
+int max_candidate_pairs
}
class _UnionFind {
-dict~str,str~ _parent
-dict~str,int~ _size
+__init__()
+make(x str)
+find(x str) str
+union(a str,b str)
+groups() dict~str,list~str~~
}
class MemoryStore {
+list_beliefs_for_indexing() list~tuple~str,str~~
}
class dedup_module {
+float DEFAULT_JACCARD_MIN
+float DEFAULT_LEVENSHTEIN_MIN
+int DEFAULT_MAX_CANDIDATE_PAIRS
+jaccard(a set~str~,b set~str~) float
+levenshtein_distance(a str,b str) int
+levenshtein_ratio(a str,b str) float
+dedup_audit(store MemoryStore,jaccard_min float,levenshtein_min float,max_candidate_pairs int) DedupAuditReport
+cluster_pairs(pairs Iterable~DuplicatePair~) tuple~DuplicateCluster~
+load_dedup_config(start Path) DedupConfig
+format_audit_report(report DedupAuditReport) str
}
dedup_module --> DuplicatePair
dedup_module --> DuplicateCluster
dedup_module --> DedupAuditReport
dedup_module --> DedupConfig
dedup_module --> _UnionFind
dedup_module --> MemoryStore
DedupAuditReport "*" o-- "*" DuplicatePair
DedupAuditReport "*" o-- "*" DuplicateCluster
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
format_audit_report, the truncation line always reportsDEFAULT_MAX_CANDIDATE_PAIRSeven when a differentmax_candidate_pairswas used for the run; consider using the value fromreportso the output reflects the actual cap in effect. - The
dedup_auditdocstring mentions degrading gracefully on FTS5 errors, but the implementation no longer touches FTS5; updating that wording would avoid confusion about what the function actually depends on and how failures are handled.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `format_audit_report`, the truncation line always reports `DEFAULT_MAX_CANDIDATE_PAIRS` even when a different `max_candidate_pairs` was used for the run; consider using the value from `report` so the output reflects the actual cap in effect.
- The `dedup_audit` docstring mentions degrading gracefully on FTS5 errors, but the implementation no longer touches FTS5; updating that wording would avoid confusion about what the function actually depends on and how failures are handled.
## Individual Comments
### Comment 1
<location path="src/aelfrice/dedup.py" line_range="319-328" />
<code_context>
+# --- Top-level audit entry point ---------------------------------------
+
+
+def dedup_audit(
+ store: MemoryStore,
+ *,
+ jaccard_min: float = DEFAULT_JACCARD_MIN,
+ levenshtein_min: float = DEFAULT_LEVENSHTEIN_MIN,
+ max_candidate_pairs: int = DEFAULT_MAX_CANDIDATE_PAIRS,
+) -> DedupAuditReport:
+ """Walk the store, find near-duplicate belief pairs, return a report.
+
+ Read-only: no edges are inserted, no beliefs are mutated. The
+ write-path hook (insert SUPERSEDES edges) is deferred behind the
+ #197 bench gate.
+
+ Raises `ValueError` on malformed thresholds; degrades gracefully
+ on per-belief FTS5 errors (skips that belief, logs nothing).
+ """
</code_context>
<issue_to_address>
**issue:** dedup_audit does not actually degrade gracefully on per-belief/tokenization errors
The current implementation will propagate exceptions from `store.list_beliefs_for_indexing()`, `tokenize()`, or `_jaccard_prefiltered_pairs`, so a single bad belief/tokenization error aborts the whole audit. To match the docstring’s “degrades gracefully” guarantee, consider catching per-belief failures at iteration/tokenization time and skipping those beliefs, while still raising on configuration issues (e.g. the existing `ValueError` threshold checks).
</issue_to_address>
### Comment 2
<location path="src/aelfrice/dedup.py" line_range="515-517" />
<code_context>
+ lines.append("=" * 40)
+ lines.append(f"Beliefs scanned : {report.n_beliefs_scanned}")
+ lines.append(f"Candidate pairs visited : {report.n_candidate_pairs}")
+ if report.truncated:
+ lines.append(
+ f" (truncated to {DEFAULT_MAX_CANDIDATE_PAIRS} — see "
+ f"[dedup] max_candidate_pairs)"
+ )
</code_context>
<issue_to_address>
**issue (bug_risk):** Truncation message always prints the default cap, ignoring configured/CLI max_candidate_pairs
When `report.truncated` is true, this always prints `DEFAULT_MAX_CANDIDATE_PAIRS`, even when the effective `max_candidate_pairs` was set via config or `--dedup-max-pairs`. That makes the report misleading (e.g., it may claim truncation at 5000 pairs when a different limit was used). Consider passing the effective `max_candidate_pairs` into `DedupAuditReport` and using that value here instead of the module-level default.
</issue_to_address>
### Comment 3
<location path="src/aelfrice/dedup.py" line_range="190-199" />
<code_context>
+def _jaccard_prefiltered_pairs(
</code_context>
<issue_to_address>
**suggestion:** Candidate pair count semantics differ from the docstring description
The docstring describes `raw_count` as "the raw candidate count (all O(n^2) pairs visited)", but the code skips pairs with blank content and only increments `raw_count` for the remaining ones. This makes `n_candidate_pairs` effectively a "non-empty pair" count rather than a full O(n^2) visit count. Either move the `raw_count` increment before the blank-content checks or update the docstring to match the current behavior.
Suggested implementation:
```python
raw_count += 1
if not content_a or not content_b:
continue
```
If the blank-content check or `raw_count` increment is formatted differently in your actual code (e.g., different indentation, extra conditions, or comments between them), adjust the SEARCH/REPLACE block accordingly so that:
1. `raw_count += 1` is executed immediately after the pair `(content_a, content_b)` is formed / selected in the nested loops.
2. All subsequent `continue` statements that filter out blank content occur *after* incrementing `raw_count`, ensuring `raw_count` counts every visited pair, even those with blank content.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+319
to
+328
| def dedup_audit( | ||
| store: MemoryStore, | ||
| *, | ||
| jaccard_min: float = DEFAULT_JACCARD_MIN, | ||
| levenshtein_min: float = DEFAULT_LEVENSHTEIN_MIN, | ||
| max_candidate_pairs: int = DEFAULT_MAX_CANDIDATE_PAIRS, | ||
| ) -> DedupAuditReport: | ||
| """Walk the store, find near-duplicate belief pairs, return a report. | ||
|
|
||
| Read-only: no edges are inserted, no beliefs are mutated. The |
There was a problem hiding this comment.
issue: dedup_audit does not actually degrade gracefully on per-belief/tokenization errors
The current implementation will propagate exceptions from store.list_beliefs_for_indexing(), tokenize(), or _jaccard_prefiltered_pairs, so a single bad belief/tokenization error aborts the whole audit. To match the docstring’s “degrades gracefully” guarantee, consider catching per-belief failures at iteration/tokenization time and skipping those beliefs, while still raising on configuration issues (e.g. the existing ValueError threshold checks).
Comment on lines
+515
to
+517
| if report.truncated: | ||
| lines.append( | ||
| f" (truncated to {DEFAULT_MAX_CANDIDATE_PAIRS} — see " |
There was a problem hiding this comment.
issue (bug_risk): Truncation message always prints the default cap, ignoring configured/CLI max_candidate_pairs
When report.truncated is true, this always prints DEFAULT_MAX_CANDIDATE_PAIRS, even when the effective max_candidate_pairs was set via config or --dedup-max-pairs. That makes the report misleading (e.g., it may claim truncation at 5000 pairs when a different limit was used). Consider passing the effective max_candidate_pairs into DedupAuditReport and using that value here instead of the module-level default.
Comment on lines
+190
to
+199
| def _jaccard_prefiltered_pairs( | ||
| beliefs: list[tuple[str, str]], | ||
| *, | ||
| jaccard_min: float, | ||
| max_pairs: int, | ||
| ) -> tuple[ | ||
| list[tuple[str, str, str, str, frozenset[str], frozenset[str], float]], | ||
| int, | ||
| bool, | ||
| ]: |
There was a problem hiding this comment.
suggestion: Candidate pair count semantics differ from the docstring description
The docstring describes raw_count as "the raw candidate count (all O(n^2) pairs visited)", but the code skips pairs with blank content and only increments raw_count for the remaining ones. This makes n_candidate_pairs effectively a "non-empty pair" count rather than a full O(n^2) visit count. Either move the raw_count increment before the blank-content checks or update the docstring to match the current behavior.
Suggested implementation:
raw_count += 1
if not content_a or not content_b:
continueIf the blank-content check or raw_count increment is formatted differently in your actual code (e.g., different indentation, extra conditions, or comments between them), adjust the SEARCH/REPLACE block accordingly so that:
raw_count += 1is executed immediately after the pair(content_a, content_b)is formed / selected in the nested loops.- All subsequent
continuestatements that filter out blank content occur after incrementingraw_count, ensuringraw_countcounts every visited pair, even those with blank content.
Collaborator
Author
|
[claim:review:kulili:2026-05-03T15:57:15Z] |
Collaborator
Author
|
[claim:review:Toug:2026-05-03T15:57:17Z] |
Collaborator
Author
|
[release:review:Toug:2026-05-03T15:57:22Z] |
Collaborator
Author
|
[claim:review:Gylf:2026-05-03T15:57:29Z] |
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
Collaborator
Author
|
[release:review:Gylf:2026-05-03T15:57:33Z] |
Collaborator
Author
|
[claim:review:Setr:2026-05-03T15:57:36Z] |
Collaborator
Author
|
[release:review:Setr:2026-05-03T15:57:41Z] |
Collaborator
Author
|
Reviewed: code is clean, deterministic, audit-only as advertised; all 18 checks green; signed; discretion grep on diff is clean. Sanitized one token in the PR body before noticing the FF state. Branch is no longer FF — main moved to aa1aabb (#375 merged). Rebase needed before merge: Releasing review claim. Will re-claim after rebase or another reviewer can pick it up. |
Collaborator
Author
|
[release:review:kulili:2026-05-03T15:59:20Z] |
Collaborator
Author
|
[claim:review:Kulili:2026-05-03T16:42:12Z] |
Collaborator
Author
|
[release:review:Kulili:2026-05-03T16:42:42Z] |
Collaborator
Author
|
[claim:review:Setr:2026-05-03T16:48:26Z] |
Stdlib-only port of the research-line dedup detector. Pairs Jaccard over lowercase Unicode-word tokens (>= 0.8 default) with Levenshtein ratio (>= 0.85 default) as the second-stage gate; both must clear for a pair to count as a near-duplicate. Defaults from the #197 ratification (Jaccard 0.8, Levenshtein 0.85, max-pairs 5000). `dedup_audit(store, ...)` is read-only: walks every belief pair via direct O(n^2) Jaccard prefilter (the FTS5 path misses paraphrases where stemming diverges, e.g. "don't" / "do not"), runs Levenshtein ratio on prefilter survivors, returns a `DedupAuditReport` with pairs + union-find-collapsed clusters. No edges inserted, no beliefs mutated. Sampling is deterministic (sort by `(id_a, id_b)`, truncate at `max_candidate_pairs`) so the same store produces the same report across runs. The CLI surface (`aelf doctor dedup`) and `[dedup]` config block land in the next commit; the write-path SUPERSEDES hook is the bench-gated R2 deferred behind the corpus benchmark. 32 tests: similarity primitives, union-find clustering, audit-pass read-only contract, cluster chain collapse, threshold floor behaviour (paraphrase rejection at default Jaccard, relaxed-mode recovery), invalid-threshold guards, format_audit_report shape.
Wires the audit pass behind `aelf doctor --dedup`. New flags `--dedup-jaccard`, `--dedup-levenshtein`, `--dedup-max-pairs` override the per-run thresholds; `[dedup]` block in `.aelfrice.toml` provides project-level defaults. Read-only — no edges inserted, no beliefs mutated; the write-path SUPERSEDES hook is the bench-gated R2 deferred behind the corpus benchmark per #197 ratification. Config loader follows the `[rebuilder]` / `[implicit_feedback]` convention: walk up from cwd looking for `.aelfrice.toml`, malformed values degrade to defaults with a stderr trace, never raises. 8 new tests: TOML loader (default fallthrough, well-formed override, out-of-range fallback, wrong-type fallback, malformed-TOML fallback) + CLI integration (clean store exit 0, pair detection through main(), threshold overrides via flags). Existing 100 cli/doctor tests still pass.
`docs/dedup.md`: usage, per-run flags, [dedup] config block, defaults table, output-shape sample, explicit "audit-only by design — write-path is bench-gated R2" call-out. LIMITATIONS § Sharp edges: new bullet on near-duplicates from different ingest paths, pointing at the new audit surface. Tightens but does not remove the existing onboard-non-incremental caveat — the audit lists clusters, it does not collapse them.
robotrocketscience
force-pushed
the
feat/issue-197-dedup-audit
branch
from
May 3, 2026 16:49
5809301 to
931c20e
Compare
Collaborator
Author
|
[release:review:Setr:2026-05-03T16:51:21Z] |
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
2 participants
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.
First atomic PR for #197 (dedup module — v2.0 evaluation). Implements
R1: audit-only
aelf doctor --dedupper the ratification at#197
(2026-04-29: ship at v2.0, port research-line
dedup.pystdlib-only,defaults Jaccard 0.8 + Levenshtein 0.85, 5000-pair cap, audit-only
first then bench-gated write-path hook).
What
src/aelfrice/dedup.py— stdlib-only near-duplicate detector pairingJaccard over the existing lowercase Unicode-word tokenizer with a
two-row Levenshtein-distance ratio. Both thresholds must clear for a
pair to count as a near-duplicate; defaults match the ratification
(Jaccard ≥ 0.8, Levenshtein ratio ≥ 0.85, max 5000 pairs).
dedup_audit(store, ...)is read-only:DedupAuditReportwith deterministic ordering (sortedby
(id_a, id_b), truncated atmax_candidate_pairs).aelf doctor --dedupprints the clustered report. Per-run flags--dedup-jaccard,--dedup-levenshtein,--dedup-max-pairsoverride config; the
[dedup]section in.aelfrice.tomlprovidesproject-level defaults via the same loader pattern as
[rebuilder]and
[implicit_feedback].What this PR does not change
SUPERSEDES hook is the bench-gated R2 deferred behind the v2.0
corpus benchmark. R2 lands once [v2.0] Bench-gate harness — consume lab corpus, skip-when-absent on public CI #319's per-module bench gate
shows neutral-or-positive p@k and recall on the existing lab
corpus.
not" tokenise into disjoint sets after the FTS5 tokenizer's
apostrophe handling, so FTS5 misses the exact paraphrase shape
this detector targets. Direct O(n²) Jaccard prefilter clears
in seconds at the live-store median (~1.6k beliefs ≈ 1.3M pairs;
set-intersection on small token sets ~1-10 µs/pair).
Verification
uv run pytest tests/test_dedup.py -q→ 40 passed.uv run pytest tests/ --ignore=tests/regression --ignore=tests/bench_gate --ignore=tests/e2e -q→ 2139 passed, 14 skipped (no regression vs main).git diff origin/main...HEAD: clean.uv tool run ruff check src/aelfrice/dedup.py tests/test_dedup.py→ clean.Test plan
Refs
write-path hook (R2) stays open on Deduplication module (dedup) — v2.0 evaluation #197 until the v2.0 corpus
bench-gate via [v2.0] Bench-gate harness — consume lab corpus, skip-when-absent on public CI #319 grades neutral-or-positive.
docs/v2_dedup.md.#197
Summary by Sourcery
Add an audit-only near-duplicate detection facility for beliefs and expose it via the doctor CLI, with configurable thresholds and reporting.
New Features:
aelf doctor --dedupsubcommand with configurable thresholds and pair caps to run the dedup audit and print a human-readable report.Enhancements:
[dedup]configuration from.aelfrice.tomlwith validated defaults and graceful fallback behaviour.Documentation:
docs/dedup.mddescribing the dedup audit command, configuration, thresholds, and limitations.docs/LIMITATIONS.mdto note that near-duplicate beliefs from different ingest paths persist and reference the new audit command.Tests:
aelf doctor --dedup.