[NIT] The checkYAMLDepth function receives maxNodes int as a parameter but MaxYAMLNodes is passed as the constant from the call site. The maxNodes parameter is never varied between call sites — it's always MaxYAMLNodes. Could simplify by using the constant directly in the function, but having it as a parameter makes testing easier (allows injecting lower limits in tests). Current approach is acceptable.
[NIT] The TestYAMLEmptyFileRejection test creates the temp dir outside the tests loop, reusing a single dir for all subtests. Since each subtest writes to tc.name+".yaml" (different file names), there's no collision. However, t.TempDir() is called once at the top of the test function rather than inside each subtest — this is fine since the file names don't overlap, but using t.TempDir() inside each t.Run would be slightly more idiomatic for isolation.
[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(...) }.
[NIT] The TestYAMLEmptyFileRejection subtests write files into a shared dir created outside the loop. If two subtests run with the same filename pattern, they'd overwrite each other. Currently the filenames are distinct (completely_empty.yaml, whitespace_only.yaml, comment_only.yaml) so there's no actual issue, but using t.TempDir() inside each subtest would be the idiomatic pattern.
[MINOR] The validated map stores the depth at which a node was last validated, used for depth-aware short-circuiting. However, the map is keyed by ast.Node (interface type). Interface map keys use pointer identity for concrete pointer types, which is correct here. But the comment says "maps each node to the minimum depth" — the map is actually storing the current visit's depth (overwriting each time the node is visited at a greater depth), not the minimum. The comment should say "most recently validated depth" or "maximum depth at which validated" to avoid confusion.
[NIT] In TestYAMLEmptyFileRejection, the dir variable is declared in the outer test function but files are written using paths under dir from within subtests. Since t.TempDir() is called on the outer t, it will be cleaned up when the outer test completes, which is fine. However, each subtest creates a file with the test case name as part of the filename directly in the shared dir. This is fine for parallel safety (test cases have unique names) but it is slightly more idiomatic to call t.TempDir() inside the subtest.
[MINOR] The checkYAMLDepth function increments *nodeCount before the depth-aware short-circuit check (validated map lookup). This means a node revisited at a shallower depth (which immediately returns via the short-circuit) still increments the counter. The comment acknowledges this as 'intentional conservative overcounting', which is a reasonable security posture. However, it also means the node count can be inflated by the number of times aliases reference the same shallow subtree, potentially causing false positives for legitimate YAML with many alias references to the same anchor. This is a trade-off that is documented and defensible, but worth noting.
[MINOR] The checkYAMLDepth switch statement has no default case. While this is intentional (scalar types are leaf nodes and need no recursion), a comment inside the switch or a default: // scalar leaf node, no children case would make it clearer to future readers that the omission is deliberate rather than an oversight. The existing comment above the closing brace partially covers this but is outside the switch body.
[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.
[MINOR] TestYAMLDeeplyNestedRejection comment says 'With 25 levels, we exceed MaxYAMLDepth (20)' but the depth counting depends on both MappingNode and MappingValueNode traversal (each contributes depth+1 for key, depth+1 for value). The comment would benefit from a concrete trace of how the depth accumulates to make the test self-documenting and easier to maintain if MaxYAMLDepth changes.
[MINOR] The go 1.26.2 in go.mod references a Go version that does not exist as a stable release (current stable is 1.23/1.24 range). This is likely a typo or pre-release version. While not introduced by this PR, it's worth noting as it could cause toolchain issues.
[MINOR] The nodeCount increment (line 213, *nodeCount++) happens before the depth-aware short-circuit check (line 222, if prevDepth, ok := validated[node]; ok && depth <= prevDepth). This means if a node is revisited at a shallower depth (triggering the short-circuit), it still increments the count, causing overcounting of shared subtrees referenced via aliases at multiple depths. This could cause false positives on the MaxYAMLNodes limit for legitimately large but valid documents that share anchors extensively. The nodeCount increment should arguably be inside the path where we actually traverse, or this behavior should be explicitly documented as intentional (conservative overcounting).
[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.
[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.
[MINOR] The nodeCount is incremented before the visiting cycle detection check. For cyclic structures, this means a node that triggers cycle detection (returns early) still increments nodeCount. This slightly over-counts nodes in cyclic test scenarios, but since real YAML parsed by parser.ParseBytes cannot have true reference cycles in the AST (only alias references that point forward), this is functionally harmless in production. However, in the unit tests that construct artificial cycles, the count will be inflated.