Skip to content

fix: audit records <stream> placeholder for streamed bodies (#4)#8

Merged
henriquebastos merged 3 commits into
mainfrom
claude/issue-4-20250915-2231
Jun 13, 2026
Merged

fix: audit records <stream> placeholder for streamed bodies (#4)#8
henriquebastos merged 3 commits into
mainfrom
claude/issue-4-20250915-2231

Conversation

@henriquebastos

Copy link
Copy Markdown
Owner

Why

Closes #4. Audit.audit() materialized every request/response body (request.body/response.text). For streamed bodies (file-like/generator uploads, or stream=True downloads) that either crashes or defeats streaming by loading everything into memory. Audit must never break or distort the request it observes.

What

Records a <stream> placeholder instead of materializing streamed bodies (decision recorded in decisions.md, option 1).

Builds on the original 2025 fix, reworked so it actually passes its tests on current main:

  • Response: detect streaming via the stream kwarg Audit.send already receives from Session.send — the original response.raw.isclosed() sniff was False for ordinary responses and produced a false <stream>.
  • Request: keep the file-like/iterable detection (correct for real adapters); the failing file-like test relied on responses, which reads the BytesIO into a string before audit sees it — replaced with a small canned adapter that leaves request.body intact.
  • Conformed the 2025 code to current lint (stream made keyword-only for FBT002).

Folds Ariad docs: roadmap → Done (incl. completed branch cleanup), decisions.md → landed.

Validation

  • pytest: 50 passed locally (all 6 TestAuditStreams green); CI runs lint + py3.10–3.13.
  • pre-commit (ruff 0.11.9) clean.

claude Bot and others added 3 commits June 13, 2026 13:28
- Add comprehensive test cases for stream scenarios
- Implement _safe_get_request_body() for request stream detection
- Implement _safe_get_response_body() for response stream detection
- Return '<stream>' placeholder for file-like objects, generators, and streamed responses
- Preserve existing behavior for strings and bytes
- Follows Option 1 (Skip/Empty approach) from issue analysis

Fixes #4

Co-authored-by: Henrique Bastos <henriquebastos@users.noreply.github.com>
The original fix keyed response streaming off response.raw.isclosed(),
which is False for ordinary responses and produced a false <stream>.
Use the stream kwarg that Audit.send already receives from Session.send
instead, and emit <stream> only when stream=True.

The file-like request test relied on `responses`, which reads the
BytesIO into a string before audit sees it; replace it with a canned
adapter that leaves request.body untouched. Conform the 2025 code to
the current lint rules.
Move issue #4 to Done, mark the completed branch cleanup, refresh
Current Focus/Active now that all pending fixes have landed, and flip
the audit-stream decision to landed with its detection mechanism.
@claude

claude Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @henriquebastos's task —— View job


Pull Request Review Complete ✅

Review Checklist:

  • Read repository guidelines (no CLAUDE.md found)
  • Analyze core implementation in audit.py
  • Review test coverage in test_audit.py
  • Check documentation updates
  • Assess code quality and best practices
  • Identify potential bugs or security concerns
  • Evaluate performance implications

🎯 Overall Assessment: EXCELLENT

This PR demonstrates exceptional code quality and engineering practices. The implementation is clean, well-tested, and thoroughly documented. I found no significant issues and several commendable practices.

📋 Code Quality & Best Practices

Strengths:

  • Clean separation of concerns: _safe_get_request_body() and _safe_get_response_body() methods clearly separate stream detection logic audit.py:64-84
  • Defensive programming: Proper type checking with isinstance() and hasattr() guards audit.py:68-73
  • Keyword-only parameter: Making stream keyword-only in audit() method follows modern Python best practices audit.py:86
  • Consistent placeholder: Using "<stream>" as a clear, non-ambiguous placeholder for all stream types
  • Backwards compatibility: Non-stream requests/responses work exactly as before

Minor observations:

  • Code follows the repository's ruff configuration consistently (line length, import order, etc.)
  • Type hints are properly used throughout with TypedDict definitions

🐛 Bugs & Issues: NONE FOUND

The implementation correctly handles all stream detection scenarios:

  1. Request streams: File-like objects (hasattr(body, "read")) and non-bytes iterables (generators)
  2. Response streams: Uses the stream kwarg passed from Session.send
  3. Non-streams: Strings, bytes, and None are handled as before

The original bug with response.raw.isclosed() giving false positives has been properly fixed by using the reliable stream parameter.

🚀 Performance Considerations

Excellent performance design:

  • Zero materialization: Stream bodies are never read/consumed, preserving memory efficiency
  • Lazy evaluation: Stream detection uses simple type checks without expensive operations
  • No regression: Non-stream paths have identical performance to the original implementation

The <stream> placeholder approach is the optimal solution for this use case—it preserves streaming semantics while maintaining audit functionality.

🔒 Security: NO CONCERNS

  • No secrets or sensitive data exposure
  • Stream content is explicitly not logged (by design)
  • No injection vulnerabilities introduced
  • Proper input validation on body types

🧪 Test Coverage: COMPREHENSIVE

The test suite in test_audit.py is exemplary:

New tests cover all scenarios:

  • File-like request bodies (with custom CannedAdapter to preserve stream nature) test_audit.py:177-192
  • Generator request bodies test_audit.py:194-210
  • String/bytes request bodies (regression tests) test_audit.py:212-236
  • Streamed responses test_audit.py:238-249
  • Normal responses (regression tests) test_audit.py:251-262

Testing innovation: The CannedAdapter (test_audit.py:155-172) is a clever solution to test file-like bodies without the responses library consuming them first.

📚 Documentation Quality

Excellent documentation updates:

  • Decision recorded: Clear rationale in decisions.md with implementation details decisions.md:29-38
  • Roadmap updated: Progress properly tracked roadmap/index.md:30
  • Code comments: Clear method docstrings explaining stream detection logic audit.py:65,80

🔧 Minor Suggestions

  1. Type annotation enhancement (very minor):

    # Current
    def _safe_get_request_body(self, request):
    # Could be
    def _safe_get_request_body(self, request) -> str:
  2. Consider adding a constant for the placeholder (optional):

    STREAM_PLACEHOLDER = "<stream>"

🎉 Commendable Practices

  1. TDD Approach: Tests were clearly written to drive the implementation
  2. Comprehensive regression testing: Ensures existing functionality isn't broken
  3. Clean git history: Well-structured commits with clear messages
  4. Documentation-driven development: Decision rationale captured before implementation

Recommendation: ✅ APPROVE

This PR is ready to merge. It successfully solves the streaming audit problem with an elegant, well-tested solution that maintains backwards compatibility and follows excellent engineering practices.


@henriquebastos henriquebastos merged commit 5f290b0 into main Jun 13, 2026
6 checks passed
@henriquebastos henriquebastos deleted the claude/issue-4-20250915-2231 branch June 13, 2026 16:31
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.

Problem with auditlog when request or response is stream

1 participant