[NIT] The time package is imported but used only in TestAICoreClient_TokenExpiry and TestAICoreClient_WithTimeout. Not a problem, just noting that removing "time" from the import if those tests were moved would be needed. Current state is correct.
[NIT] Trailing space on the default: 'default' line (aicore-resource-group default value). Minor formatting inconsistency — matches the pre-existing similar trailing space on the original llm-provider default that was fixed in this PR.
[MINOR] Deployment cache is never invalidated. Once deployments are fetched, they are cached indefinitely. If a deployment goes down and is replaced (new deployment URL, same model name), the client will continue using the stale URL until restart. Consider adding a TTL or a re-fetch-on-error path (e.g., clear the cache and retry when CompleteAnthropic/CompleteOpenAI gets a non-2xx response).
[NIT] The struct field http *http.Client shadows the stdlib package name http. While it compiles fine, it can confuse readers and linters. Consider renaming to httpClient *http.Client (consistent with how other Go stdlib code names embedded HTTP clients).
[NIT] The anthropicRequest struct's AnthropicVersion field is set to the hard-coded string "bedrock-2023-05-31" at the call site rather than using a named constant. A package-level constant like AICoreAnthropicVersion would make this easier to update and self-documenting, similar to the already-defined AICoreOpenAIAPIVersion.
[MINOR] The deployment.ID field is stored but never used — only deployment.URL is referenced. This is dead storage. Either use it (e.g. in log messages for diagnostics) or remove the field and the struct altogether in favour of storing just the URL string.
[MINOR] In getDeploymentURL, there's a potential inefficiency: getToken is called before acquiring the write lock, which is correct for avoiding deadlock, but if fetchDeployments fails and the function is called again concurrently, multiple goroutines could redundantly call getToken (which itself acquires a lock). This is a minor performance nit rather than a correctness issue since getToken is idempotent and safe to call concurrently.
[NIT] The comment // Use a pointer to capture the server URL for use in the handler is slightly misleading — baseURL is a string, not a pointer. The actual mechanism is that the closure captures the variable by reference. The comment could be dropped or rephrased.
[NIT] Trailing whitespace on the default: 'default' line for aicore-resource-group. Minor but inconsistent with the rest of the file.
[MINOR] The deployment struct has an ID field that is populated from the deployments API response but never used anywhere in the code. If it's genuinely unused, it adds noise; if it was intended for logging or future use, a comment explaining its purpose would help. Consider removing it or adding a comment.
[MINOR] The deployment cache is never invalidated. If an AI Core deployment becomes unavailable (transitions from RUNNING to STOPPED) after the initial fetch, the cached stale URL will be used until the process restarts. This is likely acceptable for the current use case (short-lived CI jobs), but could cause hard-to-debug failures in longer-lived deployments. A TTL on the deployment cache would make this more robust.
[NIT] The field name http shadows the net/http package import. While Go resolves this correctly (the field is accessed via the receiver, the package via the bare name), it's a minor readability concern. The existing llm/client.go uses the same pattern so it's consistent within the codebase, but something like httpClient would be clearer.
[MINOR] Deployments are cached indefinitely after first fetch. If a deployment is restarted, updated, or a new one is added for a model, the client will keep using the stale cached URL until the process restarts. Consider adding a TTL or a mechanism to refresh on 4xx/5xx errors from the deployment URL, similar to how tokenExpiry handles token refresh.
[NIT] Committing a TODO.md containing a self-review into the main branch adds noise to the repository. This file is transient review scaffolding and would be better kept out of version control (e.g., in .gitignore) or removed before merge.
[NIT] The test comment // Use a pointer to capture the server URL for use in the handler is slightly misleading — baseURL is a plain string variable captured by closure, not a pointer.
[NIT] The deployment struct has an exported ID and URL field but the type is unexported. The ID field is stored in the map but never read back — only URL is used. The ID field could be removed unless planned for future use (e.g., logging, metrics).
[MINOR] The TODO.md self-review correctly notes that CompleteAnthropic and CompleteOpenAI lack the Content-Length body validation that doRequest has in client.go. Given these methods duplicate the HTTP execution pattern rather than going through doRequest, any future fix to the retry/validation logic in doRequest won't automatically apply here. Consider extracting a shared HTTP execution helper for AI Core, or routing through doRequest via an adapter.