fix(deps): replace gopkg.in/yaml.v3 with github.com/goccy/go-yaml #89

Merged
aweiker merged 13 commits from review-bot-issue-87 into main 2026-05-13 03:47:02 +00:00
3 changed files with 60 additions and 6 deletions
Showing only changes of commit 5cedeee9f4 - Show all commits
+1 -1
View File
4
@@ -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 |
1
+21 -5
View File
@@ -5,6 +5,7 @@ import (
"embed"
"encoding/json"
"fmt"
"io"
"os"
"sort"
"strings"
2
@@ -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()
Review

[MINOR] JSON decoding disallows unknown fields but does not verify EOF to catch trailing data after a valid JSON object. Consider an additional decode into a dummy value to ensure no extra tokens remain.

**[MINOR]** JSON decoding disallows unknown fields but does not verify EOF to catch trailing data after a valid JSON object. Consider an additional decode into a dummy value to ensure no extra tokens remain.
err = dec.Decode(&p)
if err == nil {
Review

[MINOR] The JSON trailing content check uses dec.More(), which is only meaningful inside arrays/objects and may be confusing at top level. Relying solely on attempting a second Decode and expecting io.EOF is sufficient and simpler.

**[MINOR]** The JSON trailing content check uses dec.More(), which is only meaningful inside arrays/objects and may be confusing at top level. Relying solely on attempting a second Decode and expecting io.EOF is sufficient and simpler.
// 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
Review

[MINOR] The trailing-content check for JSON has a subtle logic issue. dec.More() returns true if there are more values in the current JSON array/object — it's not designed to detect trailing content after a top-level value. The intended check should be dec.Decode(&dummy) != io.EOF alone (which returns nil if there's another value, or io.EOF if the stream is done). The dec.More() call before the Decode is redundant and slightly misleading: if More() is true, the subsequent Decode will also succeed (returning nil, not io.EOF), so the condition triggers correctly either way. However, the dec.More() shortcut could confuse readers since More() is documented for use inside arrays/objects. Consider simplifying to just if err2 := dec.Decode(&dummy); err2 != io.EOF { err = fmt.Errorf(...) }.

**[MINOR]** The trailing-content check for JSON has a subtle logic issue. `dec.More()` returns true if there are more values in the current JSON array/object — it's not designed to detect trailing content after a top-level value. The intended check should be `dec.Decode(&dummy) != io.EOF` alone (which returns nil if there's another value, or io.EOF if the stream is done). The `dec.More()` call before the Decode is redundant and slightly misleading: if More() is true, the subsequent Decode will also succeed (returning nil, not io.EOF), so the condition triggers correctly either way. However, the `dec.More()` shortcut could confuse readers since `More()` is documented for use inside arrays/objects. Consider simplifying to just `if err2 := dec.Decode(&dummy); err2 != io.EOF { err = fmt.Errorf(...) }`.
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)
42
@@ -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
}
1
+38
View File
16
@@ -858,3 +858,41 @@ identity: test identity
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
t.Errorf("Name = %q, want %q", p.Name, "test")
}
}
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
func TestJSONTrailingContentRejected(t *testing.T) {
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
tests := []struct {
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
name string
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
content string
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
}{
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
{
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
name: "trailing garbage after object",
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
content: `{"name":"test","identity":"test identity"}garbage`,
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
},
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
{
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
name: "two JSON objects",
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
content: `{"name":"test","identity":"test identity"}{"name":"other"}`,
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
},
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
{
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
name: "trailing array",
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
content: `{"name":"test","identity":"test identity"}[]`,
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
},
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
}
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
for _, tt := range tests {
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
t.Run(tt.name, func(t *testing.T) {
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
dir := t.TempDir()
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
path := filepath.Join(dir, "test.json")
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
if err := os.WriteFile(path, []byte(tt.content), 0644); err != nil {
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
t.Fatalf("failed to write test file: %v", err)
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
}
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
_, err := LoadPersona(path)
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
if err == nil {
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
t.Fatal("expected error for trailing content, got nil")
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
}
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
if !strings.Contains(err.Error(), "trailing content") {
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
t.Errorf("error = %q, want to contain 'trailing content'", err.Error())
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
}
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
})
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
}
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
}
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
Review

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.