internal/agent/model_config.go (1)

132-132: Consider adding a doc comment for valid ThinkingEffort values.

The field is correctly added with omitempty for optional serialization. For maintainability, consider adding a brief comment documenting the expected values (e.g., "low", "medium", "high" or similar) so future developers understand the valid range without needing to trace through the validation logic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agent/model_config.go` at line 132, Add a concise doc comment above
the ThinkingEffort struct field in model_config.go explaining the
expected/allowed string values (e.g., "low", "medium", "high" or whatever the
validation accepts) so future readers know the valid range without tracing
validation logic; reference the ThinkingEffort string field in the struct and
include note about omitempty and whether values are case-sensitive or mapped to
enums.

internal/agent/provider_cache_test.go (1)

25-25: Add one test for non-empty ProviderDeps path.

These updates correctly align callsites, but the suite still validates only ProviderDeps{}. Please add one case with populated deps (OAuth manager path) so the new dependency-based behavior is covered.

Also applies to: 29-29, 46-46, 62-62, 73-73, 99-99, 195-195, 252-252

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agent/provider_cache_test.go` at line 25, Add a test case that calls
cache.GetOrCreate with a non-empty ProviderDeps (e.g.,
ProviderDeps{OAuthManagerPath: "some/path"}) instead of only ProviderDeps{} so
the dependency-based path is exercised; update the test(s) that currently invoke
cache.GetOrCreate(cfg, ProviderDeps{}) to also call cache.GetOrCreate(cfg,
ProviderDeps{OAuthManagerPath: "<valid-or-temp-path>"}), then assert the same
expectations you have for p1/model1 (or the relevant returned values) to ensure
the OAuth-manager path branch is covered. Ensure you adjust all similar test
sites that currently use ProviderDeps{} (including other occurrences calling
cache.GetOrCreate, cfg, p1, model1) to include one non-empty deps case.

internal/agent/api_test.go (1)

1094-1109: Capture model-b requests too.

Line 1108 only receives a second request if SendMessage keeps reusing model-a's provider. Registering model-b with a capturing provider would keep this test focused on thinking-effort propagation instead of current provider-reuse semantics.

Suggested change

 reqCh := make(chan *llm.ChatRequest, 2)
 api, _ := testAPIWithModels(t, modelA, modelB)
 api.providers.Set(modelA.ToLLMConfig(), newCapturingProvider(reqCh, simpleStopResponse("a")))
