diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cead8b..551e778 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,16 @@ All notable changes to this project will be documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). +## [Unreleased] + +### Fixed +- **4xx errors now report why.** `_make_request` ended non-retryable statuses with `raise_for_status()`, whose curl_cffi message is `HTTP Error {code}: {reason}` — and HTTP/2 carries no reason phrase, so a refused request logged as bare `HTTP Error 403:` and the response body (the only explanation the provider gives) was discarded. The body's `detail`/`error`/`message` is now carried into the `ProviderError`, redacted and truncated. This is what made the media 403s on `GET /backend-api/files/{id}/download` undiagnosable. +- **`redact_secrets` missed compound key names.** It matched keys exactly, so `access_token`, `api_key`, and `session-token` passed through un-redacted into debug-logged response bodies; matching now applies per word ("keywords", "monkey", "tokenizer" stay intact). +- **`tests/test_config.py::TestSessionLimiterConfig::test_defaults` depended on the developer's `.env`.** `load_config()` calls `load_dotenv(override=False)`, which re-populated the variable the test had just deleted — so it passed only on a machine with no `.env`. The test now stubs dotenv discovery. + +### Changed +- Media download failures are bucketed as `forbidden` (403 — the asset exists, we were refused) separately from `download-error`, so the run summary distinguishes it from `expired-or-missing` (404). + ## [0.8.0] - 2026-07-06 Focused on the Claude Code provider after the tool's on-disk layout changed and diff --git a/src/media.py b/src/media.py index bb41f88..80ba689 100644 --- a/src/media.py +++ b/src/media.py @@ -131,10 +131,7 @@ def resolve_media( logger.warning( "[media] Could not download %s: %s", ref[:60], e.original ) - reason = "expired-or-missing" if "404" in str(e.original) or "not found" in str( - e.original - ).lower() else "download-error" - report.record_media_failed(reason) + report.record_media_failed(_classify_failure(e)) continue ext = _pick_extension(mime, file_name) @@ -153,6 +150,21 @@ def resolve_media( return downloaded +def _classify_failure(error: ProviderError) -> str: + """Bucket a download failure for the run summary. + + The buckets separate "the asset is gone" from "the asset is there but we + were refused" — different causes, different fixes, so lumping both into + download-error hides which one you have. + """ + detail = str(error.original).lower() + if "404" in detail or "not found" in detail: + return "expired-or-missing" + if "403" in detail or "forbidden" in detail: + return "forbidden" + return "download-error" + + def _safe_asset_name(provider, ref: str) -> str | None: """A stable, filesystem-safe name for the asset (the provider file ID).""" parser = getattr(provider, "parse_asset_file_id", None) diff --git a/src/providers/base.py b/src/providers/base.py index 7f46196..7e66b34 100644 --- a/src/providers/base.py +++ b/src/providers/base.py @@ -33,6 +33,9 @@ logger = logging.getLogger(__name__) # Request timeouts (connect, read) in seconds REQUEST_TIMEOUT = (10, 30) +# Longest error-body excerpt to carry into a ProviderError message. +_ERROR_BODY_CHARS = 300 + # Retry configuration MAX_RETRIES = 3 BACKOFF_BASE = 2.0 @@ -115,6 +118,32 @@ def resolve_request_delay() -> float: return DEFAULT_REQUEST_DELAY return value +def _describe_error_body(response: Any) -> str: + """Summarise an error response body for a log line. + + Providers explain 4xx in the body (ChatGPT uses ``detail``), so a bare + status code is not a diagnosis. Prefers ``detail``/``error``/``message``, + falls back to a truncated raw excerpt, and redacts before it is logged. + """ + try: + body = response.json() + except Exception: + try: + text = (response.text or "").strip() + except Exception: + return "no response body" + if not text: + return "empty response body" + return f"body: {text[:_ERROR_BODY_CHARS]}" + + if isinstance(body, dict): + for key in ("detail", "error", "message"): + if key in body: + value = redact_secrets(body[key]) + return f"{key}: {str(value)[:_ERROR_BODY_CHARS]}" + return f"body: {str(redact_secrets(body))[:_ERROR_BODY_CHARS]}" + + # Realistic Chrome User-Agent USER_AGENT = ( "Mozilla/5.0 (X11; Linux x86_64) " @@ -384,7 +413,19 @@ class BaseProvider(ABC): continue # ── Other HTTP errors ────────────────────────────────────── - response.raise_for_status() + # Raise with the response body attached. curl_cffi formats + # raise_for_status() as "HTTP Error {code}: {reason}", and + # HTTP/2 carries no reason phrase — so the bare exception + # reads "HTTP Error 403:" and says nothing about the cause. + # The provider's JSON `detail` is the only explanation there is. + if not response.ok: + raise ProviderError( + self.provider_name, + f"{method} {url}", + RuntimeError( + f"HTTP {response.status_code} — {_describe_error_body(response)}" + ), + ) # ── Success ──────────────────────────────────────────────── body = response.json() diff --git a/src/utils.py b/src/utils.py index c5e9238..04e4b55 100644 --- a/src/utils.py +++ b/src/utils.py @@ -76,11 +76,27 @@ def build_export_path( return base_dir.joinpath(*parts) / filename +def _is_sensitive_key(key: object) -> bool: + """True if a mapping key names a secret. + + Matches the whole key and each of its underscore/dash-separated words, so + compound names carry too: exact-match alone let ``access_token`` and + ``api_key`` through into logged response bodies. Word-level matching keeps + innocent keys ("keywords", "monkey") intact. + """ + if not isinstance(key, str): + return False + lowered = key.lower() + if lowered in _SENSITIVE_KEYS: + return True + return any(part in _SENSITIVE_KEYS for part in re.split(r"[^a-z0-9]+", lowered)) + + def redact_secrets(data: object) -> object: """Recursively redact sensitive values from a dict/list for safe logging. - Keys matching _SENSITIVE_KEYS (case-insensitive) have their values - replaced with "[REDACTED]". + Keys naming a secret (see _is_sensitive_key) have their values replaced + with "[REDACTED]". Args: data: Any JSON-serializable object. @@ -90,7 +106,7 @@ def redact_secrets(data: object) -> object: """ if isinstance(data, dict): return { - k: "[REDACTED]" if k.lower() in _SENSITIVE_KEYS else redact_secrets(v) + k: "[REDACTED]" if _is_sensitive_key(k) else redact_secrets(v) for k, v in data.items() } if isinstance(data, list): diff --git a/tests/test_config.py b/tests/test_config.py index 15ac29c..fe78220 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -60,7 +60,13 @@ class TestSessionLimiterConfig: """MAX_CONVERSATIONS_PER_RUN and REQUEST_DELAY parsing in load_config.""" def _load(self, monkeypatch, tmp_path, **env): + from src import config as config_module from src.config import load_config + # load_config() calls load_dotenv(override=False), which re-populates + # any variable this test just deleted from the developer's real .env — + # so test_defaults only saw defaults on a machine without one. Stub it: + # these tests are about parsing the environment, not discovering .env. + monkeypatch.setattr(config_module, "load_dotenv", lambda *a, **k: False) monkeypatch.setenv("EXPORT_DIR", str(tmp_path / "exports")) monkeypatch.setenv("CACHE_DIR", str(tmp_path / "cache")) for key in ( diff --git a/tests/test_media.py b/tests/test_media.py index 939fbb5..b535848 100644 --- a/tests/test_media.py +++ b/tests/test_media.py @@ -170,6 +170,22 @@ class TestResolveMedia: # Still renders as a placeholder, not a broken image link assert render_blocks_to_markdown([block]).startswith("> 🖼️") + def test_forbidden_counted_separately_from_generic_error(self, tmp_path): + """403 is a distinct bucket: the asset exists, we were refused.""" + ref = "sediment://file_denied" + provider = _FakeProvider( + fail_refs={ref: RuntimeError("HTTP 403 — detail: unauthorized")} + ) + block = make_image_placeholder(ref=ref, source="model_generated") + report = LossReport() + + resolve_media( + _conv_with([block]), provider, tmp_path, "provider/project/year", + "images", report, + ) + assert report.media_failed["forbidden"] == 1 + assert "download-error" not in report.media_failed + def test_provider_without_download_asset(self, tmp_path): """claude-code has no remote assets — resolve_media must no-op.""" class NoDownload: diff --git a/tests/test_providers.py b/tests/test_providers.py index 6028103..b61a635 100644 --- a/tests/test_providers.py +++ b/tests/test_providers.py @@ -1116,3 +1116,69 @@ class TestClaudeDriftCanary: p = self._provider([{"uuid": "u1", "name": "N", "updated_at": "z"}], self._detail([])) assert DRIFT_ERROR in _sev(p.check_drift()) + + +# --------------------------------------------------------------------------- +# 4xx diagnostics: curl_cffi renders raise_for_status() as +# "HTTP Error {code}: {reason}", and HTTP/2 has no reason phrase — so a bare +# 403 logged as "HTTP Error 403:" says nothing. The body carries the cause. +# --------------------------------------------------------------------------- + + +class TestErrorBodyDiagnostics: + class _Resp: + ok = False + status_code = 403 + reason = "" + headers: dict = {} + + def __init__(self, payload=None, text=""): + self._payload = payload + self.text = text + + def json(self): + if self._payload is None: + raise ValueError("not json") + return self._payload + + def _provider(self, response): + from src.providers.chatgpt import ChatGPTProvider + p = ChatGPTProvider.__new__(ChatGPTProvider) + p._request_delay = 0 + p._last_request_at = None + p._session = type("S", (), {"request": lambda *a, **k: response})() + return p + + def test_detail_field_surfaces_in_error(self): + from src.providers.base import ProviderError + + resp = self._Resp(payload={"detail": "File not accessible to this account"}) + with pytest.raises(ProviderError) as exc: + self._provider(resp)._make_request("GET", "https://x/files/f1/download") + message = str(exc.value.original) + assert "403" in message + assert "File not accessible to this account" in message + + def test_non_json_body_excerpted(self): + from src.providers.base import ProviderError + + resp = self._Resp(text="Forbidden") + with pytest.raises(ProviderError) as exc: + self._provider(resp)._make_request("GET", "https://x/files/f1/download") + assert "Forbidden" in str(exc.value.original) + + def test_empty_body_says_so_rather_than_nothing(self): + from src.providers.base import ProviderError + + resp = self._Resp(text="") + with pytest.raises(ProviderError) as exc: + self._provider(resp)._make_request("GET", "https://x/files/f1/download") + assert "empty response body" in str(exc.value.original) + + def test_secrets_in_error_body_are_redacted(self): + from src.providers.base import _describe_error_body + + resp = self._Resp(payload={"error": {"message": "no", "access_token": "sk-abc"}}) + described = _describe_error_body(resp) + assert "sk-abc" not in described + assert "[REDACTED]" in described diff --git a/tests/test_utils.py b/tests/test_utils.py index a91713a..41b6a1f 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -145,3 +145,21 @@ class TestFormatTokenStatus: expiry = datetime.now(tz=timezone.utc) + timedelta(days=10, hours=12) result = format_token_status("tok", expiry) assert "10 days" in result + + +class TestRedactCompoundKeys: + """Exact-match redaction let compound secret names through into logs.""" + + def test_compound_secret_keys_redacted(self): + result = redact_secrets( + {"access_token": "sk-abc", "api_key": "k1", "session-token": "s1"} + ) + assert result == { + "access_token": "[REDACTED]", + "api_key": "[REDACTED]", + "session-token": "[REDACTED]", + } + + def test_innocent_keys_containing_a_secret_word_kept(self): + result = redact_secrets({"keywords": ["a"], "monkey": "b", "tokenizer": "c"}) + assert result == {"keywords": ["a"], "monkey": "b", "tokenizer": "c"}