Compare commits
5 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| db479d0ff4 | |||
| cabbb5a55a | |||
| 55cf3fd4b9 | |||
| f48288bf2e | |||
| b4c994d0fa |
+24
-2
@@ -28,12 +28,33 @@ jobs:
|
||||
include:
|
||||
- name: sonnet
|
||||
token_secret: SONNET_REVIEW_TOKEN
|
||||
model: gpt-5
|
||||
provider: anthropic
|
||||
llm_path: /anthropic/v1
|
||||
model: claude-sonnet-4-6
|
||||
- name: gpt
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-5
|
||||
- name: gpt41
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-4.1
|
||||
- name: gpt5-mini
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-5-mini
|
||||
- name: gpt41-mini
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-4.1-mini
|
||||
- name: security
|
||||
token_secret: SECURITY_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-5
|
||||
system_prompt_file: SECURITY_REVIEW.md
|
||||
steps:
|
||||
@@ -49,9 +70,10 @@ jobs:
|
||||
PR_NUMBER: ${{ github.event.pull_request.number }}
|
||||
REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
|
||||
REVIEWER_NAME: ${{ matrix.name }}
|
||||
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}
|
||||
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}${{ matrix.llm_path }}
|
||||
LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
|
||||
LLM_MODEL: ${{ matrix.model }}
|
||||
LLM_PROVIDER: ${{ matrix.provider }}
|
||||
CONVENTIONS_FILE: "CONVENTIONS.md"
|
||||
PATTERNS_REPO: "rodin/go-patterns"
|
||||
PATTERNS_FILES: "README.md,patterns/"
|
||||
|
||||
+28
-12
@@ -254,25 +254,41 @@ func main() {
|
||||
slog.Warn("context trimmed to fit budget", "trimmed", budgetResult.Trimmed)
|
||||
}
|
||||
|
||||
// Step 8: Call LLM
|
||||
// Step 8: Call LLM (with retry on parse failure)
|
||||
slog.Info("sending request to LLM", "model", *llmModel)
|
||||
messages := []llm.Message{
|
||||
{Role: "system", Content: budgetResult.SystemPrompt},
|
||||
{Role: "user", Content: budgetResult.UserPrompt},
|
||||
}
|
||||
|
||||
response, err := llmClient.Complete(ctx, messages)
|
||||
if err != nil {
|
||||
slog.Error("LLM request failed", "model", *llmModel, "error", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
slog.Info("LLM response received", "bytes", len(response))
|
||||
var response string
|
||||
var result *review.ReviewResult
|
||||
for attempt := 1; attempt <= 2; attempt++ {
|
||||
if attempt > 1 {
|
||||
slog.Warn("retrying LLM request after parse failure", "attempt", attempt)
|
||||
time.Sleep(time.Second)
|
||||
}
|
||||
|
||||
// Step 9: Parse response
|
||||
result, err := review.ParseResponse(response)
|
||||
if err != nil {
|
||||
slog.Error("failed to parse LLM response", "error", err)
|
||||
os.Exit(1)
|
||||
response, err = llmClient.Complete(ctx, messages)
|
||||
if err != nil {
|
||||
slog.Error("LLM request failed", "model", *llmModel, "error", err, "attempt", attempt)
|
||||
if attempt == 2 {
|
||||
os.Exit(1)
|
||||
}
|
||||
continue
|
||||
}
|
||||
slog.Info("LLM response received", "bytes", len(response), "attempt", attempt)
|
||||
|
||||
// Step 9: Parse response
|
||||
result, err = review.ParseResponse(response)
|
||||
if err != nil {
|
||||
slog.Error("failed to parse LLM response", "error", err, "attempt", attempt)
|
||||
if attempt == 2 {
|
||||
os.Exit(1)
|
||||
}
|
||||
continue
|
||||
}
|
||||
break
|
||||
}
|
||||
slog.Info("review parsed", "verdict", result.Verdict, "findings", len(result.Findings))
|
||||
|
||||
|
||||
+51
-5
@@ -75,12 +75,52 @@ type Message struct {
|
||||
// Complete sends a chat completion request and returns the assistant's response content.
|
||||
// The first message with role "system" is treated as the system prompt.
|
||||
func (c *Client) Complete(ctx context.Context, messages []Message) (string, error) {
|
||||
switch c.provider {
|
||||
case ProviderAnthropic:
|
||||
return c.completeAnthropic(ctx, messages)
|
||||
default:
|
||||
return c.completeOpenAI(ctx, messages)
|
||||
var result string
|
||||
var err error
|
||||
|
||||
for attempt := 0; attempt < 2; attempt++ {
|
||||
switch c.provider {
|
||||
case ProviderAnthropic:
|
||||
result, err = c.completeAnthropic(ctx, messages)
|
||||
default:
|
||||
result, err = c.completeOpenAI(ctx, messages)
|
||||
}
|
||||
|
||||
if err == nil {
|
||||
return result, nil
|
||||
}
|
||||
|
||||
// Only retry on response body read errors (transient network issues).
|
||||
// Do not retry on context cancellation, status errors, or parse errors
|
||||
// that indicate a structural API problem.
|
||||
if !isRetryableError(err) {
|
||||
return "", err
|
||||
}
|
||||
|
||||
if attempt == 0 && ctx.Err() == nil {
|
||||
// Brief pause before retry to allow transient issues to resolve.
|
||||
time.Sleep(500 * time.Millisecond)
|
||||
}
|
||||
}
|
||||
|
||||
return "", err
|
||||
}
|
||||
|
||||
// isRetryableError returns true for transient errors worth retrying.
|
||||
func isRetryableError(err error) bool {
|
||||
if err == nil {
|
||||
return false
|
||||
}
|
||||
s := err.Error()
|
||||
// Body read failures (connection reset, truncation)
|
||||
if strings.Contains(s, "read response") {
|
||||
return true
|
||||
}
|
||||
// Unexpected body length (our content-length validation)
|
||||
if strings.Contains(s, "body length mismatch") {
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// --- OpenAI-compatible implementation ---
|
||||
@@ -231,6 +271,12 @@ func (c *Client) doRequest(req *http.Request, parse func([]byte) (string, error)
|
||||
return "", fmt.Errorf("read response: %w", err)
|
||||
}
|
||||
|
||||
// Validate body length against Content-Length header when present.
|
||||
// A mismatch indicates the response was truncated in transit.
|
||||
if cl := resp.ContentLength; cl > 0 && int64(len(body)) < cl {
|
||||
return "", fmt.Errorf("body length mismatch: Content-Length=%d, received=%d", cl, len(body))
|
||||
}
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
return "", fmt.Errorf("LLM API error (status %d): %s", resp.StatusCode, string(body))
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@ package llm
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
@@ -295,3 +296,131 @@ func TestWithProvider(t *testing.T) {
|
||||
t.Errorf("expected provider anthropic, got %s", client.provider)
|
||||
}
|
||||
}
|
||||
|
||||
func TestComplete_RetryOnBodyReadError(t *testing.T) {
|
||||
attempts := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
attempts++
|
||||
if attempts == 1 {
|
||||
// First attempt: send headers then close connection abruptly
|
||||
// Simulate by writing partial response and flushing with wrong Content-Length
|
||||
w.Header().Set("Content-Length", "1000")
|
||||
w.WriteHeader(http.StatusOK)
|
||||
w.Write([]byte(`{"choices":[{"message":{"con`))
|
||||
// The test HTTP server will close the connection after handler returns,
|
||||
// but Content-Length mismatch means client gets fewer bytes than expected
|
||||
return
|
||||
}
|
||||
// Second attempt: succeed
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
json.NewEncoder(w).Encode(ChatResponse{
|
||||
Choices: []struct {
|
||||
Message struct {
|
||||
Content string `json:"content"`
|
||||
} `json:"message"`
|
||||
}{{Message: struct {
|
||||
Content string `json:"content"`
|
||||
}{Content: "success"}}},
|
||||
})
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "key", "model")
|
||||
got, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
|
||||
if err != nil {
|
||||
t.Fatalf("expected retry to succeed, got error: %v", err)
|
||||
}
|
||||
if got != "success" {
|
||||
t.Errorf("expected %q, got %q", "success", got)
|
||||
}
|
||||
if attempts != 2 {
|
||||
t.Errorf("expected 2 attempts, got %d", attempts)
|
||||
}
|
||||
}
|
||||
|
||||
func TestComplete_ContentLengthMismatch(t *testing.T) {
|
||||
attempts := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
attempts++
|
||||
if attempts == 1 {
|
||||
// Claim Content-Length is larger than actual body
|
||||
w.Header().Set("Content-Length", "500")
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(http.StatusOK)
|
||||
// Write less than 500 bytes
|
||||
w.Write([]byte(`{"choices":[{"message":{"content":"partial"}}]}`))
|
||||
return
|
||||
}
|
||||
// Second attempt succeeds
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
json.NewEncoder(w).Encode(ChatResponse{
|
||||
Choices: []struct {
|
||||
Message struct {
|
||||
Content string `json:"content"`
|
||||
} `json:"message"`
|
||||
}{{Message: struct {
|
||||
Content string `json:"content"`
|
||||
}{Content: "complete"}}},
|
||||
})
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "key", "model")
|
||||
got, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
|
||||
if err != nil {
|
||||
t.Fatalf("expected retry to succeed on content-length mismatch, got: %v", err)
|
||||
}
|
||||
if got != "complete" {
|
||||
t.Errorf("expected %q, got %q", "complete", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestComplete_NoRetryOnAPIError(t *testing.T) {
|
||||
attempts := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
attempts++
|
||||
w.WriteHeader(http.StatusBadRequest)
|
||||
w.Write([]byte(`{"error":"bad request"}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "key", "model")
|
||||
_, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 400, got nil")
|
||||
}
|
||||
if attempts != 1 {
|
||||
t.Errorf("should not retry on API errors, got %d attempts", attempts)
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsRetryableError(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
err string
|
||||
expected bool
|
||||
}{
|
||||
{"nil formatted", "", false},
|
||||
{"read response error", "read response: unexpected EOF", true},
|
||||
{"body length mismatch", "body length mismatch: Content-Length=1000, received=500", true},
|
||||
{"API error", "LLM API error (status 400): bad request", false},
|
||||
{"parse error", "parse response: unexpected end of JSON input", false},
|
||||
{"request error", "LLM request: connection refused", false},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
if tt.err == "" {
|
||||
if isRetryableError(nil) {
|
||||
t.Error("nil error should not be retryable")
|
||||
}
|
||||
return
|
||||
}
|
||||
err := fmt.Errorf("%s", tt.err)
|
||||
got := isRetryableError(err)
|
||||
if got != tt.expected {
|
||||
t.Errorf("isRetryableError(%q) = %v, want %v", tt.err, got, tt.expected)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
+240
-1
@@ -29,7 +29,19 @@ func ParseResponse(response string) (*ReviewResult, error) {
|
||||
|
||||
var result ReviewResult
|
||||
if err := json.Unmarshal([]byte(cleaned), &result); err != nil {
|
||||
return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw response: %s", err, response)
|
||||
// LLMs sometimes produce JSON with unescaped quotes inside string values.
|
||||
// Try to repair before giving up.
|
||||
repaired := repairJSON(cleaned)
|
||||
if err2 := json.Unmarshal([]byte(repaired), &result); err2 != nil {
|
||||
// Include diagnostic info: lengths help identify truncation
|
||||
rawLen := len(response)
|
||||
cleanedLen := len(cleaned)
|
||||
preview := cleaned
|
||||
if len(preview) > 200 {
|
||||
preview = preview[:100] + "..." + preview[len(preview)-100:]
|
||||
}
|
||||
return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw length: %d, cleaned length: %d\nCleaned preview: %s", err, rawLen, cleanedLen, preview)
|
||||
}
|
||||
}
|
||||
|
||||
// Validate verdict
|
||||
@@ -74,3 +86,230 @@ func extractJSON(s string) string {
|
||||
s = strings.TrimSpace(s)
|
||||
return s
|
||||
}
|
||||
|
||||
// repairJSON attempts to fix common LLM JSON issues:
|
||||
// - Unescaped double quotes inside string values
|
||||
//
|
||||
// Strategy: walk the JSON structurally. Object keys are parsed normally (LLMs
|
||||
// get those right). For string VALUES, we find all candidate closing quotes and
|
||||
// pick the LAST one that leaves valid JSON structure afterward — maximizing
|
||||
// string content, which is the correct bias for the "LLM put unescaped quotes
|
||||
// in a string value" failure mode.
|
||||
func repairJSON(s string) string {
|
||||
runes := []rune(s)
|
||||
var out strings.Builder
|
||||
out.Grow(len(s) + 64)
|
||||
|
||||
i := 0
|
||||
for i < len(runes) {
|
||||
c := runes[i]
|
||||
|
||||
if c != '"' {
|
||||
out.WriteRune(c)
|
||||
i++
|
||||
continue
|
||||
}
|
||||
|
||||
// We hit an opening quote. Determine if this is a key or a value.
|
||||
// Keys: the standard JSON parser in LLMs gets keys right, so we parse
|
||||
// them normally (first unescaped quote closes).
|
||||
// Values: may contain unescaped quotes — use the repair heuristic.
|
||||
isValue := isValuePosition(runes, i)
|
||||
|
||||
if !isValue {
|
||||
// Parse key/simple string normally
|
||||
out.WriteRune('"')
|
||||
i++
|
||||
for i < len(runes) {
|
||||
ch := runes[i]
|
||||
if ch == '\\' && i+1 < len(runes) {
|
||||
out.WriteRune(ch)
|
||||
i++
|
||||
out.WriteRune(runes[i])
|
||||
i++
|
||||
continue
|
||||
}
|
||||
if ch == '"' {
|
||||
out.WriteRune('"')
|
||||
i++
|
||||
break
|
||||
}
|
||||
out.WriteRune(ch)
|
||||
i++
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
// Value string — find the correct close using last-valid-candidate heuristic
|
||||
out.WriteRune('"')
|
||||
i++
|
||||
|
||||
closeIdx := findClosingQuote(runes, i)
|
||||
|
||||
// Write everything between open and close, escaping interior quotes
|
||||
for j := i; j < closeIdx; j++ {
|
||||
ch := runes[j]
|
||||
if ch == '\\' && j+1 < closeIdx {
|
||||
// Already-escaped sequence — pass through
|
||||
out.WriteRune(ch)
|
||||
j++
|
||||
out.WriteRune(runes[j])
|
||||
} else if ch == '"' {
|
||||
out.WriteRune('\\')
|
||||
out.WriteRune('"')
|
||||
} else {
|
||||
out.WriteRune(ch)
|
||||
}
|
||||
}
|
||||
|
||||
// Write the closing quote
|
||||
out.WriteRune('"')
|
||||
i = closeIdx + 1
|
||||
}
|
||||
|
||||
return out.String()
|
||||
}
|
||||
|
||||
// isValuePosition determines if the quote at position i is opening a JSON value
|
||||
// string (as opposed to an object key). We only apply repair to values that
|
||||
// follow ':' since those are the free-text fields where LLMs produce unescaped
|
||||
// quotes. Array elements and keys are left alone (parsed normally).
|
||||
func isValuePosition(runes []rune, i int) bool {
|
||||
// Look backward, skipping whitespace, for the preceding structural char
|
||||
j := i - 1
|
||||
for j >= 0 && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
|
||||
j--
|
||||
}
|
||||
if j < 0 {
|
||||
return false
|
||||
}
|
||||
// After ':' → definitely a value
|
||||
return runes[j] == ':'
|
||||
}
|
||||
|
||||
// findClosingQuote finds the index of the true closing quote for a JSON string
|
||||
// value starting at position start (the character after the opening quote).
|
||||
// It collects all unescaped quote candidates and returns the FIRST one that
|
||||
// produces valid JSON continuation (deeper lookahead verifies the next token).
|
||||
func findClosingQuote(runes []rune, start int) int {
|
||||
// Collect all candidate positions for the closing quote.
|
||||
var candidates []int
|
||||
for j := start; j < len(runes); j++ {
|
||||
if runes[j] == '\\' {
|
||||
j++ // skip escaped character
|
||||
continue
|
||||
}
|
||||
if runes[j] == '"' {
|
||||
candidates = append(candidates, j)
|
||||
}
|
||||
}
|
||||
|
||||
if len(candidates) == 0 {
|
||||
return len(runes)
|
||||
}
|
||||
|
||||
if len(candidates) == 1 {
|
||||
return candidates[0]
|
||||
}
|
||||
|
||||
// Try candidates from FIRST to LAST. The correct closing quote is the
|
||||
// earliest one that produces valid JSON structure after it (verified by
|
||||
// deeper lookahead that checks the next token is a valid JSON start).
|
||||
for _, idx := range candidates {
|
||||
if isValidJSONAfterClose(runes, idx+1) {
|
||||
return idx
|
||||
}
|
||||
}
|
||||
|
||||
// Fallback: return the last candidate
|
||||
return candidates[len(candidates)-1]
|
||||
}
|
||||
|
||||
// isValidJSONAfterClose checks whether the runes after a candidate closing quote
|
||||
// look like valid JSON continuation for a VALUE string. Since we only use this
|
||||
// for value positions, ':' is NOT a valid continuation (values are never keys).
|
||||
// Checks deeper structure to avoid being fooled by JSON-like content in strings.
|
||||
func isValidJSONAfterClose(runes []rune, pos int) bool {
|
||||
j := pos
|
||||
for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
|
||||
j++
|
||||
}
|
||||
|
||||
if j >= len(runes) {
|
||||
return true
|
||||
}
|
||||
|
||||
next := runes[j]
|
||||
if next == '}' || next == ']' {
|
||||
// Closing a container. Verify what follows the close is also valid:
|
||||
// another structural char, comma, or EOF.
|
||||
return isValidAfterContainerClose(runes, j+1)
|
||||
}
|
||||
if next == ',' {
|
||||
// After comma, must be followed by a valid JSON token
|
||||
j++
|
||||
for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
|
||||
j++
|
||||
}
|
||||
if j >= len(runes) {
|
||||
return false // trailing comma with nothing after — invalid
|
||||
}
|
||||
return isJSONTokenStart(runes, j)
|
||||
}
|
||||
// ':' is NOT valid here — we're in a value position, not a key.
|
||||
// Any other character is also invalid.
|
||||
return false
|
||||
}
|
||||
|
||||
// isValidAfterContainerClose checks that after a } or ], the continuation is
|
||||
// structurally valid: more closes, comma+token, or EOF.
|
||||
func isValidAfterContainerClose(runes []rune, pos int) bool {
|
||||
j := pos
|
||||
for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
|
||||
j++
|
||||
}
|
||||
if j >= len(runes) {
|
||||
return true
|
||||
}
|
||||
next := runes[j]
|
||||
if next == '}' || next == ']' {
|
||||
return isValidAfterContainerClose(runes, j+1)
|
||||
}
|
||||
if next == ',' {
|
||||
j++
|
||||
for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
|
||||
j++
|
||||
}
|
||||
if j >= len(runes) {
|
||||
return false
|
||||
}
|
||||
return isJSONTokenStart(runes, j)
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// isJSONTokenStart returns true if the rune could begin a JSON value or key.
|
||||
// For keywords (true/false/null), verifies the full keyword is present.
|
||||
func isJSONTokenStart(runes []rune, pos int) bool {
|
||||
if pos >= len(runes) {
|
||||
return false
|
||||
}
|
||||
r := runes[pos]
|
||||
switch {
|
||||
case r == '"': // string
|
||||
return true
|
||||
case r == '{' || r == '[': // object or array
|
||||
return true
|
||||
case r == 't': // true
|
||||
return pos+4 <= len(runes) && string(runes[pos:pos+4]) == "true"
|
||||
case r == 'f': // false
|
||||
return pos+5 <= len(runes) && string(runes[pos:pos+5]) == "false"
|
||||
case r == 'n': // null
|
||||
return pos+4 <= len(runes) && string(runes[pos:pos+4]) == "null"
|
||||
case r >= '0' && r <= '9': // number
|
||||
return true
|
||||
case r == '-': // negative number
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package review
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"testing"
|
||||
)
|
||||
|
||||
@@ -112,3 +113,112 @@ func TestParseResponse_MarkdownFencesNoLang(t *testing.T) {
|
||||
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"])
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user