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

CHANGELOG.md

@@ -11,6 +11,20 @@ alone.
 
 ### Fixed
 
+- **A skip in the review screen is now honoured on a row that also
+  matched.** The push took the automatic match whenever there was one and
+  only consulted your decision otherwise, so a row that was skipped and
+  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 matched row carrying no release group id — possible
+  in a state file written by an older version — is left out instead of
+  failing at Lidarr's lookup.
+- **A Lidarr URL with credentials in it no longer appears in error
+  messages.** `http://user:pw@host` is how you get through a reverse proxy
+  that asks for basic auth, and the whole URL was quoted back in every
+  connection error, timeout and proxy error page. The password is stripped
+  from what's printed; the request still sends it.
+
 - **The review screen pinned a CPU core while it waited for you.** The
   progress screens poll for a keypress so they can notice `q` mid-run, and
   that non-blocking mode stayed on the window afterwards — `curses.wrapper`

chartarr/cli.py

@@ -352,20 +352,34 @@ def stage_review(rows, artist_col, title_col, state: State) -> None:
 
 
 def import_set(rows, state: State) -> list[dict]:
+    """the albums to push: automatic matches plus accepted review picks.
+
+    a decision outranks the match result. review only offers uncertain
+    rows, but a row can be decided and then match cleanly on a --rematch,
+    and the state file is editable — either way the answer the user gave
+    is the one they meant, so an explicit skip is honoured on a matched
+    row and a re-pick replaces the automatic choice.
+    """
     by_key = {r["_key"]: r for r in rows}
     out = []
     for key, res in state.results.items():
         row = by_key.get(key)
         if row is None:
             continue
-        if res["status"] == "matched":
-            out.append({"key": key, "row": row, "rgid": res["release_group_mbid"],
-                        "artist_mbid": res.get("artist_mbid")})
+        d = state.decisions.get(key) or {}
+        if d.get("action") == "skip":
+            continue
+        if d.get("action") == "accept":
+            rgid, artist_mbid = d.get("mbid"), d.get("artist_mbid")
+        elif res["status"] == "matched":
+            rgid, artist_mbid = res.get("release_group_mbid"), res.get("artist_mbid")
         else:
-            d = state.decisions.get(key)
-            if d and d.get("action") == "accept":
-                out.append({"key": key, "row": row, "rgid": d["mbid"],
-                            "artist_mbid": d.get("artist_mbid")})
+            continue
+        # an older state file can carry a matched row with no id; pushing
+        # it would fail at lidarr's lookup, so leave it out
+        if rgid:
+            out.append({"key": key, "row": row, "rgid": rgid,
+                        "artist_mbid": artist_mbid})
     return out
 
 

chartarr/lidarr.py

@@ -13,6 +13,7 @@
 
 import json
 import time
+import urllib.parse
 
 import requests
 
@@ -26,6 +27,27 @@ def __init__(self, message: str, status: int = 0, body: str = ""):
         self.body = body
 
 
+def _safe_url(url: str) -> str:
+    """the url with any user:password@ removed, for showing in messages.
+
+    a lidarr url can carry basic-auth credentials — behind a reverse proxy
+    that asks for them, http://user:pw@host is how you get through. those
+    end up in every connection error otherwise, and errors get pasted into
+    bug reports.
+    """
+    try:
+        parts = urllib.parse.urlsplit(url)
+        host = parts.hostname
+    except ValueError:
+        return url
+    if not host:
+        return url  # nothing parsed as a host; nothing to hide
+    if parts.port:
+        host = f"{host}:{parts.port}"
+    return urllib.parse.urlunsplit(
+        (parts.scheme, host, parts.path, parts.query, parts.fragment))
+
+
 def _is_duplicate(status: int, body: str) -> bool:
     """true if lidarr is saying "this album is already here".
 
@@ -59,42 +81,54 @@ def _is_duplicate(status: int, body: str) -> bool:
 class Lidarr:
     def __init__(self, url: str, api_key: str):
         self.base = url.rstrip("/")
+        # what the user sees in errors: the same address minus any
+        # credentials, so a pasted traceback doesn't carry a password
+        self.shown = _safe_url(self.base)
         self.s = requests.Session()
         self.s.headers["X-Api-Key"] = api_key
 
+    def _scrub(self, text) -> str:
+        """text with this instance's url swapped for the credential-free one."""
+        out = str(text)
+        return out.replace(self.base, self.shown) if self.base != self.shown else out
+
     def _call(self, path: str, method: str = "GET", **kw):
         url = f"{self.base}/api/v1/{path}"
         try:
             r = self.s.request(method, url, timeout=60, **kw)
         except requests.ConnectionError as e:
             raise LidarrError(
-                f"Can't reach Lidarr at {self.base} — is it running, and is the "
+                f"Can't reach Lidarr at {self.shown} — is it running, and is the "
                 f"URL right? (the address you use in your browser)") from e
         except requests.Timeout as e:
