fix(logging): redact runner exception telemetry

[seonghobae]

Authority

• protected ancestor: develop@042b0c70531b229af3acbd0421a2f23098d848b3
• telemetry foundation / stacked base: fix(security): bound exception telemetry without raw traceback #1700 fix/structured-exception-telemetry@2d6800aea110ac04eeedcd5faa91479f62877408
• exact candidate head: c57b2d110c079fa862c97251aa835215f014365d
• owner finding: security(logging): stop treating exc_info tracebacks as exception-detail redaction #1698
• lifecycle: Draft — source/test/doctoring slice present; current-head static review has no blocking finding; formal independent approval and hosted evidence pending

Verified finding

backend/api/runner_ws.py retained two raw exception telemetry paths after #1700 established the shared bounded logging contract:

1. ConnectionManager.dispatch_command() used logger.exception() for arbitrary runner transport/dispatch failures;
2. _record_connector_signal_event_safely() used exc_info=True for SQLAlchemy persistence failures.

Python logging can render the caught exception value and traceback through those paths. Transport/provider failures may contain token-shaped values or internal paths, while database failures may contain credential-bearing connection strings or query/system detail. This is a concrete CWE-532 / OWASP Logging Cheat Sheet confidentiality gap, not a blanket logging rewrite.

RED → minimal causal repair

Test-only RED history precedes the production edit:

• 566f1f737d5aef8b0e7f5711950aecf6936dafd8 — database persistence failure with a PostgreSQL URI and token-like value must not render raw exception/traceback data;
• 4e8e800e31d1fd2bfbd0d5b0a70c8759ff29df3a — runner transport failure with an internal path and token-like value must preserve the existing deterministic public error while redacting telemetry.

Production repair 3026bac5d7c23c2bda08c4fa22d962531a247bbc consumes #1700's core.safe_logging.redacted_exception_info() contract instead of creating a runner-local sanitizer. Existing log severity and fixed operation messages are preserved. Public runner response, retry classification, signal-event persistence semantics, connection ownership, and database transaction scope are unchanged.

Doctoring a085240288baac27d4bb1373a62ed8ab73aa2abf records the decision, rejected alternatives, evidence limits, and traceability to CWE-532, OWASP Logging Cheat Sheet, and Python 3.14 logging documentation.

CodeRabbit's first exact-range inspection of predecessor a085240... found one valid test-isolation defect: the dispatch regression wrote module-global runner_ws.manager connection state without cleanup. Current-head repair c57b2d110c079fa862c97251aa835215f014365d adds an autouse manager reset before and after each case. CodeRabbit then re-inspected exact c57b2d1... against exact #1700 and reported no blocking issue, explicitly confirming that the fixture repairs the prior finding, the two production sinks use the bounded helper, the focused tests cover the intended confidentiality/public-response contracts, and git diff --check is clean. That response is a source-inspection comment, not a formal GitHub APPROVED review; list_pull_request_reviews remains empty and no formal approval is claimed.

Effective delta

Exactly three files relative to #1700; fresh compare is ahead 5 / behind 0 with merge base exactly 2d6800aea110ac04eeedcd5faa91479f62877408:

• backend/api/runner_ws.py
• backend/tests/test_runner_signal_exception_telemetry.py
• docs/doctoring/runner-signal-exception-telemetry.md

No workflow, dependency, database schema, provider contract, or UI change is included.

Verification boundary

The focused tests exercise both real logging sinks with secret-bearing exceptions and require the rendered records to retain only the operation message, exception type, and exception_fingerprint= marker while excluding the injected detail/token and traceback header. The dispatch test also requires the existing runner_dispatch_failed response. Test state is reset before and after each case.

Neither this connector session nor CodeRabbit's inspection executed repository tests, so those tests are source-backed acceptance contracts rather than a claimed local GREEN receipt. Exact-head hosted checks and formal independent approval are mandatory. Predecessor #1700 checks/reviews do not transfer.

