mirror of
https://github.com/block/buzz.git
synced 2026-11-02 07:22:55 +00:00
fix(desktop): auto-restart setup-mode agents when an edit satisfies readiness
A harness spawned with BUZZ_ACP_SETUP_PAYLOAD runs as a nudge-only setup listener and never re-evaluates its config — readiness is only computed at spawn. Fixing the missing model/provider in Edit Agent therefore changed nothing: the stale setup-mode process stayed connected to the relay and kept replying "needs configuration" to every mention, with no hint that a restart was required. Stamp was-spawned-in-setup-mode on the in-memory ManagedAgentProcess at spawn time (runtime-only, never persisted), and in update_managed_agent stop + respawn the process when the edit makes the record pass readiness — inside the same store-lock section the save already holds, reusing stop_managed_agent_process / start_managed_agent_process. The respawn is best-effort: a failure is surfaced via record.last_error and never fails the save itself. Healthy (non-setup-mode) processes keep the existing "takes effect on next spawn" contract and the needs_restart badge. The readiness gate lives in setup_mode_now_ready, which mirrors the spawn-time computation (effective harness resolution + effective env), with unit tests covering fixed config, still-missing model, still-missing credential, and the non-setup-mode no-op. Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
This commit is contained in:
parent
36fd41c5ce
commit
b76292d457
@@ -233,7 +233,11 @@ const overrides = new Map([
|
||||
// (if let Some(provider_update) = input.provider { record.provider = provider_update; }).
|
||||
// +8: harness_override thread-through in update_managed_agent so a deliberate
|
||||
// Custom pin routes to update_time_agent_command_override (comment + call).
|
||||
["src-tauri/src/commands/agent_models.rs", 1079],
|
||||
// +54: setup-mode auto-restart in update_managed_agent (stop + respawn a
|
||||
// live setup-listener process when the edit satisfies readiness) + the
|
||||
// expanded doc contract. Load-bearing fix for the "needs configuration"
|
||||
// forever-loop; queued to split with the discovery modules above.
|
||||
["src-tauri/src/commands/agent_models.rs", 1133],
|
||||
// draft-persistence predicate: submit-time `loadDraft` check + inline comment
|
||||
// + deps-array entry in submitMessage closes the never-persisted-boundary
|
||||
// defect (Thufir Pass-3 finding). Load-bearing correctness fix; queued to
|
||||
|
||||
@@ -13,7 +13,8 @@ use crate::{
|
||||
build_managed_agent_summary, current_instance_id, default_agent_workdir,
|
||||
discovery_env_with_baked_floor, find_managed_agent_mut, known_acp_runtime,
|
||||
load_managed_agents, load_personas, managed_agent_avatar_url, missing_command_message,
|
||||
normalize_agent_args, resolve_command, save_managed_agents, sync_managed_agent_processes,
|
||||
normalize_agent_args, resolve_command, save_managed_agents, setup_mode_now_ready,
|
||||
start_managed_agent_process, stop_managed_agent_process, sync_managed_agent_processes,
|
||||
try_regenerate_nest, AgentModelInfo, AgentModelsResponse, UpdateManagedAgentRequest,
|
||||
UpdateManagedAgentResponse, DEFAULT_ACP_COMMAND,
|
||||
},
|
||||
@@ -786,9 +787,17 @@ async fn run_agent_models_command(
|
||||
|
||||
/// Update mutable fields on an existing managed agent record.
|
||||
///
|
||||
/// Does NOT auto-restart the agent. Runtime config changes (system prompt,
|
||||
/// parallelism, commands, toolsets) take effect on the next agent spawn.
|
||||
/// Name changes are synced to the relay immediately via a kind:0 re-publish.
|
||||
/// Does NOT auto-restart a healthy agent. Runtime config changes (system
|
||||
/// prompt, parallelism, commands, toolsets) take effect on the next agent
|
||||
/// spawn. Name changes are synced to the relay immediately via a kind:0
|
||||
/// re-publish.
|
||||
///
|
||||
/// ONE exception: a live process that was spawned in setup mode (readiness
|
||||
/// failed at spawn, so buzz-acp is a nudge-only setup listener) is stopped
|
||||
/// and respawned when this edit makes the record pass readiness — a
|
||||
/// setup-mode harness never re-evaluates its config, so without the respawn
|
||||
/// the agent would nudge "needs configuration" forever after the user fixed
|
||||
/// it. See `setup_mode_now_ready`.
|
||||
#[tauri::command]
|
||||
pub async fn update_managed_agent(
|
||||
input: UpdateManagedAgentRequest,
|
||||
@@ -910,6 +919,53 @@ pub async fn update_managed_agent(
|
||||
|
||||
record.updated_at = now_iso();
|
||||
|
||||
// ── Auto-restart out of setup mode ──────────────────────────────────
|
||||
//
|
||||
// A harness spawned with BUZZ_ACP_SETUP_PAYLOAD runs as a nudge-only
|
||||
// setup listener and NEVER re-evaluates its config — readiness is only
|
||||
// computed at spawn (see `spawn_agent_child`). Without this, a user
|
||||
// who fixes the missing model/provider in Edit Agent keeps getting
|
||||
// "needs configuration" nudges forever because the stale setup-mode
|
||||
// process is still the one connected to the relay.
|
||||
//
|
||||
// So: when this edit makes a live setup-mode process ready, stop it
|
||||
// and respawn in place. Best-effort — a respawn failure must not fail
|
||||
// the save itself (the edit already landed); it is surfaced via
|
||||
// `last_error` on the record like other spawn failures.
|
||||
let was_setup_mode = runtimes
|
||||
.get(&record.pubkey)
|
||||
.is_some_and(|rt| rt.in_setup_mode);
|
||||
if was_setup_mode {
|
||||
let personas = load_personas(&app).unwrap_or_default();
|
||||
if setup_mode_now_ready(record, &personas, true) {
|
||||
// Same owner-hex read as `commands::agents::workspace_owner_hex`;
|
||||
// `state.keys` is acquired inside the store lock elsewhere too
|
||||
// (see `retain_managed_agent_pending`), so ordering is safe.
|
||||
let owner_hex = {
|
||||
let keys = state.keys.lock().map_err(|e| e.to_string())?;
|
||||
keys.public_key().to_hex()
|
||||
};
|
||||
let restart =
|
||||
stop_managed_agent_process(&app, record, &mut runtimes).and_then(|()| {
|
||||
start_managed_agent_process(&app, record, &mut runtimes, Some(&owner_hex))
|
||||
});
|
||||
match restart {
|
||||
Ok(()) => eprintln!(
|
||||
"buzz-desktop: agent {} config now satisfies readiness — \
|
||||
auto-restarted out of setup mode",
|
||||
record.name
|
||||
),
|
||||
Err(e) => {
|
||||
eprintln!(
|
||||
"buzz-desktop: auto-restart of agent {} out of setup mode failed: {e}",
|
||||
record.name
|
||||
);
|
||||
record.last_error = Some(format!("auto-restart failed: {e}"));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
save_managed_agents(&app, &records)?;
|
||||
|
||||
let record = records
|
||||
|
||||
@@ -134,6 +134,7 @@ pub fn finish_spawn(
|
||||
child: std::process::Child,
|
||||
log_path: std::path::PathBuf,
|
||||
spawn_config_hash: u64,
|
||||
in_setup_mode: bool,
|
||||
agent_name: &str,
|
||||
) -> super::ManagedAgentProcess {
|
||||
let job = create_job_for_child(child.id());
|
||||
@@ -147,6 +148,7 @@ pub fn finish_spawn(
|
||||
child,
|
||||
log_path,
|
||||
spawn_config_hash,
|
||||
in_setup_mode,
|
||||
job,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1620,7 +1620,7 @@ pub fn spawn_agent_child(
|
||||
//
|
||||
// The JSON format mirrors `setup_mode::SetupPayload` in buzz-acp:
|
||||
// { "agent_name": "...", "agent_pubkey": "...", "requirements": [{ "surface": "...", ... }] }
|
||||
{
|
||||
let in_setup_mode = {
|
||||
use crate::managed_agents::{
|
||||
agent_readiness, resolve_effective_agent_env, AgentReadiness, Requirement,
|
||||
};
|
||||
@@ -1682,6 +1682,7 @@ pub fn spawn_agent_child(
|
||||
command.env_remove("BUZZ_ACP_SETUP_PAYLOAD");
|
||||
|
||||
// Set the payload only when desktop computed NotReady.
|
||||
let in_setup_mode = setup_payload_json.is_some();
|
||||
if let Some(json) = setup_payload_json {
|
||||
command.env("BUZZ_ACP_SETUP_PAYLOAD", json);
|
||||
eprintln!(
|
||||
@@ -1689,7 +1690,8 @@ pub fn spawn_agent_child(
|
||||
record.name
|
||||
);
|
||||
}
|
||||
}
|
||||
in_setup_mode
|
||||
};
|
||||
// Only emit BUZZ_ACP_IDLE_TIMEOUT when the user has explicitly set an
|
||||
// override. When unset, the buzz-acp harness applies its own default
|
||||
// (see `DEFAULT_IDLE_TIMEOUT_SECS` in crates/buzz-acp/src/config.rs),
|
||||
@@ -1890,6 +1892,7 @@ pub fn spawn_agent_child(
|
||||
child,
|
||||
log_path,
|
||||
spawn_config_hash,
|
||||
in_setup_mode,
|
||||
&record.name,
|
||||
));
|
||||
#[cfg(not(windows))]
|
||||
@@ -1897,9 +1900,43 @@ pub fn spawn_agent_child(
|
||||
child,
|
||||
log_path,
|
||||
spawn_config_hash,
|
||||
in_setup_mode,
|
||||
})
|
||||
}
|
||||
|
||||
/// Returns `true` when a process spawned in setup mode would now pass the
|
||||
/// readiness check under the record's current configuration — i.e. an edit
|
||||
/// has satisfied the missing requirements and only a respawn will get the
|
||||
/// agent out of the nudge-only setup listener (a setup-mode harness never
|
||||
/// re-evaluates its config; readiness is only computed at spawn, see
|
||||
/// `spawn_agent_child`).
|
||||
///
|
||||
/// `was_spawned_in_setup_mode` is the flag stamped on
|
||||
/// [`ManagedAgentProcess::in_setup_mode`] at spawn time. Passing the flag
|
||||
/// (rather than the process) keeps this unit-testable without a live child.
|
||||
pub fn setup_mode_now_ready(
|
||||
record: &ManagedAgentRecord,
|
||||
personas: &[crate::managed_agents::types::PersonaRecord],
|
||||
was_spawned_in_setup_mode: bool,
|
||||
) -> bool {
|
||||
use crate::managed_agents::{agent_readiness, resolve_effective_agent_env, AgentReadiness};
|
||||
|
||||
if !was_spawned_in_setup_mode {
|
||||
return false;
|
||||
}
|
||||
// Mirror the spawn-time readiness computation exactly: resolve the
|
||||
// effective harness (persona wins unless explicitly pinned), then build
|
||||
// the effective env the child would receive.
|
||||
let effective_command = crate::managed_agents::effective_agent_command(
|
||||
record.persona_id.as_deref(),
|
||||
personas,
|
||||
record.agent_command_override.as_deref(),
|
||||
);
|
||||
let runtime_meta = known_acp_runtime(&effective_command);
|
||||
let effective = resolve_effective_agent_env(record, personas, runtime_meta);
|
||||
matches!(agent_readiness(&effective), AgentReadiness::Ready)
|
||||
}
|
||||
|
||||
fn child_rust_log_filter() -> String {
|
||||
match std::env::var("RUST_LOG") {
|
||||
Ok(existing) if existing.contains("buzz_acp") => existing,
|
||||
|
||||
@@ -608,3 +608,60 @@ fn grandchild_inherits_pgid_of_process_group_leader() {
|
||||
unsafe { libc::kill(-harness_pid, libc::SIGTERM) };
|
||||
let _ = harness.wait();
|
||||
}
|
||||
|
||||
// ── setup_mode_now_ready tests ───────────────────────────────────────
|
||||
//
|
||||
// The auto-restart gate in `update_managed_agent`: a live process spawned in
|
||||
// setup mode should be respawned exactly when the edited record now passes
|
||||
// readiness. These drive the pure seam (`setup_mode_now_ready`) with the
|
||||
// spawn-time flag injected, so no live child process is needed.
|
||||
|
||||
use super::setup_mode_now_ready;
|
||||
|
||||
/// Record that passes buzz-agent readiness: provider + model + matching
|
||||
/// credential. No persona and no override, so the effective command resolves
|
||||
/// to the `buzz-agent` default.
|
||||
fn ready_record() -> ManagedAgentRecord {
|
||||
let mut record = fixture(RespondTo::Anyone, vec![], Some("tag".into()));
|
||||
record.model = Some("claude-opus-4-5".into());
|
||||
record.provider = Some("anthropic".into());
|
||||
record
|
||||
.env_vars
|
||||
.insert("ANTHROPIC_API_KEY".into(), "sk-test".into());
|
||||
record
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn setup_mode_now_ready_true_when_config_fixed() {
|
||||
// The Fizz bug: spawned in setup mode (model missing), then the user set
|
||||
// model/provider in Edit Agent → the edit must trigger a respawn.
|
||||
let record = ready_record();
|
||||
assert!(setup_mode_now_ready(&record, &[], true));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn setup_mode_now_ready_false_when_still_not_ready() {
|
||||
// Model still missing → stay in setup mode; a respawn would just land
|
||||
// back in the setup listener.
|
||||
let mut record = ready_record();
|
||||
record.model = None;
|
||||
assert!(!setup_mode_now_ready(&record, &[], true));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn setup_mode_now_ready_false_when_credential_still_missing() {
|
||||
// Provider/model present but the provider credential is absent — the
|
||||
// same NotReady the spawn path would compute.
|
||||
let mut record = ready_record();
|
||||
record.env_vars.remove("ANTHROPIC_API_KEY");
|
||||
assert!(!setup_mode_now_ready(&record, &[], true));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn setup_mode_now_ready_false_for_normal_process() {
|
||||
// A process NOT spawned in setup mode must never be auto-restarted by an
|
||||
// edit — edits to healthy agents keep the existing "takes effect on next
|
||||
// spawn" contract (the needs_restart badge covers drift).
|
||||
let record = ready_record();
|
||||
assert!(!setup_mode_now_ready(&record, &[], false));
|
||||
}
|
||||
|
||||
@@ -247,6 +247,13 @@ pub struct ManagedAgentProcess {
|
||||
/// `runtime_pid` have no `ManagedAgentProcess` entry, so their spawn
|
||||
/// config is unknown and the badge stays off.
|
||||
pub spawn_config_hash: u64,
|
||||
/// `true` when this process was spawned with `BUZZ_ACP_SETUP_PAYLOAD` set —
|
||||
/// i.e. readiness failed at spawn and buzz-acp is running as a nudge-only
|
||||
/// setup listener instead of the agent pool. Runtime-only — never
|
||||
/// persisted. `update_managed_agent` uses this to auto-restart the process
|
||||
/// once an edit satisfies readiness, since a setup-mode harness never
|
||||
/// re-evaluates its config on its own.
|
||||
pub in_setup_mode: bool,
|
||||
/// Win32 Job Object owning the harness + its entire process tree. Closing
|
||||
/// the handle (via `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`) kills the whole
|
||||
/// tree — the Windows mirror of the Unix process-group teardown. `None`
|
||||
|
||||
Reference in New Issue
Block a user