Skip to content

Conversation

@matdev83
Copy link
Owner

@matdev83 matdev83 commented Oct 12, 2025

Summary

  • return a structured JsonRepairResult from JsonRepairService so primitive payloads like null are treated as successful repairs
  • update the JSON repair middleware and streaming processor to use the new result object and propagate repaired content
  • expand unit coverage to assert proper handling of null payloads and the revised repair contract

Testing

  • pytest --override-ini addopts="" tests/unit/core/services/test_json_repair_service.py tests/unit/core/services/test_json_repair_middleware.py tests/unit/json_repair_processor_test.py
  • pytest --override-ini addopts="" (fails: missing pytest_asyncio, respx, pytest_httpx)

https://chatgpt.com/codex/tasks/task_e_68ec26ffb7448333ace9a74c4ba180a1

Summary by CodeRabbit

  • Bug Fixes

    • Correctly preserves and accepts JSON "null" payloads, marking them as repaired when appropriate.
    • Improves streaming behavior: emits repaired content when successful and reliably falls back to original content on failure.
  • Refactor

    • Unified success/failure handling for JSON repair to enhance robustness, observability, and consistent behavior across non-streaming and streaming flows.
  • Tests

    • Added tests for null payload handling and failure scenarios to strengthen reliability.

@coderabbitai
Copy link

coderabbitai bot commented Oct 12, 2025

Walkthrough

The JSON repair flow now returns a JsonRepairResult object across middleware, streaming processor, and service layers. Logic, metrics, and logging were updated to branch on result.success and use result.content. Tests were revised and expanded, including explicit handling of "null" payloads and adjustments to mocks and assertions.

Changes

Cohort / File(s) Summary
Core service API refactor
src/core/services/json_repair_service.py
Added immutable JsonRepairResult dataclass. Changed repair_and_validate_json to return JsonRepairResult instead of dict|None. Updated repair_json return type to Any. Standardized error handling and logging; explicit schema is-not-none checks.
Middleware adaptation
src/core/app/middleware/json_repair_middleware.py
Switched to JsonRepairResult usage. Control flow, logging, and content mutation based on result.success/result.content. Consolidated metrics under computed suffix json_repair.non_streaming.{metric_suffix}. Imported JsonRepairResult.
Streaming processor adaptation
src/core/services/streaming/json_repair_processor.py
Replaced tuple returns with JsonRepairResult across process paths. Updated _handle_json_completion signature and callers. Adjusted success/failure handling, logging, metrics, and final buffer flush. Import updated to include JsonRepairResult.
Middleware tests
tests/unit/core/services/test_json_repair_middleware.py
Added test for "null" payload: content remains "null", repaired flag True.
Service tests
tests/unit/core/services/test_json_repair_service.py
Updated assertions to use result.success/content. Added test for allowing "null" payload producing success with None content.
Streaming tests
tests/unit/json_repair_processor_test.py
Imported JsonRepairResult. Updated failing service double to return JsonRepairResult. Renamed test to reflect failure instead of None return.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant Middleware
  participant JsonRepairService

  Client->>Middleware: process(response)
  Middleware->>JsonRepairService: repair_and_validate_json(payload, schema?, strict)
  JsonRepairService-->>Middleware: JsonRepairResult{ success, content }
  alt success == true
    Middleware->>Middleware: replace response.content with result.content
    Middleware->>Metrics: emit json_repair.non_streaming.{suffix}
    Middleware-->>Client: response (repaired)
  else success == false
    Middleware->>Metrics: emit json_repair.non_streaming.{suffix}
    Middleware-->>Client: response (original)
  end
Loading
sequenceDiagram
  autonumber
  participant StreamSrc as Stream Source
  participant Processor as JsonRepairProcessor
  participant Service as JsonRepairService
  participant Downstream

  StreamSrc->>Processor: chunk data
  Processor->>Processor: buffer/parse blocks
  Processor->>Service: repair_and_validate_json(block, schema?, strict?)
  Service-->>Processor: JsonRepairResult{ success, content }
  alt success
    Processor->>Downstream: emit result.content
  else failure
    Processor->>Downstream: emit original buffer
  end
  Note over Processor: On final flush, same success/failure handling
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I nibble bytes with whiskered care,
Stitching braces in the air.
A carrot-colored Result I bring—
success or not, a tidy thing.
If null appears, I do not fret;
I twitch, I fix—our JSON set.
Hop, repair, and ship—no sweat! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.53% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly summarizes the primary change—improving JSON repair behavior specifically for null payloads—and directly reflects the updates to the repair service, middleware, and tests described in the PR without extra detail or ambiguity.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/fix-identified-bug-in-files-i-k-b9g6ji

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dd216cf and f76099a.

📒 Files selected for processing (6)
  • src/core/app/middleware/json_repair_middleware.py (2 hunks)
  • src/core/services/json_repair_service.py (7 hunks)
  • src/core/services/streaming/json_repair_processor.py (5 hunks)
  • tests/unit/core/services/test_json_repair_middleware.py (1 hunks)
  • tests/unit/core/services/test_json_repair_service.py (1 hunks)
  • tests/unit/json_repair_processor_test.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting

Files:

  • tests/unit/core/services/test_json_repair_middleware.py
  • src/core/services/streaming/json_repair_processor.py
  • tests/unit/json_repair_processor_test.py
  • tests/unit/core/services/test_json_repair_service.py
  • src/core/services/json_repair_service.py
  • src/core/app/middleware/json_repair_middleware.py
