feat(btn): implement evidence-backed full handling - #331
Open
Audionut wants to merge 10 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe change adds BTN duplicate detection, typed metadata mapping, release-origin and resolution comparisons, upload naming and validation rules, paginated reservation checks, workflow output updates, and stale-cache fallback coverage. ChangesBTN duplicate evaluation
Estimated code review effort: 5 (Critical) | ~90 minutes 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 |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
internal/trackers/impl/standalone/btn/upload.go (1)
1310-1346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog evidence-unavailable decisions.
The canonical-season, API-error, missing-release-name, and invalid-timestamp branches block the upload without a warning log. Emit a
Warnfwith a stabledecision=evidence_unavailablefield before these returns. Do not include the remote error or response content.As per coding guidelines, “use warnings for blocked outcomes.”
🤖 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/upload.go` around lines 1310 - 1346, Add a Warnf with stable decision=evidence_unavailable context immediately before each evidence-unavailable return in the BTN reservation check: missing canonical season, btnAPISearchTorrents failure, missing release name, and invalid matching timestamp. Keep remote error and response content out of these warnings, while preserving the existing error returns and blocked/allowed logs.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 `@docs/trackerdata/BTN_FULL_HANDLING_IMPLEMENTATION_PLAN.md`:
- Around line 62-72: Label the “Current-State Findings” section in the
implementation plan as pre-implementation behavior, such as by renaming it to
“Pre-implementation Findings.” Ensure the statements about the 50-result cap,
empty DupeEntry.Source, and missing BTN DupePolicy are clearly presented as
historical findings rather than the current contract.
- Around line 37-39: Clarify the daily-show search rules in the implementation
plan: specify whether a daily response whose returned row count matches BTN’s
reported results count qualifies as an authoritative date-and-category scope for
public complete=true. If it does, document that explicit exception and its
conditions; otherwise require all daily responses to remain incomplete.
In `@docs/trackerdata/EVIDENCE.md`:
- Around line 8-11: Update the completion contract in EVIDENCE.md so daily
searches explicitly either qualify as an exception to provider/tracker-group
binding or never return complete=true; align the later BTN daily-search guidance
and implementation plan with that same rule, avoiding wording that treats a
count-matching daily response as complete by default.
- Around line 142-143: Align the Phase 3 disposition for internal, language, and
pilot coexistence across docs/trackerdata/EVIDENCE.md (lines 142-143) and
docs/trackerdata/BTN_FULL_HANDLING_IMPLEMENTATION_PLAN.md (lines 210-225):
either remove these cases from the automatic plan or document the exact typed
facts, evidence ID, policy, and tests that justify automation. Ensure both
documents consistently reflect the same disposition.
In `@internal/metadata/tracker_data_test.go`:
- Around line 542-553: Update the test around loadBTNClaimedTitles to make the
initial request succeed and the subsequent claimed-titles refresh request fail
with context.Canceled. Record transport calls, assert the expected request count
before validating the stale cached title fallback, and keep the existing
fallback assertion.
In `@internal/trackers/dupe/set.go`:
- Line 190: Update the predicate comparison branch using compareDimensionFacts
so DimensionUnknown is added to missing alongside other non-matching results,
while preserving the existing DimensionEqual handling. Ensure ambiguous SD
versus concrete SD resolution is treated as indeterminate and the capacity rule
fails closed.
In `@internal/trackers/impl/standalone/btn/definition.go`:
- Around line 51-52: Update the normalization in isBTNInternalGroupName to trim
surrounding whitespace before calling strings.TrimPrefix, then retain the
existing lowercase normalization and internal-group matching behavior.
In `@internal/trackers/impl/standalone/btn/dupe_policy.go`:
- Around line 45-60: Require authoritative bilateral facts by setting
RequiresComplete: true on the generic different-value condition, the WEB source
condition in btnWEBCodecRule, and the related codec condition. Preserve
ambiguous or partial-fact cases as deferred for review rather than allowing
evaluateTrackerRule to make an automatic decision.
In `@internal/trackers/impl/standalone/btn/dupe.go`:
- Around line 503-509: Update firstPresent to skip entries whose value is nil,
continuing through subsequent aliases instead of returning them as present; only
return true for non-nil values, preserving the existing false result when no
usable alias exists. Add or update a btnFlags test covering null HDR fields to
verify flagsPresent and flagsComplete remain false when no non-null HDR evidence
is available.
In `@internal/trackers/impl/standalone/btn/upload_integration_test.go`:
- Around line 1898-1906: The httptest server handler must validate pagination
offsets instead of selecting responses solely by call count. Decode each
request’s JSON-RPC parameters, assert offset zero on the first request and the
expected next-page offset on the second, then retain the existing page-specific
responses and unexpected-request failure behavior.
In `@internal/trackers/impl/standalone/btn/upload.go`:
- Around line 1386-1390: Update the page-torrent aggregation loop to reject
duplicate IDs before assigning to torrents: after validating the trimmed ID in
the existing search flow, check whether id already exists in torrents and return
a malformed-evidence error if so; only insert unseen IDs.
---
Nitpick comments:
In `@internal/trackers/impl/standalone/btn/upload.go`:
- Around line 1310-1346: Add a Warnf with stable decision=evidence_unavailable
context immediately before each evidence-unavailable return in the BTN
reservation check: missing canonical season, btnAPISearchTorrents failure,
missing release name, and invalid matching timestamp. Keep remote error and
response content out of these warnings, while preserving the existing error
returns and blocked/allowed logs.
🪄 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: 53594069-0074-41fb-a706-58f41aae392b
⛔ Files ignored due to path filters (1)
webui/src/api/generated/release-workflow.tsis excluded by!**/generated/**
📒 Files selected for processing (30)
docs/trackerdata/BTN_FULL_HANDLING_IMPLEMENTATION_PLAN.mddocs/trackerdata/EVIDENCE.mdinternal/metadata/tracker_data_test.gointernal/trackers/definition.gointernal/trackers/dupe/candidate.gointernal/trackers/dupe/evaluator.gointernal/trackers/dupe/findings.gointernal/trackers/dupe/normalize.gointernal/trackers/dupe/normalize_test.gointernal/trackers/dupe/service_test.gointernal/trackers/dupe/set.gointernal/trackers/dupe/types.gointernal/trackers/impl/responsibility_ledger_test.gointernal/trackers/impl/standalone/btn/definition.gointernal/trackers/impl/standalone/btn/definition_test.gointernal/trackers/impl/standalone/btn/dupe.gointernal/trackers/impl/standalone/btn/dupe_policy.gointernal/trackers/impl/standalone/btn/dupe_policy_test.gointernal/trackers/impl/standalone/btn/dupe_test.gointernal/trackers/impl/standalone/btn/name.gointernal/trackers/impl/standalone/btn/name_test.gointernal/trackers/impl/standalone/btn/profile.gointernal/trackers/impl/standalone/btn/upload.gointernal/trackers/impl/standalone/btn/upload_integration_test.gointernal/trackers/impl/standalone/btn/validation.gointernal/trackers/impl/standalone/btn/validation_test.gointernal/trackers/projection.gointernal/webserver/openapi/release-workflow-v1.jsonpkg/api/dupes.gopkg/api/workflow_contracts.go
Contributor
Author
|
Fixed the embedded reservation logging nitpick in 5aca6f60: evidence-unavailable paths now log safe reason-only decisions without remote errors or payloads. |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/workflow_dupes.go (1)
271-300: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winProject
ReleaseOriginin public workflow matches.
publicDupeMatchescopies candidate facts but omitsevaluation.ReleaseOrigin. This drops BTN origin evidence from the workflow API. AddReleaseOrigintoapi.DupeMatchProjectionhere and add regression coverage.As per coding guidelines, preserve CLI and WebUI behavior where workflows are shared.
🤖 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/core/workflow_dupes.go` around lines 271 - 300, Update the api.DupeMatchProjection construction in publicDupeMatches to copy evaluation.ReleaseOrigin, preserving the candidate’s origin evidence in public workflow matches. Add regression coverage verifying ReleaseOrigin is included, without changing shared CLI or WebUI behavior.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/impl/standalone/btn/dupe_policy_test.go`:
- Around line 47-60: Correct the “web without region ignores pal ntsc” test
fixtures so both target and candidate use the same episode, and add explicit
opposing PAL/NTSC evidence to exercise the Web-region rule. Keep the expected
DupeRelationCoexists result and the existing btnPolicyTarget/btnPolicyCandidate
setup.
---
Outside diff comments:
In `@internal/core/workflow_dupes.go`:
- Around line 271-300: Update the api.DupeMatchProjection construction in
publicDupeMatches to copy evaluation.ReleaseOrigin, preserving the candidate’s
origin evidence in public workflow matches. Add regression coverage verifying
ReleaseOrigin is included, without changing shared CLI or WebUI behavior.
🪄 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: 476fd375-27ed-4834-920c-cf1eb2a7978f
📒 Files selected for processing (24)
internal/core/workflow_dupes.gointernal/core/workflow_dupes_test.gointernal/core/workflow_preflight.gointernal/metadata/tracker_data_test.gointernal/trackers/dupe/evaluator.gointernal/trackers/dupe/evaluator_test.gointernal/trackers/dupe/findings.gointernal/trackers/dupe/service.gointernal/trackers/dupe/service_test.gointernal/trackers/dupe/set.gointernal/trackers/dupe/set_test.gointernal/trackers/impl/standalone/btn/definition.gointernal/trackers/impl/standalone/btn/dupe.gointernal/trackers/impl/standalone/btn/dupe_policy.gointernal/trackers/impl/standalone/btn/dupe_policy_test.gointernal/trackers/impl/standalone/btn/dupe_test.gointernal/trackers/impl/standalone/btn/upload.gointernal/trackers/impl/standalone/btn/upload_integration_test.gointernal/trackers/impl/standalone/btn/validation.gointernal/trackers/impl/standalone/btn/validation_test.gointernal/trackers/projection.gointernal/trackers/projection_test.gowebui/src/pages/dupe_check/index.test.tsxwebui/src/pages/dupe_check/index.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/trackers/impl/standalone/btn/validation.go
- internal/trackers/dupe/set.go
- internal/trackers/dupe/findings.go
- internal/metadata/tracker_data_test.go
- internal/trackers/impl/standalone/btn/validation_test.go
- internal/trackers/impl/standalone/btn/definition.go
- internal/trackers/impl/standalone/btn/dupe_policy.go
- internal/trackers/impl/standalone/btn/upload.go
- internal/trackers/projection.go
- internal/trackers/impl/standalone/btn/dupe_test.go
- internal/trackers/impl/standalone/btn/upload_integration_test.go
Comment on lines
+47
to
+60
| { | ||
| name: "web without region ignores pal ntsc", | ||
| target: func() api.TrackerDuplicateTarget { | ||
| target := btnPolicyTarget("WEB-DL", "1080p", "H.264", "P2P") | ||
| target.Episode = 12 | ||
| return target | ||
| }(), | ||
| candidate: func() dupe.TrackerCandidate { | ||
| candidate := btnPolicyCandidate("WEB-DL", "1080p", "H.264", "P2P") | ||
| candidate.Episode = 13 | ||
| return candidate | ||
| }(), | ||
| want: api.DupeRelationCoexists, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the stated PAL/NTSC behavior.
The fixtures use different episodes and add no PAL or NTSC evidence. Coexists can result from disjoint episodes, so this test does not protect the Web-region rule. Use the same episode and explicit PAL/NTSC evidence.
Proposed test correction
target := btnPolicyTarget("WEB-DL", "1080p", "H.264", "P2P")
target.Episode = 12
+target.Names = []string{"Example.Show.S01E12.PAL.WEB-DL-GRP"}
candidate := btnPolicyCandidate("WEB-DL", "1080p", "H.264", "P2P")
-candidate.Episode = 13
+candidate.Episode = 12
+candidate.Name = "Example.Show.S01E12.NTSC.WEB-DL-GRP"📝 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.
Suggested change
| { | |
| name: "web without region ignores pal ntsc", | |
| target: func() api.TrackerDuplicateTarget { | |
| target := btnPolicyTarget("WEB-DL", "1080p", "H.264", "P2P") | |
| target.Episode = 12 | |
| return target | |
| }(), | |
| candidate: func() dupe.TrackerCandidate { | |
| candidate := btnPolicyCandidate("WEB-DL", "1080p", "H.264", "P2P") | |
| candidate.Episode = 13 | |
| return candidate | |
| }(), | |
| want: api.DupeRelationCoexists, | |
| }, | |
| { | |
| name: "web without region ignores pal ntsc", | |
| target: func() api.TrackerDuplicateTarget { | |
| target := btnPolicyTarget("WEB-DL", "1080p", "H.264", "P2P") | |
| target.Episode = 12 | |
| target.Names = []string{"Example.Show.S01E12.PAL.WEB-DL-GRP"} | |
| return target | |
| }(), | |
| candidate: func() dupe.TrackerCandidate { | |
| candidate := btnPolicyCandidate("WEB-DL", "1080p", "H.264", "P2P") | |
| candidate.Episode = 12 | |
| candidate.Name = "Example.Show.S01E12.NTSC.WEB-DL-GRP" | |
| return candidate | |
| }(), | |
| want: api.DupeRelationCoexists, | |
| }, |
🤖 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_policy_test.go` around lines 47 -
60, Correct the “web without region ignores pal ntsc” test fixtures so both
target and candidate use the same episode, and add explicit opposing PAL/NTSC
evidence to exercise the Web-region rule. Keep the expected DupeRelationCoexists
result and the existing btnPolicyTarget/btnPolicyCandidate setup.
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.
Dependency
Summary
SDwith concrete 480/576 evidence at the shared dupe normalization root.Scope
General movies remain unsupported. Manual TV-movie/special questionnaires, separate folder-name output, and internal/language/pilot rules lacking authoritative bilateral facts remain deferred; mixed packs and staff-owned decisions stay reviewable.
Validation
make lintmake test-gomake test-frontendmake backendmake gofix-check-changed,make logpolicy, contract generation check, andgit diff --checkLinks
Summary by CodeRabbit
New Features
Bug Fixes