feat(agents): harness-agnostic effort write path and spawn bridge - #4625
Conversation
2d8f97a to
e9e8f6a
Compare
c93e1c4 to
febf701
Compare
80df98f to
7c4c183
Compare
c60c87c to
5882e44
Compare
kalvinnchau
left a comment
There was a problem hiding this comment.
[P2] Pin-to-inherit can retain stale effort when the current runtime has no effort key — desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx:500
handleInheritHarnessChange decides whether to clear effort from the pre-toggle prospectiveRuntime. For an agent pinned to Claude (no thinkingEnvVar) whose linked persona uses Goose, selecting Inherit skips the clear. On the next render the prospective runtime becomes Goose, but the record's existing BUZZ_AGENT_THINKING_EFFORT remains; resolveInheritedRuntimeSubmission only excludes effort keys from the persona layer, so that record value is saved and the spawn bridge aliases it into Goose effort. This violates the stated pin↔inherit clear contract and can silently resurrect an effort value from an unrelated prior runtime. Resolve the runtime for nextInherit inside the handler (or clear whenever either the before or after runtime owns effort), and pin the Claude-pin → Goose-persona transition with an integration-level state/submit test.
[P2] Consumed legacy effort is also emitted as an advanced field — desktop/src-tauri/src/managed_agents/config_bridge/reader.rs:125
For a Goose record containing only a valid legacy BUZZ_AGENT_THINKING_EFFORT, build_thinking_field consumes it and exposes canonical thinking_effort, but normalized_env_keys contains only GOOSE_THINKING_EFFORT. The loop at line 140 therefore also emits the legacy key in surface.advanced. The config panel presents one persisted setting twice, with conflicting semantics, and an edit can appear to target a generic env var while the normalized field is already interpreting it. Invalid or unconsumed legacy values should remain advanced, so hide the legacy key only when record-tier alias resolution actually consumed it; add a surface test for valid-consumed versus invalid-visible legacy values.
Validation at exact PR tip 5882e4494656b0ce3deeffb2c1d1c1dea25e47f3:
just desktop-test: 4,774 passed, 0 failedjust desktop-tauri-test: passed (workspace; one pre-existing ignored native performance test)git diff --check a8e5c89e2..HEAD: clean
c4a6e99 to
513a75c
Compare
3bbf0e8 to
a9eff9e
Compare
af3d4d5 to
a1778cd
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
-
[P2] Preserve custom-runtime effort env in the restart snapshot —
desktop/src-tauri/src/managed_agents/spawn_snapshot.rs:181The launch projection deliberately uses an empty suppress set for an unknown/custom runtime so wrapper-specific variables such as
GOOSE_THINKING_EFFORTremain ordinary pass-through env.SpawnConfigSnapshot::from_inputs, however, unconditionally removes every globally known effort key. For an unknown command,effective_effortonly readsBUZZ_ACP_EFFORT_LEVEL, so an exact-caseGOOSE_THINKING_EFFORTdisappears from bothsnapshot.envandsnapshot.effort_leveleven though the child receives it. Changing that variable therefore produces no restart diff and can leave a running custom-wrapper agent on the old effort despite auto-restart-on-config-change. Scope snapshot cleanup to the runtime projection: known runtimes can collapse their suppress set, while unknown runtimes must retain non-sentinel effort-looking keys as ordinary env. Please pin the custom-wrapper case with a snapshot regression test.
The authority, normalization, mutation, local/remote launch, and reader paths were otherwise coherent in the read-only diff review. Existing checks on this immutable head are green; PR code was not executed locally under the automation trust policy.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested at exact head 239cbdd30cefee7a3a28d02271fc21de5226967f.
P2 — Mixed-case ACP sentinel changes for a custom runtime still disappear from restart snapshots
The new custom-runtime snapshot scoping fixes non-sentinel keys such as GOOSE_THINKING_EFFORT, but it still loses a mixed-case ACP sentinel on Windows.
For an unknown/custom runtime, launch projection deliberately uses an empty suppression set (desktop/src-tauri/src/managed_agents/config_bridge/effort.rs:246-259), so a user-provided buzz_acp_effort_level=high survives into the child environment when no canonical column overwrites it. Windows treats that key as BUZZ_ACP_EFFORT_LEVEL. Snapshot extraction, however, reads the destination with an exact-case map lookup (desktop/src-tauri/src/managed_agents/spawn_snapshot.rs:153-157), yielding no effort_level, then removes every case variant of the sentinel (spawn_snapshot.rs:194-203; config_bridge/effort.rs:208-226). Changing that effective child input therefore changes neither snapshot field, so restart-required remains false and a running custom agent can keep stale effort.
Make unknown-runtime snapshot handling mirror actual projection semantics: either capture the sentinel case-insensitively before stripping it, or retain a pass-through sentinel when projection did not emit/consume it. Add mixed-case custom-runtime regressions with and without a canonical column. The new test at spawn_snapshot/tests_ext.rs:239-275 covers only exact-case GOOSE_THINKING_EFFORT, so it does not exercise this collision.
The prior non-sentinel custom-runtime blocker is otherwise closed. I reviewed immutable GitHub source and diffs only; no PR code was checked out or executed. Hosted checks were green at review time.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes's GitHub account.
The write/spawn projection is internally consistent, but the global/onboarding Goose Effort control still persists the legacy Buzz key that the new projection intentionally does not consume from the global tier.
deriveAgentConfigFieldModelsetscurrentPersistence.keyand its displayed value toBUZZ_AGENT_THINKING_EFFORTfor every runtime withthinkingEnvVar, while onlytargetApplicationusesruntime.thinkingEnvVar(desktop/src/features/agents/lib/agentConfigCore.ts:206-220).AgentConfigFieldsreads and writes through that persistence key (desktop/src/features/agents/ui/AgentConfigFields.tsx:244-257,638-640,876-885). Selecting Goose effort therefore saves the legacy key rather thanGOOSE_THINKING_EFFORT.- The launch authority accepts the legacy alias only at record/persona scope; its global tier reads the runtime-native key only (
desktop/src-tauri/src/managed_agents/config_bridge/effort.rs:362-383). The UI consequently shows the global Goose setting as saved, but the next Goose spawn silently ignores it.
Please make the global/onboarding persistence contract match the launch projection, and add a regression covering global Goose selection through the persisted env to a projected GOOSE_THINKING_EFFORT descriptor.
EffortPickerField gained required disabled/value/onChange props in #4625. Wire them through the merged dialog using the same Save-gated pattern as the now-deleted AgentInstanceEditDialog: effortLevel state + effortTouched ref in AgentEditMergedDialog, passed as effortValue/onEffortChange to the instance section, which forwards to EffortPickerField. In useAgentEditMergedSubmit, add effortLevel/effortTouched to AgentEditSubmitState and include effortLevel in the locked update_managed_agent call when touched. When no other I-fields changed (agentInput would be null), synthesise a minimal input from inst.pubkey so the effort write is still atomic. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…runtime-switch reset IMPORTANT-1: Add effortLevel comparison to observedStateMatchesAgentInput (agentSaveCoordinator.ts) with tri-state semantics — absent submission skips, null clears, string compares against observed column. ManagedAgent now exposes effortLevel from ManagedAgentSummary (Rust types.rs + runtime.rs) so the comparator can detect a backend-rejected effort write rather than closing as success. Regressions: 4 family-14 coordinator tests; mutation (remove compare) turns the 2 rejected-edit tests RED while the 2 settled tests stay GREEN. IMPORTANT-2: Replace raw effortLevel include in useAgentEditMergedSubmit with resolveEffortSubmission — suppresses effort on pin→inherit (agentCommand:"") and on unchanged selections, matching the deleted AgentInstanceEditDialog's PR #4625 semantics exactly. originalEffortLevel threaded from configSurfaceQuery into AgentEditSubmitState. IMPORTANT-3: Add setEffortLevel and effortTouched to RuntimeHandlersInput; call setEffortLevel(null) and effortTouched.current = false in handleRuntimeDropdownChange (useAgentEditRuntimeHandlers.ts) so runtime switches do not carry stale vocabulary values into the next Save. Port the 6 load-bearing mounted production-seam tests from the deleted agentInstanceEditCancelSafety.test.mjs to agentEditMergedCancelSafety.test.mjs against AgentEditMergedDialog: selection+Cancel zero writes, ordinary Save carries effortLevel, rejected Save stays open, pin→inherit suppresses effort, access+effort atomic. Runtime-switch reset covered by hook-level test in agentEditMergedRuntimeReset.test.mjs. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Summary
Genericizes the agent effort write-side so Goose participates in the same canonical effort contract as buzz-agent. A spawn bridge translates the canonical key to whatever the target harness expects at launch time. Read/write/spawn paths all derive their vocabulary from runtime metadata rather than a hardcoded buzz-agent list.
What changed
Rust — config bridge + spawn path
apply_spawn_effort_envineffort.rs: production command-boundary seam — writes baked env, runs the effort projection, strips per-runtime suppress set, and emits exactly one projected key.apply_effort_to_spawn_commandinruntime.rs: thin wrapper returning a#[must_use] EffortApplied(())token (private field — unforgeable outside the function).spawn_agent_childcalls it aslet effort = apply_effort_to_spawn_command(...)and passesefforttospawn_with_effort_proof. Deleting the call is a compile error:effortis undefined at thespawn_with_effort_proofsite. Deletingapply_spawn_effort_envinside the wrapper turns the production-sequence tests RED.apply_record_field_updatesinagent_models_update.rs: returnsResult<RecordFieldsApplied, String>(#[must_use]token).update_managed_agentcalls it aslet applied = apply_record_field_updates(...)?then passesappliedtostamp_record_updated_at. Deleting the call is a compile error:appliedis undefined at thestamp_record_updated_atsite.apply_effort_launch_to_commandskips the suppress loop whenpreserve_passthrough && value.is_none(), preserving ambient ACP sentinels.EnvVarGuard: prior value stored asOsString(var_os) so non-Unicode values are restored exactly on Drop. A singlePROCESS_ENV_MUTEXinmanaged_agents/mod.rsis shared bylock_path_mutex()andlock_env_mutex()— any two tests calling either helper are mutually exclusive with each other. Tests in other modules (app_state_tests,agent_config_tests,reader_tests) maintain their own independent locks and are not in this domain.strip_effort_keys_from_commandmarked#[cfg(test)]; import path ineffort_cmd_tests.rsfixed.TypeScript — renderer + model cleanup
AgentConfigFieldsorphan-model cleanup effect: theisHarnessNativeEffortearly-return was skipping the model clear on provider→Custom transitions. Refined to: return early only when model is already null; clear model once while preserving the harness-native effort key (Carl P2).onConfigChangeloop.EffortSelectField/humanizeEffortLabel: runtime-native option labels title-cased (off→Off) with raw canonical values preserved for round-trip fidelity.AgentConfigFields: drives effort renderer fromselectedRuntime.effortCanonicalValues(harness-native path) or the model/provider catalog (buzz-agent/provider path), selected byisHarnessNativeEffort.Docs
desktop/src/features/agents/AGENTS.mditem 14: updated from deletedpersistAgentEffortLeveldirect-write contract to the shipped Save-gatedupdate_managed_agent.effortLevelpath. Consistent withEffortPickerField's own doc comment.Tests
agent_models_update_tests.rs: seam tests viaapply_record_field_updates— non-local rejects, local set/clear, ordering invariant, ACP-sentinel sweep.record_field_updates_persist_effort_to_disk(renamed from the prior false-claim name) drives load→apply→stamp→save→load via a mock AppHandle + tempdir, assertingeffort_levelpersists to disk. Manual HOME/XDG restore replaced with RAIIEnvVarGuard(panic-safe,OsString-exact).effort_cmd_tests.rs/effort_tests.rs: production-sequence seam tests viaapply_effort_to_spawn_command. Spawns/usr/bin/envto verify child's real env.EnvVarGuardfor panic-safe restore. Windows twin usingcmd /c set.effortAutoClear.test.mjs: five mounted stateful journeys viaAgentConfigFieldswithuseCustomSelect=true. Covers: custom trigger shows "Off" at mount; provider-empty mount is a stable fixed point; provider→Custom switch converges; stale Anthropic model cleared on Custom switch with Goose effort preserved (Carl P2 regression); Settings-style Save/reread preserves effort.agentDefaultsEditor.test.mjs: two full Save/Next journey tests through the real production parent trees. Both start withGOOSE_THINKING_EFFORT: "low"and operate the real Popover-based effort control (click trigger → click "off" option) before Save/Next, asserting zero writes after selection. Theset_global_agent_configstub captures the submitted payload; each test asserts rawGOOSE_THINKING_EFFORT: "off"in the captured config. The stub stores its canonical response from the actual payload; the fresh remount'sget_global_agent_configreturns that stored object (not a hand-written fixture), then asserts "Off" shown. TheDefaultConfigSteptest starts withisDirty: false— the real-control effort selection callsonConfigChange → updateDraft → isDirtyRef=true, making thecommit()on Next load-bearing.Mutation evidence
let effort = apply_effort_to_spawn_command(...)call fromspawn_agent_child→ compile error:error[E0425]: cannot find valueeffort`` atspawn_with_effort_proofsite.let applied = apply_record_field_updates(...)?fromupdate_managed_agent→ compile error:error[E0425]: cannot find valueapplied`` atstamp_record_updated_atsite.apply_spawn_effort_envfrom insideapply_effort_to_spawn_commandwrapper →production_sequence_goose_inherited_collision_resolved_in_childRED.isHarnessNativeEffort &&guard in cleanupuseEffectto bareif (isHarnessNativeEffort) return→ stale model not cleared → Carl P2 regression test RED.isHarnessNativeEffort ||from the nothing-to-clear condition → provider-empty mount emitsonConfigChange→ loop test RED.isHarnessNativeEffortbranch inAgentConfigFields.tsx:634-636→ bothagentDefaultsEditor.test.mjsmount assertions fail: trigger shows "Select" instead of initial effort label.preserve_passthroughguard inapply_effort_launch_to_command→production_sequence_custom_inherited_acp_sentinel_survivesRED.GOOSE_THINKING_EFFORTfrom theset_global_agent_configstub payload → payload assertion inagentDefaultsEditor.test.mjsfails (undefined !== "off") → RED (verified).DefaultConfigSteptest (soisDirtystays false) →commit()is a no-op → write-count assertion after Next fails (0 instead of 1) → RED.