Skip to content

feat: add configurable auto model modes - #389

Merged
atishpatel merged 22 commits into
mainfrom
atish/auto-model-modes
Oct 19, 2026
Merged

atishpatel merged 22 commits into
mainfrom
atish/auto-model-modes

Conversation

@atishpatel

@atishpatel atishpatel commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Add configurable Auto modes so each new prompt can choose a model and reasoning effort. The picker lists modes first with branching-arrow icons, shows the concrete model, and offers expandable routing details. Steering keeps the current model; choosing a concrete model exits Auto.

goose.autoRouting shares the endpoint, timeout, optional bearer-token environment variable, and optional route selector. Each mode defines its candidates, effort bounds, and fallback. An optional default mode applies to new chats. Public setup examples use Databricks.

Routing allows 24,000 characters each for task text, loaded skill instructions, each file preview, and recent agent-visible conversation. Replay coordination and hidden transcript records are excluded. Requests run through the native backend; failed or invalid selections use the configured fallback. Queue coordination preserves explicit model choices when a routing response arrives late. Full bounded responses remain in transient UI details only. Logs record transport metadata, and feedback exports omit legacy logs containing raw responses. Removed Auto modes clear stale intent so existing chats can continue on their concrete model. Failed config loads and built-in defaults preserve Auto intent until an authoritative configuration confirms removal.

New chats refresh runtime config before choosing their initial model, including reused drafts and CLI creation. Refresh failures retain the last valid source config or usable built-in defaults. New chats wait for provider endpoint and catalog updates before selecting their target. Native-issued routing revisions avoid JSON number conversion errors. Detached windows resync stale configuration before retrying, and native checks reject responses whose configuration changed during the request. Overlapping initial loads preserve successful results without letting an older load overwrite the newer cache. This supplies the refresh boundary for future distribution-owned LaunchDarkly overrides; Auto routing remains file-configured for now. The queue behavior continues to follow LAWS/CHAT.md.

Related issue

None found in the existing issues or PRs.

Testing

Rebuilt and export-verified the native distribution at 792c81d. Startup and new-chat creation succeeded; Auto Balanced completed a synthetic prompt using GLM Flash 5.3/high. The overlapping-load race is covered by a deterministic regression that holds the newer load pending while the older load completes.

Built and export-verified the native macOS distribution at 9c775f0. Auto Balanced completed a synthetic prompt with GLM Flash 5.3/high. Renaming an Auto mode in the disposable bundled config appeared on the next new chat, which completed another routed prompt. The original config was restored byte-for-byte. Numeric precision, stale-window recovery, provider synchronization, and refresh races have regression coverage.

Earlier live tests at e8c1778 passed Balanced and Premium routing, fresh-prompt rerouting, expandable rationale, steering with the same model, and manual selection exiting Auto. A temporary bundled-config change appeared only after a new chat. Invalid JSON retained the last valid config and completed inference. An unreachable router used the configured fallback and displayed its reason. The original config was restored byte-for-byte. Screenshots below show 4d03af0.

Screenshots

Native distribution UI at 4d03af0, with the sidebar collapsed and synthetic prompts.

Before: concrete models After: Auto modes first
Before: concrete model picker After: Balanced and Premium Auto modes

Completed live prompt with the selected model visible:

Auto Balanced using GLM Flash 5.3 after a completed prompt

Generated with Codex

Comment thread src/features/chat/lib/autoRoutingPrompt.ts Outdated
@atishpatel

Copy link
Copy Markdown
Contributor Author

Automated review

NEEDS HUMAN REVIEW — berdctl sends into an Auto session deadlock on their own dispatch lease (sessionSend.ts passes no sessionSelectionToken), locking that session until restart; reviewed f98c9ae. (per amp)

From amp:

F1 — [BLOCKER] berdctl sends into an Auto session deadlock on their own dispatch lease

Location:

