From 45d009dd0682fbf5b4fe97300059afe6426c883d Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Tue, 12 May 2026 13:38:48 -0700
Subject: [PATCH] fix(review): address review feedback on persona YAML handling

- Reorder empty doc check before multi-doc check for natural flow
- Detect nil-body docs (whitespace-only, comment-only input)
- Add explanatory comment on pointer identity for cycle detection map
- Improve depth-counting test comment with AST walker specifics
- Add TestYAMLEmptyFileRejection covering empty/whitespace/comment inputs

Addresses MINOR and NIT findings from sonnet, gpt, and security reviews.
MAJOR (allowlist violation) tracked in issue #91.
---
 review/persona.go      | 15 ++++++++++-----
 review/persona_test.go | 34 +++++++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/review/persona.go b/review/persona.go
index 4c263b0..57dbd0c 100644
--- a/review/persona.go
+++ b/review/persona.go
@@ -171,16 +171,18 @@ func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
 		return err
 	}
 
+	// Reject empty YAML input (whitespace-only, comment-only, or truly empty files).
+	// The parser returns a single doc with nil body for these cases.
+	if len(file.Docs) == 0 || file.Docs[0].Body == nil {
+		return fmt.Errorf("empty YAML document")
+	}
+
 	// Reject multi-document YAML files - silently ignoring additional documents
 	// could lead to confusing behavior where users think their changes take effect.
 	if len(file.Docs) > 1 {
 		return fmt.Errorf("multi-document YAML is not supported; only single-document files are allowed")
 	}
 
-	if len(file.Docs) == 0 {
-		return fmt.Errorf("empty YAML document")
-	}
-
 	nodeCount := 0
 	if err := checkYAMLDepth(file.Docs[0].Body, 0, maxDepth, MaxYAMLNodes, make(map[ast.Node]struct{}), &nodeCount); err != nil {
 		return err
@@ -210,7 +212,10 @@ func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, seen map[ast.N
 		return fmt.Errorf("YAML node count exceeds maximum (%d)", maxNodes)
 	}
 
-	// Cycle detection: if we've seen this node before, we're in a cycle.
+	// Cycle detection: uses pointer identity (ast.Node is an interface, but all
+	// concrete node types are pointers) to detect revisits. This intentionally
+	// compares pointer identity, not structural equality, since we want to track
+	// specific node instances in the parsed AST graph.
 	if _, ok := seen[node]; ok {
 		return nil // Already validated this subtree, skip to avoid infinite recursion.
 	}
diff --git a/review/persona_test.go b/review/persona_test.go
index e7e66ca..69be577 100644
--- a/review/persona_test.go
+++ b/review/persona_test.go
@@ -459,7 +459,8 @@ func TestYAMLDeeplyNestedRejection(t *testing.T) {
 	path := filepath.Join(dir, "deeply-nested.yaml")
 
 	// Build a deeply nested YAML structure that exceeds MaxYAMLDepth (20).
-	// Each level adds to the depth count via mapping values.
+	// Each nested mapping key generates a MappingValueNode, incrementing depth
+	// by 1 per level in the AST walk. With 25 levels, we exceed MaxYAMLDepth (20).
 	var sb strings.Builder
 	sb.WriteString("name: test\nidentity: test\nnested:\n")
 	indent := "  "
@@ -483,6 +484,37 @@ func TestYAMLDeeplyNestedRejection(t *testing.T) {
 	}
 }
 
+
+func TestYAMLEmptyFileRejection(t *testing.T) {
+	dir := t.TempDir()
+
+	tests := []struct {
+		name    string
+		content string
+	}{
+		{"completely empty", ""},
+		{"whitespace only", "   \n\n  "},
+		{"comment only", "# just a comment\n"},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			path := filepath.Join(dir, tc.name+".yaml")
+			if err := os.WriteFile(path, []byte(tc.content), 0644); err != nil {
+				t.Fatalf("failed to write test file: %v", err)
+			}
+
+			_, err := LoadPersona(path)
+			if err == nil {
+				t.Error("expected error for empty YAML input, got nil")
+			}
+			if err != nil && !strings.Contains(err.Error(), "empty YAML document") {
+				t.Errorf("expected error containing %q, got: %v", "empty YAML document", err)
+			}
+		})
+	}
+}
+
 func TestYAMLFileSizeLimit(t *testing.T) {
 	dir := t.TempDir()
 	path := filepath.Join(dir, "huge.yaml")