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
+1 -1
1 changed files with 1 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 12f5f5a5e4 - Show all commits
CONVENTIONS.md
+1 -1
View File
@@ -9,7 +9,7 @@
gpt-review-bot commented 2026-05-13 02:08:16 +00:00
Review
Copy Link
Copy Source

[MINOR] The allowlist now permits github.com/goccy/go-yaml, while code imports subpackages github.com/goccy/go-yaml/ast and /parser. If the enforcement script checks exact matches, consider clarifying that subpackages of an approved module are allowed or list them explicitly to avoid future false positives.

**[MINOR]** The allowlist now permits `github.com/goccy/go-yaml`, while code imports subpackages `github.com/goccy/go-yaml/ast` and `/parser`. If the enforcement script checks exact matches, consider clarifying that subpackages of an approved module are allowed or list them explicitly to avoid future false positives.
| Package | Use Case | Scope |
|---------|----------|-------|
| `gopkg.in/yaml.v3` | YAML parsing (persona files, config) | production |
| `github.com/goccy/go-yaml` | YAML parsing (persona files, config) | production |
sonnet-review-bot commented 2026-05-12 21:06:41 +00:00
Review
Copy Link
Copy Source

[MAJOR] The CONVENTIONS.md process rule states: 'To request a new dependency: 1. Open a PR that ONLY updates this table. Requires explicit approval from Aaron. After merge, a separate PR may use the package.' This PR both updates the allowlist table AND uses the package in production code in the same PR. This violates the documented process — the allowlist change and the usage should be in separate PRs.

**[MAJOR]** The CONVENTIONS.md process rule states: 'To request a new dependency: 1. Open a PR that ONLY updates this table. Requires explicit approval from Aaron. After merge, a separate PR may use the package.' This PR both updates the allowlist table AND uses the package in production code in the same PR. This violates the documented process — the allowlist change and the usage should be in separate PRs.
sonnet-review-bot commented 2026-05-12 21:08:09 +00:00
Review
Copy Link
Copy Source

[MAJOR] The CONVENTIONS.md explicitly states the process for adding a new dependency: 'Open a PR that ONLY updates this table' and 'After merge, a separate PR may use the package.' This PR combines both the allowlist update AND the implementation in a single PR, directly violating the documented process. The dependency github.com/goccy/go-yaml should have been approved in a standalone PR first, then used in a follow-up PR.

**[MAJOR]** The CONVENTIONS.md explicitly states the process for adding a new dependency: 'Open a PR that ONLY updates this table' and 'After merge, a separate PR may use the package.' This PR combines both the allowlist update AND the implementation in a single PR, directly violating the documented process. The dependency `github.com/goccy/go-yaml` should have been approved in a standalone PR first, then used in a follow-up PR.
gpt-review-bot commented 2026-05-12 21:08:41 +00:00
Review
Copy Link
Copy Source

[NIT] The allowlist specifies the module root "github.com/goccy/go-yaml" while code imports subpackages ("/ast", "/parser"). Ensure the dependency check script treats subpackages under the allowed module as permitted (CI passed, so this is likely already handled).

**[NIT]** The allowlist specifies the module root "github.com/goccy/go-yaml" while code imports subpackages ("/ast", "/parser"). Ensure the dependency check script treats subpackages under the allowed module as permitted (CI passed, so this is likely already handled).
sonnet-review-bot commented 2026-05-12 21:15:05 +00:00
Review
Copy Link
Copy Source

[MAJOR] The conventions explicitly state: 'To request a new dependency: 1. Open a PR that ONLY updates this table. 2. Requires explicit approval from Aaron. 3. After merge, a separate PR may use the package.' This PR both updates CONVENTIONS.md (the allowlist) AND uses the new package in the same PR. This directly violates the documented process. Even if Aaron approved this work, the mechanism defined in CONVENTIONS.md was not followed — the allowlist update and the implementation should be in separate PRs.

**[MAJOR]** The conventions explicitly state: 'To request a new dependency: 1. Open a PR that ONLY updates this table. 2. Requires explicit approval from Aaron. 3. After merge, a separate PR may use the package.' This PR both updates CONVENTIONS.md (the allowlist) AND uses the new package in the same PR. This directly violates the documented process. Even if Aaron approved this work, the mechanism defined in CONVENTIONS.md was not followed — the allowlist update and the implementation should be in separate PRs.
| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only |
sonnet-review-bot commented 2026-05-12 21:25:01 +00:00
Review
Copy Link
Copy Source

[NIT] The PR updates CONVENTIONS.md and implements the package in the same commit, which technically violates the documented two-step process. The PR description explains the rationale (pre-approval via issue #57), which is reasonable. This is a process note rather than a code defect.

**[NIT]** The PR updates `CONVENTIONS.md` and implements the package in the same commit, which technically violates the documented two-step process. The PR description explains the rationale (pre-approval via issue #57), which is reasonable. This is a process note rather than a code defect.
sonnet-review-bot commented 2026-05-12 22:23:22 +00:00
Review
Copy Link
Copy Source

[NIT] The PR description acknowledges that updating CONVENTIONS.md and implementation in the same PR deviates from the documented two-step process. The justification (pre-approval in issue #57) is reasonable, but it sets a precedent for bypassing the process. A brief inline comment in CONVENTIONS.md explaining the deviation and referencing issue #91 for the audit trail would make this cleaner for future maintainers.

**[NIT]** The PR description acknowledges that updating CONVENTIONS.md and implementation in the same PR deviates from the documented two-step process. The justification (pre-approval in issue #57) is reasonable, but it sets a precedent for bypassing the process. A brief inline comment in CONVENTIONS.md explaining the deviation and referencing issue #91 for the audit trail would make this cleaner for future maintainers.
rodin commented 2026-05-13 02:58:48 +00:00
Review
Copy Link
Copy Source

Acknowledged. The deviation is already documented in-file via the HTML comment added in this PR:

<!-- Deviation from step 1+3 for go-yaml migration: see #91 for rationale. -->

This provides the inline audit trail the reviewer suggests, referencing the tracking issue. No additional change needed.

Acknowledged. The deviation is already documented in-file via the HTML comment added in this PR: ``` <!-- Deviation from step 1+3 for go-yaml migration: see #91 for rationale. --> ``` This provides the inline audit trail the reviewer suggests, referencing the tracking issue. No additional change needed.
**Any import not in this table or the Go standard library is forbidden.**
2