[MINOR] The two-pass decode approach (first into yaml.Node for depth checking, then a fresh decoder with KnownFields) re-parses the raw bytes a second time. This means a malicious YAML that causes the first-pass decoder to consume significant CPU (e.g., extremely long scalar strings) gets two chances to do so. The depth/node count limits reduce but don't fully eliminate this risk. A minor concern since file size is bounded at 64KB, but worth noting.
[NIT] In ListBuiltinPersonas, the map population pattern is slightly redundant: if !seen[personaName] { seen[personaName] = true }. Since we only ever set it to true, the guard !seen[personaName] is equivalent to just seen[personaName] = true (maps return zero-value false for missing keys, and overwriting with true is idempotent). The condition adds no value. Could simplify to seen[personaName] = true.
[MINOR] The two-pass decode approach (first into yaml.Node for depth check, then strict decode from raw bytes) is necessary because KnownFields doesn't work on yaml.Node.Decode(), but this means the input bytes are parsed twice. The comment explains this correctly. A minor concern: if gopkg.in/yaml.v3 ever adds KnownFields support to node.Decode(), this could be simplified — but for now this is the correct workaround and is well-documented.
[NIT] The comment on LoadBuiltinPersona says 'Try YAML first (preferred format)' but then falls back to JSON. Since the embed directive is now //go:embed personas/*.yaml only (JSON files were deleted), the JSON fallback in LoadBuiltinPersona is dead code for built-in personas. It's harmless and provides forwards-compatibility for the unlikely case someone adds a .json builtin later, but could be simplified.
[NIT] The design doc's error cases table says 'Deeply nested YAML
[NIT] In ListBuiltinPersonas, the seen map is map[string]bool and the code checks if !seen[personaName] before setting it to true. The !seen[...] check is redundant since setting it to true again would be a no-op. The code works correctly but the guard condition (if !seen[personaName]) adds noise — it could just be seen[personaName] = true unconditionally.
[MINOR] The alias depth check uses return checkYAMLDepth(node.Alias, depth, maxDepth) — i.e., the alias itself doesn't add to the depth. This is intentional per the comment, but YAML aliases can be used to create large fan-out structures. An alias to a deeply nested structure effectively doubles the depth impact without incrementing depth for the alias node itself. For the persona use case with MaxYAMLDepth=20 this is acceptable, but worth noting the design tradeoff.
[NIT] The design doc's Error Cases table says 'Deeply nested YAML
[MINOR] The unmarshalYAMLWithDepthLimit function silently ignores multi-document YAML files (only parses the first document). While the comment acknowledges this, it could be a footgun: a user who accidentally writes --- in their persona file will get no error, just silent truncation. Consider calling dec.Decode a second time and returning an error if a second document is found, to give users a clearer signal.
[MINOR] The alias-following logic in checkYAMLDepth does not count the alias node's own depth level before recursing into the alias target. If an alias node appears near the depth limit, the target's content is checked starting from the same depth value as the alias itself (not depth+1). This means a deeply aliased structure could potentially bypass the limit by 1 level. While the impact is minimal given the depth=20 limit, incrementing depth before recursing into the alias target would be more consistent.
[NIT] In ListBuiltinPersonas, if !seen[personaName] { seen[personaName] = true } can be simplified to seen[personaName] = true — the guard is unnecessary since setting an already-true bool to true is a no-op.
[NIT] In LoadBuiltinPersona, the JSON fallback path reassigns data, err = embeddedPersonas.ReadFile(...) but the original err from the YAML attempt is already discarded by the if err == nil { return ... } pattern. This is fine and correct, but worth noting that the error variable from the YAML attempt is silently dropped. Since the YAML error is expected (file doesn't exist), this is intentional and acceptable.
[MINOR] The design document's parseYAML code snippet in the 'YAML Parsing with Depth Protection' section shows a naive yaml.Unmarshal without depth checking, but the actual implementation correctly uses the two-pass Node approach. The doc snippet is stale/inconsistent with the implementation. The paragraph below it explains the correct approach, but the code block above it is misleading. This is documentation-only and doesn't affect behavior, but could confuse future contributors.
[MINOR] The unmarshalYAMLWithDepthLimit function uses interface{} as the parameter type for out instead of any. The project targets the latest stable Go release, and any is the idiomatic alias since Go 1.18. This is inconsistent with the rest of the codebase convention.
[NIT] In ListBuiltinPersonas, the map update if !seen[personaName] { seen[personaName] = true } can be simplified to just seen[personaName] = true — setting to true when already true is a no-op. The guard is unnecessary.
[MINOR] The decoder created with yaml.NewDecoder is used to decode a single document but does not check for a second call to Decode returning io.EOF to confirm there's only one document. For untrusted user-supplied YAML files, silently ignoring additional documents is acceptable (they won't affect the parsed struct), but it's worth noting that multi-document YAML files will have their additional documents silently ignored rather than surfacing an error. This is a minor UX issue, not a correctness bug.
[MINOR] The design document says to use github.com/goccy/go-yaml but the implementation uses gopkg.in/yaml.v3. The design document is now inaccurate/misleading. If the decision was made to use gopkg.in/yaml.v3 instead, the design document should be updated to reflect this, and the security claim about depth protection should be addressed explicitly.