diff --git a/cmd/entire/cli/agent/claudecode/reviewer.go b/cmd/entire/cli/agent/claudecode/reviewer.go index bc80457fd4..d24f1f9364 100644 --- a/cmd/entire/cli/agent/claudecode/reviewer.go +++ b/cmd/entire/cli/agent/claudecode/reviewer.go @@ -113,7 +113,12 @@ func parseClaudeOutputBuf(r io.Reader, maxBuf int) <-chan reviewtypes.Event { } switch env.Type { case envelopeTypeAssistant: - for _, block := range env.Message.Content { + var message claudeMessage + if err := json.Unmarshal(env.Message, &message); err != nil { + out <- reviewtypes.RunError{Err: fmt.Errorf("claude stream-json: %w", err)} + continue + } + for _, block := range message.Content { switch block.Type { case "text": if block.Text != "" { @@ -133,12 +138,12 @@ func parseClaudeOutputBuf(r io.Reader, maxBuf int) <-chan reviewtypes.Event { // (see the parser doc). Emitting the running sum keeps // mid-run values on the cumulative Tokens contract; the // true {In, Out} tally comes from `result` below. - in := env.Message.Usage.InputTokens + - env.Message.Usage.CacheReadInputTokens + - env.Message.Usage.CacheCreationInputTokens - if in > 0 && env.Message.ID != "" { - if _, seen := seenMsgIDs[env.Message.ID]; !seen { - seenMsgIDs[env.Message.ID] = struct{}{} + in := message.Usage.InputTokens + + message.Usage.CacheReadInputTokens + + message.Usage.CacheCreationInputTokens + if in > 0 && message.ID != "" { + if _, seen := seenMsgIDs[message.ID]; !seen { + seenMsgIDs[message.ID] = struct{}{} cumInputTokens += in out <- reviewtypes.Tokens{In: cumInputTokens, Out: 0} } @@ -172,9 +177,10 @@ func parseClaudeOutputBuf(r io.Reader, maxBuf int) <-chan reviewtypes.Event { } type claudeEnvelope struct { - Type string `json:"type"` - Message claudeMessage `json:"message"` - IsError bool `json:"is_error"` + Type string `json:"type"` + // Message has an event-specific shape; only assistant events carry claudeMessage. + Message json.RawMessage `json:"message"` + IsError bool `json:"is_error"` // Usage reuses the package-local messageUsage type (declared in types.go) // rather than a duplicate ad-hoc struct, so the two consumers of the // Claude API usage shape (transcript parsing + stream-json review parser) diff --git a/cmd/entire/cli/agent/claudecode/reviewer_test.go b/cmd/entire/cli/agent/claudecode/reviewer_test.go index 0a44573e71..173e98fbbf 100644 --- a/cmd/entire/cli/agent/claudecode/reviewer_test.go +++ b/cmd/entire/cli/agent/claudecode/reviewer_test.go @@ -390,6 +390,58 @@ func TestParseClaudeOutput_GarbledLineEmitsRunErrorAndContinues(t *testing.T) { } } +func TestParseClaudeOutput_PermissionDeniedEventDoesNotFailRun(t *testing.T) { + t.Parallel() + // Claude Code 2.1.278 emits permission_denied system events with a + // string-valued message. That is a valid envelope, not malformed output, + // so it must not turn an otherwise-successful review into a failed run. + input := `{"type":"system","subtype":"permission_denied","tool_name":"Read","tool_use_id":"toolu_1","message":"Permission denied","uuid":"u1","session_id":"s1"}` + "\n" + + `{"type":"assistant","message":{"id":"msg_1","content":[{"type":"text","text":"finding survived"}]}}` + "\n" + + `{"type":"result","subtype":"success","is_error":false,"usage":{"output_tokens":1}}` + "\n" + events := collectEvents(parseClaudeOutput(strings.NewReader(input))) + + var sawText, sawSuccess bool + for _, ev := range events { + switch event := ev.(type) { + case reviewtypes.RunError: + t.Errorf("valid permission_denied event emitted RunError: %v", event.Err) + case reviewtypes.AssistantText: + sawText = sawText || event.Text == "finding survived" + case reviewtypes.Finished: + sawSuccess = event.Success + } + } + if !sawText { + t.Error("assistant text after permission_denied event was lost") + } + if !sawSuccess { + t.Error("expected Finished{Success:true}") + } +} + +func TestParseClaudeOutput_MalformedAssistantMessageEmitsRunError(t *testing.T) { + t.Parallel() + input := `{"type":"assistant","message":"not an assistant message object"}` + "\n" + + `{"type":"result","subtype":"success","is_error":false,"usage":{"output_tokens":1}}` + "\n" + events := collectEvents(parseClaudeOutput(strings.NewReader(input))) + + var sawRunError, sawSuccess bool + for _, ev := range events { + if _, ok := ev.(reviewtypes.RunError); ok { + sawRunError = true + } + if fin, ok := ev.(reviewtypes.Finished); ok && fin.Success { + sawSuccess = true + } + } + if !sawRunError { + t.Error("expected RunError for malformed assistant message") + } + if !sawSuccess { + t.Error("expected parser to continue through the successful result") + } +} + // TestParseClaudeOutput_EmitsCumulativeInputDuringRun captures the live-token // contract for Claude. The `Tokens` type is documented as cumulative running // totals (each emission replaces the previous), so mid-run emissions must be diff --git a/cmd/entire/cli/review_bridge.go b/cmd/entire/cli/review_bridge.go index 0185d8c545..9ff418f13b 100644 --- a/cmd/entire/cli/review_bridge.go +++ b/cmd/entire/cli/review_bridge.go @@ -16,6 +16,8 @@ import ( "regexp" "strconv" "strings" + "unicode" + "unicode/utf8" "github.com/entireio/cli/cmd/entire/cli/agent" "github.com/entireio/cli/cmd/entire/cli/agent/claudecode" @@ -99,6 +101,12 @@ func reviewTrailFindingInputs(profileName, verdict string) []api.TrailReviewComm } items := splitReviewVerdictFindings(verdict) if len(items) == 0 { + // A clean verdict is review metadata, not a finding. Posting it as a + // severity-less whole-change comment makes the Trail show an + // "Unspecified finding" even though the review found nothing. + if isCleanReviewVerdict(verdict) { + return nil + } // The verdict spans the whole change, so it uses "verdict" kind: // the API requires a valid granularity and rejects an empty value. return []api.TrailReviewCommentInput{reviewTrailFindingInputWithKind(profileName, verdict, "verdict")} @@ -112,6 +120,27 @@ func reviewTrailFindingInputs(profileName, verdict string) []api.TrailReviewComm return inputs } +func isCleanReviewVerdict(verdict string) bool { + line := strings.ToLower(strings.TrimSpace(lastNonEmptyLine(verdict))) + line = strings.TrimSpace(strings.TrimLeft(line, "#>")) + line = strings.TrimLeft(line, "*_`") + rest, ok := strings.CutPrefix(line, "approve") + if !ok { + return false + } + if rest != "" { + first, _ := utf8.DecodeRuneInString(rest) + if !unicode.IsSpace(first) && !unicode.IsPunct(first) && !unicode.IsSymbol(first) { + return false + } + } + + rest = strings.TrimLeftFunc(rest, func(r rune) bool { + return unicode.IsSpace(r) || unicode.IsPunct(r) || unicode.IsSymbol(r) + }) + return !strings.HasPrefix(rest, "with nits") +} + func reviewTrailFindingInputsFromJSON(verdict string) ([]api.TrailReviewCommentInput, bool) { line := lastNonEmptyLine(verdict) if !strings.HasPrefix(line, "{") || !strings.Contains(line, "\"comments\"") { diff --git a/cmd/entire/cli/review_bridge_test.go b/cmd/entire/cli/review_bridge_test.go index c70dd53be5..0f4a07ac92 100644 --- a/cmd/entire/cli/review_bridge_test.go +++ b/cmd/entire/cli/review_bridge_test.go @@ -139,13 +139,47 @@ func TestReviewTrailFindingInputsAcceptsRunnerStyleJSONLastLine(t *testing.T) { } } -func TestReviewTrailFindingInputsSingleVerdictUnchanged(t *testing.T) { - inputs := reviewTrailFindingInputs("general", "APPROVE - no actionable findings.") - if len(inputs) != 1 { - t.Fatalf("inputs = %d, want 1", len(inputs)) +func TestReviewTrailFindingInputsCleanVerdictProducesNoFindings(t *testing.T) { + t.Parallel() + + tests := map[string]string{ + "single verdict": "APPROVE - no actionable findings.", + "agent fallback": "## codex\n\nI checked the scoped diff and all call sites.\napprove — no actionable defects found.", + "period separator": "approve. no actionable findings.", + "plain-language reason": "approve with no actionable findings", + "bold verdict": "**approve** — no actionable findings.", + "punctuation boundary": "approve! looks good.", + } + for name, verdict := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + if inputs := reviewTrailFindingInputs("general", verdict); len(inputs) != 0 { + t.Fatalf("inputs = %d, want 0 for a clean review", len(inputs)) + } + }) } - if inputs[0].Body == nil || !strings.Contains(*inputs[0].Body, "Review verdict (profile: general)") { - t.Fatalf("single body = %v, want verdict/profile header", inputs[0].Body) +} + +func TestReviewTrailFindingInputsPreservesUnstructuredNonCleanVerdict(t *testing.T) { + t.Parallel() + + tests := map[string]string{ + "request changes": "REQUEST CHANGES - missing input validation.", + "approve with nits": "APPROVE WITH NITS - rename the confusing variable.", + "punctuated approve nits": "APPROVE: WITH NITS - rename the confusing variable.", + "non-verdict approve word": "APPROVED pending another review.", + } + for name, verdict := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + inputs := reviewTrailFindingInputs("general", verdict) + if len(inputs) != 1 { + t.Fatalf("inputs = %d, want 1", len(inputs)) + } + if inputs[0].Body == nil || !strings.Contains(*inputs[0].Body, "Review verdict (profile: general)") { + t.Fatalf("single body = %v, want verdict/profile header", inputs[0].Body) + } + }) } }