Skip to content

chore(trackers): log which rename signal fired - #262

Open
maksii wants to merge 1 commit into
mainfrom
chore/log-rename-signal
Open

chore(trackers): log which rename signal fired#262
maksii wants to merge 1 commit into
mainfrom
chore/log-rename-signal

Conversation

@maksii

@maksii maksii commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

isRenamedRelease fires on three separate heuristics — the srrdb scene comparison, an *arr rename token, and the space-rename check — but reports a deliberately generic user-facing reason for all three, since the reason is disclosed and must not reveal which detection tripped.

That left no way to tell them apart when diagnosing a false positive. A user reports "modified_release fired and it shouldn't have" and there is nothing in the logs saying which of the three did it.

isRenamedRelease now returns the signal name (srrdb, arr-token, space-rename) as a third value, and EvaluateRules logs it at debug alongside the source path, video path and group. The disclosed reason is unchanged — the signal is logging-only, which the doc comment states explicitly so nobody wires it into user-facing output later.

Pure observability, no behaviour change. go build ./..., go vet and the trackers suite (28 packages) pass.

Summary by CodeRabbit

  • Bug Fixes

    • Improved modified-release detection with clearer diagnostic classifications.
    • Applied stricter handling for database-confirmed release changes while allowing other detected changes to be waived.
    • Preserved user-facing explanations and prevented local file paths from appearing in diagnostics.
    • Improved tracking logs with consistent release-change details and dispositions.
  • Tests

    • Expanded coverage for renamed releases, including common path and naming variations.
    • Added validation for clean-release behavior and diagnostic consistency.

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7afb5760-476c-4865-8827-719c26a7393a

📥 Commits

Reviewing files that changed from the base of the PR and between 21ef3b0 and b59e9e1.

📒 Files selected for processing (4)
  • internal/releasepolicy/modified.go
  • internal/releasepolicy/modified_test.go
  • internal/trackers/rules.go
  • internal/trackers/rules_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • internal/trackers/rules.go
  • internal/releasepolicy/modified.go
  • internal/trackers/rules_test.go
  • internal/releasepolicy/modified_test.go

📝 Walkthrough

Walkthrough

The release policy now returns structured modified-release detections with stable signals. Tracker rules use SRRDB signals for strict failures and other signals for waivable failures. Diagnostic logging records signals and dispositions without exposing source paths.

Changes

Modified-release diagnostics

Layer / File(s) Summary
Structured detection contract
internal/releasepolicy/modified.go, internal/releasepolicy/modified_test.go
DetectModifiedRelease returns modification status, reason, and a diagnostic signal. Tests cover SRRDB, *arr-token, whitespace-rename, and zero-value results.
Tracker disposition and diagnostic logging
internal/trackers/rules.go, internal/trackers/rules_test.go
Tracker rules apply strict disposition only to SRRDB signals. Tests verify waivable heuristic detections and metadata-only diagnostic logs.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ReleasePolicy
  participant EvaluateRules
  participant DebugLogger
  ReleasePolicy->>EvaluateRules: Return modified-release detection
  EvaluateRules->>EvaluateRules: Derive disposition from signal
  EvaluateRules->>DebugLogger: Record signal and disposition
Loading

Possibly related PRs

Poem

A rabbit found three signals bright,
SRRDB strict, heuristics light.
The tracker logs the trail,
While hidden paths stay pale.
Clean releases leave no trace.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: logging which rename signal triggered modified-release detection.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/log-rename-signal

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Audionut

Audionut commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

No code findings. This remains valid after the latest #273 changes: equivalent signal-level diagnostics are still absent.

Current state after #273

releasepolicy.DetectModifiedRelease owns all three detection paths—authoritative SRRDB comparison, *arr ID token, and whitespace rename—but returns only (bool, reason).

trackers.evaluateRules consumes that result through api.RuleSubject, preserves the generic user-facing failure, and currently derives strict-versus-waivable disposition by separately re-reading SceneRenamed. No log identifies which detection fired.

