[MINOR] The contains and containsHelper helpers are reimplementing strings.Contains, which is already available in the standard library. The helpers add no value and the custom implementation has a subtle bug: containsHelper is only called when len(s) > 0, but the outer contains check len(s) >= len(substr) already handles the empty-substr case. Just use strings.Contains directly — it handles all edge cases correctly and is idiomatic Go.
[MINOR] The BuildPersonaSystemPrompt function has 30+ consecutive sb.WriteString(...) calls for the hardcoded JSON output format schema. This duplicates the output format specification that already lives in the base BuildSystemBase() function. If the JSON schema changes (e.g., adding a new field), both functions must be updated. Consider extracting the shared output format section into a package-level constant or helper function to keep the schema definition in one place.
[MINOR] FormatMarkdownWithDisplay duplicates nearly all of the logic in FormatMarkdown. The only difference is the headerName/sentinelName split. FormatMarkdown could simply delegate to FormatMarkdownWithDisplay(result, reviewerName, reviewerName), eliminating ~35 lines of duplication. Duplicated formatting logic is a maintenance hazard — a future change (e.g., adding a new section) must be applied in two places.
[MINOR] When there are multiple RUNNING deployments for the same model name, the last one in the list silently wins (map assignment overwrites). If SAP AI Core could return multiple running deployments for the same model, this could select an unexpected one. The behavior is undocumented; at minimum a log warning would be helpful.
[MINOR] Duplicate doc comment. The getDeploymentURL function has its doc comment written twice: the first instance at line 148 is a plain 'getDeploymentURL returns the deployment URL...' and then it repeats again at line 149. One should be removed.
[MINOR] The fetchDeployments call inside getDeploymentURL is invoked while holding c.mu (write lock), and fetchDeployments calls c.http.Do which is a network I/O operation. The design comment acknowledges fetching the token outside the lock, but the deployment fetch itself still holds the lock during network I/O. This is fine for the intended short-lived CI use case (as noted in the struct doc), but it means concurrent callers waiting for other models will block during this I/O. The code comment could explicitly mention this constraint.
[NIT] The struct field http *http.Client shadows the imported net/http package name within the struct's method set. While Go resolves this correctly (field access via c.http.Do vs package via http.NewRequestWithContext), naming the field httpClient would be clearer and avoid any reader confusion.
[MINOR] The TestIsAnthropicModel test uses the old-style loop rather than t.Run subtests, making it harder to see which specific case failed. Per the testing patterns established in the codebase, table-driven tests should use t.Run with named subtests.
[NIT] The anthropicRequest struct fields have inconsistent alignment after the addition of AnthropicVersion - the field names are left-aligned but the types are misaligned compared to the original struct. gofmt handles this automatically, so this is only relevant if gofmt wasn't run, but worth noting.
[NIT] The BuildPersonaSystemPrompt function builds the JSON output format using many individual sb.WriteString calls for what is essentially a multi-line string constant. This is harder to read and maintain than a single const or backtick string. Consider using a const for the static instructions block.
[MINOR] The contains and containsHelper helpers re-implement strings.Contains from scratch. This is unnecessary — strings.Contains is already in the standard library and is used everywhere else in the test files. This also violates the principle of using standard library functions.
[NIT] The design doc still references YAML in several places (Persona struct with yaml: tags, .review/personas/trading.yaml, security.yaml phase items) after the JSON revision was made. These are internal doc inconsistencies but worth cleaning up for accuracy.
[MINOR] FormatMarkdownWithDisplay and FormatMarkdown share almost identical logic (only the header/footer differ). This duplication means any change to the findings table, summary format, etc. must be made in two places. Consider having FormatMarkdown delegate to FormatMarkdownWithDisplay, or extracting the shared body-building logic into a private helper.
[MINOR] filepath is imported but used only for filepath.Join("personas", filename) in LoadBuiltinPersona. Since embed.FS uses forward-slash paths on all platforms (per Go spec), using "personas/" + filename (plain string concatenation) would be both simpler and more correct — filepath.Join uses the OS separator on Windows, which would break the embed.FS lookup.
[NIT] Import "path/filepath" is not in alphabetical order with the other imports — it appears after "strings" instead of between "os/exec" and "strings". goimports would fix this automatically.
[MINOR] The maxTokens parameter in CompleteAnthropic is part of the public signature but the only caller (completeAICore in client.go) always passes the hardcoded value 8192. This leaks an implementation detail into the public API. Consider making maxTokens an internal constant or adding it to AICoreConfig, since callers currently have no way to control it through the Client interface.