src/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions

Files:

  • src/core/services/streaming/json_repair_processor.py
  • src/core/services/json_repair_service.py
  • src/core/app/middleware/json_repair_middleware.py
🧬 Code graph analysis (5)
tests/unit/core/services/test_json_repair_middleware.py (2)
src/core/app/middleware/json_repair_middleware.py (2)
  • JsonRepairMiddleware (20-107)
  • process (31-107)
src/core/interfaces/response_processor_interface.py (1)
  • ProcessedResponse (8-30)
src/core/services/streaming/json_repair_processor.py (3)
src/core/services/json_repair_service.py (2)
  • JsonRepairResult (18-22)
  • repair_and_validate_json (32-80)
src/core/common/exceptions.py (1)
  • JSONParsingError (231-238)
src/core/services/metrics_service.py (1)
  • inc (10-12)
tests/unit/json_repair_processor_test.py (2)
tests/unit/core/services/test_json_repair_middleware.py (1)
  • json_repair_service (13-14)
src/core/services/json_repair_service.py (2)
  • JsonRepairResult (18-22)
  • JsonRepairService (25-301)
tests/unit/core/services/test_json_repair_service.py (2)
tests/unit/core/services/test_json_repair_middleware.py (1)
  • json_repair_service (13-14)
src/core/services/json_repair_service.py (2)
  • repair_and_validate_json (32-80)
  • JsonRepairService (25-301)
src/core/app/middleware/json_repair_middleware.py (2)
src/core/services/json_repair_service.py (3)
  • JsonRepairResult (18-22)
  • JsonRepairService (25-301)
  • repair_and_validate_json (32-80)
src/core/services/metrics_service.py (1)
  • inc (10-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test (3.10)
🔇 Additional comments (12)
tests/unit/core/services/test_json_repair_middleware.py (1)

76-86: LGTM! Excellent test coverage for null payloads.

The test correctly verifies that "null" is handled as a valid JSON primitive, maintaining the content as "null" after repair and setting the repaired flag to True.

tests/unit/core/services/test_json_repair_service.py (2)

43-48: LGTM! Proper adaptation to JsonRepairResult API.

The variable rename from repaired to result and the updated assertions correctly reflect the new structured return type.


51-57: LGTM! Comprehensive null payload test.

The test correctly verifies that "null" is parsed as a valid JSON primitive, with success=True and content=None, which aligns with the PR objective to properly handle null payloads.

tests/unit/json_repair_processor_test.py (1)

7-48: LGTM! Proper test double implementation.

The FailingJsonRepairService now correctly returns JsonRepairResult(success=False, content=None) instead of None, aligning with the new API contract. The test rename from ...returns_none to ...fails better describes the actual behavior.

src/core/app/middleware/json_repair_middleware.py (2)

12-15: LGTM! Proper import of JsonRepairResult.

The import correctly includes JsonRepairResult alongside JsonRepairService, enabling the structured result handling throughout the middleware.


75-105: LGTM! Clean implementation of JsonRepairResult flow.

The changes properly handle the structured result object:

  • The new metric_suffix logic consolidates metric emission into a single call, improving maintainability
  • json.dumps(repair_result.content) correctly serializes repaired content, including null payloads (where content=None becomes "null")
  • Success/failure branching is clear and correct
src/core/services/json_repair_service.py (3)

17-23: LGTM! Well-designed result type.

The JsonRepairResult dataclass with frozen=True is an excellent choice:

  • Provides clear structure with success and content fields
  • Immutability prevents accidental modification
  • Type hints enable better static analysis

32-80: LGTM! Robust implementation of structured result API.

The changes properly implement the JsonRepairResult contract:

  • The condition change from if schema to if schema is not None on Line 51 is more explicit and correct
  • All paths consistently return JsonRepairResult with appropriate success flags
  • On schema validation failure (Line 69), returning success=False with the repaired content allows callers to access partially-valid JSON in best-effort mode
  • On parsing failure (Line 80), returning content=None correctly signals complete failure

82-93: LGTM! Correct return type for JSON values.

The return type change from dict[str, Any] to Any correctly reflects that JSON values can be primitives (null, bool, number, string), arrays, or objects. This properly handles cases like "null" which parse to None.

src/core/services/streaming/json_repair_processor.py (3)

70-75: LGTM! Proper result handling in streaming flow.

The logic correctly branches on repair_result.success:

  • On success, serializes repair_result.content to JSON
  • On failure, falls back to raw buffer content
    This ensures users always receive output while maintaining repair quality where possible.

154-185: LGTM! Robust completion handling with proper error recovery.

The _handle_json_completion method correctly:

  • Returns JsonRepairResult consistently
  • Handles exceptions gracefully in non-strict mode (Line 168), returning a failure result instead of propagating errors
  • Emits appropriate metrics based on success/failure
  • Logs failures with helpful context

199-215: LGTM! Consistent result handling in final buffer flush.

The final buffer flush maintains consistency with the completion flow:

  • Uses JsonRepairResult for repair outcomes
  • Serializes successful repairs with json.dumps(repair_result.content)
  • Falls back to raw buffer on failure
  • Properly emits metrics for both paths

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Oct 12, 2025

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/core/services/json_repair_service.py 84.61% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants