diff --git a/crates/adaptive/src/closing/consolidate.rs b/crates/adaptive/src/closing/consolidate.rs index 2cb4a60..d840be1 100644 --- a/crates/adaptive/src/closing/consolidate.rs +++ b/crates/adaptive/src/closing/consolidate.rs @@ -77,6 +77,9 @@ pub async fn consolidate( if rows.is_empty() { return Vec::new(); } + if was_a_plain_errand(satisfied, &rows) { + return Vec::new(); + } // Everything already stored, not a retrieval view. Retrieval answers "what // applies to this task" and cuts by help rate, so a lesson written moments // ago — nothing has had the chance to apply it — sorts last and is dropped. @@ -121,6 +124,24 @@ pub async fn consolidate( kept } +/// An episode that was one errand, and worked. +/// +/// The only shape worth skipping, and the test is deliberately narrow. An +/// errand is a goal with no procedure in it, answered in a turn — there is +/// nothing there for a different task to act on, and asking costs a call to be +/// told so. That is the whole economic case for the errand path, and paying a +/// consolidation call on every trivial goal would give most of it straight back. +/// +/// Every other shape still consolidates, including the two that look similar: +/// +/// * **an errand that failed** — the most informative errand there is. Something +/// read as one turn of work and was not, and *that* generalises. +/// * **an errand followed by a plan** — more than one row, so the trail is a +/// real one and worth reading whole. +fn was_a_plain_errand(satisfied: bool, rows: &[LedgerRow]) -> bool { + satisfied && rows.len() == 1 && rows[0].approach_sig == "errand" +} + /// One line per attempt, numbered, because the model cites rows by number. fn render(goal: &Goal, satisfied: bool, rows: &[LedgerRow], existing: &[Lesson]) -> String { let attempts = rows @@ -309,4 +330,36 @@ mod tests { assert!(rendered.contains("- L7:"), "{rendered}"); assert!(rendered.contains("corroborate by id"), "{rendered}"); } + + #[test] + fn a_satisfied_one_turn_errand_is_not_worth_a_consolidation_call() { + // The economics the errand path exists for. Three calls become four if + // every trivial goal still pays a consolidator to be told there was + // nothing in it. + assert!(was_a_plain_errand(true, &[row("r1", "errand")])); + } + + #[test] + fn an_errand_that_failed_is_the_most_informative_kind_there_is() { + // Something read as one turn of work and was not. That generalises, + // and it is exactly what the triage needs told back to it. + assert!(!was_a_plain_errand(false, &[row("r1", "errand")])); + } + + #[test] + fn an_errand_followed_by_a_real_plan_still_consolidates() { + // More than one row means a real trail, whatever the first row was. + assert!(!was_a_plain_errand( + true, + &[row("r1", "errand"), row("r2", "authored:abc")] + )); + } + + #[test] + fn an_ordinary_satisfied_episode_is_untouched_by_the_gate() { + // The gate must be narrow: one wrong `true` here silently stops the + // whole loop learning, and nothing downstream would report it. + assert!(!was_a_plain_errand(true, &[row("r1", "selected:weekly")])); + assert!(!was_a_plain_errand(true, &[row("r1", "authored:abc")])); + } } diff --git a/crates/adaptive/src/closing/mod.rs b/crates/adaptive/src/closing/mod.rs index 8535a58..8f789ae 100644 --- a/crates/adaptive/src/closing/mod.rs +++ b/crates/adaptive/src/closing/mod.rs @@ -158,7 +158,10 @@ pub async fn close( // scoring its parent instead would leave the two indistinguishable and // the promotion gate with nothing to compare. Approach::Selected { workflow_id, .. } => Some(workflow_id.clone()), - Approach::Authored { .. } => None, + // Neither has a stored procedure behind it, so there is no counter to + // move. An errand additionally has nothing to *become* one: the row it + // leaves is the whole record of it. + Approach::Authored { .. } | Approach::Errand { .. } => None, }; let row_id = ledger .append(&LedgerRow { @@ -265,7 +268,9 @@ fn decide_next(verdict: &Verdict, attempt: u32, stalled: u32, budget: &Budget) - fn why(approach: &Approach) -> String { match approach { - Approach::Selected { why, .. } | Approach::Authored { why, .. } => why.clone(), + Approach::Selected { why, .. } + | Approach::Authored { why, .. } + | Approach::Errand { why } => why.clone(), } } diff --git a/crates/adaptive/src/contracts.rs b/crates/adaptive/src/contracts.rs index 934102a..d29a532 100644 --- a/crates/adaptive/src/contracts.rs +++ b/crates/adaptive/src/contracts.rs @@ -264,6 +264,25 @@ pub enum Approach { /// and makes an identical re-author visible as the repeat it is. fingerprint: String, }, + /// One turn of work with no procedure in it. + /// + /// The third answer to "does anything stored do this", and the one that + /// says the question was wrong: some goals are not a procedure at all. + /// "What is the disk usage of this directory" has nothing in it worth + /// writing down, and putting it through authoring pays a large planning + /// call to produce a one-step graph, then files that graph where it dilutes + /// every later selection. + /// + /// It is deliberately the *narrowest* of the three. An errand is judged + /// like any other attempt and can fail; what it cannot do is leave anything + /// behind. Nothing is kept ([`crate::closing::keep`] takes only authored + /// graphs), nothing is repaired (there is no procedure to vary), and the + /// signature is a constant so a second errand inside one episode is visibly + /// a repeat — if one turn did not do it, the goal was not an errand. + Errand { + /// Why no stored workflow was needed and none is worth writing. + why: String, + }, } impl Approach { @@ -277,6 +296,13 @@ impl Approach { match self { Self::Selected { workflow_id, .. } => format!("selected:{workflow_id}"), Self::Authored { fingerprint, .. } => format!("authored:{fingerprint}"), + // No discriminator, on purpose. Two authored attempts are told + // apart by their fingerprints because the second may be a genuinely + // different graph; two errands cannot be, because an errand carries + // no plan to differ in. Signing them the same is what makes the + // second one read as the repeat it is — and what lets `decide` stop + // offering the option once it has been spent. + Self::Errand { .. } => "errand".to_string(), } } } @@ -285,6 +311,37 @@ impl Approach { mod tests { use super::*; + #[test] + fn two_errands_in_one_episode_are_visibly_the_same_attempt() { + // Unlike two authored graphs, which may genuinely differ and are told + // apart by their fingerprints. An errand carries no plan to differ in, + // so the constant signature is what puts it in the exclusion list and + // stops an episode spending its budget on identical single turns. + let first = Approach::Errand { + why: "one turn of work".into(), + }; + let second = Approach::Errand { + why: "still one turn, honestly".into(), + }; + assert_eq!(first.signature(), "errand"); + assert_eq!(first.signature(), second.signature()); + } + + #[test] + fn an_errand_cannot_collide_with_a_stored_workflow_called_errand() { + // The namespacing that makes the constant safe: `selected:` prefixes + // every workflow id, so no shelf entry can occupy the errand slot. + let selected = Approach::Selected { + workflow_id: "errand".into(), + why: String::new(), + }; + assert_eq!(selected.signature(), "selected:errand"); + assert_ne!( + selected.signature(), + Approach::Errand { why: String::new() }.signature() + ); + } + #[test] fn an_unrecognised_blocker_is_continuable_rather_than_terminal() { // `goal_not_meet` — one letter — used to end a run at attempt 3 of 12. diff --git a/crates/adaptive/src/driver.rs b/crates/adaptive/src/driver.rs index 320ac0a..057d588 100644 --- a/crates/adaptive/src/driver.rs +++ b/crates/adaptive/src/driver.rs @@ -341,7 +341,12 @@ impl Loop<'_> { // and the next attempt writes another, seeing why this one fell // short. A variant of a one-off is a stored procedure nobody asked // for. - Approach::Authored { .. } => return None, + // + // An errand is the same argument at its limit — there is no + // procedure at all, only a turn that did not land. What it needs is + // a plan, and `decide` will write one: the errand is spent, so the + // next attempt cannot choose it again. + Approach::Authored { .. } | Approach::Errand { .. } => return None, }; let evidence = ran.evidence(); if !graph_is_suspect(&closed.verdict, &evidence) { diff --git a/crates/adaptive/src/intake/mod.rs b/crates/adaptive/src/intake/mod.rs index b4e2b20..0c53835 100644 --- a/crates/adaptive/src/intake/mod.rs +++ b/crates/adaptive/src/intake/mod.rs @@ -26,6 +26,7 @@ use serde_json::{Map, Value}; use tinyflows::caps::Capabilities; use tinyflows::model::WorkflowGraph; use tinyflows::store::{WorkflowStore, WorkflowSummary}; +use tinyflows::validate::validate_all; use crate::contracts::{Approach, Goal}; use crate::host::HostFacts; @@ -156,7 +157,18 @@ pub async fn decide( let shown: Vec = lessons.iter().map(|l| l.id.clone()).collect(); - if let Some(chosen) = select(goal, &candidates, &past, caps, conn).await? { + // One errand per episode. A goal answered in a turn is an errand; a goal + // that took a turn and is still not done was never one, and offering the + // option again would let an episode spend its whole budget on single turns + // that each fall the same way short. + let errand_allowed = !tried.iter().any(|sig| sig == "errand"); + + if let Some(chosen) = select(goal, &candidates, &past, errand_allowed, caps, conn).await? { + // An errand names no stored workflow, so there is nothing to bind and + // nothing to load: its graph is lowered from the goal itself. + if matches!(chosen.approach, Approach::Errand { .. }) { + return errand_attempt(goal, chosen, facts, shown); + } // `select` answers with an id; the graph and the input check come from // the store. Returning the choice unbound would hand the engine an // empty graph, which compiles to nothing and reads as the work failing. @@ -178,7 +190,11 @@ pub async fn decide( Supply a value for every required input this time, or \ decline so a graph is written instead." ); - if let Some(retry) = select(goal, &candidates, ¬ed, caps, conn).await? { + // No errand on the retry: the question was asked and answered + // one call ago, and this round exists to fix a binding slip. + // Re-offering it would let a model that could not fill an input + // reach for the one answer that needs none. + if let Some(retry) = select(goal, &candidates, ¬ed, false, caps, conn).await? { match bind(retry, store) { Ok(attempt) => { return Ok(Attempt { @@ -215,6 +231,45 @@ pub async fn decide( }) } +/// Finish an errand: lower the one-step graph and hold it to the same gates. +/// +/// Both gates matter here even though no model wrote the graph. `validate_all` +/// is free and catches a lowering that stopped producing a runnable shape. +/// `HostFacts::check` is the one that earns its place: an errand is an agent +/// turn, and a host with no agent capability must refuse it as +/// [`Unsupported`](IntakeError::Unsupported) — the same answer authoring would +/// give — rather than emit a graph the runner will fail on and the judge will +/// then blame on the work. +fn errand_attempt( + goal: &Goal, + chosen: Attempt, + facts: &HostFacts, + shown: Vec, +) -> Result { + let graph = recipe::errand(&goal.text)?; + + let problems = validate_all(&graph); + if !problems.is_empty() { + return Err(IntakeError::Invalid( + problems + .iter() + .map(ToString::to_string) + .collect::>() + .join("; "), + )); + } + let refused = facts.check(&graph); + if !refused.is_empty() { + return Err(IntakeError::Unsupported(refused.join("; "))); + } + + Ok(Attempt { + graph, + lessons_shown: shown, + ..chosen + }) +} + /// The stored workflows worth offering, with what is known about each. /// /// Three filters, each removing something a planner must not be shown: diff --git a/crates/adaptive/src/intake/recipe.rs b/crates/adaptive/src/intake/recipe.rs index bb4971e..6a7654e 100644 --- a/crates/adaptive/src/intake/recipe.rs +++ b/crates/adaptive/src/intake/recipe.rs @@ -300,6 +300,33 @@ pub fn lower( Ok((graph, inputs, why)) } +/// The one-step graph an [`Errand`](crate::contracts::Approach::Errand) runs. +/// +/// Built by handing [`lower`] a recipe nobody wrote, rather than assembling +/// nodes directly. The graph is trivial enough that hand-building it would look +/// like the simpler option, and that is the trap: it would be a second, +/// unexercised definition of what an `ask` step compiles to, and the first +/// change to the envelope path or the trigger node would leave errands quietly +/// producing a shape the rest of the loop no longer reads. Going through the +/// same door as an authored plan costs one `json!` and cannot drift. +/// +/// Deterministic in the goal alone — no model call. The whole economic argument +/// for the errand path is that recognising one is free once `select` has +/// answered, so lowering it must not spend anything either. +/// +/// # Errors +/// Only if `lower` refuses this recipe, which would mean the shared lowering +/// path had stopped accepting a bare `ask` step. +pub(crate) fn errand(goal: &str) -> Result { + let recipe = json!({ + "why": "one turn of work, no procedure in it", + "declared": [], + "inputs": {}, + "steps": [{ "id": "errand", "ask": goal.trim() }], + }); + lower(&recipe, &[]).map(|(graph, _, _)| graph) +} + /// Ask steps that restate a declared value instead of relying on the /// attachment. Only distinctive values count — refusing a plan because an /// ask contains the word "on" would block perfectly reusable recipes — and diff --git a/crates/adaptive/src/intake/recipe_tests.rs b/crates/adaptive/src/intake/recipe_tests.rs index e31ac96..aba79e3 100644 --- a/crates/adaptive/src/intake/recipe_tests.rs +++ b/crates/adaptive/src/intake/recipe_tests.rs @@ -691,3 +691,86 @@ fn a_scripts_stdout_is_read_from_inside_its_json_because_that_is_where_it_is() { "declared inputs too: {rendered}" ); } + +#[test] +fn an_errand_lowers_to_one_agent_turn_that_validates() { + let graph = super::errand("how much disk is this directory using").expect("lowers"); + + assert!( + validate_all(&graph).is_empty(), + "the engine must accept it: {:?}", + validate_all(&graph) + ); + // A trigger and exactly one agent node. Anything more means the errand + // grew a procedure, which is the one thing it is defined as not having. + assert_eq!(graph.nodes.len(), 2, "{:?}", graph.nodes); + assert_eq!(graph.nodes[0].kind, NodeKind::Trigger); + assert_eq!(graph.nodes[1].kind, NodeKind::Agent); + assert!( + graph.inputs.is_empty(), + "an errand declares nothing: it is answered from the goal alone" + ); +} + +#[test] +fn an_errands_prompt_actually_carries_the_goal() { + // Evaluated, not string-matched. The `item.json.text` defect shipped past a + // reviewer *and* a test because both read the expression instead of running + // it — a prompt that resolves to nothing looks fine as source. + let graph = super::errand(" how much disk is this directory using ").expect("lowers"); + let prompt = graph.nodes[1].config["prompt"] + .as_str() + .expect("the agent node carries a prompt"); + let scope = json!({ + "run": { "inputs": {} }, "inputs": {}, + "item": null, "items": [], "nodes": {} + }); + let rendered = tinyflows::expr::resolve(&json!(prompt), &scope); + let rendered = rendered.as_str().expect("resolves to a string"); + assert_eq!( + rendered, "how much disk is this directory using", + "the goal, trimmed, and nothing else bolted on" + ); +} + +#[test] +fn an_errand_is_the_same_lowering_an_authored_ask_gets() { + // Why `errand` goes through `lower` rather than building two nodes by hand: + // a second definition of what an `ask` compiles to would drift silently the + // first time the envelope path changed. + let errand = super::errand("say something").expect("lowers"); + let (authored, _, _) = lower( + &json!({ + "why": "one turn of work, no procedure in it", + "declared": [], "inputs": {}, + "steps": [{ "id": "errand", "ask": "say something" }] + }), + &[], + ) + .expect("lowers"); + assert_eq!(errand.nodes[1].config, authored.nodes[1].config); +} + +#[test] +fn every_control_character_in_a_goal_survives_as_a_valid_jq_literal() { + let scope = json!({ "run": { "inputs": {} }, "inputs": {}, "item": null, + "items": [], "nodes": {} }); + let mut broke = Vec::new(); + for code in 0u32..0x20 { + let ch = char::from_u32(code).expect("control char"); + let goal = format!("disk{ch}usage"); + let Ok(graph) = super::errand(&goal) else { + broke.push(format!("U+{code:04X}: lowering refused it")); + continue; + }; + let prompt = graph.nodes[1].config["prompt"].as_str().expect("prompt"); + let rendered = tinyflows::expr::resolve(&json!(prompt), &scope); + if rendered.as_str().is_none() { + broke.push(format!("U+{code:04X}: {rendered:?}")); + } + } + assert!( + broke.is_empty(), + "control characters that broke the prompt: {broke:?}" + ); +} diff --git a/crates/adaptive/src/intake/select.rs b/crates/adaptive/src/intake/select.rs index 2a5304b..43ba95a 100644 --- a/crates/adaptive/src/intake/select.rs +++ b/crates/adaptive/src/intake/select.rs @@ -76,9 +76,11 @@ impl Candidate { const SYSTEM: &str = "\ You choose whether a saved workflow already does what a goal asks for. -Return JSON: {\"workflow_id\": str | null, \"why\": str, \"inputs\": {name: value}} +Return JSON: {\"workflow_id\": str | null, \"errand\": bool, \"why\": str, + \"inputs\": {name: value}} - workflow_id: the id of the workflow that does this, or null. +- errand: true only when the goal is one turn of work with no procedure in it. - why: one line. When you decline, say what is missing — it is read by whoever writes the replacement. - inputs: values for that workflow's declared inputs, taken from the goal. Only @@ -95,12 +97,37 @@ when it plainly matches; it just carries no evidence. When this episode has already tried something, decline rather than choose a workflow that would fall short the same way. Being told a second time that the -report has no numbers in it costs a full run and establishes nothing."; +report has no numbers in it costs a full run and establishes nothing. + +Set errand only when there is no procedure in the goal — one turn of work, +answered and finished, with nothing a later goal would want to reuse. Ask +whether you would want this written down and offered as a choice next month. + + errand \"how much disk is this directory using\" + errand \"what did the last commit change\" + NOT \"summarise a paper into three bullets\" — one step, and exactly the + kind of thing worth having on the shelf + NOT \"check the PR and fix whatever CI says\" — one sentence, many turns + +Short is not the test, and a single step is not the test: a one-step procedure +can be the most reused thing here. The test is whether a *procedure* exists. + +An errand is not an escape from a hard goal. Anything that needs several turns, +or that could fail in a way worth retrying differently, is not one — say so and +decline instead, so a graph gets written."; /// Ask whether any candidate does the job, and bind its inputs if one does. /// /// `Ok(None)` means nothing fitted — the ordinary case on a cold store, and the -/// caller's cue to author. +/// caller's cue to author. `Ok(Some)` is either a +/// [`Selected`](Approach::Selected) whose graph the caller loads from the store, +/// or an [`Errand`](Approach::Errand) whose graph the caller lowers; both come +/// back with an empty graph, because what fills it is not this function's job. +/// +/// `errand_allowed` is false once this episode has already spent its errand — +/// see [`Approach::signature`]. Withholding the option is structural rather +/// than left to the prompt, because "you already tried that" is exactly the +/// instruction a model talks itself out of on attempt three. /// /// # Errors /// When inference fails, or the chosen workflow cannot be loaded or bound. @@ -108,30 +135,70 @@ pub async fn select( goal: &Goal, candidates: &[Candidate], past: &str, + errand_allowed: bool, caps: &Capabilities, conn: Option<&str>, ) -> Result> { - // Not a shortcut — a correctness point. With nothing to choose from the - // answer can only be "none", and asking costs a call to be told so. - if candidates.is_empty() { + // With nothing to choose from and no errand to offer, the answer can only + // be "none" and asking costs a call to be told so. + // + // This used to be unconditional, on that reasoning — and the reasoning + // stopped holding the moment there was a third answer. A cold store is + // precisely where a trivial goal is most likely, so short-circuiting here + // would have made the errand path unreachable exactly where it pays most, + // while looking correct. The cost is honest and worth stating: a cold-store + // episode that is *not* an errand now pays one small extra call, against + // saving a full authoring call and its run whenever it is. + if candidates.is_empty() && !errand_allowed { return Ok(None); } - let listing = candidates - .iter() - .map(Candidate::render) - .collect::>() - .join("\n"); - let user = format!( - "# Goal\n{}\n\n# Saved workflows\n{listing}{past}", - goal.text.trim() - ); + let shelf = if candidates.is_empty() { + "# Saved workflows\n(none yet — nothing to choose from, so the only \ + question is whether this is an errand)" + .to_string() + } else { + format!( + "# Saved workflows\n{}", + candidates + .iter() + .map(Candidate::render) + .collect::>() + .join("\n") + ) + }; + let spent = if errand_allowed { + String::new() + } else { + "\n\n# This episode has already spent its errand\nOne turn was tried \ + and did not finish the goal, so it is not an errand. Choose a workflow \ + or decline; `errand` will be ignored." + .to_string() + }; + let user = format!("# Goal\n{}\n\n{shelf}{past}{spent}", goal.text.trim()); let answer = ask(caps, conn, Tier::Select, SYSTEM, &user).await?; let Some(id) = answer["workflow_id"] .as_str() .filter(|s| !s.trim().is_empty()) else { + // Declining and calling it an errand are different answers, and only + // one of them skips authoring. Read second so a model that names a + // workflow *and* sets the flag is taken at its first word — the + // workflow is the more specific claim, and the more easily checked. + if errand_allowed && answer["errand"].as_bool().unwrap_or(false) { + return Ok(Some(Attempt { + approach: Approach::Errand { + why: answer["why"].as_str().unwrap_or_default().to_string(), + }, + // Lowered by the caller, which is what holds the host facts the + // one-step graph has to be checked against. + graph: WorkflowGraph::default(), + inputs: Map::new(), + resume: None, + lessons_shown: Vec::new(), + })); + } return Ok(None); }; // A model naming something that is not on the list has hallucinated an id; @@ -216,6 +283,7 @@ fn inputs_of(answer: &Value) -> Map { #[cfg(test)] mod tests { use super::*; + use serde_json::json; fn candidate(id: &str, applied: u32, helped: u32) -> Candidate { Candidate { @@ -256,6 +324,157 @@ mod tests { assert!(c.render().contains("name: only-an-id")); } + /// A model that answers with `reply` and records what it was shown. + /// + /// The call count is the point of several tests below: whether `select` + /// asks at all is a cost decision, and asserting on the *answer* would not + /// notice a version that skipped the call and returned the same thing. + struct Scripted { + reply: Value, + asked: std::sync::Mutex>, + } + + impl Scripted { + fn new(reply: Value) -> std::sync::Arc { + std::sync::Arc::new(Self { + reply, + asked: std::sync::Mutex::new(Vec::new()), + }) + } + fn calls(&self) -> usize { + self.asked.lock().expect("log").len() + } + fn last(&self) -> String { + self.asked + .lock() + .expect("log") + .last() + .cloned() + .unwrap_or_default() + } + } + + #[async_trait::async_trait] + impl tinyflows::caps::LlmProvider for Scripted { + async fn complete( + &self, + request: Value, + _conn: Option<&str>, + ) -> tinyflows::error::Result { + self.asked.lock().expect("log").push( + request["messages"][1]["content"] + .as_str() + .unwrap_or_default() + .to_string(), + ); + Ok(self.reply.clone()) + } + } + + async fn choose( + provider: &std::sync::Arc, + candidates: &[Candidate], + errand_allowed: bool, + ) -> Option { + let caps = Capabilities { + llm: provider.clone(), + ..tinyflows::caps::mock::mock_capabilities() + }; + select( + &Goal::new("how much disk is this directory using"), + candidates, + "", + errand_allowed, + &caps, + None, + ) + .await + .expect("selection answers") + } + + #[tokio::test] + async fn an_errand_answer_becomes_an_errand_approach() { + let provider = Scripted::new(json!({ + "workflow_id": null, + "errand": true, + "why": "one turn of work, no procedure in it", + })); + let chosen = choose(&provider, &[candidate("pr-review", 4, 4)], true) + .await + .expect("an errand is an answer, not a decline"); + match chosen.approach { + Approach::Errand { why } => assert!(why.contains("one turn")), + other => panic!("expected an errand, got {other:?}"), + } + } + + #[tokio::test] + async fn an_empty_shelf_is_still_asked_when_an_errand_is_possible() { + // The bug this exists to prevent, and it would have been invisible: a + // cold store is exactly where a trivial goal is most likely, so the old + // unconditional short-circuit made the errand path unreachable at the + // one moment it pays for itself — while every test of the *answer* + // still passed. + let provider = + Scripted::new(json!({ "workflow_id": null, "errand": true, "why": "trivial" })); + let chosen = choose(&provider, &[], true).await; + assert_eq!(provider.calls(), 1, "an empty shelf must still be asked"); + assert!(matches!( + chosen.map(|a| a.approach), + Some(Approach::Errand { .. }) + )); + } + + #[tokio::test] + async fn an_empty_shelf_with_no_errand_left_is_not_asked_at_all() { + // The other half: with nothing to choose from and no errand to offer, + // the answer can only be "none", and the call is pure cost. + let provider = Scripted::new(json!({ "workflow_id": null, "errand": true, "why": "x" })); + assert!(choose(&provider, &[], false).await.is_none()); + assert_eq!(provider.calls(), 0, "nothing to ask about"); + } + + #[tokio::test] + async fn a_spent_errand_is_refused_even_when_the_model_asks_for_one() { + // Prompt-only enforcement is not enforcement: attempt three is exactly + // where a model talks itself back into the answer that needs no inputs. + let provider = + Scripted::new(json!({ "workflow_id": null, "errand": true, "why": "again" })); + let chosen = choose(&provider, &[candidate("pr-review", 4, 4)], false).await; + assert!(chosen.is_none(), "a spent errand reads as a decline"); + assert!( + provider.last().contains("already spent its errand"), + "and the prompt says why: {}", + provider.last() + ); + } + + #[tokio::test] + async fn naming_a_workflow_wins_over_also_setting_the_errand_flag() { + // A contradictory answer taken at its more specific — and more easily + // checked — word, rather than at whichever field is read first. + let provider = Scripted::new(json!({ + "workflow_id": "pr-review", + "errand": true, + "why": "both", + "inputs": { "repo": "openhuman" }, + })); + let chosen = choose(&provider, &[candidate("pr-review", 4, 4)], true) + .await + .expect("an answer"); + assert!(matches!(chosen.approach, Approach::Selected { .. })); + } + + #[test] + fn the_prompt_refuses_to_treat_a_short_goal_as_an_errand() { + // The distinction the whole triage turns on. A one-step procedure can + // be the most reused thing in the store, so brevity must not be the + // test — if this guidance goes, the flag starts eating the shelf. + assert!(SYSTEM.contains("no procedure in the goal")); + assert!(SYSTEM.contains("Short is not the test")); + assert!(SYSTEM.contains("not an escape from a hard goal")); + } + #[test] fn the_prompt_tells_the_model_that_declining_is_allowed() { // The single most important line in it: a model pushed to always pick diff --git a/crates/adaptive/tests/driver.rs b/crates/adaptive/tests/driver.rs index a544f2e..c01df08 100644 --- a/crates/adaptive/tests/driver.rs +++ b/crates/adaptive/tests/driver.rs @@ -854,3 +854,168 @@ async fn a_failed_goal_run_leaves_no_residue_anywhere_durable() { "the attempts are on the record even though no graph was kept" ); } + +#[tokio::test] +async fn an_errand_answers_the_goal_without_leaving_a_procedure_behind() { + // The whole claim of the errand path, end to end: a goal with no procedure + // in it is answered in one turn, and the shelf is exactly as it was. + struct Triage { + tiers: Mutex>, + } + #[async_trait] + impl LlmProvider for Triage { + async fn complete(&self, request: Value, _conn: Option<&str>) -> EngineResult { + let tier = request["tier"].as_str().unwrap_or_default().to_string(); + self.tiers.lock().expect("lock").push(tier.clone()); + Ok(match tier.as_str() { + "select" => json!({ + "workflow_id": null, "errand": true, + "why": "one turn of work, no procedure in it" + }), + "judge" => json!({ + "satisfied": true, "blocker": "", "gap": "", "advanced": true + }), + // Reached only if the loop wrongly authored or consolidated — + // both are asserted absent below. + _ => json!({ "why": "should not be asked", "inputs": {}, "steps": [] }), + }) + } + } + + let provider = Arc::new(Triage { + tiers: Mutex::new(Vec::new()), + }); + let caps = Capabilities { + llm: provider.clone(), + ..mock_capabilities() + }; + let ledger = MemoryLedger::new(); + let store = store("errand"); + // A workflow on the shelf, so `select` is genuinely asked rather than + // short-circuited — this test is about the answer, not about the cold-store + // path, which `select`'s own tests cover. + let seeded = store.list().expect("list").len(); + + let runner = Local { + caps: &caps, + workspace: &Unobserved, + }; + let engine = Loop { + ledger: &ledger, + store: &store, + caps: &caps, + facts: &HostFacts::unknown(), + runner: &runner, + clock: &Frozen, + budget: Default::default(), + conn: None, + }; + + let finished = engine + .run( + "ep-errand", + &Goal::new("how much disk is this directory using"), + ) + .await + .expect("the episode runs"); + + assert_eq!(finished.status, EpisodeStatus::Satisfied); + assert_eq!(finished.attempts, 1, "one turn, not a retry loop"); + + let rows = ledger.rows("ep-errand").await.expect("rows"); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].approach_sig, "errand"); + assert!( + rows[0].workflow_id.is_none(), + "an errand scores no procedure, because there is none" + ); + + // The point of the whole path: nothing filed. A one-off on the shelf is + // worse than useless — it dilutes every later selection with a row that + // matches once and can never match again. + assert_eq!( + store.list().expect("list").len(), + seeded, + "an errand must not be kept" + ); + // Asserted on the *calls*, not on the result. An empty lesson list is also + // what a consolidator that ran and found nothing returns, so the weaker + // assertion would pass with the gate removed entirely. + let tiers = provider.tiers.lock().expect("lock").clone(); + assert!( + !tiers.iter().any(|t| t == "consolidate"), + "a plain errand must not pay a consolidation call: {tiers:?}" + ); + assert!( + !tiers.iter().any(|t| t == "author"), + "nor an authoring one: {tiers:?}" + ); + assert!(finished.lessons.is_empty()); +} + +#[tokio::test] +async fn a_failed_errand_escalates_to_authoring_instead_of_repeating_itself() { + // The guard that stops the cheap path becoming a trap. If one turn did not + // do it, the goal was never an errand — and a model that keeps saying it is + // must not be able to spend the whole budget on identical single turns. + struct Insistent { + seen: Mutex>, + } + #[async_trait] + impl LlmProvider for Insistent { + async fn complete(&self, request: Value, _conn: Option<&str>) -> EngineResult { + let tier = request["tier"].as_str().unwrap_or_default().to_string(); + self.seen.lock().expect("lock").push(tier.clone()); + Ok(match tier.as_str() { + // Always insists, every attempt. + "select" => json!({ "workflow_id": null, "errand": true, "why": "trivial" }), + "judge" => json!({ + "satisfied": false, "blocker": "goal_not_met", + "gap": "it did not finish", "advanced": false + }), + "consolidate" => json!({ "lessons": [], "corroborate": [] }), + _ => json!({ + "why": "a real plan", + "inputs": {}, + "steps": [{ "id": "attempt", "run": "echo attempt-done" }], + }), + }) + } + } + let provider = Arc::new(Insistent { + seen: Mutex::new(Vec::new()), + }); + let caps = Capabilities { + llm: provider.clone(), + ..mock_capabilities() + }; + let ledger = MemoryLedger::new(); + let store = store("errand-escalate"); + let runner = Local { + caps: &caps, + workspace: &Unobserved, + }; + let engine = Loop { + ledger: &ledger, + store: &store, + caps: &caps, + facts: &HostFacts::unknown(), + runner: &runner, + clock: &Frozen, + budget: Default::default(), + conn: None, + }; + + let goal = Goal::new("do something that only looks trivial"); + engine.attempt("ep-esc", &goal).await.expect("attempt one"); + engine.attempt("ep-esc", &goal).await.expect("attempt two"); + + let rows = ledger.rows("ep-esc").await.expect("rows"); + assert_eq!(rows.len(), 2); + assert_eq!(rows[0].approach_sig, "errand"); + assert!( + rows[1].approach_sig.starts_with("authored:"), + "the second attempt must be a real plan, got {}", + rows[1].approach_sig + ); +} diff --git a/crates/adaptive/tests/intake.rs b/crates/adaptive/tests/intake.rs index 9424e00..e09d324 100644 --- a/crates/adaptive/tests/intake.rs +++ b/crates/adaptive/tests/intake.rs @@ -146,11 +146,36 @@ fn stored(id: &str, description: &str, required_input: Option<&str>) -> Workflow } } +/// A selection call that declines, scripted ahead of an authoring reply. +/// +/// Needed since the errand triage landed: `select` now has a third answer, so +/// it is asked even when the shelf is empty — the old short-circuit assumed the +/// answer could only be "none", and that stopped being true. Written into each +/// script rather than defaulted inside the harness, so a test still shows every +/// call its path makes instead of hiding one behind a lenient double. +fn select_declines() -> Value { + json!({ "workflow_id": null, "errand": false, "why": "nothing stored fits" }) +} + +/// The authoring prompt, found by what it says rather than by its position. +/// +/// Positional indexing broke the moment a call was added in front of it, and +/// would break again; the authoring system prompt is self-identifying. +fn authoring_prompt(llm: &std::sync::Arc) -> String { + llm.prompts() + .into_iter() + .find(|p| p.contains("You plan how to achieve a goal")) + .expect("the author was asked") +} + #[tokio::test] async fn an_empty_store_authors_without_asking_whether_to_select() { // With nothing to choose from the answer can only be "none". Spending a // call to be told so is the cost of every cold start. - let llm = std::sync::Arc::new(Scripted::new(vec![authored_reply("fresh", None)])); + let llm = std::sync::Arc::new(Scripted::new(vec![ + select_declines(), + authored_reply("fresh", None), + ])); let caps = caps_with(llm.clone()); let (store, _root) = empty_store("1"); let ledger = MemoryLedger::new(); @@ -168,13 +193,19 @@ async fn an_empty_store_authors_without_asking_whether_to_select() { .expect("decide"); assert!(matches!(attempt.approach, Approach::Authored { .. })); - assert_eq!( - llm.prompts().len(), - 1, - "exactly one call: the authoring one" + // Two calls, and the first one is new: a triage that costs a small call to + // ask whether this is an errand at all. It used to be one, on the reasoning + // that with nothing to choose from the answer could only be "none" — true + // until `select` gained a third answer. The trade is deliberate: a cold + // shelf is exactly where a trivial goal would otherwise pay the full + // authoring call and get a one-step graph filed for it. + assert_eq!(llm.prompts().len(), 2, "a triage call, then authoring"); + assert!( + llm.prompts()[0].contains("whether a saved workflow already does"), + "the first call is the triage" ); assert!( - llm.prompts()[0].contains("You plan how to achieve a goal"), + authoring_prompt(&llm).contains("You plan how to achieve a goal"), "authoring must speak the recipe surface, not graph syntax" ); } @@ -257,7 +288,10 @@ async fn a_workflow_already_tried_this_episode_is_not_offered_again() { // The property the whole retry edge rests on. Without it attempt two // re-selects what attempt one already failed on, and the episode pays // twice for one dead end. - let llm = std::sync::Arc::new(Scripted::new(vec![authored_reply("written", None)])); + let llm = std::sync::Arc::new(Scripted::new(vec![ + select_declines(), + authored_reply("written", None), + ])); let caps = caps_with(llm.clone()); let (store, _root) = empty_store("4"); store @@ -285,10 +319,22 @@ async fn a_workflow_already_tried_this_episode_is_not_offered_again() { matches!(attempt.approach, Approach::Authored { .. }), "the only stored workflow was excluded, so authoring is the only path left" ); - assert_eq!( - llm.prompts().len(), - 1, - "with every candidate excluded the list is empty and selection is skipped entirely" + // The triage still runs — a goal can be an errand whatever the shelf holds + // — but the excluded workflow must not appear in front of it. Asserting on + // what the chooser was *shown* is the claim this test is named for; + // asserting the call never happened only ever stood in for it. + // Precisely: absent from the *shelf*. It still appears further down, in the + // rendered history — that is the exclusion list doing its job, and asserting + // the id is absent altogether would forbid the very thing that tells the + // planner not to repeat it. + let shown = &llm.prompts()[0]; + assert!( + shown.contains("(none yet"), + "with every candidate excluded the shelf is empty: {shown}" + ); + assert!( + shown.contains("[selected:pr-review]"), + "and the history still says what was tried: {shown}" ); } @@ -447,7 +493,12 @@ async fn an_authored_graph_that_does_not_validate_is_an_error_not_a_return_value "why": "forgot the steps", "inputs": {}, }); - let llm = std::sync::Arc::new(Scripted::new(vec![broken.clone(), broken.clone(), broken])); + let llm = std::sync::Arc::new(Scripted::new(vec![ + select_declines(), + broken.clone(), + broken.clone(), + broken, + ])); let caps = caps_with(llm); let (store, _root) = empty_store("7"); let ledger = MemoryLedger::new(); @@ -468,7 +519,10 @@ async fn an_authored_graph_that_does_not_validate_is_an_error_not_a_return_value #[tokio::test] async fn a_disabled_workflow_is_never_offered() { - let llm = std::sync::Arc::new(Scripted::new(vec![authored_reply("written", None)])); + let llm = std::sync::Arc::new(Scripted::new(vec![ + select_declines(), + authored_reply("written", None), + ])); let caps = caps_with(llm.clone()); let (store, _root) = empty_store("8"); let mut off = stored("switched-off", "would have matched", None); @@ -488,10 +542,10 @@ async fn a_disabled_workflow_is_never_offered() { .await .expect("decide"); - assert_eq!( - llm.prompts().len(), - 1, - "offering a disabled workflow invites a choice that cannot be honoured" + assert!( + !llm.prompts()[0].contains("switched-off"), + "offering a disabled workflow invites a choice that cannot be honoured: {}", + llm.prompts()[0] ); } @@ -509,6 +563,7 @@ async fn a_graph_naming_a_worker_this_host_lacks_is_refused_before_it_runs() { "steps": [{ "id": "work", "ask": "do the thing", "worker": "desktop" }], }); let llm = std::sync::Arc::new(Scripted::new(vec![ + select_declines(), insistent.clone(), insistent.clone(), insistent, @@ -549,11 +604,14 @@ async fn a_graph_naming_a_worker_this_host_lacks_is_refused_before_it_runs() { async fn the_authoring_prompt_carries_what_the_host_permits() { // The facts below say agent work must name a worker, so the reply's ask // step names one — the same gate this test exists to see rendered. - let llm = std::sync::Arc::new(Scripted::new(vec![json!({ - "why": "fine", - "inputs": {}, - "steps": [{ "id": "work", "ask": "Do it directly.", "worker": "laptop" }], - })])); + let llm = std::sync::Arc::new(Scripted::new(vec![ + select_declines(), + json!({ + "why": "fine", + "inputs": {}, + "steps": [{ "id": "work", "ask": "Do it directly.", "worker": "laptop" }], + }), + ])); let caps = caps_with(llm.clone()); let (store, _root) = empty_store("facts-rendered"); let ledger = MemoryLedger::new(); @@ -578,7 +636,7 @@ async fn the_authoring_prompt_carries_what_the_host_permits() { .await .expect("decide"); - let prompt = &llm.prompts()[0]; + let prompt = &authoring_prompt(&llm); assert!(prompt.contains("What this host permits"), "{prompt}"); assert!(prompt.contains("every agent node must name config.agent_ref")); assert!(prompt.contains("Only manual triggers fire here.")); @@ -752,7 +810,10 @@ async fn the_author_is_shown_what_this_episode_already_tried() { // because nothing told it otherwise. The exclusion list only guards // *selection*; authoring has no structural guard at all. let (store, ledger, _root) = with_history("retry-1").await; - let llm = std::sync::Arc::new(Scripted::new(vec![authored_reply("third-idea", None)])); + let llm = std::sync::Arc::new(Scripted::new(vec![ + select_declines(), + authored_reply("third-idea", None), + ])); let caps = caps_with(llm.clone()); decide( @@ -767,7 +828,7 @@ async fn the_author_is_shown_what_this_episode_already_tried() { .await .expect("decide"); - let prompt = &llm.prompts()[0]; + let prompt = &authoring_prompt(&llm); assert!(prompt.contains("Already tried this episode"), "{prompt}"); assert!( prompt.contains("asked an agent to write it from memory"), @@ -832,7 +893,10 @@ async fn lessons_from_other_episodes_reach_the_planner() { .await .expect("promote"); - let llm = std::sync::Arc::new(Scripted::new(vec![authored_reply("informed", None)])); + let llm = std::sync::Arc::new(Scripted::new(vec![ + select_declines(), + authored_reply("informed", None), + ])); let caps = caps_with(llm.clone()); decide( @@ -847,7 +911,7 @@ async fn lessons_from_other_episodes_reach_the_planner() { .await .expect("decide"); - let prompt = &llm.prompts()[0]; + let prompt = &authoring_prompt(&llm); assert!(prompt.contains("Learned from earlier episodes"), "{prompt}"); assert!(prompt.contains("read them from the source"), "{prompt}"); } @@ -858,7 +922,10 @@ async fn a_first_attempt_is_told_nothing_it_would_have_to_ignore() { // empty "already tried" heading reads as a claim that something was. let (store, _root) = empty_store("retry-4"); let ledger = MemoryLedger::new(); - let llm = std::sync::Arc::new(Scripted::new(vec![authored_reply("first", None)])); + let llm = std::sync::Arc::new(Scripted::new(vec![ + select_declines(), + authored_reply("first", None), + ])); let caps = caps_with(llm.clone()); decide( @@ -873,9 +940,12 @@ async fn a_first_attempt_is_told_nothing_it_would_have_to_ignore() { .await .expect("decide"); - let prompt = &llm.prompts()[0]; - assert!(!prompt.contains("Already tried"), "{prompt}"); - assert!(!prompt.contains("Learned from earlier"), "{prompt}"); + // Every prompt, not just one: an empty heading is noise whichever planner + // reads it, and the triage call sees the same rendered past the author does. + for prompt in llm.prompts() { + assert!(!prompt.contains("Already tried"), "{prompt}"); + assert!(!prompt.contains("Learned from earlier"), "{prompt}"); + } } #[tokio::test] @@ -887,10 +957,10 @@ async fn two_authored_attempts_leave_two_distinct_signatures() { let mut signatures = Vec::new(); for (n, name) in [(0, "shape-one"), (1, "shape-two")] { - let llm = std::sync::Arc::new(Scripted::new(vec![authored_reply( - name, - if n == 1 { Some("repo") } else { None }, - )])); + let llm = std::sync::Arc::new(Scripted::new(vec![ + select_declines(), + authored_reply(name, if n == 1 { Some("repo") } else { None }), + ])); let attempt = decide( &Goal::new("write the weekly report"), "ep-sigs",