Summary
#1491 landed the #1452 fallback on main (75ef3ca..). The fallback itself can raise, which loses the record — the exact failure #1452 existed to prevent, one level down inside the guard.
This was raised as a blocking review finding on #1493 and independently by a red-team pass, and the fix was written and tested there. #1491 merged only the first of that branch's two commits, and #1493 was subsequently closed, so the guard never reached main.
The defect
src/youtube_extension/backend/config/logging_config.py, current main:
safe["serialization_error"] = f"{type(exc).__name__}: {exc}"The f-string calls exc.__str__. When the primary json.dumps fails because a value's __str__ raised, the exception it raised may itself be unrenderable — and then this line raises inside the handler meant to save the record. logging swallows it via Handler.handleError and drops the record.
Reproduction
Verified against origin/main just now, logging healthy → poisoned → healthy through a real StreamHandler:
Input (extra={"request_id": …}) |
Records reaching sink |
value whose __str__ raises an exception whose own __str__ raises |
2 of 3 |
value whose __str__ raises an exception whose __str__ returns a non-str (so str() raises TypeError) |
2 of 3 |
class EvilExc(Exception):
def __str__(self): raise RuntimeError("exc.__str__ exploded")
class RaisesEvil:
def __str__(self): raise EvilExc()
logger.info("poisoned", extra={"request_id": RaisesEvil()}) # record lostThe second row is worth noting separately: it does not need a raising __str__ on the exception at all, only one returning the wrong type. A guard written against the first case alone still misses it.
Everything else #1452 asked for does work on main — circular containers and plain exploding-__str__ values are handled, and the degraded path still escapes attacker content (no #1429 regression). This is strictly the residual case.
Severity: low
Reaching it requires an extra value whose __str__ raises and an exception that cannot itself be rendered. No live call site passes a non-scalar — CodeRabbit's search and mine independently found no direct extra={"request_id": ...} call site, and the request_id created in error_handling_middleware.py is a UUID string. Not attacker-reachable: a header-supplied value is a string, and strings serialize fine.
It is a robustness gap in a fallback, reachable only once you are already in the fallback. Filing it because it is a known-and-diagnosed defect in shipped code, not because it is urgent.
Fix
Written, tested, and pushed — it is the second commit on claude/clever-heisenberg-ctwlue (5baae24), which is 52 lines on top of what main already has:
try:
reason = f"{type(exc).__name__}: {exc}"
except Exception: # noqa: BLE001 - the class name alone still tells us why
reason = type(exc).__name__
safe["serialization_error"] = reasontype(exc).__name__ is a plain class attribute and cannot raise, and safe holds only filtered scalars plus that string, so the final json.dumps cannot raise either. That terminates the recursion rather than moving it one level along.
Includes 2 parametrized regression tests covering both inputs. Non-vacuous: both fail against the unguarded version and pass with it (2 failed, 23 passed → 25 passed). ruff clean.
CodeRabbit reviewed the fix and confirmed it addresses the finding.
Note on why this needs a deliberate step
1b93605 is not an ancestor of main — #1491 squash-merged it — so claude/clever-heisenberg-ctwlue now reports as conflicting even though its only real delta is the 52 lines above. The conflict is history divergence from the squash, not overlapping edits. Cherry-picking or rebasing the single guard commit onto main resolves it:
git fetch origin claude/clever-heisenberg-ctwlue
git checkout -b fix/format-json-guard origin/main
git cherry-pick 5baae24
I have deliberately not reopened #1493 or opened a replacement PR — the close was an explicit disposition and reversing it is not mine to do. Filing this so the diagnosis and the tested fix are not lost with the closed PR.
Acceptance criteria
- A record whose enrichment raises an unrenderable exception is still emitted, with
level authoritative.
- The same holds when the exception's
__str__ returns a non-str.
- Regression tests cover both and fail against the current
main implementation.
Summary
#1491 landed the #1452 fallback on
main(75ef3ca..). The fallback itself can raise, which loses the record — the exact failure #1452 existed to prevent, one level down inside the guard.This was raised as a blocking review finding on #1493 and independently by a red-team pass, and the fix was written and tested there. #1491 merged only the first of that branch's two commits, and #1493 was subsequently closed, so the guard never reached
main.The defect
src/youtube_extension/backend/config/logging_config.py, currentmain:The f-string calls
exc.__str__. When the primaryjson.dumpsfails because a value's__str__raised, the exception it raised may itself be unrenderable — and then this line raises inside the handler meant to save the record.loggingswallows it viaHandler.handleErrorand drops the record.Reproduction
Verified against
origin/mainjust now, logging healthy → poisoned → healthy through a realStreamHandler:extra={"request_id": …})__str__raises an exception whose own__str__raises__str__raises an exception whose__str__returns a non-str(sostr()raisesTypeError)The second row is worth noting separately: it does not need a raising
__str__on the exception at all, only one returning the wrong type. A guard written against the first case alone still misses it.Everything else #1452 asked for does work on
main— circular containers and plain exploding-__str__values are handled, and the degraded path still escapes attacker content (no #1429 regression). This is strictly the residual case.Severity: low
Reaching it requires an
extravalue whose__str__raises and an exception that cannot itself be rendered. No live call site passes a non-scalar — CodeRabbit's search and mine independently found no directextra={"request_id": ...}call site, and therequest_idcreated inerror_handling_middleware.pyis a UUID string. Not attacker-reachable: a header-supplied value is a string, and strings serialize fine.It is a robustness gap in a fallback, reachable only once you are already in the fallback. Filing it because it is a known-and-diagnosed defect in shipped code, not because it is urgent.
Fix
Written, tested, and pushed — it is the second commit on
claude/clever-heisenberg-ctwlue(5baae24), which is 52 lines on top of whatmainalready has:type(exc).__name__is a plain class attribute and cannot raise, andsafeholds only filtered scalars plus that string, so the finaljson.dumpscannot raise either. That terminates the recursion rather than moving it one level along.Includes 2 parametrized regression tests covering both inputs. Non-vacuous: both fail against the unguarded version and pass with it (
2 failed, 23 passed→25 passed).ruffclean.CodeRabbit reviewed the fix and confirmed it addresses the finding.
Note on why this needs a deliberate step
1b93605is not an ancestor ofmain— #1491 squash-merged it — soclaude/clever-heisenberg-ctwluenow reports as conflicting even though its only real delta is the 52 lines above. The conflict is history divergence from the squash, not overlapping edits. Cherry-picking or rebasing the single guard commit ontomainresolves it:I have deliberately not reopened #1493 or opened a replacement PR — the close was an explicit disposition and reversing it is not mine to do. Filing this so the diagnosis and the tested fix are not lost with the closed PR.
Acceptance criteria
levelauthoritative.__str__returns a non-str.mainimplementation.