-            raise LidarrError(f"Lidarr at {self.base} timed out.") from e
+            raise LidarrError(f"Lidarr at {self.shown} timed out.") from e
         except (requests.exceptions.InvalidSchema,
                 requests.exceptions.MissingSchema,
                 requests.exceptions.InvalidURL) as e:
             raise LidarrError(
-                f"{self.base} is not a URL Lidarr can be reached at — it needs "
+                f"{self.shown} is not a URL Lidarr can be reached at — it needs "
                 f"to start with http:// or https://") from e
         except requests.RequestException as e:
-            raise LidarrError(f"Lidarr request failed: {e}") from e
+            # requests quotes the url it was given, credentials and all
+            raise LidarrError(
+                f"Lidarr request to {self.shown} failed: {self._scrub(e)}") from e
 
         if r.status_code == 401:
             raise LidarrError(
                 "Lidarr rejected the API key (401). Copy it from "
                 "Settings → General → Security → API Key.", status=401)
         if r.status_code >= 400:
-            raise LidarrError(f"HTTP {r.status_code}: {r.text[:300] or 'no details'}",
-                              status=r.status_code, body=r.text)
+            # a proxy's error page can quote the request url back at us
+            raise LidarrError(
+                f"HTTP {r.status_code}: {self._scrub(r.text)[:300] or 'no details'}",
+                status=r.status_code, body=r.text)
         if not r.text:
             return None
         try:
             return r.json()
         except ValueError as e:
             raise LidarrError(
-                f"Lidarr returned something that isn't JSON — is {self.base} "
+                f"Lidarr returned something that isn't JSON — is {self.shown} "
                 f"really Lidarr, and not a login page or another service?") from e
 
     def status(self) -> dict:

tests/test_cli.py

@@ -437,6 +437,65 @@ def test_without_the_search_flag_nothing_is_searched(
     assert api.searched == []
 
 
+def test_a_skip_is_honoured_even_on_a_row_that_matched(tmp_path):
+    # a row can match cleanly on a --rematch after being decided, and the
+    # state file is editable; the answer the user gave is the one they meant
+    state = State(tmp_path / "s.jsonl")
+    state.add_result("k", {"status": "matched", "release_group_mbid": "rg-auto",
+                           "artist_mbid": "am"})
+    state.add_decision("k", {"action": "skip"})
+    rows = [{"_key": "k", "artist": "a", "title": "t"}]
+    assert cli.import_set(rows, state) == []
+
+
+def test_a_repick_replaces_the_automatic_choice(tmp_path):
+    state = State(tmp_path / "s.jsonl")
+    state.add_result("k", {"status": "matched", "release_group_mbid": "rg-auto",
+                           "artist_mbid": "am-auto"})
+    state.add_decision("k", {"action": "accept", "mbid": "rg-user",
+                             "artist_mbid": "am-user"})
+    rows = [{"_key": "k", "artist": "a", "title": "t"}]
+    items = cli.import_set(rows, state)
+    assert [(i["rgid"], i["artist_mbid"]) for i in items] == [("rg-user", "am-user")]
+
+
+def test_an_undone_decision_falls_back_to_the_match(tmp_path):
+    # u in the review screen writes a clear record; the row should go back
+    # to whatever the matcher said rather than dropping out of the push
+    state = State(tmp_path / "s.jsonl")
+    state.add_result("k", {"status": "matched", "release_group_mbid": "rg-auto",
+                           "artist_mbid": "am"})
+    state.add_decision("k", {"action": "skip"})
+    state.add_decision("k", {"action": "clear"})
+    rows = [{"_key": "k", "artist": "a", "title": "t"}]
+    assert [i["rgid"] for i in cli.import_set(rows, state)] == ["rg-auto"]
+
+
+def test_a_matched_row_with_no_release_group_is_left_out(tmp_path):
+    # an older state file can hold one; pushing it dies at lidarr's lookup
+    state = State(tmp_path / "s.jsonl")
+    state.add_result("k", {"status": "matched"})
+    rows = [{"_key": "k", "artist": "a", "title": "t"}]
+    assert cli.import_set(rows, state) == []
+
+
+def test_an_accepted_review_row_still_pushes(tmp_path):
+    # the ordinary path, unchanged: uncertain row, user picks a candidate
+    state = State(tmp_path / "s.jsonl")
+    state.add_result("k", {"status": "review", "release_group_mbid": "rg-guess"})
+    state.add_decision("k", {"action": "accept", "mbid": "rg-picked",
+                             "artist_mbid": "am"})
+    rows = [{"_key": "k", "artist": "a", "title": "t"}]
+    assert [i["rgid"] for i in cli.import_set(rows, state)] == ["rg-picked"]
+
+
+def test_an_undecided_review_row_is_not_pushed(tmp_path):
+    state = State(tmp_path / "s.jsonl")
+    state.add_result("k", {"status": "review", "release_group_mbid": "rg-guess"})
+    rows = [{"_key": "k", "artist": "a", "title": "t"}]
+    assert cli.import_set(rows, state) == []
+
+
 def test_rematch_clears_only_the_rows_nothing_was_found_for(tmp_path):
     state = State(tmp_path / "s.jsonl")
     state.add_result("miss", {"status": "not_found"})

tests/test_lidarr.py

@@ -285,3 +285,79 @@ def test_html_login_page_is_not_mistaken_for_lidarr():
                   body="<html><body>Sign in to continue</body></html>")
     with pytest.raises(LidarrError, match="really Lidarr"):
         api().status()
