Skip to content

Conversation

@jwm4
Copy link
Collaborator

@jwm4 jwm4 commented Jan 11, 2026

Problem

When running review_roadmap with self-reflection enabled, the reflection node was logging a warning:

reflection_response_not_json   content='```json\n{"passed": true, "notes": "Self-review: Roadmap is well-structured...

The LLM was returning valid JSON, but wrapped in markdown code fences (json ... ) because the prompt shows the expected format that way. This caused json.loads() to fail, triggering the fallback path.

Solution

  • Add regex-based code fence stripping before JSON parsing
  • Handle both complete code fences (json...\n) and truncated ones (where the closing ``` is missing)
  • The fix is robust and will still work if the LLM returns plain JSON without code fences

Testing

Added two new tests:

  • test_reflection_handles_json_in_code_fence - Tests complete code fence handling
  • test_reflection_handles_truncated_code_fence - Tests truncated code fence handling

All 91 tests pass with 84.32% coverage.

The LLM often returns JSON wrapped in markdown code fences (```json ... ```)
because the prompt shows the expected format that way. This caused JSON parsing
to fail, triggering the fallback path.

Changes:
- Add regex-based code fence stripping before JSON parsing
- Handle both complete and truncated code fences
- Add tests for code fence parsing scenarios
@jwm4
Copy link
Collaborator Author

jwm4 commented Jan 11, 2026

🗺️ Auto-Generated Review Roadmap

This roadmap was automatically generated by review-roadmap.
Model: anthropic-vertex/claude-opus-4-5


Review Roadmap: Strip Markdown Code Fences from Reflection JSON

High-Level Summary

This PR fixes a parsing issue in the reflection node of the review roadmap agent. When the LLM returns JSON wrapped in markdown code fences (```json ... ```), the json.loads() call was failing because it received the raw fenced content instead of clean JSON. The fix adds regex-based preprocessing to strip these fences before parsing, with handling for both complete and truncated fence patterns.


Review Order

This is a small, focused bug fix. I recommend reviewing in this order:

1. Start with the Implementation (nodes.py)

Lines to examine:

Key questions:

  • Does the regex correctly handle all edge cases (with/without json language tag, with/without newlines)?
  • Is the import placement appropriate (inside the function)?

2. Then Review the Tests (test_agent_nodes.py)

Lines to examine:

Key questions:

  • Do the test cases cover the scenarios mentioned in the description?
  • Are assertions sufficient to verify correct behavior?

Watch Outs

1. Regex Edge Cases in nodes.py

  • Pattern r'^```(?:json)?\s*\n?(.*?)\n?```$': The ? after \n makes newlines optional. Verify this handles:
    • ```json{"passed": true}``` (no newlines at all)
    • ```\n{"passed": true}\n``` (no language tag)
    • Content with internal newlines in the JSON

2. Double Stripping Logic

  • Lines 483-488: The fallback branch uses two separate re.sub() calls. Consider whether a truncated response like ```json\n{"passed": true (no closing fence, no closing brace) would still fail at json.loads() — that's expected, but worth confirming the flow gracefully falls back.

3. Import Placement

  • The import re is added at line 475 inside the function alongside import json. This is consistent with the existing pattern but could be moved to module-level for clarity — a minor style consideration.

4. Test Coverage Completeness

  • Confirm both tests actually exercise distinct code paths (one hits the main if match: branch, the other hits the else: truncated handling branch)
  • Consider: Is there a test for plain JSON with no fences to ensure the fix doesn't break the happy path?

Existing Discussions

No comments found on this PR yet. This is a fresh review.


Quick Checklist

  • Regex handles all fence variations (with/without language tag, newlines)
  • Truncated fence handling is robust
  • Plain JSON (no fences) still parses correctly
  • Tests clearly map to the two code paths (complete fence vs truncated)
  • No regressions to existing reflection functionality

@jwm4 jwm4 merged commit d8aaa89 into main Jan 11, 2026
1 check passed
@jwm4 jwm4 deleted the fix/reflection-json-code-fence-parsing branch January 11, 2026 18:26
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.

2 participants