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 144a36a2a7 - Show all commits
docs/DESIGN-57-yaml-persona.md
+2 -2
View File
@@ -9,7 +9,7 @@ JSON is awkward for persona files that contain multi-line text (identity, severi
- Backwards compatibility: existing JSON personas must continue to work - Backwards compatibility: existing JSON personas must continue to work
- Security: protect against DoS via deeply nested YAML (AIKIDO-2024-10486) - Security: protect against DoS via deeply nested YAML (AIKIDO-2024-10486)
- Consistency: use `.yaml` extension (not `.yml`) - Consistency: use `.yaml` extension (not `.yml`)
- Library: use `gopkg.in/yaml.v3` (approved in CONVENTIONS.md) with explicit depth limiting - Library: use `github.com/goccy/go-yaml` v1.16.0+ (approved in CONVENTIONS.md); has built-in depth protection via `MaxYAMLDepth`/`MaxYAMLNodes` constants
gpt-review-bot commented 2026-05-12 20:54:44 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The design doc claims using go-yaml's built-in depth protection via MaxYAMLDepth/MaxYAMLNodes instead of a manual depth walk, but the implementation still performs a custom AST depth/node-count check. Update the doc to reflect the actual approach or adopt the library's built-in options if available.

**[MINOR]** The design doc claims using go-yaml's built-in depth protection via MaxYAMLDepth/MaxYAMLNodes instead of a manual depth walk, but the implementation still performs a custom AST depth/node-count check. Update the doc to reflect the actual approach or adopt the library's built-in options if available.
sonnet-review-bot commented 2026-05-12 21:15:05 +00:00
Review
Copy Link
Copy Source

[NIT] The design document still contains the pseudocode showing the old gopkg.in/yaml.v3 api (yaml.Node, yaml.NewDecoder, etc.) with a note saying it's outdated. Consider either removing the old pseudocode entirely or replacing it with the actual implementation approach, since having misleading pseudocode in design docs can confuse future contributors.

**[NIT]** The design document still contains the pseudocode showing the old gopkg.in/yaml.v3 api (yaml.Node, yaml.NewDecoder, etc.) with a note saying it's outdated. Consider either removing the old pseudocode entirely or replacing it with the actual implementation approach, since having misleading pseudocode in design docs can confuse future contributors.
## Proposed Approach ## Proposed Approach
gpt-review-bot commented 2026-05-12 20:54:15 +00:00
Review
Copy Link
Copy Source

[MINOR] Documentation states the library’s built-in depth protection (MaxYAMLDepth/MaxYAMLNodes decoder options) is used instead of manual checks, but the code still performs an AST-based depth/node count walk before decoding. Update the design doc to reflect the current implementation or use the library-provided options if intended.

**[MINOR]** Documentation states the library’s built-in depth protection (MaxYAMLDepth/MaxYAMLNodes decoder options) is used instead of manual checks, but the code still performs an AST-based depth/node count walk before decoding. Update the design doc to reflect the current implementation or use the library-provided options if intended.
1
 @@ -63,7 +63,7 @@ func checkYAMLDepth(node *yaml.Node, depth, maxDepth int) error {
} }
``` ```
The `gopkg.in/yaml.v3` library does not have built-in depth protection, so we implement explicit depth checking by first decoding into a `yaml.Node`, walking the tree to verify depth (including alias resolution), then decoding into the target struct. The `github.com/goccy/go-yaml` library provides built-in depth protection via `MaxYAMLDepth` and `MaxYAMLNodes` decoder options. We use these instead of a manual depth-checking walk.
sonnet-review-bot commented 2026-05-12 21:06:41 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The design doc still contains the old gopkg.in/yaml.v3-based code sample for unmarshalYAMLWithDepthLimit and checkYAMLDepth (using yaml.Node and yaml.AliasNode) from the original implementation. These code snippets now describe a design that was NOT implemented — the actual implementation uses the goccy/go-yaml AST approach. The design doc should be updated to reflect the actual implementation, or the code samples removed/replaced.

**[NIT]** The design doc still contains the old `gopkg.in/yaml.v3`-based code sample for `unmarshalYAMLWithDepthLimit` and `checkYAMLDepth` (using `yaml.Node` and `yaml.AliasNode`) from the original implementation. These code snippets now describe a design that was NOT implemented — the actual implementation uses the `goccy/go-yaml` AST approach. The design doc should be updated to reflect the actual implementation, or the code samples removed/replaced.
## State/Data Model ## State/Data Model