From b9b7be3b4e67bd3cd91addbc41171fe6df731e22 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Tue, 12 May 2026 19:06:52 -0700
Subject: [PATCH] fix: address review #2888 findings (comment clarity, test
 cleanup)

- Clarify depth-aware short-circuit comment to unambiguously describe
  the relationship between current depth and previous validation depth
- Add comment to MappingValueNode case explaining intentional depth+2
  behavior from parent MappingNode perspective
- Restructure unmarshalYAMLWithDepthLimit doc comment as bullet list
  covering all three safety checks (depth, multi-doc, strict fields)
- Replace t.Error with t.Fatal in TestYAMLEmptyFileRejection to remove
  redundant nil guard on subsequent err.Error() call
---
 review/persona.go      | 25 ++++++++++++++++---------
 review/persona_test.go |  4 ++--
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/review/persona.go b/review/persona.go
index 19e822f..7504f66 100644
--- a/review/persona.go
+++ b/review/persona.go
@@ -166,11 +166,10 @@ func parsePersona(data []byte, source string) (*Persona, error) {
 	return &p, nil
 }
 
-// unmarshalYAMLWithDepthLimit unmarshals YAML data with explicit depth limiting
-// and strict field checking. This protects against stack exhaustion from deeply
-// nested structures and catches typos in field names.
-// Multi-document YAML files are rejected to prevent confusing behavior
-// where additional documents are silently ignored.
+// unmarshalYAMLWithDepthLimit unmarshals YAML data with three safety checks:
+//   - Depth limiting: rejects AST trees exceeding maxDepth to prevent stack exhaustion.
+//   - Multi-document rejection: prevents silent data loss from ignored extra documents.
+//   - Strict field checking: rejects unknown YAML keys to catch typos early.
 func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
 	// First pass: parse into AST to check depth limits, node counts, and
 	// multi-document rejection. This prevents stack exhaustion before we
@@ -247,10 +246,13 @@ func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[
 		return fmt.Errorf("YAML node count exceeds maximum (%d)", maxNodes)
 	}
 
-	// Depth-aware short-circuit: only skip re-checking a node if we previously
-	// validated it at the same or deeper effective depth. If this visit is at a
-	// greater depth than before (e.g., alias referenced deeper in the tree),
-	// we must re-traverse to catch depth limit violations.
+	// Depth-aware short-circuit: skip re-validation only when the current visit
+	// depth is the same or shallower than the depth at which this node was
+	// previously validated. A shallower (or equal) current depth means the
+	// prior, deeper validation already covered any subtree depth violations.
+	// If the current depth exceeds the previous validation depth (e.g., an alias
+	// references this node deeper in the tree), we must re-traverse to ensure
+	// the combined effective depth doesn't exceed maxDepth.
 	//
 	// Note: using ast.Node (interface) as map key relies on pointer identity,
 	// which is correct because all goccy/go-yaml AST node types are pointer
@@ -273,6 +275,11 @@ func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[
 			}
 		}
 	case *ast.MappingValueNode:
+		// Both Key and Value are visited at depth+1 relative to this
+		// MappingValueNode. Since MappingNode visits its MappingValueNode
+		// children at depth+1 as well, keys and values end up at depth+2
+		// from the parent MappingNode. This is intentional: it mirrors the
+		// actual nesting structure (mapping → key-value pair → key/value).
 		if err := checkYAMLDepth(n.Key, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
 			return err
 		}
diff --git a/review/persona_test.go b/review/persona_test.go
index 9b50f0f..c23698f 100644
--- a/review/persona_test.go
+++ b/review/persona_test.go
@@ -510,9 +510,9 @@ func TestYAMLEmptyFileRejection(t *testing.T) {
 
 			_, err := LoadPersona(path)
 			if err == nil {
-				t.Error("expected error for empty YAML input, got nil")
+				t.Fatal("expected error for empty YAML input, got nil")
 			}
-			if err != nil && !strings.Contains(err.Error(), "empty YAML document") {
+			if !strings.Contains(err.Error(), "empty YAML document") {
 				t.Errorf("expected error containing %q, got: %v", "empty YAML document", err)
 			}
 		})