CONVENTIONS.md

@@ -21,6 +21,8 @@ To request a new dependency:
 2. Requires explicit approval from Aaron
 3. After merge, a separate PR may use the package
 
+<!-- Deviation from step 1+3 for go-yaml migration: see #91 for rationale. -->
+
 *Enforcement: `scripts/check-deps.sh` parses this table — update only here.*
 
 ## Error Handling

review/persona.go

@@ -199,6 +199,11 @@ func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
 
 	// Second pass: decode with strict field checking enabled.
 	// Strict() rejects unknown keys, catching typos like "focuss" or "identiy".
+	//
+	// Safety note: goccy/go-yaml's decoder does not expand YAML aliases
+	// recursively — it resolves them via the pre-built AST, which our first
+	// pass already depth-checked. Alias chains that would exceed depth limits
+	// are caught above; the decoder merely reads the resolved scalar values.
 	dec := yaml.NewDecoder(bytes.NewReader(data), yaml.Strict())
 	return dec.Decode(out)
 }
@@ -246,6 +251,10 @@ func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[
 	// 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.
+	//
+	// 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
+	// receivers (*MappingNode, *SequenceNode, etc.), never value types.
 	if prevDepth, ok := validated[node]; ok && depth <= prevDepth {
 		return nil
 	}
@@ -288,7 +297,9 @@ func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[
 		// level ensures that deeply nested anchors are caught at definition
 		// time rather than only when referenced via alias. This +1 is
 		// asymmetric with alias (which also increments) — by design, the
-		// combined budget is halved for anchored content that is later aliased.
+		// effective depth budget for anchored-then-aliased content is reduced
+		// because both the definition site and the reference site each consume
+		// a level, making deeply nested anchor/alias pairs hit the limit sooner.
 		if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
 			return err
 		}

review/persona_test.go

@@ -463,9 +463,10 @@ func TestYAMLDeeplyNestedRejection(t *testing.T) {
 	//   - Document root parsed at depth 0
 	//   - Root MappingNode children (MappingValueNodes) visited at depth 1
 	//   - "nested" MappingValueNode: key at depth 2, value at depth 2
-	//   - Each levelN mapping adds +1 depth (MappingNode → MappingValueNode → value)
+	//   - Each levelN adds depth via MappingValueNode traversal (key + value)
-	//   - After 25 levels: effective depth reaches ~27, well past MaxYAMLDepth (20)
+	//   - Exact depth per level depends on AST structure (MappingNode wrapping),
-	// The test uses 25 levels to provide a comfortable margin above the limit.
+	//     but 25 levels reliably exceeds MaxYAMLDepth (20) with comfortable margin.
+	// The test uses 25 levels rather than exactly 21 to avoid brittleness.
 	var sb strings.Builder
 	sb.WriteString("name: test\nidentity: test\nnested:\n")
 	indent := "  "