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.
This commit is contained in:
+10
-5
@@ -171,16 +171,18 @@ func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
|
|||||||
return err
|
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
|
// Reject multi-document YAML files - silently ignoring additional documents
|
||||||
// could lead to confusing behavior where users think their changes take effect.
|
// could lead to confusing behavior where users think their changes take effect.
|
||||||
if len(file.Docs) > 1 {
|
if len(file.Docs) > 1 {
|
||||||
return fmt.Errorf("multi-document YAML is not supported; only single-document files are allowed")
|
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
|
nodeCount := 0
|
||||||
if err := checkYAMLDepth(file.Docs[0].Body, 0, maxDepth, MaxYAMLNodes, make(map[ast.Node]struct{}), &nodeCount); err != nil {
|
if err := checkYAMLDepth(file.Docs[0].Body, 0, maxDepth, MaxYAMLNodes, make(map[ast.Node]struct{}), &nodeCount); err != nil {
|
||||||
return err
|
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)
|
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 {
|
if _, ok := seen[node]; ok {
|
||||||
return nil // Already validated this subtree, skip to avoid infinite recursion.
|
return nil // Already validated this subtree, skip to avoid infinite recursion.
|
||||||
}
|
}
|
||||||
|
|||||||
+33
-1
@@ -459,7 +459,8 @@ func TestYAMLDeeplyNestedRejection(t *testing.T) {
|
|||||||
path := filepath.Join(dir, "deeply-nested.yaml")
|
path := filepath.Join(dir, "deeply-nested.yaml")
|
||||||
|
|
||||||
// Build a deeply nested YAML structure that exceeds MaxYAMLDepth (20).
|
// 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
|
var sb strings.Builder
|
||||||
sb.WriteString("name: test\nidentity: test\nnested:\n")
|
sb.WriteString("name: test\nidentity: test\nnested:\n")
|
||||||
indent := " "
|
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) {
|
func TestYAMLFileSizeLimit(t *testing.T) {
|
||||||
dir := t.TempDir()
|
dir := t.TempDir()
|
||||||
path := filepath.Join(dir, "huge.yaml")
|
path := filepath.Join(dir, "huge.yaml")
|
||||||
|
|||||||
Reference in New Issue
Block a user