Fresh exact-head workflow lookup for c57b2d1... returns zero PR-triggered repository workflows despite the material three-file delta and exact valid stacked parent. Treat this as another #1691 stacked-admission RED, not GREEN. Do not manufacture evidence through temporary retargeting, dummy/no-op commits, workflow copying, synthetic statuses, or predecessor receipt transfer.

Stack boundary

Keep Draft. Do not merge into #1700 while #1700 remains an unintegrated prerequisite. After #1700 and the stacked-workflow foundation integrate normally, reconcile this exact effective delta onto protected ancestry and regenerate all required exact-head evidence. No force push, destructive rebase, self-approval, review dismissal, admin bypass, or gate weakening.

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

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.}

[seonghobae]

@coderabbitai review

Please review exact current head a085240288baac27d4bb1373a62ed8ab73aa2abf against exact stacked base #1700 2d6800aea110ac04eeedcd5faa91479f62877408. Scope is the three-file runner exception-telemetry slice only; predecessor approvals do not transfer.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="2d6800aea110ac04eeedcd5faa91479f62877408"
head="a085240288baac27d4bb1373a62ed8ab73aa2abf"

git rev-parse "${base}^{commit}"
git rev-parse "${head}^{commit}"
git diff --name-status "$base" "$head"
git diff --check "$base" "$head"
git diff --find-renames --unified=80 "$base" "$head" -- \
  backend/api/runner_ws.py \
  backend/tests/test_runner_signal_exception_telemetry.py \
  docs/doctoring/runner-signal-exception-telemetry.md

printf '\n--- runner_ws outline ---\n'
ast-grep outline backend/api/runner_ws.py --items all --match 'ConnectionManager|_record_connector_signal_event_safely|dispatch_command' || true

printf '\n--- logging helper definitions/usages ---\n'
rg -n -C 4 --glob '*.py' '\bredacted_exception_info\s*\(' core backend || true

printf '\n--- focused test outline ---\n'
ast-grep outline backend/tests/test_runner_signal_exception_telemetry.py --items all || true

Length of output: 34817

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- safe_logging outline ---'
ast-grep outline backend/core/safe_logging.py --items all

printf '%s\n' '--- safe_logging implementation ---'
sed -n '1,180p' backend/core/safe_logging.py

printf '%s\n' '--- test fixture definitions ---'
fd -HI '^conftest\.py$' backend -x sh -c 'echo "--- $1"; rg -n -C 5 "(runner_ws|manager\.reset|autouse|fixture)" "$1" || true' sh {}

printf '%s\n' '--- runner manager test mutations and reset calls ---'
rg -n -C 5 --glob '*.py' '(?:runner_ws\.)?manager\.(?:active_connections|connection_records|pending_responses|reset)\b' backend/tests

printf '%s\n' '--- runner-related test modules ---'
fd -HI 'runner.*\.py$' backend/tests -x sh -c 'echo "--- $1"; rg -n -C 3 "(?:runner_ws\.)?manager\.(?:reset|active_connections|connection_records)" "$1" || true' sh {}

Length of output: 12706

---

@seonghobae I reviewed exact head a085240288baac27d4bb1373a62ed8ab73aa2abf against base 2d6800aea110ac04eeedcd5faa91479f62877408.

I found one test-isolation issue.

• backend/tests/test_runner_signal_exception_telemetry.py: The dispatch test writes directly to the module-global runner_ws.manager.active_connections and runner_ws.manager.connection_records, but it does not reset them. A later test can select _SecretFailingRunnerConnection for org-acme and workspace-org-acme. This can make test results depend on collection order. Add an autouse fixture in this module that calls runner_ws.manager.reset() before and after each test, or clean up the specific connection entries in a finally block.

The two production logging paths correctly use redacted_exception_info(exc). The helper replaces the exception value and traceback with bounded synthetic exception data. The focused tests cover both affected paths.

I did not execute repository tests. The review environment only inspected source files.

You are interacting with an AI system.

[seonghobae]

