Fix Claude review stream parsing and clean verdict findings

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)

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

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\"") {

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)
+			}
+		})
 	}
 }