diff --git a/Cargo.lock b/Cargo.lock index c33d4f7a..0e0e03f3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -126,12 +126,14 @@ dependencies = [ "anyhow", "async-trait", "chrono", + "fs2", "llm", "serde", "serde_json", "tempfile", "tokio", "tracing", + "uuid", ] [[package]] diff --git a/ROADMAP.md b/ROADMAP.md index abea6c54..fdd55ecc 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -182,7 +182,10 @@ clean. > the model only as the framed goal-turn message), and a > host-driven `GoalController` (`code_assistant_core::goals`) that drives > `Running` goals through `start_turn_if_idle` while the app is open — -> deliberately no unattended auto-resume after process end. Remaining from +> deliberately no crash recovery or claim adoption. A persisted in-flight +> claim may belong to another live instance and is left untouched; after a +> real crash only the user can clear it by replacing or cancelling the goal. +> Remaining from > this section: step 3 (run/delegation convergence, see Next). The highest-value shared migration is the generic part of PAL's durable goal @@ -201,7 +204,8 @@ reasoning. The following pieces belong upstream: controller's `Wait` decision folds out of the same state machine, so leaving the wait types behind would split that machine across repositories; - a default bounded LLM evaluator that requires concrete verification evidence; -- generic create/show/pause/resume/cancel/update operations and tool contracts. +- generic storage and lifecycle primitives; each host chooses its smaller + user-facing command language. The following remain PAL responsibilities: @@ -284,17 +288,17 @@ All met as of 2026-07-16: - ✅ code-assistant can create and continue a goal in an ordinary session (user-set via `/goal` → `goal_commands` + `GoalController`; the earlier model-facing `goal` tool was removed — only the user sets goals). -- ✅ A goal may survive session reload without promising unattended - auto-resume (store-persisted; surfaced on demand via `/goal` list/show; - the controller loop lives and dies with the process). +- ✅ A goal may survive session reload without promising crash recovery (the + controller loop lives and dies with the process; persisted in-flight claims + are never adopted or restarted by code-assistant). - ✅ PAL uses the shared model/controller while retaining its stronger restart and channel semantics (re-export shims since 2026-07-16). -- ✅ Concurrent pause/cancel and turn completion cannot overwrite each other - (revision/claim-token merge in the shared store, pinned by tests in both - consumers). -- ✅ Every claimed attempt consumes budget or closes with an explicit - abandonment reason; crashes cannot silently refund work (Busy-abandonment is - the sole refund path; stale in-flight claims fold as interrupted attempts). +- ✅ `/goal ` atomically replaces the session's one + current goal; `/goal cancel` removes it. Neither path reaches the model, and + the command works in TUI, GPUI and ACP clients. +- ✅ Every attempt completed by this process consumes budget or closes with an + explicit Busy abandonment. A claim whose owner disappears stays parked; + code-assistant does not infer a crash from shared persisted state. ## Next: unify runs and delegation diff --git a/crates/agent_orchestration/Cargo.toml b/crates/agent_orchestration/Cargo.toml index 2fb908b4..89361710 100644 --- a/crates/agent_orchestration/Cargo.toml +++ b/crates/agent_orchestration/Cargo.toml @@ -7,9 +7,11 @@ edition = "2021" anyhow = "1.0" async-trait = "0.1" chrono = { version = "0.4", features = ["serde"] } +fs2 = "0.4" llm = { path = "../llm" } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" +uuid = { version = "1", features = ["v4"] } tokio = { version = "1", features = ["sync"] } tracing = "0.1" diff --git a/crates/agent_orchestration/src/goals.rs b/crates/agent_orchestration/src/goals.rs index 18b052ee..38d6107e 100644 --- a/crates/agent_orchestration/src/goals.rs +++ b/crates/agent_orchestration/src/goals.rs @@ -18,10 +18,13 @@ use crate::OwnerKey; use chrono::NaiveDateTime; +use fs2::FileExt; use serde::{Deserialize, Serialize}; use std::collections::HashMap; -use std::path::PathBuf; -use std::sync::{Arc, Mutex, OnceLock}; +use std::fs::{File, OpenOptions}; +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex, MutexGuard, OnceLock}; +use uuid::Uuid; /// Where a goal is in its lifecycle. `Done` and `Failed` are terminal. /// @@ -529,6 +532,133 @@ impl Goal { self.note = Some(reason.into()); Ok(()) } + + // --- Store-level folds ------------------------------------------------- + // + // The claim/finish/abandon protocol is policy, not persistence: every + // repository implementation (the JSON store here, a host's transactional + // store) must apply exactly the same per-goal transition or the token + // guarantees silently diverge. These folds mutate `self` (the currently + // persisted goal) against a caller-held snapshot/claim and tell the store + // whether — and what — to persist. + + /// Claim fold: begin an attempt against `snapshot`, or report why not. + /// Mutates `self` (revision bumped) except in the `Stale` case. + pub fn fold_claim(&mut self, snapshot: &Goal, now: NaiveDateTime) -> anyhow::Result { + if self.revision != snapshot.revision + || self.state != GoalState::Running + || self.in_flight.is_some() + { + return Ok(ClaimFold::Stale); + } + if self.enforce_deadline(now) { + self.revision = self.revision.saturating_add(1); + return Ok(ClaimFold::Enforced); + } + if self.turns_remaining() == 0 { + self.fail("turn budget exhausted", now)?; + self.revision = self.revision.saturating_add(1); + return Ok(ClaimFold::Enforced); + } + + self.begin_attempt(now)?; + self.revision = self.revision.saturating_add(1); + Ok(ClaimFold::Claimed) + } + + /// Finish fold: close the claimed attempt on `self`, merging a concurrent + /// user transition instead of overwriting it (see + /// [`GoalStore::finish_attempt`] for the semantics). `None` when the claim + /// token no longer matches — `self` is untouched and must not be + /// persisted. Errors if `claim` carries no in-flight token. + pub fn fold_finish( + &mut self, + claim: &Goal, + completion: AttemptCompletion, + now: NaiveDateTime, + ) -> anyhow::Result> { + let Some(token) = claim.in_flight.as_ref() else { + anyhow::bail!("cannot finish a goal snapshot without an in-flight attempt"); + }; + if self.in_flight.as_ref() != Some(token) { + return Ok(None); + } + + let stopped_state = + (self.state != GoalState::Running).then_some((self.state, self.note.clone())); + let was_terminal = self.state.is_terminal(); + let decision = match completion { + AttemptCompletion::Evaluated(evaluation) => { + // The attempt itself was claimed while Running. Temporarily + // restore that state to fold its verdict, then reinstate a + // concurrent user stop unless the attempt reached a terminal + // outcome. Thus progress never resumes a preempted goal, while + // verified completion and hard envelope failures remain final. + if stopped_state.is_some() { + self.state = GoalState::Running; + } + let decision = self.apply_evaluation(evaluation, now)?; + if let Some((state, note)) = stopped_state { + if was_terminal || !self.state.is_terminal() { + self.state = state; + self.note = note; + self.updated_at = now; + ControllerDecision::Preempted + } else { + decision + } + } else { + decision + } + } + AttemptCompletion::ControllerError(reason) => { + let decision = self.record_attempt_error(reason, now)?; + if let Some((state, note)) = stopped_state { + if was_terminal { + self.state = state; + self.note = note; + self.updated_at = now; + ControllerDecision::Preempted + } else { + decision + } + } else { + decision + } + } + }; + self.revision = self.revision.saturating_add(1); + Ok(Some(decision)) + } + + /// Abandon fold: release the claim without spending budget. `false` when + /// the token no longer matches (`self` untouched). Errors if `claim` + /// carries no in-flight token. + pub fn fold_abandon(&mut self, claim: &Goal) -> anyhow::Result { + let Some(token) = claim.in_flight.as_ref() else { + anyhow::bail!("cannot abandon a goal snapshot without an in-flight attempt"); + }; + if self.in_flight.as_ref() != Some(token) { + return Ok(false); + } + self.in_flight = None; + self.revision = self.revision.saturating_add(1); + Ok(true) + } +} + +/// Outcome of [`Goal::fold_claim`]: what the repository must do with the +/// mutated goal. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ClaimFold { + /// The snapshot is stale or the goal is not claimable — nothing changed, + /// nothing to persist, no work may be dispatched. + Stale, + /// The envelope fired (deadline passed / budget exhausted): the goal + /// mutated and must be persisted, but no attempt was claimed. + Enforced, + /// An attempt was begun: persist the goal, then dispatch the turn. + Claimed, } /// One turn's judgement against the contract, produced by a [`GoalEvaluator`] @@ -638,8 +768,9 @@ pub trait GoalEvaluator: Send + Sync { /// JSON-file persistence for goals (`goals.json`). Every operation reloads the /// current file and writes through atomic tmp+rename. Instances for the same -/// path share a process-local lock, while revisions reject stale snapshots and -/// attempt tokens let completion merge with concurrent lifecycle changes. +/// path share a process-local mutex and an OS advisory lock serializes +/// read-modify-write transactions across processes. Revisions reject stale +/// snapshots and attempt tokens merge concurrent lifecycle changes. pub struct GoalStore { path: PathBuf, lock: Arc>, @@ -652,9 +783,15 @@ impl GoalStore { Self { path, lock } } + fn transaction(&self) -> anyhow::Result<(MutexGuard<'_, ()>, File)> { + let process_guard = self.lock.lock().expect("goal store lock poisoned"); + let file_guard = lock_store_file(&self.path)?; + Ok((process_guard, file_guard)) + } + /// All goals, terminal ones included; a missing file yields an empty list. pub fn list(&self) -> anyhow::Result> { - let _guard = self.lock.lock().expect("goal store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; self.load_unlocked() } @@ -678,7 +815,7 @@ impl GoalStore { /// Add a goal with a caller-supplied id; the id must be unique. pub fn add(&self, goal: Goal) -> anyhow::Result<()> { - let _guard = self.lock.lock().expect("goal store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut goals = self.load_unlocked()?; if goals.iter().any(|g| g.id == goal.id) { anyhow::bail!("goal id {} already exists", goal.id); @@ -697,7 +834,7 @@ impl GoalStore { budget: Budget, now: NaiveDateTime, ) -> anyhow::Result { - let _guard = self.lock.lock().expect("goal store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut goals = self.load_unlocked()?; let base = format!("goal-{}", now.format("%Y%m%d-%H%M%S")); let mut id = base.clone(); @@ -712,6 +849,53 @@ impl GoalStore { Ok(goal) } + /// Replace every non-terminal goal owned by `owner` with one fresh goal, + /// in a single read-modify-write transaction. Terminal evidence ledgers + /// remain history. This is the persistence primitive for hosts that expose + /// exactly one current goal per owner. Returns the new goal and the number + /// of current goals it replaced. + pub fn replace_current_for_owner( + &self, + owner: OwnerKey, + objective: impl Into, + contract: CompletionContract, + budget: Budget, + now: NaiveDateTime, + ) -> anyhow::Result<(Goal, usize)> { + let (_process_guard, _file_guard) = self.transaction()?; + let mut goals = self.load_unlocked()?; + let before = goals.len(); + goals.retain(|goal| goal.owner != owner || goal.state.is_terminal()); + let replaced = before - goals.len(); + // Replacement deletes the old ledger entry, but durable waits and + // child links can outlive it briefly. A UUID prevents the new goal + // from ever addressing those links, including after cancel + set in + // the same second. + let id = format!( + "goal-{}-{}", + now.format("%Y%m%d-%H%M%S"), + Uuid::new_v4().simple() + ); + let goal = Goal::new(id, owner, objective, contract, budget, now); + goals.push(goal.clone()); + self.save_unlocked(&goals)?; + Ok((goal, replaced)) + } + + /// Remove every non-terminal goal owned by `owner` in one transaction, + /// preserving terminal evidence ledgers. Returns the number removed. + pub fn remove_current_for_owner(&self, owner: &OwnerKey) -> anyhow::Result { + let (_process_guard, _file_guard) = self.transaction()?; + let mut goals = self.load_unlocked()?; + let before = goals.len(); + goals.retain(|goal| &goal.owner != owner || goal.state.is_terminal()); + let removed = before - goals.len(); + if removed > 0 { + self.save_unlocked(&goals)?; + } + Ok(removed) + } + /// A single goal by id. pub fn get(&self, id: &str) -> anyhow::Result> { Ok(self.list()?.into_iter().find(|g| g.id == id)) @@ -720,7 +904,7 @@ impl GoalStore { /// Persist a mutated goal (after `apply_evaluation`, a lifecycle edge, …). /// Errors if the id is unknown — an update never silently creates. pub fn update(&self, goal: &Goal) -> anyhow::Result { - let _guard = self.lock.lock().expect("goal store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut goals = self.load_unlocked()?; let Some(slot) = goals.iter_mut().find(|g| g.id == goal.id) else { anyhow::bail!("unknown goal id {}", goal.id); @@ -747,34 +931,23 @@ impl GoalStore { snapshot: &Goal, now: NaiveDateTime, ) -> anyhow::Result> { - let _guard = self.lock.lock().expect("goal store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut goals = self.load_unlocked()?; let Some(goal) = goals.iter_mut().find(|goal| goal.id == snapshot.id) else { return Ok(None); }; - if goal.revision != snapshot.revision - || goal.state != GoalState::Running - || goal.in_flight.is_some() - { - return Ok(None); - } - if goal.enforce_deadline(now) { - goal.revision = goal.revision.saturating_add(1); - self.save_unlocked(&goals)?; - return Ok(None); - } - if goal.turns_remaining() == 0 { - goal.fail("turn budget exhausted", now)?; - goal.revision = goal.revision.saturating_add(1); - self.save_unlocked(&goals)?; - return Ok(None); + match goal.fold_claim(snapshot, now)? { + ClaimFold::Stale => Ok(None), + ClaimFold::Enforced => { + self.save_unlocked(&goals)?; + Ok(None) + } + ClaimFold::Claimed => { + let claimed = goal.clone(); + self.save_unlocked(&goals)?; + Ok(Some(claimed)) + } } - - goal.begin_attempt(now)?; - goal.revision = goal.revision.saturating_add(1); - let claimed = goal.clone(); - self.save_unlocked(&goals)?; - Ok(Some(claimed)) } /// Atomically close a previously claimed attempt. The in-flight marker is @@ -787,62 +960,18 @@ impl GoalStore { completion: AttemptCompletion, now: NaiveDateTime, ) -> anyhow::Result> { - let Some(token) = claim.in_flight.as_ref() else { - anyhow::bail!("cannot finish a goal snapshot without an in-flight attempt"); - }; - let _guard = self.lock.lock().expect("goal store lock poisoned"); + anyhow::ensure!( + claim.in_flight.is_some(), + "cannot finish a goal snapshot without an in-flight attempt" + ); + let (_process_guard, _file_guard) = self.transaction()?; let mut goals = self.load_unlocked()?; let Some(goal) = goals.iter_mut().find(|goal| goal.id == claim.id) else { return Ok(None); }; - if goal.in_flight.as_ref() != Some(token) { + let Some(decision) = goal.fold_finish(claim, completion, now)? else { return Ok(None); - } - - let stopped_state = - (goal.state != GoalState::Running).then_some((goal.state, goal.note.clone())); - let was_terminal = goal.state.is_terminal(); - let decision = match completion { - AttemptCompletion::Evaluated(evaluation) => { - // The attempt itself was claimed while Running. Temporarily - // restore that state to fold its verdict, then reinstate a - // concurrent user stop unless the attempt reached a terminal - // outcome. Thus progress never resumes a preempted goal, while - // verified completion and hard envelope failures remain final. - if stopped_state.is_some() { - goal.state = GoalState::Running; - } - let decision = goal.apply_evaluation(evaluation, now)?; - if let Some((state, note)) = stopped_state { - if was_terminal || !goal.state.is_terminal() { - goal.state = state; - goal.note = note; - goal.updated_at = now; - ControllerDecision::Preempted - } else { - decision - } - } else { - decision - } - } - AttemptCompletion::ControllerError(reason) => { - let decision = goal.record_attempt_error(reason, now)?; - if let Some((state, note)) = stopped_state { - if was_terminal { - goal.state = state; - goal.note = note; - goal.updated_at = now; - ControllerDecision::Preempted - } else { - decision - } - } else { - decision - } - } }; - goal.revision = goal.revision.saturating_add(1); let finished = goal.clone(); self.save_unlocked(&goals)?; Ok(Some((finished, decision))) @@ -852,19 +981,18 @@ impl GoalStore { /// already owns the session. No goal work was dispatched, so this is the /// sole path that clears an in-flight marker without spending budget. pub fn abandon_attempt(&self, claim: &Goal) -> anyhow::Result> { - let Some(token) = claim.in_flight.as_ref() else { - anyhow::bail!("cannot abandon a goal snapshot without an in-flight attempt"); - }; - let _guard = self.lock.lock().expect("goal store lock poisoned"); + anyhow::ensure!( + claim.in_flight.is_some(), + "cannot abandon a goal snapshot without an in-flight attempt" + ); + let (_process_guard, _file_guard) = self.transaction()?; let mut goals = self.load_unlocked()?; let Some(goal) = goals.iter_mut().find(|goal| goal.id == claim.id) else { return Ok(None); }; - if goal.in_flight.as_ref() != Some(token) { + if !goal.fold_abandon(claim)? { return Ok(None); } - goal.in_flight = None; - goal.revision = goal.revision.saturating_add(1); let abandoned = goal.clone(); self.save_unlocked(&goals)?; Ok(Some(abandoned)) @@ -872,7 +1000,7 @@ impl GoalStore { /// Remove a goal; `false` when the id is unknown. pub fn remove(&self, id: &str) -> anyhow::Result { - let _guard = self.lock.lock().expect("goal store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut goals = self.load_unlocked()?; let before = goals.len(); goals.retain(|g| g.id != id); @@ -913,7 +1041,7 @@ impl GoalStore { owner: &OwnerKey, now: NaiveDateTime, ) -> anyhow::Result> { - let _guard = self.lock.lock().expect("goal store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut goals = self.load_unlocked()?; let mut paused = Vec::new(); for goal in goals @@ -943,7 +1071,7 @@ impl GoalStore { note: Option, now: NaiveDateTime, ) -> anyhow::Result { - let _guard = self.lock.lock().expect("goal store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut goals = self.load_unlocked()?; let Some(goal) = goals.iter_mut().find(|g| g.id == goal_id) else { return Ok(false); @@ -969,6 +1097,20 @@ fn goal_store_lock(path: &std::path::Path) -> Arc> { .clone() } +fn lock_store_file(path: &Path) -> anyhow::Result { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + let lock_path = path.with_extension("json.lock"); + let file = OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(&lock_path)?; + FileExt::lock_exclusive(&file)?; + Ok(file) +} + /// Storage seam for goals. The bundled JSON [`GoalStore`] implements it; a /// host's transactional repository (see PAL's "one orchestration store" plan) /// implements the same trait so controllers and tools never bind to a @@ -977,6 +1119,27 @@ fn goal_store_lock(path: &std::path::Path) -> Arc> { pub trait GoalRepository: Send + Sync { fn list(&self) -> anyhow::Result>; fn add(&self, goal: Goal) -> anyhow::Result<()>; + /// Create and persist a goal with a store-assigned id. + fn add_new( + &self, + owner: OwnerKey, + objective: String, + contract: CompletionContract, + budget: Budget, + now: NaiveDateTime, + ) -> anyhow::Result; + /// Replace every non-terminal goal of `owner` with one fresh goal, in one + /// transaction; returns the new goal and how many it replaced. + fn replace_current_for_owner( + &self, + owner: OwnerKey, + objective: String, + contract: CompletionContract, + budget: Budget, + now: NaiveDateTime, + ) -> anyhow::Result<(Goal, usize)>; + /// Remove every non-terminal goal of `owner`; returns how many. + fn remove_current_for_owner(&self, owner: &OwnerKey) -> anyhow::Result; fn get(&self, id: &str) -> anyhow::Result>; fn update(&self, goal: &Goal) -> anyhow::Result; fn claim_attempt(&self, snapshot: &Goal, now: NaiveDateTime) -> anyhow::Result>; @@ -1006,6 +1169,29 @@ impl GoalRepository for GoalStore { fn add(&self, goal: Goal) -> anyhow::Result<()> { GoalStore::add(self, goal) } + fn add_new( + &self, + owner: OwnerKey, + objective: String, + contract: CompletionContract, + budget: Budget, + now: NaiveDateTime, + ) -> anyhow::Result { + GoalStore::add_new(self, owner, objective, contract, budget, now) + } + fn replace_current_for_owner( + &self, + owner: OwnerKey, + objective: String, + contract: CompletionContract, + budget: Budget, + now: NaiveDateTime, + ) -> anyhow::Result<(Goal, usize)> { + GoalStore::replace_current_for_owner(self, owner, objective, contract, budget, now) + } + fn remove_current_for_owner(&self, owner: &OwnerKey) -> anyhow::Result { + GoalStore::remove_current_for_owner(self, owner) + } fn get(&self, id: &str) -> anyhow::Result> { GoalStore::get(self, id) } @@ -1796,6 +1982,49 @@ mod tests { assert_ne!(a.id, b.id); } + #[test] + fn replacing_an_owner_never_reuses_the_old_goal_id() { + let (store, _dir) = store(); + let now = at(2026, 7, 14, 9, 0); + let old = store + .add_new(owner(), "old", contract(), Budget::turns(1), now) + .unwrap(); + + let (new, replaced) = store + .replace_current_for_owner(owner(), "new", contract(), Budget::turns(1), now) + .unwrap(); + + assert_eq!(replaced, 1); + assert_ne!(old.id, new.id); + assert_eq!(store.list().unwrap(), vec![new]); + } + + #[test] + fn replacing_and_removing_current_goals_preserves_terminal_history() { + let (store, _dir) = store(); + let now = at(2026, 7, 14, 9, 0); + let mut history = store + .add_new(owner(), "finished", contract(), Budget::turns(1), now) + .unwrap(); + history.fail("recorded", now).unwrap(); + history = store.update(&history).unwrap(); + let current = store + .add_new(owner(), "current", contract(), Budget::turns(1), now) + .unwrap(); + + let (replacement, replaced) = store + .replace_current_for_owner(owner(), "new", contract(), Budget::turns(1), now) + .unwrap(); + assert_eq!(replaced, 1); + assert!(store.get(¤t.id).unwrap().is_none()); + assert_eq!(store.get(&history.id).unwrap(), Some(history.clone())); + + assert_eq!(store.remove_current_for_owner(&owner()).unwrap(), 1); + assert!(store.get(&replacement.id).unwrap().is_none()); + assert_eq!(store.list().unwrap(), vec![history]); + assert_eq!(store.remove_current_for_owner(&owner()).unwrap(), 0); + } + #[test] fn add_rejects_a_duplicate_id() { let (store, _dir) = store(); diff --git a/crates/agent_orchestration/src/waits.rs b/crates/agent_orchestration/src/waits.rs index 78685492..b96182d6 100644 --- a/crates/agent_orchestration/src/waits.rs +++ b/crates/agent_orchestration/src/waits.rs @@ -22,10 +22,12 @@ use crate::OwnerKey; use chrono::NaiveDateTime; +use fs2::FileExt; use serde::{Deserialize, Serialize}; use std::collections::HashMap; -use std::path::PathBuf; -use std::sync::{Arc, Mutex, OnceLock}; +use std::fs::{File, OpenOptions}; +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex, MutexGuard, OnceLock}; /// What a wait is blocked on. The domain layer treats each barrier's /// satisfaction opaquely: `Until` resolves against the clock alone, @@ -347,8 +349,8 @@ pub trait WaitProbe: Send + Sync { /// JSON-file persistence for durable waits (`waits.json`). Mirrors /// [`crate::goals::GoalStore`]: every operation reloads the current file and /// writes through atomic tmp+rename, instances for the same path share a -/// process-local lock, and [`WaitStore::update`] rejects a stale revision so a -/// sweep snapshot cannot clobber a concurrent cancellation. +/// process-local mutex, an OS advisory lock serializes transactions across +/// processes, and [`WaitStore::update`] rejects stale revisions. /// /// The store holds only *live* (`Armed`) waits. Resolving a wait is the job of /// the gateway sweep, which wakes the owning goal and then [`WaitStore::remove`]s @@ -370,9 +372,15 @@ impl WaitStore { Self { path, lock } } + fn transaction(&self) -> anyhow::Result<(MutexGuard<'_, ()>, File)> { + let process_guard = self.lock.lock().expect("wait store lock poisoned"); + let file_guard = lock_store_file(&self.path)?; + Ok((process_guard, file_guard)) + } + /// All persisted waits (normally all `Armed`); a missing file is empty. pub fn list(&self) -> anyhow::Result> { - let _guard = self.lock.lock().expect("wait store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; self.load_unlocked() } @@ -397,7 +405,7 @@ impl WaitStore { /// Add a wait with a caller-supplied id; the id must be unique. Prefer /// [`WaitStore::arm`], which also supersedes a goal's earlier barrier. pub fn add(&self, wait: Wait) -> anyhow::Result<()> { - let _guard = self.lock.lock().expect("wait store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut waits = self.load_unlocked()?; if waits.iter().any(|w| w.id == wait.id) { anyhow::bail!("wait id {} already exists", wait.id); @@ -418,7 +426,7 @@ impl WaitStore { now: NaiveDateTime, ) -> anyhow::Result { let goal_id = goal_id.into(); - let _guard = self.lock.lock().expect("wait store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut waits = self.load_unlocked()?; // Supersede: a goal never holds two armed barriers. waits.retain(|w| w.goal_id != goal_id); @@ -443,7 +451,7 @@ impl WaitStore { /// Persist a mutated wait against the revision it was loaded at. Errors on /// an unknown id (an update never silently creates) or a revision conflict. pub fn update(&self, wait: &Wait) -> anyhow::Result { - let _guard = self.lock.lock().expect("wait store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut waits = self.load_unlocked()?; let Some(slot) = waits.iter_mut().find(|w| w.id == wait.id) else { anyhow::bail!("unknown wait id {}", wait.id); @@ -465,7 +473,7 @@ impl WaitStore { /// Remove a wait; `false` when the id is unknown. A resolved wait is removed /// (not persisted terminal) — the goal's wake note carries the outcome. pub fn remove(&self, id: &str) -> anyhow::Result { - let _guard = self.lock.lock().expect("wait store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut waits = self.load_unlocked()?; let before = waits.len(); waits.retain(|w| w.id != id); @@ -505,7 +513,7 @@ impl WaitStore { /// ids. Since a resolved wait is removed anyway, cancellation is a removal — /// the reason is not persisted, only logged by the caller. pub fn cancel_for_goal(&self, goal_id: &str) -> anyhow::Result> { - let _guard = self.lock.lock().expect("wait store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut waits = self.load_unlocked()?; let mut cancelled = Vec::new(); waits.retain(|w| { @@ -531,7 +539,7 @@ impl WaitStore { owner: &OwnerKey, now: NaiveDateTime, ) -> anyhow::Result> { - let _guard = self.lock.lock().expect("wait store lock poisoned"); + let (_process_guard, _file_guard) = self.transaction()?; let mut waits = self.load_unlocked()?; let mut taken = Vec::new(); waits.retain(|w| { @@ -636,6 +644,20 @@ fn wait_store_lock(path: &std::path::Path) -> Arc> { .clone() } +fn lock_store_file(path: &Path) -> anyhow::Result { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + let lock_path = path.with_extension("json.lock"); + let file = OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(&lock_path)?; + FileExt::lock_exclusive(&file)?; + Ok(file) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/code_assistant_core/src/goal_commands.rs b/crates/code_assistant_core/src/goal_commands.rs index 5529d317..a54f1e1b 100644 --- a/crates/code_assistant_core/src/goal_commands.rs +++ b/crates/code_assistant_core/src/goal_commands.rs @@ -13,8 +13,8 @@ //! live here so every frontend gets the same behavior. use crate::goals::session_owner; -use agent_orchestration::goals::{Budget, CompletionContract, Goal, GoalState, GoalStore}; -use anyhow::{anyhow, Result}; +use agent_orchestration::goals::{Budget, CompletionContract, GoalStore}; +use anyhow::{bail, Result}; use chrono::NaiveDateTime; use std::path::Path; @@ -23,57 +23,35 @@ use std::path::Path; /// until the user notices. pub const USER_GOAL_MAX_TURNS: u32 = 10; +/// The complete user-facing syntax. Frontends show this when `/goal` is +/// submitted without the required objective. +pub const GOAL_USAGE: &str = "Usage: /goal or /goal cancel"; + /// A parsed `/goal` invocation. #[derive(Debug, Clone, PartialEq, Eq)] pub enum GoalCommand { - /// Bare `/goal` (or `/goal list`): the session's goals and their state. - List, - /// `/goal show [id]`: one goal in full detail. - Show { id: Option }, - /// `/goal pause [id]`: the controller stops driving the goal. - Pause { id: Option }, - /// `/goal resume [id]`: the controller drives the goal again. - Resume { id: Option }, - /// `/goal cancel [id]`: give up; the goal stays on the ledger as failed. - Cancel { id: Option }, - /// `/goal `: commit the session to a new goal whose contract - /// is the user's condition, verbatim. - Commit { condition: String }, + /// `/goal cancel`: delete the session's current goal. + Cancel, + /// `/goal `: set or replace the session's one goal. + Set { objective: String }, } impl GoalCommand { - /// Parse the raw text after `/goal`. A leading lifecycle keyword with at - /// most one trailing token is a lifecycle command; everything else is the - /// condition of a new goal (so prose that merely starts with "cancel …" - /// still commits a goal). - pub fn parse(input: &str) -> GoalCommand { + /// Parse the raw text after `/goal`. Only the exact single word `cancel` + /// is control syntax; every other non-empty string is the user's objective + /// verbatim. A bare `/goal` is deliberately invalid, so UIs can keep the + /// user in the `/goal ` template. + pub fn parse(input: &str) -> Result { let input = input.trim(); - let tokens: Vec<&str> = input.split_whitespace().collect(); - let id = (tokens.len() == 2).then(|| tokens[1].to_string()); - match (tokens.first().map(|t| t.to_lowercase()), tokens.len()) { - (None, _) => GoalCommand::List, - (Some(word), 1) => match word.as_str() { - "list" | "status" => GoalCommand::List, - "show" => GoalCommand::Show { id: None }, - "pause" => GoalCommand::Pause { id: None }, - "resume" => GoalCommand::Resume { id: None }, - "cancel" => GoalCommand::Cancel { id: None }, - _ => GoalCommand::Commit { - condition: input.to_string(), - }, - }, - (Some(word), 2) => match word.as_str() { - "show" => GoalCommand::Show { id }, - "pause" => GoalCommand::Pause { id }, - "resume" => GoalCommand::Resume { id }, - "cancel" => GoalCommand::Cancel { id }, - _ => GoalCommand::Commit { - condition: input.to_string(), - }, - }, - _ => GoalCommand::Commit { - condition: input.to_string(), - }, + if input.is_empty() { + bail!(GOAL_USAGE); + } + if input.eq_ignore_ascii_case("cancel") { + Ok(GoalCommand::Cancel) + } else { + Ok(GoalCommand::Set { + objective: input.to_string(), + }) } } } @@ -93,8 +71,7 @@ pub fn run_goal_command_now( } /// Execute a `/goal` command for the given session against the store at -/// `goals_path`. Returns the text the frontend should show; errors are -/// user-facing too (unknown id, ambiguous target, …). +/// `goals_path`. Returns the text the frontend should show. pub fn run_goal_command( goals_path: &Path, session_id: &str, @@ -103,210 +80,48 @@ pub fn run_goal_command( ) -> Result { let store = GoalStore::new(goals_path); match command { - GoalCommand::List => list(&store, session_id), - GoalCommand::Show { id } => show(&store, session_id, id.as_deref()), - GoalCommand::Pause { id } => pause(&store, session_id, id.as_deref(), now), - GoalCommand::Resume { id } => resume(&store, session_id, id.as_deref(), now), - GoalCommand::Cancel { id } => cancel(&store, session_id, id.as_deref(), now), - GoalCommand::Commit { condition } => commit(&store, session_id, condition, now), + GoalCommand::Cancel => cancel(&store, session_id), + GoalCommand::Set { objective } => set(&store, session_id, objective, now), } } -fn commit( - store: &GoalStore, - session_id: &str, - condition: &str, - now: NaiveDateTime, -) -> Result { - let condition = condition.trim(); - if condition.is_empty() { - return Err(anyhow!("the goal needs a condition: /goal ")); - } +fn set(store: &GoalStore, session_id: &str, objective: &str, now: NaiveDateTime) -> Result { // The user's condition is the whole contract: it is both the objective // and the outcome, verified against the session's own turn evidence. let contract = CompletionContract::new( - condition, + objective, "evidence from the session's turns (commands run and their results, artifacts \ produced) shows the condition holds", - "the user pauses or cancels the goal", + "the user replaces or cancels the goal", ); - let goal = store.add_new( + let (goal, replaced) = store.replace_current_for_owner( session_owner(session_id), - condition, + objective, contract, Budget::turns(USER_GOAL_MAX_TURNS), now, )?; + let verb = if replaced == 0 { "set" } else { "replaced" }; Ok(format!( - "Goal {} set: {}\nThe controller pursues it while the app is open (up to {} turns); \ - /goal shows progress, /goal cancel gives it up.", - goal.id, condition, goal.budget.max_turns, - )) -} - -fn list(store: &GoalStore, session_id: &str) -> Result { - let mut goals = session_goals(store, session_id)?; - if goals.is_empty() { - return Ok("No goals on record for this session. Set one with /goal .".into()); - } - // Active goals first, then terminal ones; stable within each group. - goals.sort_by_key(|g| g.state.is_terminal()); - let mut lines = vec![format!("{} goal(s):", goals.len())]; - for g in goals { - let note = g - .note - .as_deref() - .map(|n| format!(" — {n}")) - .unwrap_or_default(); - lines.push(format!( - "- {} [{}] {}/{} turns: {}{}", - g.id, - g.state.label(), - g.turns_used(), - g.budget.max_turns, - g.objective, - note, - )); - } - Ok(lines.join("\n")) -} - -fn show(store: &GoalStore, session_id: &str, id: Option<&str>) -> Result { - let goal = target_goal(store, session_id, id)?; - let mut lines = vec![ - format!( - "Goal {} [{}]: {}", - goal.id, - goal.state.label(), - goal.objective - ), - format!("- Done when: {}", goal.contract.outcome), - format!("- Verify by: {}", goal.contract.verification), - format!("- Stop if: {}", goal.contract.stop_condition), - format!( - "- Budget: {}/{} turns used", - goal.turns_used(), - goal.budget.max_turns, - ), - ]; - if let Some(note) = &goal.note { - lines.push(format!("Note: {note}")); - } - if !goal.attempts.is_empty() { - lines.push("Recent attempts:".to_string()); - for attempt in goal.attempts.iter().rev().take(5).rev() { - lines.push(format!( - "- {} [{:?}] {}", - attempt.at.format("%Y-%m-%d %H:%M"), - attempt.verdict, - attempt.summary, - )); - } - } - Ok(lines.join("\n")) -} - -fn pause( - store: &GoalStore, - session_id: &str, - id: Option<&str>, - now: NaiveDateTime, -) -> Result { - let mut goal = target_goal(store, session_id, id)?; - if goal.state == GoalState::Paused { - return Ok(format!("Goal {} is already paused.", goal.id)); - } - goal.pause(now) - .map_err(|_| finished_error(&goal.id, goal.state))?; - store.update(&goal)?; - Ok(format!( - "Paused goal {}. /goal resume drives it again.", - goal.id - )) -} - -fn resume( - store: &GoalStore, - session_id: &str, - id: Option<&str>, - now: NaiveDateTime, -) -> Result { - let mut goal = target_goal(store, session_id, id)?; - if goal.state == GoalState::Running { - return Ok(format!("Goal {} is already running.", goal.id)); - } - goal.resume(now) - .map_err(|_| finished_error(&goal.id, goal.state))?; - store.update(&goal)?; - Ok(format!( - "Resumed goal {}; the controller drives it on the next pass.", - goal.id - )) -} - -fn cancel( - store: &GoalStore, - session_id: &str, - id: Option<&str>, - now: NaiveDateTime, -) -> Result { - let mut goal = target_goal(store, session_id, id)?; - goal.fail("cancelled by the user", now) - .map_err(|_| finished_error(&goal.id, goal.state))?; - store.update(&goal)?; - Ok(format!( - "Cancelled goal {}; it stays on the ledger as failed.", - goal.id + "Goal {verb}: {}\nThe controller pursues it while the app is open (up to {} turns). \ + Use /goal cancel to remove it.", + goal.objective, goal.budget.max_turns, )) } -fn session_goals(store: &GoalStore, session_id: &str) -> Result> { - let owner = session_owner(session_id); - Ok(store - .list()? - .into_iter() - .filter(|g| g.owner == owner) - .collect()) -} - -/// Resolve the goal a lifecycle command targets: an explicit id (which must -/// belong to this session), or — when omitted — the session's single -/// non-terminal goal. -fn target_goal(store: &GoalStore, session_id: &str, id: Option<&str>) -> Result { - if let Some(id) = id { - let goal = store - .get(id)? - .ok_or_else(|| anyhow!("no goal with id {id}"))?; - if goal.owner != session_owner(session_id) { - return Err(anyhow!("goal {id} belongs to another session")); - } - return Ok(goal); +fn cancel(store: &GoalStore, session_id: &str) -> Result { + let removed = store.remove_current_for_owner(&session_owner(session_id))?; + if removed == 0 { + Ok("No goal is set for this session.".into()) + } else { + Ok("Goal cancelled and removed.".into()) } - let mut open: Vec = session_goals(store, session_id)? - .into_iter() - .filter(|g| !g.state.is_terminal()) - .collect(); - match open.len() { - 0 => Err(anyhow!("this session has no open goal")), - 1 => Ok(open.remove(0)), - _ => Err(anyhow!( - "this session has {} open goals — name one: {}", - open.len(), - open.iter() - .map(|g| g.id.as_str()) - .collect::>() - .join(", "), - )), - } -} - -fn finished_error(id: &str, state: GoalState) -> anyhow::Error { - anyhow!("goal {id} is already {} and cannot change", state.label()) } #[cfg(test)] mod tests { use super::*; + use agent_orchestration::goals::GoalState; use chrono::NaiveDate; fn now() -> NaiveDateTime { @@ -317,12 +132,8 @@ mod tests { } fn run(dir: &std::path::Path, session_id: &str, input: &str) -> Result { - run_goal_command( - &dir.join("goals.json"), - session_id, - &GoalCommand::parse(input), - now(), - ) + let command = GoalCommand::parse(input)?; + run_goal_command(&dir.join("goals.json"), session_id, &command, now()) } fn store(dir: &std::path::Path) -> GoalStore { @@ -330,35 +141,26 @@ mod tests { } #[test] - fn parse_separates_lifecycle_keywords_from_conditions() { - assert_eq!(GoalCommand::parse(""), GoalCommand::List); - assert_eq!(GoalCommand::parse(" list "), GoalCommand::List); - assert_eq!(GoalCommand::parse("status"), GoalCommand::List); - assert_eq!(GoalCommand::parse("show"), GoalCommand::Show { id: None }); + fn parse_enforces_the_small_user_owned_command_language() { + assert_eq!(GoalCommand::parse("").unwrap_err().to_string(), GOAL_USAGE); + assert_eq!(GoalCommand::parse(" CANCEL ").unwrap(), GoalCommand::Cancel); assert_eq!( - GoalCommand::parse("pause g-1"), - GoalCommand::Pause { - id: Some("g-1".into()) + GoalCommand::parse("all tests pass").unwrap(), + GoalCommand::Set { + objective: "all tests pass".into() } ); + // Only exact `cancel` is control syntax; this remains an objective. assert_eq!( - GoalCommand::parse("all tests pass"), - GoalCommand::Commit { - condition: "all tests pass".into() - } - ); - // A three-word phrase starting with a keyword is a condition, not a - // lifecycle command with a two-token id. - assert_eq!( - GoalCommand::parse("cancel the subscription in the billing portal"), - GoalCommand::Commit { - condition: "cancel the subscription in the billing portal".into() + GoalCommand::parse("cancel the subscription").unwrap(), + GoalCommand::Set { + objective: "cancel the subscription".into() } ); } #[test] - fn commit_persists_a_session_owned_running_goal_with_the_condition_as_contract() { + fn set_persists_one_session_owned_goal_with_the_objective_as_contract() { let dir = tempfile::tempdir().unwrap(); let message = run(dir.path(), "sess-1", "the CI badge is green").unwrap(); @@ -369,109 +171,47 @@ mod tests { assert_eq!(goals[0].objective, "the CI badge is green"); assert_eq!(goals[0].contract.outcome, "the CI badge is green"); assert_eq!(goals[0].budget.max_turns, USER_GOAL_MAX_TURNS); - assert!(message.contains(&goals[0].id)); + assert!(message.contains("Goal set")); } #[test] - fn list_and_lifecycle_are_scoped_to_the_session() { + fn setting_again_replaces_only_the_current_sessions_goal() { let dir = tempfile::tempdir().unwrap(); - run(dir.path(), "sess-1", "mine holds").unwrap(); + run(dir.path(), "sess-1", "old objective").unwrap(); run(dir.path(), "sess-2", "theirs holds").unwrap(); + let message = run(dir.path(), "sess-1", "new objective").unwrap(); - let listed = run(dir.path(), "sess-1", "").unwrap(); - assert!(listed.contains("mine holds")); - assert!(!listed.contains("theirs holds")); - - let foreign_id = store(dir.path()) - .list() - .unwrap() - .into_iter() - .find(|g| g.owner == session_owner("sess-2")) - .unwrap() - .id; - let error = run(dir.path(), "sess-1", &format!("cancel {foreign_id}")).unwrap_err(); - assert!(error.to_string().contains("another session")); + let goals = store(dir.path()).list().unwrap(); + assert_eq!(goals.len(), 2); + assert!( + goals + .iter() + .any(|goal| goal.owner == session_owner("sess-1") + && goal.objective == "new objective") + ); + assert!(goals + .iter() + .any(|goal| goal.owner == session_owner("sess-2") && goal.objective == "theirs holds")); + assert!(!goals.iter().any(|goal| goal.objective == "old objective")); + assert!(message.contains("Goal replaced")); } #[test] - fn lifecycle_without_an_id_targets_the_single_open_goal() { + fn cancel_deletes_the_current_sessions_goal() { let dir = tempfile::tempdir().unwrap(); - run(dir.path(), "sess-1", "the widget ships").unwrap(); + run(dir.path(), "sess-1", "mine").unwrap(); + run(dir.path(), "sess-2", "theirs").unwrap(); - run(dir.path(), "sess-1", "pause").unwrap(); - assert_eq!( - store(dir.path()).list().unwrap()[0].state, - GoalState::Paused - ); + let message = run(dir.path(), "sess-1", "cancel").unwrap(); + let goals = store(dir.path()).list().unwrap(); + assert_eq!(goals.len(), 1); + assert_eq!(goals[0].owner, session_owner("sess-2")); + assert_eq!(goals[0].state, GoalState::Running); + assert_eq!(message, "Goal cancelled and removed."); - run(dir.path(), "sess-1", "resume").unwrap(); assert_eq!( - store(dir.path()).list().unwrap()[0].state, - GoalState::Running + run(dir.path(), "sess-1", "cancel").unwrap(), + "No goal is set for this session." ); - - run(dir.path(), "sess-1", "cancel").unwrap(); - let goal = &store(dir.path()).list().unwrap()[0]; - assert_eq!(goal.state, GoalState::Failed); - assert_eq!(goal.note.as_deref(), Some("cancelled by the user")); - - // No open goal left: lifecycle commands now need nothing to act on. - let error = run(dir.path(), "sess-1", "pause").unwrap_err(); - assert!(error.to_string().contains("no open goal")); - } - - #[test] - fn lifecycle_without_an_id_refuses_an_ambiguous_target() { - let dir = tempfile::tempdir().unwrap(); - run(dir.path(), "sess-1", "goal one holds").unwrap(); - run(dir.path(), "sess-1", "goal two holds").unwrap(); - - let error = run(dir.path(), "sess-1", "cancel").unwrap_err(); - assert!(error.to_string().contains("2 open goals")); - } - - #[test] - fn show_renders_the_contract_and_ledger() { - let dir = tempfile::tempdir().unwrap(); - run(dir.path(), "sess-1", "the check passes").unwrap(); - - let shown = run(dir.path(), "sess-1", "show").unwrap(); - assert!(shown.contains("Done when: the check passes")); - assert!(shown.contains(&format!("0/{USER_GOAL_MAX_TURNS} turns used"))); - } - - #[test] - fn pause_during_an_in_flight_turn_does_not_clobber_the_claim() { - let dir = tempfile::tempdir().unwrap(); - run(dir.path(), "sess-1", "ship it").unwrap(); - let store = store(dir.path()); - let snapshot = store.list().unwrap().remove(0); - let claim = store.claim_attempt(&snapshot, now()).unwrap().unwrap(); - - // The user pauses while the controller's turn is in flight. - run(dir.path(), "sess-1", &format!("pause {}", claim.id)).unwrap(); - - // The pause advanced the revision but preserved the claim token, so - // the completed turn still merges into the ledger (as Preempted). - let paused = store.get(&claim.id).unwrap().unwrap(); - assert_eq!(paused.state, GoalState::Paused); - assert!(paused.in_flight.is_some(), "pause must not erase the claim"); - use agent_orchestration::goals::{ - AttemptCompletion, AttemptVerdict, ControllerDecision, Evaluation, - }; - let (merged, decision) = store - .finish_attempt( - &claim, - AttemptCompletion::Evaluated(Evaluation::new( - AttemptVerdict::Progressed, - "made progress", - )), - now(), - ) - .unwrap() - .unwrap(); - assert_eq!(decision, ControllerDecision::Preempted); - assert_eq!(merged.state, GoalState::Paused, "user stop wins"); - assert_eq!(merged.attempts.len(), 1, "the turn still spent its budget"); } } diff --git a/crates/code_assistant_core/src/goals.rs b/crates/code_assistant_core/src/goals.rs index cb2bcc3d..cbaede9e 100644 --- a/crates/code_assistant_core/src/goals.rs +++ b/crates/code_assistant_core/src/goals.rs @@ -18,8 +18,9 @@ //! - every claimed attempt consumes budget or closes with an explicit //! abandonment (the sole abandonment path is an atomic `Busy` answer from //! `start_turn_if_idle`, where no work was dispatched); -//! - a stale in-flight marker from an earlier process is folded as an -//! interrupted attempt — a crash cannot silently refund work; +//! - an in-flight marker is never adopted or recovered here: it may belong to +//! another live code-assistant instance, and crash recovery is a host policy +//! reserved for always-on consumers such as pal; //! - concurrent pause/cancel and turn completion merge through the store's //! revision/claim-token semantics instead of overwriting each other. @@ -257,19 +258,15 @@ impl GoalController { } async fn drive_goal(&self, snapshot: Goal, session_id: &str, now: NaiveDateTime) -> Result<()> { - // A leftover in-flight marker means an earlier process died mid-turn: - // close it as an interrupted attempt so the crash cannot refund the - // claimed budget. (Within one process, passes run sequentially and - // every dispatched turn is awaited, so a live claim never appears - // here.) + let drive_started = tokio::time::Instant::now(); + + // Never infer a crash from persisted state. Another code-assistant + // process may still own this session and turn; its file-backed session + // updates are the cross-instance source of truth. If the owner really + // disappeared, code-assistant deliberately leaves the claim parked + // until the user replaces or cancels the goal. Automatic adoption and + // restart belongs to an always-on host such as pal. if snapshot.in_flight.is_some() { - self.goals.finish_attempt( - &snapshot, - AttemptCompletion::ControllerError( - "attempt interrupted (process ended mid-turn)".into(), - ), - now, - )?; return Ok(()); } @@ -310,10 +307,11 @@ impl GoalController { return Ok(()); } Err(error) => { + let finished_at = advance_wall_clock(now, drive_started.elapsed()); self.goals.finish_attempt( &claim, AttemptCompletion::ControllerError(format!("turn dispatch failed: {error:#}")), - now, + finished_at, )?; return Ok(()); } @@ -322,29 +320,31 @@ impl GoalController { let outcome = match handle.wait().await { Ok(outcome) => outcome, Err(error) => { + let finished_at = advance_wall_clock(now, drive_started.elapsed()); self.goals.finish_attempt( &claim, AttemptCompletion::ControllerError(format!("turn outcome lost: {error:#}")), - now, + finished_at, )?; return Ok(()); } }; if let TurnStatus::Failed { error } = &outcome.status { + let finished_at = advance_wall_clock(now, drive_started.elapsed()); self.goals.finish_attempt( &claim, AttemptCompletion::ControllerError(format!("turn failed: {error}")), - now, + finished_at, )?; return Ok(()); } // A user message absorbed mid-turn does NOT pause the goal — the // user's turn simply takes natural priority and the goal continues - // afterwards (`/goal pause|cancel` is the deliberate stop). Only an + // afterwards (`/goal cancel` is the deliberate stop). Only an // explicit run cancel is a stop signal strong enough to park the - // session's goals until the user resumes them. + // session's goals until the user replaces or removes the goal. let user_took_over = outcome.status == TurnStatus::Cancelled; let evidence = goal_turn_evidence(&outcome); @@ -355,7 +355,10 @@ impl GoalController { } }; - if let Some((goal, decision)) = self.goals.finish_attempt(&claim, completion, now)? { + let finished_at = advance_wall_clock(now, drive_started.elapsed()); + if let Some((goal, decision)) = + self.goals.finish_attempt(&claim, completion, finished_at)? + { debug!("goal {}: {:?}", goal.id, decision); if let ControllerDecision::Wait(request) = decision { self.waits.arm( @@ -363,18 +366,25 @@ impl GoalController { goal.owner.clone(), request.kind, request.timeout, - now, + finished_at, )?; } } if user_took_over { - self.goals.preempt_owner(&claim.owner, now)?; + self.goals.preempt_owner(&claim.owner, finished_at)?; } Ok(()) } } +fn advance_wall_clock(base: NaiveDateTime, elapsed: std::time::Duration) -> NaiveDateTime { + chrono::Duration::from_std(elapsed) + .ok() + .and_then(|elapsed| base.checked_add_signed(elapsed)) + .unwrap_or(base) +} + /// Spawn the periodic controller loop (host-driven continuation: it lives and /// dies with the process). Modeled on /// [`spawn_wakeup_scheduler`](crate::session::spawn_wakeup_scheduler). @@ -662,27 +672,54 @@ mod tests { } #[tokio::test(flavor = "multi_thread")] - async fn a_stale_in_flight_claim_is_folded_as_an_interrupted_attempt() { + async fn an_in_flight_claim_is_never_recovered_by_code_assistant() { let fx = Fixture::new(completing_llm("unused")).await; let (_, goal) = fx.session_with_goal(3).await; - // Simulate a process that died mid-turn: the claim is persisted but - // no turn is running. - fx.goals.claim_attempt(&goal, now()).unwrap().unwrap(); + let claim = fx.goals.claim_attempt(&goal, now()).unwrap().unwrap(); let evaluator = Arc::new(ScriptedEvaluator::new(vec![])); fx.controller(evaluator).pass_at(now()).await.unwrap(); let goal = fx.goals.get(&goal.id).unwrap().unwrap(); - assert!(goal.in_flight.is_none()); - assert_eq!( - goal.attempts.len(), - 1, - "the crash cannot refund the claimed turn" - ); - assert_eq!(goal.attempts[0].verdict, AttemptVerdict::Error); + assert_eq!(goal.in_flight, claim.in_flight); + assert!(goal.attempts.is_empty()); assert_eq!(goal.state, GoalState::Running); } + #[tokio::test(flavor = "multi_thread")] + async fn a_goal_that_finishes_after_its_deadline_cannot_complete() { + struct SlowSatisfiedEvaluator; + + #[async_trait::async_trait] + impl GoalEvaluator for SlowSatisfiedEvaluator { + async fn evaluate( + &self, + _goal: &Goal, + _turn: &GoalTurnOutcome, + ) -> anyhow::Result { + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + Ok(Evaluation::new( + AttemptVerdict::Satisfied, + "finished too late", + )) + } + } + + let fx = Fixture::new(completing_llm("done")).await; + let (_, mut goal) = fx.session_with_goal(3).await; + goal.budget.deadline = Some(now() + chrono::Duration::milliseconds(10)); + goal = fx.goals.update(&goal).unwrap(); + + fx.controller(Arc::new(SlowSatisfiedEvaluator)) + .pass_at(now()) + .await + .unwrap(); + + let goal = fx.goals.get(&goal.id).unwrap().unwrap(); + assert_eq!(goal.state, GoalState::Failed); + assert_eq!(goal.note.as_deref(), Some("deadline passed")); + } + #[tokio::test(flavor = "multi_thread")] async fn a_waiting_verdict_parks_the_goal_and_a_due_clock_barrier_wakes_it() { let fx = Fixture::new(Arc::new(|_model| { diff --git a/crates/ui_acp/src/agent.rs b/crates/ui_acp/src/agent.rs index c2b7f26b..21cb91e3 100644 --- a/crates/ui_acp/src/agent.rs +++ b/crates/ui_acp/src/agent.rs @@ -91,6 +91,29 @@ fn slash_command_token(prompt: &[acp::ContentBlock]) -> Option { Some(rest.to_string()) } +/// Extract arguments from a `/goal` prompt while rejecting lookalike commands +/// such as `/goals`. ACP clients send command arguments as ordinary prompt +/// text, so this is the same boundary used by the native UIs. +fn goal_command_args(prompt: &[acp::ContentBlock]) -> Option { + let text = prompt.iter().find_map(|block| match block { + acp::ContentBlock::Text(text) => Some(text.text.trim()), + _ => None, + })?; + let split = text + .char_indices() + .find(|(_, ch)| ch.is_whitespace()) + .map(|(index, _)| index) + .unwrap_or(text.len()); + let (command, rest) = text.split_at(split); + command + .eq_ignore_ascii_case("/goal") + .then(|| rest.trim_start().to_string()) +} + +fn goal_command_has_only_text(prompt: &[acp::ContentBlock]) -> bool { + matches!(prompt, [acp::ContentBlock::Text(_)]) +} + /// Pending session that hasn't been persisted yet (deferred until first prompt). #[derive(Clone)] struct PendingSession { @@ -316,15 +339,29 @@ impl AgentState { project_name: &str, ) -> Vec { let config = SkillsConfig::load(); - discover_session_catalog(project_manager, project_name, &config) - .into_iter() - .map(|(skill, _scope_token)| { - acp::AvailableCommand::new( - skill.name.clone(), - format!("[{}] {}", skill.scope.label(), skill.description), - ) - }) - .collect() + let mut commands = vec![acp::AvailableCommand::new( + "goal", + "Set or replace the session goal: /goal ; remove it with /goal cancel", + )]; + commands.extend( + discover_session_catalog(project_manager, project_name, &config) + .into_iter() + .map(|(skill, _scope_token)| { + acp::AvailableCommand::new( + skill.name.clone(), + format!("[{}] {}", skill.scope.label(), skill.description), + ) + }), + ); + commands + } + + fn send_agent_message(&self, session_id: &acp::SessionId, message: String) { + let content = acp::ContentBlock::Text(acp::TextContent::new(message)); + let update = acp::SessionUpdate::AgentMessageChunk(acp::ContentChunk::new(content)); + let notification = acp::SessionNotification::new(session_id.clone(), update); + let (ack_tx, _ack_rx) = oneshot::channel(); + let _ = self.session_update_tx.send((notification, ack_tx)); } /// Push an `available_commands_update` for `session_id` to the client. @@ -822,6 +859,29 @@ impl AgentState { })?; } + // `/goal` is UI control, never model input. It directly replaces or + // removes the session-owned goal and completes this ACP prompt without + // starting an agent turn. + if let Some(args) = goal_command_args(&arguments.prompt) { + let message = if !goal_command_has_only_text(&arguments.prompt) { + "A goal cannot contain attachments or additional content. Use /goal \ + or /goal cancel." + .to_string() + } else { + code_assistant_core::goal_commands::GoalCommand::parse(&args) + .and_then(|command| { + code_assistant_core::goal_commands::run_goal_command_now( + &code_assistant_core::goals::default_goals_path(), + &arguments.session_id.0, + &command, + ) + }) + .unwrap_or_else(|error| format!("/goal: {error:#}")) + }; + self.send_agent_message(&arguments.session_id, message); + return Ok(acp::PromptResponse::new(acp::StopReason::EndTurn)); + } + let terminal_supported = { let caps = self.client_capabilities.lock().await; caps.as_ref().map(|caps| caps.terminal).unwrap_or(false) @@ -1163,4 +1223,32 @@ mod tests { // Empty prompt. assert_eq!(slash_command_token(&[]), None); } + + #[test] + fn goal_arguments_preserve_the_objective_and_reject_lookalikes() { + let prompt = vec![acp::ContentBlock::Text(acp::TextContent::new( + "/goal all tests pass", + ))]; + assert_eq!( + goal_command_args(&prompt).as_deref(), + Some("all tests pass") + ); + + let bare = vec![acp::ContentBlock::Text(acp::TextContent::new("/goal"))]; + assert_eq!(goal_command_args(&bare).as_deref(), Some("")); + + let lookalike = vec![acp::ContentBlock::Text(acp::TextContent::new("/goals"))]; + assert_eq!(goal_command_args(&lookalike), None); + + let with_attachment = vec![ + acp::ContentBlock::Text(acp::TextContent::new("/goal inspect the image")), + acp::ContentBlock::Image(acp::ImageContent::new("image-data", "image/png")), + ]; + assert_eq!( + goal_command_args(&with_attachment).as_deref(), + Some("inspect the image") + ); + assert!(!goal_command_has_only_text(&with_attachment)); + assert!(goal_command_has_only_text(&prompt)); + } } diff --git a/crates/ui_gpui/src/app/commands.rs b/crates/ui_gpui/src/app/commands.rs index 82b83960..ccedb2c8 100644 --- a/crates/ui_gpui/src/app/commands.rs +++ b/crates/ui_gpui/src/app/commands.rs @@ -45,6 +45,12 @@ impl Gpui { self.push_event(UiEvent::DisplayError { message }); } + pub(crate) fn display_status(&self, message: impl Into) { + self.push_event(UiEvent::ShowTransientStatus { + message: message.into(), + }); + } + fn is_current_session(&self, session_id: &str) -> bool { self.current_session_id.lock().unwrap().as_deref() == Some(session_id) } @@ -217,6 +223,23 @@ impl Gpui { }); } + /// Execute the user-owned `/goal` command without sending it to the LLM. + pub(crate) fn cmd_goal_command(&self, session_id: String, args: String) { + let gpui = self.clone(); + self.dispatch(async move { + let message = code_assistant_core::goal_commands::GoalCommand::parse(&args) + .and_then(|command| { + code_assistant_core::goal_commands::run_goal_command_now( + &code_assistant_core::goals::default_goals_path(), + &session_id, + &command, + ) + }) + .unwrap_or_else(|error| format!("/goal: {error:#}")); + gpui.display_status(message); + }); + } + pub(crate) fn cmd_queue_user_message( &self, session_id: String, diff --git a/crates/ui_gpui/src/input/skill_completion.rs b/crates/ui_gpui/src/input/skill_completion.rs index 2894d8a0..ab1aaeb5 100644 --- a/crates/ui_gpui/src/input/skill_completion.rs +++ b/crates/ui_gpui/src/input/skill_completion.rs @@ -1,9 +1,10 @@ -//! A `CompletionProvider` for the input composer that autocompletes skills. +//! A `CompletionProvider` for the input composer's slash commands and skills. //! //! When the current line starts with `/`, this provider offers the //! session's skills (read from the [`crate::Gpui`] global, populated via //! `BackendEvent::ListSkills`). Selecting one replaces the typed `/...` with -//! `/`. On submit, [`super::InputArea::on_enter`] recognizes a +//! `/`. The built-in `/goal` entry expands to the required +//! `/goal ` template. On submit, [`super::InputArea::on_enter`] recognizes a //! lone `/` that matches a known skill and translates it into a //! skill invocation (see [`skill_invocation_from_input`]). @@ -21,7 +22,9 @@ use lsp_types::{ use crate::Gpui; -/// Completion provider that suggests skills after a leading `/`. +const GOAL_DESCRIPTION: &str = "Set completion criteria, or type /goal cancel"; + +/// Completion provider that suggests built-in commands and skills after `/`. #[derive(Default)] pub struct SkillCompletionProvider; @@ -68,25 +71,43 @@ impl gpui_component::input::CompletionProvider for SkillCompletionProvider { let start = text.offset_to_position(line_start); let end = text.offset_to_position(offset); - let items: Vec = skills - .iter() - .filter(|s| { - query.is_empty() - || s.name.to_lowercase().contains(&query) - || s.description.to_lowercase().contains(&query) - }) - .map(|s| CompletionItem { - label: s.name.clone(), + let mut items = Vec::new(); + if query.is_empty() + || "goal".contains(&query) + || GOAL_DESCRIPTION.to_lowercase().contains(&query) + { + items.push(CompletionItem { + label: "goal".into(), kind: Some(CompletionItemKind::SNIPPET), - detail: Some(format!("({}) {}", s.scope_label, s.description)), - filter_text: Some(s.name.clone()), + detail: Some(GOAL_DESCRIPTION.into()), + filter_text: Some("goal".into()), text_edit: Some(CompletionTextEdit::Edit(TextEdit { range: lsp_types::Range { start, end }, - new_text: format!("/{}", s.name), + new_text: "/goal ".into(), })), ..Default::default() - }) - .collect(); + }); + } + items.extend( + skills + .iter() + .filter(|s| { + query.is_empty() + || s.name.to_lowercase().contains(&query) + || s.description.to_lowercase().contains(&query) + }) + .map(|s| CompletionItem { + label: s.name.clone(), + kind: Some(CompletionItemKind::SNIPPET), + detail: Some(format!("({}) {}", s.scope_label, s.description)), + filter_text: Some(s.name.clone()), + text_edit: Some(CompletionTextEdit::Edit(TextEdit { + range: lsp_types::Range { start, end }, + new_text: format!("/{}", s.name), + })), + ..Default::default() + }), + ); Task::ready(Ok(CompletionResponse::Array(items))) } @@ -126,11 +147,14 @@ pub enum SlashState { /// when the completion menu is open. fn query_matches(query: &str, skills: &[SkillCatalogEntry]) -> bool { let q = query.to_lowercase(); - skills.iter().any(|s| { - q.is_empty() - || s.name.to_lowercase().contains(&q) - || s.description.to_lowercase().contains(&q) - }) + q.is_empty() + || "goal".contains(&q) + || GOAL_DESCRIPTION.to_lowercase().contains(&q) + || skills.iter().any(|s| { + q.is_empty() + || s.name.to_lowercase().contains(&q) + || s.description.to_lowercase().contains(&q) + }) } /// Classify the composer input for Enter handling (see [`SlashState`]). @@ -201,6 +225,8 @@ mod tests { slash_completion_state("/rev", &skills), SlashState::MenuOpen ); + assert_eq!(slash_completion_state("/go", &[]), SlashState::MenuOpen); + assert_eq!(slash_completion_state("/goal", &[]), SlashState::MenuOpen); } #[test] diff --git a/crates/ui_gpui/src/main_screen/mod.rs b/crates/ui_gpui/src/main_screen/mod.rs index 8d3c3ed8..289c5fdf 100644 --- a/crates/ui_gpui/src/main_screen/mod.rs +++ b/crates/ui_gpui/src/main_screen/mod.rs @@ -35,6 +35,21 @@ use tracing::{debug, error, warn}; const SIDEBAR_ANIMATION_DURATION_MS: f32 = 250.0; const SIDEBAR_ANIMATION_FRAME_MS: u64 = 8; // ~120 FPS +/// Return the argument text for a syntactically separate `/goal` command. +/// Prefixes such as `/goals` remain ordinary input. +fn goal_command_args(input: &str) -> Option<&str> { + let input = input.trim(); + let split = input + .char_indices() + .find(|(_, ch)| ch.is_whitespace()) + .map(|(index, _)| index) + .unwrap_or(input.len()); + let (command, rest) = input.split_at(split); + command + .eq_ignore_ascii_case("/goal") + .then(|| rest.trim_start()) +} + #[derive(Clone, Debug, PartialEq)] enum SidebarAnimationState { Idle, @@ -444,6 +459,22 @@ impl MainScreen { attachments, branch_parent_id, } => { + if let Some(args) = goal_command_args(content) { + let gpui = cx + .try_global::() + .expect("Failed to obtain Gpui global"); + if !attachments.is_empty() || branch_parent_id.is_some() { + gpui.display_status( + "A goal cannot contain attachments or be submitted while editing. \ + Use /goal or /goal cancel.", + ); + } else if let Some(session_id) = self.current_session_id.clone() { + gpui.cmd_goal_command(session_id, args.to_string()); + } else { + gpui.display_status("No active session to manage goals"); + } + return; + } if let Some(session_id) = self.current_session_id.clone() { self.send_message( &session_id, @@ -1559,3 +1590,20 @@ impl Render for MainScreen { .when_some(new_project_dialog, |el, dialog| el.child(dialog)) } } + +#[cfg(test)] +mod goal_command_tests { + use super::goal_command_args; + + #[test] + fn extracts_goal_arguments_without_matching_lookalikes() { + assert_eq!( + goal_command_args("/goal all tests pass"), + Some("all tests pass") + ); + assert_eq!(goal_command_args(" /GOAL cancel "), Some("cancel")); + assert_eq!(goal_command_args("/goal"), Some("")); + assert_eq!(goal_command_args("/goals nope"), None); + assert_eq!(goal_command_args("ordinary message"), None); + } +} diff --git a/crates/ui_terminal/src/app.rs b/crates/ui_terminal/src/app.rs index 48ee2449..c85b8739 100644 --- a/crates/ui_terminal/src/app.rs +++ b/crates/ui_terminal/src/app.rs @@ -569,15 +569,20 @@ async fn handle_command_result( }; // Goals are user-set: the command works the store directly, the // agent is never asked to create or steer one. - let command = code_assistant_core::goal_commands::GoalCommand::parse(&args); - let message = code_assistant_core::goal_commands::run_goal_command_now( - &code_assistant_core::goals::default_goals_path(), - &session_id, - &command, - ) - .unwrap_or_else(|error| format!("/goal: {error:#}")); + let message = code_assistant_core::goal_commands::GoalCommand::parse(&args) + .and_then(|command| { + code_assistant_core::goal_commands::run_goal_command_now( + &code_assistant_core::goals::default_goals_path(), + &session_id, + &command, + ) + }) + .unwrap_or_else(|error| format!("/goal: {error:#}")); app_state.lock().await.set_info_message(Some(message)); } + // Produced only by the popup and handled where the composer is + // available in the event loop. + CommandResult::InsertInputTemplate(_) => {} CommandResult::ShowPermissionTier => { let mut state = app_state.lock().await; let message = match state.current_permission_tier { @@ -1107,8 +1112,20 @@ async fn event_loop( if !top_was_permission_prompt { clear_slash_command_word(&mut input_manager.textarea); } - handle_command_result(cmd, &app_state, &renderer, &actions) + if let crate::commands::CommandResult::InsertInputTemplate( + template, + ) = cmd + { + input_manager.textarea.insert_str(&template); + } else { + handle_command_result( + cmd, + &app_state, + &renderer, + &actions, + ) .await; + } } } } diff --git a/crates/ui_terminal/src/commands.rs b/crates/ui_terminal/src/commands.rs index 0b058ce0..2d14cd94 100644 --- a/crates/ui_terminal/src/commands.rs +++ b/crates/ui_terminal/src/commands.rs @@ -74,8 +74,7 @@ pub fn all_commands() -> &'static [SlashCommand] { SlashCommand { name: "goal", aliases: &[], - description: - "Set a durable goal: /goal (bare /goal lists; show|pause|resume|cancel [id])", + description: "Set or replace the goal: /goal ; /goal cancel removes it", }, SlashCommand { name: "skill", @@ -128,6 +127,8 @@ pub enum CommandResult { /// the raw text after `/goal`, parsed by /// `code_assistant_core::goal_commands::GoalCommand`. Goal { args: String }, + /// Replace the composer contents with a slash-command input template. + InsertInputTemplate(String), /// Show the current permission tier. ShowPermissionTier, /// Switch the permission tier. diff --git a/crates/ui_terminal/src/input.rs b/crates/ui_terminal/src/input.rs index 391a8119..ae819f2f 100644 --- a/crates/ui_terminal/src/input.rs +++ b/crates/ui_terminal/src/input.rs @@ -217,6 +217,10 @@ impl InputManager { KeyEventResult::InvokeSkill { scope, name } } CommandResult::Goal { args } => KeyEventResult::Goal { args }, + CommandResult::InsertInputTemplate(template) => { + self.textarea.insert_str(&template); + KeyEventResult::Continue + } CommandResult::ShowPermissionTier => KeyEventResult::ShowPermissionTier, CommandResult::SetPermissionTier(tier) => { KeyEventResult::SetPermissionTier(tier) diff --git a/crates/ui_terminal/src/slash_popup/command_list.rs b/crates/ui_terminal/src/slash_popup/command_list.rs index 6fd320ea..46a8bebb 100644 --- a/crates/ui_terminal/src/slash_popup/command_list.rs +++ b/crates/ui_terminal/src/slash_popup/command_list.rs @@ -93,6 +93,7 @@ pub(crate) fn dispatch_command(name: &str) -> PopupAction { "plan" => PopupAction::Commit(CommandResult::TogglePlan), "clear" => PopupAction::Commit(CommandResult::ClearContext), "compact" => PopupAction::Commit(CommandResult::CompactContext), + "goal" => PopupAction::Commit(CommandResult::InsertInputTemplate("/goal ".into())), other => PopupAction::Commit(CommandResult::InvalidCommand(format!( "Unknown command: /{other}" ))), @@ -262,6 +263,19 @@ mod tests { assert!(!stack.is_active()); } + #[test] + fn enter_on_goal_inserts_the_required_template() { + let mut stack = PopupStack::new(); + stack.push(Box::new(CommandListPopup::new())); + stack.set_query("go"); + let result = stack.handle_key(key(KeyCode::Enter)); + assert!(matches!( + result, + Some(CommandResult::InsertInputTemplate(ref template)) if template == "/goal " + )); + assert!(!stack.is_active()); + } + #[test] fn enter_on_model_pushes_submenu() { let mut stack = PopupStack::new();