Changes required after #273 merges

  • Rebase onto the updated main; move detection changes to internal/releasepolicy/modified.go and focused tests to modified_test.go.

  • Replace the positional third string with a typed result, for example ModifiedReleaseDetection{Modified, Reason, Signal} plus a ModifiedReleaseSignal enum containing stable values for srrdb, arr-token, and space-rename. The zero value must represent no detection.

  • Keep detection and signal classification in internal/releasepolicy; keep tracker rule disposition and logging in internal/trackers/rules.go.

  • Derive disposition from the returned signal (srrdb strict; heuristic signals waivable). Do not separately re-read RuleSubject.SceneRenamed, which would create a second authority for the same decision.

  • Preserve the current generic disclosed reason exactly. Signal names are diagnostic-only and must not enter RuleFailure.Reason, operation failures, workflow snapshots, API contracts, or WebUI output.

  • Emit one debug log only when the rule fires, using stable fields such as:

    trackers: rule matched tracker=%s rule=modified_release signal=%s disposition=%s

  • The signal itself is sufficient for the reported false-positive diagnosis. Do not add full local paths. If filename context is proven necessary, log only sanitized basenames as separate fields; never source/video absolute paths.

  • Preserve personal-release/disc exemptions, skipped-group behavior, candidate order, generic reason, and strict-versus-waivable behavior.

  • Keep fixtures synthetic.

Required coverage

  • assert the exact typed signal for representative SRRDB, *arr token, and whitespace detections;
  • assert no signal for clean, exempt, and skipped-group cases;
  • assert the signal never appears in the user-facing reason;
  • capture the tracker debug log and verify exact tracker/rule/signal/disposition fields;
  • verify SRRDB remains strict and both heuristic signals remain waivable across representative tracker families;
  • verify no absolute source/video path is logged.

Validation

go test -race -v -timeout 20m ./internal/releasepolicy ./internal/trackers
make gofix-check-changed
make logpolicy
make lint
make test-go
make backend
git diff --check

No shared API, workflow, CLI, or WebUI contract change is required.

@maksii
maksii force-pushed the chore/log-rename-signal branch from 22106d0 to 21ef3b0 Compare August 2, 2026 08:13
DetectModifiedRelease fires on three separate detections — the authoritative
srrdb scene comparison, an *arr rename token, and the whitespace rename check —
but reported a deliberately generic user-facing reason for all three, since the
reason is disclosed and must not reveal which detection tripped. That left no
way to tell them apart when diagnosing a false positive.

It now returns a typed ModifiedReleaseDetection carrying the signal that fired
as a stable ModifiedReleaseSignal (srrdb, arr-token, space-rename); the zero
value means no detection. Detection and signal classification stay in
releasepolicy, while disposition and logging stay in the tracker rules.

evaluateRules derives the strict-versus-waivable disposition from that signal
instead of separately re-reading SceneRenamed, so a single authority decides it
— srrdb strict, both heuristics waivable, unchanged from before. When the rule
fires it emits one debug line with stable fields:

    trackers: rule matched tracker=%s rule=modified_release signal=%s disposition=%s

The signal is diagnostic-only: it never enters the disclosed failure reason,
operation failures, workflow snapshots, API contracts, or WebUI output, and the
log carries no source or video paths. Personal-release and disc exemptions,
skipped-group behavior, candidate order, and the generic reason are unchanged.

Pure observability, no behavior change. Also reindents a pre-existing
misformatted test block in rules_test.go that gofmt flagged in passing.
@Audionut

Audionut commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

I missed what this was doing earlier.

I am strongly opinionated against providing any avenue for a user to determine what triggered the signal. A rename could also indicate a hash mismatch, and when this signal fires, users should properly access the lineage of the file.

A logging signal just wires in a potential short-circuit to proper lineage checking. Scene renames remain hard blocked, but the other cases are waivable after #273

I need to take a moment to process my own internal conflict regarding UTP. I am not a fan, at all, of their stance on renaming releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants