Repository navigation
Conversation
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
Recommend APPROVE. The exact three-dot comparison was reviewed through all four required lenses with no publishable findings and no duplicate threads. Supplied GitHub evidence was inspected separately: Frontend checks and unit tests is failed and Tauri Rust checks (macOS) is in progress, so required checks still govern merge readiness.
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.
Completed checks currently reporting failure:
- Frontend checks and unit tests: failure
Pending checks: 2 check(s) are not complete.
This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
Recommend REQUEST_CHANGES for three blocking reliability defects in Auto-mode discovery and session creation. The supplied GitHub evidence was inspected and all ten captured checks completed successfully at the reviewed head; required checks still independently govern merge readiness.
Deterministic publication result: 3 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| "--auto-mode-id requires Goose and cannot be combined with --model-id; omit --model-id and use --harness-id goose (or omit --harness-id).", | ||
| ); | ||
| } | ||
| if (!autoRouting?.modes.some((mode) => mode.id === args.auto_mode_id)) { |
There was a problem hiding this comment.
🤖 P1 · Distinguish unavailable Auto configuration (blocking)
The new explicit-mode validation ignores the runtime-config refresh result and treats an empty effective config as authoritative. On an initial or failed load, the runtime store exposes the default config with no Auto modes, so this branch reports a configured mode as unknown; the matching discovery command also presents an empty auto_modes list without warning. Existing Auto dispatch code deliberately treats unloaded, unavailable, and app-default config as uncertainty rather than proof that a saved mode was removed.
User effect: During a temporary configuration outage, an agent is incorrectly told that the requested Auto mode does not exist and discovery appears to confirm that claim, encouraging it to abandon the user's intended routing or create the child chat with a concrete model instead.
Recommended fix: Use the refresh result to distinguish authoritative ready configuration from unavailable or app-default state. For an explicit mode, return a retryable configuration-unavailable error when the mode cannot be verified; make model discovery expose the same uncertainty through its warning rather than presenting an authoritative empty mode list.
Test: Simulate the first runtime-config refresh returning unavailable, then assert that explicit Auto creation returns a retryable configuration error with no workspace, session, or message side effects and that Goose model discovery reports a warning instead of an unqualified empty auto_modes list.
| } | ||
| const autoModeId = | ||
| args.harness_id || args.model_id || args.agent_id | ||
| args.auto_mode_id ?? |
There was a problem hiding this comment.
🤖 P1 · Revalidate mode before creation (blocking)
The selected mode is validated against one captured runtime-config snapshot, but several asynchronous project, harness, model, and persona validations run before session creation. If a concurrent config refresh removes or changes that mode, target construction reads the newer live store and can fall back to plain Goose while the earlier auto_mode_id is still persisted and returned as successful. First-send preparation can then clear the stale mode and dispatch without the routing the caller requested.
User effect: A child chat can report that it was created with the requested Auto mode while its first prompt actually runs on a default or stale concrete model.
Recommended fix: Bind mode validation and execution-target derivation to one authoritative config revision or snapshot, then verify that revision and mode immediately before creating the session. Reject the command as superseded if the selection changed instead of falling back while retaining the requested mode id.
Test: Pause one asynchronous validation, replace the runtime config with a revision that removes the selected mode, resume the command, and assert that no session or message is created and a retryable superseded-selection error is returned.
| (harness) => harness.readiness === "ready", | ||
| ); | ||
| if (targets.some((harness) => harness.id === GOOSE_PROVIDER_ID)) { | ||
| await refreshRuntimeConfigForNewChat(); |
There was a problem hiding this comment.
🤖 P2 · Keep model discovery failure-isolated (blocking)
Whenever Goose is among the targets, the read-only model-list command now awaits the new-chat refresh path. That path requires backend provider synchronization and can throw, so an unrelated Goose sync failure aborts the entire unfiltered command before cached models for other ready harnesses are returned. This bypasses the command's existing stale-data warning behavior and expands a Goose-specific dependency to every-harness discovery.
User effect: An agent cannot discover otherwise available Claude, Codex, or other harness models through the default all-harness command when Goose provider synchronization is temporarily failing.
Recommended fix: Use a read-only runtime-config refresh for discovery, or isolate refresh and provider-sync failure to the Goose result by serving the last known snapshot with a warning while continuing to return unaffected harnesses. Keep strict backend synchronization on session creation.
Test: Make Goose runtime-provider synchronization reject while another ready harness has cached models, call info models without a harness filter, and assert that the other harness is returned and the Goose entry carries a warning instead of failing the whole command.
…tainty - Explicit --auto-mode-id now distinguishes an authoritative configured snapshot from initial, failed, and app-default loads: those return a retryable config_unavailable error instead of claiming the mode is unknown. - The selected mode is bound to its config snapshot and re-verified immediately before session creation; a concurrent refresh that replaces the routing rejects the command as selection_superseded with no session or message side effects instead of falling back while retaining the mode. - info models isolates Goose runtime-config refresh failures to the Goose entry (warning) so every-harness discovery still serves other ready harnesses, and warns when the Auto mode list reflects unavailable or app-default configuration.
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
Recommend COMMENT. No new publishable findings remain after duplicate suppression. One still-present session-creation race maps to an existing unresolved automation thread, and unresolved prior automated threads have no substantive human-account replies, so approval fails closed. The supplied GitHub evidence is structurally valid; three captured checks succeeded while the remaining required checks are not yet represented and still govern merge readiness.
Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 1 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
Recommend COMMENT for two non-blocking copy and terminology defects. One still-present routing-consistency concern maps to an existing unresolved automation thread and is suppressed. Supplied GitHub evidence is structurally valid: six captured checks succeeded and two are in progress, so required checks still govern merge readiness.
Deterministic publication result: 0 blocking and 2 non-blocking inline finding(s) publishable; 1 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.
| array contains each mode's `id`, display `name`, and `is_default`; these ids are | ||
| not concrete `models[].model_id` values. | ||
|
|
||
| Create a child/sub-session with an explicit configured mode: |
There was a problem hiding this comment.
🤖 P2 · Do not call this a sub-session (non-blocking)
The new agent guidance calls an ordinary session created by berdctl a child/sub-session, but this command records no parent relationship and does not create Berd's existing sub_agent session type. The same paragraph then says no parent session is inferred, so the documentation gives one operation two conflicting product meanings.
User effect: An orchestrating agent can describe or promise a parent-linked delegated session even though the resulting chat is a normal standalone background session with no parent-child continuity in Berd.
Recommended fix: Call the result a new background chat or session throughout this guidance unless session create is intentionally extended to create a real parent-linked sub-session.
Test: Add a documentation assertion or review pin that the berdctl guidance describes session create as a standalone background session and does not use the product's sub-session or sub-agent terminology.
| ) { | ||
| throw new CommandError( | ||
| "selection_superseded", | ||
| "The Auto mode selection changed while the chat was being prepared; no session was created. Retry with a currently configured --auto-mode-id.", |
There was a problem hiding this comment.
🤖 P3 · Keep default-mode recovery neutral (non-blocking)
The new superseded-selection guard also runs when Auto mode was selected implicitly from the configured default, but its error always tells the caller to retry with --auto-mode-id. A caller that supplied no routing flag is therefore told to switch to explicit pinning even though a plain retry is the appropriate recovery and the documented default behavior is meant to remain unchanged.
User effect: An agent using the normal default flow may unnecessarily pin a specific Auto mode after a transient configuration race, changing the routing intent it originally asked Berd to apply.
Recommended fix: Use recovery text that only mentions --auto-mode-id when the caller explicitly supplied it; otherwise tell the caller that Auto configuration changed and to retry without prescribing a routing choice.
Test: Exercise the superseded-selection path with no harness, model, agent, or auto-mode flag and assert that the retryable error does not instruct the caller to add --auto-mode-id.
Summary
Let child Berd chats explicitly use Auto Balanced or another configured Auto mode through
berdctl session create --auto-mode-id <id>. Today, CLI creation uses the Auto default only when harness, model, and persona are all omitted. There is no way to request a mode explicitly.berdctl info models --harness-id goosenow lists Auto modes separately from concrete models. Creation and session metadata reportauto_mode_id. Invalid modes and model/harness conflicts fail before workspace or session creation. Existing default behavior stays unchanged; this does not infer settings from the parent chat.The change preserves the configured provider/model requirement in
LAWS/AGENTS.mdand the existing first-message queue path inLAWS/CHAT.md.Related issue
No duplicate found. Extends the Auto model modes introduced in #389.
Testing
Built the
berdctlbinary and checkedsession create --helpandinfo models --help; both expose the new mode selection and discovery guidance. No live desktop end-to-end test. The change adds no new UI.