const outcome = input.dispatchToken
? await transitionSessionDispatchTarget({
...request,
dispatchToken: input.dispatchToken,
})
: await transitionSessionTarget(request);
and
const targetLease = acquisition;
let dispatched = false;
let resolveDispatch: (() => void) | undefined;
let rejectDispatch: ((error: unknown) => void) | undefined;
const dispatch = options.returnOnDispatch
? new Promise<void>((resolve, reject) => {
resolveDispatch = resolve;
rejectDispatch = reject;
})
: null;
const settlement = (async () => {
try {
const { providerId, persona } =
await prepareExistingSessionForBackgroundSend(sessionId, {
executionTarget: targetLease.target,
dispatchToken: targetLease.token,
});
const session = useChatSessionStore.getState().getSession(sessionId);
options.validateHydratedTranscript?.();
await sendPromptInBackground(
sessionId,
prompt,
providerId,
persona,
{
...(options.sendOptions ?? berdctlCrossSessionSendOptions()),
systemPrompt: session
? formatIncludedWorkspacesPrompt(session)
: undefined,
},
undefined,
beforeUserMessageCommitted,
undefined,
undefined,
() => {
dispatched = true;
resolveDispatch?.();
},
);
} catch (error) {
if (!dispatched) rejectDispatch?.(error);
throw error;
} finally {
targetLease.release();

Trigger: An agent runs sessions send or notify against an existing session that has autoModeId set. This happens whenever the user picked Auto, or whenever defaultModeId applies to UI-created chats. The path also covers the queued drain in useBerdctlQueuedMessageDrain.ts.

How it deadlocks:

  1. sendPromptToExistingSessionInBackground acquires a dispatch lease.
  2. It passes targetLease.token only to prepareExistingSessionForBackgroundSend. The sendOptions it hands to sendPromptInBackground come from berdctlCrossSessionSendOptions and carry no sessionSelectionToken, so dispatchPrompt receives dispatchToken: undefined.
  3. After the router returns, prepareAutoRoutedPrompt takes the tokenless branch and calls transitionSessionTarget(request).
  4. That call sees a foreign actor.dispatch and awaits actor.dispatchReleased (sessionTargetCoordinator.ts L798-L800).
  5. The lease is released only in the finally block, which runs after sendPromptInBackground settles. Each side waits on the other, and no timeout exists.

Impact: The berdctl command never returns. Every later queued or berdctl send to that session sees contended until the app restarts. The queued paths in queuedSessionSend.ts and useMessageQueue.ts were updated to pass the token, but this one was missed.

Fix direction: Pass sessionSelectionToken: targetLease.token into the send options in sessionSend.ts. Also consider having prepareAutoRoutedPrompt fail fast when it has no token and a foreign lease exists, instead of waiting.

F2 — [WARNING] On the home composer, choosing an Auto mode can leave Auto off or turn it off

Location:

onModeChange: (modeId: string) => {
const target = getAutoModeExecutionTarget(modeId);
if (!target) return;
const accepted = handleModelChange(
target.modelId,
{
id: target.modelId,
name: target.modelName,
providerId: target.modelProviderId,
},
"goose",
);
if (
accepted === false &&
!sameSessionExecutionTarget(session?.executionTarget, target)
)
return;
selectAutoMode(sessionId, modeId);
},

Trigger: No session exists yet, and the effective model already equals the mode's fallback.modelId.

What happens:

  1. onModeChange calls handleModelChange with "goose", which calls onModelSelected directly.
  2. onModelSelected first runs selectAutoMode(null, null). It then hits the "same model" early return (L564-L571) and returns false.
  3. With session undefined, sameSessionExecutionTarget(undefined, target) is false. onModeChange therefore returns before selectAutoMode(null, modeId).

Impact: The user cannot enable Auto from the home screen in this state. When defaultModeId is set, clicking the already-selected mode switches Auto off.

Fix direction: When there is no session, treat a same-model no-op as accepted. Alternatively, set the pending mode without going through the path that clears it.

F3 — [WARNING] An unavailable routed model fails the send instead of using the configured fallback

Location:

i18n.t("chat:errors.sessionPreparationSuperseded"),
);
}
// Provider inventory repair may substitute a different model. Never send
// the task (or stamp a misleading routing decision) on that substitute.
if (
outcome.target.modelId !== decision.modelId ||
outcome.target.modelProviderId !== decision.modelProviderId ||
outcome.target.harnessId !== "goose"
) {
throw new Error(i18n.t("chat:errors.autoModelUnavailable"));

Trigger: The router returns a configured candidate that the provider does not currently serve, so inventory repair substitutes another model.

What happens: By the time of the check, the target transition has already been applied. The session therefore sits on a substitute model that neither the user nor the router chose. The code then throws autoModelUnavailable without retrying mode.fallback. While the router keeps choosing that candidate, every fresh prompt fails the same way. This contradicts the documented "failed or invalid selections use the configured fallback."

Fix direction: Retry once with autoRoutingFallback(mode, ...), and throw only if the fallback is also unavailable.

F4 — [WARNING] Choosing a persona with a pinned model does not exit Auto

Location:

const session = useChatSessionStore.getState().getSession(sessionId);
const modeId = session?.autoModeId;
if (!modeId || session.executionTarget?.harnessId !== "goose") return;
const config = useRuntimeConfigStore.getState().config?.goose.autoRouting;
const mode = config?.modes.find((candidate) => candidate.id === modeId);
if (!mode) throw new Error(i18n.t("chat:errors.autoModeMissing"));

What happens: handlePersonaChange in useChatSessionController.ts never clears autoModeId.

  • If the persona pins a goose model, the next prompt is re-routed and the persona's explicit model is overwritten.
  • If the persona targets a non-goose harness, routing silently stops (the harnessId !== "goose" early return), but the picker still shows the Auto mode.

Fix direction: Call selectAutoMode(sessionId, null) when a persona change carries a model or harness. This matches the stated contract that choosing a concrete model exits Auto.

F5 — [WARNING] Full router responses are logged in release builds and included in feedback log exports

Location:

log::debug!(target: "auto_routing", "Router response for mode {}: {}", mode.id, String::from_utf8_lossy(&bytes));
and
.level_for("auto_routing", log::LevelFilter::Debug)

What happens: level_for("auto_routing", Debug) applies to every build. Each response, up to 1 MiB, is written to berd.log before it is validated. The new docs state that the rationale and evidence can quote task context, which includes attachment previews and conversation.

Why it matters: log_export.rs promises that opt-in feedback zips drop lines that can carry conversation or LLM content. However, sanitize_app_log_line drops only goose stdout/stderr capture lines, so these auto_routing lines are exported. Large responses can also rotate other diagnostics out of the 10 MB log quickly.

Fix direction: Log only the selection, source, and a truncated rationale at Info level, and keep the full response in memory. Alternatively, filter the auto_routing target in sanitize_app_log_line.

F6 — [WARNING] The routing request cannot be cancelled and holds the send/queue lease for up to timeoutMs

Location:

export async function routeAutoModel({
modeId,
prompt,
}: {
modeId: string;
prompt: string;
}): Promise<AutoRoutingResult> {
const config = useRuntimeConfigStore.getState().config.goose.autoRouting;
const mode = config?.modes.find((entry) => entry.id === modeId);
if (!mode) throw new Error("Auto routing mode is not configured");
try {
// The backend resolves URL, selector and candidate list from trusted runtime config.
const response = await invokeWithStartupRetry<unknown>("route_auto_model", {
modeId,
prompt,
});
return selectAutoRoutingResponse(mode, response);
} catch {
// Error messages may contain endpoint details; diagnostics use stable local codes.
return autoRoutingFallback(mode, "request_failed");
}

What happens: routeAutoModel takes no AbortSignal. assertCurrent() checks the abort only after the native invoke resolves.

Impact: When the router is slow or unreachable, Stop has no effect, and queued turns or picker changes behind the dispatch lease stall until the timeout. The timeout defaults to 5 s, and config allows up to 120 s. No duplicate send results, because the abort is honored before commit.

Fix direction: Race the invoke against signal so the lease releases immediately. Consider lowering the maximum timeout, since this call sits on the critical send path.

From codex:

F1 — [WARNING] Forward the dispatch lease token for Berdctl background sends

Location: autoRoutingDispatch.ts:73–78

Sending to an existing Goose Auto session through sessions.send can deadlock before dispatch. sendPromptToExistingSessionInBackground acquires a dispatch lease but omits sessionSelectionToken from its background-send options. Routing consequently calls transitionSessionTarget without the token and waits for that lease, whose owner releases it only after the send completes. Router fallback follows the same path. The retained lease also blocks subsequent coordinated sends and model changes in that session until recovery such as reloading the app. Forward targetLease.token from sessionSend.ts, as the updated queued-send path already does.

F2 — [WARNING] Exclude raw router responses from feedback log attachments

Location: auto_routing.rs:97

The complete router response is written to berd.log, with debug logging explicitly enabled. Rationale and evidence can quote prompts, conversation history, or attached-file contents. Feedback log export includes this file, while sanitize_app_log_line excludes only Goose stdout/stderr records and otherwise redacts recognized credential keys. Ordinary quoted task content therefore survives and is uploaded when users attach diagnostics, despite the dialog promising that prompts and responses are excluded. Keep raw responses in a non-exported diagnostic store, or exclude complete router-response records—including multiline bodies—from export.

F3 — [WARNING] Roll back Auto intent when fallback activation fails

Location: useResolvedAgentModelPicker.ts:858–872

handleModelChange reports synchronous acceptance while backend preparation continues asynchronously, and this callback immediately persists the Auto mode. If a user switches from a foreign harness to Auto and preparation fails, the existing error handler restores the previous execution target without restoring Auto intent. The picker continues displaying Auto, but prepareAutoRoutedPrompt silently skips non-Goose targets, so subsequent prompts use the restored concrete model without routing. Commit Auto intent with successful activation, or restore the previous intent when that activation rolls back.

Reviewed at commit f98c9ae.

@atishpatel
atishpatel marked this pull request as ready for review October 18, 2026 19:16
@atishpatel
atishpatel requested a review from a team October 18, 2026 19:16
Treat the home composer's same-fallback selection as accepted Auto intent.\nRestore the previous Auto mode and diagnostic if fallback activation fails,\nwhile preserving newer selections. These fixes share the activation path.

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

REQUEST_CHANGES. Fresh static Engineering review of the exact three-dot comparison 180fe23...4d03af0 found two blocking changed-line defects: routing context can disclose agent-hidden transcript text to the configured routing service, and persisted Auto selections become send-blocking when a distribution removes or renames a mode. The supplied GitHub evidence was inspected and is structurally valid for the exact head; all listed checks completed successfully, though required checks still independently govern merge readiness. Final self-check covered every changed user flow; accessibility; i18n/localization completeness; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; design-system/project rules; duplicate and overlapping findings; and concrete evidence plus user effect for blocking severity.

Deterministic publication result: 2 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Comment thread src/features/chat/lib/autoRoutingPrompt.ts Outdated
Comment thread src/features/chat/lib/autoRoutingDispatch.ts Outdated

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

REQUEST_CHANGES. Fresh static Engineering review of the exact three-dot comparison 180fe23...70a8603 found one blocking changed-line defect: a transient unavailable runtime-config result is treated as authoritative absence and permanently clears a restored chat's Auto intent. The two prior automation findings were rechecked: agent-hidden transcript content is now excluded from routing history, and genuinely removed or renamed modes recover to the concrete target. The supplied GitHub evidence is structurally valid for the exact head; several checks pass and the frontend and platform Rust checks remain in progress, while required checks independently govern merge readiness. Final self-check covered every changed user flow; accessibility; i18n/localization completeness; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; design-system/project rules; duplicate and overlapping findings; and concrete evidence plus user effect for blocking severity.

Deterministic publication result: 1 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Comment thread src/features/chat/lib/autoRoutingDispatch.ts Outdated
morgmart
morgmart previously approved these changes Oct 19, 2026

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

APPROVE. Fresh static Engineering review of the full exact three-dot comparison 180fe23...d2313ef found no publishable changed-line defects. All three prior automated blockers were rechecked and are fixed: agent-hidden and replay-coordination transcript content is excluded before routing history is selected; genuinely removed or renamed Auto modes recover to the concrete target; and unavailable runtime configuration now preserves persisted Auto intent until a ready configuration is authoritative. The changed regression tests discriminate each corrected behavior. The supplied GitHub evidence is structurally valid for the exact head and all listed checks completed successfully, though required checks still independently govern merge readiness. Final self-check covered every changed user flow; accessibility; i18n/localization completeness; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; design-system/project rules; duplicate and overlapping findings; and concrete evidence plus user effect for blocking severity.

Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Pending checks: 1 check(s) are not complete.

This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.

morgmart
morgmart previously approved these changes Oct 19, 2026

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

APPROVE. Fresh static Engineering review of the full exact three-dot comparison 180fe23...e8c1778 found no publishable changed-line defects. The new-chat runtime-config refresh, last-valid fallback, renderer/native snapshot agreement, UI and CLI creation paths, queue dispatch, routing transport, and persisted Auto recovery were traced through success, unavailable, timeout, supersession, and concurrent-request behavior. All prior automated blockers remain fixed: hidden replay content is excluded, removed modes recover safely, and failed config loads preserve Auto intent. The supplied GitHub evidence is structurally valid for the exact head and all listed checks completed successfully, though required checks still independently govern merge readiness. Final self-check covered every changed user flow; accessibility; i18n/localization completeness; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; design-system/project rules; duplicate and overlapping findings; and concrete evidence plus user effect for blocking severity.

Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Pending checks: 1 check(s) are not complete.

This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.

@atishpatel

Copy link
Copy Markdown
Contributor Author

Automated review

APPROVE WITH CAUTION — Reviewed e8c1778; prior blockers are fixed, and two nonblocking defects remain: numeric routeSelector.config values fail the native config-equality guard (WARNING), and a failed new-chat refresh downgrades an app-default config to unavailable (INFO). (per amp)

From amp:

F1 — [WARNING] Whole-number floats in routeSelector.config fail the config-equality guard, so every Auto send is rejected

Location:

fn checked_routing_config<'a>(
current: Option<&'a RuntimeAutoRoutingConfig>,
expected: &RuntimeAutoRoutingConfig,
) -> Result<&'a RuntimeAutoRoutingConfig, String> {
current
.filter(|current| *current == expected)
.ok_or_else(|| AUTO_ROUTING_CONFIG_CHANGED.to_string())
}
and
invokeWithStartupRetry<unknown>("route_auto_model", {
modeId,
prompt,
expectedAutoRouting: config,
}),
signal,
);
return selectAutoRoutingResponse(mode, response);
} catch (error) {
if (signal?.aborted) throw new DOMException("Aborted", "AbortError");
if (
(error instanceof Error ? error.message : error) ===
AUTO_ROUTING_CONFIG_CHANGED
) {
throw new PreCommitSendRejectedError(
i18n.t("chat:errors.sessionPreparationSuperseded"),
);
}

routeSelector.config accepts free-form JSON (a serde_json::Map in Rust, z.json() in the schema), and route_auto_model compares the renderer's echoed snapshot to the native config with derived PartialEq. A config such as "config": {"temperature": 1.0} breaks that comparison:

  1. Native serializes the value as 1.0.
  2. JavaScript parses it as 1 and sends back 1.
  3. serde_json reads 1 as an integer, and an integer 1 does not equal the float 1.0.

Every attempt then returns AUTO_ROUTING_CONFIG_CHANGED before any network call. The renderer reports that session preparation was superseded. The queue retries until its rejection limit and leaves the message queued. Auto cannot send with that configuration, and the error does not show the cause. Integers above 2^53 fail the same way. The documented examples use only strings, so the tests do not cover this.

Fix: compare an opaque native-issued config revision or digest instead of the round-tripped struct. Alternatively, canonicalize numbers before comparing, or reject non-integer and out-of-range numbers in validate_router_json and the matching Zod schema.

F2 — [INFO] A failed new-chat refresh replaces a running app-default config with unavailable

Location:

fn finish_load(
&self,
generation: u64,
result: RuntimeConfigLoadResult,
retain_last_valid: bool,
) -> Result<RuntimeConfigLoadResult, String> {
let result = runtime_config_load_result_for_local_byo_dev(result);
let mut cached = self
.cached
.lock()
.map_err(|_| "Runtime config cache lock poisoned".to_string())?;
if generation != cached.generation {
return Ok(cached.current.clone().unwrap_or_else(|| {
runtime_config_refresh_unavailable(
"A newer runtime config refresh is pending".to_string(),
)
}));
}
let result =
if retain_last_valid && matches!(result, RuntimeConfigLoadResult::Unavailable { .. }) {
cached.last_valid.clone().unwrap_or(result)
} else {
result
};
if matches!(&result, RuntimeConfigLoadResult::Ready { source, .. } if *source != RuntimeConfigSource::AppDefault)
{
cached.last_valid = Some(result.clone());
}
cached.current = Some(result.clone());
Ok(result)

This affects admin-runtime-config builds, and debug builds without a bundled config. When the endpoint was unreachable at startup and no cache existed, the app starts on AppDefault. refresh_for_new_chat passes allow_default_fallback = false, and last_valid never records AppDefault. If the source is still unreachable, opening a new chat makes the native current config Unavailable. After that, every ready_config caller fails, including Goose spawn (goose_serve.rs runtime_config_for_spawn), connections, diagnostics, whoami and automations. Before this change, those features kept running on the defaults.

The test new_chat_initial_source_failure_does_not_prove_auto_was_removed asserts this outcome deliberately for Auto. The effect on unrelated ready_config callers looks unintended. The feature is off by default, so impact is limited.

Fix: when no valid source config exists, keep AppDefault as current and report it to the Auto logic as non-authoritative.

From codex:

F1 — [WARNING] Refresh stale window configuration before retrying routing

Location: autoRouting.ts:174–180

Keep an Auto chat open in a detached window, change routing settings at their source, then create a new chat in the main window. The refresh updates the shared native configuration, but the detached window retains its own older Zustand snapshot. Native routing rejects that snapshot with AUTO_ROUTING_CONFIG_CHANGED; this handler rejects the send without fetching the current configuration. Every retry consequently submits the same stale snapshot, eventually exhausting the queue’s retry limit and leaving the message queued. Reloading the window or leaving Auto provides a workaround. Fetch the effective native configuration on mismatch before retrying, or synchronize configuration updates across windows.

F2 — [WARNING] Apply refreshed provider settings before resolving new-chat targets

Location: newChatRuntimeConfig.ts:7–8

The new-chat refresh loads the complete runtime configuration but only updates the config store. If the source changes an existing custom provider’s apiUrl, new chats adopt the refreshed routing configuration while Goose retains the provider’s old endpoint. Provider reconciliation requires applyRuntimeProviderConfig, which updates the catalog/model inventory and calls syncRuntimeCustomProviders; its existing callers are startup and settings, neither of which runs at this boundary. A newly configured custom provider likewise remains unregistered. Apply the effective provider configuration before resolving the new chat’s target, retaining the refresh concurrency protections.

Reviewed at commit e8c1778.

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

REQUEST_CHANGES. Fresh static Engineering review of the full exact three-dot comparison 180fe23...9c775f0 found one blocking changed-line lifecycle defect: when an older runtime-config load finishes while a newer generation is still pending and no current value exists, the older caller receives a synthetic unavailable result even if its own source read succeeded. That can make startup or native Auto routing fail or fall back during a valid concurrent refresh. Existing overlap tests cover a newer result that has already committed, not this pending gap. All prior automated blockers remain fixed. The supplied GitHub evidence is structurally valid for the exact head; the macOS Tauri Rust check failed, two checks remain in progress, and the other listed checks passed. Required checks independently govern merge readiness. Final self-check covered every changed user flow; accessibility; i18n/localization completeness; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; design-system/project rules; duplicate and overlapping findings; and concrete evidence plus user effect for blocking severity.

Deterministic publication result: 1 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Comment thread src-tauri/src/commands/runtime_config.rs Outdated
@atishpatel

Copy link
Copy Markdown
Contributor Author

Automated review

APPROVE WITH CAUTION — Reviewed 9c775f0; no high-severity defects; medium findings remain on bearerTokenEnv secret scope, overlapping native config loads (unresolved thread), provider sync blocking new chats, and effort-bound send failures. (per amp)

From amp:

F1 — [WARNING] Runtime config can send any process environment variable as a bearer token to any http(s) URL

Location:

validate_runtime_id(&config.url, "autoRouting.url")?;
let url = reqwest::Url::parse(&config.url)
.map_err(|_| "autoRouting url must be valid".to_string())?;
if !matches!(url.scheme(), "http" | "https")
|| !url.username().is_empty()
|| url.password().is_some()
|| url.query().is_some()
|| url.fragment().is_some()
{
return Err(
"autoRouting url must be http or https without credentials, query or fragment"
.to_string(),
);
}
if let Some(name) = &config.bearer_token_env {
let mut chars = name.chars();
if !chars
.next()
.is_some_and(|c| c.is_ascii_uppercase() || c == '_')
|| !chars.all(|c| c.is_ascii_uppercase() || c.is_ascii_digit() || c == '_')
{
return Err(
"autoRouting bearerTokenEnv must be an environment variable name".to_string(),
);
}
}
and
.json(&build_request(config, mode, prompt));
if let Some(name) = &config.bearer_token_env {
let token =
std::env::var(name).map_err(|_| "Routing credential unavailable".to_string())?;
if token.trim().is_empty() {
return Err("Routing credential unavailable".to_string());
}
request = request.bearer_auth(token);

Until now, runtime config could not choose or carry secrets. endpointEnv is limited to ALLOWED_ENDPOINT_ENV_KEYS (DATABRICKS_HOST), custom providers must set requiresAuth: false, and their headers go through validate_non_secret_map. The new bearerTokenEnv is checked only against the pattern [A-Z_][A-Z0-9_]*. Its value is then read with std::env::var and attached with bearer_auth to a request to autoRouting.url, which may use plain http. A remotely served or tampered admin config could therefore set bearerTokenEnv: "GITHUB_TOKEN" (or an AWS or OpenAI key) together with defaultModeId. The first prompt in any new chat would then send that credential off the machine, possibly unencrypted. Restrict bearerTokenEnv to an allowlist (for example DATABRICKS_TOKEN or a Berd-prefixed name) and require https whenever a token is attached.

F2 — [WARNING] Overlapping native config loads can return a false Unavailable result (unresolved thread)

Location:

if generation != cached.generation {
return Ok(cached.current.clone().unwrap_or_else(|| {
runtime_config_refresh_unavailable(
"A newer runtime config refresh is pending".to_string(),
)
}));

This is still present at this head, as described in #389 (comment). get() now increments generation. When the cache is still empty, a superseded load therefore returns Unavailable("A newer runtime config refresh is pending") and throws away its valid result. Before this PR, each load replaced the cache independently. A concrete impact: if the goose serve spawn's ready_config is the superseded caller, goose_serve.rs#L227-L230 logs a warning and starts goose without the runtime provider environment for the life of that process. The window is the first load, which can last up to the 5 s admin fetch timeout. When a superseded load finds no committed value, return its own completed result without caching it.

F3 — [WARNING] A custom-provider sync failure now blocks all new chat creation, and every new chat reseeds the model cache

Location:

export async function reconcileRuntimeConfigProviders(): Promise<RuntimeConfigLoadResult> {
for (;;) {
const { result, config } = useRuntimeConfigStore.getState();
await applyRuntimeProviderConfig(config, {
defaultModelInventoryMode: defaultModelInventoryModeForLoadResult(result),
requireBackendSync: true,
isCurrent: () => useRuntimeConfigStore.getState().result === result,
});
if (useRuntimeConfigStore.getState().result === result) return result;
}
and
await syncRuntimeCustomProviders(runtimeConfig, { isCurrent });
if (isCurrent()) providerBackendSyncFailure = undefined;
} catch (error) {
if (!isCurrent()) return;
providerBackendSyncFailure = { generation, error };
if (options.requireBackendSync) throw error;
console.warn("Failed to sync runtime custom providers:", error);
}
if (!isCurrent()) return;
const byoKeyProvidersEnabled =
options.byoKeyProvidersEnabled ??
getBuildFeatureState().byoKeyProviders;
const catalogStore = useProviderCatalogStore.getState();
// With bring-your-own-key off, the runtime config is the sole catalog source
// and wholesale-replaces the store (the pre-feature behavior). With it on,
// merge so the fields-bearing openai/anthropic setup-catalog entries survive
// every (re-)apply; the runtime config still wins for everything it defines.
catalogStore.setEntries(
byoKeyProvidersEnabled
? mergeRuntimeProviderCatalog(catalogStore.entries, runtimeConfig)
: providerCatalogFromRuntimeConfig(runtimeConfig),
);
useProviderModelCacheStore
.getState()
.seedRuntimeModels(runtimeModelInventory(runtimeConfig), {
fresh: options.seedModelsFresh,
runtimeManagedProviderIds: runtimeManagedModelProviderIds(
runtimeConfig,
options.defaultModelInventoryMode,
),
});
});
providerApplyTail = pending;
return (async () => {
try {

refreshRuntimeConfigForNewChat calls applyRuntimeProviderConfig with requireBackendSync: true. All new-tab, project-draft, and remote-chat paths in AppShell call it, and so do berdctl createSession and useChatSessionController. If GooseUnstableProvidersCustomRead or Update persistently fails with an error other than NotFound for a configured customProvider, every chat creation rejects. That includes non-Auto chats and other harnesses. Previously the same failure was a console.warn at startup and settings time. Separately, each new chat now runs seedRuntimeModels. For runtime providers that are not runtime-managed, that replaces the fetched model list with the static configured list, resets fetchedAt to 0, and cancels in-flight refreshes through bumpRefreshVersion. Require backend sync only when the new chat targets the affected provider, or when the config revision changed since the last successful apply. Skip reseeding when the config is unchanged.

F4 — [WARNING] Effort selection fails the send when no session option falls within the candidate's bounds

Location:

const reasoning =
outcome.configOptionsSnapshot?.reasoningEffort ??
useChatSessionStore.getState().getSession(sessionId)?.reasoningEffort;
if (reasoning?.options.length) {
const candidate = mode.models.find(
(model) => model.modelId === decision.modelId,
);
const lower = candidate?.minEffort
? EFFORTS.indexOf(candidate.minEffort)
: 0;
const upper = candidate?.maxEffort
? EFFORTS.indexOf(candidate.maxEffort)
: EFFORTS.length - 1;
const requested = EFFORTS.indexOf(decision.effort);
const supported = reasoning.options
.map((option) => ({ id: option.id, rank: EFFORTS.indexOf(option.id) }))
.filter((option) => option.rank >= lower && option.rank <= upper)
.sort(
(a, b) =>
Math.abs(a.rank - requested) - Math.abs(b.rank - requested) ||
b.rank - a.rank,
);
const selected = supported[0];
if (!selected) throw new Error(i18n.t("chat:errors.autoEffortUnavailable"));

The effort mapping only recognizes none…max. Goose reasoning configs in this repository's fixtures use off/low/medium/high, and the picker handles an off-only config explicitly. Two cases fail: a routed model whose options are only off, and a candidate with maxEffort set to none or minimal on a session exposing off/low/medium/high. In both, supported is empty and the code throws autoEffortUnavailable. This happens after the model transition has already been applied, and the mode fallback is never tried. The router will likely choose the same candidate on retry, so every Auto turn for that choice fails. The docs instead say "Only effort choices actually supported by the session are applied." When no option maps into range, skip applying an effort (or treat off as none) rather than rejecting the send.

F5 — [INFO] Routing prompt truncation can split a surrogate pair, which makes routing silently use the fallback

Location:

function bounded(text: string, limit: number): string {
return text.length <= limit
? text
: `${text.slice(0, limit)}\n[Truncated for model selection]`;
}
and
sections.push(`Recent conversation:\n${history.slice(-MAX_HISTORY_CHARS)}`);
}
return sections.join("\n\n").slice(0, MAX_CONTEXT_CHARS);

bounded, the history slice (slice(-24000)), and the final slice(0, 176000) cut by UTF-16 code unit. When a cut lands inside an astral character such as an emoji, the IPC payload contains a lone surrogate. serde_json refuses to deserialize that into the prompt: String argument. The invoke rejects before route_auto_model runs, and routeAutoModel reports request_failed and uses the fallback. Trim a dangling high surrogate at the end of a cut, or a low surrogate at the start, or slice by code point.

From codex:

F1 — [WARNING] Preserve successful overlapping configuration loads

Location: runtime_config.rs:539–544

The existing finding remains valid: when two initial loads overlap, the older successful result becomes synthetic Unavailable if the newer load is still pending. A consequential caller is Goose startup, which logs the error and continues without runtime provider environment settings; later cache recovery cannot repair that child’s environment. Return the completed valid result without overwriting the newer generation, or await the newer load.

F2 — [WARNING] Preserve Auto opt-out across detached-window return

Location: acpSessionMapping.ts:136–139

Open an Auto chat in a detached window, select its current concrete model to leave Auto, then close that window and send from the main window. The detached renderer persists autoModeId: null, but the main renderer retains its old Auto ID. Window handoff and reclaim do not synchronize this intent, and this merge explicitly preserves the stale ID even after reading persisted metadata. The next prompt therefore routes again despite the explicit opt-out. Synchronize Auto intent between windows or reload it before reclaiming dispatch ownership.

Reviewed at commit 9c775f0.

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated code review

APPROVE. Fresh static Engineering review of the full exact three-dot comparison 180fe23...792c81d found no publishable changed-line defects. The previously reported overlapping-load race is fixed: an obsolete successful read remains usable by its caller without taking cache ownership, and a discriminating concurrent-load test verifies the newer generation ultimately becomes current. The complete Auto routing, new-chat refresh, provider reconciliation, UI and CLI creation, queue, transport, persistence, stale-window resync, and failure/recovery paths were rechecked; all prior automated blockers remain fixed. The supplied GitHub evidence is structurally valid for the exact head; four checks remain in progress and the other listed checks passed. Required checks still independently govern merge readiness. Final self-check covered every changed user flow; accessibility; i18n/localization completeness; navigation and consent guards; async, error, never-completes, lifecycle, and race behavior; test honesty; design-system/project rules; duplicate and overlapping findings; and concrete evidence plus user effect for blocking severity.

Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Pending checks: 4 check(s) are not complete.

This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.

@atishpatel
atishpatel merged commit 62403e4 into main Oct 19, 2026
10 checks passed
@atishpatel
atishpatel deleted the atish/auto-model-modes branch October 19, 2026 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants