Skip to content

fix: redact reconnect exception logs - #250

Draft
Justinabox wants to merge 1 commit into
mainfrom
agent/redact-reconnect-log-20260727
Draft

fix: redact reconnect exception logs#250
Justinabox wants to merge 1 commit into
mainfrom
agent/redact-reconnect-log-20260727

Conversation

@Justinabox

@Justinabox Justinabox commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

  • redact unexpected reconnect exception logging to the retry attempt and exception class only
  • prevent raw modem/AT payloads embedded in exception messages from reaching default error logs or tracebacks
  • add a deterministic caplog regression test for the reconnect path

Partial progress toward #61 (one narrow reconnect-log privacy slice).

TDD evidence

  • RED: tests/test_modem.py::TestModemAutoReconnect::test_unexpected_reconnect_error_does_not_leak_raw_payload initially failed because the sensitive marker and traceback appeared in the log.
  • GREEN: the focused regression test passes after the minimal logging change.

Verification

  • git diff --check
  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/test_modem.py::TestModemAutoReconnect::test_unexpected_reconnect_error_does_not_leak_raw_payload -q — 1 passed
  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q — 992 passed

Risk

Low: reconnect retry and exponential-backoff control flow are unchanged; diagnostic detail is intentionally limited to exception type to keep default logs PII-safe.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix (privacy blocker); not ready to land.

The generic reconnect-exception branch is correctly redacted, but two reconnect-specific transport paths still interpolate raw exception text:

  • stale transport close() failure at DEBUG;
  • TransportError/OSError reconnect/open failure at WARNING.

Transport exception strings can carry modem/transport payloads, so this remains a raw-log sink. Please run a fresh TDD cycle with caplog sentinels for both paths, prove the current leak, then log only safe metadata such as exception type and attempt count. Rebase on current main, rerun git diff --check plus the required full pytest gate, and request fresh privacy review.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix (privacy blocker). The generic reconnect-exception branch is safely redacted, but reconnect-specific TransportError/OSError warning logging and stale-transport-close DEBUG logging still interpolate raw exception text. Please redact those sinks to safe operational metadata (attempt/type only) and add sentinel caplog regressions for both paths, then rebase on current main and rerun git diff --check plus the required full pytest gate.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix (privacy blocker), so this is not ready to land.

The generic reconnect exception branch is redacted, but the TransportError/OSError reconnect path still logs %s with the raw exception message. The current test raises RuntimeError, so it does not cover that leak.

Please log only safe metadata (attempt plus exception class) in both exception branches and add a captured-log regression using a TransportError with a sentinel message. Rebase, run the full required suite, and request a fresh privacy review.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix (privacy blocker).

The changed generic reconnect-exception branch is safe, but two existing reconnect paths still log exception text and can expose modem/AT payloads:

  1. the outer TransportError/OSError handling after open(); and
  2. the stale-close nested handler.

Independent probes on this exact head confirmed raw sentinel text appears in each path. Please start a fresh TDD cycle: add caplog regressions that exercise both paths and prove raw exception payloads are absent, watch them fail on the current head, then redact/log only an allowlisted exception category while preserving retry/backoff control flow. Re-run the required full gate after the fix.

The current generic-Exception test does not cover these two TransportError paths, so this PR is not mergeable yet.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release-lane classification: needs fix/revalidation before merge.

This draft was not independently reviewed or integration-tested in this pass and its GitHub mergeability is stale/unknown after current-main advances. Please rebase or otherwise validate it against current main, run the required full local gate, and request a fresh release review with exact head SHA and gate evidence. Do not treat absence of hosted checks as approval.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release classification: needs-fix for this release pass.

The draft has not received a current exact-head review and prescribed local integration gate over the advancing main baseline. Rebase/repackage as needed against current main, resolve any conflicts, then provide a narrow behavioral review plus passing git diff --check and the prescribed full test suite before it can be reconsidered for landing.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix. The generic reconnect exception path is redacted, but stale-close TransportError/OSError and reconnect TransportError/OSError paths still interpolate raw exception text. Add fresh caplog tests with payload-bearing sentinels for both paths, remove raw exception formatting, rebase, and rerun required gates.

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.

1 participant