@coderabbitai review

Current exact head advanced by a test-isolation repair only: c57b2d110c079fa862c97251aa835215f014365d against exact stacked base #1700 2d6800aea110ac04eeedcd5faa91479f62877408. Please review this head; the prior a085240... request is predecessor evidence only.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="2d6800aea110ac04eeedcd5faa91479f62877408"
head="c57b2d110c079fa862c97251aa835215f014365d"

printf '%s\n' '--- exact commits ---'
git rev-parse "${base}^{commit}"
git rev-parse "${head}^{commit}"
printf '%s\n' '--- ancestry ---'
git merge-base "$base" "$head"
git merge-base --is-ancestor "$base" "$head"

printf '%s\n' '--- changed files ---'
git diff --name-status "$base" "$head"
printf '%s\n' '--- whitespace errors ---'
git diff --check "$base" "$head"

printf '%s\n' '--- exact scoped diff ---'
git diff --find-renames --unified=100 "$base" "$head" -- \
  backend/api/runner_ws.py \
  backend/tests/test_runner_signal_exception_telemetry.py \
  docs/doctoring/runner-signal-exception-telemetry.md

printf '%s\n' '--- runner WebSocket symbols ---'
ast-grep outline backend/api/runner_ws.py --items all --match 'ConnectionManager|dispatch_command|_record_connector_signal_event_safely|reset'

printf '%s\n' '--- focused test symbols ---'
ast-grep outline backend/tests/test_runner_signal_exception_telemetry.py --items all

printf '%s\n' '--- reset and manager-state test usage ---'
rg -n -C 5 --glob '*.py' '(?:runner_ws\.)?manager\.(?:reset|active_connections|connection_records|pending_responses)\b' backend/tests

printf '%s\n' '--- redaction helper ---'
sed -n '1,220p' backend/core/safe_logging.py

Length of output: 38064

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="2d6800aea110ac04eeedcd5faa91479f62877408"
head="c57b2d110c079fa862c97251aa835215f014365d"

git rev-parse "${base}^{commit}"
git rev-parse "${head}^{commit}"
printf '%s\n' '--- merge base ---'
git merge-base "$base" "$head"
printf '%s\n' '--- changed files ---'
git diff --name-status "$base" "$head"
printf '%s\n' '--- diff check ---'
git diff --check "$base" "$head"
printf '%s\n' '--- scoped diff ---'
git diff --unified=100 "$base" "$head" -- backend/api/runner_ws.py backend/tests/test_runner_signal_exception_telemetry.py docs/doctoring/runner-signal-exception-telemetry.md
printf '%s\n' '--- manager outline ---'
ast-grep outline backend/api/runner_ws.py --items all --match 'ConnectionManager|dispatch_command|_record_connector_signal_event_safely|reset'
printf '%s\n' '--- test outline ---'
ast-grep outline backend/tests/test_runner_signal_exception_telemetry.py --items all
printf '%s\n' '--- redaction helper ---'
sed -n '1,220p' backend/core/safe_logging.py
printf '%s\n' '--- related fixture state ---'
rg -n -C 4 --glob '*.py' '(?:runner_ws\.)?manager\.(?:reset|active_connections|connection_records|pending_responses)\b' backend/tests

Length of output: 36769

---

@seonghobae I reviewed exact head c57b2d110c079fa862c97251aa835215f014365d against exact base 2d6800aea110ac04eeedcd5faa91479f62877408.

I found no blocking issue in the three-file delta.

The new autouse fixture resets runner_ws.manager before and after each telemetry test. This fixes the prior test-isolation finding. ConnectionManager.reset() also clears all related connection state and cancels unfinished response futures.

Both affected logging paths now use redacted_exception_info(exc). The focused tests verify the operation message, exception type, and fingerprint. They also verify that injected secret text and traceback output are absent. The dispatch test preserves the runner_dispatch_failed response.

git diff --check reported no whitespace errors.

I did not run repository tests. This inspection environment only performed static source review.

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.