fix(ai): make model discovery auth-consistent - #67
Draft
adityathebe wants to merge 2 commits into
Draft
Conversation
Gavel summary
Totals: 0 passed · 0 failed · 0 skipped · - |
adityathebe
marked this pull request as draft
August 11, 2026 17:03
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR introduces immutable authentication snapshots, endpoint-aware model discovery, immediate adapter-cache invalidation, and machine-local HMAC-based model caches with per-fingerprint storage and filesystem locking. ChangesCredential-Scoped Model Availability
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResolveModels
participant ModelCache
participant ProviderEndpoint
Client->>ResolveModels: request model resolution
ResolveModels->>ModelCache: read credential- and endpoint-scoped entry
alt cache miss
ResolveModels->>ProviderEndpoint: fetch models with token and endpoint
ProviderEndpoint-->>ResolveModels: return model list
ResolveModels->>ModelCache: atomically write locked entry
end
ResolveModels-->>Client: return resolved models
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Gavel summary
Totals: 3556 passed · 1 failed · 10 skipped · 2m28s Failing testsgithub.com/flanksource/captain/pkg/cli — TestFullCycleWithAManualAgent |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
pkg/ai/adapters_cache_test.go (1)
270-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist
t.TempDir()out of the probe closure to isolate the recapture-error path.The closure calls
t.TempDir()on every capture, so each capture also gets a differentHome. The fingerprint then differs regardless ofProbeError. The test still passes, but it no longer proves that theProbeErrorbranch is the cause.Create the directory once outside the closure.
♻️ Proposed change
wantErr := errors.New("credential vault became unreadable") + home := t.TempDir() captures := 0 adapterAuthProbe = func() AuthProbe { captures++ - probe := fakeProbe(nil, nil, nil, t.TempDir()) + probe := fakeProbe(nil, nil, nil, home) if captures > 1 { probe.ProbeError = wantErr } return probe }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ai/adapters_cache_test.go` around lines 270 - 277, Hoist the t.TempDir() call out of the adapterAuthProbe closure and store its result for reuse on every fakeProbe creation. Keep the closure’s capture counting and ProbeError assignment unchanged so repeated captures share the same Home and isolate the recapture-error behavior.pkg/ai/adapters.go (1)
355-372: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConfirm the fingerprint stays stable across processes and never leaks tokens.
frozenCredentialStatecarries the rawTokenintojson.Marshalbefore hashing. The digest is safe, but the intermediate buffer holds plaintext credentials. That buffer is not zeroed and could reach a heap dump.
stateFingerprintis also compared only in-process today. If a future change persists it, the digest becomes a credential oracle without a machine-local key, unlike the HMAC scheme used by the model cache layer of this stack.Consider hashing an HMAC of the token, keyed with the same machine key the catalog cache uses, instead of the raw token.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ai/adapters.go` around lines 355 - 372, Update the fingerprint construction around frozenCredentialState and stateFingerprint so raw credential tokens are never placed in the marshaled state. Replace each Token value with an HMAC-derived representation using the same machine key and established catalog-cache mechanism, preserving deterministic output across processes and existing non-secret credential metadata. Hash only the sanitized state and retain the current SHA-256 fingerprint format.README.md (1)
499-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the legacy cache file.
The text accurately describes the new per-entry cache.
modelCacheRootinpkg/ai/model_cache.gostops using the legacy~/.config/captain/models.jsonand only tightens its permissions to0600. Upgrading users keep a stale file that is never read. One sentence stating that the legacy file is unused and can be deleted would prevent confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 499, Update the model-cache documentation near the description of `~/.config/captain/models/` to state that the legacy `~/.config/captain/models.json` file is no longer read and may be safely deleted.pkg/ai/catalog_resolve_test.go (1)
300-302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid shadowing the
credentialspackage, and reuse the temp home path.Line 300 declares a local variable named
credentials. It shadows the importedcredentialspackage for the rest of this function. The test compiles today because it does not use the package after line 300. A later edit that needscredentials.SourceVaulthere would fail to compile.Lines 321 and 326 read
os.Getenv("HOME")twice. Capture the temp directory in a variable instead. This also removes theos-readfile-getenv-path-gostatic analysis hint at line 325, which is a false positive becauseHOMEis set byt.Setenvto a test-owned directory.♻️ Proposed refactor
- t.Setenv("HOME", t.TempDir()) + home := t.TempDir() + t.Setenv("HOME", home) prev := liveModelFetcher @@ - credentials := NewCredentialSnapshot(map[Backend]api.ResolvedAPIKey{ + snapshot := NewCredentialSnapshot(map[Backend]api.ResolvedAPIKey{ BackendOpenAI: {Token: "endpoint-token"}, }) @@ - Backend: BackendOpenAI, UseTokens: true, Credentials: credentials, APIURL: apiURL, + Backend: BackendOpenAI, UseTokens: true, Credentials: snapshot, APIURL: apiURL, @@ - entries, err := os.ReadDir(filepath.Join(os.Getenv("HOME"), ".config", "captain", "models")) + modelsDir := filepath.Join(home, ".config", "captain", "models") + entries, err := os.ReadDir(modelsDir) if err != nil { t.Fatalf("ReadDir model cache: %v", err) } for _, entry := range entries { - data, err := os.ReadFile(filepath.Join(os.Getenv("HOME"), ".config", "captain", "models", entry.Name())) + data, err := os.ReadFile(filepath.Join(modelsDir, entry.Name()))Also applies to: 321-326
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ai/catalog_resolve_test.go` around lines 300 - 302, Rename the local credentials snapshot variable in the test to avoid shadowing the imported credentials package, and capture the temporary HOME value once after t.Setenv. Reuse that variable for the path construction and file read around the affected test setup.Source: Linters/SAST tools
pkg/ai/catalog_resolve.go (2)
83-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn
nilrows on the error path for consistency.Line 85 filters
rowsbefore checkingerr. The cacheable path at lines 99-102 returnsnilwhenresolveFreshfails. Correct callers checkerrfirst, so behavior does not change, but the two paths differ.♻️ Proposed refactor
if !cacheable { rows, err := resolveFresh(ctx, opts, credentials) - return filterResolved(rows, opts.Filter), err + if err != nil { + return nil, err + } + return filterResolved(rows, opts.Filter), nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ai/catalog_resolve.go` around lines 83 - 86, Update the non-cacheable branch of the resolver around resolveFresh and filterResolved to check err before filtering; return nil rows with the error on failure, matching the cacheable path, and only call filterResolved for successful results.
298-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the
UseTokenscheck out of the loop.
!opts.UseTokensdoes not change across iterations. Move the check before the loop so the intent is clear at the point of the decision.♻️ Proposed refactor
var hmacKey []byte - for _, backend := range selectedAPIBackends(opts.Backend) { - if !opts.UseTokens { - break - } - resolved := credentials.APIKey(backend) + backends := []Backend(nil) + if opts.UseTokens { + backends = selectedAPIBackends(opts.Backend) + } + for _, backend := range backends { + resolved := credentials.APIKey(backend) if strings.TrimSpace(resolved.Token) == "" { continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ai/catalog_resolve.go` around lines 298 - 306, Move the opts.UseTokens guard out of the selectedAPIBackends loop in the surrounding token-resolution logic, checking it before iteration and skipping the entire loop when disabled. Remove the per-iteration break while preserving the existing backend filtering and token resolution behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ai/adapters_cache.go`:
- Around line 39-45: Optimize CachedAdapters around freezeAuthProbe so cache
hits do not perform full credential-file hashing while holding adapterCacheMu.
Add a short-lived memoization or equivalent cheap mtime/size pre-check for the
probe, while preserving probe error handling and invalidating or recomputing the
full fingerprint when relevant adapter state changes.
- Around line 46-69: Update the CachedAdapters retry flow around adapterProbe so
that when both attempts fail to reach a stable fingerprint, it returns the
freshest adapters result instead of the “adapter probe did not settle” error,
and does not publish that result to adapterCache. Define and export a sentinel
error for this unstable-probe condition, and return it alongside the adapters so
callers can match it with errors.Is while still receiving valid adapter data.
In `@pkg/ai/availability_test.go`:
- Around line 60-66: Both Ginkgo specs must isolate adapterAuthProbe instead of
allowing OSAuthProbe to access host credentials. In pkg/ai/availability_test.go
lines 60-66, save and restore adapterAuthProbe in DeferCleanup and set it to
return fakeProbe(nil, nil, nil, hermeticHome) alongside the cache reset; apply
the same substitution in pkg/ai/catalog_disabled_ginkgo_test.go lines 113-119
before assigning adapterProbe.
In `@pkg/ai/catalog_resolve.go`:
- Around line 88-94: Update lockModelCache to accept context.Context and acquire
the file lock with non-blocking retries, checking ctx.Done() between attempts
and returning ctx.Err() after closing the lock file when cancelled or expired.
Preserve existing handling for successful acquisition and unexpected flock
errors, and update the ResolveModels call to pass ctx so contention falls back
through its existing uncached resolve path.
---
Nitpick comments:
In `@pkg/ai/adapters_cache_test.go`:
- Around line 270-277: Hoist the t.TempDir() call out of the adapterAuthProbe
closure and store its result for reuse on every fakeProbe creation. Keep the
closure’s capture counting and ProbeError assignment unchanged so repeated
captures share the same Home and isolate the recapture-error behavior.
In `@pkg/ai/adapters.go`:
- Around line 355-372: Update the fingerprint construction around
frozenCredentialState and stateFingerprint so raw credential tokens are never
placed in the marshaled state. Replace each Token value with an HMAC-derived
representation using the same machine key and established catalog-cache
mechanism, preserving deterministic output across processes and existing
non-secret credential metadata. Hash only the sanitized state and retain the
current SHA-256 fingerprint format.
In `@pkg/ai/catalog_resolve_test.go`:
- Around line 300-302: Rename the local credentials snapshot variable in the
test to avoid shadowing the imported credentials package, and capture the
temporary HOME value once after t.Setenv. Reuse that variable for the path
construction and file read around the affected test setup.
In `@pkg/ai/catalog_resolve.go`:
- Around line 83-86: Update the non-cacheable branch of the resolver around
resolveFresh and filterResolved to check err before filtering; return nil rows
with the error on failure, matching the cacheable path, and only call
filterResolved for successful results.
- Around line 298-306: Move the opts.UseTokens guard out of the
selectedAPIBackends loop in the surrounding token-resolution logic, checking it
before iteration and skipping the entire loop when disabled. Remove the
per-iteration break while preserving the existing backend filtering and token
resolution behavior.
In `@README.md`:
- Line 499: Update the model-cache documentation near the description of
`~/.config/captain/models/` to state that the legacy
`~/.config/captain/models.json` file is no longer read and may be safely
deleted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f1fafb75-3bbb-492a-8cb6-9b193c8acf3c
📒 Files selected for processing (14)
README.mdpkg/ai/adapter_models.gopkg/ai/adapters.gopkg/ai/adapters_cache.gopkg/ai/adapters_cache_test.gopkg/ai/adapters_test.gopkg/ai/availability_test.gopkg/ai/catalog_disabled_ginkgo_test.gopkg/ai/catalog_resolve.gopkg/ai/catalog_resolve_test.gopkg/ai/live_catalog_test.gopkg/ai/model_cache.gopkg/ai/models_remote.gopkg/ai/models_remote_test.go
Adapter cache hits rehashed OAuth files while holding the global cache lock, and repeated credential rewrites discarded otherwise usable probe results. Validate hits with cheap file metadata, return unsettled snapshots uncached through a sentinel, and isolate host auth in catalog tests. Make model-cache lock acquisition context-aware so a contended entry cannot outlive the caller deadline.
adityathebe
force-pushed
the
fix/ai-auth-consistent-model-cache
branch
from
August 12, 2026 04:54
c99eb3c to
03b31a9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #60.
Model discovery could report authentication from one credential while fetching and caching models with another. This change carries one immutable credential snapshot through the entire operation.
Model caches are now scoped by backend, endpoint, and a machine-keyed credential HMAC. Writes use secure permissions, atomic replacement, and per-entry locking. The adapter cache also invalidates immediately when credentials or local runtime identity change.
Tests cover credential substitution, rotation, concurrent providers, cache isolation, and permissions.
Summary by CodeRabbit
whoamidocumentation with model-cache locations, isolation, cache controls, and HTTP logging details.