diff --git a/README.md b/README.md index e2ee34a..53868ec 100644 --- a/README.md +++ b/README.md @@ -115,7 +115,9 @@ The command help and generated runbook describe baseline selection and the rest Each eval case runs once per condition and repetition in its own clean Git repository. The two arms receive the same task and fixtures; only the condition under test changes. Assertions can combine LLM judgment with runner-owned command checks, transcript checks, and final diff limits. Scripted -`turns` resume one native harness session so follow-up answers remain part of the same conversation. +Multi-turn evals resume one native harness session so follow-up answers remain part of the same +conversation, whether the turns are scripted or derived by a responder (`eval-magic docs +conversations`). Most harness features are declared in TOML descriptors. See the current registry and resolved data instead of relying on a static compatibility table: diff --git a/docs/developer_overview.md b/docs/developer_overview.md index c502020..7d048f6 100644 --- a/docs/developer_overview.md +++ b/docs/developer_overview.md @@ -12,7 +12,8 @@ focused internal notes instead of duplicating their details. ## How an evaluation moves through the system 1. `eval-magic init` scaffolds an eval workspace next to a skill. Eval definitions describe the - task, fixtures, assertions, conditions, run count, and optional scripted follow-up turns. + task, fixtures, assertions, conditions, run count, and — for a multi-turn eval — either scripted + follow-up turns or a responder policy that derives them. 2. `eval-magic run` validates the configuration, resolves and copies the skill under test into the iteration, creates isolated task roots, stages the requested skill condition from that copy, snapshots the starting state, and writes `RUNBOOK.md`, `dispatch.json`, and related campaign @@ -147,3 +148,5 @@ implementation evidence in an internal note. `eval-magic docs isolation`. - [Shipped codebase guide](guides/codebase.md) is the repository source for `eval-magic docs codebase`. +- [Shipped conversations guide](guides/conversations.md) is the repository source for + `eval-magic docs conversations`. diff --git a/docs/guides/byoh.md b/docs/guides/byoh.md index 66a9fa4..e8af18c 100644 --- a/docs/guides/byoh.md +++ b/docs/guides/byoh.md @@ -135,9 +135,12 @@ Use this sequence: 3. Run a small eval through `run`, dispatch, `ingest`, and `finalize`. 4. Confirm that every declared enhancement was exercised by the smoke run. -Scripted `turns` require `[conversation].resume_exec_template` plus transcript extraction of ordered -assistant messages and the native session ID. There is no fresh-session fallback: `run` rejects the -case when the harness cannot preserve the conversation. +Multi-turn evals — scripted `turns` and `responder` alike — require +`[conversation].resume_exec_template` plus transcript extraction of ordered assistant messages and +the native session ID. There is no fresh-session fallback: `run` rejects the case when the harness +cannot preserve the conversation. The responder itself needs nothing further from a descriptor; it +reads the agent's message as Markdown, so it works on any harness that can resume. See +`eval-magic docs conversations`. When a shadow preflight reports a live copy, isolate every initial and resumed eval-agent dispatch before setting `isolates_live_sources = true`. The per-harness remedies and verification procedure diff --git a/docs/guides/conversations.md b/docs/guides/conversations.md new file mode 100644 index 0000000..fbbedba --- /dev/null +++ b/docs/guides/conversations.md @@ -0,0 +1,150 @@ +# Multi-turn conversations + +Most evals are one shot: the agent gets a prompt, works, and answers. Some tasks +are not like that. A realistic request often needs a decision from the user part +way through, and an eval that cannot supply one measures an agent talking to a +wall. + +An eval declares one of two ways to supply those answers. They are alternatives, +not layers — declaring both is a configuration error. + +- **`turns`** — an authored script. You say exactly what the user says, and in + what order. Use it when the exchange is the thing under test and you want it + identical in every run. +- **`responder`** — a policy that derives each answer from what the agent just + said. Use it when you do not know what the agent will ask, which is the normal + case for a real task against a real codebase. + +Declaring neither leaves the eval one shot. + +Both need a harness that can resume its own session, so a follow-up reaches the +agent that asked rather than a fresh one. `eval-magic run` rejects the eval up +front when the selected harness cannot. `eval-magic harness list` names the +`conversation-resume` capability for every harness that has it. + +## The responder + +```json +{ + "id": "add-request-caching", + "prompt": "Requests to the pricing API are slow. Can you add caching?", + "expected_output": "A working cache with the pricing endpoint under 100ms.", + "responder": { "type": "heuristic", "max_turns": 8 } +} +``` + +- **`type`** is required. `heuristic` is the only responder today. It is + deterministic and costs nothing: it reads the agent's message and applies + fixed rules, with no second model involved. +- **`max_turns`** bounds how many follow-ups the responder may synthesize. The + opening prompt is not one of them. It defaults to 8. + +Every turn the responder produces is recorded in the run's `conversation.json` +with an `origin` naming the rule that produced it, so you can audit whether the +responder distorted the run instead of taking the transcript on trust. The +eval's own opening prompt carries no `origin` — that absence is how you tell an +authored turn from a derived one. + +## What the heuristic answers + +The heuristic reads the last message of each round as Markdown and answers +exactly one shape of question: **a list of options introduced by a question.** + +A list counts as a question when the line directly above it ends with a `?`: + +``` +Which cache should I use? + +- An in-process LRU (Recommended) +- Redis +``` + +The `?` has to be the last thing said before the options appear. That is what +separates a real question from a closing summary, which is also mostly a +bulleted list and would otherwise be "answered" as though the finished task were +still open. + +Given a list, the choice is mechanical: + +| The list | Recommendation marked | The answer | +| --- | --- | --- | +| plain (`-`, `*`, `1.`) | yes | the first recommended option | +| plain | no | the first option | +| checkboxes (`- [ ]`) | yes | every recommended option | +| checkboxes | no | nothing — `None of these.` | + +Plain lists ask for exactly one choice; checkboxes ask for zero or more. That +syntax is the only signal the heuristic uses to tell them apart. + +An option counts as recommended when it carries a standalone `recommended` in +parentheses, brackets, or bold — `(Recommended)`, `[recommended]`, +`**Recommended**` — or when it is a pre-checked box, `- [x]`. + +A message that asks more than one question is answered in one turn, numbered in +the order the questions appeared. + +## How a conversation ends + +| Recorded as | When | +| --- | --- | +| `completed` | The agent's last message asked nothing. It considers the task done, so the run stops rather than burning its remaining turns. | +| `stopped`, `responder_cannot_answer` | The agent asked something with no option list. | +| `stopped`, `max_turns_reached` | The agent was still asking at the bound. | +| `timed_out` | The task outran `dispatch --timeout`. | + +A `stopped` conversation is recorded, not failed: `dispatch` exits zero and +`ingest` still records the run. But both responder stops end the conversation +with the task unfinished, so `dispatch` warns about each one by name. Read the +last assistant message before treating such a run as a data point beside a +completed one. + +Two properties are worth knowing before you read results: + +- The heuristic never guesses. A question it does not recognize stops the run + instead of inventing an answer, because a fabricated answer would silently + change what the agent was asked to do. +- It errs toward stopping. A question mark anywhere in an otherwise-finished + message stops the run rather than calling it complete. That costs a dispatch; + the alternative — recording a run as complete while the agent was still + waiting — would cost the result's credibility. + +Answering free-form questions needs a model, not rules. That is a separate +responder, and until it ships, `responder_cannot_answer` is where those runs +stop. + +## Cross-harness behaviour + +The heuristic reads plain Markdown out of the agent's message, so it needs no +per-harness support: any harness that can resume a session can run a responder +eval. Nothing is read from a harness-native question tool, and nothing needs to +be, because a dispatch runs headless with no channel to answer such a tool on. + +The shapes above are a contract, not a description of one agent. An agent that +offers options this way is answered; one that phrases them some other way stops +the run. If you are bringing your own harness and its agent asks in a shape the +table does not cover, that is a gap in the table, not in your descriptor. + +## Scripted turns + +```json +{ + "id": "clarify-before-editing", + "prompt": "The due date is wrong. Fix it.", + "expected_output": "Asks which timezone before editing.", + "turns": [ + { + "prompt": "The affected users are all in US timezones.", + "deliver_when": "agent_asks", + "agent_response_matches": "(?i)time ?zone" + }, + { "prompt": "It is a date-only field.", "deliver_when": "always" } + ] +} +``` + +Each turn is delivered in order. `deliver_when: always` delivers +unconditionally; `agent_asks` delivers only when the preceding response contains +a question mark, and `agent_response_matches` adds a regex the response must +also match. A turn whose gate is unmet stops the conversation and is recorded as +`agent_did_not_ask` or `agent_response_mismatch` — a real result about the +agent, which is usually the point of scripting the exchange. diff --git a/docs/progressive-enhancements.md b/docs/progressive-enhancements.md index efba899..084bab9 100644 --- a/docs/progressive-enhancements.md +++ b/docs/progressive-enhancements.md @@ -10,7 +10,8 @@ Harness compatibility is not a parity checklist to audit — it is **a minimal baseline every harness satisfies, plus optional enhancements** a harness's adapter opts into. Most missing enhancements have a documented lower-fidelity fallback. Native conversation resume is the deliberate exception: -an eval that declares scripted `turns` is rejected when the harness cannot preserve one session. +an eval that declares scripted `turns` or a `responder` is rejected when the harness cannot preserve +one session. ## One dispatch mechanism @@ -50,8 +51,8 @@ forces `--no-stage`; without a declared guard the run continues unguarded behind `detect-stray-writes` audit; requested models without a model flag are recorded as provenance only). Supported enhancements are provided automatically — the write guard auto-arms wherever a harness declares one and staging is active (`--no-guard` opts out). Only genuinely contradictory -flag combinations stay errors. A selected eval with `turns` also requires `[conversation]`; no -generic fresh-session fallback can preserve the meaning of a canned reply. +flag combinations stay errors. A selected eval with `turns` or a `responder` also requires +`[conversation]`; no generic fresh-session fallback can preserve the meaning of a follow-up reply. ## Where this lives in code @@ -207,14 +208,33 @@ combination. *Why harness-specific:* each CLI spells same-session continuation differently and exposes its session identifier in a different transcript event. -*What it unlocks:* an eval's ordered `turns` array. `dispatch` starts the normal one-shot command, -extracts the native session id, evaluates `agent_asks` (`?`) plus the optional response regex, and -resumes the same session for each delivered follow-up. It writes raw round transcripts -under `outputs/turn-N/` and atomically commits `conversation.json` only after a complete or normal -guardrail-stopped scenario. `ingest` skips an interrupted task with no completion artifact. - -*Fallback:* none. `run` rejects selected multi-turn evals when the harness omits this capability; -silently starting a fresh session would make the canned user response meaningless. +*What it unlocks:* an eval's ordered `turns` array **and** its `responder` policy. `dispatch` starts +the normal one-shot command, extracts the native session id, asks the eval's turn source what +follows each round, and resumes the same session for each delivered follow-up. It writes raw round +transcripts under `outputs/turn-N/` and atomically commits `conversation.json` only after a complete +or normal guardrail-stopped scenario. `ingest` skips an interrupted task with no completion +artifact. + +A scripted turn is gated by `agent_asks` (`?`) plus the optional response regex. A responder instead +*derives* each turn from the round's last assistant message and records the rule that produced it on +the turn itself. **The responder needs no descriptor field and no named capability of its own:** it +reads that message as plain Markdown — a question line followed by a list of options — so every +harness that resolves a resume template gets it for free, and none can be "missing" it. + +That portability is not a happy accident, it is forced. A dispatch runs headless with stdin +detached, so a harness-native question tool has no channel to be answered on; the runner can only +send free text as the next user turn. Text is therefore the only mechanism that fits, and it is the +one every transcript parser already normalizes into `final_text`. + +What *is* borrowed from one harness is the convention — `(Recommended)` and checkbox lists are how +Claude Code's own question UI renders choices. The recognized shapes are documented as a +harness-neutral contract in `eval-magic docs conversations`, not as "what Claude does": an agent that +offers options that way is answered identically whatever harness runs it, and one that phrases them +differently stops the run with `responder_cannot_answer` — a documented gap in the shape table, not a +missing descriptor field. Widening the table is a runner change that benefits every harness at once. + +*Fallback:* none. `run` rejects selected multi-turn evals — scripted or responder-driven — when the +harness omits this capability; silently starting a fresh session would make the answer meaningless. *Descriptor fields:* `[conversation].resume_exec_template`, with required ``, ``, `{session_arg}`, and `{prompt_arg}` placeholders, plus optional diff --git a/harnesses/template.toml b/harnesses/template.toml index 221f0e8..c200da7 100644 --- a/harnesses/template.toml +++ b/harnesses/template.toml @@ -141,8 +141,11 @@ label = "{label}" # plugin_version_field = "version" ## ------------------------------------------------------------------------------------------- -## [conversation] — native same-session continuation for scripted eval `turns`. This capability -## has no generic fallback: run rejects multi-turn evals for a harness that omits it. It requires +## [conversation] — native same-session continuation for multi-turn evals, both scripted `turns` +## and a `responder` that derives them. The responder needs nothing further from a descriptor: it +## reads the agent's own message as Markdown, so declaring this table is all it takes. +## This capability has no generic fallback: run rejects multi-turn evals for a harness that omits +## it. It requires ## [dispatch].exec_template plus transcript parsing that exposes both ordered assistant messages ## and the native session id. Named summary parsers provide those directly; a declarative extractor ## must declare [transcript.extract.assistant_messages] and [transcript.extract.session_id]. diff --git a/profiles/shared/runbook.md b/profiles/shared/runbook.md index 450ca56..ab07ab5 100644 --- a/profiles/shared/runbook.md +++ b/profiles/shared/runbook.md @@ -21,7 +21,9 @@ each task's `conversation.json`. A task that already has one is skipped, so reru command retries only what did not finish. A task that exceeds `--timeout` is recorded as timed out rather than left to stall the campaign, and a task that fails is recorded and named while the rest of the batch continues. A conversation that stops at a scripted gate is valid eval data, not a -failure. +failure. A conversation the responder stopped — because it could not answer the agent's question, +or because it hit `max_turns` — is recorded too, but it ended with the task unfinished; `dispatch` +warns about each one by name, and those runs are weaker evidence than a completed one. ``` {{INGEST_CMD}} diff --git a/schema/conversation.schema.json b/schema/conversation.schema.json index ef68fde..584fc4d 100644 --- a/schema/conversation.schema.json +++ b/schema/conversation.schema.json @@ -17,7 +17,7 @@ }, "stop_reason": { "type": "string", - "enum": ["agent_did_not_ask", "agent_response_mismatch"] + "enum": ["agent_did_not_ask", "agent_response_mismatch", "responder_cannot_answer", "max_turns_reached"] }, "stopped_before_followup": { "type": "integer", @@ -98,7 +98,51 @@ "type": { "const": "user_message" }, "ordinal": { "type": "integer", "minimum": 0 }, "round": { "type": "integer", "minimum": 1 }, - "text": { "type": "string" } + "text": { "type": "string" }, + "origin": { + "type": "object", + "required": ["responder", "answers"], + "additionalProperties": false, + "description": "How a responder derived this turn. Absent on the eval's opening prompt and on scripted turns, which are authored rather than derived.", + "properties": { + "responder": { + "type": "string", + "enum": ["heuristic"], + "description": "Which responder produced the turn." + }, + "answers": { + "type": "array", + "minItems": 1, + "description": "One entry per question the turn answered, in the order they were asked.", + "items": { + "type": "object", + "required": ["options", "rule", "chosen"], + "additionalProperties": false, + "properties": { + "question": { + "type": "string", + "description": "The question line the options hung from." + }, + "options": { + "type": "array", + "items": { "type": "string" }, + "description": "The options as the agent wrote them, before markers were stripped." + }, + "rule": { + "type": "string", + "enum": ["recommended_option", "first_option", "no_selection"], + "description": "The mechanical rule that picked this answer, so a reader can audit the selection without rerunning it." + }, + "chosen": { + "type": "array", + "items": { "type": "string" }, + "description": "The options selected, cleaned of their markers. Empty when the rule selected nothing." + } + } + } + } + } + } } }, "assistantMessage": { diff --git a/schema/evals.schema.json b/schema/evals.schema.json index 877aee0..e55a76b 100644 --- a/schema/evals.schema.json +++ b/schema/evals.schema.json @@ -78,6 +78,10 @@ "items": { "$ref": "#/definitions/scriptedTurn" }, "description": "Ordered scripted user follow-ups. Each delivered turn resumes the same agent session; absence preserves one-shot dispatch." }, + "responder": { + "$ref": "#/definitions/responder", + "description": "Derives each follow-up turn from what the agent just said, instead of scripting them. Mutually exclusive with turns; declaring neither preserves one-shot dispatch. Requires a harness with native conversation resume, exactly as turns does." + }, "expected_output": { "type": "string", "minLength": 1, @@ -118,6 +122,28 @@ "items": { "$ref": "#/definitions/assertion" }, "description": "Pass/fail criteria, added after iteration 1 when you know what outputs look like." } + }, + "allOf": [ + { + "not": { "required": ["turns", "responder"] } + } + ] + }, + "responder": { + "type": "object", + "required": ["type"], + "additionalProperties": false, + "properties": { + "type": { + "type": "string", + "enum": ["heuristic"], + "description": "Which responder answers the agent. 'heuristic' is deterministic and free: it answers a question that offers a list of options, and stops the run on anything else. Required rather than defaulted, because the responder decides what the agent hears." + }, + "max_turns": { + "type": "integer", + "minimum": 1, + "description": "Maximum follow-up turns the responder may synthesize; the opening prompt is not one of them. Defaults to 8. Reaching it is recorded as a stopped conversation, not a failure." + } } }, "scriptedTurn": { diff --git a/schema/run-record.schema.json b/schema/run-record.schema.json index be59420..ac49565 100644 --- a/schema/run-record.schema.json +++ b/schema/run-record.schema.json @@ -102,7 +102,7 @@ "properties": { "status": { "type": "string", - "enum": ["completed", "stopped"] + "enum": ["completed", "stopped", "timed_out"] }, "delivered_followups": { "type": "integer", @@ -110,15 +110,20 @@ }, "stop_reason": { "type": "string", - "enum": ["agent_did_not_ask", "agent_response_mismatch"] + "enum": ["agent_did_not_ask", "agent_response_mismatch", "responder_cannot_answer", "max_turns_reached"] }, "stopped_before_followup": { "type": "integer", "minimum": 1 }, + "timed_out_in_round": { + "type": "integer", + "minimum": 1, + "description": "The round the dispatch was killed in, when it outran its per-task deadline." + }, "events": { "type": "array", - "minItems": 2, + "minItems": 1, "items": { "oneOf": [ { "$ref": "#/definitions/userMessage" }, @@ -135,7 +140,8 @@ "required": ["status"] }, "then": { - "required": ["stop_reason", "stopped_before_followup"] + "required": ["stop_reason", "stopped_before_followup"], + "not": { "required": ["timed_out_in_round"] } } }, { @@ -144,6 +150,22 @@ "required": ["status"] }, "then": { + "not": { + "anyOf": [ + { "required": ["stop_reason"] }, + { "required": ["stopped_before_followup"] }, + { "required": ["timed_out_in_round"] } + ] + } + } + }, + { + "if": { + "properties": { "status": { "const": "timed_out" } }, + "required": ["status"] + }, + "then": { + "required": ["timed_out_in_round"], "not": { "anyOf": [ { "required": ["stop_reason"] }, @@ -151,6 +173,13 @@ ] } } + }, + { + "if": { + "properties": { "status": { "enum": ["completed", "stopped"] } }, + "required": ["status"] + }, + "then": { "properties": { "events": { "minItems": 2 } } } } ] }, @@ -162,7 +191,51 @@ "type": { "const": "user_message" }, "ordinal": { "type": "integer", "minimum": 0 }, "round": { "type": "integer", "minimum": 1 }, - "text": { "type": "string" } + "text": { "type": "string" }, + "origin": { + "type": "object", + "required": ["responder", "answers"], + "additionalProperties": false, + "description": "How a responder derived this turn. Absent on the eval's opening prompt and on scripted turns, which are authored rather than derived.", + "properties": { + "responder": { + "type": "string", + "enum": ["heuristic"], + "description": "Which responder produced the turn." + }, + "answers": { + "type": "array", + "minItems": 1, + "description": "One entry per question the turn answered, in the order they were asked.", + "items": { + "type": "object", + "required": ["options", "rule", "chosen"], + "additionalProperties": false, + "properties": { + "question": { + "type": "string", + "description": "The question line the options hung from." + }, + "options": { + "type": "array", + "items": { "type": "string" }, + "description": "The options as the agent wrote them, before markers were stripped." + }, + "rule": { + "type": "string", + "enum": ["recommended_option", "first_option", "no_selection"], + "description": "The mechanical rule that picked this answer, so a reader can audit the selection without rerunning it." + }, + "chosen": { + "type": "array", + "items": { "type": "string" }, + "description": "The options selected, cleaned of their markers. Empty when the rule selected nothing." + } + } + } + } + } + } } }, "assistantMessage": { diff --git a/src/cli/args.rs b/src/cli/args.rs index 9d5ce0a..82baca9 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -609,12 +609,15 @@ pub(crate) enum Commands { /// failed. A conversation that stops at a scripted gate is valid eval data, /// not a failure. /// - /// A task declaring scripted follow-up turns resumes the same native session - /// for every turn it delivers, and each round must report the same native - /// session ID or that task fails. A completed or normally stopped - /// conversation records `delivered_followups`; an interrupted task commits no - /// artifact, so a rerun picks it up. Inspect the per-round assistant messages - /// and the delivered count to verify a script ran as intended. + /// A multi-turn task — one declaring scripted `turns`, or a `responder` that + /// derives them — resumes the same native session for every turn it + /// delivers, and each round must report the same native session ID or that + /// task fails. A completed or normally stopped conversation records + /// `delivered_followups`; an interrupted task commits no artifact, so a + /// rerun picks it up. A responder that could not answer, or that hit its + /// `max_turns` bound, is recorded and warned about: the run ended mid-task, + /// so read its last assistant message before trusting it. See + /// `eval-magic docs conversations`. Dispatch(DispatchArgs), /// Snapshot a workspace baseline. /// @@ -775,9 +778,11 @@ pub(crate) enum Commands { /// does not run agents, ingest transcripts, finalize, or promote results. /// /// Extend the seed in `evals/evals.json`: `turns` scripts same-session - /// follow-ups, `files_root` mounts fixture sources at the task root, and a - /// per-eval `runs` value overrides `run --runs`. Add assertions after the first - /// iteration, then check the file with `eval-magic validate`. + /// follow-ups and `responder` derives them instead (see + /// `eval-magic docs conversations`), `files_root` mounts fixture sources at + /// the task root, and a per-eval `runs` value overrides `run --runs`. Add + /// assertions after the first iteration, then check the file with + /// `eval-magic validate`. Init(InitArgs), /// Promote a benchmark and gradings into a committed baseline. /// diff --git a/src/cli/run/conversation.rs b/src/cli/run/conversation.rs index f9d052e..1d10be5 100644 --- a/src/cli/run/conversation.rs +++ b/src/cli/run/conversation.rs @@ -13,7 +13,6 @@ use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; use anyhow::{Context, anyhow, bail}; -use regex::Regex; use crate::adapters::cli_command::shell_quote_arg; use crate::adapters::descriptor::subst; @@ -21,36 +20,85 @@ use crate::adapters::descriptor_adapter::DescriptorAdapter; use crate::adapters::harness::HarnessAdapter; use crate::adapters::transcript::{TranscriptEvent, TranscriptSummary}; use crate::core::{ - ConversationEvent, ConversationRecord, ConversationStatus, ConversationStopReason, DeliverWhen, - ScriptedTurn, ShellOutcome, run_in_posix_shell, + ConversationEvent, ConversationRecord, ConversationStatus, ConversationStopReason, + ShellOutcome, run_in_posix_shell, }; use crate::validation::{SchemaName, validate_against_schema}; use super::dispatch::DispatchTask; +use turn_plan::{NextTurn, TurnPlan}; + +mod responder; +mod turn_plan; /// How one dispatched task ended. A failure is not represented here — it stays /// an `Err`, which the batch driver records per task rather than propagating. #[derive(Debug, Clone, PartialEq, Eq)] pub enum TaskOutcome { - Completed { delivered_followups: u32 }, - Stopped { before_followup: u32 }, - TimedOut { round: u32 }, + Completed { + delivered_followups: u32, + source: TurnSource, + }, + Stopped { + before_followup: u32, + /// Always present in practice — the schema requires a stop reason on a + /// stopped conversation — but carried as written rather than filled in + /// with a guess, so an outcome can never name the wrong reason. + reason: Option, + }, + TimedOut { + round: u32, + }, SkippedExisting, } +/// What produced a task's follow-up turns. Only used to word an outcome: a +/// scripted run and a responder-driven one stop for different reasons and an +/// operator reading the batch summary needs to know which they are looking at. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TurnSource { + Scripted, + Responder, +} + +impl TurnSource { + fn noun(self) -> &'static str { + match self { + Self::Scripted => "scripted follow-up turn(s)", + Self::Responder => "responder turn(s)", + } + } +} + impl TaskOutcome { /// The one-line human summary of this outcome. pub fn summary(&self) -> String { match self { Self::Completed { delivered_followups: 0, + .. } => "completed".to_string(), Self::Completed { delivered_followups, - } => format!("completed with {delivered_followups} scripted follow-up turn(s)"), - Self::Stopped { before_followup } => { - format!("stopped before scripted follow-up {before_followup}") - } + source, + } => format!("completed with {delivered_followups} {}", source.noun()), + Self::Stopped { + before_followup, + reason: Some(ConversationStopReason::ResponderCannotAnswer), + } => format!( + "stopped before turn {before_followup} — the responder could not answer the \ + agent's question" + ), + Self::Stopped { + before_followup, + reason: Some(ConversationStopReason::MaxTurnsReached), + } => format!( + "stopped at the responder's max_turns bound after {} turn(s)", + before_followup.saturating_sub(1) + ), + Self::Stopped { + before_followup, .. + } => format!("stopped before scripted follow-up {before_followup}"), Self::TimedOut { round } => format!("timed out in round {round}"), Self::SkippedExisting => "skipped (already complete)".to_string(), } @@ -71,9 +119,9 @@ pub fn run_task( // One budget for the whole task, not per round: a scripted conversation is // a single dispatch from the operator's point of view. let deadline = timeout.map(|timeout| Instant::now() + timeout); - // Empty for a one-shot task: the runner drives every dispatch, so the - // follow-up loop below simply has nothing to deliver. - let turns = task.turns.as_deref().unwrap_or_default(); + // A one-shot task takes the same path with nothing to deliver, so the loop + // below is the single delivery path for every shape of task. + let plan = TurnPlan::for_task(task); let conversation_path = task .conversation_path .as_deref() @@ -95,17 +143,17 @@ pub fn run_task( let initial_template = adapter .cli_exec_command(guard, agent_model, agent_env) .ok_or_else(|| anyhow!("harness declares no initial dispatch command"))?; - // Only a scripted task resumes a session, and a harness may support one-shot - // dispatch without declaring `[conversation]` at all (cline does). Requiring - // the template up front would make those harnesses undispatchable. - let resume_template = if turns.is_empty() { - None - } else { + // Only a multi-turn task resumes a session, and a harness may support + // one-shot dispatch without declaring `[conversation]` at all (cline does). + // Requiring the template up front would make those harnesses undispatchable. + let resume_template = if plan.delivers_followups() { Some( adapter .cli_resume_command(guard, agent_model, agent_env) .ok_or_else(|| anyhow!("harness declares no native conversation resume command"))?, ) + } else { + None }; if overwrite && conversation_path.exists() { fs::remove_file(&conversation_path).with_context(|| { @@ -121,6 +169,7 @@ pub fn run_task( ordinal: 0, round: 1, text: task.user_prompt.clone(), + origin: None, }]; let mut next_ordinal = 1_u32; @@ -157,6 +206,7 @@ pub fn run_task( events, }, None, + plan.source(), ); } let first_summary = parse_round(adapter, &first_outputs, &events_filename, 1)?; @@ -177,19 +227,25 @@ pub fn run_task( let mut stopped_before_followup = None; let mut timed_out_in_round = None; - for (index, turn) in turns.iter().enumerate() { - let followup = u32::try_from(index + 1).unwrap_or(u32::MAX); - if let Some(reason) = unmet_gate(turn, &preceding_assistant)? { - stop_reason = Some(reason); - stopped_before_followup = Some(followup); - break; - } + loop { + let followup = delivered_followups.saturating_add(1); + let next = plan.next_turn(delivered_followups, &preceding_assistant, &final_message)?; + let (prompt, origin) = match next { + NextTurn::Done => break, + NextTurn::Stop(reason) => { + stop_reason = Some(reason); + stopped_before_followup = Some(followup); + break; + } + NextTurn::Deliver { text, origin } => (text, origin), + }; let round = followup.saturating_add(1); events.push(ConversationEvent::UserMessage { ordinal: next_ordinal, round, - text: turn.prompt.clone(), + text: prompt.clone(), + origin, }); next_ordinal = next_ordinal.saturating_add(1); delivered_followups = delivered_followups.saturating_add(1); @@ -197,14 +253,14 @@ pub fn run_task( let round_outputs = base_outputs.join(format!("turn-{round}")); let resume_template = resume_template .as_deref() - .expect("a task with turns resolved a resume template above"); + .expect("a task that delivers follow-ups resolved a resume template above"); let command = render_command( resume_template, eval_root, &task.dispatch_prompt_path, &round_outputs, Some(&session_id), - Some(&turn.prompt), + Some(&prompt), round, ); if execute_round( @@ -252,6 +308,7 @@ pub fn run_task( events, }, Some(final_message), + plan.source(), ) } @@ -263,6 +320,7 @@ fn write_conversation( base_outputs: &Path, conversation: ConversationRecord, final_message: Option, + source: TurnSource, ) -> anyhow::Result { let _: ConversationRecord = validate_against_schema( SchemaName::Conversation, @@ -281,9 +339,11 @@ fn write_conversation( Ok(match conversation.status { ConversationStatus::Completed => TaskOutcome::Completed { delivered_followups: conversation.delivered_followups, + source, }, ConversationStatus::Stopped => TaskOutcome::Stopped { before_followup: conversation.stopped_before_followup.unwrap_or_default(), + reason: conversation.stop_reason, }, ConversationStatus::TimedOut => TaskOutcome::TimedOut { round: conversation.timed_out_in_round.unwrap_or(1), @@ -355,26 +415,6 @@ fn append_summary_events( Ok(assistant_messages.join("\n")) } -fn unmet_gate( - turn: &ScriptedTurn, - preceding_assistant: &str, -) -> anyhow::Result> { - if turn.deliver_when == DeliverWhen::Always { - return Ok(None); - } - if !preceding_assistant.contains('?') { - return Ok(Some(ConversationStopReason::AgentDidNotAsk)); - } - if let Some(pattern) = &turn.agent_response_matches { - let regex = Regex::new(pattern) - .with_context(|| format!("invalid agent_response_matches regex {pattern:?}"))?; - if !regex.is_match(preceding_assistant) { - return Ok(Some(ConversationStopReason::AgentResponseMismatch)); - } - } - Ok(None) -} - /// Render a one-shot dispatch command: the exec template with its task /// placeholders bound and no session to resume. Judge dispatch uses this too, /// binding the iteration directory and the judge prompt. @@ -480,47 +520,10 @@ fn write_json_atomic(path: &Path, value: &impl serde::Serialize) -> anyhow::Resu mod tests { use std::collections::BTreeMap; - use super::{append_summary_events, execute_round, unmet_gate}; + use super::{append_summary_events, execute_round}; use crate::adapters::TranscriptSummary; use crate::adapters::cli_command::shell_quote_arg; use crate::adapters::transcript::TranscriptEvent; - use crate::core::{ConversationStopReason, DeliverWhen, ScriptedTurn}; - - fn conditional(pattern: Option<&str>) -> ScriptedTurn { - ScriptedTurn { - prompt: "follow up".into(), - deliver_when: DeliverWhen::AgentAsks, - agent_response_matches: pattern.map(str::to_string), - } - } - - #[test] - fn agent_asks_requires_a_question_mark() { - assert_eq!( - unmet_gate(&conditional(None), "Please provide the timezone.").unwrap(), - Some(ConversationStopReason::AgentDidNotAsk) - ); - assert_eq!( - unmet_gate(&conditional(None), "Which timezone?").unwrap(), - None - ); - } - - #[test] - fn response_pattern_is_an_additional_compatibility_gate() { - assert_eq!( - unmet_gate(&conditional(Some("(?i)time ?zone")), "Which locale?").unwrap(), - Some(ConversationStopReason::AgentResponseMismatch) - ); - assert_eq!( - unmet_gate( - &conditional(Some("(?i)time ?zone")), - "Which timezone should I use?" - ) - .unwrap(), - None - ); - } #[test] fn execute_round_creates_the_round_output_directory_before_shell_redirection() { diff --git a/src/cli/run/conversation/responder.rs b/src/cli/run/conversation/responder.rs new file mode 100644 index 0000000..ca716da --- /dev/null +++ b/src/cli/run/conversation/responder.rs @@ -0,0 +1,422 @@ +//! The heuristic responder: what a user would say next, decided mechanically. +//! +//! It reads one round's final assistant message as plain Markdown, which is why +//! it needs no harness-specific code — every harness's transcript parser +//! normalizes that text into `TranscriptSummary::final_text` already. + +use std::sync::LazyLock; + +use regex::Regex; + +use crate::core::{ResponderAnswer, ResponderKind, ResponderRule, TurnOrigin}; + +/// What the heuristic made of one assistant turn. +pub(super) enum Reading { + /// No question was asked — the agent considers the task done. + NoQuestion, + /// A question with no option list the heuristic can answer. + Unanswerable, + /// A mechanically answerable question, with the reply and its provenance. + Answer { text: String, origin: TurnOrigin }, +} + +/// A list item: `- text`, `* text`, `+ text`, `1. text`, or `1) text`. Up to +/// three leading spaces, matching Markdown's own tolerance before a deeper +/// indent turns the line into a continuation of the item above it. +static OPTION_LINE: LazyLock = + LazyLock::new(|| Regex::new(r"^ {0,3}(?:[-*+]|\d{1,3}[.)])\s+(.*)$").unwrap()); + +/// A standalone `recommended`, in parentheses, brackets, or bold. Deliberately +/// narrow: an option that merely discusses what it recommends is not a marker. +/// A task-list marker at the head of an option body: `[ ]`, `[x]`, or `[X]`. +/// Its presence anywhere in a group is what makes the group multi-select. +static CHECKBOX: LazyLock = LazyLock::new(|| Regex::new(r"^\[( |x|X)\]\s*").unwrap()); + +static RECOMMENDED: LazyLock = LazyLock::new(|| { + Regex::new(r"(?i)(\(\s*recommended\s*\)|\[\s*recommended\s*\]|\*\*\s*recommended\s*\*\*)") + .unwrap() +}); + +/// One list of options, with the lines that introduced it. +struct OptionGroup { + question: Option, + options: Vec, +} + +pub(super) fn read(final_message: &str) -> Reading { + let lines: Vec<&str> = final_message.lines().collect(); + let answers: Vec = question_groups(&lines) + .iter() + .filter_map(answer_for) + .collect(); + if answers.is_empty() { + return if final_message.contains('?') { + Reading::Unanswerable + } else { + Reading::NoQuestion + }; + } + Reading::Answer { + text: render_reply(&answers), + origin: TurnOrigin { + responder: ResponderKind::Heuristic, + answers, + }, + } +} + +/// Every option list in the message whose lead-in asks something. +fn question_groups(lines: &[&str]) -> Vec { + let mut groups = Vec::new(); + let mut index = 0; + while index < lines.len() { + let Some(option) = option_body(lines[index]) else { + index += 1; + continue; + }; + let start = index; + let mut options = vec![option]; + index += 1; + while index < lines.len() { + if let Some(option) = option_body(lines[index]) { + options.push(option); + index += 1; + } else if lines[index].trim().is_empty() + && lines + .get(index + 1) + .copied() + .and_then(option_body) + .is_some() + { + // A loose Markdown list puts a blank line between its items. + index += 1; + } else { + break; + } + } + if options.len() < 2 { + continue; + } + if let Some(question) = lead_in_question(lines, start) { + groups.push(OptionGroup { + question: Some(question), + options, + }); + } + } + groups +} + +/// The text of a list item, or `None` when the line is not one. +fn option_body(line: &str) -> Option { + let body = OPTION_LINE.captures(line)?.get(1)?.as_str().trim(); + (!body.is_empty()).then(|| body.to_string()) +} + +/// The question a group hangs from: the last line of the contiguous non-blank +/// block directly above it, and only when that line *ends* with `?`. +/// +/// Ending, not merely containing: a closing summary's list is introduced by a +/// line like `Here is what changed:`, and that line may well also carry a +/// rhetorical question earlier in the sentence. Answering such a list would +/// derail a finished task, so the `?` has to be the last thing said before the +/// options appear. +fn lead_in_question(lines: &[&str], start: usize) -> Option { + let mut end = start; + while end > 0 && lines[end - 1].trim().is_empty() { + end -= 1; + } + if end == 0 || option_body(lines[end - 1]).is_some() { + return None; + } + let question = strip_emphasis(lines[end - 1].trim()); + question.ends_with('?').then(|| question.to_string()) +} + +fn strip_emphasis(text: &str) -> &str { + text.trim_matches(|c| c == '*' || c == '_' || c == '`' || c == '#') + .trim() +} + +/// Apply the selection rules to one group. +fn answer_for(group: &OptionGroup) -> Option { + // Checkbox syntax is the only mechanical signal that a question takes zero + // or more answers rather than exactly one. + let multi_select = group.options.iter().any(|option| CHECKBOX.is_match(option)); + let recommended: Vec = group + .options + .iter() + .filter(|option| is_recommended(option)) + .map(|option| clean(option)) + .collect(); + let (rule, chosen) = match (recommended.is_empty(), multi_select) { + (false, true) => (ResponderRule::RecommendedOption, recommended), + (false, false) => ( + ResponderRule::RecommendedOption, + vec![recommended[0].clone()], + ), + (true, true) => (ResponderRule::NoSelection, Vec::new()), + (true, false) => (ResponderRule::FirstOption, vec![clean(&group.options[0])]), + }; + Some(ResponderAnswer { + question: group.question.clone(), + options: group.options.clone(), + rule, + chosen, + }) +} + +/// A marked recommendation, or a pre-checked box — the plainest statement of a +/// suggested default a Markdown list can carry. +fn is_recommended(option: &str) -> bool { + RECOMMENDED.is_match(option) + || CHECKBOX + .captures(option) + .and_then(|caps| caps.get(1)) + .is_some_and(|marker| marker.as_str() != " ") +} + +/// An option as a user would say it back: markers stripped, spacing tidied. +fn clean(option: &str) -> String { + RECOMMENDED + .replace_all(&CHECKBOX.replace(option, ""), "") + .split_whitespace() + .collect::>() + .join(" ") + .trim_end_matches(['-', '\u{2014}', ':', ',']) + .trim() + .to_string() +} + +/// What the responder says when a checkbox question recommends nothing. A user +/// still has to answer, and "nothing" is the answer the rules produced. +const NO_SELECTION_REPLY: &str = "None of these."; + +fn render_reply(answers: &[ResponderAnswer]) -> String { + answers + .iter() + .enumerate() + .map(|(index, answer)| { + let chosen = match answer.chosen.is_empty() { + true => NO_SELECTION_REPLY.to_string(), + false => answer.chosen.join(", "), + }; + // A single answer needs no ordinal; several do, so the agent can + // tell which reply belongs to which question it asked. + match answers.len() { + 1 => chosen, + _ => format!("{}. {chosen}", index + 1), + } + }) + .collect::>() + .join("\n") +} + +#[cfg(test)] +mod tests { + use super::{Reading, read}; + use crate::core::ResponderRule; + + /// The happy path #244 names: an option marked as recommended is the answer. + #[test] + fn a_recommended_option_is_chosen() { + let message = "\ +I can add caching two ways. Which do you want? + +- Use an in-process LRU cache (Recommended) +- Add Redis +"; + + let Reading::Answer { text, origin } = read(message) else { + panic!("a recommended option must be answerable"); + }; + + assert_eq!(text, "Use an in-process LRU cache"); + assert_eq!(origin.answers.len(), 1); + assert_eq!(origin.answers[0].rule, ResponderRule::RecommendedOption); + assert_eq!(origin.answers[0].chosen, ["Use an in-process LRU cache"]); + } + + /// Exactly one choice is required and none is recommended, so the list's + /// first option wins. Mechanical, not a judgement about which is better. + #[test] + fn a_plain_list_with_no_recommendation_takes_the_first_option() { + let message = "\ +Which database should I target? + +1. PostgreSQL +2. MySQL +3. SQLite +"; + + let Reading::Answer { text, origin } = read(message) else { + panic!("a plain option list is answerable"); + }; + + assert_eq!(text, "PostgreSQL"); + assert_eq!(origin.answers[0].rule, ResponderRule::FirstOption); + assert_eq!(origin.answers[0].chosen, ["PostgreSQL"]); + } + + /// A checkbox list asks for zero or more. With nothing recommended, zero is + /// the mechanical answer — the responder does not invent a preference. + #[test] + fn a_checkbox_list_with_no_recommendation_selects_nothing() { + let message = "\ +Which extras should I include? + +- [ ] Unit tests +- [ ] Integration tests +- [ ] Benchmarks +"; + + let Reading::Answer { text, origin } = read(message) else { + panic!("a checkbox list is answerable"); + }; + + assert_eq!(text, "None of these."); + assert_eq!(origin.answers[0].rule, ResponderRule::NoSelection); + assert!(origin.answers[0].chosen.is_empty()); + } + + /// Zero or more means every recommendation can be taken, unlike a + /// single-choice list where only the first can. + #[test] + fn a_checkbox_list_selects_every_recommended_option() { + let message = "\ +Which extras should I include? + +- [ ] Unit tests (Recommended) +- [ ] Integration tests +- [ ] Docs (Recommended) +"; + + let Reading::Answer { text, origin } = read(message) else { + panic!("a checkbox list is answerable"); + }; + + assert_eq!(text, "Unit tests, Docs"); + assert_eq!(origin.answers[0].rule, ResponderRule::RecommendedOption); + assert_eq!(origin.answers[0].chosen, ["Unit tests", "Docs"]); + } + + /// A pre-checked box is the plainest possible statement of a suggested + /// default, so it reads as a recommendation. + #[test] + fn a_pre_checked_box_counts_as_a_recommendation() { + let message = "\ +Which extras should I include? + +- [ ] Unit tests +- [x] Docs +"; + + let Reading::Answer { text, origin } = read(message) else { + panic!("a checkbox list is answerable"); + }; + + assert_eq!(text, "Docs"); + assert_eq!(origin.answers[0].rule, ResponderRule::RecommendedOption); + } + + /// A message may ask more than one thing. Each list is answered under its + /// own rule, and the reply is numbered so the agent can tell them apart. + #[test] + fn two_option_groups_are_answered_in_order() { + let message = "\ +A couple of decisions before I start. + +Which database? + +- PostgreSQL (Recommended) +- MySQL + +Which extras do you want? + +- [ ] Benchmarks +- [ ] Fuzzing +"; + + let Reading::Answer { text, origin } = read(message) else { + panic!("two option lists are answerable"); + }; + + assert_eq!(text, "1. PostgreSQL\n2. None of these."); + assert_eq!(origin.answers.len(), 2); + assert_eq!(origin.answers[0].rule, ResponderRule::RecommendedOption); + assert_eq!(origin.answers[1].rule, ResponderRule::NoSelection); + assert_eq!( + origin.answers[1].question.as_deref(), + Some("Which extras do you want?") + ); + } + + /// The load-bearing negative: a closing summary is mostly a bulleted list, + /// and answering one as if it were a question would derail a finished task. + /// Requiring the lead-in to ask something is what separates the two. + #[test] + fn a_closing_summary_with_a_bulleted_list_is_not_a_question() { + let message = "\ +Done. I made these changes: + +- Fixed the date parser +- Added a regression test +- Updated the changelog +"; + + assert!(matches!(read(message), Reading::NoQuestion)); + } + + /// A question with no options is the branch the LLM responder takes over. + /// Until then it stops the run rather than inventing an answer. + #[test] + fn a_free_form_question_is_unanswerable() { + let message = "Before I start — what should happen to rows with a null created_at?"; + + assert!(matches!(read(message), Reading::Unanswerable)); + } + + /// Completion detection: no question at all means the agent is done, so the + /// conversation ends instead of burning its remaining turns. + #[test] + fn a_message_with_no_question_reads_as_done() { + let message = "Caching is in place and the pricing endpoint is under 40ms."; + + assert!(matches!(read(message), Reading::NoQuestion)); + } + + /// Removing a trailing marker can leave the separator that introduced it + /// dangling, and echoing "Use an in-process LRU —" back at the agent reads + /// as a truncated thought. + #[test] + fn a_separator_left_by_a_trailing_marker_is_cleaned_up() { + let message = "\ +Which cache should I use? + +- An in-process LRU — (Recommended) +- Redis +"; + + let Reading::Answer { text, .. } = read(message) else { + panic!("a recommended option must be answerable"); + }; + + assert_eq!(text, "An in-process LRU"); + } + + /// A deliberate, documented conservatism: a stray question mark anywhere in + /// an otherwise-finished message stops the run instead of calling it done. + /// Stopping wastes a dispatch; guessing "complete" while the agent waits + /// would record a half-finished run as data. + #[test] + fn a_stray_question_mark_stops_rather_than_claiming_completion() { + let message = "\ +Why a decorator? It keeps the call sites untouched. Here is what changed: + +- Wrapped the client +- Added the cache +"; + + assert!(matches!(read(message), Reading::Unanswerable)); + } +} diff --git a/src/cli/run/conversation/turn_plan.rs b/src/cli/run/conversation/turn_plan.rs new file mode 100644 index 0000000..189a379 --- /dev/null +++ b/src/cli/run/conversation/turn_plan.rs @@ -0,0 +1,183 @@ +//! What the user says next, and why. +//! +//! The driver in [`super`] owns running a round and recording it; this owns the +//! decision between rounds. Both shapes of multi-turn eval resolve to one +//! [`TurnPlan`], so the driver has a single delivery path whatever it is running. + +use anyhow::Context; +use regex::Regex; + +use crate::cli::run::dispatch::DispatchTask; +use crate::core::{ConversationStopReason, DeliverWhen, ResponderPolicy, ScriptedTurn, TurnOrigin}; + +use super::{TurnSource, responder}; + +/// Where a task's follow-up turns come from. Resolving this once, up front, +/// keeps the driver's loop to a single delivery path whatever shape of task it +/// is running. +pub(super) enum TurnPlan<'a> { + /// A one-shot task: dispatched once, with nothing to follow up. + OneShot, + /// An authored script, delivered in order behind its gates. + Scripted(&'a [ScriptedTurn]), + /// A policy that derives each turn from what the agent just said. + Responder(&'a ResponderPolicy), +} + +/// What the plan wants to happen after a round. +pub(super) enum NextTurn { + /// The conversation is finished — a script ran out, or the agent stopped + /// asking. + Done, + /// Halt and record why. A normal outcome, not a failure. + Stop(ConversationStopReason), + /// Send this as the next user turn. `origin` names the responder rule that + /// produced it, and is absent for an authored scripted turn. + Deliver { + text: String, + origin: Option, + }, +} + +impl<'a> TurnPlan<'a> { + pub(super) fn for_task(task: &'a DispatchTask) -> Self { + // `turns` and `responder` are mutually exclusive by config validation, + // so the order here only decides which wins if that gate is ever + // bypassed — the authored script does, being the more explicit of the two. + match (task.turns.as_deref(), task.responder.as_ref()) { + (Some(turns), _) if !turns.is_empty() => Self::Scripted(turns), + (_, Some(responder)) => Self::Responder(responder), + _ => Self::OneShot, + } + } + + /// Whether this plan can resume a session, and therefore needs the + /// harness's resume template. + pub(super) fn delivers_followups(&self) -> bool { + !matches!(self, Self::OneShot) + } + + pub(super) fn source(&self) -> TurnSource { + match self { + Self::Responder(_) => TurnSource::Responder, + // A one-shot task delivers nothing, so its source never reaches the + // wording — either arm would do, and the scripted one is the older. + Self::OneShot | Self::Scripted(_) => TurnSource::Scripted, + } + } + + /// Decide what follows a round. + /// + /// `preceding_assistant` is every assistant message of the round joined, + /// which is what a scripted gate has always been evaluated against. + /// `final_message` is just the round's last message — the one a user would + /// actually be answering — and is what the responder reads. + pub(super) fn next_turn( + &self, + delivered: u32, + preceding_assistant: &str, + final_message: &str, + ) -> anyhow::Result { + match self { + Self::OneShot => Ok(NextTurn::Done), + Self::Scripted(turns) => { + let Some(turn) = turns.get(delivered as usize) else { + return Ok(NextTurn::Done); + }; + if let Some(reason) = unmet_gate(turn, preceding_assistant)? { + return Ok(NextTurn::Stop(reason)); + } + Ok(NextTurn::Deliver { + text: turn.prompt.clone(), + origin: None, + }) + } + Self::Responder(policy) => Ok(responder_turn(policy, delivered, final_message)), + } + } +} + +/// Classify the agent's last message, then bound the result. +/// +/// Classification comes first deliberately: an agent that has stopped asking +/// has finished the task, and finishing on the last permitted turn is a +/// completion, not a run that ran out of budget. +fn responder_turn(policy: &ResponderPolicy, delivered: u32, final_message: &str) -> NextTurn { + match responder::read(final_message) { + responder::Reading::NoQuestion => NextTurn::Done, + responder::Reading::Unanswerable => { + NextTurn::Stop(ConversationStopReason::ResponderCannotAnswer) + } + responder::Reading::Answer { text, origin } => { + if delivered >= policy.max_turns() { + return NextTurn::Stop(ConversationStopReason::MaxTurnsReached); + } + NextTurn::Deliver { + text, + origin: Some(origin), + } + } + } +} + +fn unmet_gate( + turn: &ScriptedTurn, + preceding_assistant: &str, +) -> anyhow::Result> { + if turn.deliver_when == DeliverWhen::Always { + return Ok(None); + } + if !preceding_assistant.contains('?') { + return Ok(Some(ConversationStopReason::AgentDidNotAsk)); + } + if let Some(pattern) = &turn.agent_response_matches { + let regex = Regex::new(pattern) + .with_context(|| format!("invalid agent_response_matches regex {pattern:?}"))?; + if !regex.is_match(preceding_assistant) { + return Ok(Some(ConversationStopReason::AgentResponseMismatch)); + } + } + Ok(None) +} + +#[cfg(test)] +mod tests { + use super::unmet_gate; + use crate::core::{ConversationStopReason, DeliverWhen, ScriptedTurn}; + + fn conditional(pattern: Option<&str>) -> ScriptedTurn { + ScriptedTurn { + prompt: "follow up".into(), + deliver_when: DeliverWhen::AgentAsks, + agent_response_matches: pattern.map(str::to_string), + } + } + + #[test] + fn agent_asks_requires_a_question_mark() { + assert_eq!( + unmet_gate(&conditional(None), "Please provide the timezone.").unwrap(), + Some(ConversationStopReason::AgentDidNotAsk) + ); + assert_eq!( + unmet_gate(&conditional(None), "Which timezone?").unwrap(), + None + ); + } + + #[test] + fn response_pattern_is_an_additional_compatibility_gate() { + assert_eq!( + unmet_gate(&conditional(Some("(?i)time ?zone")), "Which locale?").unwrap(), + Some(ConversationStopReason::AgentResponseMismatch) + ); + assert_eq!( + unmet_gate( + &conditional(Some("(?i)time ?zone")), + "Which timezone should I use?" + ) + .unwrap(), + None + ); + } +} diff --git a/src/cli/run/dispatch.rs b/src/cli/run/dispatch.rs index 5661745..870090b 100644 --- a/src/cli/run/dispatch.rs +++ b/src/cli/run/dispatch.rs @@ -15,8 +15,8 @@ use serde::{Deserialize, Serialize}; use crate::adapters::{CliManifestContext, adapter_for}; use crate::core::fs::artifact_path; use crate::core::{ - AvailableSkill, Eval, Harness, POSIX_TOOLING_REQUIREMENT, ScriptedTurn, SkillSource, - SourceRecord, + AvailableSkill, Eval, Harness, POSIX_TOOLING_REQUIREMENT, ResponderPolicy, ScriptedTurn, + SkillSource, SourceRecord, }; use super::RunError; @@ -63,6 +63,11 @@ pub struct DispatchTask { /// The skill under test this task stages, as the run resolved it. #[serde(default, skip_serializing_if = "Option::is_none")] pub skill_source: Option, + /// The policy that derives this task's follow-up turns, when the eval + /// declares one instead of scripting them. Recorded here so the plan names + /// how the conversation was driven, not just what it produced. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub responder: Option, #[serde(default, skip_serializing)] pub dispatch_prompt: String, } @@ -104,6 +109,8 @@ pub struct DispatchTaskOpts<'a> { pub codebase: Option<&'a SourceRecord>, /// The skill under test this task stages, if any. pub skill_source: Option<&'a SkillSource>, + /// The responder policy this eval declares, if any. + pub responder: Option<&'a ResponderPolicy>, } fn render_available_skills_block_for_harness( @@ -291,6 +298,7 @@ pub fn build_dispatch_task(opts: &DispatchTaskOpts) -> Result Vec { self.reports .iter() @@ -126,6 +130,24 @@ impl DispatchSummary { agent finished before the deadline", report.description )), + Ok(TaskOutcome::Stopped { + reason: Some(ConversationStopReason::ResponderCannotAnswer), + .. + }) => Some(format!( + "{} stopped: the responder could not answer the agent's question, so the run \ + ended mid-task. Read the last assistant message under its outputs before \ + trusting this data point.", + report.description + )), + Ok(TaskOutcome::Stopped { + reason: Some(ConversationStopReason::MaxTurnsReached), + .. + }) => Some(format!( + "{} stopped at the responder's max_turns bound with the agent still asking, \ + so the run ended mid-task. Raise max_turns or read the transcript before \ + trusting this data point.", + report.description + )), Ok(_) => None, }) .collect() diff --git a/src/cli/run/fixtures.rs b/src/cli/run/fixtures.rs index 3b2753e..24f31f7 100644 --- a/src/cli/run/fixtures.rs +++ b/src/cli/run/fixtures.rs @@ -200,6 +200,7 @@ mod tests { isolation: None, turns: None, codebase: None, + responder: None, } } diff --git a/src/cli/run/orchestrate/build.rs b/src/cli/run/orchestrate/build.rs index 43dd66f..0051d86 100644 --- a/src/cli/run/orchestrate/build.rs +++ b/src/cli/run/orchestrate/build.rs @@ -219,6 +219,7 @@ pub(super) fn write_dispatch( user_prompt: &ev.prompt, fixtures, turns: ev.turns.as_deref(), + responder: ev.responder.as_ref(), outputs_dir: &outputs_dir_str, cond_dir: &run_dir_str, bootstrap_content: staged.bootstrap_content.as_deref(), diff --git a/src/cli/run/orchestrate/mod.rs b/src/cli/run/orchestrate/mod.rs index 0a707f2..437c498 100644 --- a/src/cli/run/orchestrate/mod.rs +++ b/src/cli/run/orchestrate/mod.rs @@ -243,17 +243,32 @@ pub fn command_run(ctx: &RunContext, opts: &RunOptions) -> Result<(), RunError> // to the eval config actually selected for the run. let resolved = resolve::resolve_request(ctx, opts)?; - if resolved - .selected_evals - .iter() - .any(|eval| eval.turns.as_ref().is_some_and(|turns| !turns.is_empty())) - && !adapter_for(ctx.harness).has_conversation_resume() - { - return Err(RunError::msg(format!( - "--harness {} cannot run evals with scripted follow-up turns: its descriptor \ - declares no [conversation] native resume capability", - adapter_for(ctx.harness).label() - ))); + // Both ways of driving a conversation need the same capability: without one + // preserved session, a follow-up answers a fresh agent that never asked. + // Reported here rather than at dispatch time, so the gap surfaces before a + // workspace is built. + if !adapter_for(ctx.harness).has_conversation_resume() { + let label = adapter_for(ctx.harness).label(); + if resolved + .selected_evals + .iter() + .any(|eval| eval.turns.as_ref().is_some_and(|turns| !turns.is_empty())) + { + return Err(RunError::msg(format!( + "--harness {label} cannot run evals with scripted follow-up turns: its descriptor \ + declares no [conversation] native resume capability" + ))); + } + if resolved + .selected_evals + .iter() + .any(|eval| eval.responder.is_some()) + { + return Err(RunError::msg(format!( + "--harness {label} cannot run evals with a responder: its descriptor declares no \ + [conversation] native resume capability" + ))); + } } // The harness preflight provides supported enhancements automatically (the diff --git a/src/cli/run/util.rs b/src/cli/run/util.rs index 1e2dcf4..18cb216 100644 --- a/src/cli/run/util.rs +++ b/src/cli/run/util.rs @@ -476,6 +476,7 @@ mod tests { isolation: None, turns: None, codebase: None, + responder: None, } } diff --git a/src/core/types.rs b/src/core/types.rs index 92278cb..90657f8 100644 --- a/src/core/types.rs +++ b/src/core/types.rs @@ -121,6 +121,12 @@ pub struct Eval { /// serializes exactly as it did before the field existed. #[serde(default, skip_serializing_if = "Option::is_none")] pub codebase: Option, + /// Derives each follow-up from what the agent just said, instead of + /// scripting them. Mutually exclusive with [`Self::turns`]; absence of both + /// preserves one-shot dispatch. Appended last so an eval that declares none + /// serializes exactly as it did before the field existed. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub responder: Option, } /// One scripted user follow-up delivered after an assistant response. @@ -140,6 +146,31 @@ pub enum DeliverWhen { AgentAsks, } +/// How the runner answers the agent when an eval has no scripted script to +/// follow: the alternative to [`ScriptedTurn`], not a layer on top of it. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ResponderPolicy { + #[serde(rename = "type")] + pub kind: ResponderKind, + /// Maximum follow-up turns the responder may synthesize. The opening prompt + /// is not one of them, so this counts exactly what `delivered_followups` + /// counts. `None` takes [`DEFAULT_RESPONDER_MAX_TURNS`]. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub max_turns: Option, +} + +/// The bound a responder eval gets when it declares none: high enough that a +/// real clarifying exchange is not cut short, low enough that an agent stuck in +/// a question loop cannot burn a campaign. +pub const DEFAULT_RESPONDER_MAX_TURNS: u32 = 8; + +impl ResponderPolicy { + /// The bound this policy actually runs under. + pub fn max_turns(&self) -> u32 { + self.max_turns.unwrap_or(DEFAULT_RESPONDER_MAX_TURNS) + } +} + /// Legacy per-eval isolation hint. Every new run is task-scoped regardless. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] @@ -414,6 +445,13 @@ pub enum ConversationStatus { pub enum ConversationStopReason { AgentDidNotAsk, AgentResponseMismatch, + /// The agent asked something the responder could not answer mechanically. + /// This is the branch an LLM responder takes over; until then the run stops + /// here rather than inventing a reply. + ResponderCannotAnswer, + /// The agent was still asking when the responder's `max_turns` bound was + /// reached. A bounded conversation, not a failed one. + MaxTurnsReached, } /// One globally ordered event across every delivered conversation round. @@ -424,6 +462,12 @@ pub enum ConversationEvent { ordinal: u32, round: u32, text: String, + /// How a responder derived this turn. Absent on the seeded eval prompt + /// and on scripted turns, which are authored rather than derived — the + /// absence is what tells the two apart. Appended last so a scripted + /// conversation serializes exactly as it did before the field existed. + #[serde(default, skip_serializing_if = "Option::is_none")] + origin: Option, }, AssistantMessage { ordinal: u32, @@ -441,6 +485,53 @@ pub enum ConversationEvent { }, } +/// Where a synthesized user turn came from, recorded on the turn itself so a +/// reader can audit whether the responder distorted the run. Absent on the +/// seeded eval prompt and on scripted turns, which are authored, not derived. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct TurnOrigin { + pub responder: ResponderKind, + /// One entry per question the turn answered, in the order they were asked. + pub answers: Vec, +} + +/// Which responder produced a turn. `heuristic` is the only one that exists +/// today; the LLM answering agent adds its own so the two stay distinguishable +/// in a record. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ResponderKind { + Heuristic, +} + +/// How the responder answered one question, with the evidence it read. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ResponderAnswer { + /// The question line the options hung from, when the message carried one. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub question: Option, + /// The options as written, before markers were stripped. + pub options: Vec, + pub rule: ResponderRule, + /// Empty when the rule selected nothing. + pub chosen: Vec, +} + +/// The mechanical rule that picked one answer. Naming it on the turn is what +/// makes a synthesized conversation auditable rather than mysterious. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ResponderRule { + /// Exactly one option was marked as recommended, or every recommended + /// option of a checkbox list was taken. + RecommendedOption, + /// Exactly one choice was required and none was recommended, so the first + /// option won. + FirstOption, + /// A checkbox list required zero or more choices and recommended none. + NoSelection, +} + /// The result of grading one assertion. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct AssertionResult { @@ -570,6 +661,7 @@ mod tests { isolation: None, turns: None, codebase: None, + responder: None, }; let out = serde_json::to_value(&eval).unwrap(); assert!(out.get("files").is_none()); @@ -594,6 +686,7 @@ mod tests { isolation: Some(Isolation::Isolated), turns: None, codebase: None, + responder: None, }; let out = serde_json::to_value(&eval).unwrap(); assert_eq!( diff --git a/src/core/types/artifact_tests.rs b/src/core/types/artifact_tests.rs index a09d0c8..d245dae 100644 --- a/src/core/types/artifact_tests.rs +++ b/src/core/types/artifact_tests.rs @@ -189,3 +189,123 @@ fn run_record_roundtrips_a_stopped_multi_turn_conversation() { "assistant_message" ); } + +/// A responder-driven record has to satisfy three contracts at once: the Rust +/// types, `conversation.schema.json` (which the driver validates against before +/// writing), and `run-record.schema.json` (which ingest validates against +/// afterwards). Checking one alone lets the other two drift. +#[test] +fn a_responder_record_satisfies_both_schemas_and_roundtrips() { + use crate::validation::{SchemaName, validate_against_schema}; + + let conversation = json!({ + "status": "stopped", + "delivered_followups": 1, + "stop_reason": "responder_cannot_answer", + "stopped_before_followup": 2, + "events": [ + { "type": "user_message", "ordinal": 0, "round": 1, "text": "Add caching." }, + { "type": "assistant_message", "ordinal": 1, "round": 1, "text": "Which cache?\n\n- LRU (Recommended)\n- Redis\n" }, + { + "type": "user_message", + "ordinal": 2, + "round": 2, + "text": "LRU", + "origin": { + "responder": "heuristic", + "answers": [{ + "question": "Which cache?", + "options": ["LRU (Recommended)", "Redis"], + "rule": "recommended_option", + "chosen": ["LRU"] + }] + } + }, + { "type": "assistant_message", "ordinal": 3, "round": 2, "text": "What TTL suits you?" } + ] + }); + + let parsed: ConversationRecord = + validate_against_schema(SchemaName::Conversation, &conversation, "conversation.json") + .unwrap(); + assert_eq!( + parsed.stop_reason, + Some(ConversationStopReason::ResponderCannotAnswer) + ); + let ConversationEvent::UserMessage { origin, .. } = &parsed.events[2] else { + panic!("event 2 is the synthesized turn"); + }; + let origin = origin + .as_ref() + .expect("a synthesized turn names its origin"); + assert_eq!(origin.responder, ResponderKind::Heuristic); + assert_eq!(origin.answers[0].rule, ResponderRule::RecommendedOption); + + // The seeded prompt is authored, not derived, so it carries no origin at + // all — the field's absence is what distinguishes the two. + let ConversationEvent::UserMessage { origin, .. } = &parsed.events[0] else { + panic!("event 0 is the eval prompt"); + }; + assert!(origin.is_none()); + + let record = json!({ + "eval_id": "add-caching", + "condition": "with_skill", + "skill_path": null, + "prompt": "Add caching.", + "files": [], + "final_message": "What TTL suits you?", + "tool_invocations": [], + "total_tokens": null, + "duration_ms": null, + "conversation": conversation + }); + let record: RunRecord = + validate_against_schema(SchemaName::RunRecord, &record, "run.json").unwrap(); + + assert_eq!( + serde_json::to_value(&record).unwrap()["conversation"], + serde_json::to_value(record.conversation.clone().unwrap()).unwrap() + ); +} + +/// A conversation that outran its deadline is written by the driver and read +/// back by ingest, so the run-record schema has to accept the same shape +/// `conversation.schema.json` does — including a round-1 timeout, whose only +/// event is the seeded prompt. +#[test] +fn a_timed_out_conversation_satisfies_the_run_record_schema() { + use crate::validation::{SchemaName, validate_against_schema}; + + let conversation = json!({ + "status": "timed_out", + "delivered_followups": 0, + "timed_out_in_round": 1, + "events": [ + { "type": "user_message", "ordinal": 0, "round": 1, "text": "Add caching." } + ] + }); + let _: ConversationRecord = + validate_against_schema(SchemaName::Conversation, &conversation, "conversation.json") + .unwrap(); + + let record = json!({ + "eval_id": "add-caching", + "condition": "with_skill", + "skill_path": null, + "prompt": "Add caching.", + "files": [], + "final_message": "", + "tool_invocations": [], + "total_tokens": null, + "duration_ms": null, + "conversation": conversation + }); + + let record: RunRecord = + validate_against_schema(SchemaName::RunRecord, &record, "run.json").unwrap(); + assert_eq!( + record.conversation.unwrap().status, + ConversationStatus::TimedOut + ); +} diff --git a/src/pipeline/grade/transcript_check.rs b/src/pipeline/grade/transcript_check.rs index dd060aa..c4a8fe9 100644 --- a/src/pipeline/grade/transcript_check.rs +++ b/src/pipeline/grade/transcript_check.rs @@ -319,6 +319,7 @@ mod tests { ordinal: 0, round: 1, text: "Fix it".into(), + origin: None, }, ConversationEvent::AssistantMessage { ordinal: 1, @@ -329,6 +330,7 @@ mod tests { ordinal: 2, round: 2, text: "US timezones".into(), + origin: None, }, ConversationEvent::ToolInvocation { ordinal: 3, diff --git a/src/pipeline/record_runs.rs b/src/pipeline/record_runs.rs index 1abf49a..349f49e 100644 --- a/src/pipeline/record_runs.rs +++ b/src/pipeline/record_runs.rs @@ -69,10 +69,14 @@ struct DispatchTask { #[serde(default)] conversation_path: Option, /// Present only for a scripted task. Every task carries a - /// `conversation_path`, so this is what tells a task whose rounds are - /// unknown-without-the-artifact from a one-shot task. + /// `conversation_path`, so this and `responder` are what tell a task whose + /// rounds are unknown-without-the-artifact from a one-shot task. #[serde(default)] turns: Option, + /// Present only for a responder-driven task — the other way a task's rounds + /// become unknown without its completion artifact. + #[serde(default)] + responder: Option, /// Group this task belongs to; absent for a single-group run. Carried so the /// session-surface report can be joined back to the comparison cells a /// shadow finding names. @@ -175,7 +179,7 @@ impl RecordRunsResult { )) } - /// Warn when a scripted task never produced its runner-owned completion + /// Warn when a multi-turn task never produced its runner-owned completion /// artifact. Raw per-turn transcripts are intentionally not ingested /// without it because the driver may have failed between turns. pub fn incomplete_conversation_warning(&self) -> Option { @@ -185,7 +189,7 @@ impl RecordRunsResult { } let plural = if n == 1 { "" } else { "s" }; Some(format!( - "⚠ {n} scripted conversation{plural} skipped — conversation.json is missing, so \ + "⚠ {n} multi-turn conversation{plural} skipped — conversation.json is missing, so \ eval-magic cannot distinguish a completed/stopped scenario from an interrupted \ dispatch. Re-run `eval-magic dispatch` — it retries exactly the tasks with no \ completion artifact." @@ -221,11 +225,11 @@ pub fn record_runs( let mut surface_tasks: Vec = Vec::new(); for task in &tasks { let conversation = conversation::for_task(task)?; - // Keyed on `turns`, not on `conversation_path`: every task declares a - // conversation artifact, so its presence does not distinguish a scripted - // one. A scripted task without the artifact is genuinely incomplete — - // which rounds ran is unknown. - if task.turns.is_some() && conversation.is_none() { + // Keyed on what drives the turns, not on `conversation_path`: every task + // declares a conversation artifact, so its presence does not distinguish + // a multi-turn one. A scripted or responder-driven task without the + // artifact is genuinely incomplete — which rounds ran is unknown. + if (task.turns.is_some() || task.responder.is_some()) && conversation.is_none() { result.skipped_incomplete_conversation += 1; continue; } diff --git a/src/pipeline/record_runs/tests/conversation.rs b/src/pipeline/record_runs/tests/conversation.rs index 921ca07..2a19c75 100644 --- a/src/pipeline/record_runs/tests/conversation.rs +++ b/src/pipeline/record_runs/tests/conversation.rs @@ -1,4 +1,5 @@ use super::*; +use crate::core::ConversationStatus; #[test] fn assembles_multi_turn_run_using_last_cumulative_codex_tokens_and_summed_duration() { @@ -268,3 +269,106 @@ fn does_not_record_partial_timing_when_a_conversation_round_transcript_is_missin assert!(paths[0].run_record_path.exists()); assert!(!paths[0].timing_path.exists()); } + +/// A task whose completion artifact is missing is only "incomplete" if it was +/// meant to have rounds. That was keyed on `turns`, which a responder-driven +/// task does not declare — so without this it would be recorded from turn 1 +/// alone, as though the conversation had never been interrupted. +#[test] +fn a_responder_task_without_its_completion_artifact_is_skipped_as_incomplete() { + let root = TempDir::new().unwrap(); + let iter = dirs(&root); + let paths = write_iteration( + &iter, + &[FixtureTask { + eval_id: "clarify", + condition: "with_skill", + final_message: Some("Which cache?"), + }], + ); + let round_dir = paths[0].outputs_dir.join("turn-1"); + fs::create_dir_all(&round_dir).unwrap(); + write_claude_events(&round_dir, "Which cache?"); + + let dispatch_path = iter.join("dispatch.json"); + let mut dispatch: Value = + serde_json::from_str(&fs::read_to_string(&dispatch_path).unwrap()).unwrap(); + dispatch["tasks"][0]["responder"] = json!({ "type": "heuristic" }); + dispatch["tasks"][0]["conversation_path"] = json!( + iter.join("eval-clarify") + .join("with_skill") + .join("conversation.json") + .to_string_lossy() + ); + fs::write( + &dispatch_path, + serde_json::to_string_pretty(&dispatch).unwrap(), + ) + .unwrap(); + + let result = record_runs(&iter, 1, Harness::resolve("claude-code").unwrap(), false).unwrap(); + + assert_eq!(result.skipped_incomplete_conversation, 1); + assert_eq!(result.recorded, 0); + assert!(!paths[0].run_record_path.exists()); +} + +/// A task killed at its deadline still has rounds worth recording. Ingest +/// clones the whole conversation into `run.json` and validates it there, so the +/// run-record schema has to accept `timed_out` exactly as the conversation +/// schema does — otherwise a single hung task fails the whole ingest. +#[test] +fn records_a_run_whose_conversation_timed_out_in_a_later_round() { + let root = TempDir::new().unwrap(); + let iter = dirs(&root); + let paths = write_iteration( + &iter, + &[FixtureTask { + eval_id: "clarify", + condition: "with_skill", + final_message: None, + }], + ); + let conversation_path = iter + .join("eval-clarify") + .join("with_skill") + .join("conversation.json"); + fs::write( + &conversation_path, + serde_json::to_string_pretty(&json!({ + "status": "timed_out", + "delivered_followups": 1, + "timed_out_in_round": 2, + "events": [ + {"type": "user_message", "ordinal": 0, "round": 1, "text": "Fix it."}, + {"type": "assistant_message", "ordinal": 1, "round": 1, "text": "Which timezone?"}, + {"type": "user_message", "ordinal": 2, "round": 2, "text": "US timezones."} + ] + })) + .unwrap(), + ) + .unwrap(); + let round_dir = paths[0].outputs_dir.join("turn-1"); + fs::create_dir_all(&round_dir).unwrap(); + write_claude_events(&round_dir, "Which timezone?"); + + let dispatch_path = iter.join("dispatch.json"); + let mut dispatch: Value = + serde_json::from_str(&fs::read_to_string(&dispatch_path).unwrap()).unwrap(); + dispatch["tasks"][0]["responder"] = json!({ "type": "heuristic" }); + dispatch["tasks"][0]["conversation_path"] = + json!(conversation_path.to_string_lossy().to_string()); + fs::write( + &dispatch_path, + serde_json::to_string_pretty(&dispatch).unwrap(), + ) + .unwrap(); + + let result = record_runs(&iter, 1, Harness::resolve("claude-code").unwrap(), false).unwrap(); + + assert_eq!(result.recorded, 1); + let run = read_run(&iter, "clarify", "with_skill"); + let conversation = run.conversation.unwrap(); + assert_eq!(conversation.status, ConversationStatus::TimedOut); + assert_eq!(conversation.timed_out_in_round, Some(2)); +} diff --git a/src/validation/evals.rs b/src/validation/evals.rs index 16742f9..ca22837 100644 --- a/src/validation/evals.rs +++ b/src/validation/evals.rs @@ -16,6 +16,7 @@ use crate::validation::schema::{SchemaName, validate_against_schema}; /// returning the typed config on success. pub fn validate_evals_config(config: &Value, source: &str) -> Result { validate_codebase_declarations(config, source)?; + validate_turn_source_declarations(config, source)?; let validated: EvalsConfig = validate_against_schema(SchemaName::Evals, config, source)?; let mut seen = HashSet::new(); @@ -63,12 +64,16 @@ pub fn validate_evals_config(config: &Value, source: &str) -> Result Result<(), Va Ok(()) } +/// Reject an eval that declares both ways of driving a conversation. Checked +/// before the schema for the same reason the codebase rules are: the schema +/// states it as a bare `not`, which reports the whole eval as disallowed and +/// never names the two fields that clash. +fn validate_turn_source_declarations(config: &Value, source: &str) -> Result<(), ValidationError> { + let evals = config.get("evals").and_then(Value::as_array); + for (index, eval) in evals.into_iter().flatten().enumerate() { + if eval.get("turns").is_none() || eval.get("responder").is_none() { + continue; + } + let id = eval + .get("id") + .and_then(Value::as_str) + .map_or_else(|| format!("evals[{index}]"), str::to_string); + return Err(ValidationError::InvalidConfig { + path: source.to_string(), + message: format!( + "eval '{id}': declares both 'turns' and 'responder'; a conversation is either \ + scripted or derived, not both" + ), + }); + } + Ok(()) +} + fn validate_codebase(source: &str, label: &str, value: &Value) -> Result<(), ValidationError> { // A non-object is a plain type error the schema words perfectly well. let Some(fields) = value.as_object() else { @@ -443,6 +473,63 @@ mod tests { } } + #[test] + fn accepts_an_eval_declaring_only_a_responder() { + let mut config = base(); + config["evals"][0]["responder"] = json!({ "type": "heuristic", "max_turns": 3 }); + + let parsed = validate_evals_config(&config, "evals.json").unwrap(); + let responder = parsed.evals[0].responder.as_ref().unwrap(); + assert_eq!(responder.kind, crate::core::ResponderKind::Heuristic); + assert_eq!(responder.max_turns, Some(3)); + } + + /// The two ways to drive a conversation are alternatives, not layers: a + /// scripted array says exactly what the user says, a responder derives it. + #[test] + fn rejects_responder_and_turns_together() { + let mut config = base(); + config["evals"][0]["responder"] = json!({ "type": "heuristic" }); + config["evals"][0]["turns"] = json!([{ "prompt": "go on", "deliver_when": "always" }]); + + let error = validate_evals_config(&config, "evals.json") + .unwrap_err() + .to_string(); + + assert!(error.contains("responder"), "{error}"); + assert!(error.contains("turns"), "{error}"); + } + + /// A bound of zero would dispatch turn 1 and refuse to answer anything, + /// which is a one-shot eval written the long way round. + #[test] + fn rejects_a_zero_max_turns() { + let mut config = base(); + config["evals"][0]["responder"] = json!({ "type": "heuristic", "max_turns": 0 }); + + let error = validate_evals_config(&config, "evals.json") + .unwrap_err() + .to_string(); + + assert!(error.contains("max_turns"), "{error}"); + } + + /// The check needs a multi-turn conversation to read, and a responder + /// produces one just as a scripted array does. + #[test] + fn assistant_message_matches_accepts_a_responder_eval() { + let mut config = base(); + config["evals"][0]["responder"] = json!({ "type": "heuristic" }); + config["evals"][0]["assertions"] = json!([{ + "id": "asked", + "type": "transcript_check", + "check": "assistant_message_matches", + "pattern": "timezone" + }]); + + validate_evals_config(&config, "evals.json").unwrap(); + } + #[test] fn rejects_an_empty_scripted_turns_array() { let mut config = base(); diff --git a/tests/golden/claude-code/runbook.golden.md b/tests/golden/claude-code/runbook.golden.md index 68dd4f0..da4b5e3 100644 --- a/tests/golden/claude-code/runbook.golden.md +++ b/tests/golden/claude-code/runbook.golden.md @@ -21,7 +21,9 @@ each task's `conversation.json`. A task that already has one is skipped, so reru command retries only what did not finish. A task that exceeds `--timeout` is recorded as timed out rather than left to stall the campaign, and a task that fails is recorded and named while the rest of the batch continues. A conversation that stops at a scripted gate is valid eval data, not a -failure. +failure. A conversation the responder stopped — because it could not answer the agent's question, +or because it hit `max_turns` — is recorded too, but it ended with the task unfinished; `dispatch` +warns about each one by name, and those runs are weaker evidence than a completed one. ``` eval-magic ingest --skill-dir /tmp/skills --skill widget-skill --iteration 2 --harness claude-code diff --git a/tests/golden/cline/runbook.golden.md b/tests/golden/cline/runbook.golden.md index 36364a8..fb6e3fe 100644 --- a/tests/golden/cline/runbook.golden.md +++ b/tests/golden/cline/runbook.golden.md @@ -21,7 +21,9 @@ each task's `conversation.json`. A task that already has one is skipped, so reru command retries only what did not finish. A task that exceeds `--timeout` is recorded as timed out rather than left to stall the campaign, and a task that fails is recorded and named while the rest of the batch continues. A conversation that stops at a scripted gate is valid eval data, not a -failure. +failure. A conversation the responder stopped — because it could not answer the agent's question, +or because it hit `max_turns` — is recorded too, but it ended with the task unfinished; `dispatch` +warns about each one by name, and those runs are weaker evidence than a completed one. ``` eval-magic ingest --skill-dir /tmp/skills --skill widget-skill --iteration 2 --harness cline diff --git a/tests/golden/codex/runbook.golden.md b/tests/golden/codex/runbook.golden.md index 6515b77..0908bc5 100644 --- a/tests/golden/codex/runbook.golden.md +++ b/tests/golden/codex/runbook.golden.md @@ -21,7 +21,9 @@ each task's `conversation.json`. A task that already has one is skipped, so reru command retries only what did not finish. A task that exceeds `--timeout` is recorded as timed out rather than left to stall the campaign, and a task that fails is recorded and named while the rest of the batch continues. A conversation that stops at a scripted gate is valid eval data, not a -failure. +failure. A conversation the responder stopped — because it could not answer the agent's question, +or because it hit `max_turns` — is recorded too, but it ended with the task unfinished; `dispatch` +warns about each one by name, and those runs are weaker evidence than a completed one. ``` eval-magic ingest --skill-dir /tmp/skills --skill widget-skill --iteration 2 --harness codex diff --git a/tests/golden/opencode/runbook.golden.md b/tests/golden/opencode/runbook.golden.md index 0e40956..4683741 100644 --- a/tests/golden/opencode/runbook.golden.md +++ b/tests/golden/opencode/runbook.golden.md @@ -21,7 +21,9 @@ each task's `conversation.json`. A task that already has one is skipped, so reru command retries only what did not finish. A task that exceeds `--timeout` is recorded as timed out rather than left to stall the campaign, and a task that fails is recorded and named while the rest of the batch continues. A conversation that stops at a scripted gate is valid eval data, not a -failure. +failure. A conversation the responder stopped — because it could not answer the agent's question, +or because it hit `max_turns` — is recorded too, but it ended with the task unfinished; `dispatch` +warns about each one by name, and those runs are weaker evidence than a completed one. ``` eval-magic ingest --skill-dir /tmp/skills --skill widget-skill --iteration 2 --harness opencode diff --git a/tests/run/conversation.rs b/tests/run/conversation.rs index 527145d..0d3dd2d 100644 --- a/tests/run/conversation.rs +++ b/tests/run/conversation.rs @@ -8,6 +8,7 @@ use std::fs; use std::path::Path; mod dispatch; +mod responder; #[test] fn multi_turn_eval_dispatch_records_followups_and_conversation_artifact_path() { diff --git a/tests/run/conversation/responder.rs b/tests/run/conversation/responder.rs new file mode 100644 index 0000000..2d9f3ac --- /dev/null +++ b/tests/run/conversation/responder.rs @@ -0,0 +1,331 @@ +//! Conversations driven by the heuristic responder rather than a script. +//! +//! Each test swaps the frozen descriptor's dispatch templates for a POSIX stub +//! that answers differently per round, the way every driver test here does. + +use super::{dispatch_one, stub_exec_template}; +use crate::helpers::*; +use predicates::prelude::PredicateBooleanExt; +use predicates::str::contains; +use std::fs; +use std::path::{Path, PathBuf}; + +/// An evals config whose single eval is driven by the responder. +fn responder_evals(max_turns: Option) -> String { + let bound = match max_turns { + Some(turns) => format!(", \"max_turns\": {turns}"), + None => String::new(), + }; + format!( + r#"{{ + "skill_name": "mr-review", + "evals": [{{ + "id": "caching", + "prompt": "Requests to the pricing API are slow. Add caching.", + "expected_output": "caching is in place", + "responder": {{ "type": "heuristic"{bound} }} + }}] + }}"# + ) +} + +/// Prepare a responder-driven iteration against the codex harness. +fn prepare(skill_dir: &Path, cwd: &Path) { + skill_eval() + .current_dir(cwd) + .args(["run", "--skill-dir"]) + .arg(skill_dir) + .args([ + "--skill", + "mr-review", + "--mode", + "new-skill", + "--harness", + "codex", + "--no-guard", + ]) + .assert() + .success(); +} + +/// A stub emitting `$2` as its agent message for every round, plus the session +/// id and usage events a transcript needs to parse. Written as a POSIX script +/// and invoked through `sh`, because that is the shape of a real exec template. +fn stub(dir: &Path, name: &str) -> PathBuf { + let script = dir.join(name); + fs::write( + &script, + r#"#!/bin/sh +outputs=$1 +message=$2 +printf '%s\n' '{"type":"thread.started","thread_id":"session-1"}' > "$outputs/codex-events.jsonl" +printf '%s' '{"type":"item.completed","item":{"id":"m1","type":"agent_message","text":"' >> "$outputs/codex-events.jsonl" +printf '%s' "$message" >> "$outputs/codex-events.jsonl" +printf '%s\n' '"}}' >> "$outputs/codex-events.jsonl" +printf '%s\n' '{"type":"turn.completed","usage":{"input_tokens":2,"output_tokens":3}}' >> "$outputs/codex-events.jsonl" +"#, + ) + .unwrap(); + script +} + +/// Wire an initial message and a resume message into the frozen descriptor. +fn stub_rounds(tmp: &Path, cwd: &Path, initial: &str, resumed: &str) { + let script = stub(tmp, "fake-codex.sh"); + let quoted = script.to_string_lossy().to_string(); + stub_exec_template( + cwd, + &format!("sh \"{quoted}\" \"{initial}\" "), + ); + let dispatch_path = iteration_dir(cwd).join("dispatch.json"); + let mut dispatch = read_json(&dispatch_path); + dispatch["harness_descriptor"]["conversation"]["resume_exec_template"] = + serde_json::json!(format!( + "sh \"{quoted}\" \"{resumed}\" {{session_arg}} {{prompt_arg}}" + )); + fs::write( + &dispatch_path, + format!("{}\n", serde_json::to_string_pretty(&dispatch).unwrap()), + ) + .unwrap(); +} + +/// The acceptance criterion from the ticket: a responder eval with no scripted +/// turns runs to completion, and the recommended option is both selected and +/// recorded as the reason it was selected. +#[test] +fn a_responder_eval_answers_a_recommended_option_and_completes() { + let tmp = tempfile::TempDir::new().unwrap(); + let (skill_dir, cwd) = setup(tmp.path(), &responder_evals(None)); + prepare(&skill_dir, &cwd); + stub_rounds( + tmp.path(), + &cwd, + "Which cache should I use?\\n\\n- In-process LRU (Recommended)\\n- Redis\\n", + "Caching is in place and the endpoint is under 40ms.", + ); + + dispatch_one(&skill_dir, &cwd, "codex", 0, false) + .assert() + .success(); + + let dispatch = read_json(&iteration_dir(&cwd).join("dispatch.json")); + let task = &dispatch["tasks"][0]; + let conversation = read_json(Path::new(task["conversation_path"].as_str().unwrap())); + + assert_eq!(conversation["status"], "completed", "{conversation}"); + assert_eq!(conversation["delivered_followups"], 1); + let synthesized = conversation["events"] + .as_array() + .unwrap() + .iter() + .filter(|event| event["type"] == "user_message") + .nth(1) + .expect("the responder delivered a second user turn"); + assert_eq!(synthesized["text"], "In-process LRU"); + assert_eq!(synthesized["round"], 2); + assert_eq!(synthesized["origin"]["responder"], "heuristic"); + assert_eq!( + synthesized["origin"]["answers"][0]["rule"], + "recommended_option" + ); + assert_eq!( + synthesized["origin"]["answers"][0]["question"], + "Which cache should I use?" + ); +} + +/// The opening prompt is authored, not derived, so it carries no origin. The +/// absence is what lets a reader tell a real user turn from a synthesized one. +#[test] +fn the_opening_prompt_carries_no_responder_origin() { + let tmp = tempfile::TempDir::new().unwrap(); + let (skill_dir, cwd) = setup(tmp.path(), &responder_evals(None)); + prepare(&skill_dir, &cwd); + stub_rounds(tmp.path(), &cwd, "Done, caching is in place.", "unused"); + + dispatch_one(&skill_dir, &cwd, "codex", 0, false) + .assert() + .success(); + + let dispatch = read_json(&iteration_dir(&cwd).join("dispatch.json")); + let conversation = read_json(Path::new( + dispatch["tasks"][0]["conversation_path"].as_str().unwrap(), + )); + assert_eq!(conversation["status"], "completed"); + assert_eq!(conversation["delivered_followups"], 0); + assert!( + conversation["events"][0]["origin"].is_null(), + "the eval prompt is authored: {conversation}" + ); +} + +/// Reaching the bound is a recorded outcome, not a failure: the command still +/// exits zero and the artifact says exactly why the conversation ended. +#[test] +fn a_responder_run_that_reaches_max_turns_is_recorded_not_failed() { + let tmp = tempfile::TempDir::new().unwrap(); + let (skill_dir, cwd) = setup(tmp.path(), &responder_evals(Some(2))); + prepare(&skill_dir, &cwd); + let asking = "Which cache should I use?\\n\\n- In-process LRU (Recommended)\\n- Redis\\n"; + stub_rounds(tmp.path(), &cwd, asking, asking); + + dispatch_one(&skill_dir, &cwd, "codex", 0, false) + .assert() + .success(); + + let dispatch = read_json(&iteration_dir(&cwd).join("dispatch.json")); + let task = &dispatch["tasks"][0]; + let conversation = read_json(Path::new(task["conversation_path"].as_str().unwrap())); + + assert_eq!(conversation["status"], "stopped", "{conversation}"); + assert_eq!(conversation["stop_reason"], "max_turns_reached"); + assert_eq!(conversation["delivered_followups"], 2); + assert_eq!(conversation["stopped_before_followup"], 3); + assert!( + !Path::new(task["outputs_dir"].as_str().unwrap()) + .join("turn-4") + .exists(), + "the bound is the last round dispatched" + ); +} + +/// The greppable branch the LLM responder will take over. It stops the run +/// rather than inventing an answer, and says so loudly — a conversation that +/// ended mid-task must not be mistaken for a clean data point. +#[test] +fn a_question_the_responder_cannot_classify_stops_the_run() { + let tmp = tempfile::TempDir::new().unwrap(); + let (skill_dir, cwd) = setup(tmp.path(), &responder_evals(None)); + prepare(&skill_dir, &cwd); + stub_rounds( + tmp.path(), + &cwd, + "What should happen to rows with a null created_at?", + "unused", + ); + + dispatch_one(&skill_dir, &cwd, "codex", 0, false) + .assert() + .success() + .stderr(contains("could not answer")); + + let dispatch = read_json(&iteration_dir(&cwd).join("dispatch.json")); + let task = &dispatch["tasks"][0]; + let conversation = read_json(Path::new(task["conversation_path"].as_str().unwrap())); + + assert_eq!(conversation["status"], "stopped", "{conversation}"); + assert_eq!(conversation["stop_reason"], "responder_cannot_answer"); + assert_eq!(conversation["delivered_followups"], 0); + assert_eq!(conversation["stopped_before_followup"], 1); + assert!( + !Path::new(task["outputs_dir"].as_str().unwrap()) + .join("turn-2") + .exists(), + "an unanswerable question delivers no turn" + ); +} + +/// A responder needs the same native-resume capability a scripted array does: +/// starting a fresh session each round would make the answer meaningless. `run` +/// has to say so at prep time, not leave it to fail mid-dispatch. +#[test] +fn a_responder_eval_is_rejected_on_a_harness_without_native_resume() { + let tmp = tempfile::TempDir::new().unwrap(); + let (skill_dir, cwd) = setup(tmp.path(), &responder_evals(None)); + let descriptor_dir = cwd.join(".eval-magic").join("harnesses"); + fs::create_dir_all(&descriptor_dir).unwrap(); + fs::write( + descriptor_dir.join("cool.toml"), + r#"label = "cool-custom-harness" + +[dispatch] +exec_template = "cool-cli run --cd {model_arg} > /final-message.md" +"#, + ) + .unwrap(); + + skill_eval() + .current_dir(&cwd) + .args(["run", "--skill-dir"]) + .arg(&skill_dir) + .args([ + "--skill", + "mr-review", + "--mode", + "new-skill", + "--harness", + "cool-custom-harness", + ]) + .assert() + .failure() + .stderr( + contains("responder") + .and(contains("cool-custom-harness")) + .and(contains("conversation")), + ); +} + +/// Mode B parity: a revision run drives a responder conversation the same way a +/// new-skill run does, against the snapshot/promote path. +#[test] +fn revision_mode_runs_a_responder_eval() { + let tmp = tempfile::TempDir::new().unwrap(); + let (skill_dir, cwd) = setup(tmp.path(), &responder_evals(None)); + + skill_eval() + .current_dir(&cwd) + .args(["snapshot", "--skill-dir"]) + .arg(&skill_dir) + .args(["--skill", "mr-review", "--label", "baseline"]) + .assert() + .success(); + skill_eval() + .current_dir(&cwd) + .args(["run", "--skill-dir"]) + .arg(&skill_dir) + .args([ + "--skill", + "mr-review", + "--mode", + "revision", + "--harness", + "codex", + "--no-guard", + ]) + .assert() + .success(); + stub_rounds( + tmp.path(), + &cwd, + "Which cache should I use?\\n\\n- In-process LRU (Recommended)\\n- Redis\\n", + "Caching is in place.", + ); + + skill_eval() + .current_dir(&cwd) + .args(["dispatch", "--skill-dir"]) + .arg(&skill_dir) + .args([ + "--skill", + "mr-review", + "--iteration", + "1", + "--harness", + "codex", + ]) + .assert() + .success() + .stdout(contains("2 completed")); + + let dispatch = read_json(&iteration_dir(&cwd).join("dispatch.json")); + for task in dispatch["tasks"].as_array().unwrap() { + let conversation = read_json(Path::new(task["conversation_path"].as_str().unwrap())); + assert_eq!(conversation["status"], "completed", "{conversation}"); + assert_eq!(conversation["delivered_followups"], 1); + } + assert_eq!( + dispatch["tasks"][0]["responder"]["type"], "heuristic", + "the plan records how the conversation was driven" + ); +}