Honour a review decision over the match, and keep credentials out of error messages

[alperien]

Two real bugs found by fix/robustness-audit, ported onto current main rather than merged.

Why ported and not merged: that branch forked from c152e39, before PR #3. Merging it in either direction reverts monitor="existing" back to monitor="none" (the headline v0.1.0 bug — every pushed album silently unmonitored), swaps content-derived state keys back to rank-based ones, and drops alias search and the live/compilation demotion entirely. The findings are worth having; the base underneath them is not.

A skip was ignored on a row that also matched

import_set took the automatic match whenever there was one and only consulted your decision otherwise. So a row you skipped, which then matched on a later --rematch, went to Lidarr anyway — and a re-pick lost to the candidate the matcher had led with.

A decision now outranks the match result: a skip is respected wherever it appears, an accept always wins, and an undone decision (u, from #4) falls back to the match. A matched row carrying no release group id — possible in a state file from an older version — is dropped rather than failing at Lidarr's lookup.

A Lidarr URL with credentials appeared in error messages

http://user:pw@host is how you get through a reverse proxy that wants basic auth. The whole URL was quoted back in connection errors, timeouts, non-JSON replies, and any proxy error page that echoed the request. Errors get pasted into bug reports.

The password is stripped from what's printed; the request is unchanged and still authenticates. Verified both ways — the message shows http://lidarr.test:8686, the wire still carries admin:hunter2.

The branch's other two claims were already fixed on main

• Its Unreachable is main's SearchUnavailable — same semantics, and main's also honours Retry-After and treats all 4xx as a real answer.
• Its error normalisation is a subset of main's, which additionally keeps the InvalidSchema/MissingSchema handling the branch dropped (that's the "you typed 127.0.0.1:8686 without http://" message).

Its tied-rank fix is moot: main keys state by sha1(artist+title), so ranks aren't keys at all.

Verification

• 13 new tests, 135 passing on Python 3.10 and 3.12; ruff clean
• uv build + twine check --strict pass
• Both bugs reproduced before and confirmed fixed after

The start-downloads redesign from that branch is deliberately not here — it rests on a claim about searchForNewAlbum I'm verifying separately, and it deserves its own PR.

Summary by CodeRabbit

• Bug Fixes
  • Explicit review decisions now take precedence over automatic matching results.
  • Skipped, undecided, or incomplete entries are no longer sent for import.
  • Older state entries without release group IDs are handled safely.
  • Lidarr error messages now hide passwords and provide clearer connection and request details.
  • Authenticated Lidarr requests continue to work while credentials remain protected in displayed errors.

[coderabbitai]

📝 Walkthrough

Walkthrough

The import flow now gives explicit review decisions precedence over automatic matches and skips rows without release group IDs. Lidarr errors redact embedded credentials while authenticated requests continue using the original URL, with tests covering both behaviors.

Changes

Import selection precedence

Layer / File(s)	Summary
Decision-aware import selection chartarr/cli.py, tests/test_cli.py, CHANGELOG.md	import_set() applies skip and accept decisions before matched results, falls back after cleared decisions, excludes incomplete rows, and documents the fixes in the changelog.

Lidarr credential scrubbing

Layer / File(s)	Summary
Credential-safe Lidarr errors chartarr/lidarr.py, tests/test_lidarr.py	Lidarr sanitizes credentials in displayed URLs and error responses while retaining authenticated request URLs, with coverage for URL parsing and failure modes.

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

Suggested reviewers: claude

