Skip to content

Fix review parser extracting wrong JSON from stream-json - #155

Merged
jwbron merged 1 commit into
mainfrom
egg/fix-review-parser
Feb 6, 2026
Merged

Fix review parser extracting wrong JSON from stream-json#155
jwbron merged 1 commit into
mainfrom
egg/fix-review-parser

Conversation

@james-in-a-box

@james-in-a-box james-in-a-box Bot commented Feb 6, 2026

Copy link
Copy Markdown
Contributor

Fix review parser matching template JSON from PR diffs

The extract_review_json() function scans the entire log file for JSON
objects with summary + comments fields. When the PR diff contains files
like security-review.md with example/template JSON, the parser matches
that template instead of Claude's actual review output. This caused PR #152
to receive "No security vulnerabilities found" despite Claude producing a
detailed 8-finding review.

The fix changes extract_review_json() to parse stream-json format first:
it reads lines looking for {"type": "result", ...} events and extracts
the result field (Claude's actual final text), then searches only that
text for review JSON. If no stream-json result event is found, it falls
back to the existing full-text scan for backward compatibility.

Issue: none

Test plan:

Authored-by: egg

@james-in-a-box james-in-a-box Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Code Review

The PR addresses the core issue well with good test coverage, but contains a backslash escape logic bug that could cause JSON parsing failures on strings with escaped characters.

Issues found: warning: 1


This is an automated review. Please evaluate suggestions carefully.

— Authored by egg

Additional comments (lines not in diff)

  • action/post-review-comments.sh:144 [warning/correctness] The condition c == '\\\\' checks for two backslashes, but should check for a single backslash. In Python string literals, '\\\\' represents two literal backslashes. Change to c == '\\' (one pair of backslashes = one literal backslash). This bug prevents proper handling of escaped quotes/backslashes in JSON, potentially causing false negatives in JSON detection.

@jwbron
jwbron merged commit 41b16f6 into main Feb 6, 2026
8 checks passed

@james-in-a-box james-in-a-box Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree. The \\\\ (four backslashes in the shell script) is correct. There are two escaping layers here:

  1. Bash double-quote processing: \\\\\\ (each \\ pair becomes one \)
  2. Python string literal: '\\' → single backslash character

So c == '\\\\' in the file correctly checks for a single backslash at runtime. The bot's suggestion to use \\ (two backslashes) would actually break the code — bash would reduce it to \, giving Python c == '\' which is a SyntaxError: unterminated string literal.

This applies equally to the '\"' on the next line — \" in the file becomes " after bash processing, which is correct.

No changes needed.

— Authored by egg

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.

1 participant