diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ea3ffd..0917ba8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,17 @@ ## Unreleased +- The planning agent no longer re-litigates the thread's scope root on every turn. The root is chosen once, when the thread is created, and nothing in the UI moves it afterwards — so an agent that opens each reply by judging whether the directory suits the work is spending the turn on the one thing the engineer cannot act on, and has already read. The system prompt now states that the root is fixed and off the table, and a thread with any history is told that what it has already established stands and should not be restated. + +- The planning pane keeps your message on screen while the backend answers it. The turn task owns the thread and only writes it on completion, so between pressing Enter and the reply landing the text existed nowhere the pane drew: cleared from the input, not yet in the transcript, with only the thinking spinner where it had been. It read as though the pane had swallowed the message, or errored. The in-flight message is now shown as a normal `you` turn until the real one is loaded back from disk — and it is dropped, not duplicated, when the reply arrives or the draft is returned to the input after a failure. + +- The orchestrator's system prompt is built from the tools the active persona actually has. Every persona used to get the same three opening paragraphs — "use your tools to inspect sessions, start new ones, type into them, and wire pipes between them", the pause/kill explanation, and "for WAITING, ERROR and DEAD events you MUST report" — with a single corrective sentence appended at the very bottom, after the memory dump. In a read-only persona like `assistant` all of that describes tools it was never offered and events it never receives, and the model reached for them anyway. The capability sentence is now generated from the allowlist, the pause/kill and event paragraphs appear only when they apply, and the persona note sits with the description it qualifies instead of at the end. +- A persona no longer silently discards a setting you wrote yourself, and the most recent instruction wins. `Persona::apply` overwrote unconditionally, so with `persona = "orchestrator"` in the config an explicit `approval = "propose"` was replaced by the persona's `"auto"` and the gate you asked for never existed. Now: the persona named in the config leaves explicitly-set `[orchestrator]` keys alone (both come from the same file, and the spelled-out key is the more specific one) and says in the chat pane which of its settings it declined to apply; `/persona ` and the Orchestrator menu at runtime override everything, because you are asking for that persona right now. The same ordering makes a runtime Approval toggle stick until you next swap personas. +- A persona picked at runtime now survives an orchestrator restart. `start_orchestrator` re-read `[orchestrator].persona` and layered it over whatever you had chosen, so Stop/Start or "Restart To Apply Changes" silently reverted the pick — and those rows sit in the same menu as the Persona row, one keystroke away, with Restart being the action the menu itself prompts for after any other change. Switching from the menu therefore looked like it did not take, while `/persona` appeared to work only because you rarely restart right after using it. The config persona is now applied only when the user has not chosen one; "Reload Config From Disk" is still a deliberate reset back to the file. +- An unrecognized `[orchestrator] approval` value now gates instead of granting full autonomy. Only the literal `"propose"` ever turned the gate on, so `"auto_approve"`, `"approve"` or any typo silently meant "run every tool call immediately" — the failure mode you cannot see. Anything that is not `"auto"` is now treated as `"propose"`, and the bad value is reported in the chat pane at orchestrator start and by `linkshell doctor`. + +- Fixed Claude permission dialogs never registering as WAITING — so the session bar stayed on RUNNING, the chat pane never surfaced the request, and the orchestrator was never woken for it. The dialog itself matched, but Claude draws its input hint (`⏵⏵ accept edits on`) *below* the box in the same repaint, and that trailing line reads as RUNNING; with no JSONL watcher reporting for the session, RUNNING was allowed to clear the pattern-detected dialog milliseconds after it appeared. RUNNING now never releases a dialog — only positive evidence that it is gone (READY/THINKING, or a watcher report) does, which is what the watcher path already did. + - oh-my-pi sessions no longer flap between THINKING and READY every couple of seconds. omp animates its spinner in an unterminated tail rather than completing lines, and the idle timeout that reverts a stalled session to READY was fed only by complete lines — so a session that was visibly working looked idle after 2s, got flipped back to THINKING by the next spinner frame, and oscillated for the whole turn. A *changed* tail now counts as output; an unchanged one (the PTY reader re-sends it once a second) still does not, so a session whose screen has genuinely stopped moving idles out as before. - oh-my-pi sessions get a real stats watcher (`src/omp_log.rs`), reading the per-turn `usage` records omp writes to `~/.omp/agent/sessions` (honouring `OMP_HOME`) instead of scraping the screen — so tokens, cost, context, and model are the ones omp itself recorded. `/new` starts a fresh transcript file, which is why token tracking used to reset to zero mid-session; the watcher follows the roll and carries the previous transcript's totals forward. Context, being a snapshot of the live prompt rather than a total, does follow `/new` down. - Stacked output panes now have a visible edge between them. The left bar separates side-by-side panes, but vertically split panes butted one pane's last row of output directly against the next pane's title with nothing in between. Each pane's title row now doubles as its top border: a rule runs from the end of the title to the right margin. No row of output is spent on it. diff --git a/docs/config-reference.md b/docs/config-reference.md index 707b920..75e8185 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -114,6 +114,10 @@ accept `~`). is just a slow tool — its context stays coherent, which matters for local models. While a proposal is pending the orchestrator processes nothing else; incoming events coalesce into its next turn. + `"auto"` and `"propose"` are the only values. Anything else — `"approve"`, + `"auto_approve"`, a typo — is treated as `"propose"` and reported in the + chat pane and by `linkshell doctor`, so a misspelling can never quietly + hand the agent full autonomy. - `auto_approve` (default `["list_sessions", "read_output", "use_skill"]`) — tools that skip the gate. The default set is read-only, so routine observation stays fluid and only session-mutating calls (`start_session`, @@ -152,7 +156,24 @@ Local LLMs require `endpoint` and `model`; `system` and `api_key` are optional. ## `[[personas]]` Named behavioural presets layered over `[orchestrator]`. Every field is -optional; omitted fields inherit from `[orchestrator]`. Set +optional; omitted fields inherit from `[orchestrator]`. + +Precedence is "whatever you asked for most recently": + +- The persona named by `[orchestrator].persona` leaves keys you wrote + explicitly under `[orchestrator]` alone — both come from the same file, and + the key you spelled out is the more specific of the two. The chat pane says + which of its settings the persona therefore did not apply. +- `/persona ` at runtime, and the Orchestrator menu's Persona row, + apply the persona in full: you are picking it now, so it overrides the + config file and any earlier runtime tweak. +- A runtime field change (the menu's Approval row, say) holds until the next + persona swap. +- A persona chosen at runtime survives stopping, starting and restarting the + orchestrator. "Reload Config From Disk" is the deliberate way back to + `[orchestrator].persona`. + +Set `[orchestrator].persona` to pick the one applied at startup, or switch at runtime with `/persona ` (history is preserved). @@ -165,7 +186,11 @@ persona cannot turn correctness off. - `approval`, `auto_approve` — propose-mode gating. - `allowed_tools` — tools present in the schema at all. `list_sessions` is always kept. - `max_tool_iterations`, `tool_dedup_secs`, `max_context_tokens`, `event_tail_lines`. -- `note` — appended to the system prompt under a `## Persona` heading. +- `note` — added to the system prompt beside the capability description it + qualifies. The rest of that description is generated from `allowed_tools`: + a persona without `send_input` is not told it can type into sessions, and + one with `events = []` is not given the "you must report every WAITING + event" rule. Builtins: `assistant` (reactive, read-only, propose), `monitor` (watches and reports, writes gated), `orchestrator` (acts autonomously, tightest dedup diff --git a/src/app.rs b/src/app.rs index 8bb42f6..786714c 100644 --- a/src/app.rs +++ b/src/app.rs @@ -310,6 +310,13 @@ pub struct PlanningState { pub backend: Option, /// True while a turn or commit is in flight; the input is not sent twice. pub busy: bool, + /// The message sent by the turn currently in flight. The background task + /// owns the thread and only writes it on completion, so without this the + /// text vanishes the instant you press Enter — cleared from the input, + /// not yet in the transcript — and the pane looks like it swallowed the + /// message. Shown in the transcript as a pending turn until the real one + /// is loaded back from disk. + pub pending_user: Option, /// Latest progress line from the running turn. pub status: String, /// Last error, kept visible until the next successful turn. @@ -646,6 +653,12 @@ pub struct App { pub orchestrator_status: Option<(String, std::time::Instant)>, /// Name of the active persona (empty = bare [orchestrator] config). pub orchestrator_persona: String, + /// The active persona was chosen at runtime (`/persona`, the menu) rather + /// than read from `[orchestrator].persona`. A restart must not undo that + /// choice: Stop/Start and "Restart To Apply Changes" sit in the same menu + /// as the Persona row, so re-reading the config persona on start silently + /// threw the user's pick away seconds after they made it. + pub persona_user_selected: bool, /// Pristine [orchestrator] config. Personas layer over *this*, never over /// an already-layered config, so swapping A -> B -> A is idempotent. pub orchestrator_base: Option, @@ -769,6 +782,7 @@ impl App { orchestrator_ctx_max: None, orchestrator_status: None, orchestrator_persona: String::new(), + persona_user_selected: false, orchestrator_base: None, orch_event_cooldowns: HashMap::new(), last_permission_request: None, @@ -2583,18 +2597,40 @@ impl App { b } }; - let want = base.persona.clone(); + // A persona picked at runtime survives stop/start and restart — the + // layered config it produced is already in self.config, so there is + // nothing to re-apply. Only fall back to the config's persona when + // the user has not chosen one themselves. + let want = if self.persona_user_selected { + String::new() + } else { + base.persona.clone() + }; if !want.is_empty() && self.orchestrator_persona != want { if let Some(p) = self.personas().into_iter().find(|p| p.name == want) { + let kept = p.overridden_by_config(&base); let mut cfg = (*self.config).clone(); - cfg.orchestrator = p.apply(&base); + cfg.orchestrator = p.apply_at_startup(&base); self.config = Arc::new(cfg); self.orchestrator_persona = want; + if !kept.is_empty() { + self.chat_system(format!( + "persona {} did not set {} — your [orchestrator] {} wins. \ + /persona {} at runtime applies the persona in full.", + p.name, + kept.join(", "), + if kept.len() == 1 { "value" } else { "values" }, + p.name, + )); + } } else { self.chat_system(format!("unknown persona \"{}\" in config; ignoring", want)); } } let cfg = self.config.orchestrator.clone(); + if let Some(warning) = cfg.approval_warning() { + self.chat_system(warning); + } match cfg.class()? { crate::config::OrchestratorClass::Api(_) => { if self.config.agents.contains_key(&cfg.name) { @@ -4203,7 +4239,11 @@ impl App { b } }; + // Asked for by hand, now: the persona wins outright over the config + // file and over any earlier runtime tweak. Whatever the user did last + // is what they meant — including across a restart. let cfg = persona.apply(&base); + self.persona_user_selected = true; let Some(h) = &self.orchestrator else { // No API orchestrator running: still record it so a later // /orchestrator start picks the persona up. @@ -5034,6 +5074,13 @@ impl App { MenuAction::ReloadConfig => { self.config = std::sync::Arc::new(crate::config::load()); self.orchestrator_config_dirty = false; + // Reloading is a deliberate "go back to the file": drop the + // remembered base and the runtime persona pick along with it, + // or the next start would layer the new config under stale + // values from the old one. + self.orchestrator_base = None; + self.persona_user_selected = false; + self.orchestrator_persona.clear(); self.command_result = "config reloaded from disk".into(); MenuOutcome::Stay } @@ -6658,6 +6705,7 @@ impl App { self.planning.thread = Some(thread); self.planning.input.clear(); self.planning.cursor = 0; + self.planning.pending_user = None; self.planning.scroll = 0; self.planning.error.clear(); self.planning.overflow = false; @@ -6674,6 +6722,8 @@ impl App { pub fn planning_open_thread(&mut self, id: &str) -> Result<(), String> { let thread = crate::planning::store::load(id).map_err(|e| e.to_string())?; self.planning.thread = Some(thread); + // A pending turn belongs to the thread it was typed into. + self.planning.pending_user = None; self.planning.scroll = 0; self.planning.error.clear(); self.planning.overflow = false; @@ -6727,6 +6777,9 @@ impl App { self.planning.input.clear(); self.planning.cursor = 0; self.planning.busy = true; + // Keep the message on screen while it is in flight; the transcript + // only gains the real turn when the reply lands. + self.planning.pending_user = Some(text.clone()); self.planning.status = "sending".to_string(); self.planning.error.clear(); self.planning.overflow = false; @@ -6795,6 +6848,9 @@ impl App { return; } self.planning.busy = false; + // The turn is on disk now; the reload below brings back the real + // message, so the placeholder has done its job. + self.planning.pending_user = None; self.planning.last_peak_tokens = peak_tokens; self.planning.status.clear(); // The background task owns its own copy of the thread, so reload from @@ -6831,6 +6887,9 @@ impl App { } self.planning.busy = false; self.planning.status.clear(); + // The draft comes back into the input below, so showing it as a + // pending turn as well would double it. + self.planning.pending_user = None; self.planning.error = error; self.planning.overflow = overflow; // Restore the unsent message so a switch-and-retry costs nothing. @@ -6852,6 +6911,7 @@ impl App { return; } self.planning.busy = false; + self.planning.pending_user = None; self.planning.status = if stale.is_empty() { format!("plan revision {} written to {}", revision, path) } else { @@ -8073,6 +8133,95 @@ mod tests { ); } + #[tokio::test] + async fn an_in_flight_planning_message_stays_on_screen() { + // The turn task owns the thread and only writes it on completion, so + // between Enter and the reply the text lives nowhere else: cleared + // from the input, not yet in the transcript. It used to vanish under + // the thinking spinner, which reads as the pane having eaten it. + let mut cfg = crate::config::Config::default(); + cfg.planning.backends.insert( + "fake".into(), + crate::planning::Backend { + name: "fake".into(), + provider: "lmstudio".into(), + endpoint: "http://127.0.0.1:1/v1".into(), + model: "fake".into(), + ..Default::default() + }, + ); + let mut app = make_app_with_config(cfg); + let thread = crate::planning::store::Thread::new("t", std::path::PathBuf::from("/tmp")); + let id = thread.id.clone(); + app.planning.thread = Some(thread); + app.planning.backend = app.config.planning.default_backend(); + app.planning.input = "how should I structure retries".into(); + + app.planning_send(); + assert!(app.planning.input.is_empty(), "the draft left the input"); + assert_eq!( + app.planning.pending_user.as_deref(), + Some("how should I structure retries"), + "so it must be visible in the transcript instead" + ); + + // On failure the draft returns to the input; showing it as a pending + // turn as well would double it. + app.handle_planning_failed( + id.clone(), + "how should I structure retries".into(), + "boom".into(), + false, + ); + assert!(app.planning.pending_user.is_none()); + assert_eq!(app.planning.input, "how should I structure retries"); + + // On success the reply handler reloads the thread, which carries the + // real user turn — the placeholder must go with it. + app.planning.input = "second try".into(); + app.planning_send(); + assert!(app.planning.pending_user.is_some()); + app.handle_planning_reply(id, "ok".into(), "fake".into(), "fake".into(), 10, None); + assert!(app.planning.pending_user.is_none()); + } + + #[tokio::test] + async fn a_persona_picked_at_runtime_survives_an_orchestrator_restart() { + let mut cfg = crate::config::Config::default(); + cfg.orchestrator.enabled = true; + // Provider whose start path needs no network or child process. + cfg.orchestrator.provider = "lmstudio".into(); + cfg.orchestrator.persona = "orchestrator".into(); + let mut app = make_app_with_config(cfg); + + app.start_orchestrator().unwrap(); + assert_eq!(app.orchestrator_persona, "orchestrator"); + assert_eq!(app.config.orchestrator.approval, "auto"); + + // The user picks a different persona from the menu / /persona. + let (si, ii, _) = find_item(&app, "Orchestrator", "Persona"); + app.open_menu(); + app.activate_menu_index(si, ii); + let picked = app.orchestrator_persona.clone(); + assert_ne!(picked, "orchestrator", "the cycle moved off the config one"); + + // Stop/Start — both a keystroke away in the same menu — used to + // silently re-apply [orchestrator].persona over the user's choice. + app.stop_orchestrator(); + app.start_orchestrator().unwrap(); + assert_eq!(app.orchestrator_persona, picked); + assert_eq!( + app.config.orchestrator.approval, + crate::config::builtin_personas() + .iter() + .find(|p| p.name == picked) + .unwrap() + .approval + .clone() + .unwrap() + ); + } + #[test] fn approval_toggles_between_auto_and_propose() { let mut app = make_app(); diff --git a/src/config.rs b/src/config.rs index 4aab3c7..3a5d9ab 100644 --- a/src/config.rs +++ b/src/config.rs @@ -144,6 +144,12 @@ pub struct OrchestratorConfig { pub providers: Vec, /// Context budgets offered by the menu. Empty = a built-in ladder. pub context_choices: Vec, + /// Keys written explicitly under `[orchestrator]` in the config file. + /// A persona only fills in keys the user left unset — writing + /// `approval = "propose"` there must survive a persona swap, or the + /// setting silently does nothing. Populated by `parse`, not by serde. + #[serde(skip)] + pub explicit_keys: std::collections::HashSet, } impl Default for OrchestratorConfig { @@ -192,6 +198,7 @@ impl Default for OrchestratorConfig { models: Vec::new(), providers: Vec::new(), context_choices: Vec::new(), + explicit_keys: std::collections::HashSet::new(), } } } @@ -367,8 +374,30 @@ impl OrchestratorConfig { out } + /// Whether the propose gate is on. Only the literal "auto" turns it off: + /// an unrecognized value ("auto_approve", "approve", a typo) gates rather + /// than silently granting full autonomy, because that is the direction + /// that cannot hurt anyone. `approval_warning` reports the misspelling. + pub fn approval_gates(&self) -> bool { + self.approval.trim() != "auto" + } + + /// Complaint about an `approval` value that is neither "auto" nor + /// "propose", for the chat pane and `linkshell doctor`. + pub fn approval_warning(&self) -> Option { + let value = self.approval.trim(); + if value == "auto" || value == "propose" { + return None; + } + Some(format!( + "[orchestrator] approval = \"{}\" is not a valid value (use \"auto\" or \ + \"propose\"); treating it as \"propose\" so tool calls are gated", + value + )) + } + pub fn approval_required(&self, tool: &str) -> bool { - if self.approval != "propose" { + if !self.approval_gates() { return false; } // kill_session has its own confirmation flow (/confirm-kill). @@ -437,10 +466,16 @@ impl OrchestratorConfig { /// explaining the contract. Idempotent and best-effort; called when an /// orchestrator starts. pub fn ensure_agent_files(&self) { - if self.skills_dir.is_empty() { - if let Some(dir) = config_path().and_then(|p| p.parent().map(|d| d.join("skills"))) { - let _ = std::fs::create_dir_all(dir); - } + // Skills: create the directory and drop in the shipped defaults. + // install_defaults never overwrites an existing file, so a default + // the user has edited stays edited. + let skills_dir = if self.skills_dir.is_empty() { + config_path().and_then(|p| p.parent().map(|d| d.join("skills"))) + } else { + Some(std::path::PathBuf::from(expand_tilde(&self.skills_dir))) + }; + if let Some(dir) = skills_dir { + crate::orchestrator::install_default_skills(&dir); } if let Some(path) = self.memory_path() { if !path.exists() { @@ -606,40 +641,98 @@ pub struct Persona { } impl Persona { - /// Layer this persona over a base orchestrator config. + /// Layer this persona over `base`, overriding every field it sets. + /// + /// This is the runtime path (`/persona `, the Orchestrator menu): + /// the user picked this persona just now, so it outranks anything in the + /// config file and any earlier runtime tweak. `apply_at_startup` is the + /// quieter version used when the persona comes from the config itself. pub fn apply(&self, base: &OrchestratorConfig) -> OrchestratorConfig { + self.layer(base, false) + } + + /// Layer this persona over `base` without touching keys the user wrote + /// explicitly under `[orchestrator]`. Used only for the persona named in + /// the config: there both settings come from the same file, and the more + /// specific one — the key the user spelled out — is what they meant. + pub fn apply_at_startup(&self, base: &OrchestratorConfig) -> OrchestratorConfig { + self.layer(base, true) + } + + fn layer(&self, base: &OrchestratorConfig, defer_to_config: bool) -> OrchestratorConfig { let mut cfg = base.clone(); + let mine = |key: &str| !defer_to_config || !base.explicit_keys.contains(key); if let Some(v) = &self.events { - cfg.events = v.clone(); + if mine("events") { + cfg.events = v.clone(); + } } if let Some(v) = self.event_cooldown_secs { - cfg.event_cooldown_secs = v; + if mine("event_cooldown_secs") { + cfg.event_cooldown_secs = v; + } } if let Some(v) = &self.approval { - cfg.approval = v.clone(); + if mine("approval") { + cfg.approval = v.clone(); + } } if let Some(v) = &self.auto_approve { - cfg.auto_approve = v.clone(); + if mine("auto_approve") { + cfg.auto_approve = v.clone(); + } } if let Some(v) = &self.allowed_tools { - cfg.allowed_tools = v.clone(); + if mine("allowed_tools") { + cfg.allowed_tools = v.clone(); + } } if let Some(v) = self.max_tool_iterations { - cfg.max_tool_iterations = v; + if mine("max_tool_iterations") { + cfg.max_tool_iterations = v; + } } if let Some(v) = self.tool_dedup_secs { - cfg.tool_dedup_secs = v; + if mine("tool_dedup_secs") { + cfg.tool_dedup_secs = v; + } } if let Some(v) = self.max_context_tokens { - cfg.max_context_tokens = v; + if mine("max_context_tokens") { + cfg.max_context_tokens = v; + } } if let Some(v) = self.event_tail_lines { - cfg.event_tail_lines = v; + if mine("event_tail_lines") { + cfg.event_tail_lines = v; + } } cfg.persona_note = self.note.clone(); cfg.persona = self.name.clone(); cfg } + + /// Keys `apply_at_startup` would leave alone because the user set them + /// explicitly under `[orchestrator]`. Surfaced at startup so "the persona + /// did nothing" is never a silent outcome. A runtime swap uses `apply` + /// and overrides everything, so it has nothing to report. + pub fn overridden_by_config(&self, base: &OrchestratorConfig) -> Vec<&'static str> { + [ + ("events", self.events.is_some()), + ("event_cooldown_secs", self.event_cooldown_secs.is_some()), + ("approval", self.approval.is_some()), + ("auto_approve", self.auto_approve.is_some()), + ("allowed_tools", self.allowed_tools.is_some()), + ("max_tool_iterations", self.max_tool_iterations.is_some()), + ("tool_dedup_secs", self.tool_dedup_secs.is_some()), + ("max_context_tokens", self.max_context_tokens.is_some()), + ("event_tail_lines", self.event_tail_lines.is_some()), + ] + .into_iter() + .filter(|(key, wanted)| *wanted && base.explicit_keys.contains(*key)) + .map(|(key, _)| key) + .collect() + } } /// The three shipped personas, used when no `[[personas]]` entry matches. @@ -1276,11 +1369,27 @@ pub fn save_profile(profile: &Profile) -> anyhow::Result { pub fn parse(content: &str) -> anyhow::Result { let mut cfg: Config = toml::from_str(content)?; + cfg.orchestrator.explicit_keys = orchestrator_keys(content); validate_profiles(&cfg)?; cfg.derive_planning_backends(); Ok(cfg) } +/// Which keys the user actually wrote under `[orchestrator]`. Serde cannot +/// tell "absent" from "set to the default value", and a persona needs that +/// distinction to know what it may fill in. +fn orchestrator_keys(content: &str) -> std::collections::HashSet { + content + .parse::() + .ok() + .and_then(|v| { + v.get("orchestrator") + .and_then(|o| o.as_table()) + .map(|t| t.keys().cloned().collect()) + }) + .unwrap_or_default() +} + impl Config { /// Offer the model endpoints already configured elsewhere as planning /// backends, so the picker is usable without a second copy of the same diff --git a/src/doctor.rs b/src/doctor.rs index a6bb929..6332b93 100644 --- a/src/doctor.rs +++ b/src/doctor.rs @@ -192,11 +192,16 @@ fn run_with(context: &Context, out: &mut dyn Write) -> i32 { match context.config.as_deref() { Some(path) if path.exists() => match crate::config::load_strict(path) { - Ok(_) => report( - "config validity", - Level::Ok, - "TOML and profile references are valid".into(), - ), + // A misspelled `approval` parses fine but silently changes how + // much the orchestrator may do unasked, so it is worth a line. + Ok(cfg) => match cfg.orchestrator.approval_warning() { + Some(warning) => report("config validity", Level::Warn, warning), + None => report( + "config validity", + Level::Ok, + "TOML and profile references are valid".into(), + ), + }, Err(error) => report( "config validity", Level::Fail, diff --git a/src/orchestrator/mod.rs b/src/orchestrator/mod.rs index 61e28ca..896d7bc 100644 --- a/src/orchestrator/mod.rs +++ b/src/orchestrator/mod.rs @@ -13,6 +13,8 @@ mod anthropic; mod openai; mod skills; +pub use skills::install_defaults as install_default_skills; + use crate::config::{ApiProvider, OrchestratorClass, OrchestratorConfig}; use crate::events::{AppEvent, OrchestratorReq}; use tokio::sync::mpsc; @@ -787,6 +789,15 @@ async fn exec_tool( // ── Prompts ──────────────────────────────────────────────────────────────── fn system_prompt(cfg: &OrchestratorConfig) -> String { + // Describe the job the persona actually has. A read-only persona used to + // be told, in the opening paragraphs, to type into sessions, start them + // and wire pipes — with one corrective sentence buried at the very + // bottom — so it kept reaching for tools it was not given. + let tools: std::collections::HashSet<&str> = + allowed_specs(cfg).into_iter().map(|(n, _, _)| n).collect(); + let can = |t: &str| tools.contains(t); + let drives = can("send_input") || can("start_session") || can("pipe_add"); + let mut p = String::from( "You are the resident orchestrator agent inside linkshell, a terminal \ multiplexer for AI coding sessions (claude, codex, opencode, oh-my-pi, aider, shells). \ @@ -794,27 +805,80 @@ You watch over all sessions on the user's behalf.\n\ \n\ Session states: STARTING, READY (idle, will accept input), THINKING/RUNNING (busy), \ WAITING (blocked on user input — the waiting_prompt field says what it asked), \ -ERROR, DEAD. Use your tools to inspect sessions, start new ones in any directory, \ -type into them, and wire pipes between them. Tool session_id arguments take the raw \ +ERROR, DEAD. ", + ); + if drives { + let mut verbs = vec!["inspect sessions"]; + if can("start_session") { + verbs.push("start new ones in any directory"); + } + if can("send_input") { + verbs.push("type into them"); + } + if can("pipe_add") { + verbs.push("wire pipes between them"); + } + p.push_str(&format!("Use your tools to {}. ", join_and(&verbs))); + } else { + p.push_str( + "You observe and advise: you can read sessions and report to the user, but \ +you have no tools to type into them, start them, or change them in any way. When \ +something needs doing, say what you would do and let the user do it — do not claim to \ +have done it, and do not describe a tool call as if it had run. ", + ); + } + p.push_str( + "Tool session_id arguments take the raw \ `id` from list_sessions; the user sees 1-based `display` numbers, so when talking to \ -the user, call sessions by their display number or name.\n\ -\n\ -You cannot kill sessions. request_kill only files a request the user must approve \ -with /confirm-kill. You can however pause_session/resume_session: pausing stops a \ -session's process (SIGSTOP) without losing its context — its state shows PAUSED — \ -which is the right lever when concurrent sessions contend for limited CPU or RAM.\n\ -\n\ -Messages starting with [linkshell event] are automatic notifications that a session \ -changed state. For WAITING, ERROR, and DEAD events you MUST report: investigate \ +the user, call sessions by their display number or name.\n", + ); + if can("pause_session") || can("request_kill") { + p.push_str( + "\nYou cannot kill sessions. request_kill only files a request the user must \ +approve with /confirm-kill. You can however pause_session/resume_session: pausing stops \ +a session's process (SIGSTOP) without losing its context — its state shows PAUSED — \ +which is the right lever when concurrent sessions contend for limited CPU or RAM.\n", + ); + } + if !cfg.events.is_empty() { + p.push_str( + "\nMessages starting with [linkshell event] are automatic notifications that a \ +session changed state. For WAITING, ERROR, and DEAD events you MUST report: investigate \ briefly (read_output) and tell the user in one or two sentences what happened, what \ it needs, and what you suggest — never answer these with just `ok`. Only when ALL \ events in the message are informational (READY/STARTING/THINKING/RUNNING) and nothing \ depends on them: make no tool calls and reply with exactly `ok` — that reply is \ -suppressed and never shown to the user. \ -Messages starting \ -with [linkshell] are system notes.\n\ +suppressed and never shown to the user.\n", + ); + } + p.push_str( + "\nMessages starting with [linkshell] are system notes.\n\ \n\ Your replies render in a small chat pane: be concise, no markdown headers.", + ); + // The persona's own words go here, next to the capability description + // they qualify, rather than after the memory dump at the end. + if !cfg.persona_note.trim().is_empty() { + p.push_str("\n\nYour persona is \""); + p.push_str(&cfg.persona); + p.push_str("\": "); + p.push_str(cfg.persona_note.trim()); + p.push('\n'); + } + p.push_str( + "\n\nStanding rules, above any task:\n\ +- Installing, upgrading, or removing software is the user's call, never yours. \ +If a session prompts to install something, report what it wants and wait for \ +explicit approval — do not answer the prompt, and do not run the install \ +yourself.\n\ +- If you cannot confirm that input you sent to a session was picked up, say so \ +in one line — which session, what you sent — and move on with the rest of the \ +work. Do not silently resend, and do not stall on it.\n\ +- The same holds for anything destructive and hard to undo: rm -rf, git reset \ +--hard, force pushes, dropping or resetting a branch, truncating or dropping \ +data. Describe what you would run and wait to be told to run it.\n\ +- Report uncertainty as uncertainty. A guess presented as an observation is \ +worse than saying you do not know.", ); if let Some(list) = skills_section(cfg, false) { p.push_str( @@ -823,7 +887,7 @@ matches a skill's description, load it and follow it:\n", ); p.push_str(&list); } - if cfg.approval == "propose" { + if cfg.approval_gates() { p.push_str( "\nSome of your tool calls require the user's approval before they run; \ they may take a while to return while the user decides. A tool result of \ @@ -843,16 +907,18 @@ and continue; otherwise report what you wanted to do and why.\n", if let Some(memory) = memory_section(cfg) { p.push_str(&memory); } - // Persona note last of the static text (before memory), so a persona - // swap invalidates as little of the cached prefix as possible. - if !cfg.persona_note.trim().is_empty() { - p.push_str("\n\n## Persona\n\n"); - p.push_str(cfg.persona_note.trim()); - p.push('\n'); - } p } +/// "a, b and c" — for listing capabilities in prose. +fn join_and(parts: &[&str]) -> String { + match parts { + [] => String::new(), + [one] => one.to_string(), + [rest @ .., last] => format!("{} and {}", rest.join(", "), last), + } +} + /// Briefing typed into a CLI-class orchestrator session once it is READY. pub fn cli_briefing(cfg: &OrchestratorConfig) -> String { cfg.ensure_agent_files(); @@ -875,6 +941,16 @@ Keep chat messages short.\n\ Lines arriving that start with [linkshell event] mean a session changed state \ (WAITING/ERROR/DEAD): investigate with `list`/`read`, then summarize for the user via \ `linkshell-ctl chat`. Do not modify files unless the user asks; your role is coordination.", + ); + p.push_str( + "\nStanding rules, above any task: installing, upgrading, or removing \ +software is the user's call — if a session prompts to install something, report it \ +via `linkshell-ctl chat` and wait for explicit approval rather than answering the \ +prompt or running the install yourself. Anything destructive and hard to undo — \ +rm -rf, git reset --hard, force pushes, dropping a branch, discarding data — needs \ +the same explicit approval: describe the command, then wait. If you cannot confirm that input you sent \ +to a session was picked up, say so in one line and move on; never silently resend. \ +Report uncertainty as uncertainty.\n", ); if let Some(list) = skills_section(cfg, true) { p.push_str( @@ -1294,6 +1370,16 @@ mod tests { assert!(!cfg.approval_required("kill_session")); cfg.approval = "auto".to_string(); assert!(!cfg.approval_required("send_input")); + assert!(cfg.approval_warning().is_none()); + + // Only "auto" opens the gate. A misspelling used to mean full + // autonomy, silently — now it gates and says so. + for typo in ["auto_approve", "approve", "ask", "yes"] { + cfg.approval = typo.to_string(); + assert!(cfg.approval_required("send_input"), "{typo}"); + let warning = cfg.approval_warning().unwrap_or_default(); + assert!(warning.contains(typo), "{warning}"); + } } #[tokio::test] @@ -1451,6 +1537,64 @@ mod tests { assert!(orch.apply(&base).tool_dedup_secs > 0); } + #[test] + fn a_persona_does_not_clobber_a_setting_the_user_wrote_explicitly() { + let cfg = crate::config::parse( + "[orchestrator]\nprovider = \"lmstudio\"\napproval = \"propose\"\n", + ) + .unwrap() + .orchestrator; + let personas = crate::config::builtin_personas(); + let orch = personas.iter().find(|p| p.name == "orchestrator").unwrap(); + + // Startup: both settings come from the same file, and the explicit + // key is the more specific one. The persona wants approval = "auto"; + // the config said "propose", which is the point of writing it down. + let layered = orch.apply_at_startup(&cfg); + assert_eq!(layered.approval, "propose"); + assert!(layered.approval_required("send_input")); + // Not silent about what it declined to set. + assert_eq!(orch.overridden_by_config(&cfg), vec!["approval"]); + // Fields the user left alone still come from the persona. + assert_eq!(layered.event_cooldown_secs, 15); + + // Runtime (/persona orchestrator): the user is asking for this + // persona now, so it outranks the config file. + let layered = orch.apply(&cfg); + assert_eq!(layered.approval, "auto"); + assert!(!layered.approval_required("send_input")); + } + + #[test] + fn a_read_only_persona_is_not_told_it_can_drive_sessions() { + let personas = crate::config::builtin_personas(); + let assistant = personas.iter().find(|p| p.name == "assistant").unwrap(); + let cfg = assistant.apply(&OrchestratorConfig::default()); + let prompt = system_prompt(&cfg); + + // The capability paragraph used to promise tools the persona lacks. + assert!(!prompt.contains("Use your tools to"), "{prompt}"); + assert!(!prompt.contains("start new ones"), "{prompt}"); + assert!(!prompt.contains("wire pipes between them"), "{prompt}"); + assert!(!prompt.contains("pause_session"), "{prompt}"); + assert!(prompt.contains("You observe and advise"), "{prompt}"); + // No events reach this persona, so the "you MUST report" rule is off. + assert!(!prompt.contains("[linkshell event]"), "{prompt}"); + // The persona's own words sit with the capabilities they qualify, + // not after the memory block at the very end. + let note = prompt.find("reactive assistant").expect("persona note"); + let chat_pane = prompt.find("small chat pane").expect("chat pane line"); + assert!(note > chat_pane); + assert!(note < prompt.find("Standing rules").expect("standing rules")); + + // The autonomous persona still gets the full description. + let orch = personas.iter().find(|p| p.name == "orchestrator").unwrap(); + let prompt = system_prompt(&orch.apply(&OrchestratorConfig::default())); + assert!(prompt.contains("type into them"), "{prompt}"); + assert!(prompt.contains("wire pipes between them"), "{prompt}"); + assert!(prompt.contains("[linkshell event]"), "{prompt}"); + } + #[test] fn repeat_calls_are_suppressed_within_the_window() { let mut log = CallLog::new(60); diff --git a/src/orchestrator/skills.rs b/src/orchestrator/skills.rs index 221bbc8..402983b 100644 --- a/src/orchestrator/skills.rs +++ b/src/orchestrator/skills.rs @@ -13,6 +13,103 @@ pub struct Skill { pub path: PathBuf, } +/// Skills shipped with linkshell, installed into the skills directory the +/// first time an orchestrator starts. Each is written only when a file of +/// that name is absent, so a user's edits are never clobbered. A deleted +/// default is restored on next start; to drop one for good, empty the file +/// or override skills_dir. +pub const DEFAULT_SKILLS: &[(&str, &str)] = &[ + ( + "install-approval", + "---\n\ +description: A session wants to install software or run a destructive command\n\ +---\n\ +\n\ +# Install approval\n\ +\n\ +A session has stopped on a prompt that would change what is installed on this\n\ +machine or in the project: a package manager confirmation (apt, dnf, brew,\n\ +pip, cargo, npm, go get), a toolchain download, a container pull, or an\n\ +agent asking permission to add a dependency.\n\ +\n\ +You do not approve these. The user does.\n\ +\n\ +1. `read_output` on the session to capture the exact prompt and the exact\n\ + command or package list it is about to act on.\n\ +2. Tell the user in one or two lines: which session (display number), what it\n\ + wants to install, and what it will do to the system if allowed.\n\ +3. Stop. Do not type `y`, `yes`, `1`, Enter, or anything else into that\n\ + session, and do not run the install yourself in a shell session.\n\ +4. Wait for the user to say so explicitly. \"Go ahead\", \"approve it\", or a\n\ + direct instruction naming the session counts; silence and general\n\ + encouragement do not.\n\ +5. When approved, send exactly the response the prompt expects, then confirm\n\ + what happened.\n\ +\n\ +If several sessions are blocked on installs at once, list them all and let the\n\ +user decide in one pass rather than asking repeatedly.\n\ +\n\ +Uninstalls, version downgrades, and anything touching a system-wide path get\n\ +the same treatment. Reading a lockfile or asking what version is installed\n\ +does not — that is inspection, not change.\n\ +\n\ +## Destructive commands\n\ +\n\ +Same rule, same procedure, for anything hard to undo: `rm -rf`, `git reset\n\ +--hard`, `git clean -fdx`, force pushes, deleting or resetting a branch,\n\ +dropping a table, truncating a file, overwriting a config the user maintains.\n\ +\n\ +Quote the exact command and name what it destroys — not \"cleaning up the\n\ +build directory\" but the path it will delete and whether anything there is\n\ +untracked. The user can only approve what they can see. If a session is\n\ +already blocked on such a command, do not confirm it for them; if you were\n\ +about to run one yourself in a shell session, describe it instead and wait.\n", + ), + ( + "uncertain-input", + "---\n\ +description: You cannot tell whether text you sent to a session was received\n\ +---\n\ +\n\ +# Uncertain input\n\ +\n\ +You typed into a session and cannot confirm it landed: the output is\n\ +unchanged, the state never moved off READY, the pane is showing a pager or a\n\ +full-screen editor, or the session was mid-render when you wrote.\n\ +\n\ +Do not guess, and do not retype. A blind resend can double-execute a command,\n\ +answer a prompt you never saw, or dump text into an editor buffer.\n\ +\n\ +1. `read_output` once more on that session — a slow session often just needed\n\ + a moment.\n\ +2. If the output shows your text, carry on normally.\n\ +3. If it does not, say so plainly: which session, what you tried to send, and\n\ + that you are not sure it was received. One or two lines.\n\ +4. Move on to the rest of the user's request. An unconfirmed input is not a\n\ + reason to stall everything else, and it is not a reason to keep polling the\n\ + same session in a loop.\n\ +5. Let the user decide whether to resend. If they ask you to, send it once.\n\ +\n\ +The same applies when a session's state is ambiguous — READY but with a\n\ +half-drawn prompt, or output that stopped mid-line. Report the ambiguity\n\ +instead of resolving it by assumption.\n", + ), +]; + +/// Write the shipped defaults into `dir`, skipping any that already exist. +/// Best-effort and idempotent. +pub fn install_defaults(dir: &Path) { + if std::fs::create_dir_all(dir).is_err() { + return; + } + for (name, body) in DEFAULT_SKILLS { + let path = dir.join(format!("{name}.md")); + if !path.exists() { + let _ = std::fs::write(&path, body); + } + } +} + /// Load skill metadata (not the bodies) from `dir`, sorted by name. pub fn load_skills(dir: &Path) -> Vec { let mut skills: Vec = std::fs::read_dir(dir) @@ -141,4 +238,26 @@ mod tests { assert_eq!(skill_list(&skills, false), "- deploy: Deploy checklist"); assert!(skill_list(&skills, true).contains("(/tmp/deploy.md)")); } + + #[test] + fn install_defaults_seeds_without_clobbering_user_edits() { + let dir = temp_skills_dir(&[]); + install_defaults(&dir); + let loaded = load_skills(&dir); + assert_eq!(loaded.len(), DEFAULT_SKILLS.len()); + // Descriptions come from the frontmatter, so they show up in prompts. + assert!(loaded.iter().all(|s| !s.description.is_empty())); + + // A user edit survives a second install. + let edited = dir.join("install-approval.md"); + std::fs::write(&edited, "---\ndescription: mine\n---\nbody").unwrap(); + std::fs::remove_file(dir.join("uncertain-input.md")).unwrap(); + install_defaults(&dir); + // A deleted default is restored. + assert_eq!( + std::fs::read_to_string(&edited).unwrap(), + "---\ndescription: mine\n---\nbody" + ); + assert!(dir.join("uncertain-input.md").exists()); + } } diff --git a/src/planning/mod.rs b/src/planning/mod.rs index 594bfb1..f3c65ae 100644 --- a/src/planning/mod.rs +++ b/src/planning/mod.rs @@ -265,6 +265,26 @@ fn system_prompt(thread: &Thread) -> String { "\n\nScope root (all paths resolve against it): {}", thread.root.display() )); + // The root is chosen once, when the thread is created, and nothing in the + // UI can move it afterwards. Without this the model re-opens the question + // every turn — "this isn't the right directory for that" — which the + // engineer cannot act on and has already read. + p.push_str( + "\nThe root was fixed when this thread was created and cannot be changed, by you \ +or by the engineer. Do not assess whether it is the right directory and do not suggest \ +moving or re-rooting the work. If something you need falls outside it, say once, in a \ +line, what you cannot see, then plan with what is in front of you.", + ); + // `thread` does not yet include the message being sent, so an empty + // transcript really does mean this is the opening turn. + if !thread.messages.is_empty() { + p.push_str( + "\n\nThis is a continuing conversation. Everything you have already established — \ +scope, caveats, what you can and cannot see, how you read the codebase — stands, and the \ +engineer has read it. Do not restate it. Pick up from the transcript and answer what was \ +just asked.", + ); + } if !thread.reads.is_empty() { let stale = thread.stale_reads(); if !stale.is_empty() { @@ -837,6 +857,8 @@ mod tests { fn system_prompt_pins_the_root_and_flags_stale_grounding() { let mut t = Thread::new("t", PathBuf::from("/tmp/repo")); assert!(system_prompt(&t).contains("/tmp/repo")); + // The root cannot be moved, so re-litigating it wastes the turn. + assert!(system_prompt(&t).contains("cannot be changed")); t.record_read(tools::ReadRecord { rel: "vanished.rs".to_string(), hash: 7, @@ -845,4 +867,20 @@ mod tests { let p = system_prompt(&t); assert!(p.contains("vanished.rs"), "stale reads are surfaced: {}", p); } + + #[test] + fn a_continuing_thread_is_told_not_to_restate_what_it_already_said() { + // The opening turn has nothing to repeat yet. + let mut t = Thread::new("t", PathBuf::from("/tmp/repo")); + assert!(!system_prompt(&t).contains("continuing conversation")); + + // `thread` never carries the message being sent, so one prior + // exchange is what makes the next turn a continuation. + t.messages.push(Message::user("how should retries work")); + t.messages.push(Message::assistant( + "Back off exponentially.", + &Backend::default(), + )); + assert!(system_prompt(&t).contains("continuing conversation")); + } } diff --git a/src/session.rs b/src/session.rs index aa8945e..4d230e6 100644 --- a/src/session.rs +++ b/src/session.rs @@ -773,6 +773,12 @@ impl Session { self.pattern_waiting = false; self.state = self.watcher_state.clone().unwrap_or(new_state); } + // Same reasoning as above, for the no-watcher case: a Claude + // dialog is drawn with the input hint ("⏵⏵ accept edits on") + // rendered *below* it in the same frame, and that trailing line + // reads as RUNNING. Letting it through wiped WAITING milliseconds + // after the dialog appeared. + SessionState::Running if self.pattern_waiting => {} _ if !self.ipc_state => { self.pattern_waiting = false; self.state = new_state; @@ -1411,11 +1417,19 @@ mod tests { fn pattern_states_apply_freely_without_a_watcher() { let mut s = session(SessionKind::Aider); + s.apply_pattern_state(SessionState::Running); + assert_eq!(s.state, SessionState::Running); + s.apply_pattern_state(SessionState::Waiting); assert_eq!(s.state, SessionState::Waiting); - // No watcher state to fall back on: a complete RUNNING line clears it. + // A trailing RUNNING line from the same repaint (a Claude dialog is + // drawn with the input hint below it) must not wipe the dialog. s.apply_pattern_state(SessionState::Running); - assert_eq!(s.state, SessionState::Running); + assert_eq!(s.state, SessionState::Waiting); + // Ready is positive evidence the dialog is gone, and releases it. + s.apply_pattern_state(SessionState::Ready); + assert_eq!(s.state, SessionState::Ready); + assert!(!s.pattern_waiting); } #[test] diff --git a/src/ui.rs b/src/ui.rs index 39a08ca..83659ab 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -3443,6 +3443,20 @@ fn draw_planning_transcript( lines.push(Line::from("")); } + // The in-flight message, shown exactly like a real "you" turn so the + // transcript reads continuously while the backend thinks. It is not in + // the thread yet — the turn task writes that on completion. + if let Some(pending) = &app.planning.pending_user { + lines.push(Line::from(Span::styled( + " you", + Style::default().fg(t.text_dim), + ))); + for l in wrap_text(pending, width.saturating_sub(2).max(8)) { + lines.push(Line::from(format!(" {}", l))); + } + lines.push(Line::from("")); + } + if app.planning.busy && !app.planning.status.is_empty() { lines.push(Line::from(Span::styled( format!(" {} {}", spinner_frame(), app.planning.status),