refactor(dupe): make duplicate checks evidence driven - #327
Conversation
Broaden tracker searches to enumerate same-work candidates before policy evaluation. Track work scope and effective completeness separately while preserving native candidate facts. Centralize exact-file, pack, and general coexistence rules. Refs #325
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDuplicate search handling now records work scope, effective completeness, wrong-work counts, pagination warnings, and structured results. Unit3D searches filter conflicting TMDB IDs and retain richer duplicates. Duplicate evaluation and tracker policies now use generalized identity, fallback, and precedence rules. ChangesDuplicate search and evaluation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TrackerAdapter
participant SearchEvidence
participant DuplicateEvaluator
TrackerAdapter->>SearchEvidence: return entries, work scope, and pagination metadata
SearchEvidence->>DuplicateEvaluator: provide EffectiveComplete and WrongWorkCount
DuplicateEvaluator->>DuplicateEvaluator: apply identity and policy findings
DuplicateEvaluator-->>TrackerAdapter: return duplicate relations and evidence
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/trackers/impl/standalone/hds/dupe.go (1)
103-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDetect an actual next page before marking the search incomplete.
Line 108 treats every pagination link with
pages=as a next-page link. On the final page, a link to an earlier page can satisfy this condition. The loop then stops at Line 115 withcomplete == false, so a complete empty provider-ID search becomes blocked.Accept only an explicit next control, or parse the target page and require it to be greater than
page.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/trackers/impl/standalone/hds/dupe.go` around lines 103 - 115, Update the next-page detection in the pagination loop around commonhttp.FirstNode so a generic pages= link is not accepted as the next page. Require either an explicit “Next”/“>>” control or parse the href and verify its target page is greater than the current page variable, preserving complete=true when no actual next page exists.
🧹 Nitpick comments (3)
internal/trackers/dupe/evaluator_test.go (1)
988-1003: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd negative coverage for generic shared file names.
TestEvaluatePartialCandidateFileSetWithExactBasenameIsExactuses the distinctive basenameExample.Release.2026.mkv. The new overlap rule inexactCandidatealso matches generic member names. Add a case that shares only a generic file such assample.mkvand asserts that the relation is notapi.DupeRelationExactDuplicate. This test gap shares one root cause with the overlap rule ininternal/trackers/dupe/evaluator.goat Line 90.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/trackers/dupe/evaluator_test.go` around lines 988 - 1003, Extend TestEvaluatePartialCandidateFileSetWithExactBasenameIsExact with negative coverage using a generic shared filename such as sample.mkv, while keeping the candidate file set partial. Assert that Evaluate does not return api.DupeRelationExactDuplicate for this case, covering the generic-name overlap behavior in exactCandidate.internal/trackers/impl/dupe_policy_test.go (1)
205-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the remaining evidence fixtures with the new work-scope contract.
assertRelationandassertPTPnow passWorkScope: dupe.WorkScopeTrackerGroup.assertAREvaluationat Line 231 andassertRTFat Line 272 still passdupe.SearchEvidence{Complete: true}with no work scope. Those evaluations are therefore effectively incomplete. The assertions still pass because they check onlyCandidates[0].Relation. Set the same work scope in those helpers to keep the fixtures consistent.Also applies to: 375-378
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/trackers/impl/dupe_policy_test.go` around lines 205 - 208, Update the SearchEvidence fixtures in assertAREvaluation and assertRTF to include WorkScope: dupe.WorkScopeTrackerGroup alongside Complete: true, matching assertRelation and assertPTP so these evaluations remain complete under the new work-scope contract.internal/trackers/impl/azfamily/dupe.go (1)
165-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog pagination decisions.
Line 167 and Line 171 stop enumeration but emit no operator-visible warning. Log the blocked outcome with the tracker, page count, completion state, and decision reason. Do not log
pageURL.Log normal completion at DEBUG level.
As per coding guidelines, add operator-visible progress and decision-point logs. Use warnings for blocked outcomes and DEBUG for troubleshooting context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/trackers/impl/azfamily/dupe.go` around lines 165 - 213, Add operator-visible logs in the pagination loop: when the max-page safety bound or repeated-page check stops enumeration, emit a warning containing the tracker, page count, incomplete completion state, and decision reason without logging pageURL. When pagination reaches normal completion, emit a DEBUG log with the tracker, page count, and completed state, using the surrounding AZ-family enumeration function’s existing logger.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/trackers/data/unit3d.go`:
- Around line 463-468: Update the warning construction in the WrongWorkCount
handling to use “row” when WrongWorkCount equals 1 and “rows” otherwise,
preserving the existing message for plural counts. Update the exact assertion in
the Unit3D test to expect the corrected singular message.
- Around line 460-462: Update the result accumulation around dedupeUnit3DEntries
so entries from /api/torrents/filter and /api/torrents/pending use distinct
deduplication namespaces. Prefix or otherwise namespace pending entry IDs before
appending, ensuring dedupeUnit3DEntries removes only repeats from the same
endpoint and preserves distinct candidates with matching numeric IDs.
In `@internal/trackers/dupe/evaluator.go`:
- Around line 88-92: Update exactCandidate in
internal/trackers/dupe/evaluator.go at lines 88-92 to require full
target-file-set containment or otherwise exclude generic basename-only matches
such as sample.mkv. In internal/trackers/dupe/evaluator_test.go at lines
988-1003, add coverage where target and candidate share only sample.mkv and
assert the result is not api.DupeRelationExactDuplicate.
In `@internal/trackers/dupe/service.go`:
- Around line 558-574: Extend api.DupeSearchEvidence in dupes.go with WorkScope
and WrongWorkCount, then populate and preserve both fields in the
duplicate-search result and API/webui projection around the tracker completion
flow. Use the existing search.WorkScope and search.WrongWorkCount values so
consumers can identify effective incompleteness and excluded-row counts.
In `@internal/trackers/impl/azfamily/dupe.go`:
- Around line 207-211: Update the pagination flow around nextAZPage to validate
nextPage with the existing same-origin URL helper against site.baseURL before
the request adds loadedCookies; treat rejected or empty URLs as completion and
do not follow them. Add a test covering an external rel="next" URL.
In `@internal/trackers/impl/standalone/bt/dupe.go`:
- Line 171: Update processBTGroupPage and the related release-name extraction
flow so TV pack folder extraction is determined from the candidate page rather
than the uploaded release. Preserve the candidate page’s pack release name when
the first file is an episode, while retaining folder-based extraction for
folder-based TV packs. Add a regression test covering a TV pack whose first file
is an episode and asserting the pack release name is preserved.
In `@internal/trackers/impl/standalone/czt/dupe.go`:
- Around line 146-147: Update the comment above cztSearchQuery to accurately
describe the current precedence, noting that meta.Release.Title is selected
before exact upload/client names. Keep the query implementation unchanged and
remove the stale claim that exact names are preferred first.
In `@internal/trackers/impl/standalone/hdt/dupe.go`:
- Around line 53-57: Validate the resolved title query before performing tracker
searches: in internal/trackers/impl/standalone/hdt/dupe.go lines 53-57 and
internal/trackers/impl/standalone/is/dupe.go lines 62-65, return
dupe.NotRun(dupe.NotRunMissingMetadata, ...) when query is empty; otherwise
preserve the existing parameter setup and search flow.
---
Outside diff comments:
In `@internal/trackers/impl/standalone/hds/dupe.go`:
- Around line 103-115: Update the next-page detection in the pagination loop
around commonhttp.FirstNode so a generic pages= link is not accepted as the next
page. Require either an explicit “Next”/“>>” control or parse the href and
verify its target page is greater than the current page variable, preserving
complete=true when no actual next page exists.
---
Nitpick comments:
In `@internal/trackers/dupe/evaluator_test.go`:
- Around line 988-1003: Extend
TestEvaluatePartialCandidateFileSetWithExactBasenameIsExact with negative
coverage using a generic shared filename such as sample.mkv, while keeping the
candidate file set partial. Assert that Evaluate does not return
api.DupeRelationExactDuplicate for this case, covering the generic-name overlap
behavior in exactCandidate.
In `@internal/trackers/impl/azfamily/dupe.go`:
- Around line 165-213: Add operator-visible logs in the pagination loop: when
the max-page safety bound or repeated-page check stops enumeration, emit a
warning containing the tracker, page count, incomplete completion state, and
decision reason without logging pageURL. When pagination reaches normal
completion, emit a DEBUG log with the tracker, page count, and completed state,
using the surrounding AZ-family enumeration function’s existing logger.
In `@internal/trackers/impl/dupe_policy_test.go`:
- Around line 205-208: Update the SearchEvidence fixtures in assertAREvaluation
and assertRTF to include WorkScope: dupe.WorkScopeTrackerGroup alongside
Complete: true, matching assertRelation and assertPTP so these evaluations
remain complete under the new work-scope contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb88ded3-7d94-4910-8f04-191518cd4d73
📒 Files selected for processing (67)
internal/trackers/data/unit3d.gointernal/trackers/data/unit3d_fixture_test.gointernal/trackers/data/unit3d_test.gointernal/trackers/definition.gointernal/trackers/dupe/evaluator.gointernal/trackers/dupe/evaluator_test.gointernal/trackers/dupe/findings.gointernal/trackers/dupe/normalize.gointernal/trackers/dupe/service.gointernal/trackers/dupe/service_test.gointernal/trackers/dupe/set.gointernal/trackers/dupe/set_test.gointernal/trackers/dupe/types.gointernal/trackers/dupe/types_test.gointernal/trackers/impl/azfamily/definition.gointernal/trackers/impl/azfamily/dupe.gointernal/trackers/impl/azfamily/dupe_test.gointernal/trackers/impl/dupe_handlers_contract_test.gointernal/trackers/impl/dupe_policy_test.gointernal/trackers/impl/standalone/ant/dupe.gointernal/trackers/impl/standalone/ant/dupe_fixture_test.gointernal/trackers/impl/standalone/ant/dupe_test.gointernal/trackers/impl/standalone/ar/dupe.gointernal/trackers/impl/standalone/ar/dupe_test.gointernal/trackers/impl/standalone/asc/dupe.gointernal/trackers/impl/standalone/bhd/dupe.gointernal/trackers/impl/standalone/bhd/dupe_test.gointernal/trackers/impl/standalone/bjs/dupe.gointernal/trackers/impl/standalone/bjs/dupe_test.gointernal/trackers/impl/standalone/bt/dupe.gointernal/trackers/impl/standalone/btn/dupe.gointernal/trackers/impl/standalone/btn/dupe_test.gointernal/trackers/impl/standalone/czt/dupe.gointernal/trackers/impl/standalone/czt/dupe_test.gointernal/trackers/impl/standalone/dc/dupe.gointernal/trackers/impl/standalone/dc/dupe_test.gointernal/trackers/impl/standalone/ff/dupe.gointernal/trackers/impl/standalone/fl/dupe.gointernal/trackers/impl/standalone/gpw/dupe.gointernal/trackers/impl/standalone/gpw/dupe_test.gointernal/trackers/impl/standalone/hdb/dupe.gointernal/trackers/impl/standalone/hds/dupe.gointernal/trackers/impl/standalone/hdt/dupe.gointernal/trackers/impl/standalone/internal/jsondupe/list.gointernal/trackers/impl/standalone/is/dupe.gointernal/trackers/impl/standalone/mtv/dupe.gointernal/trackers/impl/standalone/mtv/dupe_test.gointernal/trackers/impl/standalone/mtv/profile.gointernal/trackers/impl/standalone/nbl/dupe.gointernal/trackers/impl/standalone/ptp/dupe.gointernal/trackers/impl/standalone/ptp/dupe_test.gointernal/trackers/impl/standalone/pts/dupe.gointernal/trackers/impl/standalone/rtf/dupe.gointernal/trackers/impl/standalone/spd/dupe.gointernal/trackers/impl/standalone/spd/dupe_test.gointernal/trackers/impl/standalone/thr/dupe.gointernal/trackers/impl/standalone/tl/dupe.gointernal/trackers/impl/unit3d/dupe.gointernal/trackers/impl/unit3d/sites/aither/profile.gointernal/trackers/impl/unit3d/sites/hhd/profile.gointernal/trackers/impl/unit3d/sites/lume/profile.gointernal/trackers/impl/unit3d/sites/otw/profile.gointernal/trackers/impl/unit3d/sites/sp/profile.gointernal/trackers/impl/unit3d/sites/ulcx/profile.gointernal/trackers/projection.gointernal/trackers/registry.gointernal/trackers/registry_test.go
💤 Files with no reviewable changes (3)
- internal/trackers/impl/standalone/mtv/profile.go
- internal/trackers/impl/azfamily/definition.go
- internal/trackers/registry.go
| if result.WrongWorkCount > 0 { | ||
| result.Warning = appendUnit3DWarning( | ||
| result.Warning, | ||
| fmt.Sprintf("Unit3D search omitted %d rows with conflicting TMDB IDs", result.WrongWorkCount), | ||
| ) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the singular form in the operator-visible warning.
For one omitted row the message reads Unit3D search omitted 1 rows with conflicting TMDB IDs. Use a singular form for one row. The assertion in internal/trackers/data/unit3d_test.go at Line 238 checks the exact substring omitted 1 rows with conflicting TMDB IDs, so update that test with the message.
🐛 Proposed fix
if result.WrongWorkCount > 0 {
+ rows := "rows"
+ if result.WrongWorkCount == 1 {
+ rows = "row"
+ }
result.Warning = appendUnit3DWarning(
result.Warning,
- fmt.Sprintf("Unit3D search omitted %d rows with conflicting TMDB IDs", result.WrongWorkCount),
+ fmt.Sprintf("Unit3D search omitted %d %s with conflicting TMDB IDs", result.WrongWorkCount, rows),
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if result.WrongWorkCount > 0 { | |
| result.Warning = appendUnit3DWarning( | |
| result.Warning, | |
| fmt.Sprintf("Unit3D search omitted %d rows with conflicting TMDB IDs", result.WrongWorkCount), | |
| ) | |
| } | |
| if result.WrongWorkCount > 0 { | |
| rows := "rows" | |
| if result.WrongWorkCount == 1 { | |
| rows = "row" | |
| } | |
| result.Warning = appendUnit3DWarning( | |
| result.Warning, | |
| fmt.Sprintf("Unit3D search omitted %d %s with conflicting TMDB IDs", result.WrongWorkCount, rows), | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/trackers/data/unit3d.go` around lines 463 - 468, Update the warning
construction in the WrongWorkCount handling to use “row” when WrongWorkCount
equals 1 and “rows” otherwise, preserving the existing message for plural
counts. Update the exact assertion in the Unit3D test to expect the corrected
singular message.
| Search: api.DupeSearchEvidence{ | ||
| Complete: search.Complete, | ||
| Complete: effectiveComplete, | ||
| Pages: search.Pages, | ||
| CandidateCount: len(candidates), | ||
| Scope: search.Scope, | ||
| Warnings: cloneNotes(search.Warnings), | ||
| }, | ||
| } | ||
| s.logger.Infof( | ||
| "dupechecking: search tracker=%s state=completed candidates=%d complete=%t candidate_action=%t review_required=%t", | ||
| "dupechecking: search tracker=%s state=completed work_scope=%s received=%d evaluated=%d wrong_work=%d exhaustive=%t effective_complete=%t candidate_action=%t review_required=%t", | ||
| tracker, | ||
| search.WorkScope, | ||
| len(raw), | ||
| len(candidates), | ||
| search.WrongWorkCount, | ||
| search.Complete, | ||
| effectiveComplete, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the public search-evidence schema and all projections/consumers.
ast-grep run --pattern 'type DupeSearchEvidence struct { $$$ }' --lang go internal
rg -n -C 4 '\bDupeSearchEvidence\b|\bWorkScope\b|\bWrongWorkCount\b' internalRepository: autobrr/upbrr
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -u
echo "Tracked files count:"
git ls-files | wc -l
echo "Candidates by filename:"
git ls-files | rg '(^|/)dupe/(service|api).*\.go$|(^|/)api|internal/trackers/dupe|api\.go$' | head -200
echo "Search symbols:"
rg -n -C 3 'DupeSearchEvidence|EffectiveComplete|WorkScope|WrongWorkCount|Duplicate' . --glob '*.go' --glob '*.jsonschema' --glob '*.ts' --glob '*.tsx' --glob '*.js' | head -300Repository: autobrr/upbrr
Length of output: 24428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "DupeSearchEvidence definition:"
sed -n '80,130p' pkg/api/dupes.go | cat -n
echo "DupeAssessmentResult Search evidence fields:"
sed -n '31,230p' pkg/api/workflow_contracts.go | cat -n
echo "internal/trackers/dupe/service.go relevant section:"
sed -n '520,590p' internal/trackers/dupe/service.go | cat -n
echo "Exact occurrences:"
rg -n -C 5 '\bu\.\b|\.Search\b|DupeSearchEvidence|Complete:\s*search\.EffectiveComplete|WorkScope|WrongWorkCount' pkg/api internal/webserver internal/trackers/dupe webui/src/api webui/src --glob '*.go' --glob '*.tsx' --glob '*.ts' | head -300Repository: autobrr/upbrr
Length of output: 38763
Publish duplicate-search result constraints.
DupeSearchEvidence.Complete uses search.EffectiveComplete(), and EffectiveComplete() drops WorkScopeTitle and WorkScopeUnknown. Add WorkScope and WrongWorkCount to pkg/api/dupes.go:DupeSearchEvidence and preserve them in the API/webui projection. Consumers need these fields to tell why an exhaustive adapter result is effectively incomplete and how many rows were excluded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/trackers/dupe/service.go` around lines 558 - 574, Extend
api.DupeSearchEvidence in dupes.go with WorkScope and WrongWorkCount, then
populate and preserve both fields in the duplicate-search result and API/webui
projection around the tracker completion flow. Use the existing search.WorkScope
and search.WrongWorkCount values so consumers can identify effective
incompleteness and excluded-row counts.
Keep distinct Unit3D rows that reuse numeric IDs while still merging true overlaps by ID and name. Prefer candidate folder names for folder-based BT packs and correct singular wrong-work diagnostics.
Reject off-origin AZ pagination URLs before cookies are attached and log pagination outcomes. Distinguish earlier HDS page links from forward navigation.
Stop HDT and IS title fallback searches when no usable title exists. Document CZT's title-first query precedence.
Mark AR and RTF policy fixtures with an authoritative work scope so completeness matches the new contract.
|
CodeRabbit embedded follow-ups: HDS forward-page detection and AZ pagination logging are fixed in 34197b3d; AR/RTF work-scope fixtures are fixed in 2aba0fd4. The generic-filename negative test was not added because any exact basename overlap is an explicit requirement of this refactor. The non-blocking docstring metric was not expanded into boilerplate for internal helpers; repository lint and documentation checks pass. |
Any-single-shared-basename file identity over-blocked in three ways: auxiliary companions (subtitles, nfo) that repeat their names across distinct releases of one work established exact identity between different resolutions; a proposed season pack was blocked as an exact duplicate of a single existing episode instead of trumping it; and known-conflicting sizes no longer vetoed identity for equal file sets. File identity now compares primary video basenames only. Equal video sets are exact when sizes agree or are unknown; a single-video proposal contained in a larger candidate remains exact (existing packs and collections cover the proposal, with no size comparison since the candidate size describes more content); every other overlap defers to pack precedence and slot policy. Partial candidate file lists stay usable through the coverage rule.
… change Scoping file identity to video content with pack-aware coverage changed exact-identity behavior after the v3 bump was already published in this branch. The policy ID feeds RuleFinding rule IDs and the duplicate policy fingerprint that guards captured-assessment lineage, so the semantic change must be visible to both.
The refactor's goal is that evidence records what scope was actually searched, but WorkScope and WrongWorkCount only reached server logs. Carry both on api.DupeSearchEvidence (workflow contracts regenerated) so API and webui consumers can distinguish an exhaustive provider-bound search from an incomplete title fallback and see how many rows were excluded as conflicting works.
Season-pack containment replaced per-tracker opt-in precedence rules with an unconditional general finding keyed only on season numbers and content kind. On title-fallback searches the candidate set is not proven to belong to the proposed work, so a different show sharing a season number could surface existing_preferred and silently block a legitimate upload. Thread the search work scope into candidate findings and emit pack containment only for provider-ID or tracker-group bound searches, where same-work identity is authoritative. Title-fallback searches already require review through effective completeness, which remains the correct outcome for ambiguous candidates.
The single-file stem fallback compared any lone target file's stem to the candidate release name, so a solitary auxiliary file (an nfo whose stem matches the release) could establish exact identity — contradicting the file-identity rule that auxiliary files never do. Gate the fallback on the same video-extension check the file-set comparison uses.
…cope Pack-containment evaluation now requires provider-ID or tracker-group work binding; the shared per-tracker fixture ran without a work scope and lost its season-pack direction expectations.
Slot-difference findings unconditionally take tracker-matched priority since the evidence-driven refactor, leaving this opt-in flag with two setters and no readers. Remove the field and its ar/lst assignments so the policy surface matches actual behavior.
Cover the review-requested generic member-name cases: a shared generic video basename between differing video sets, and identical generic single-file sets with conflicting known sizes, neither of which may establish exact identity.
|
Pushed eight commits to the branch. Three lines in the description are now out of date because of them, and two changes aren't described at all. Flagging rather than editing your PR body. Summary, line 4: "under Behavior, line 4: "any exact proposed/candidate file basename blocks, even when file lists or counts are partial" is no longer accurate. File identity now compares primary video basenames only (5c0e514, with the stem fallback gated the same way in acf76b9). Two concrete cases drove it:
Both are covered by tests now ( Suggested replacement: "exact file identity compares primary video basenames, either as equal video sets with agreeing sizes or a single proposed video contained in a larger candidate; auxiliary files and fuzzy names do not become exact identity" Behavior, line 6: "same-season pack versus episode direction is universal" is now scope-gated (6758a35). Suggested replacement: "same-season pack versus episode direction applies when the search is provider-ID or tracker-group bound; title-fallback candidates fall through to review rather than directional precedence" Not currently described:
|
| warnings := []string{"BTN search is bounded to 50 results"} | ||
| return dupe.ResolvedWithSearch(entries, nil, dupe.SearchEvidence{ | ||
| WorkScope: workScope, | ||
| Pages: 1, | ||
| Scope: "bounded_result_set", | ||
| Warnings: warnings, | ||
| }) |
There was a problem hiding this comment.
Didn't feel comfortable making a decision here, so flagging for you.
This literal leaves Complete at the zero value, and EffectiveComplete() (dupe/types.go:239-241) needs both halves:
return e.Complete && (e.WorkScope == WorkScopeProviderID || e.WorkScope == WorkScopeTrackerGroup)so BTN never returns an effectively-complete search on any path, including the trackerID(meta) != "" case that sets WorkScopeTrackerGroup.
Downstream that becomes:
evaluator.go:79-81:!effectiveCompletesetsRequiresAction = trueregardless, even when the search came back with zero candidates.workflow_dupes.go:349:result.HasDupes || !result.Search.CompletegivesDupeDecisionPendingplusRequiredActionReviewDuplicates.onEvidencedefaults toblockfor plain--unattended(releaseworkflow/composite_upload.go:295-302), so the lane blocks on every run, including for releases with nothing on the tracker.dupe_check/index.tsx:65-70gates its risk-acknowledgement banner onsearch.complete === false, and the backend synthesizes aninsufficient_evidencematch for display (workflow_dupes.go:299-318), so a clean release on one of these trackers renders as "1 potential dupe · review" with a danger-toned candidate that has no actual duplicate behind it.
isTV at line 45 rejects everything except TV, so this covers all of BTN's supported content. Pack precedence takes a hit too, since collectPackContainmentFinding only runs for provider/tracker-group scope and the imdb/tvdb/searchstr branches don't qualify.
The part I didn't want to just "fix": the "BTN search is bounded to 50 results" warning is accurate. The API really is capped, so Complete: false is honest rather than an oversight. But a capped endpoint can never prove exhaustion, so under the current model BTN is permanently unusable in unattended flows. Feels like the model is missing a way to say "we got everything the API will give us, bound to one work" separately from "we stopped early or couldn't bind the work".
Two groups land in the same place for different reasons:
Completenever set, so effective completeness is impossible regardless of scope:asc,bjs,bt,btn,ff,fl,hdt,is,pts(grep -c "Complete:" internal/trackers/impl/standalone/*/dupe.goreturns 0 for each).WorkScopehardcoded to title with no other branch, so effective completeness and pack precedence are both out of reach even when enumeration is genuinely exhaustive:ar(ar/dupe.go:150),czt(czt/dupe.go:139),tl(tl/dupe.go:85). All three handle TV.
So whatever gets decided here applies to about half the registered adapters.
Options as I see them:
- Adapters set
Complete: trueonce they've enumerated everything available and the search is work-bound, keeping the warning for the cap. - Add a third state (something like
bounded) so consumers can tell capped enumeration apart from genuine incompleteness. - Leave it and document that these trackers always need review.
| t.Fatalf("unexpected ASC entries: %#v", entries) | ||
| } | ||
| }, | ||
| scope: dupe.WorkScopeProviderID, |
There was a problem hiding this comment.
Pinning scope/enumerated/effective per adapter is a good addition, and the values that are here match the adapters (I checked each one). The gap is coverage: this table pins 11 of the 25 registered dupe adapters.
Pinned: ASC, BT, FL, FF, BJS, HDS, HDT, IS, PTS, THR, TL.
Not pinned: ANT, AZ-family, BHD, BTN, CZT, DC, GPW, HDB, MTV, NBL, PTP, RTF, SPD, Unit3D.
BTN is the one I'd most want covered, since it can never reach EffectiveComplete on any path (see my other comment) and nothing in the suite would catch that today. Making the table total would also mean that whichever way the bounded-vs-incomplete question goes, the blast radius shows up as one test diff instead of being discovered per tracker later.
Related, in dupe_policy_test.go: assertRelation for AR (line 208-211) and assertRTF (line 278-281) both evaluate under WorkScope: dupe.WorkScopeTrackerGroup, but neither adapter can emit that scope at runtime. ar/dupe.go:150 sets WorkScopeTitle unconditionally, and rtfWorkScope (rtf/dupe.go:155-160) returns only WorkScopeProviderID or WorkScopeTitle. That matters for the AR season-pack subtests specifically: pack containment only runs for provider/tracker-group scope, so those assertions pass under a scope AR never produces, while the real AR path can't reach pack precedence at all. PTP's use of WorkScopeTrackerGroup at line 384-386 is correct, its adapter really does bind a torrent group.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/trackers/impl/standalone/btn/dupe.go (3)
216-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve
trackerID(meta)once.The switch calls
trackerID(meta)for the case test and again for the assignment. One local variable removes the duplicate call and keeps the two values identical.♻️ Proposed refactor
title := searchTitle(meta) date, daily := btnDailyDate(meta.DailyEpisodeDate) + groupID := trackerID(meta) filter := make(map[string]any) workScope := dupe.WorkScopeUnknown switch { - case trackerID(meta) != "": + case groupID != "": workScope = dupe.WorkScopeTrackerGroup - filter["id"] = trackerID(meta) + filter["id"] = groupID🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/trackers/impl/standalone/btn/dupe.go` around lines 216 - 241, In btnDupeFilter, resolve trackerID(meta) once into a local variable before the switch, then use that variable for both the case condition and filter assignment while preserving the existing workScope and filter behavior.
69-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
maxPagesfallback.
deps.MaxPages(100)already returns 100 when no policy value applies, sos.maxPagesis never<= 0for the constructed adapter. The local fallback duplicates the default in two places. If you keep the guard for zero-value structs built in tests, move the constant to a package-level identifier so both sites stay in sync.♻️ Proposed refactor
+const btnDupeMaxPages = 100 + - maxPages := s.maxPages - if maxPages <= 0 { - maxPages = 100 - } + maxPages := s.maxPages + if maxPages <= 0 { + maxPages = btnDupeMaxPages + }Then use
deps.MaxPages(btnDupeMaxPages)innewDuplicateAdapter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/trackers/impl/standalone/btn/dupe.go` around lines 69 - 79, Remove the local maxPages fallback in the duplicate-tracking flow and rely on the adapter’s configured value. If zero-value test structs must remain supported, define a package-level default identifier and reuse it in both this initialization and newDuplicateAdapter’s deps.MaxPages call.
80-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd decision-point logging to the pagination loop.
The BTN adapter drops its logger (
_ = logger), so this multi-request loop emits no operator-visible evidence. Sibling adapters log the same decisions:internal/trackers/impl/standalone/hdb/dupe.gologspages,complete, and per-page failure codes, andinternal/trackers/impl/standalone/ar/dupe.gologs pages, advertised pages, accepted results, and the final decision. Without those logs, a truncated or warning-bearing BTN search cannot be diagnosed from logs alone.Store the logger on
dupeSearcherand add stable key/value fields for the per-page failure branch and the final outcome. Do not log the token or the filter values that carry it.As per coding guidelines: "Keep logging levels purposeful, add operator-visible progress and decision-point logs, use warnings for blocked outcomes, DEBUG for troubleshooting context, TRACE for detailed flow, and stable key/value-style fields."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/trackers/impl/standalone/btn/dupe.go` around lines 80 - 144, Update dupeSearcher to retain the provided logger instead of discarding it, then instrument the BTN pagination loop with stable key/value logging for per-page failure details and the final search outcome, including pages and completion/warning state as appropriate. Add operator-visible progress or decision-point logs at purposeful levels, using warnings for blocked or truncated outcomes and DEBUG/TRACE for troubleshooting detail. Do not include the token or filter values in any log fields.Source: Coding guidelines
internal/trackers/impl/standalone/btn/dupe_test.go (1)
255-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the partial-failure path.
The new pagination tests cover the success path and the daily one-shot path. They do not cover the branch where a later page fails after page 1 succeeded. That branch keeps the already collected entries and sets the warning "BTN search stopped after a partial request failure". The mock transport already returns an error for any request beyond the supplied response sequence, so a single-response sequence with a reported total of 3 exercises the branch directly.
Add a test that supplies one page with fewer rows than the reported count, then asserts that the entries survive,
Completeisfalse, and the partial-failure warning is present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/trackers/impl/standalone/btn/dupe_test.go` around lines 255 - 320, Add a test alongside TestBTNHandlerPaginatesUntilReportedTotal using a single mock response reporting 3 results but containing fewer rows, so the next pagination request fails. Assert the collected entries are preserved, SearchEvidence().Complete is false, and the warnings include exactly “BTN search stopped after a partial request failure”; also verify the expected initial request parameters if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/trackers/impl/standalone/btn/dupe_test.go`:
- Around line 255-320: Add a test alongside
TestBTNHandlerPaginatesUntilReportedTotal using a single mock response reporting
3 results but containing fewer rows, so the next pagination request fails.
Assert the collected entries are preserved, SearchEvidence().Complete is false,
and the warnings include exactly “BTN search stopped after a partial request
failure”; also verify the expected initial request parameters if needed.
In `@internal/trackers/impl/standalone/btn/dupe.go`:
- Around line 216-241: In btnDupeFilter, resolve trackerID(meta) once into a
local variable before the switch, then use that variable for both the case
condition and filter assignment while preserving the existing workScope and
filter behavior.
- Around line 69-79: Remove the local maxPages fallback in the
duplicate-tracking flow and rely on the adapter’s configured value. If
zero-value test structs must remain supported, define a package-level default
identifier and reuse it in both this initialization and newDuplicateAdapter’s
deps.MaxPages call.
- Around line 80-144: Update dupeSearcher to retain the provided logger instead
of discarding it, then instrument the BTN pagination loop with stable key/value
logging for per-page failure details and the final search outcome, including
pages and completion/warning state as appropriate. Add operator-visible progress
or decision-point logs at purposeful levels, using warnings for blocked or
truncated outcomes and DEBUG/TRACE for troubleshooting detail. Do not include
the token or filter values in any log fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd40ca58-363b-4e4c-98d0-5e275b2f320e
📒 Files selected for processing (2)
internal/trackers/impl/standalone/btn/dupe.gointernal/trackers/impl/standalone/btn/dupe_test.go
|
Addressed all four CodeRabbit nitpicks in 43943690: BTN now retains and uses its logger for page failures and final completion decisions, removes the redundant runtime max-page fallback, resolves the tracker group ID once, and covers preservation of first-page entries when a later page request fails. |
2 participants
Closes #325.
Supersedes #326. Builds on the tracker evidence and policy work in #316.
Summary
general/duplicate/v4SlotDifferencesOverrideGeneralconfiguration now that tracker-matched slot findings always take priorityWhy
#325 exposed a byte-identical duplicate that passed because its resolved year differed from the tracker's name. The year comparison was one symptom of a broader flow problem: adapters mixed discovery with policy, discarded rows using the proposed upload's current slot, and reported endpoint exhaustion as complete even when the query did not prove same-work coverage. Once a candidate was filtered out, the evaluator could not recover it regardless of size, files, provider context, or tracker policy.
#326 addresses the reported year case with provider-aware name matching and metadata precedence. This PR instead fixes the boundary that allowed that case and related false negatives: searches return the broadest defensible same-work candidate set, evidence records what scope was actually searched, and the evaluator alone decides whether candidates conflict or coexist. This also follows the signal from #316, where tracker-specific disc and release-slot fixes showed that discovery filters and policy decisions were spread across adapters.
Behavior
coexistsresults remain retained evidence but stay outside the actionable duplicate listValidation
make test-gogo test -race -v -timeout 20m ./internal/webserver/... ./pkg/apimake test-frontendmake lintmake logpolicymake pathpolicymake gofix-check-changedgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes