Wire harness-native effort selection#2171
Conversation
| match response { | ||
| Ok(Ok(response)) => { | ||
| if response.get("configOptions").is_some() { | ||
| session_resp.raw = response; |
There was a problem hiding this comment.
This replaces the entire session/new result with a set_config_option response. ACP config-option updates may be partial, so this can discard models, session metadata, and config categories omitted from the update; extract_model_state below then silently loses the unstable catalog. Please keep the original response and merge only refreshed configOptions (using a corrected merge_config_options).
| let id = option.get("id").or_else(|| option.get("configId")); | ||
| if let Some(existing) = session_options | ||
| .iter_mut() | ||
| .find(|existing| existing.get("id").or_else(|| existing.get("configId")) == id) |
There was a problem hiding this comment.
The comparison here treats two absent IDs as equal (None == None). That means the first existing ID-less/malformed option can be overwritten by every unrelated ID-less refreshed option. Please merge only when the refreshed option has a concrete non-empty id/configId; otherwise append it or ignore it defensively. A regression test with two distinct ID-less options would pin this down.
| Ok(Ok(response)) => { | ||
| crate::acp::merge_config_options(&mut resp.raw, &response); | ||
| } | ||
| Ok(Err(error)) => tracing::warn!( |
There was a problem hiding this comment.
This fallback is safe for an unsupported/stale effort value, but not for every request failure. session_set_config_option can return transport/protocol errors, and the outer timeout can leave the stdio stream desynchronized; swallowing either and reusing this session risks corrupting the next prompt. Please mirror apply_model_switch / permission handling: propagate I/O, protocol, exit, write-timeout, and outer-timeout failures so the pool recreates the session, while keeping application-level rejection non-fatal.
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
Adding a few more findings on top of Wes's existing comments (his three cover the ACP defects: the session/new payload replacement, the ID-less option merge, and the swallowed transport errors on the effort path — no need to repeat those). Overall this reads as genuinely additive to the existing config bridge rather than a rework, which is great. The onboarding one below is the only thing I'd consider blocking alongside Wes's three; the rest are smaller.
| }, | ||
| render: "deferredUntilNativeOptionsAvailable", | ||
| value: null, | ||
| render: "control", |
There was a problem hiding this comment.
This flip from deferredUntilNativeOptionsAvailable to render: "control" is what's failing CI — onboarding-agent-defaults.spec.ts asserts the effort control is absent for Claude during onboarding and now sees count = 1, deterministically on all three attempts, so it's not a flake. But I think this is bigger than a stale test: the plan of record says onboarding doesn't change outside the two sanctioned exceptions, and resolveDisclosure keeps showEffortField: true for onboarding-essential, so Claude users now get a new control on the onboarding defaults page. I'd rather this be an explicit decision than a test edit — either amend the plan to sanction harness-native effort rendering in onboarding, or gate it out of the onboarding-essential disclosure and keep the rule intact. Both seem defensible to me, I just want the plan and the code to agree on which one we picked.
| let merged_env = discovery_env_with_baked_floor(merged_env); | ||
| // Native catalogs carry per-model options that provider APIs cannot. | ||
| let use_harness_catalog = | ||
| known_acp_runtime(agent_command).is_some_and(|runtime| runtime.supports_acp_native_config); |
There was a problem hiding this comment.
Two concerns with keying this off supports_acp_native_config. First, it's a behavior change for existing goose users that I don't see called out in the description — model discovery now skips the direct provider-API paths (OpenAI/Anthropic/Databricks HTTP) entirely and shells out to the adapter's models command instead, which means different latency and failure modes (needs a working adapter binary, not just an API key) and possibly a different model list. Second, supports_acp_native_config was defined for the tier-1a config/read/config/write capability, so if that flag ever flips for another harness, its discovery path silently changes too. I'd prefer a dedicated discovery_via_harness_catalog field on KnownAcpRuntime (or an explicit runtime-id check) so the two capabilities can't drift apart accidentally.
| ? effortOptions.map((option) => option.value) | ||
| : buzzEffortConfig.validValues; | ||
| const effortDefault = usesHarnessNativeEffort | ||
| ? effortCurrentValue |
There was a problem hiding this comment.
Small one: for harness-native effort the Default (X) label sources from effortCurrentValue, which is whatever the discovery probe session reports — and the probe env includes the user's own env vars, so a user-set value can end up labeled as the harness default. Only a labeling issue, but probably cheap to fix or at least document.
| ? effortOptions.length > 0 | ||
| ? effortOptions.map((option) => option.value) | ||
| : currentEffortForAutoClear | ||
| ? [currentEffortForAutoClear] |
There was a problem hiding this comment.
The [currentEffortForAutoClear] fallback correctly avoids wiping the user's value when an older adapter returns no thought_level options, but the dropdown degenerates to a single option in that case. Worth a regression test pinning the intended behavior for the empty-effortOptions case so it doesn't silently change later.
| // Legacy Goose configs stored effort under Buzz Agent's key. Honor that | ||
| // choice at launch without rewriting storage; the next user save swaps it | ||
| // to the runtime's native key. | ||
| if let Some((thinking_key, effort)) = legacy_effort_spawn_bridge( |
There was a problem hiding this comment.
Latent seam worth documenting or testing now: legacy_effort_spawn_bridge only sees effective_user_env (global → persona → agent), while baked build env is written directly onto the command via build_buzz_agent_provider_defaults earlier in this function. So a future baked BUZZ_AGENT_THINKING_EFFORT would never translate to GOOSE_THINKING_EFFORT. No impact today since releases bake no effort, but two independent reviewers hit this same trap while going through the PR — an invariant comment plus a test here would keep it from biting whoever adds baked effort later.
| return config.env_vars[key]?.trim() || null; | ||
| } | ||
|
|
||
| export function migrateLegacyEffortPersistence( |
There was a problem hiding this comment.
Mostly for release awareness: since this runs on every onConfigChange, any save through AgentConfigFields for a goose config migrates storage (legacy key → native key, legacy key deleted) even when effort wasn't the field being edited. Read-fallback covers display so I think the behavior is right, but it's a one-way storage migration triggered by unrelated saves — worth a line in the release notes.
3 participants
Context
PR 2.6 (#2158) introduced the Agent Config Core and made the mismatch explicit: Goose effort was persisted under Buzz Agent's key even though Goose reads
GOOSE_THINKING_EFFORT, while Claude Code's native ACPeffort/thought_leveloption was discarded by discovery.This stacked PR makes the Effort control truthful end to end without changing onboarding's layout or interaction model.
Learnings
thought_level.getProviderEffortConfigwas incorrect.Decisions and why
BUZZ_AGENT_THINKING_EFFORTvalues remain visible and are honored at launch. The next save atomically moves the value toGOOSE_THINKING_EFFORT; Buzz never double-writes.BUZZ_ACP_EFFORT. This mirrorsBUZZ_ACP_MODEL, preserves Buzz's global/definition/instance inheritance model, and is applied through the adapter's native config option when each session starts.What this PR does
thought_levelconfig options in the pre-spawn ACP probe.GOOSE_THINKING_EFFORT, including honest legacy migration.BUZZ_ACP_EFFORT, and applies throughsession/set_config_optionat session creation.What this PR defers
preferred_runtime(PR 3).Verification
just ci✅thinking_effort,thought_level) ✅effort,thought_level) ✅onboarding-agent-defaults.spec.ts: no assertions changed. A local full run was blocked by stale onboarding identity state in the E2E environment before reaching the config page; the shared unit/build/CI gates pass.buzz-pair-relayfailures:test_global_conn_capandtest_conn_counter_no_leak; branch was pushed with hooks bypassed only afterjust cipassed.