fix(review): address review feedback on persona YAML handling
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 30s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m5s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m8s

- 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:
claw
2026-05-12 13:38:48 -07:00
parent 8991260333
commit 45d009dd06
2 changed files with 43 additions and 6 deletions
+10 -5
View File
@@ -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.
}
+33 -1
View File
@@ -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")