-api.providers.Set(modelB.ToLLMConfig(), newStopProvider("b"))
+api.providers.Set(modelB.ToLLMConfig(), newCapturingProvider(reqCh, simpleStopResponse("b")))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agent/api_test.go` around lines 1094 - 1109, The test registers
modelB with newStopProvider so the second request isn't captured; update the
provider registration to use a capturing provider for modelB (e.g., call
api.providers.Set(modelB.ToLLMConfig(), newCapturingProvider(reqCh,
simpleStopResponse("b")))) so that the second request emitted by api.SendMessage
is received on reqCh; look for the reqCh, testAPIWithModels, api.providers.Set,
newCapturingProvider/newStopProvider, CreateSession and SendMessage calls to
make the change.

internal/cmd/context.go (1)

576-591: Extract the encrypted OAuth-store bootstrap into one helper.

This key → encryptor → store sequence is now duplicated here, in buildRemoteNodeResolver, and again in internal/service/worker/remote_handler.go. Centralizing it would keep the directory layout and warning behavior from drifting.

Possible extraction

+func newAgentOAuthManager(dataDir string) (*agentoauth.Manager, error) {
+	encKey, err := crypto.ResolveKey(dataDir)
+	if err != nil {
+		return nil, err
+	}
+	enc, err := crypto.NewEncryptor(encKey)
+	if err != nil {
+		return nil, err
+	}
+	store, err := fileagentoauth.New(filepath.Join(dataDir, "agent", "oauth"), enc)
+	if err != nil {
+		return nil, err
+	}
+	return agentoauth.NewManager(store), nil
+}
+
 	encKey, err := crypto.ResolveKey(c.Config.Paths.DataDir)
 	if err != nil {
 		logger.Warn(c, "Failed to resolve encryption key for agent OAuth store", tag.Error(err))
 	} else {
-		enc, err := crypto.NewEncryptor(encKey)
-		if err != nil {
-			logger.Warn(c, "Failed to create encryptor for agent OAuth store", tag.Error(err))
-		} else {
-			store, err := fileagentoauth.New(filepath.Join(c.Config.Paths.DataDir, "agent", "oauth"), enc)
-			if err != nil {
-				logger.Warn(c, "Failed to create agent OAuth store", tag.Error(err))
-			} else {
-				result.OAuthManager = agentoauth.NewManager(store)
-			}
-		}
+		manager, err := newAgentOAuthManager(c.Config.Paths.DataDir)
+		if err != nil {
+			logger.Warn(c, "Failed to create agent OAuth manager", tag.Error(err))
+		} else {
+			result.OAuthManager = manager
+		}
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/cmd/context.go` around lines 576 - 591, Extract the repeated
key→encryptor→store bootstrapping into a single helper (e.g.,
bootstrapAgentOAuthStore or NewAgentOAuthManager) that encapsulates
crypto.ResolveKey, crypto.NewEncryptor, fileagentoauth.New and returns the
created agentoauth.Manager (or the store plus manager) and any error; replace
the inline sequences in context.go (where result.OAuthManager is set) and in
buildRemoteNodeResolver and internal/service/worker/remote_handler.go to call
this helper, preserving the same filepath.Join(c.Config.Paths.DataDir, "agent",
"oauth") layout and the existing logger.Warn messages (use the same messages and
tag.Error(err) when bubbling warnings) so behavior and logs remain identical.

internal/agentoauth/openai_codex.go (1)

154-170: Consider adding timeout to HTTP client for token exchange.

Using http.DefaultClient without a timeout could cause indefinite hangs if the OpenAI auth server is unresponsive. Consider using a client with a configured timeout (e.g., 30 seconds).

♻️ Proposed fix to add timeout