🚥 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 summarizes the two main changes: review decisions now override matches, and credentials are scrubbed from Lidarr error messages.
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 port/robustness

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@chartarr/lidarr.py`:
- Around line 120-124: Update the LidarrError construction in the HTTP error
branch to avoid retaining raw r.text in its public body field. Store the
scrubbed response body instead, ensuring add_album() cannot reintroduce
credentials when interpolating e.body into duplicate-album errors.
- Around line 39-48: Update _safe_url to redact userinfo from both successfully
parsed URLs and ValueError fallback strings before returning them. Use
parts.netloc when rebuilding parsed URLs, removing any text through the final
“@” while preserving the host, port, path, query, and fragment; ensure malformed
authorities that raise during urlsplit are sanitized before the fallback is
returned and stored in self.shown.
- Around line 99-114: Update the exception handlers in the Lidarr request flow
to raise each sanitized LidarrError with suppressed exception chaining instead
of `from e`. Apply this to the ConnectionError, Timeout, invalid-URL, and
generic RequestException paths, preserving their existing sanitized messages and
ensuring formatted tracebacks do not expose credential-bearing request details.

🪄 Autofix (Beta)

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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: de4ac47c-51de-4a69-a0ee-a902e34b08ea

📥 Commits

Reviewing files that changed from the base of the PR and between c267148 and 4bff770.

📒 Files selected for processing (5)

• CHANGELOG.md
• chartarr/cli.py
• chartarr/lidarr.py
• tests/test_cli.py
• tests/test_lidarr.py

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
python - <<'PY'
from urllib.parse import urlsplit

assert urlsplit("http://admin:hunter2@").hostname is None
try:
    urlsplit("http://admin:hunter2@[::1")
except ValueError:
    pass
else:
    raise AssertionError("expected malformed IPv6 authority to fail parsing")
PY

Repository: alperien/chartarr

Length of output: 155

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Files:"
git ls-files | rg '(^|/)lidarr\.py$|errors|exceptions|shown' || true

echo
echo "chartarr/lidarr.py outline:"
ast-grep outline chartarr/lidarr.py --view compact || true

echo
echo "chartarr/lidarr.py relevant sections:"
cat -n chartarr/lidarr.py | sed -n '1,180p'

echo
echo "Search shown/raise_from/raise LidarrError:"
rg -n "shown\s*=|raise .*from|LidarrError|shown" chartarr/lidarr.py

Repository: alperien/chartarr

Length of output: 9600

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "chartarr/lidarr.py tail:"
cat -n chartarr/lidarr.py | sed -n '180,250p'

echo
echo "Python urlsplit edge-case verifier:"
python3 - <<'PY'
from urllib.parse import urlsplit, urlunsplit

cases = [
    "http://admin:hunter2@",
    "http://admin:hunter2",
    "http://admin:hunter2@[::1",
    "http://admin:hunter2@[::1]:80",
    "http://admin:hunter2@host/path",
    "http://host/path",
    "./bad",
]
for url in cases:
    try:
        p = urlsplit(url)
        print(url, "hostname=", p.hostname, "netloc=", p.netloc, "text=", repr(p.geturl()))
    except Exception as e:
        print(url, "exception", type(e).__name__, "=", str(e))
PY

Repository: alperien/chartarr

Length of output: 3489

---

Redact malformed authorities before returning them.

urlsplit("http://admin:hunter2@") has hostname is None but still emits admin:hunter2@, so _safe_url() stores the credential-bearing string in self.shown. A malformed IPv6 authority with userinfo also raises before any scrubbing. Strip userinfo from parts.netloc for parsed URLs and from parse-error fallbacks before storing shown, otherwise connection/invalid-URL tracebacks can still expose credentials.

🤖 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 `@chartarr/lidarr.py` around lines 39 - 48, Update _safe_url to redact userinfo
from both successfully parsed URLs and ValueError fallback strings before
returning them. Use parts.netloc when rebuilding parsed URLs, removing any text
through the final “@” while preserving the host, port, path, query, and
fragment; ensure malformed authorities that raise during urlsplit are sanitized
before the fallback is returned and stored in self.shown.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
python - <<'PY'
import traceback

secret = "http://admin:hunter2@lidarr.test:8686"
try:
    raise RuntimeError(secret)
except RuntimeError as cause:
    try:
        raise ValueError("sanitized") from cause
    except ValueError as error:
        assert secret in "".join(traceback.format_exception(error))
PY

Repository: alperien/chartarr

Length of output: 155

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'Files matching lidarr.py:\n'
fd -a 'lidarr\.py$' . || true

printf '\nRelevant snippets and imports:\n'
while IFS= read -r f; do
  printf '\n=== %s ===\n' "$f"
  wc -l "$f"
  ast-grep outline "$f" --view compact || true
  sed -n '1,150p' "$f" | cat -n
done < <(fd 'lidarr\.py$' .)

printf '\nSearch for LidarrError definition and tests involving LidarrError:\n'
rg -n "class LidarrError|LidarrError|requests\.ConnectionError|Lidarr request|format_exception|traceback" .

Repository: alperien/chartarr

Length of output: 344

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'Relevant snippets and imports:\n'
sed -n '1,150p' chartarr/lidarr.py | cat -n

printf '\nSearch for LidarrError definition and tests involving LidarrError:\n'
rg -n "class LidarrError|LidarrError|requests\.ConnectionError|Lidarr request|format_exception|traceback" .

Repository: alperien/chartarr

Length of output: 9752

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
import traceback
import urllib.parse

base = "http://admin:hunter2@lidarr.test:8686"
safe = "http://lidarr.test:8686"

try:
    raise RuntimeError(f"connection refused to {base}")
except RuntimeError as cause:
    try:
        raise RuntimeError(f"Lidarr request to {safe} failed: connection refused to {base}") from cause
    except RuntimeError as error:
        text = "".join(traceback.format_exception(error))
        print("contains_base:", base in text)
        print("contains_safe:", safe in text)
        print("contains_credentials:", "`@lidarr.test`" in text)
        for line in text.splitlines():
            if base in line or safe in line:
                print("relevant_trace_line:", line)
PY

printf '\nTests around credential-bearing errors:\n'
sed -n '230,360p' tests/test_lidarr.py | cat -n

printf '\ncli prints traceback usage:\n'
rg -n "traceback|LidarrError|print\\(" chartarr/cli.py tests/test_lidarr.py

Repository: alperien/chartarr

Length of output: 803

---

Suppress the credential-bearing exception chain.

The sanitized Lidarr_error message hides credentials in LidarrError.args, but raising from e keeps the original requests exception in formatted tracebacks, including its credential-bearing URL. Raise from None for these paths and assert traceback.format_exception(type(e), e, e.__traceback__) preserves the sanitized message only.

🤖 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 `@chartarr/lidarr.py` around lines 99 - 114, Update the exception handlers in
the Lidarr request flow to raise each sanitized LidarrError with suppressed
exception chaining instead of `from e`. Apply this to the ConnectionError,
Timeout, invalid-URL, and generic RequestException paths, preserving their
existing sanitized messages and ensuring formatted tracebacks do not expose
credential-bearing request details.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not retain the raw proxy response body on LidarrError.

The displayed HTTP message is scrubbed, but body=r.text retains credentials. add_album() later inserts e.body into a new user-visible duplicate-album error, reintroducing a proxy-echoed password. Store the scrubbed body, or keep any raw body private and never interpolate it into errors.

🤖 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 `@chartarr/lidarr.py` around lines 120 - 124, Update the LidarrError
construction in the HTTP error branch to avoid retaining raw r.text in its
public body field. Store the scrubbed response body instead, ensuring
add_album() cannot reintroduce credentials when interpolating e.body into
duplicate-album errors.