From 0522cfe3cbd4ce04e40d9aabddf3ac49a3088dc8 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Tue, 12 May 2026 14:42:22 -0700
Subject: [PATCH] address self-review findings on PR #89

MINOR fixes:
- docs/DESIGN-57-yaml-persona.md: fix Error Cases table entry to reflect
  custom AST walk (checkYAMLDepth) instead of stale library-level reference
- review/persona.go: add EOF check after JSON decode to reject trailing
  garbage after a valid JSON object (prevents silent acceptance of malformed
  input like '{"name":"x"}garbage')
- review/persona_test.go: add TestJSONTrailingContentRejected test

NIT fixes:
- review/persona.go: add default case to checkYAMLDepth switch with
  explanatory comment about scalar leaf nodes
- review/persona.go: document AnchorNode depth+1 conservative asymmetry
- review/persona.go: simplify redundant if-guard in ListBuiltinPersonas
---
 docs/DESIGN-57-yaml-persona.md |  2 +-
 review/persona.go              | 26 ++++++++++++++++++-----
 review/persona_test.go         | 38 ++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/docs/DESIGN-57-yaml-persona.md b/docs/DESIGN-57-yaml-persona.md
index 7c2dda5..067a9de 100644
--- a/docs/DESIGN-57-yaml-persona.md
+++ b/docs/DESIGN-57-yaml-persona.md
@@ -53,7 +53,7 @@ No new state. Same `Persona` struct, just different parsing.
 | Error | Handling |
 |-------|----------|
 | Invalid YAML syntax | Return parse error with source file |
-| Deeply nested YAML | Library rejects (v1.16.0+ fix) |
+| Deeply nested YAML | Custom AST walk (`checkYAMLDepth`) rejects before decode |
 | Unknown extension | Fall back to JSON parsing |
 | Missing required fields | Validation rejects after parse |
 
diff --git a/review/persona.go b/review/persona.go
index fb3cb69..c9c71cb 100644
--- a/review/persona.go
+++ b/review/persona.go
@@ -5,6 +5,7 @@ import (
 	"embed"
 	"encoding/json"
 	"fmt"
+	"io"
 	"os"
 	"sort"
 	"strings"
@@ -120,9 +121,7 @@ func ListBuiltinPersonas() []string {
 		default:
 			continue
 		}
-		if !seen[personaName] {
-			seen[personaName] = true
-		}
+		seen[personaName] = true
 	}
 	names := make([]string, 0, len(seen))
 	for name := range seen {
@@ -148,6 +147,15 @@ func parsePersona(data []byte, source string) (*Persona, error) {
 		dec := json.NewDecoder(bytes.NewReader(data))
 		dec.DisallowUnknownFields()
 		err = dec.Decode(&p)
+		if err == nil {
+			// Reject trailing content after the first valid JSON object.
+			// Without this check, input like `{"name":"x"}garbage` would
+			// silently succeed because Decoder stops after one object.
+			var dummy json.RawMessage
+			if dec.More() || dec.Decode(&dummy) != io.EOF {
+				err = fmt.Errorf("unexpected trailing content after JSON object")
+			}
+		}
 	}
 	if err != nil {
 		return nil, fmt.Errorf("parse persona %s: %w", source, err)
@@ -275,6 +283,12 @@ func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[
 			return err
 		}
 	case *ast.AnchorNode:
+		// Increment depth for anchor values as a conservative measure: the
+		// anchor definition itself is structural, and treating it as a depth
+		// 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.
 		if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
 			return err
 		}
@@ -282,8 +296,10 @@ func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[
 		if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
 			return err
 		}
-		// Scalar types (StringNode, IntegerNode, FloatNode, BoolNode, NullNode,
-		// InfinityNode, NanNode, LiteralNode, MergeKeyNode) are leaf nodes.
+	default:
+		// Scalar leaf nodes (StringNode, IntegerNode, FloatNode, BoolNode,
+		// NullNode, InfinityNode, NanNode, LiteralNode, MergeKeyNode) have
+		// no children to recurse into.
 	}
 	return nil
 }
diff --git a/review/persona_test.go b/review/persona_test.go
index 4f93c13..6154831 100644
--- a/review/persona_test.go
+++ b/review/persona_test.go
@@ -858,3 +858,41 @@ identity: test identity
 		t.Errorf("Name = %q, want %q", p.Name, "test")
 	}
 }
+
+func TestJSONTrailingContentRejected(t *testing.T) {
+	tests := []struct {
+		name    string
+		content string
+	}{
+		{
+			name:    "trailing garbage after object",
+			content: `{"name":"test","identity":"test identity"}garbage`,
+		},
+		{
+			name:    "two JSON objects",
+			content: `{"name":"test","identity":"test identity"}{"name":"other"}`,
+		},
+		{
+			name:    "trailing array",
+			content: `{"name":"test","identity":"test identity"}[]`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := t.TempDir()
+			path := filepath.Join(dir, "test.json")
+			if err := os.WriteFile(path, []byte(tt.content), 0644); err != nil {
+				t.Fatalf("failed to write test file: %v", err)
+			}
+
+			_, err := LoadPersona(path)
+			if err == nil {
+				t.Fatal("expected error for trailing content, got nil")
+			}
+			if !strings.Contains(err.Error(), "trailing content") {
+				t.Errorf("error = %q, want to contain 'trailing content'", err.Error())
+			}
+		})
+	}
+}