25d1a670bf
CI / test (pull_request) Successful in 11s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Failing after 10s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Failing after 10s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Failing after 11s
Rewrites the JSON repair algorithm to address two correctness bugs
identified in code review:
1. Interior quoted word before comma: "say "yes", and go" was
misidentified as structural close because "," followed the quote.
2. JSON-shaped content in strings: {"key": "val"} inside a string
value was being parsed as actual JSON structure.
The new approach:
- Distinguishes keys from values (only values need repair)
- Uses first-valid-candidate scan with deep lookahead
- Verifies that after a candidate close, the continuation is not just
a structural char but a complete valid JSON pattern
- Validates keyword tokens (true/false/null) fully, not just first char
- Checks container closes recursively for valid continuation
Adds comprehensive tests for all reported edge cases plus a complex
combined scenario with nested JSON-like content, quoted words before
commas, and multiple failure modes in one string.
225 lines
7.4 KiB
Go
225 lines
7.4 KiB
Go
package review
|
|
|
|
import (
|
|
"encoding/json"
|
|
"testing"
|
|
)
|
|
|
|
func TestParseResponse_ValidJSON(t *testing.T) {
|
|
input := `{
|
|
"verdict": "APPROVE",
|
|
"summary": "Looks good",
|
|
"findings": [
|
|
{"severity": "NIT", "file": "main.go", "line": 10, "finding": "Consider renaming"}
|
|
],
|
|
"recommendation": "Ship it"
|
|
}`
|
|
|
|
result, err := ParseResponse(input)
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if result.Verdict != "APPROVE" {
|
|
t.Errorf("expected verdict APPROVE, got %q", result.Verdict)
|
|
}
|
|
if result.Summary != "Looks good" {
|
|
t.Errorf("expected summary %q, got %q", "Looks good", result.Summary)
|
|
}
|
|
if len(result.Findings) != 1 {
|
|
t.Fatalf("expected 1 finding, got %d", len(result.Findings))
|
|
}
|
|
if result.Findings[0].Severity != "NIT" {
|
|
t.Errorf("expected severity NIT, got %q", result.Findings[0].Severity)
|
|
}
|
|
if result.Findings[0].File != "main.go" {
|
|
t.Errorf("expected file main.go, got %q", result.Findings[0].File)
|
|
}
|
|
if result.Findings[0].Line != 10 {
|
|
t.Errorf("expected line 10, got %d", result.Findings[0].Line)
|
|
}
|
|
if result.Recommendation != "Ship it" {
|
|
t.Errorf("expected recommendation %q, got %q", "Ship it", result.Recommendation)
|
|
}
|
|
}
|
|
|
|
func TestParseResponse_MarkdownFences(t *testing.T) {
|
|
input := "```json\n{\"verdict\": \"REQUEST_CHANGES\", \"summary\": \"Issues found\", \"findings\": [{\"severity\": \"MAJOR\", \"file\": \"a.go\", \"line\": 5, \"finding\": \"Bug\"}], \"recommendation\": \"Fix it\"}\n```"
|
|
|
|
result, err := ParseResponse(input)
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if result.Verdict != "REQUEST_CHANGES" {
|
|
t.Errorf("expected verdict REQUEST_CHANGES, got %q", result.Verdict)
|
|
}
|
|
if len(result.Findings) != 1 {
|
|
t.Fatalf("expected 1 finding, got %d", len(result.Findings))
|
|
}
|
|
if result.Findings[0].Severity != "MAJOR" {
|
|
t.Errorf("expected severity MAJOR, got %q", result.Findings[0].Severity)
|
|
}
|
|
}
|
|
|
|
func TestParseResponse_InvalidJSON(t *testing.T) {
|
|
_, err := ParseResponse("this is not json")
|
|
if err == nil {
|
|
t.Fatal("expected error for invalid JSON, got nil")
|
|
}
|
|
}
|
|
|
|
func TestParseResponse_InvalidVerdict(t *testing.T) {
|
|
input := `{"verdict": "MAYBE", "summary": "Hmm", "findings": [], "recommendation": "Dunno"}`
|
|
_, err := ParseResponse(input)
|
|
if err == nil {
|
|
t.Fatal("expected error for invalid verdict, got nil")
|
|
}
|
|
}
|
|
|
|
func TestParseResponse_InvalidSeverity(t *testing.T) {
|
|
input := `{"verdict": "APPROVE", "summary": "Ok", "findings": [{"severity": "CRITICAL", "file": "x.go", "line": 1, "finding": "bad"}], "recommendation": "Fix"}`
|
|
_, err := ParseResponse(input)
|
|
if err == nil {
|
|
t.Fatal("expected error for invalid severity, got nil")
|
|
}
|
|
}
|
|
|
|
func TestParseResponse_EmptyFindings(t *testing.T) {
|
|
input := `{"verdict": "APPROVE", "summary": "All good", "findings": [], "recommendation": "Merge"}`
|
|
result, err := ParseResponse(input)
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if len(result.Findings) != 0 {
|
|
t.Errorf("expected 0 findings, got %d", len(result.Findings))
|
|
}
|
|
}
|
|
|
|
func TestParseResponse_MissingFields(t *testing.T) {
|
|
// verdict is empty string — should fail validation
|
|
input := `{"summary": "Ok", "findings": [], "recommendation": "Merge"}`
|
|
_, err := ParseResponse(input)
|
|
if err == nil {
|
|
t.Fatal("expected error for missing verdict, got nil")
|
|
}
|
|
}
|
|
|
|
func TestParseResponse_MarkdownFencesNoLang(t *testing.T) {
|
|
input := "```\n{\"verdict\": \"APPROVE\", \"summary\": \"Fine\", \"findings\": [], \"recommendation\": \"Good\"}\n```"
|
|
result, err := ParseResponse(input)
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if result.Verdict != "APPROVE" {
|
|
t.Errorf("expected APPROVE, got %q", result.Verdict)
|
|
}
|
|
}
|
|
|
|
func TestParseResponse_UnescapedQuotesInStrings(t *testing.T) {
|
|
// Real failure from CI: Sonnet puts unescaped quotes like (e.g. "28") in findings
|
|
input := `{"verdict": "APPROVE", "summary": "Clean PR", "findings": [{"severity": "NIT", "file": "ci/Dockerfile", "line": 14, "finding": "The comment says OTP_VERSION is the major version (e.g. \"28\") but it actually contains unescaped quotes like (e.g. "28") which breaks JSON"}], "recommendation": "Ship it"}`
|
|
|
|
result, err := ParseResponse(input)
|
|
if err != nil {
|
|
t.Fatalf("expected repair to handle unescaped quotes, got error: %v", err)
|
|
}
|
|
if result.Verdict != "APPROVE" {
|
|
t.Errorf("expected APPROVE, got %q", result.Verdict)
|
|
}
|
|
if len(result.Findings) != 1 {
|
|
t.Fatalf("expected 1 finding, got %d", len(result.Findings))
|
|
}
|
|
}
|
|
|
|
func TestRepairJSON_NoOpOnValid(t *testing.T) {
|
|
valid := `{"key": "value", "num": 42}`
|
|
result := repairJSON(valid)
|
|
if result != valid {
|
|
t.Errorf("repairJSON should not modify valid JSON\n got: %s\n want: %s", result, valid)
|
|
}
|
|
}
|
|
|
|
func TestRepairJSON_FixesUnescapedQuotes(t *testing.T) {
|
|
// Interior quote followed by non-structural character
|
|
input := `{"msg": "use "foo" here"}`
|
|
result := repairJSON(input)
|
|
|
|
// Should be parseable now
|
|
var m map[string]interface{}
|
|
if err := json.Unmarshal([]byte(result), &m); err != nil {
|
|
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
|
}
|
|
}
|
|
|
|
func TestRepairJSON_InteriorQuoteBeforeComma(t *testing.T) {
|
|
// Bug reported by reviewer: interior quoted word immediately before a comma
|
|
input := `{"msg": "say "yes", and go"}`
|
|
result := repairJSON(input)
|
|
|
|
var m map[string]interface{}
|
|
if err := json.Unmarshal([]byte(result), &m); err != nil {
|
|
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
|
}
|
|
// The full string content should be preserved
|
|
msg, ok := m["msg"].(string)
|
|
if !ok {
|
|
t.Fatal("msg field missing or not a string")
|
|
}
|
|
if msg != `say "yes", and go` {
|
|
t.Errorf("unexpected msg content: %q", msg)
|
|
}
|
|
}
|
|
|
|
func TestRepairJSON_InteriorQuoteBeforeCloseBrace(t *testing.T) {
|
|
// Bug reported by reviewer: JSON-shaped syntax inside string values
|
|
input := `{"msg": "input map {"key": "val"} caused error"}`
|
|
result := repairJSON(input)
|
|
|
|
var m map[string]interface{}
|
|
if err := json.Unmarshal([]byte(result), &m); err != nil {
|
|
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
|
}
|
|
}
|
|
|
|
func TestRepairJSON_MultipleFields(t *testing.T) {
|
|
// Multiple string fields with unescaped quotes in different positions
|
|
input := `{"a": "hello "world"", "b": "foo"}`
|
|
result := repairJSON(input)
|
|
|
|
var m map[string]interface{}
|
|
if err := json.Unmarshal([]byte(result), &m); err != nil {
|
|
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
|
}
|
|
if _, ok := m["b"]; !ok {
|
|
t.Error("expected 'b' field to be preserved")
|
|
}
|
|
}
|
|
|
|
func TestRepairJSON_PreservesEscapedQuotes(t *testing.T) {
|
|
// Already-escaped quotes should not be double-escaped
|
|
input := `{"msg": "already \"escaped\" here"}`
|
|
result := repairJSON(input)
|
|
|
|
if result != input {
|
|
t.Errorf("repairJSON should not modify already-escaped quotes\n got: %s\n want: %s", result, input)
|
|
}
|
|
|
|
var m map[string]interface{}
|
|
if err := json.Unmarshal([]byte(result), &m); err != nil {
|
|
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
|
}
|
|
}
|
|
|
|
func TestRepairJSON_ComplexNestedContent(t *testing.T) {
|
|
// Combines both reviewer bugs: quoted words before commas AND JSON-like content
|
|
input := `{"verdict": "APPROVE", "findings": [{"finding": "The map {"key": "val"} and (e.g. "28") and say "yes", then stop"}]}`
|
|
result := repairJSON(input)
|
|
|
|
var parsed map[string]interface{}
|
|
if err := json.Unmarshal([]byte(result), &parsed); err != nil {
|
|
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
|
}
|
|
if parsed["verdict"] != "APPROVE" {
|
|
t.Errorf("expected verdict APPROVE, got %v", parsed["verdict"])
|
|
}
|
|
}
|