+var tokenClient = &http.Client{
+	Timeout: 30 * time.Second,
+}
+
 func exchangeToken(ctx context.Context, form url.Values) (*Credential, error) {
 	req, err := http.NewRequestWithContext(ctx, http.MethodPost, openAICodexTokenURL, strings.NewReader(form.Encode()))
 	if err != nil {
 		return nil, fmt.Errorf("create token request: %w", err)
 	}
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")

-	resp, err := http.DefaultClient.Do(req)
+	resp, err := tokenClient.Do(req)
 	if err != nil {
 		return nil, fmt.Errorf("exchange OAuth token: %w", err)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agentoauth/openai_codex.go` around lines 154 - 170, The
exchangeToken function uses http.DefaultClient which has no timeout and can
hang; replace the HTTP call to use a dedicated http.Client with a timeout (e.g.,
30s) instead of http.DefaultClient.Do; update the call in exchangeToken where
resp, err := http.DefaultClient.Do(req) to use a locally constructed client (or
a shared client variable) with Timeout set so the token exchange is bounded,
keeping all other behavior (headers, body read/close, and status handling) the
same and still using the existing openAICodexTokenURL and ctx.

internal/persis/fileagentoauth/store.go (1)

219-228: Consider logging time parse failures.

Failed time parsing is silently ignored, leaving zero-value timestamps. While this is safe, logging a warning could help diagnose corrupted credential files.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/persis/fileagentoauth/store.go` around lines 219 - 228, When
parseTime fails for stored.ExpiresAt or stored.UpdatedAt the error is currently
ignored; update the blocks that call parseTime (around stored.ExpiresAt and
stored.UpdatedAt) to log a warning including the field name, the raw string
(stored.ExpiresAt / stored.UpdatedAt) and the parse error before leaving the
timestamp as the zero value; keep the existing behavior of only assigning
cred.ExpiresAt / cred.UpdatedAt on successful parse. Use the project's logging
mechanism (or fmt/log if none exists) to emit the warning so parse failures are
visible for debugging.

internal/agentoauth/manager.go (1)

174-176: Minor: Consider iterating over m.providers keys instead of hardcoded list.

Line 175 uses a hardcoded []string{ProviderOpenAICodex} while m.providers already contains the registered providers. If providers are added/removed, this list must be updated manually.

Suggested improvement

 func (m *Manager) Status(ctx context.Context) ([]ProviderStatus, error) {
 	statuses := make([]ProviderStatus, 0, len(m.providers))
-	for _, provider := range []string{ProviderOpenAICodex} {
+	for provider := range m.providers {
 		prov, err := m.lookupProvider(provider)

Note: If ordering matters, you may want to sort the keys or maintain a separate ordered list.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/agentoauth/manager.go` around lines 174 - 176, The loop currently
iterates a hardcoded slice []string{ProviderOpenAICodex}; change it to iterate
the keys of m.providers so new providers are handled automatically: build a
slice of provider names from the map m.providers (optionally sort it if
deterministic order is required) and then call m.lookupProvider for each name to
populate statuses (types: ProviderStatus, variables: statuses, provider, prov,
err, method: lookupProvider). Ensure the code replaces the literal
ProviderOpenAICodex usage with the dynamic key iteration.

internal/llm/providers/openaicodex/codex.go (1)

238-241: Consider logging JSON parse errors for debugging.

Silent continue on unmarshal errors (line 240) could hide malformed events from Codex. While skipping unknown data is reasonable, logging at debug level would help diagnose issues in production.

Suggested improvement

 		var event responseEvent
 		if err := json.Unmarshal([]byte(data), &event); err != nil {
+			// Log at debug level to aid troubleshooting without noise
+			// logger.Debug(ctx, "skipping unparseable SSE event", "error", err, "data", data)
 			continue
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/llm/providers/openaicodex/codex.go` around lines 238 - 241, The JSON
unmarshal error is being swallowed when parsing Codex events; update the block
around json.Unmarshal([]byte(data), &event) (responseEvent/event) to log the
error at debug level including the error and the raw data before continuing
(e.g., call the package's debug logger with a message like "failed to unmarshal
Codex response" plus err and data) so malformed events are visible for
troubleshooting, then continue as before.

internal/service/frontend/api/v1/agent_auth.go (1)

85-97: Consider using returned credential to avoid extra store lookup.

The credential returned by CompleteLogin (line 85) is discarded, then providerStatus (line 94) re-fetches it from the store. While functionally correct, this creates redundant I/O.

Possible optimization

If ProviderStatus can be constructed from the credential, consider building the response directly:

cred, err := a.agentOAuthManager.CompleteLogin(...)
if err != nil {
    return nil, toAgentAuthError(ctx, "complete login", err)
}
// Build status from cred instead of re-fetching

However, if providerStatus includes additional data beyond the credential, the current approach is fine.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/frontend/api/v1/agent_auth.go` around lines 85 - 97,
CompleteLogin currently returns a credential that is ignored and then
providerStatus re-reads the store, causing redundant I/O; change the flow in the
handler to capture the returned credential from
a.agentOAuthManager.CompleteLogin (e.g., store it in a local variable like cred)
and use that credential to construct the ProviderStatus response directly (or
add a helper that builds providerStatus from the credential) instead of calling
providerStatus(ctx, request.ProviderId) again; if providerStatus truly requires
extra store-derived fields, update or add a function (e.g.,
providerStatusFromCredential or augmentProviderStatus) that merges cred with the
additional data to avoid a full duplicate lookup.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes