rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

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
Conversation 61 Commits 13 Files Changed 6
+2 -2
1 changed files with 2 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 493349e11a - Show all commits
review/persona.go
+2 -2
View File
4
 @@ -152,7 +152,7 @@ func parsePersona(data []byte, source string) (*Persona, error) {
// Without this check, input like `{"name":"x"}garbage` would
// silently succeed because Decoder stops after one object.
var dummy json.RawMessage
sonnet-review-bot commented 2026-05-12 21:43:22 +00:00
Review
Copy Link
Copy Source

[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 {
if err2 := dec.Decode(&dummy); err2 != io.EOF {
err = fmt.Errorf("unexpected trailing content after JSON object")
}
}
19
 @@ -205,7 +205,7 @@ func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
sonnet-review-bot commented 2026-05-12 22:23:22 +00:00
Review
Copy Link
Copy Source

[MINOR] The unmarshalYAMLWithDepthLimit function performs a two-pass decode (AST parse then full decode). The second pass with yaml.Strict() will also parse the YAML from scratch, so the file bytes are parsed twice. This is intentional per the comment but worth noting: if the goccy/go-yaml Strict() decoder also does alias resolution internally, the depth protection from the first pass only guards the structural AST walk, not the decoder's internal expansion. This appears acceptable given the design, but should be validated that yaml.Strict() doesn't recurse unboundedly on crafted alias chains during decode.

**[MINOR]** The `unmarshalYAMLWithDepthLimit` function performs a two-pass decode (AST parse then full decode). The second pass with `yaml.Strict()` will also parse the YAML from scratch, so the file bytes are parsed twice. This is intentional per the comment but worth noting: if the goccy/go-yaml `Strict()` decoder also does alias resolution internally, the depth protection from the first pass only guards the structural AST walk, not the decoder's internal expansion. This appears acceptable given the design, but should be validated that `yaml.Strict()` doesn't recurse unboundedly on crafted alias chains during decode.
rodin commented 2026-05-13 02:58:48 +00:00
Review
Copy Link
Copy Source

Already addressed. The comment block immediately above the dec := yaml.NewDecoder(...) call explicitly documents this:

Safety note: goccy/go-yaml's decoder does not expand YAML aliases recursively — it resolves them via the pre-built AST, which our first pass already depth-checked. Alias chains that would exceed depth limits are caught above; the decoder merely reads the resolved scalar values.

The two-pass design is intentional: pass 1 validates structure/depth on the AST (where we have full control), pass 2 uses Strict() for field validation (which doesn't re-expand aliases recursively). No change needed.

Already addressed. The comment block immediately above the `dec := yaml.NewDecoder(...)` call explicitly documents this: > Safety note: goccy/go-yaml's decoder does not expand YAML aliases recursively — it resolves them via the pre-built AST, which our first pass already depth-checked. Alias chains that would exceed depth limits are caught above; the decoder merely reads the resolved scalar values. The two-pass design is intentional: pass 1 validates structure/depth on the AST (where we have full control), pass 2 uses `Strict()` for field validation (which doesn't re-expand aliases recursively). No change needed.
// checkYAMLDepth recursively checks that YAML AST nodes don't exceed the depth
gpt-review-bot commented 2026-05-13 02:08:16 +00:00
Review
Copy Link
Copy Source

[MINOR] Alias cycles are treated as non-errors (cycle detection returns nil). While this prevents recursion issues, consider whether rejecting explicit alias cycles with an error would be a safer fail-fast behavior rather than relying on the decoder to handle them.

**[MINOR]** Alias cycles are treated as non-errors (cycle detection returns nil). While this prevents recursion issues, consider whether rejecting explicit alias cycles with an error would be a safer fail-fast behavior rather than relying on the decoder to handle them.
// limit or the total node count limit. It uses two tracking maps:
// - validated: maps each node to the minimum depth at which it was previously
// - validated: maps each node to the maximum depth at which it was previously
// checked. If a node is revisited at a deeper depth (e.g., via an alias),
// we re-check it to ensure the combined effective depth doesn't exceed limits.
gpt-review-bot commented 2026-05-12 21:25:39 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] ListBuiltinPersonas treats .json and .yml as valid extensions although built-ins are embedded as YAML only; this is harmless but could be simplified if built-ins will never include those formats.

**[NIT]** ListBuiltinPersonas treats .json and .yml as valid extensions although built-ins are embedded as YAML only; this is harmless but could be simplified if built-ins will never include those formats.
// - visiting: per-path recursion stack for true cycle detection. A node on the
sonnet-review-bot commented 2026-05-13 01:46:46 +00:00
Review
Copy Link
Copy Source

[MINOR] The validated map stores the depth at which a node was last validated, but depth <= prevDepth as the short-circuit condition means a node visited at depth 3 that was previously validated at depth 5 would be skipped — which is correct (deeper previous validation is more conservative). However the comment says 'validated it at the same or deeper effective depth' which is slightly ambiguous. The logic is correct but 'same or shallower current depth compared to the previous validation depth' would be clearer.

**[MINOR]** The `validated` map stores the depth at which a node was last validated, but `depth <= prevDepth` as the short-circuit condition means a node visited at depth 3 that was previously validated at depth 5 would be skipped — which is correct (deeper previous validation is more conservative). However the comment says 'validated it at the same or deeper effective depth' which is slightly ambiguous. The logic is correct but 'same or shallower current depth compared to the previous validation depth' would be clearer.
19