diff --git a/CONVENTIONS.md b/CONVENTIONS.md index 089ef1f..a460d70 100644 --- a/CONVENTIONS.md +++ b/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 + + *Enforcement: `scripts/check-deps.sh` parses this table — update only here.* ## Error Handling diff --git a/review/persona.go b/review/persona.go index 9a42884..3a041f9 100644 --- a/review/persona.go +++ b/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 } diff --git a/review/persona_test.go b/review/persona_test.go index c928d74..5a39c2e 100644 --- a/review/persona_test.go +++ b/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) - // - After 25 levels: effective depth reaches ~27, well past MaxYAMLDepth (20) - // The test uses 25 levels to provide a comfortable margin above the limit. + // - Each levelN adds depth via MappingValueNode traversal (key + value) + // - Exact depth per level depends on AST structure (MappingNode wrapping), + // 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 := " "