+
+
+# --- credentials in the url stay out of the error text ---
+
+CREDS = "http://admin:hunter2@lidarr.test:8686"
+CREDS_API = CREDS + "/api/v1"
+
+
+def creds_api():
+    return Lidarr(CREDS, "sekrit")
+
+
+def test_safe_url_drops_only_the_credentials():
+    assert lidarr._safe_url(CREDS) == BASE
+    assert lidarr._safe_url("http://:tok3n@lidarr.test:8686") == BASE
+    # everything else about the url survives untouched
+    assert lidarr._safe_url("https://u:p@host/base?x=1#f") == "https://host/base?x=1#f"
+    assert lidarr._safe_url("http://lidarr.test:8686") == "http://lidarr.test:8686"
+    # a default port isn't invented, and unparseable input is left alone
+    assert lidarr._safe_url("http://lidarr.test") == "http://lidarr.test"
+    assert lidarr._safe_url("not a url") == "not a url"
+
+
+@responses.activate
+def test_connection_error_does_not_echo_the_password():
+    responses.add(responses.GET, CREDS_API + "/system/status",
+                  body=requests.ConnectionError("refused"))
+    with pytest.raises(LidarrError) as e:
+        creds_api().status()
+    assert "hunter2" not in str(e.value)
+    assert "lidarr.test:8686" in str(e.value)  # still says where it tried
+
+
+@responses.activate
+def test_timeout_does_not_echo_the_password():
+    responses.add(responses.GET, CREDS_API + "/system/status",
+                  body=requests.Timeout("60s"))
+    with pytest.raises(LidarrError) as e:
+        creds_api().status()
+    assert "hunter2" not in str(e.value)
+
+
+@responses.activate
+def test_a_generic_request_failure_does_not_echo_the_password():
+    # requests puts the url it was handed into its own message
+    responses.add(responses.GET, CREDS_API + "/system/status",
+                  body=requests.TooManyRedirects(f"too many redirects for {CREDS}"))
+    with pytest.raises(LidarrError) as e:
+        creds_api().status()
+    assert "hunter2" not in str(e.value)
+
+
+@responses.activate
+def test_a_proxy_error_page_quoting_the_url_is_scrubbed():
+    responses.add(responses.GET, CREDS_API + "/system/status", status=502,
+                  body=f"Bad gateway while proxying to {CREDS}/api/v1/system/status")
+    with pytest.raises(LidarrError) as e:
+        creds_api().status()
+    assert "hunter2" not in str(e.value)
+
+
+@responses.activate
+def test_non_json_reply_does_not_echo_the_password():
+    responses.add(responses.GET, CREDS_API + "/system/status", status=200,
+                  body="<html>login</html>")
+    with pytest.raises(LidarrError) as e:
+        creds_api().status()
+    assert "hunter2" not in str(e.value)
+
+
+@responses.activate
+def test_the_credentials_are_still_sent_to_lidarr():
+    # scrubbing is for the message only; the request must keep working
+    responses.add(responses.GET, CREDS_API + "/system/status", json={"version": "3"})
+    assert creds_api().status() == {"version": "3"}
+    assert "admin:hunter2" in responses.calls[0].request.url