[NIT] The final line counts direct deps via wc -l on $IMPORTS, but $IMPORTS could contain blank lines depending on the go list output format when some template expansions produce empty strings. The grep -v '^$' earlier helps but the count may still be off by one if there's a trailing newline. Minor cosmetic issue.
[MINOR] The -oP flag for PCRE-style regex (\K) is a GNU grep extension and is not available on macOS (BSD grep). This will silently fail to parse the allowlist on macOS developer machines (the `
[MINOR] go list -m all includes indirect/transitive dependencies. If yaml.v3 or go-cmp pull in their own transitive deps, those would appear here and trigger false-positive violations. Consider using `go mod edit -json
[MINOR] The ALLOWED_PATTERN variable is built but never used. The actual checking is done in the loop with direct string comparison against the ALLOWED array. The dead code is harmless but confusing — either use the pattern (e.g., with grep) or remove it.
[NIT] echo -e is not POSIX-compliant and behavior varies across shells (e.g., on some systems /bin/bash may not support -e with printf semantics). The \n in the violations string also won't render on all platforms. Consider using printf instead: printf 'The following imports are not in the allowlist:%b\n' "$VIOLATIONS".
[NIT] The process for adding new dependencies doesn't mention who has approval authority (e.g. maintainers, a specific review process). Adding a brief note like 'requires approval from a maintainer' would make the governance expectation explicit.
[NIT] go-cmp is a test-only dependency, but the table doesn't distinguish test dependencies from production dependencies. A 'Scope' column (e.g. 'test only' vs 'production') would clarify that go-cmp should never appear in non-test code, preventing accidental production use.
[MAJOR] Adds github.com/goccy/go-yaml v1.19.2 as a dependency. The repository's CONVENTIONS.md explicitly states 'Go standard library only — no external dependencies.' This PR directly violates that constraint. A stdlib-only YAML parser would need to be implemented manually, or the feature scope should be reconsidered (e.g., keeping only JSON for external files and using a minimal hand-rolled YAML subset, or representing persona files differently). The dependency cannot be added regardless of how useful YAML support would be.
[NIT] TestLoadPersonaCaseInsensitiveExtension tests .YAML and .YML (uppercase) extensions by creating files with those names and parsing them. On case-insensitive filesystems (macOS) this will work, but on case-sensitive filesystems (Linux CI), creating a file named test.YAML and reading it back is fine — however, this is testing OS-level file creation, not just the parsing logic. The test is correct but would be cleaner using parsePersona directly with a fake source path like "test.YAML" rather than creating real files.
[MINOR] The design doc says 'Consistency: use .yaml extension (not .yml)' as a constraint, but the actual implementation supports both .yaml and .yml (and the open questions section documents this decision). The constraint section of the design doc is now inconsistent with the implemented behavior and should be updated to reflect the actual decision.
[NIT] The fallback to JSON in LoadBuiltinPersona has a comment 'Fall back to JSON for backwards compatibility', but the embeddedPersonas embed directive now only includes *.yaml files (//go:embed personas/*.yaml). The JSON files have been deleted. This means the JSON fallback path in LoadBuiltinPersona is dead code — it can never succeed because no .json files are embedded. The fallback should either be removed or the embed directive should include *.json if JSON built-in personas need to be supported.
[MINOR] The seen map in ListBuiltinPersonas is used only to deduplicate persona names (it only ever stores true), but a map[string]struct{} would be more idiomatic. More importantly, the deduplication logic has a subtle inefficiency: it checks if !seen[personaName] before setting to true, but since the zero value of bool is false, the check is redundant — seen[personaName] = true is sufficient by itself regardless of the current value. The check adds no correctness but implies to readers it does something meaningful.
[MINOR] ListBuiltinPersonas returns a non-deterministic (map-iteration) order of persona names. While the return value isn't documented as sorted, callers such as the error message in LoadBuiltinPersona join the names to display to users, and the non-deterministic order makes tests and error messages flaky. The slice should be sorted with slices.Sort (or sort.Strings) before returning.
[NIT] The maxErrorBodyLen constant is unexported and only used within aicore.go, but truncateBody is also unexported. Both are fine as-is, but maxErrorBodyLen = 200 is a magic number without a unit comment. Consider renaming to maxErrorBodyBytes or adding // bytes in the comment for clarity.
[NIT] The anthropicRequest struct reformatting (alignment change) is a cosmetic-only change unrelated to the feature. Not a problem, just worth noting it's pure noise in the diff.
[MINOR] In fetchDeployments, when multiple RUNNING deployments exist for the same model name (as tested in TestAICoreClient_DeploymentFetch), the last one in iteration order wins — the map is overwritten silently. The test asserts deploy-789 (the second RUNNING gpt-5) is selected, but this is only guaranteed because JSON array order is preserved in Go's json.Unmarshal. This is fragile; a comment noting the last-write-wins behavior (or explicitly preferring the last RUNNING entry) would clarify intent.