feat(lock): near-duplicate lock-dedup hygiene at write time + audit (#1016) - #1035
Merged
Merged
Conversation
…1016) Locks are injected unbounded and never trimmed (#379), so re-locking slightly-reworded ground truth accumulates near-duplicate locks that inflate the injection (a real 24-lock store had a 3-lock cluster = one fact re-locked with wording drift, 12.5%). Implement the #1016-C sub-task as dedup (the R&D-validated lever; distillation tested weak), reusing the existing dedup engine (Jaccard>=0.8 AND Levenshtein>=0.85): - aelf lock warns when the new lock near-duplicates an existing one, naming it and suggesting unlock/delete. Warning only; the lock still writes (user ground truth). - aelf doctor --dedup --dedup-locks scopes the audit to locked beliefs so the backlog cluster is findable. New dedup.find_near_duplicate_locks() + a locked_only flag on dedup_audit() (full-store audit byte-identical by default). No auto-delete: cleanup stays user-confirmed.
Reviewer's GuideImplements lock-specific dedup hygiene by reusing the existing similarity engine: write-time near-duplicate warnings for Sequence diagram for aelf lock write-time near-duplicate warningsequenceDiagram
actor User
participant CLI as _cmd_lock
participant Store as MemoryStore
participant Dedup as find_near_duplicate_locks
User->>CLI: aelf lock --statement
CLI->>Store: lock_belief
Store-->>CLI: actual_id
CLI->>Dedup: find_near_duplicate_locks(store, statement, exclude_id=actual_id)
Dedup->>Store: _locked_beliefs_for_indexing(store)
Store-->>Dedup: list_locked_beliefs
Dedup-->>CLI: [DuplicatePair]
alt [near_dups is non-empty]
CLI->>User: print warning about near-duplicate locks
else [near_dups is empty]
CLI->>User: no hygiene warning
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
![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 1 issue, and left some high level feedback:
- The
find_near_duplicate_lockspath does a full scan over all locked beliefs on every lock write; if the locked set grows large this could become noticeably slow, so consider a configurable cap, early-exit behavior, or reusing any existing index/FTS machinery to bound the cost. dedup_audit(locked_only=True)currently signals the scope only via a separate CLI print; if you expect other callers or tooling to use this mode, it may be clearer to surface thelocked_onlyflag directly inDedupAuditReportso consumers can inspect the scope programmatically.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `find_near_duplicate_locks` path does a full scan over all locked beliefs on every lock write; if the locked set grows large this could become noticeably slow, so consider a configurable cap, early-exit behavior, or reusing any existing index/FTS machinery to bound the cost.
- `dedup_audit(locked_only=True)` currently signals the scope only via a separate CLI print; if you expect other callers or tooling to use this mode, it may be clearer to surface the `locked_only` flag directly in `DedupAuditReport` so consumers can inspect the scope programmatically.
## Individual Comments
### Comment 1
<location path="src/aelfrice/cli.py" line_range="4975" />
<code_context>
),
)
+ locked_only = bool(getattr(args, "dedup_locks", False))
store = _open_store()
try:
</code_context>
<issue_to_address>
**nitpick:** The `getattr`/`bool` wrapping around `args.dedup_locks` seems unnecessary.
Because this argument is declared with `action="store_true"` and `default=False`, `args.dedup_locks` should always be present and already boolean. Unless there is a known caller that omits it, using `locked_only = args.dedup_locks` would be clearer and would surface any unexpected missing attribute instead of silently defaulting to `False`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds lock-dedup hygiene: a new ChangesLock dedup hygiene
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
… review) store_true already yields a bool; keep getattr for sibling-arg consistency and direct-Namespace test safety. Addresses Sourcery.
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 33548e9 → |
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
1 participant
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.
Part of #1016 — the #1016-C (lock hygiene) sub-task. Does not close the umbrella (frozen/reference tiers, lock-vs-hook docs remain).
Why
Locks are injected unbounded and never trimmed (#379), so re-locking slightly-reworded ground truth quietly accumulates near-duplicate locks that inflate the injection budget (#1016). Evidence on the real 24-lock store: a 3-lock cluster that is one fact ("v3.0 #592 eval-harness status") re-locked with κ/kappa wording drift (pairwise J≥0.92, L≥0.88) — 12.5%, matching the ~14% estimate. 2 of those 3 are redundant.
Design note — dedup, not distillation
The issue's AC-C originally framed hygiene as "char-cap / distillation." Prior R&D found distillation weak (locks are already dense, ~1.08× compression) and dedup/supersession the real lever, so this implements dedup. Flagging the deviation explicitly.
What
Reuses the existing
dedupengine (Jaccard ≥ 0.8 AND Levenshtein ≥ 0.85) — no new similarity code:aelf lockprints a hygiene warning when the new lock near-duplicates an existing lock, naming it and suggestingaelf unlock/aelf delete. Warning only; the lock still writes (it is user-asserted ground truth).aelf doctor --dedup --dedup-locksscopes the dedup audit to the user-locked set, so the existing cluster is findable without wading through the full-store report. Live store shows the 3-member cluster.New
dedup.find_near_duplicate_locks()+ alocked_onlyflag ondedup_audit(). The default full-store audit is byte-identical. No belief is auto-deleted — locks are ground truth, so cleanup stays user-confirmed.Tests
tests/test_dedup.py:find_near_duplicate_locks(finds / excludes self / ignores unlocked / below-threshold / blank),dedup_audit(locked_only=True)(scopes to locks, default unchanged), and a--dedup-locksCLI test.tests/test_cli_lock_via_worker.py: write-time warning fires on a near-dup and stays silent otherwise.Verification
pytest tests/test_dedup.py tests/test_cli_lock_via_worker.py tests/test_lock_management.py tests/test_lock_contract.py tests/test_slash_commands.py→ 249 passed, 1 skipped.uvx vulture … --min-confidence 80→ clean.uvx typos→ clean.aelf doctor --dedup --dedup-lockssurfaces the 3-lock cluster.🤖 Generated with Claude Code
Summary by Sourcery
Add lock-scoped near-duplicate detection and auditing to improve lock hygiene without changing existing dedup behavior.
New Features:
aelf lockthat warns when a new lock closely matches existing user-locked beliefs.aelf doctor --dedup --dedup-locksthat limits the report to user-locked beliefs.Enhancements:
find_near_duplicate_locksand alocked_onlyoption ondedup_auditwhile keeping the default full-store audit unchanged.Documentation:
Tests:
--dedup-locksflag.Summary by CodeRabbit
New Features
aelf locknow warns when a new lock is very similar to an existing one, helping prevent duplicate locks from piling up.aelf doctor --dedup-locksadds a locked-only deduplication scan for checking user-locked beliefs separately.Bug Fixes