fix(logging): add payload-safe helpers for exceptions, URLs, ids and repeat warnings

[chopratejas]

Description

Log records are kept, rotated, attached to bug reports and shipped to log collectors, so no level, DEBUG included, may carry request content, tool output or credentials. Review of the cleanup PRs (#4136) found that moving exception detail to logger.debug(..., exc_info=True) still leaks it: a compressor, hook or storage backend can quote the payload or a connection URL in its exception message, and the rendered traceback prints that message.

This PR adds one small shared module, headroom/log_safety.py, so every log line describes a failure the same safe way. The open cleanup PRs that log failures will use it instead of each inventing their own redaction.

Refs #4136

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Performance improvement
• Code refactoring (no functional changes)

Changes Made

• New headroom/log_safety.py:
  • describe_exception(exc): each exception in the cause chain as its type plus the innermost Headroom code locations (at most 3), never the message. Only Headroom's own frames are named: the frame's globals must be the namespace of the module registered under that name, the name must be a plain headroom.* identifier, the code's file must be the module's own file, and the function name a plain identifier. Library, stdlib and runtime-compiled frames are skipped, since any text they carry can be chosen at runtime. Locations come from the module name, so .pyc-only and zip installs keep theirs. A class name not shaped like source identifiers becomes <exception>; an OSError keeps a plain-int errno and the system's text for it. With HEADROOM_DEBUG_DUMP=full it returns the full traceback.
  • content_logging_enabled(): one reading of that opt-in.
  • redact_url(url): keeps only scheme, host and port. Userinfo, query, fragment and the path (shown as /<path>) are dropped; the path is kept only with HEADROOM_DEBUG_DUMP=full. The host is kept only when it is an IP address (an IPv6 zone id is dropped) or a strict DNS name (letters, digits and -; non-ASCII names in IDNA form). Any other host, including one with _ (key-shaped, e.g. sk_live_…), is shown as <host>, with scheme and port kept. A URL with no host is accepted only for file:///, sqlite:/// and unix:///. Anything else, for example a pasted sk-key:secret, gives <unparseable url>. A scheme outside a short known list is shown as <scheme>.
  • safe_id(value): the built-in repr of a str, bytes or number, capped at 80 characters, so an id can neither forge a log line nor flood the log. No __repr__ of the caller's object runs: any other type is shown as <TypeName>, and an int too long to print under the process's digit limit as <int of N bits>.
  • WarnOnce(limit, what, window_seconds=3600): inside each window (an hour by default), each key warns once, for at most limit keys. Past that it logs one overflow notice and stays quiet until the window ends, so a failure across many keys gives at most limit + 1 warnings an hour. When the window ends it starts over, so a failure that keeps recurring, or a newly broken hook, warns again once an hour. A key is used up only while WARNING is enabled. forget(key) re-arms a key once its failure clears, but cannot reopen the cap after the overflow notice. The window must be positive.
• headroom/proxy/handlers/_debug_dump.py:
  • uses redact_url instead of its own query-only copy, so a redacted dump's URL loses userinfo and the path as well as the query (a full dump keeps the path, never the query);
  • uses content_logging_enabled(), so the opt-in values are defined in one place.
• New tests/test_log_safety.py:
  • content and credential canaries never appear in the description;
  • OSError errno handling;
  • the content opt-in;
  • cause chains, cycles and from None;
  • package-relative frame paths, as on a CI checkout;
  • URL redaction, including IPv6 and an unparseable URL;
  • id bounding;
  • WarnOnce: bounded across three passes over 65 keys; not used up while WARNING is disabled; re-armed by forget; not reopened by forget inside a window; starting over after the window.
  • A huge errno value.

Testing

• Unit tests pass (pytest)
• Linting passes (ruff check .)
• Type checking passes (mypy headroom)
• New tests added for new functionality
• Manual testing performed

Test Output

$ pytest tests/test_log_safety.py tests/test_debug_dump_gating.py tests/test_prefix_tracker_client_rewrote_reply.py
46 passed
$ pytest tests/test_log_safety.py   # from a checkout at .../headroom/headroom, as on GitHub Actions
20 passed
$ ruff check / ruff format (0.16.8): All checks passed;  mypy (2 modules): Success

Real Behavior Proof

• Environment: macOS, Python 3.12, repo .venv, this branch's worktree. Temp HOME and HEADROOM_WORKSPACE_DIR.
• Exact command / steps:
  1. The shared smoke harness, bash smoke.sh <this worktree>, does these steps in order:
     • starts python -m headroom.cli proxy --port <random> from this checkout, with HOME and HEADROOM_WORKSPACE_DIR in a temp dir, HEADROOM_OFFLINE=1, and ANTHROPIC_TARGET_API_URL/OPENAI_TARGET_API_URL pointing at a local fake upstream;
     • waits for /health;
     • POSTs one /v1/messages and one /v1/chat/completions request, each with a 400-row JSON tool result;
     • prints the status codes, the /stats savings, and the proxy.log WARNING/ERROR lines.
  2. A direct call of the helpers, on an exception whose message carries a fake key.
• Observed result: both requests return 200 with the usual savings. The description names the type and the location, and leaves out the key. The opt-in restores the message.
• Not tested: Windows paths in a real traceback. Backslashes are normalized, which the path test covers.

branch: cleanup/log-safety-helpers @ 88f934b5
health: 200
POST /v1/messages        -> HTTP200 smoke-ok
POST /v1/chat/completions -> HTTP200 smoke-ok
stats: requests=2 failed=0 tokens_saved=13574 savings_percent=99.82

describe_exception -> RuntimeError at <string>:5 in store <- <string>:6 in <module>
redact_url -> https://proxy.local:8787/stats?<redacted>
safe_id -> 'm1\nWARNING forged'
with HEADROOM_DEBUG_DUMP=full -> RuntimeError: sk-live-123

Runtime Rollout Safety

• Rollout-managed feature(s): None.
• Minimum rollout channel: Not applicable.
• Stable/default behavior changed: Only the opt-in request debug dump (HEADROOM_DEBUG_DUMP=1), whose URL now also drops userinfo and the path. The new module has no callers yet.
• Kill switch / disable path: Not applicable.
• Unsafe override required: No.
• Qualification impact: None.
• Rollback path: Revert this single commit.

Review Readiness

• I have performed a self-review
• This PR is ready for human review

Checklist

• My code follows the project's style guidelines
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I did not edit CHANGELOG.md — it is generated by release-please from my Conventional Commit PR title (a CI guard enforces this)

Screenshots (if applicable)

Not applicable.

Additional Notes

Part of maintainer-requested cleanup #4136. Please merge this one early: the logging PRs flagged in review (#4092, #4094, #4105, #4111, #4124, #4127) pick it up so they can stop logging exception text at DEBUG. Until it merges, those PRs merge this branch in, so its files show in their diffs.

This PR is a little over the 300-line guideline; about half of that is tests.

Not done here: converting the existing exc_info=True lines already on main to describe_exception. That is a separate sweep PR after the cleanup PRs land.

🤖 Generated with Claude Code

[github-actions]

PR governance

This PR does not yet satisfy the required template fields:

• Fill in Real Behavior Proof → Exact command / steps.

Please update the PR body, or move the PR back to draft while it is still in progress.

[JerrettDavis]

The shared helper direction is useful, and all 14 helper tests pass. Two safety blockers remain with content logging disabled; I independently reproduced both and considered the existing inline findings.

1. _describe_one copies OSError.strerror into its result. That field is caller-supplied text: describe_exception(OSError(5, 'backend rejected PRIVATE_CREDENTIAL_CANARY\nFORGED_LOG_ENTRY')) includes both the credential canary and the actual newline when the returned description is formatted into a DEBUG record. Truncating to 80 characters does not sanitize it. Please retain the exception class and a validated numeric errno; omit arbitrary strerror, or derive a trusted canonical description from a validated errno rather than the exception's string. Add a regression formatting a real LogRecord with content opt-in unset and assert both the canary and injected newline are absent, including chained errors.

2. redact_url deliberately retains paths, but the newly changed upstream error-dump call site passes the complete upstream URL without establishing that its path is safe. https://upstream.example/botPRIVATE_CREDENTIAL_CANARY/v1/messages retains the credential after redaction. Please make the dump safe for credential-bearing paths by omitting them or logging only an explicitly known safe route; test arbitrary and percent-encoded path secrets in default/error mode. A helper docstring warning does not protect this production call site.

These are new maintainer findings on exact commit 88f934b; no prior maintainer review was present. Current main is an ancestor, diff --check is clean, and GitHub reports MERGEABLE. Qualification stopped at the concrete blockers; no green-CI or merge-readiness claim. No contributor code was changed.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 95.14563% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
headroom/log_safety.py	95.07%	6 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

[chopratejas]

Found while picking this up in #4111: at 189387a, describe_exception(OSError(10**30, "x")) raises OverflowError: Python int too large to convert to C int from os.strerror. Because callers use it inside except blocks, a backend that raises an OSError with an out-of-range errno would turn a logged fallback into an unhandled error. Devin flagged the same thing on #4111 (log_safety.py line 64).

[JerrettDavis]

Re-reviewed changed head cd50ad3, including the discussion and inline replies. The earlier OSError.strerror blocker is fixed: the helper uses system errno text, and my credential/newline reproduction no longer leaks. All 18 helper tests pass.

The URL blocker remains at the production call site. With HEADROOM_DEBUG_DUMP=redacted (no full-content opt-in), I called write_upstream_error_dump with an upstream URL containing a path credential. The actual saved JSON retains https://upstream.example/botPRIVATE_CREDENTIAL_CANARY/v1/messages. A second call with the percent-encoded path /bot%50RIVATE_CREDENTIAL_CANARY/v1/messages also persists the secret. This reproduction exercises the writer, not just redact_url.

I read the reply that route names cannot reliably be distinguished from secrets. That is precisely why an arbitrary upstream path should not be treated as safe. The helper's docstring and PR-body restriction are not enforced by write_upstream_error_dump, which accepts the complete URL. Please omit arbitrary paths from redacted diagnostics, or substitute an explicitly known safe route, and add production-writer regressions for plain and encoded path credentials. Full-content mode can retain its explicit opt-in behavior.

Qualification stops at this reproduced blocker. No contributor source changes or merge were performed.

[chopratejas]

@JerrettDavis thanks, you're right that the docstring wasn't an enforcement.

1. Fixed in b566a75: redact_url keeps only scheme, host and port. Userinfo, query and fragment are always dropped, and any path becomes /<path>. The path survives only with the explicit content opt-in HEADROOM_DEBUG_DUMP=full; a full dump still never gets the query. The redacted upstream-error dump writes https://upstream.example/<path> for both your plain /botPRIVATE_CREDENTIAL_CANARY/... and percent-encoded /bot%50RIVATE... URLs. test_redacted_dump_never_writes_path_credentials runs both through the real write_upstream_error_dump.
2. Also in b566a75, from the reviews on the PRs that use this helper:
   • describe_exception no longer formats an errno outside the C int range ([Errno out of range]), so OSError(10**10000) can't make the description raise;
   • WarnOnce now resets every window for all keys, so a recurring failure warns again once an hour, still at most limit + 1 warnings per window;
   • a zero or negative window is rejected.
3. The dependent PRs (fix(transforms): log the compression failures that silently become "no savings" #4092, fix(savings,ccr): log the ledger and MCP failures that are swallowed silently #4105, fix(memory): log storage and SQLite failures that currently make retrieval miss silently #4111, fix(proxy): let HEADROOM_LOG_LEVEL=debug turn on Headroom's own debug logs #4137, and refactor(transforms): share one CCR persist helper for the Rust-backed compressors #4094/refactor(integrations): share the local/cloud compress backend of asgi and litellm_callback #4124/fix(proxy): make handler failures debuggable from the logs #4127 once their main-merge conflicts are resolved) carry this commit.

[devin-ai-integration]

Note

Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.

Devin Review found 3 new potential issues.

[devin-ai-integration]

🔍 Exception diagnostics differ from described behavior

The PR description promises <dynamic code> for runtime frames and locations for imported libraries. describe_exception now omits both, leaving only exception types when no Headroom frame qualifies.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[chopratejas]

Fixed: the PR description now matches the code. Only Headroom's own frames are named and other frames are skipped (no <dynamic code> token); the redact_url and safe_id rules are written out in full.