[MINOR] ListBuiltinPersonas iterates over a map[string]bool to build the names slice, producing non-deterministic ordering. This can cause flaky tests or confusing error messages ("available: docs, security, architect" vs "available: architect, docs, security" depending on map iteration order). The slice should be sorted before returning: sort.Strings(names).
[MAJOR] The design checklist explicitly requires "Test for deeply nested YAML rejection" (item 7 in the completion checklist), but no such test exists. This is especially important given that the security rationale for this PR includes protection against AIKIDO-2024-10486 (deeply nested YAML DoS). Without this test, the security property is unverified.
[MAJOR] The design document (docs/DESIGN-57-yaml-persona.md) explicitly specifies github.com/goccy/go-yaml v1.16.0+ as the required library, citing its built-in protection against deeply nested structures (AIKIDO-2024-10486 / DoS vulnerability fix). The implementation instead uses gopkg.in/yaml.v3, which does NOT have the same built-in depth limiting. The comment in parsePersona says "go-yaml v1.16.0+ has built-in protection against deeply nested structures" — this is factually incorrect for gopkg.in/yaml.v3. This is a security gap: a maliciously crafted deeply-nested YAML file could cause a stack overflow. Either switch to github.com/goccy/go-yaml (and add it to the allowlist), or add explicit depth limiting when using gopkg.in/yaml.v3.
[NIT] The comment // go-yaml v1.16.0+ has built-in protection against deeply nested structures is misleading/incorrect for gopkg.in/yaml.v3. It appears to have been written for goccy/go-yaml and copied over without updating. This should either be removed or corrected to accurately describe gopkg.in/yaml.v3's actual behavior.
[NIT] The comment says 'POSIX-compatible' for the awk parsing but the outer loop uses a Bash process substitution < <(...) which is Bash-specific. The comment is mildly misleading — it means awk itself is POSIX, not the overall approach.
[MINOR] If go list returns non-module output (e.g. build errors) the error is captured in DIRECT_IMPORTS and the early-exit error message will contain the raw go toolchain output. This works adequately but the error message could be confusing. Minor quality-of-life issue.
[MINOR] The script checks go.mod direct dependencies for allowlist compliance, but the scope enforcement (lines 97-111) checks the full production import graph via go list -deps. These two checks are at different granularities and could diverge. For example, a direct dependency might be test-only in practice but go list -deps ./... (which doesn't filter test files) would still traverse it. Using go list -deps -test=false ./... more explicitly conveys intent, though the current flag-less form already excludes test builds.
[NIT] The note 'Transitive dependencies of approved packages are automatically allowed' is a policy statement, but the enforcement script only checks direct module dependencies (via go list -m ... all with .Indirect filtered out). This is correct and intentional, but it's worth confirming the wording matches: transitive deps won't appear as violations, which aligns with the statement.
[MINOR] The test-scope enforcement uses `go list -deps -f '...' ./...
[MINOR] The go list -m output includes the current module itself on the first line, and the comment says 'Filter out the module itself (first line)'. However, tail -n +2 strips the first line unconditionally. If go list -m ... all ever reorders output or includes a blank first line (e.g., when run in a workspace), this assumption breaks silently. A more robust approach would be to filter by comparing against $(go list -m) (the current module path) rather than relying on line position.
[NIT] The sentence 'Transitive dependencies of approved packages are automatically allowed' is a policy statement that the check-deps.sh script does NOT currently verify (it only checks direct deps via go list -m). This is correct behavior—you generally don't want to enumerate all transitive deps—but the wording could be clearer: something like 'Transitive dependencies pulled in by approved packages do not need to be listed here' to make it clear this is intentional, not an oversight.
[MINOR] The test-only production check uses grep -q "^${test_pkg}" without anchoring the end of the pattern. A package path like github.com/google/go-cmp would inadvertently match a hypothetical github.com/google/go-cmp-extended. Should be `grep -qE "^${test_pkg}(/
[NIT] The comment says 'POSIX-compatible' but declare -A (associative arrays) is a bash 4+ feature and is not POSIX. The comment should say 'bash 4+' to accurately describe the requirement. macOS ships bash 3.2 by default (GPLv2 licensing constraint), so this script would fail on stock macOS without Homebrew bash. Worth documenting the bash version requirement in a shebang comment or README.
[NIT] The table lists github.com/google/go-cmp with scope 'test only', but the enforcement script (check-deps.sh) does not differentiate between production and test-only scope — it checks all direct go.mod dependencies uniformly. If go-cmp ends up in go.mod as a direct dependency (which it will when used in _test.go files), it passes the allowlist check regardless of where it's imported. The 'test only' scope annotation is purely documentation with no mechanical enforcement. This is acceptable but worth noting so future maintainers don't assume it's enforced.
[MINOR] go list -m -f '{{if not .Indirect}}{{.Path}}{{end}}' all lists module-level direct dependencies, not package-level imports. This means a module in go.mod that isn't actually imported in any Go source file will still be flagged. More precisely, it will miss the case where someone adds a go.mod entry for an unapproved module but the script will catch modules not in go.mod at all (since they wouldn't be listed). This is probably fine for the intended purpose, but the comment 'DIRECT dependencies only' may mislead — it means direct in the go.mod sense, not 'actually used in source'. Consider a brief clarifying comment.