diff --git a/CODED.md b/CODED.md index 6000f65..8148e9d 100644 --- a/CODED.md +++ b/CODED.md @@ -26,17 +26,22 @@ make # run before considering a change done go test ./internal/foo/... # target just the changed package while iterating ``` -`make` regenerates `prompts/agent-run.txt` alongside everything else. Review -that diff whenever a change touches the system prompt, a tool's -description/schema, or the agent loop's reminder/formatting logic — it's +`make` regenerates every file under `prompts/` alongside everything else — +one file per shipped agent definition (`agent-run.txt` for `main`, +`explorer-run.txt` for `explorer`, and so on as more definitions ship). +Review that diff whenever a change touches a system prompt, a tool's +description/schema, or a protocol's reminder/formatting logic — an +unexpected diff in a dump you didn't mean to touch usually means a shared +piece (a tool description, `agent.DefaultIdentity`, `internal/agent/protocol`'s +generated text) changed underneath more than one definition. Every dump is meant to be committed alongside the code that produced it. ## Layout - `cmd/coded` — CLI entry point, flag parsing, UI mode selection. - `cmd/promptdump` — dev tool that drives a scripted fake conversation - through the real agent loop/tools/permissions to produce - `prompts/agent-run.txt`. + through the real agent loop/tools/permissions, once per shipped agent + definition, to produce `prompts/*.txt`. - `internal/agent` — the turn-loop primitives (UI-agnostic): conversation state, ``/`` wrapping, provider-stream accumulation into a single assistant Message, tool dispatch. No loop of @@ -72,8 +77,8 @@ meant to be committed alongside the code that produced it. - `internal/oneshot` — plain-text renderer for `-p` / non-interactive mode. - `internal/render` — turns `agent.Event` error/retry payloads into short, user-facing text shared by both renderers. -- `adr/` — architecture decision records; `prompts/agent-run.txt` — the - committed prompt dump. +- `adr/` — architecture decision records; `prompts/` — one committed prompt + dump per shipped agent definition (see `cmd/promptdump`). ## Conventions diff --git a/PLAN.md b/PLAN.md index 870f8a3..1256402 100644 --- a/PLAN.md +++ b/PLAN.md @@ -27,8 +27,8 @@ consequences. ### Agents -- **Agent** — a named, configured participant: an identity, a tool subset, a model, a thinking scheme, a permission posture. v0.1 has exactly one, unnamed and implicit; from v0.2 the main agent is just the default one. *(v0.2 for the plural)* -- **Agent definition** — the file that declares an agent, in Claude Code's markdown-with-frontmatter format, with coded's extensions namespaced under a `coded:` key so the file stays valid for both. Narrowing-only on anything security-relevant: it can subtract tools and tighten permission mode, never widen either. *(v0.2)* +- **Agent** — a named, configured participant: an identity, a tool subset, a model, a thinking scheme. v0.1 has exactly one, unnamed and implicit; from v0.2 the main agent is just the default one. *(v0.2 for the plural)* +- **Agent definition** — the file that declares an agent, in Claude Code's markdown-with-frontmatter format, with coded's extensions namespaced under a `coded:` key so the file stays valid for both. It declares no permission posture of its own: the mode is the session's, shared by the whole tree. *(v0.2)* - **Sub-agent** — an agent invoked by another agent through the `Spawn` tool, running to completion in an isolated conversation and returning only its final answer. Its entire life is one turn: one input — which is its goal — its own iterations, one final answer, one outcome. Fresh context is the point: the parent gets the conclusion, not the tool traffic. *(v0.2)* - **Thinking** — reasoning the harness can see, require, check, and reuse: written in-band as tagged sections of an ordinary response, structured by a scheme, enforced on every turn. The invariant is about dependence, not prohibition — a provider's own extended thinking may pass through and be rendered, but **no scheme's correctness may rely on it, and every scheme must hold on a model that has none.** Off by default, because with it on the model does its real reasoning in a block that cannot be inspected, composed, or fed to a later pass, leaving the half that can as a summary of the half that mattered. That is the whole reason Pass and Protocol exist rather than a `thinking: high` knob. - **Thinking scheme** — how one agent's turn is structured: an ordered list of passes. A single-pass scheme is the default and is exactly today's behavior; self-ask is two. This is the concept Claude Code's agent format cannot express, and the reason coded's format extends it. *(v0.2)* @@ -44,7 +44,7 @@ consequences. - **Step** — one unit of a multi-step tool call (a command in a Bash batch), permissioned, executed, and streamed independently. - **Subject** — the structured description of a call that rules match against: a path's segments, a command's words. The tool decides how its own arguments decompose and reports **only facts about that shape** — never a judgment about which generalizations are safe to grant. That judgment is policy and lives entirely in the permission engine. - **Rule** — an allow or deny decision scoped to a tool and optionally to its arguments (`Bash(git diff*)`). Read from settings, hot-reloaded on change, and extendable at the approval prompt. -- **Permission mode** — the session's default posture when no rule matches: `default`, `accept-edits`, `bypass`, `plan`. +- **Permission mode** — the session's default posture when no rule matches: `default`, `accept-edits`, `bypass`, `plan`. Exactly one is live at a time, set by the harness and shared by every agent in the tree. - **Project root** — a boundary, not a preference: a call reaching outside it asks even under `bypass`, and is therefore denied under `-p`. ### Architecture @@ -195,8 +195,8 @@ Goals: - [ ] Markdown-with-frontmatter agent definitions loaded from `.coded/agents/*.md` (project), `~/.coded/agents/*.md` (global), and `.claude/agents/*.md` (compatibility), most-specific wins on name collision. - [ ] Claude Code's field set works unchanged — `name`, `description`, `tools`, `model` — so an existing `.claude/agents` directory is usable as-is; every coded extension lives under a single `coded:` key so a definition coded understands stays valid for Claude Code, and unknown keys are ignored rather than rejected. -- [ ] `coded:` extensions: `scheme` (below), `permission_mode` (narrowing only), `max_iterations`, `budget`, `context` (which project docs and files the sub-agent is seeded with), and `checklists`. -- [ ] Definitions are **narrowing-only** on anything security-relevant: an agent's `tools` list can subtract from the registry but never add to it, and its `permission_mode` can only be stricter than the session's. An agent file is checked into a repo and arrives with the code — it must not be able to widen what the user approved. +- [ ] `coded:` extensions: `scheme` (below), `max_iterations`, `budget`, `context` (which project docs and files the sub-agent is seeded with), and `checklists`. +- [ ] Definitions declare **no permission mode**: one mode, set by the harness, applies to every agent in the tree, and nothing switches it around a spawn. An agent's `tools` list selects from what the harness registered and cannot introduce a tool the binary doesn't have. - [ ] Built-in definitions shipped in-binary (`main`, `reviewer`, `explorer`), overridable by a same-named file on disk, so the feature has real users on day one and the format is exercised by our own agents. - [ ] `coded agents list` / `/agents` shows what's loaded, from where, and what each one is allowed to do. @@ -266,7 +266,7 @@ tool is that same pattern with a different payload. - [ ] A built-in `Spawn` tool that invokes a named agent with a prompt, runs it to completion in an isolated conversation, and returns its final answer as the tool result. Named for what it does rather than after Claude Code's `Task`: an invocation is exactly one turn of a fresh agent, and "task" would name a rung of the work ladder we already have a word for. Compatibility is about the agent-definition file format, not tool names. Fresh context is the point: the parent gets the conclusion, not the sub-agent's tool traffic. - [ ] `agent.Event` gains an agent path so nested activity is attributable, plus `EventAgentStarted`/`EventAgentComplete`. The TUI renders a sub-agent as a collapsible group; `-p` renders it indented. This is an ADR: the event stream stops being flat. -- [ ] One permission engine for the whole tree. A sub-agent never gets its own rule set, prompts serialize globally (one question on screen at a time, whoever asked), and an "always" answer applies session-wide as it does today. +- [ ] One permission engine for the whole tree. A sub-agent never gets its own rule set or its own mode, prompts serialize globally (one question on screen at a time, whoever asked), and an "always" answer applies session-wide as it does today. - [ ] Sub-agents run **sequentially** in v0.2, but the event stream, the permission serialization, and the session layout are designed for concurrency so parallelism is a scheduler change and not a rewrite. djinni set `DevPhase.parallel` and never read it; we're not shipping the flag before the scheduler. - [ ] Session persistence nests: a sub-agent transcript is its own file referenced by the parent's, so `coded sessions list` still lists conversations and not fragments, and a run can be replayed in full. - [ ] Budgets: a per-invocation ceiling on round-trips and tokens, inherited and decremented down the tree, surfaced in the usage banner per agent. djinni's only bounds were 20 rounds and 20 consecutive build failures, with no cost ceiling anywhere — this is the fix for that. @@ -333,7 +333,7 @@ Features: ## Cross-cutting, throughout -- [ ] ADRs for the decisions this roadmap makes: reasoning being explicit and enforced rather than delegated to a provider's own thinking, intent being explicit too (goal, todo list, and a declared turn outcome), the event stream gaining an agent dimension and a turn outcome, agent definitions being narrowing-only, the archetype living in the repo as data, workflow stages being derived from the archetype rather than restating it, checkpoint commits being opt-in, and enforcement being path-scoped. +- [ ] ADRs for the decisions this roadmap makes: reasoning being explicit and enforced rather than delegated to a provider's own thinking, intent being explicit too (goal, todo list, and a declared turn outcome), the event stream gaining an agent dimension and a turn outcome, one permission mode covering the whole agent tree, the archetype living in the repo as data, workflow stages being derived from the archetype rather than restating it, checkpoint commits being opt-in, and enforcement being path-scoped. - [ ] `cmd/promptdump` grows with each milestone — every agent, scheme, pass, and archetype rendering ends up in a committed dump, because a system whose behavior is prompts needs its prompts in the diff. - [ ] Per-agent and per-stage token accounting on the existing usage banner, and a run summary at the end of a workflow. - [ ] Every new subsystem — schemes, archetypes, workflows — is loaded from data with a built-in default, and a malformed file keeps the previous good state instead of crashing, matching how permission rules already hot-reload. diff --git a/adr/2026-07-01_00-38-09_ui-agnostic-agent-loop.md b/adr/2026-07-01_00-38-09_ui-agnostic-agent-loop.md index 9029303..ec74e32 100644 --- a/adr/2026-07-01_00-38-09_ui-agnostic-agent-loop.md +++ b/adr/2026-07-01_00-38-09_ui-agnostic-agent-loop.md @@ -35,3 +35,6 @@ events carrying a response channel, not callbacks into a UI. - The stream is the only path to a front end, so anything appended to history without emitting events is invisible to the UI by construction — `agent.SyntheticReadmeExchange` is the one deliberate case. +- A sub-agent's activity is still just events on this same stream, not a + parallel one a consumer has to know how to multiplex — see + [The event stream gains an agent dimension](2026-08-08_03-48-52_event-stream-agent-dimension.md). diff --git a/adr/2026-08-08_03-48-52_event-stream-agent-dimension.md b/adr/2026-08-08_03-48-52_event-stream-agent-dimension.md new file mode 100644 index 0000000..368dd0d --- /dev/null +++ b/adr/2026-08-08_03-48-52_event-stream-agent-dimension.md @@ -0,0 +1,63 @@ +# The event stream gains an agent dimension + +- **Status:** Accepted +- **Date:** 2026-08-08 03:48:52 +- **Commit:** `f0ab06e` + +## Context + +[UI-agnostic agent loop driven by an event stream][agent-loop-adr] +established `agent.Event` as the only path from the turn loop to a front +end. Once a turn can spawn a sub-agent — all tool calls, permission prompts, +and text are all still events on the *same* stream a flat consumer already +drains, with nothing in the `Event` shape saying which agent in the tree +produced any of them. + +Two designs were available: give a spawned agent its own, separate event +channel that the parent explicitly multiplexes, or keep one channel and +tag each event with where it came from. A separate channel is how a naive +`Spawn` implementation reads at first (a sub-agent's `Run` already returns +one), but it means every consumer — both front ends today, anything else +later — has to know about the tree shape and multiplex channels itself. +It also doesn't compose: a grandchild's channel would need multiplexing +into its child's, which multiplexes into the parent's, duplicating the +same fan-in logic at every level. + +[agent-loop-adr]: 2026-07-01_00-38-09_ui-agnostic-agent-loop.md + +## Decision + +Every `Event` gains two fields, set once by whichever agent produced it: + +* `AgentName string`, the full chain from the outermost agent to this one + joined by `/` (`"main"` for the root, `"main/explorer"` for a direct child, + and so on for deeper nesting), +* `AgentDepth int`, that chain's length minus one (`0` at the root). + +An `Agent` now carries its own `Name`/`Depth`, fixed at construction -- +`"main"`/`0` for the one root `Agent` `cmd/coded` builds, or the calling +agent's `Name`+`"/"`+the new definition's `Name` / calling agent's +`Depth`+`1` for a child `definition.NewChild` builds. +`Agent.Run` wraps its internal event channel in a single relay before +returning it, copying `Name`/`Depth` onto every event the channel carries. + +Because a child's own `Run` call already stamps every event with its full +chain before `Spawn` ever sees it, `agent.ForwardChild` just relays without +rewriting anything. + +## Consequences + +- The stream stays flat and single-channel, which is what the previous + record's testability argument (a fake provider and a channel drain, no + terminal involved) depends on — that continues to hold for a tree of + agents exactly as it did for one. +- A consumer that ignores `AgentName`/`AgentDepth` still works: every + event still arrives, in order, on the one channel it already reads. +- `AgentName`/`AgentDepth`-aware rendering is still each front end's own + problem to solve well. +- `AgentDepth` being its own field means indentation-by-depth never has + to infer depth from `AgentName`, and nothing has to parse `AgentName` + back apart to find the innermost name either — the two questions ("how deep" + and "which agent") are answered by two fields instead of one being + overloaded for both. + diff --git a/adr/2026-08-10_01-53-43_singleton-permission-mode.md b/adr/2026-08-10_01-53-43_singleton-permission-mode.md new file mode 100644 index 0000000..b62f626 --- /dev/null +++ b/adr/2026-08-10_01-53-43_singleton-permission-mode.md @@ -0,0 +1,85 @@ +# One permission mode for the whole agent tree + +- **Status:** Accepted +- **Date:** 2026-08-10 01:53:43 +- **Commit:** `6cdc597` + +## Context + +[Four permission modes with risk-based fallback][modes-adr] gave a session one +posture for calls no rule matches. v0.2 adds agent definitions and `Spawn`, and +with them a question that posture never had to answer: when a parent spawns a +child, whose mode applies? + +Two designs were available. + +**A permission mode per agent definition.** `Definition` carries a +`permission_mode` alongside its identity, tool subset and scheme, and a spawned +child runs under its own posture rather than its parent's. A read-only +investigator can then be pinned to `plan` regardless of how loose the session +around it is. Because a definition is authored data, this design also has to +decide what happens when a definition declares a mode *looser* than the session's: +either the declaration is combined with the parent's so that only the stricter +of the two survives, or a widening declaration raises a permission prompt the +way a tool call does. + +**One mode for the session.** The harness owns a single mode, resolved from the +flag and config layers at startup and changeable by the user at any point after. +Every agent in the tree consults that one value, an agent's definition says +nothing about permissions at all, and a spawn does not change the posture in +force. + +[modes-adr]: 2026-07-01_00-38-09_permission-modes.md + +## Decision + +Permission mode belongs to the session. There is exactly one value in force at +any moment: the harness resolves it from the flag and config layers at startup, +and from then on it changes only in response to explicit user input about the +permission mode — never on its own, and never as a side effect of anything the +agent tree does. Such a change applies immediately to every agent, the ones +already running as much as the ones spawned afterward. `Definition` carries no +permission mode, and a spawn does not change the value. + +Per-definition mode was rejected on four counts. + +- **Neither answer to the widening question is good.** Keeping the stricter of + the two discards a declaration silently, in exactly the case its author cared + about — and it guards one authoring surface while `.coded/settings.json` sets + the mode outright at higher precedence (see + [Layered config with a fixed merge order][config-adr]), which is not a + boundary. Prompting on escalation asks for unbounded authority mid-turn, where + refusing kills the whole sub-agent rather than one call. +- **It brackets the mode around control flow.** The user changing the mode is + one writer, no nesting, the new value in force from then on. A per-agent mode + installs on spawn and *restores* on completion, so what applies depends on + where in the tree execution is — sound only while spawns are sequential, since + two overlapping children would restore over each other. +- **Its demonstrated uses belong elsewhere, or are wrong.** `explorer`'s `plan` + is belt-and-braces over its own read-only tool list, which a test checks + earlier and more cheaply — plus a reluctance to prompt on a sub-agent's + behalf, which has it backwards: under a manual mode the user wants to be asked + about what a sub-agent does, exactly as about what the main agent does. + +[config-adr]: 2026-07-01_00-38-09_config-merge-order.md + +## Consequences + +- Parallel sub-agents get cheaper rather than harder. The mode stays one value + that every agent reads and only the user writes, so concurrent children all + observe the same posture — and a change the user makes mid-run reaches all of + them at once, which is the behavior to want. +- Per-agent prompt posture becomes inexpressible. The mode is the fallback for + calls no rule covers, so "this agent may edit, but always ask" had a spelling + and now has none; the remaining answer — don't grant the tool — turns a middle + setting into all-or-nothing. +- Sub-agent operations are prompted for, and that is the intent. Under a manual + mode an unmatched call raises a prompt wherever in the tree it came from, and + the user answers it the way they answer the main agent's. Making a sub-agent + quieter than its parent is not a goal the mode is asked to serve; what remains + open is only presentation — which agent is asking, and one question at a time + once spawns run concurrently. +- A `permission_mode` key in an agent file (supported by Claude Code) is now + an unknown key, and unknown keys are ignored rather than rejected — so a + definition written against a harness that honors one does less than its author + intended, silently. diff --git a/adr/README.md b/adr/README.md index ab2edef..69bccb8 100644 --- a/adr/README.md +++ b/adr/README.md @@ -80,3 +80,5 @@ Package-by-package layout is in [CODED.md](../CODED.md#layout). | 2026-07-26 09:14:04 | [Retry lives in a provider middleware; mid-stream failures are terminal](2026-07-26_09-14-04_retry-middleware-no-mid-stream-resume.md) | | 2026-08-01 18:16:10 | [The response-format protocol is data, not code](2026-08-01_18-16-10_response-protocol-as-data.md) | | 2026-08-07 21:31:47 | [The response protocol is enforced live, mid-stream, not just after](2026-08-07_21-31-47_mid-stream-protocol-enforcement.md) | +| 2026-08-08 03:48:52 | [The event stream gains an agent dimension](2026-08-08_03-48-52_event-stream-agent-dimension.md) | +| 2026-08-10 01:53:43 | [One permission mode for the whole agent tree](2026-08-10_01-53-43_singleton-permission-mode.md) | diff --git a/cmd/coded/main.go b/cmd/coded/main.go index 67718eb..92c0ba7 100644 --- a/cmd/coded/main.go +++ b/cmd/coded/main.go @@ -3,14 +3,15 @@ package main import ( "context" + "errors" "flag" "fmt" "os" "time" "github.com/mchalapuk/coded/internal/agent" + "github.com/mchalapuk/coded/internal/agent/definition" "github.com/mchalapuk/coded/internal/agent/protocol" - "github.com/mchalapuk/coded/internal/agent/react" "github.com/mchalapuk/coded/internal/config" "github.com/mchalapuk/coded/internal/llm" "github.com/mchalapuk/coded/internal/llm/anthropic" @@ -21,6 +22,7 @@ import ( "github.com/mchalapuk/coded/internal/terminal/oneshot" "github.com/mchalapuk/coded/internal/terminal/tui" "github.com/mchalapuk/coded/internal/tool/builtin" + "github.com/mchalapuk/coded/internal/tool/builtin/spawn" ) var subcommands = map[string]func([]string) error{ @@ -71,18 +73,34 @@ const ( exitGeneric = 1 exitAuth = 2 exitProvider = 3 // rate-limited/overloaded/server/network, retries exhausted + // exitAbandoned distinguishes "the agent stopped deliberately and said + // why" from both success and a crash -- a script needs to tell those + // apart (see PLAN.md's Turn outcomes section). Not reachable yet: no + // turn loop produces agent.OutcomeAbandoned until the planning-pass + // milestone gives a turn a goal/todo list to abandon (see + // agent.TurnResult's doc comment) -- the mapping is wired now so that + // milestone is exit-code-complete on day one instead of needing this + // switch revisited. + exitAbandoned = 4 ) // exitCodeFor picks the process exit code for a top-level run error and, // for anything oneshot.Render (or the TUI) hasn't already rendered to -// stderr itself, prints it here. A *llm.Error reaching this point was -// always produced by runOneShot's protocol.Agent.Run loop, whose event stream -// oneshot.Render already rendered as a classified cause + next step (see -// its EventError case) -- printing it again here would just duplicate that -// message in a less useful, unclassified form, so this only picks the exit -// code for it. Every other error (config, session, TUI setup, ...) never -// reached a renderer, so it's printed here, as before. +// stderr itself, prints it here. A *oneshot.TurnError reaching this point +// carries the root turn's own Outcome (see oneshot.Render) -- checked +// first, since OutcomeAbandoned has no classified *llm.Error to fall back +// to and still needs its own exit code. Every other error (config, +// session, TUI setup, a *llm.Error unwrapped from a TurnError or reaching +// here directly) is classified exactly as before. func exitCodeFor(err error) int { + var te *oneshot.TurnError + if errors.As(err, &te) { + if te.Outcome == agent.OutcomeAbandoned { + fmt.Fprintln(os.Stderr, "coded:", err) + return exitAbandoned + } + err = te.Err + } pe, ok := llm.AsError(err) if !ok { fmt.Fprintln(os.Stderr, "coded:", err) @@ -143,9 +161,25 @@ func run(opts runOptions) error { permEngine := permission.NewWithSource(permission.Mode(settings.PermissionMode), ruleSource) permEngine.SetRoot(cwd) - a := react.New(p, builtin.NewRegistry(), permEngine) + sess, err := resolveSession(opts, cwd, p.Name(), settings.Model) + if err != nil { + return err + } + + registry := builtin.NewRegistry() + registry.Register(&spawn.Tool{ + Provider: p, + Tools: registry, + Perm: permEngine, + Definitions: definition.Spawnable(), + Session: sess, + }) + + a, err := definition.NewProtocolAgent(definition.Main, p, registry, permEngine) + if err != nil { + return err + } a.Model = settings.Model - a.System = agent.DefaultIdentity a.ProjectContext = func() string { return project.Block(cwd) } a.Readme = func() protocol.ReadmeInfo { content, found := project.Readme(cwd) @@ -155,10 +189,6 @@ func run(opts runOptions) error { return config.AppendRule(config.ProjectConfigPath(cwd), spec, decision) } - sess, err := resolveSession(opts, cwd, p.Name(), a.Model) - if err != nil { - return err - } if msgs, err := sess.LoadMessages(); err == nil && len(msgs) > 0 { a.LoadMessages(msgs) } diff --git a/cmd/coded/main_test.go b/cmd/coded/main_test.go index 716cc69..2f81d24 100644 --- a/cmd/coded/main_test.go +++ b/cmd/coded/main_test.go @@ -6,9 +6,11 @@ import ( "testing" "time" + "github.com/mchalapuk/coded/internal/agent" "github.com/mchalapuk/coded/internal/config" "github.com/mchalapuk/coded/internal/llm" "github.com/mchalapuk/coded/internal/llm/anthropic" + "github.com/mchalapuk/coded/internal/terminal/oneshot" ) type fakeLister []llm.Model @@ -91,6 +93,16 @@ func TestExitCodeForClassifiesProviderErrors(t *testing.T) { {"invalid request", &llm.Error{Kind: llm.ErrInvalidRequest}, exitGeneric}, {"context length", &llm.Error{Kind: llm.ErrContextLength}, exitGeneric}, {"unclassified", errors.New("config error"), exitGeneric}, + { + "TurnError wrapping a classified provider error still classifies", + &oneshot.TurnError{Outcome: agent.OutcomeErrored, Err: &llm.Error{Kind: llm.ErrAuth}}, + exitAuth, + }, + { + "TurnError with OutcomeExhausted and an unclassified error", + &oneshot.TurnError{Outcome: agent.OutcomeExhausted, Err: agent.ErrBudgetExhausted}, + exitGeneric, + }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -101,6 +113,26 @@ func TestExitCodeForClassifiesProviderErrors(t *testing.T) { } } +// TestExitCodeForAbandonedIsDistinctFromEveryOtherCode confirms +// OutcomeAbandoned gets its own exit code, checked before any *llm.Error +// unwrapping -- an abandoned turn may carry no error at all (see +// agent.TurnResult's doc comment), so it must not fall through to +// exitGeneric just because there's nothing for llm.AsError to classify. +// Not reachable from a real run yet (see PLAN.md's Goals and plans +// milestone), but exercised directly here so the mapping is proven correct +// before anything can actually produce it. +func TestExitCodeForAbandonedIsDistinctFromEveryOtherCode(t *testing.T) { + got := exitCodeFor(&oneshot.TurnError{Outcome: agent.OutcomeAbandoned}) + if got != exitAbandoned { + t.Fatalf("exitCodeFor(OutcomeAbandoned) = %d, want exitAbandoned (%d)", got, exitAbandoned) + } + for _, other := range []int{exitGeneric, exitAuth, exitProvider} { + if got == other { + t.Fatalf("exitAbandoned (%d) collides with an existing exit code %d", exitAbandoned, other) + } + } +} + func TestBuildProviderOpenAIHasNoLister(t *testing.T) { t.Setenv("HOME", t.TempDir()) t.Setenv("OPENAI_API_KEY", "test-key") diff --git a/cmd/promptdump/main.go b/cmd/promptdump/main.go index 73f95a9..397a33a 100644 --- a/cmd/promptdump/main.go +++ b/cmd/promptdump/main.go @@ -1,10 +1,11 @@ // Command promptdump drives a scripted, fake agent run through the real // agent loop, tool registry, and permission engine -- only the model itself -// is mocked -- and dumps the exact request stream it produced to a single -// human-readable file under ./prompts/. It exists so the prompt the model -// actually sees (system prompt, tool schemas, conversation history, and the -// react-tag reminder injected before each turn) can be inspected without -// burning a real API call or instrumenting a live session. +// is mocked -- and dumps the exact request stream it produced to a +// human-readable file under ./prompts/, one file per shipped agent +// definition. It exists so the prompt the model actually sees (system +// prompt, tool schemas, conversation history, and the format-reminder +// injected before each turn) can be inspected without burning a real API +// call or instrumenting a live session. package main import ( @@ -18,6 +19,8 @@ import ( "time" "github.com/mchalapuk/coded/internal/agent" + "github.com/mchalapuk/coded/internal/agent/definition" + "github.com/mchalapuk/coded/internal/agent/explorer" "github.com/mchalapuk/coded/internal/agent/protocol" "github.com/mchalapuk/coded/internal/agent/react" "github.com/mchalapuk/coded/internal/llm" @@ -41,15 +44,35 @@ func run() error { } defer cleanup() - sp := &scriptedProvider{turns: scriptedConversation(workDir)} + if err := dumpAgent(workDir, "agent-run.txt", definition.Main, + scriptedConversation(workDir), "Add an email validator to validators.go, with a test."); err != nil { + return err + } + if err := dumpAgent(workDir, "explorer-run.txt", definition.Explorer, + scriptedExplorerConversation(workDir), "Find where retry backoff is implemented."); err != nil { + return err + } + return nil +} + +// dumpAgent builds def's agent against a fresh scriptedProvider replaying +// turns, drives one real Run call with prompt, and writes the resulting +// request/message stream to prompts/filename -- the same treatment every +// shipped definition gets, so a prompt change in any of them shows up as a +// reviewable diff (djinni's `npm run generate` dumped one file per phase +// for the same reason). +func dumpAgent(workDir, filename string, def definition.Definition, turns [][]llm.Event, prompt string) error { + sp := &scriptedProvider{turns: turns} permEngine := permission.New(permission.ModeBypass, nil) permEngine.SetRoot(workDir) - a := react.New(sp, builtin.NewRegistry(), permEngine) + a, err := definition.NewProtocolAgent(def, sp, builtin.NewRegistry(), permEngine) + if err != nil { + return fmt.Errorf("building %s agent: %w", def.Name, err) + } a.Model = anthropic.DefaultModel a.MaxTokens = 8192 - a.System = agent.DefaultIdentity a.ProjectContext = func() string { return project.Block(workDir) } a.Readme = func() protocol.ReadmeInfo { content, found := project.Readme(workDir) @@ -58,7 +81,7 @@ func run() error { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - for range a.Run(ctx, "Add an email validator to validators.go, with a test.") { + for range a.Run(ctx, prompt) { // Drained without rendering: this script cares about the resulting // request/message stream, not the live event feed a TUI would show. } @@ -72,7 +95,7 @@ func run() error { if err := os.MkdirAll("prompts", 0o755); err != nil { return fmt.Errorf("creating ./prompts: %w", err) } - path := filepath.Join("prompts", "agent-run.txt") + path := filepath.Join("prompts", filename) if err := os.WriteFile(path, []byte(out), 0o644); err != nil { return fmt.Errorf("writing dump: %w", err) } @@ -227,6 +250,40 @@ func reactTagged(contents ...string) string { return react.Protocol().WrapSections(contents...) + "\n" } +// scriptedExplorerConversation is scriptedConversation's equivalent for the +// explorer definition: one tool-call turn (a Grep, tagged under explorer's +// own / protocol, not react's) followed by a final +// answer. The Grep pattern and workDir are real, so the tool actually runs +// -- and this workspace genuinely has no match for it, which is what makes +// the scripted final answer ("no backoff-related code exists here") the +// same claim a real explorer would have reached from this Grep's real +// output, not just a plausible-sounding fake. +func scriptedExplorerConversation(workDir string) [][]llm.Event { + turn1Text := explorerTagged( + "The task is to find where retry backoff is computed. I haven't looked at this workspace yet, " + + "so I'll grep for \"backoff\" across it before concluding anything.", + ) + turn2Text := explorerTagged( + "Grep found no matches for \"backoff\" anywhere in the workspace.", + "No backoff-related code exists in this workspace -- searched for \"backoff\" (case-sensitive, "+ + "whole tree) and found nothing. Not found: any retry/backoff implementation.", + ) + + return [][]llm.Event{ + textAndTools(turn1Text, + toolCall(agent.SyntheticToolUseID("Grep", 1), "Grep", mustJSON(map[string]any{"pattern": "backoff", "path": workDir})), + ), + finalAnswer(turn2Text), + } +} + +// explorerTagged is reactTagged's twin for explorer.Protocol(): a +// preamble for a tool-call turn, plus a second content for the final turn's +// close. +func explorerTagged(contents ...string) string { + return explorer.Protocol().WrapSections(contents...) + "\n" +} + func mustJSON(v any) json.RawMessage { b, err := json.Marshal(v) if err != nil { diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 9053957..4bebe85 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -51,6 +51,25 @@ type Agent struct { System string MaxTokens int + // Name is this agent's place in the spawn tree: the full chain from + // the root to here, joined by "/" -- "main" for the one root Agent + // cmd/coded builds (definition.Main.Name is "main", and + // definition.New copies a Definition's Name onto the Agent it + // builds), "main/explorer" for a direct child, and so on for deeper + // nesting (definition.NewChild extends its parent's Name with its + // own). Left "" by New for a caller that doesn't care (e.g. a bare + // test agent). Copied onto every Event Run's output channel ever + // carries -- see Run and Event.AgentName -- so nothing in StreamTurn, + // DispatchToolCalls, or any other internal emission site needs to + // know it exists. + Name string + // Depth is how many "/"-separated segments precede the last one in + // Name (0 at the root, i.e. Name has no "/" at all). Kept as its own + // field, set alongside Name by the same callers, rather than derived + // by splitting Name, so depth-based indentation (see Event.AgentDepth) + // never has to parse it back apart. + Depth int + // MaxIterations bounds the number of provider round-trips within a // single Run call, guarding against a model stuck in a tool-call loop. // Zero (the default from New) falls back to defaultMaxIterations. @@ -82,6 +101,26 @@ type Agent struct { // still honors the choice. PersistRule func(spec string, decision permission.Decision) error + // Budget, if set, bounds round-trips and tokens for this agent and + // (once shared via Budget.Sub with a spawned child's own Agent) the + // whole tree beneath it. StreamTurn records against it as each + // provider call happens; the turn loop (Run, protocol.Agent.run) + // checks Exhausted before starting a new iteration and halts if it's + // out. Nil is unlimited, today's behavior for every existing caller + // that never sets one. + Budget *Budget + + // LastOutcome is set at the end of every Run call to that turn's own + // TurnResult, and read at the start of the next one: a non-Completed + // outcome is rendered into a block on the opening user message (see + // previousOutcomeBlock, NewTurnMessage) so the model knows the prior + // turn didn't finish cleanly instead of silently continuing as if + // history simply had nothing more to do. Not modelled as a new + // Situation (PLAN.md's Turn outcomes section) -- it's a plain stored + // message like any other, just harness-authored rather than typed. + // Nil until the first Run call ends. + LastOutcome *TurnResult + // mu guards messages. A wrapping layer's turn loop (see // react.Agent.Run) executes on its own goroutine, while the caller's // own goroutine reads the conversation concurrently through @@ -95,15 +134,41 @@ type Agent struct { } // New creates an Agent. p, tools, and perm must not be nil. +// +// Name defaults to "main": a bare Agent, with no Definition/NewChild telling +// it otherwise, behaves as the root of its own tree for AgentName/ +// AgentDepth purposes (see Event.AgentName) -- the same role nil Path +// played for every bare Agent.Run/protocol.Agent.Run call before this +// field existed. definition.New overrides it with the constructing +// Definition's own Name; definition.NewChild overrides that again with the +// full parent-prefixed chain for an actual spawned child. func New(p llm.Provider, tools *tool.Registry, perm *permission.Engine) *Agent { return &Agent{ Provider: p, Tools: tools, Permission: perm, MaxIterations: defaultMaxIterations, + Name: "main", } } +// EndTurn sets a.LastOutcome to result and sends the terminal EventTurnEnded +// (carrying err, if any -- see Event.Err's doc comment) on out. The single +// place every turn loop's exit path goes through, so LastOutcome can never +// drift out of sync with what was actually emitted on the stream. +func (a *Agent) EndTurn(out chan<- Event, err error, result *TurnResult) { + a.LastOutcome = result + out <- Event{Type: EventTurnEnded, Err: err, Result: result} +} + +// SetBudget assigns b as this agent's Budget. A trivial setter, but a +// necessary one: definition.Agent's minimal interface (Run/Messages/ +// LoadMessages) has no field access, so it needs a method to let +// definition.NewChild attach a spawned child's shared budget (see +// Budget.Sub) without knowing which concrete type (*Agent or +// *protocol.Agent, which gets this for free via embedding) it's holding. +func (a *Agent) SetBudget(b *Budget) { a.Budget = b } + // defaultMaxIterations bounds MaxIterations when left at zero. const defaultMaxIterations = 50 @@ -173,7 +238,7 @@ func (a *Agent) RequestMessagesParts() (prefix, suffix []llm.Message) { if last < 0 { return msgs, nil } - block := "" + var block string if a.ProjectContext != nil { block = a.ProjectContext() } @@ -263,6 +328,25 @@ func WrapHarnessMessage(text string) string { return "\n" + text + "\n" } +// NewTurnMessage builds the user-role message that opens a turn: userInput +// wrapped via WrapUserMessage, plus -- when prev (the prior turn's own +// TurnResult, i.e. Agent.LastOutcome) says that turn didn't finish cleanly +// -- a second, harness-authored block naming how it ended (see +// previousOutcomeBlock). Both loops that open a turn (this package's run +// and protocol.Agent's) call this rather than building the message +// themselves, so the note is stored as ordinary history exactly once, not +// re-derived and re-injected on every request the turn makes -- unlike +// ProjectContext, which does need re-deriving since it can change turn to +// turn (see withProjectContext), an outcome is fixed the moment the turn +// that produced it ends. +func NewTurnMessage(userInput string, prev *TurnResult) llm.Message { + content := []llm.ContentBlock{{Type: llm.ContentText, Text: WrapUserMessage(userInput)}} + if block := previousOutcomeBlock(prev); block != "" { + content = append(content, llm.ContentBlock{Type: llm.ContentText, Text: WrapHarnessMessage(block)}) + } + return llm.Message{Role: llm.RoleUser, Content: content} +} + // SyntheticToolUseID returns a stable, human-readable ID for the n-th // fabricated call to a tool named toolName within one conversation, pairing // a synthetic tool_use block with its tool_result. The react package's @@ -479,6 +563,7 @@ func (a *Agent) StreamTurn(ctx context.Context, out chan<- Event, system string, if err != nil { return llm.Message{}, "", VerdictNone, err } + a.Budget.RecordRoundTrip() var textBuf strings.Builder // emitted is how much of textBuf has been forwarded to out; the rest is @@ -556,6 +641,9 @@ func (a *Agent) StreamTurn(ctx context.Context, out chan<- Event, system string, pt.ended = true } case llm.EventUsage: + if ev.Usage != nil { + a.Budget.RecordTokens(ev.Usage.InputTokens + ev.Usage.OutputTokens) + } out <- Event{Type: EventUsage, Usage: ev.Usage} case llm.EventRetrying: out <- Event{ @@ -664,9 +752,8 @@ func ExtractToolCalls(msg llm.Message) []ToolCallInfo { } // Run starts a new turn with userInput and returns a channel of Events. The -// channel is closed when the agent has produced a final answer -// (EventTurnComplete) or hit a terminal error (EventError). The caller must -// drain the channel; for any EventPermissionRequested the caller must +// channel is closed after exactly one EventTurnEnded, whatever its Outcome. +// The caller must drain the channel; for any EventPermissionRequested the caller must // eventually call Respond, including after ctx cancellation, or the agent // goroutine will leak. // @@ -675,8 +762,28 @@ func ExtractToolCalls(msg llm.Message) []ToolCallInfo { // reads. A caller that needs those wraps protocol.Agent instead, which // embeds this type and defines its own Run over the same two primitives. func (a *Agent) Run(ctx context.Context, userInput string) <-chan Event { + raw := make(chan Event, 16) + go a.run(ctx, userInput, raw) + return StampIdentity(raw, a.Name, a.Depth) +} + +// StampIdentity relays every event raw carries to a new channel with +// AgentName/AgentDepth set from name/depth -- the one place either field is +// ever written, so nothing upstream of Run (StreamTurn, DispatchToolCalls, +// dispatchSteppedTool, askPermission, EndTurn) needs to set them itself. +// Exported so protocol.Agent's own Run -- a separate implementation that +// doesn't call this one (see this package's doc comment) -- can wrap its +// own output channel the same way. +func StampIdentity(raw <-chan Event, name string, depth int) <-chan Event { out := make(chan Event, 16) - go a.run(ctx, userInput, out) + go func() { + defer close(out) + for ev := range raw { + ev.AgentName = name + ev.AgentDepth = depth + out <- ev + } + }() return out } @@ -686,10 +793,7 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- Event) { // turnStart marks the state to roll back to if the very first provider // call of this Run invocation fails -- see the iter == 0 branch below. turnStart := a.MessageCount() - a.AppendMessage(llm.Message{ - Role: llm.RoleUser, - Content: []llm.ContentBlock{{Type: llm.ContentText, Text: WrapUserMessage(userInput)}}, - }) + a.AppendMessage(NewTurnMessage(userInput, a.LastOutcome)) maxIter := a.MaxIterations if maxIter == 0 { @@ -697,6 +801,10 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- Event) { } for iter := 0; iter < maxIter; iter++ { + if a.Budget.Exhausted() { + a.EndTurn(out, ErrBudgetExhausted, &TurnResult{Outcome: OutcomeExhausted, Reason: ErrBudgetExhausted.Error()}) + return + } assistantMsg, stopReason, _, err := a.StreamTurn(ctx, out, a.SystemPrompt(), a.RequestMessages(), nil) if err != nil { if iter == 0 { @@ -709,14 +817,14 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- Event) { // valid exchange. a.PruneMessages(turnStart, a.MessageCount()) } - out <- Event{Type: EventError, Err: err} + a.EndTurn(out, err, &TurnResult{Outcome: OutcomeForError(err), Reason: err.Error()}) return } a.AppendMessage(assistantMsg) toolCalls := ExtractToolCalls(assistantMsg) if stopReason != llm.StopToolUse || len(toolCalls) == 0 { - out <- Event{Type: EventTurnComplete} + a.EndTurn(out, nil, &TurnResult{Outcome: OutcomeCompleted}) return } @@ -727,13 +835,14 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- Event) { // drop it rather than leave it dangling in history. n := a.MessageCount() a.PruneMessages(n-1, n) - out <- Event{Type: EventError, Err: err} + a.EndTurn(out, err, &TurnResult{Outcome: OutcomeForError(err), Reason: err.Error()}) return } a.AppendMessage(resultMsg) } - out <- Event{Type: EventError, Err: fmt.Errorf("agent: exceeded max iterations (%d) without reaching a final answer", maxIter)} + maxIterErr := fmt.Errorf("%w (%d) without reaching a final answer", ErrMaxIterationsExceeded, maxIter) + a.EndTurn(out, maxIterErr, &TurnResult{Outcome: OutcomeExhausted, Reason: maxIterErr.Error()}) } // DispatchToolCalls evaluates each call against the permission engine, @@ -750,7 +859,24 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- Event) { // dispatchSteppedTool. A denial mid-batch doesn't abort the rest -- every // call produces a result block either way, since both APIs reject a // tool_use with no matching tool_result on the next request. +// reporterAdapter implements tool.Reporter over an Event channel: the +// bridge that lets a tool.EventReportingTool (e.g. the Spawn tool) emit +// real agent.Event values without the tool package ever importing this one +// (see tool.Reporter's doc comment for why it has to stay untyped on that +// side). A reported value that isn't an Event is silently dropped rather +// than panicking -- defensive against a future EventReportingTool +// implementation reporting something else by mistake; there's no +// EventReportingTool that does today. +type reporterAdapter struct{ out chan<- Event } + +func (r reporterAdapter) Report(v any) { + if ev, ok := v.(Event); ok { + r.out <- ev + } +} + func (a *Agent) DispatchToolCalls(ctx context.Context, calls []ToolCallInfo, out chan<- Event) (llm.Message, error) { + ctx = WithCallerIdentity(ctx, a.Name, a.Depth) resultMsg := llm.Message{Role: llm.RoleUser} for _, tc := range calls { @@ -758,6 +884,7 @@ func (a *Agent) DispatchToolCalls(ctx context.Context, calls []ToolCallInfo, out t, found := a.Tools.Get(tc.Name) st, stepped := t.(tool.SteppedTool) + rt, reporting := t.(tool.EventReportingTool) var result tool.Result denied := false @@ -770,6 +897,19 @@ func (a *Agent) DispatchToolCalls(ctx context.Context, calls []ToolCallInfo, out if err != nil { return llm.Message{}, err } + case reporting: + // Deliberately no permission.Check here -- see the Spawn tool's + // own doc comment for why an EventReportingTool call is never + // asked about at this level: everything it does internally + // (a spawned agent's own tool calls) still goes through this + // same Check for each of those calls individually, so gating + // the call itself here would just be a second, redundant + // prompt for one decision. + var err error + result, err = rt.ExecuteReporting(ctx, tc.Input, reporterAdapter{out}) + if err != nil { + return llm.Message{}, err + } default: subject := t.Subject(tc.Input) decision := a.Permission.Check(permission.ToolCall{Tool: tc.Name, Subject: subject, Risk: t.Risk()}) diff --git a/internal/agent/budget.go b/internal/agent/budget.go new file mode 100644 index 0000000..3f67620 --- /dev/null +++ b/internal/agent/budget.go @@ -0,0 +1,110 @@ +package agent + +import ( + "errors" + "sync" +) + +// ErrBudgetExhausted is returned (wrapped in an EventError) when a Budget's +// ceiling is reached before a turn could finish -- distinct from a generic +// provider failure so a caller can tell "ran out of budget" from +// "something broke" (e.g. via errors.Is). The turn halts cleanly at an +// iteration boundary, the same shape the existing max-iterations halt +// already has, so conversation state stays exactly as resumable as any +// other clean halt -- nothing pruned mid-turn, nothing left dangling. +var ErrBudgetExhausted = errors.New("agent: budget exhausted") + +// Budget bounds provider round-trips and tokens across one agent tree: the +// same instance is shared by a root agent and every sub-agent it spawns +// (see Sub), so their combined usage -- not each one's independently -- is +// what's checked against the ceiling. This is the fix for a gap djinni +// (coded's predecessor) shipped with and documented but never closed: its +// only bounds were a flat round count and a consecutive-build-failure +// count, with no cost ceiling anywhere a tree of chats could hit (see +// PLAN.md's Sub-agents section). +// +// A nil *Budget (the zero value for an Agent that never sets one) is +// unlimited: every existing caller that doesn't construct one sees no +// behavior change. Either ceiling left at zero is itself unlimited, so +// NewBudget(0, 100000) bounds only tokens, and vice versa. +type Budget struct { + mu sync.Mutex + maxRoundTrips int // 0 = unlimited + maxTokens int // 0 = unlimited; input+output combined + roundTrips int + tokens int +} + +// NewBudget creates a Budget with the given ceilings. +func NewBudget(maxRoundTrips, maxTokens int) *Budget { + return &Budget{maxRoundTrips: maxRoundTrips, maxTokens: maxTokens} +} + +// Sub returns b itself: a spawned child shares its parent's remaining +// budget rather than getting a separate allocation, so what's bounded is +// the whole tree's combined usage, not each agent's independently. Kept as +// a method -- rather than having a caller just pass b along directly -- +// so a future per-child sub-ceiling (PLAN.md's Sub-agents section +// describes budgets as "inherited and decremented down the tree", which +// this satisfies; a narrower ceiling layered on top of the shared pool is +// a natural extension, not a redesign) has a single call site to change. +func (b *Budget) Sub() *Budget { return b } + +// RecordRoundTrip records that one more provider round-trip happened. +// Called once per actual call to a Provider (see StreamTurn), regardless of +// which loop (bare or enforced) or which agent in the tree made it. A nil +// receiver is a safe no-op, so every call site can call it unconditionally +// on an Agent's possibly-unset Budget field rather than guarding first. +func (b *Budget) RecordRoundTrip() { + if b == nil { + return + } + b.mu.Lock() + defer b.mu.Unlock() + b.roundTrips++ +} + +// RecordTokens records n more tokens (input+output combined) spent. Nil-safe, +// see RecordRoundTrip. +func (b *Budget) RecordTokens(n int) { + if b == nil { + return + } + b.mu.Lock() + defer b.mu.Unlock() + b.tokens += n +} + +// Exhausted reports whether either ceiling has already been reached or +// exceeded, without recording any new usage. Checked at the top of a turn +// loop's iteration (see protocol.Agent.run), before the round-trip it would +// spend, so a budget that ran out doesn't get spent past -- the same +// "check before you commit to it" shape the existing max-iterations bound +// already has. Nil-safe: a nil Budget is never exhausted, matching "nil is +// unlimited" above. +func (b *Budget) Exhausted() bool { + if b == nil { + return false + } + b.mu.Lock() + defer b.mu.Unlock() + if b.maxRoundTrips > 0 && b.roundTrips >= b.maxRoundTrips { + return true + } + if b.maxTokens > 0 && b.tokens >= b.maxTokens { + return true + } + return false +} + +// Usage returns the round-trips and tokens recorded so far, for a per-agent +// breakdown on the usage banner (see PLAN.md's Sub-agents section). Nil-safe: +// a nil Budget reports (0, 0). +func (b *Budget) Usage() (roundTrips, tokens int) { + if b == nil { + return 0, 0 + } + b.mu.Lock() + defer b.mu.Unlock() + return b.roundTrips, b.tokens +} diff --git a/internal/agent/budget_test.go b/internal/agent/budget_test.go new file mode 100644 index 0000000..e8058a9 --- /dev/null +++ b/internal/agent/budget_test.go @@ -0,0 +1,70 @@ +package agent + +import "testing" + +func TestBudgetNilIsUnlimited(t *testing.T) { + var b *Budget + if b.Exhausted() { + t.Fatalf("nil Budget must not report Exhausted") + } +} + +func TestBudgetZeroCeilingsAreUnlimited(t *testing.T) { + b := NewBudget(0, 0) + b.RecordRoundTrip() + b.RecordTokens(1_000_000) + if b.Exhausted() { + t.Fatalf("Budget with both ceilings at 0 must be unlimited") + } +} + +func TestBudgetRoundTripCeiling(t *testing.T) { + b := NewBudget(2, 0) + if b.Exhausted() { + t.Fatalf("fresh Budget should not be exhausted") + } + b.RecordRoundTrip() + if b.Exhausted() { + t.Fatalf("Budget should not be exhausted after 1 of 2 round-trips") + } + b.RecordRoundTrip() + if !b.Exhausted() { + t.Fatalf("Budget should be exhausted after reaching its round-trip ceiling") + } +} + +func TestBudgetTokenCeiling(t *testing.T) { + b := NewBudget(0, 100) + b.RecordTokens(60) + if b.Exhausted() { + t.Fatalf("Budget should not be exhausted below its token ceiling") + } + b.RecordTokens(50) + if !b.Exhausted() { + t.Fatalf("Budget should be exhausted once tokens reach its ceiling") + } +} + +func TestBudgetSubSharesTheSamePool(t *testing.T) { + parent := NewBudget(2, 0) + child := parent.Sub() + child.RecordRoundTrip() + if parent.Exhausted() { + t.Fatalf("1 of 2 round-trips recorded via a child should not exhaust the shared parent") + } + child.RecordRoundTrip() + if !parent.Exhausted() { + t.Fatalf("round-trips recorded via a child must count against the shared parent budget") + } +} + +func TestBudgetUsageReportsRecordedTotals(t *testing.T) { + b := NewBudget(0, 0) + b.RecordRoundTrip() + b.RecordRoundTrip() + b.RecordTokens(42) + rt, tok := b.Usage() + if rt != 2 || tok != 42 { + t.Fatalf("Usage() = (%d, %d), want (2, 42)", rt, tok) + } +} diff --git a/internal/agent/definition/agent.go b/internal/agent/definition/agent.go new file mode 100644 index 0000000..ba8b46b --- /dev/null +++ b/internal/agent/definition/agent.go @@ -0,0 +1,136 @@ +package definition + +import ( + "context" + "fmt" + + "github.com/mchalapuk/coded/internal/agent" + "github.com/mchalapuk/coded/internal/agent/protocol" + "github.com/mchalapuk/coded/internal/llm" + "github.com/mchalapuk/coded/internal/permission" + "github.com/mchalapuk/coded/internal/tool" +) + +// Agent is the minimal surface New's two Loop branches both already +// satisfy: *protocol.Agent (LoopEnforced) and *agent.Agent (LoopBare) each +// define Run with this exact signature, plus Messages/LoadMessages for +// session persistence -- see agent.Agent's and protocol.Agent's own doc +// comments. Kept intentionally small: a caller that needs +// protocol.Agent-specific behavior (Readme, PersistRule, Protocol()) should +// use NewProtocolAgent instead of widening this interface to cover it. +type Agent interface { + Run(ctx context.Context, userInput string) <-chan agent.Event + Messages() []llm.Message + LoadMessages(msgs []llm.Message) + SetBudget(b *agent.Budget) +} + +// New builds the agent def describes: tools resolves to +// tools.Subset(def.Tools) when def.Tools is non-nil, or tools unchanged +// (the caller's full registry) when it's nil. The returned value's +// concrete type is chosen by def.Loop -- *protocol.Agent for LoopEnforced, +// *agent.Agent for LoopBare -- and either way its System, Model, and +// MaxIterations fields are set from def before it's returned. prov, tools, +// and perm must not be nil. +func New(def Definition, prov llm.Provider, tools *tool.Registry, perm *permission.Engine) (Agent, error) { + scoped, err := resolveRegistry(tools, def.Tools) + if err != nil { + return nil, fmt.Errorf("definition %q: %w", def.Name, err) + } + + switch def.Loop { + case LoopBare: + a := agent.New(prov, scoped, perm) + a.System = def.Identity + a.Model = def.Model + a.MaxIterations = def.MaxIterations + a.Name = def.Name + return a, nil + default: // LoopEnforced + a := protocol.NewAgent(def.Protocol, prov, scoped, perm) + a.System = def.Identity + a.Model = def.Model + a.MaxIterations = def.MaxIterations + a.Name = def.Name + return a, nil + } +} + +// NewProtocolAgent builds def's agent the same way New does, then asserts +// it's the concrete *protocol.Agent type -- what cmd/coded's main agent and +// any other LoopEnforced definition's caller actually needs, for the +// protocol.Agent-only fields (Readme, PersistRule) and methods (Protocol()) +// that Agent's minimal interface doesn't expose. Returns an error rather +// than panicking if def declares LoopBare: the caller chose this +// constructor, not the definition author, so a mismatch is reported like +// any other construction failure. +func NewProtocolAgent(def Definition, prov llm.Provider, tools *tool.Registry, perm *permission.Engine) (*protocol.Agent, error) { + a, err := New(def, prov, tools, perm) + if err != nil { + return nil, err + } + pa, ok := a.(*protocol.Agent) + if !ok { + return nil, fmt.Errorf("definition %q: declares LoopBare, not an enforced protocol agent", def.Name) + } + return pa, nil +} + +// NewChild builds def's agent as a child spawned from parent: its Tools +// narrows against parent.Tools' actual contents (see ResolveTools) rather +// than being resolved against the raw tool.Registry in isolation, so a +// child cannot gain a tool its parent doesn't have -- and, since this is +// how every level resolves, not even transitively through a grandchild. +// Its Budget is parent.Budget.Sub(): the same shared pool, not a fresh +// allocation (see Budget.Sub's doc comment), so the whole tree's combined +// usage is what a ceiling bounds. Its Name extends parent.Name with def's +// own ("main" + "/" + "explorer" = "main/explorer"), and its Depth is +// parent.Depth + 1 -- see agent.Agent.Name and Event.AgentName for what +// those end up stamped on. +// +// perm is the single permission.Engine shared across the whole tree (see +// PLAN.md's Sub-agents section and the singleton-permission-mode ADR): +// NewChild passes it straight through, and nothing in the spawn path ever +// switches its mode -- the child's calls are permissioned exactly as the +// parent's are, through the one mode the session is in. Returns an error if +// def declares LoopBare: every spawn target this milestone is LoopEnforced, +// and a caller needing protocol.Agent-only behavior (Protocol(), Readme) the +// way Spawn does can't get it from Agent's minimal interface. +func NewChild(def Definition, parent *agent.Agent, perm *permission.Engine) (*protocol.Agent, error) { + resolved := def + resolved.Tools = ResolveTools(toolNames(parent.Tools), def.Tools) + + child, err := NewProtocolAgent(resolved, parent.Provider, parent.Tools, perm) + if err != nil { + return nil, err + } + child.SetBudget(parent.Budget.Sub()) + child.Name = parent.Name + "/" + def.Name + child.Depth = parent.Depth + 1 + return child, nil +} + +// toolNames extracts r's registered tool names in its own registration +// order, the shape ResolveTools needs to narrow a child's own Tools list +// against what its parent actually has available. +func toolNames(r *tool.Registry) []string { + list := r.List() + names := make([]string, len(list)) + for i, t := range list { + names[i] = t.Name() + } + return names +} + +// resolveRegistry applies a Definition's own Tools restriction to the +// registry a caller supplies: nil leaves it unchanged, a non-nil list scopes +// it down via Registry.Subset. This is the single-level case -- no parent to +// narrow against -- of the same policy ResolveTools applies when a child +// definition's list is combined with an already-resolved parent list at +// spawn time. +func resolveRegistry(tools *tool.Registry, names []string) (*tool.Registry, error) { + if names == nil { + return tools, nil + } + return tools.Subset(names) +} diff --git a/internal/agent/definition/agent_test.go b/internal/agent/definition/agent_test.go new file mode 100644 index 0000000..3a3d9f1 --- /dev/null +++ b/internal/agent/definition/agent_test.go @@ -0,0 +1,314 @@ +package definition + +import ( + "context" + "testing" + "time" + + "github.com/mchalapuk/coded/internal/agent" + "github.com/mchalapuk/coded/internal/agent/protocol" + "github.com/mchalapuk/coded/internal/llm" + "github.com/mchalapuk/coded/internal/permission" + "github.com/mchalapuk/coded/internal/tool" + "github.com/mchalapuk/coded/internal/tool/builtin" + "github.com/mchalapuk/coded/internal/tool/builtin/read" + "github.com/mchalapuk/coded/internal/tool/builtin/write" +) + +// fakeProvider replays a scripted sequence of turns; each call to Stream +// consumes the next turn in the script. A package-local copy of the same +// helper agent/run_test.go and protocol/agent_test.go each keep, per this +// codebase's existing convention (see agent/run_test.go's doc comment on its +// own copy): none of the three packages can import another's test file. +type fakeProvider struct { + turns [][]llm.Event + calls int +} + +func (f *fakeProvider) Name() string { return "fake" } + +func (f *fakeProvider) Stream(ctx context.Context, req llm.Request) (<-chan llm.Event, error) { + if f.calls >= len(f.turns) { + panic("fakeProvider: ran out of scripted turns") + } + turn := f.turns[f.calls] + f.calls++ + ch := make(chan llm.Event, len(turn)) + for _, e := range turn { + ch <- e + } + close(ch) + return ch, nil +} + +func textTurn(s string) []llm.Event { + return []llm.Event{ + {Type: llm.EventTextDelta, Text: s}, + {Type: llm.EventMessageStop, StopReason: llm.StopEndTurn}, + } +} + +// testReactTags is the minimal required-section preamble reactLikeProtocol +// demands, matching the convention internal/agent/protocol's own tests use +// (see its reactTags) -- so a scripted response doesn't trip the +// format-violation path unless a test means it to. +const testReactTags = "ot" + +func compliantTurn(result string) []llm.Event { + return []llm.Event{ + {Type: llm.EventTextDelta, Text: testReactTags + "" + result + ""}, + {Type: llm.EventMessageStop, StopReason: llm.StopEndTurn}, + } +} + +func drain(t *testing.T, ch <-chan agent.Event, timeout time.Duration) []agent.Event { + t.Helper() + var got []agent.Event + deadline := time.After(timeout) + for { + select { + case ev, ok := <-ch: + if !ok { + return got + } + got = append(got, ev) + case <-deadline: + t.Fatal("timed out draining agent event stream") + } + } +} + +func testRegistry() *tool.Registry { + r := tool.NewRegistry() + r.Register(read.Tool{}) + r.Register(write.Tool{}) + r.Prompt = "test prompt" + return r +} + +// TestNewLoopEnforcedRunsProtocolAgent covers the LoopEnforced branch: New +// must return a *protocol.Agent whose turn goes through the protocol's own +// react-style result section, not a bare completion. +func TestNewLoopEnforcedRunsProtocolAgent(t *testing.T) { + def := Definition{ + Name: "test-enforced", + Identity: "you are a test agent", + Protocol: reactLikeProtocol(), + Loop: LoopEnforced, + } + fp := &fakeProvider{turns: [][]llm.Event{compliantTurn("done")}} + perm := permission.New(permission.ModeDefault, nil) + + a, err := New(def, fp, testRegistry(), perm) + if err != nil { + t.Fatalf("New: %v", err) + } + if _, ok := a.(*protocol.Agent); !ok { + t.Fatalf("New with LoopEnforced returned %T, want *protocol.Agent", a) + } + + events := drain(t, a.Run(context.Background(), "hello"), time.Second) + var sawComplete bool + for _, ev := range events { + if isCompleted(ev) { + sawComplete = true + } + } + if !sawComplete { + t.Fatalf("events = %+v, want an EventTurnEnded with OutcomeCompleted", events) + } +} + +// TestNewLoopBareRunsPlainAgent covers the LoopBare branch: New must return +// a bare *agent.Agent that completes on a plain text reply with no +// response-format contract enforced -- proof that agent.Agent.Run is a real, +// exercised code path, not orphaned by protocol.Agent existing. +func TestNewLoopBareRunsPlainAgent(t *testing.T) { + def := Definition{ + Name: "test-bare", + Identity: "you are a test agent", + Loop: LoopBare, + } + fp := &fakeProvider{turns: [][]llm.Event{textTurn("plain reply, no sections")}} + perm := permission.New(permission.ModeDefault, nil) + + a, err := New(def, fp, testRegistry(), perm) + if err != nil { + t.Fatalf("New: %v", err) + } + if _, ok := a.(*agent.Agent); !ok { + t.Fatalf("New with LoopBare returned %T, want *agent.Agent", a) + } + + events := drain(t, a.Run(context.Background(), "hello"), time.Second) + var sawComplete bool + for _, ev := range events { + if isCompleted(ev) { + sawComplete = true + } + if ev.Type == agent.EventTurnEnded && ev.Result != nil && ev.Result.Outcome != agent.OutcomeCompleted { + t.Fatalf("unexpected non-completed outcome from bare loop: %+v", ev.Result) + } + } + if !sawComplete { + t.Fatalf("events = %+v, want an EventTurnEnded with OutcomeCompleted", events) + } +} + +// isCompleted reports whether ev is an EventTurnEnded with OutcomeCompleted +// -- the replacement for the old bare EventTurnComplete check, used +// throughout this file's tests. +func isCompleted(ev agent.Event) bool { + return ev.Type == agent.EventTurnEnded && ev.Result != nil && ev.Result.Outcome == agent.OutcomeCompleted +} + +func TestNewAppliesToolRestriction(t *testing.T) { + def := Definition{ + Name: "test-restricted", + Identity: "x", + Protocol: reactLikeProtocol(), + Loop: LoopEnforced, + Tools: []string{"Read"}, + } + fp := &fakeProvider{} + perm := permission.New(permission.ModeDefault, nil) + + a, err := New(def, fp, testRegistry(), perm) + if err != nil { + t.Fatalf("New: %v", err) + } + pa := a.(*protocol.Agent) + if _, ok := pa.Tools.Get("Write"); ok { + t.Fatalf("restricted agent still has Write registered") + } + if _, ok := pa.Tools.Get("Read"); !ok { + t.Fatalf("restricted agent lost Read, which its Tools list named") + } +} + +func TestNewUnknownToolNameErrors(t *testing.T) { + def := Definition{ + Name: "test-typo", + Loop: LoopEnforced, + Tools: []string{"DoesNotExist"}, + } + if _, err := New(def, &fakeProvider{}, testRegistry(), permission.New(permission.ModeDefault, nil)); err == nil { + t.Fatalf("New with unknown tool name: got nil error, want one naming the typo") + } +} + +// TestNewExplorerAgainstFullRegistry builds the real, shipped Explorer +// definition against the real builtin tool registry -- proof that the whole +// path (Registry.Subset, the tool-restriction resolution in New) works end +// to end for a definition that actually ships, not just a synthetic test +// fixture. +func TestNewExplorerAgainstFullRegistry(t *testing.T) { + pa, err := NewProtocolAgent(Explorer, &fakeProvider{}, builtin.NewRegistry(), permission.New(permission.ModeDefault, nil)) + if err != nil { + t.Fatalf("NewProtocolAgent(Explorer, ...): %v", err) + } + for _, mutating := range []string{"Write", "Edit", "Bash"} { + if _, ok := pa.Tools.Get(mutating); ok { + t.Errorf("explorer agent still has %q registered", mutating) + } + } + for _, readonly := range []string{"Read", "Grep", "Glob"} { + if _, ok := pa.Tools.Get(readonly); !ok { + t.Errorf("explorer agent is missing %q", readonly) + } + } + if pa.Protocol().Name != "explorer" { + t.Errorf("explorer agent's protocol = %q, want \"explorer\"", pa.Protocol().Name) + } +} + +// TestNewChildCannotWidenBeyondParent covers the property NewChild exists +// for: a child definition naming a tool its parent's own registry doesn't +// contain must not gain it, even though the underlying tools.Registry +// passed to NewProtocolAgent internally is the SAME shared full registry +// (parent.Tools), not some smaller one -- the narrowing has to come from +// resolving against parent.Tools' actual contents, not from the registry +// object's own size. +func TestNewChildCannotWidenBeyondParent(t *testing.T) { + parentDef := Definition{ + Name: "parent", Loop: LoopEnforced, Protocol: reactLikeProtocol(), + Tools: []string{"Read"}, // parent itself is already restricted to Read + } + parent, err := New(parentDef, &fakeProvider{}, testRegistry(), permission.New(permission.ModeDefault, nil)) + if err != nil { + t.Fatalf("New(parentDef): %v", err) + } + parentAgent := parent.(*protocol.Agent) + + childDef := Definition{ + Name: "child", Loop: LoopEnforced, Protocol: reactLikeProtocol(), + Tools: []string{"Read", "Write"}, // asks for Write, which parent lacks + } + child, err := NewChild(childDef, parentAgent.Agent, permission.New(permission.ModeDefault, nil)) + if err != nil { + t.Fatalf("NewChild: %v", err) + } + if _, ok := child.Tools.Get("Write"); ok { + t.Fatalf("child gained Write, which its parent's own registry never had") + } + if _, ok := child.Tools.Get("Read"); !ok { + t.Fatalf("child lost Read, which both parent and child named") + } +} + +func TestNewChildSharesParentBudget(t *testing.T) { + parentDef := Definition{Name: "parent", Loop: LoopEnforced, Protocol: reactLikeProtocol()} + parent, err := New(parentDef, &fakeProvider{}, testRegistry(), permission.New(permission.ModeDefault, nil)) + if err != nil { + t.Fatalf("New(parentDef): %v", err) + } + parentAgent := parent.(*protocol.Agent) + parentAgent.Budget = agent.NewBudget(1, 0) + + childDef := Definition{Name: "child", Loop: LoopEnforced, Protocol: reactLikeProtocol()} + child, err := NewChild(childDef, parentAgent.Agent, permission.New(permission.ModeDefault, nil)) + if err != nil { + t.Fatalf("NewChild: %v", err) + } + child.Budget.RecordRoundTrip() + if !parentAgent.Budget.Exhausted() { + t.Fatalf("a round-trip recorded via the child must exhaust the shared parent budget") + } +} + +func TestNewChildRejectsLoopBare(t *testing.T) { + parentDef := Definition{Name: "parent", Loop: LoopEnforced, Protocol: reactLikeProtocol()} + parent, err := New(parentDef, &fakeProvider{}, testRegistry(), permission.New(permission.ModeDefault, nil)) + if err != nil { + t.Fatalf("New(parentDef): %v", err) + } + parentAgent := parent.(*protocol.Agent) + + childDef := Definition{Name: "child", Loop: LoopBare} + if _, err := NewChild(childDef, parentAgent.Agent, permission.New(permission.ModeDefault, nil)); err == nil { + t.Fatalf("NewChild with LoopBare: got nil error, want one rejecting the mismatch") + } +} + +func TestNewProtocolAgentRejectsLoopBare(t *testing.T) { + def := Definition{Name: "test-bare", Loop: LoopBare} + _, err := NewProtocolAgent(def, &fakeProvider{}, testRegistry(), permission.New(permission.ModeDefault, nil)) + if err == nil { + t.Fatalf("NewProtocolAgent with LoopBare: got nil error, want one rejecting the mismatch") + } +} + +// reactLikeProtocol is a minimal three-section protocol shaped like +// react.Protocol() (two thinking sections plus a trailing result), built +// locally instead of importing internal/agent/react so this test doesn't +// depend on that package's exact section names. +func reactLikeProtocol() protocol.Protocol { + return protocol.Protocol{ + Name: "test-react-like", + Sections: []protocol.Section{ + {Name: "obs", Kind: protocol.KindThinking}, + {Name: "thoughts", Kind: protocol.KindThinking}, + {Name: "result", Kind: protocol.KindResult}, + }, + } +} diff --git a/internal/agent/definition/builtin.go b/internal/agent/definition/builtin.go new file mode 100644 index 0000000..fcd48a4 --- /dev/null +++ b/internal/agent/definition/builtin.go @@ -0,0 +1,73 @@ +package definition + +import ( + "github.com/mchalapuk/coded/internal/agent" + "github.com/mchalapuk/coded/internal/agent/explorer" + "github.com/mchalapuk/coded/internal/agent/react" +) + +// Main is coded's default agent, reproducing today's hardcoded wiring +// exactly: agent.DefaultIdentity, react.Protocol(), no tool restriction of +// its own (Tools is nil -- whatever registry the caller passes to New), and +// the loop's own default iteration cap (MaxIterations left at zero). Model +// is left empty on purpose: cmd/coded +// assigns it from session config after construction, the same way it does +// today, rather than a Definition baking in a model choice for the one +// agent whose model the user actually configures. +var Main = Definition{ + Name: "main", + Description: "coded's default agent: reads and edits files, runs shell commands, and explains its reasoning concisely.", + Identity: agent.DefaultIdentity, + Protocol: react.Protocol(), + Loop: LoopEnforced, +} + +// Explorer is coded's read-only investigation sub-agent: searches the +// codebase for something specific and reports back file:line pointers +// rather than prose (see explorer.Identity). Tools is narrowed to the +// read-only set -- Read, Grep, Glob, no Write/Edit/Bash -- which is the +// whole mechanism keeping it read-only; its calls are permissioned like any +// other agent's, through the one session-wide permission.Engine (see the +// singleton-permission-mode ADR). MaxIterations is lower than Main's +// default: an investigation that hasn't converged in 20 round-trips is a +// sign the question was too broad for one spawn, not a reason to keep +// going. SeedReadme is true: an explorer benefits from repo orientation +// more than a narrow one-shot child would (see Definition.SeedReadme). +var Explorer = Definition{ + Name: "explorer", + Description: "read-only sub-agent that searches the codebase for something specific and reports back file:line pointers, not prose.", + Identity: explorer.Identity, + Tools: []string{"Read", "Grep", "Glob"}, + Protocol: explorer.Protocol(), + Loop: LoopEnforced, + MaxIterations: 20, + SeedReadme: true, +} + +// Builtins returns a fresh Registry holding coded's shipped agent +// definitions. Fresh rather than a package-level singleton: matches the +// tool.NewRegistry() / builtin.NewRegistry() pattern this package's own +// Registry mirrors, and keeps a caller free to register its own definitions +// alongside the built-ins without mutating shared state. +func Builtins() *Registry { + r := NewRegistry() + r.Register(Main) + r.Register(Explorer) + return r +} + +// Spawnable returns a fresh Registry holding the definitions valid as a +// Spawn target (the built-in Spawn tool's Definitions field, and the TUI's +// /spawn command) -- Builtins() minus Main. A nested unrestricted agent +// isn't a capability this milestone means to expose: spawning "main" would +// be structurally harmless (narrowing can only equal or shrink what a +// child gets, never widen it -- see ResolveTools), but it's not a +// documented, intended use of Spawn either, so it's simply not offered as +// a choice. The one place this list is decided, so cmd/coded's wiring and +// the TUI's /spawn command can't quietly diverge on which definitions are +// spawnable. +func Spawnable() *Registry { + r := NewRegistry() + r.Register(Explorer) + return r +} diff --git a/internal/agent/definition/definition.go b/internal/agent/definition/definition.go new file mode 100644 index 0000000..d1f391c --- /dev/null +++ b/internal/agent/definition/definition.go @@ -0,0 +1,93 @@ +// Package definition turns "what is this agent" into data: a Definition +// holds the identity, tool subset, protocol, and loop that today's +// cmd/coded wires by hand into a single hardcoded agent. New reads a +// Definition and builds the concrete agent it describes; Registry is where +// named Definitions (built-in today, loaded from disk in a later milestone) +// live so a caller -- cmd/coded, and eventually the Spawn tool -- can look +// one up by name instead of constructing it inline. +// +// A Definition declares no permission posture. Permission mode belongs to +// the session, not to any one agent: it is held by the one permission.Engine +// the whole tree shares, written only in response to explicit user input, +// and never bracketed around a spawn (see the singleton-permission-mode +// ADR). Tools is the one field a child narrows against its parent's actual +// set (see ResolveTools) -- naming a tool the parent doesn't have is a +// no-op, never a grant. +package definition + +import ( + "github.com/mchalapuk/coded/internal/agent/protocol" +) + +// Loop selects which turn loop a Definition runs under. Both are real, +// present-day types: LoopEnforced drives protocol.Agent (format-violation +// detection, the synthetic README read, the required-sections contract), +// LoopBare drives the plain agent.Agent loop that has none of that -- +// present since v0.1 for a caller that wants tool calls with no +// response-format ceremony, and kept rather than folded away so a future +// definition can still choose it explicitly. +type Loop int + +const ( + // LoopEnforced runs the agent under a Protocol: required sections, + // violation retry, the synthetic README read. This is every built-in + // definition's loop today. + LoopEnforced Loop = iota + // LoopBare runs the agent under agent.Agent's own Run: tool calls with + // no response-format contract on top. + LoopBare +) + +// Definition is the configured identity of one named agent: what +// agent.Agent's exported fields (System, Model, MaxIterations) and +// protocol.Agent's Protocol are set to before a turn ever runs, plus the +// tool subset it's allowed relative to whoever spawned it. +type Definition struct { + // Name is how this Definition is looked up in a Registry and how the + // Spawn tool's schema names it to the model. + Name string + // Description is a one-line summary of what the agent is for -- shown + // by a future `coded agents list` / `/agents`, and, once Spawn exists, + // in its tool schema so the model can choose the right agent by name. + Description string + // Identity is the agent's system-prompt preamble, assigned to the + // built agent's System field. Corresponds to agent.DefaultIdentity for + // the built-in main agent. + Identity string + // Tools restricts which tools from the registry passed to New are + // available to this agent: nil means no restriction of its own (every + // tool the caller's registry contains); a non-nil list means exactly + // those names, resolved via tool.Registry.Subset. This is the + // definition's own declared restriction, not yet narrowed against a + // parent -- see ResolveTools for how a child's own list and its + // parent's already-resolved list combine when spawning. + Tools []string + // Model, if non-empty, is assigned to the built agent's Model field. + // Left empty for the main agent, whose model comes from session + // config instead -- New only assigns when non-empty, so an empty + // Definition.Model leaves the caller free to set it directly. + Model string + // Protocol is the response-format contract this agent's turns enforce + // when Loop is LoopEnforced. Ignored when Loop is LoopBare. + Protocol protocol.Protocol + // Loop selects the turn loop this Definition runs under; see Loop. + Loop Loop + // MaxIterations bounds provider round-trips within a single Run call. + // Zero falls back to the loop's own default (50), the same behavior + // the underlying agent.Agent/protocol.Agent apply when never set + // explicitly. + MaxIterations int + // SeedReadme declares whether this agent should get the synthetic + // first-turn README read (protocol.Agent.Readme / SyntheticReadmeExchange) + // when it's spawned as a child. New does not act on this field itself -- + // wiring .Readme needs a working directory, which only the spawning + // caller (Spawn) has, the same reason ProjectContext and PersistRule are + // assigned after construction rather than carried on Definition. It is + // also not consulted for the root agent: cmd/coded wires main's .Readme + // directly today and continues to, regardless of this field's value on + // Main. A sub-agent whose job is investigation (explorer) benefits from + // the repo orientation a README gives it; a narrow one-shot child spawned + // to answer something specific pays the extra turn for no benefit, so + // this is per-definition rather than a single yes/no for every spawn. + SeedReadme bool +} diff --git a/internal/agent/definition/narrow.go b/internal/agent/definition/narrow.go new file mode 100644 index 0000000..48860c0 --- /dev/null +++ b/internal/agent/definition/narrow.go @@ -0,0 +1,30 @@ +package definition + +// ResolveTools computes the effective tool list for a child spawned under +// parent, given the child's own declared restriction. nil is the +// unrestricted sentinel at both ends: a nil parent means "whatever the +// registry contains" and a nil child means "no restriction of its own", so +// nil-nil resolves to nil (still unrestricted) and either side supplying a +// list narrows to it. When both supply a list, the result is their +// intersection, order taken from child -- a child's own list selects from +// what its parent actually has, so naming a tool the parent lacks is simply +// a no-op, never a grant. +func ResolveTools(parent, child []string) []string { + if child == nil { + return parent + } + if parent == nil { + return child + } + allowed := make(map[string]bool, len(parent)) + for _, name := range parent { + allowed[name] = true + } + var resolved []string + for _, name := range child { + if allowed[name] { + resolved = append(resolved, name) + } + } + return resolved +} diff --git a/internal/agent/definition/narrow_test.go b/internal/agent/definition/narrow_test.go new file mode 100644 index 0000000..6217c72 --- /dev/null +++ b/internal/agent/definition/narrow_test.go @@ -0,0 +1,49 @@ +package definition + +import ( + "reflect" + "testing" +) + +func TestResolveToolsBothNilStaysUnrestricted(t *testing.T) { + if got := ResolveTools(nil, nil); got != nil { + t.Fatalf("ResolveTools(nil, nil) = %v, want nil", got) + } +} + +func TestResolveToolsChildNarrowsUnrestrictedParent(t *testing.T) { + got := ResolveTools(nil, []string{"Read", "Grep"}) + want := []string{"Read", "Grep"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ResolveTools(nil, %v) = %v, want %v", want, got, want) + } +} + +func TestResolveToolsChildInheritsParentRestriction(t *testing.T) { + parent := []string{"Read", "Grep", "Glob"} + got := ResolveTools(parent, nil) + if !reflect.DeepEqual(got, parent) { + t.Fatalf("ResolveTools(%v, nil) = %v, want %v", parent, got, parent) + } +} + +// TestResolveToolsCannotWiden is the property this whole function exists +// for: a child naming a tool its parent lacks must never gain it. Write is +// absent from parent, so it must be absent from the result no matter that +// the child asked for it. +func TestResolveToolsCannotWiden(t *testing.T) { + parent := []string{"Read", "Grep"} + child := []string{"Grep", "Write"} + got := ResolveTools(parent, child) + want := []string{"Grep"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("ResolveTools(%v, %v) = %v, want %v (Write must be dropped, not granted)", parent, child, got, want) + } +} + +func TestResolveToolsDisjointYieldsEmpty(t *testing.T) { + got := ResolveTools([]string{"Read"}, []string{"Write"}) + if len(got) != 0 { + t.Fatalf("ResolveTools disjoint sets = %v, want empty", got) + } +} diff --git a/internal/agent/definition/registry.go b/internal/agent/definition/registry.go new file mode 100644 index 0000000..03f211c --- /dev/null +++ b/internal/agent/definition/registry.go @@ -0,0 +1,41 @@ +package definition + +// Registry holds named Definitions, preserving registration order the same +// way tool.Registry preserves tool order -- for the same reason: List's +// order is what a future `/agents` listing and Spawn's tool-schema enum +// show the model, and that's worth being a deliberate choice at the +// registration site rather than an alphabetical accident. +type Registry struct { + order []string + defs map[string]Definition +} + +// NewRegistry creates an empty Registry. +func NewRegistry() *Registry { + return &Registry{defs: make(map[string]Definition)} +} + +// Register adds d to the registry, keyed by d.Name. Registering a name +// already present overwrites the previous entry in place, without changing +// its position in registration order. +func (r *Registry) Register(d Definition) { + if _, exists := r.defs[d.Name]; !exists { + r.order = append(r.order, d.Name) + } + r.defs[d.Name] = d +} + +// Get looks up a Definition by name. +func (r *Registry) Get(name string) (Definition, bool) { + d, ok := r.defs[name] + return d, ok +} + +// List returns all registered Definitions in registration order. +func (r *Registry) List() []Definition { + out := make([]Definition, 0, len(r.order)) + for _, name := range r.order { + out = append(out, r.defs[name]) + } + return out +} diff --git a/internal/agent/definition/registry_test.go b/internal/agent/definition/registry_test.go new file mode 100644 index 0000000..002c262 --- /dev/null +++ b/internal/agent/definition/registry_test.go @@ -0,0 +1,85 @@ +package definition + +import "testing" + +func TestRegistryGetMissing(t *testing.T) { + r := NewRegistry() + if _, ok := r.Get("nope"); ok { + t.Fatalf("Get on empty registry: got ok=true, want false") + } +} + +func TestRegistryListPreservesRegistrationOrder(t *testing.T) { + r := NewRegistry() + r.Register(Definition{Name: "b"}) + r.Register(Definition{Name: "a"}) + r.Register(Definition{Name: "c"}) + got := r.List() + if len(got) != 3 || got[0].Name != "b" || got[1].Name != "a" || got[2].Name != "c" { + t.Fatalf("List() = %v, want order [b a c]", got) + } +} + +func TestRegistryReRegisterOverwritesInPlace(t *testing.T) { + r := NewRegistry() + r.Register(Definition{Name: "a", Description: "first"}) + r.Register(Definition{Name: "b"}) + r.Register(Definition{Name: "a", Description: "second"}) + got := r.List() + if len(got) != 2 || got[0].Name != "a" || got[0].Description != "second" || got[1].Name != "b" { + t.Fatalf("List() = %+v, want [a(second) b], position unchanged on re-register", got) + } +} + +func TestBuiltinsRegistersMain(t *testing.T) { + d, ok := Builtins().Get("main") + if !ok { + t.Fatalf("Builtins() has no \"main\" definition") + } + if d.Tools != nil { + t.Errorf("main.Tools = %v, want nil (unrestricted)", d.Tools) + } + if d.Loop != LoopEnforced { + t.Errorf("main.Loop = %v, want LoopEnforced", d.Loop) + } + if d.Identity == "" { + t.Errorf("main.Identity is empty") + } +} + +// TestBuiltinsRegistersExplorer asserts explorer's exact tool allowlist, +// not just the absence of known-mutating names: with PermissionMode gone +// (see the singleton-permission-mode ADR), this list is the entire +// mechanism keeping explorer read-only, so it must catch an unexpected +// addition, not just a known-bad one. +func TestBuiltinsRegistersExplorer(t *testing.T) { + d, ok := Builtins().Get("explorer") + if !ok { + t.Fatalf(`Builtins() has no "explorer" definition`) + } + want := []string{"Read", "Grep", "Glob"} + if len(d.Tools) != len(want) { + t.Fatalf("explorer.Tools = %v, want exactly %v", d.Tools, want) + } + for i, name := range want { + if d.Tools[i] != name { + t.Errorf("explorer.Tools[%d] = %q, want %q", i, d.Tools[i], name) + } + } + if d.MaxIterations == 0 || d.MaxIterations >= 50 { + t.Errorf("explorer.MaxIterations = %d, want a positive bound below Main's default", d.MaxIterations) + } +} + +// TestSpawnableExcludesMain guards the property Spawn's whole safety +// argument depends on cmd/coded and the TUI's /spawn both getting for +// free by sharing this one list: a nested unrestricted agent is never an +// offered choice, even though nothing would technically break if it were. +func TestSpawnableExcludesMain(t *testing.T) { + if _, ok := Spawnable().Get("main"); ok { + t.Fatalf("Spawnable() includes \"main\", which must never be a Spawn target") + } + if _, ok := Spawnable().Get("explorer"); !ok { + t.Fatalf("Spawnable() is missing \"explorer\"") + } +} diff --git a/internal/agent/event.go b/internal/agent/event.go index 00aeb8b..16ab3fd 100644 --- a/internal/agent/event.go +++ b/internal/agent/event.go @@ -11,8 +11,8 @@ import ( // EventType discriminates Event payloads emitted while running a turn (see // Agent.StreamTurn, Agent.DispatchToolCalls, and Agent.Run, which adds -// EventTurnComplete/EventError on top). These are UI-agnostic: the TUI and -// the one-shot renderer both consume the same stream. +// EventTurnEnded on top). These are UI-agnostic: the TUI and the one-shot +// renderer both consume the same stream. type EventType string const ( @@ -46,11 +46,23 @@ const ( // run continues after this event, it is not terminal. See // llm.WithRetry. EventRetrying EventType = "retrying" - // EventTurnComplete announces the agent has produced a final answer (no - // more tool calls pending) and is waiting for the next user input. - EventTurnComplete EventType = "turn_complete" - // EventError carries a terminal error; the run ends after this event. - EventError EventType = "error" + // EventTurnEnded announces the turn is over -- replacing the old + // EventTurnComplete/EventError split with one event carrying exactly + // one Outcome (see Event.Result and TurnResult), rather than a + // separate event for the success and failure cases. Every turn ends + // with exactly one of these; the run ends after this event either way. + EventTurnEnded EventType = "turn_ended" + // EventAgentStarted brackets the start of a sub-agent's run, carrying + // the same AgentName/AgentDepth its own events will carry (see + // Event.AgentName) so a consumer can match it against the + // EventAgentComplete that closes the same group. Emitted by whatever + // spawns the sub-agent (see the Spawn tool), not by the sub-agent + // itself -- a fresh agent has no way to know it was spawned, let alone + // under what name. + EventAgentStarted EventType = "agent_started" + // EventAgentComplete closes the group EventAgentStarted opened, once + // the sub-agent's own event channel has closed. + EventAgentComplete EventType = "agent_complete" ) // Event is one item in the stream returned by Agent.Run (or @@ -60,6 +72,33 @@ const ( type Event struct { Type EventType + // AgentName identifies which agent in the tree produced this event: + // the full chain from the outermost agent to this one, joined by "/" + // -- "main" for the root agent's own events (every event a bare + // Agent.Run/protocol.Agent.Run call produces, unless its Agent.Name + // was left at its zero value), "main/explorer" for a direct child, + // and so on for deeper nesting. Copied from the producing Agent's own + // Name once, by Run itself (see Agent.Run/stampIdentity), not + // rewritten as the event bubbles up through a parent's forwarding -- + // so none of StreamTurn, DispatchToolCalls, or any other internal + // emission site needs to know it exists. + AgentName string + // AgentDepth is AgentName's chain length minus one (0 at the root), + // copied from the producing Agent's own Depth alongside AgentName. + // Kept as its own field, not derived by splitting AgentName, so + // depth-based rendering (indentation) never has to parse it back + // apart. + AgentDepth int + + // Agent carries the sub-agent's name and a summary of the prompt it + // was given. Set only on EventAgentStarted; nil otherwise. + Agent *AgentInfo + + // Result carries how the turn ended. Set only on EventTurnEnded; nil + // otherwise. See TurnResult and Err below for where the error a failed + // turn carried under the old EventError now lives. + Result *TurnResult + // Text carries an incremental chunk of assistant output: the visible // reply on EventTextDelta, or extended-thinking content on // EventThinkingDelta. Empty for every other Type. @@ -96,11 +135,12 @@ type Event struct { // EventUsage; nil otherwise. Usage *llm.Usage - // Err carries the failure behind the event: on EventError it's - // terminal, the run has already ended and Err is the reason; on - // EventRetrying it's the error that triggered the retry the run is - // about to attempt, and the run continues afterward. Nil for every - // other Type. + // Err carries the failure behind the event: on EventTurnEnded with + // Result.Outcome of OutcomeErrored or OutcomeExhausted, it's the error + // that ended the turn (the run has already ended); on EventRetrying + // it's the error that triggered the retry the run is about to attempt, + // and the run continues afterward. Nil for every other Type, including + // EventTurnEnded with any other Outcome. Err error // RetryAttempt, RetryMaxAttempts, and RetryDelay describe the retry @@ -112,6 +152,19 @@ type Event struct { RetryDelay time.Duration } +// AgentInfo describes a sub-agent whose run is bracketed by +// EventAgentStarted/EventAgentComplete. +type AgentInfo struct { + // Name is the spawned agent's definition name (e.g. "explorer") -- the + // last "/"-separated segment of that agent's own events' AgentName. + Name string + // PromptSummary is a short, single-line summary of the prompt the + // sub-agent was given -- the whole prompt can be long and is already + // visible in the sub-agent's own first turn once expanded, so this is + // what a renderer shows in a collapsed group's header. + PromptSummary string +} + // ToolCallInfo describes a tool call the agent is about to evaluate/execute. type ToolCallInfo struct { ID string diff --git a/internal/agent/explorer/protocol.go b/internal/agent/explorer/protocol.go new file mode 100644 index 0000000..f43cd36 --- /dev/null +++ b/internal/agent/explorer/protocol.go @@ -0,0 +1,65 @@ +// Package explorer holds the response-format contract and identity text for +// coded's built-in explorer sub-agent: a read-only agent spawned to search +// the codebase for something specific and report back pointers, not prose. +// See internal/agent/react for the shape this package mirrors -- a Protocol +// plus the doc comment explaining it -- and PLAN.md's Sub-agents section for +// why the explorer needs its own protocol rather than react's: react's +// is free-form, and a free-form answer from a sub-agent defeats the +// point of spawning one (see Identity's doc comment). +package explorer + +import ( + "github.com/mchalapuk/coded/internal/agent/protocol" +) + +// Identity is the explorer's system-prompt preamble. Its whole job is +// stated plainly here rather than left to be inferred from the protocol's +// section descriptions: a parent that spawns a sub-agent gets back its +// section and nothing else -- not the tool calls, not the +// intermediate reasoning -- so an explorer that answers in prose forces the +// parent to re-read every file it names just to verify the claim, which +// spends exactly the context spawning was supposed to save. Pointers +// (file:line plus a one-line claim) let the parent trust the claim or check +// it cheaply; a narrative doesn't. +const Identity = `You are an explorer sub-agent: you search a codebase for something specific ` + + `and report back where it is, not a summary of what you read. You have read-only tools ` + + `(Read, Grep, Glob) -- no edits, no shell commands. Your entire answer becomes another ` + + `agent's only view of what you found: it will not re-run your searches or re-read the files ` + + `you looked at, so an answer written as prose forces it to redo your work to verify anything ` + + `you claim. Report file:line pointers with a one-line claim about what's there, not a narrative ` + + `of your investigation. Report explicitly what you searched for and did not find -- that ` + + `negative result is what stops the caller from repeating a search that already came up empty.` + +// Protocol is the explorer's response-format contract: a preamble +// every response opens with (KindThinking, the same role react's +// / play -- stating what's being looked for and where +// it's already been looked, so a tool-call turn is never just a bare tool +// call with no stated intent), and a trailing section +// (KindResult) carrying the actual answer: file:line pointers with claims, +// plus what was searched for and not found. One result section, not two -- +// see protocol.Protocol's doc comment on why exactly one KindResult section +// is the whole package's invariant -- so "found" and "not found" are one +// section's two halves rather than two separately-enforced sections; Identity +// and findings' Description are what ask the model to structure it that way, +// since the section contract itself only enforces presence, not internal +// shape. +func Protocol() protocol.Protocol { + return protocol.Protocol{ + Name: "explorer", + Sections: []protocol.Section{ + { + Name: "search", Kind: protocol.KindThinking, + Description: "stating what you're looking for, where you've already looked, and what you still expect to check", + }, + { + Name: "findings", Kind: protocol.KindResult, + Description: "your findings as file:line pointers with a one-line claim each, followed by what you searched for and did not find", + }, + }, + SyntheticRead: func(filePath string) []string { + return []string{ + "I have not yet read " + filePath + ".", + } + }, + } +} diff --git a/internal/agent/explorer/protocol_test.go b/internal/agent/explorer/protocol_test.go new file mode 100644 index 0000000..b7b631b --- /dev/null +++ b/internal/agent/explorer/protocol_test.go @@ -0,0 +1,71 @@ +package explorer + +import "testing" + +// TestProtocolPromptTextUnchanged pins PromptText()'s generated output for +// the shipped protocol -- a model-facing regression guard, the same role +// react's own pinned test plays. Explorer is the first shipped protocol with +// a single required non-result section, which is worth pinning on its own: +// it's what caught requiredSentence's singular/plural bug (see +// protocol/prompt.go's blockWord/all handling) before this protocol ever +// reached a model. +func TestProtocolPromptTextUnchanged(t *testing.T) { + want := `IMPORTANT: Every message you write must open with one tagged block in this order: ` + + `... stating what you're looking for, where you've already looked, and what ` + + `you still expect to check — followed by either the tool call or calls, or your final answer ` + + `wrapped in .... Be sure to start your message with ... ` + + `but use those sections only once per your message. End your message after ` + + `... section. This format is required for all your responses. Responses ` + + `not following the format will be rejected. Always include it, exactly once each, in every response.` + if got := Protocol().PromptText(); got != want { + t.Fatalf("Protocol().PromptText() = %q, want %q", got, want) + } +} + +// TestProtocolReminderTextUnchanged is TestProtocolPromptTextUnchanged's +// reminder-side equivalent. +func TestProtocolReminderTextUnchanged(t *testing.T) { + want := "\nIMPORTANT: Your response must start with a content block " + + "containing ... section, in that order, before either your tool calls or " + + "your final answer, wrapped in ...." + + "\n" + if got := Protocol().ReminderText(); got != want { + t.Fatalf("Protocol().ReminderText() = %q, want %q", got, want) + } +} + +func TestProtocolWrapSectionsRequiredOnly(t *testing.T) { + p := Protocol() + got := p.WrapSections("search content") + want := "\nsearch content\n" + if got != want { + t.Fatalf("WrapSections() = %q, want %q", got, want) + } +} + +func TestProtocolWrapSectionsIncludingOptionalResult(t *testing.T) { + p := Protocol() + got := p.WrapSections("search content", "the findings") + want := "\nsearch content\n\n\nthe findings\n" + if got != want { + t.Fatalf("WrapSections() = %q, want %q", got, want) + } +} + +func TestProtocolValidates(t *testing.T) { + if err := Protocol().Validate(); err != nil { + t.Fatalf("Protocol().Validate() = %v, want nil", err) + } +} + +// TestSyntheticReadMatchesRequiredSectionCount guards the contract +// protocol.Protocol.syntheticReadMessage relies on (see its doc comment): +// SyntheticRead must return exactly one entry per always-required section -- +// here, just , since is the trailing optional result. +func TestSyntheticReadMatchesRequiredSectionCount(t *testing.T) { + p := Protocol() + got := p.SyntheticRead("README.md") + if len(got) != len(p.Sections)-1 { + t.Fatalf("SyntheticRead returned %d entries, want %d (one per required section)", len(got), len(p.Sections)-1) + } +} diff --git a/internal/agent/forward.go b/internal/agent/forward.go new file mode 100644 index 0000000..7e93970 --- /dev/null +++ b/internal/agent/forward.go @@ -0,0 +1,27 @@ +package agent + +// ForwardChild drains child until it closes, relaying every event to emit +// unchanged. A pure relay: child's own Run already stamped AgentName/ +// AgentDepth on every event it produces (see Agent.Run/stampIdentity), so +// there's nothing left for a forwarding hop to rewrite -- whatever the +// child (or something the child itself spawned) put on its own out channel +// is, by construction, everything ForwardChild sees here. +// +// emit is a sink, not a channel, so a caller reached through +// tool.EventReportingTool (see the Spawn tool) can pass a Reporter's +// Report method directly -- the tool package can't import this one to +// accept a chan<- Event parameter without an import cycle (see +// tool.Reporter's doc comment), but a plain func(Event) has no such +// constraint. A channel-based caller wraps trivially: func(ev Event) { +// out <- ev }. +// +// Blocks until child closes. The caller is responsible for whatever emit +// writes to staying open long enough to receive every relayed event, and +// for bracketing the call with EventAgentStarted/EventAgentComplete if +// that's the shape it wants observers to see (see the Spawn tool, +// ForwardChild's only caller). +func ForwardChild(child <-chan Event, emit func(Event)) { + for ev := range child { + emit(ev) + } +} diff --git a/internal/agent/forward_test.go b/internal/agent/forward_test.go new file mode 100644 index 0000000..4cdd8ab --- /dev/null +++ b/internal/agent/forward_test.go @@ -0,0 +1,41 @@ +package agent + +import "testing" + +// TestForwardChildRelaysEventsUnchanged covers ForwardChild's whole job now +// that AgentName/AgentDepth are stamped once by the producing Agent's own +// Run (see stampIdentity): it's a pure relay, so whatever a child's channel +// carries -- including an event that already has AgentName/AgentDepth set, +// as if it bubbled up through a grandchild's own forwarding -- must come out +// the other side identical. +func TestForwardChildRelaysEventsUnchanged(t *testing.T) { + child := make(chan Event, 2) + child <- Event{Type: EventTextDelta, Text: "hello", AgentName: "main/explorer", AgentDepth: 1} + child <- Event{Type: EventTurnEnded, Result: &TurnResult{Outcome: OutcomeCompleted}, AgentName: "main/explorer/explorer", AgentDepth: 2} + close(child) + + var got []Event + ForwardChild(child, func(ev Event) { got = append(got, ev) }) + + if len(got) != 2 { + t.Fatalf("got %d events, want 2", len(got)) + } + if got[0].AgentName != "main/explorer" || got[0].AgentDepth != 1 { + t.Errorf("got[0] AgentName/AgentDepth = %q/%d, want main/explorer/1", got[0].AgentName, got[0].AgentDepth) + } + if got[1].AgentName != "main/explorer/explorer" || got[1].AgentDepth != 2 { + t.Errorf("got[1] AgentName/AgentDepth = %q/%d, want main/explorer/explorer/2", got[1].AgentName, got[1].AgentDepth) + } +} + +func TestForwardChildEmptyChildEmitsNothing(t *testing.T) { + child := make(chan Event) + close(child) + + var got []Event + ForwardChild(child, func(ev Event) { got = append(got, ev) }) + + if len(got) != 0 { + t.Fatalf("got %d events from an empty child, want 0", len(got)) + } +} diff --git a/internal/agent/identity.go b/internal/agent/identity.go new file mode 100644 index 0000000..09b0cbd --- /dev/null +++ b/internal/agent/identity.go @@ -0,0 +1,33 @@ +package agent + +import "context" + +// callerIdentityKey is the context.Context key WithCallerIdentity/ +// CallerIdentityFromContext share; an unexported type so no other package +// can collide with it. +type callerIdentityKey struct{} + +type callerIdentity struct { + Name string + Depth int +} + +// WithCallerIdentity attaches the calling Agent's own Name/Depth to ctx. +// DispatchToolCalls sets this once, ahead of every tool call in a batch, so +// an EventReportingTool (e.g. Spawn) can learn who's calling it without +// needing per-agent fields of its own: the same *spawn.Tool instance is +// shared by reference across the whole tree (tool.Registry.Subset narrows a +// child's registry by reusing, not rebuilding, its parent's tool +// instances), so identity has to travel with the call, not live on the +// tool. +func WithCallerIdentity(ctx context.Context, name string, depth int) context.Context { + return context.WithValue(ctx, callerIdentityKey{}, callerIdentity{Name: name, Depth: depth}) +} + +// CallerIdentityFromContext reads back what WithCallerIdentity attached. ok +// is false if ctx never got one -- a call path that bypasses +// DispatchToolCalls, such as a tool exercised directly in a unit test. +func CallerIdentityFromContext(ctx context.Context) (name string, depth int, ok bool) { + v, ok := ctx.Value(callerIdentityKey{}).(callerIdentity) + return v.Name, v.Depth, ok +} diff --git a/internal/agent/outcome.go b/internal/agent/outcome.go new file mode 100644 index 0000000..7378f0f --- /dev/null +++ b/internal/agent/outcome.go @@ -0,0 +1,102 @@ +package agent + +import ( + "context" + "errors" + "fmt" +) + +// ErrMaxIterationsExceeded is wrapped into the error a turn loop returns +// when it hits its iteration cap without reaching a final answer -- see +// OutcomeForError, which maps it (like ErrBudgetExhausted) to +// OutcomeExhausted rather than OutcomeErrored. +var ErrMaxIterationsExceeded = errors.New("agent: exceeded max iterations") + +// OutcomeForError classifies a terminal turn error into an Outcome: a +// cancelled context is OutcomeCancelled, budget or iteration exhaustion is +// OutcomeExhausted, anything else (a provider failure, a malformed +// response the turn loop couldn't recover from, ...) is OutcomeErrored -- +// the same class of failure the pre-Outcome EventError used to signal on +// its own, now just carried as an Outcome alongside the same error. +func OutcomeForError(err error) Outcome { + if errors.Is(err, context.Canceled) { + return OutcomeCancelled + } + if errors.Is(err, ErrBudgetExhausted) || errors.Is(err, ErrMaxIterationsExceeded) { + return OutcomeExhausted + } + return OutcomeErrored +} + +// Outcome is how a turn ended, carried on EventTurnEnded -- see +// TurnResult and PLAN.md's Turn outcomes section for the full table this +// implements. +type Outcome string + +const ( + // OutcomeCompleted means the turn produced a final answer with nothing + // left open. Every clean finish today ends here. + OutcomeCompleted Outcome = "completed" + // OutcomeAbandoned means the agent stopped deliberately and said why -- + // the goal turned out unachievable, a permission was refused, the plan + // was wrong. Reachable only once a turn has a goal/todo list to + // abandon (see PLAN.md's Goals and plans milestone) -- no turn loop + // produces it yet. + OutcomeAbandoned Outcome = "abandoned" + // OutcomeCancelled means the user (or a caller's context) interrupted + // the turn before it could finish. + OutcomeCancelled Outcome = "cancelled" + // OutcomeExhausted means the harness stopped the turn: the iteration + // cap or a Budget ceiling was hit before a final answer. + OutcomeExhausted Outcome = "exhausted" + // OutcomeErrored means a terminal provider or dispatch failure ended + // the turn -- the same class of failure the pre-Outcome EventError + // used to signal on its own; TurnResult carries the same error Event.Err + // always has. + OutcomeErrored Outcome = "errored" + // OutcomeIncomplete means a final answer arrived with open items and no + // stated reason -- a protocol violation once there's a todo list to + // leave items open on (see PLAN.md's Goals and plans milestone); no + // turn loop produces it yet, for the same reason OutcomeAbandoned + // doesn't. + OutcomeIncomplete Outcome = "incomplete" +) + +// TurnResult carries how a turn ended: Outcome plus, for Abandoned or +// Errored, why. Carried on EventTurnEnded (see Event.Result) and, once fed +// forward (see PLAN.md's Turn outcomes section), injected into the next +// turn's request as context rather than modelled as a new Situation -- +// "user-input-after-abandoned and friends put the combinatorics in the +// wrong place." +type TurnResult struct { + Outcome Outcome + // Reason is the agent's own stated reason for OutcomeAbandoned, or a + // short harness-authored explanation for OutcomeExhausted/OutcomeErrored + // (e.g. "exceeded max iterations (50)"). Empty for OutcomeCompleted. + Reason string + // OpenItems names whatever's left when Outcome is Incomplete or + // Abandoned. Always empty today -- no turn loop tracks a todo list yet + // (see PLAN.md's Goals and plans milestone). + OpenItems []string +} + +// previousOutcomeBlock renders r as a short note for the message that opens +// the next turn (see Agent.LastOutcome, NewTurnMessage), so the model knows +// the prior turn didn't finish cleanly instead of silently continuing as if +// history simply had nothing more to do. Nil r, or a Completed outcome, +// renders "" -- an ordinary clean finish needs no forward context, which is +// also what keeps this silent for the overwhelming majority of turns in a +// normal session. +func previousOutcomeBlock(r *TurnResult) string { + if r == nil || r.Outcome == OutcomeCompleted { + return "" + } + reason := r.Reason + if reason == "" { + reason = string(r.Outcome) + } + return fmt.Sprintf( + "\nThe previous turn did not complete normally -- outcome: %s. %s\n", + r.Outcome, reason, + ) +} diff --git a/internal/agent/outcome_test.go b/internal/agent/outcome_test.go new file mode 100644 index 0000000..5eff5ec --- /dev/null +++ b/internal/agent/outcome_test.go @@ -0,0 +1,150 @@ +package agent + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/mchalapuk/coded/internal/llm" + "github.com/mchalapuk/coded/internal/permission" + "github.com/mchalapuk/coded/internal/tool" +) + +func TestPreviousOutcomeBlockNilIsEmpty(t *testing.T) { + if got := previousOutcomeBlock(nil); got != "" { + t.Errorf("previousOutcomeBlock(nil) = %q, want \"\"", got) + } +} + +func TestPreviousOutcomeBlockCompletedIsEmpty(t *testing.T) { + if got := previousOutcomeBlock(&TurnResult{Outcome: OutcomeCompleted}); got != "" { + t.Errorf("previousOutcomeBlock(Completed) = %q, want \"\" -- a clean finish needs no forward context", got) + } +} + +func TestPreviousOutcomeBlockNonCompletedNamesOutcomeAndReason(t *testing.T) { + got := previousOutcomeBlock(&TurnResult{Outcome: OutcomeExhausted, Reason: "budget exhausted"}) + if !strings.Contains(got, "exhausted") || !strings.Contains(got, "budget exhausted") { + t.Errorf("previousOutcomeBlock(Exhausted) = %q, want it to name both the outcome and the reason", got) + } +} + +// TestRunInjectsPreviousOutcomeIntoNextTurnsRequest drives the real +// end-to-end path: a first Run call that halts on OutcomeExhausted (a +// round-trip budget of 1 forces a tool-calling turn to stop before it +// finishes), then a second Run call, and checks the second call's outgoing +// request actually carries the previous-outcome block -- not just that the +// helper function renders correctly in isolation. +func TestRunInjectsPreviousOutcomeIntoNextTurnsRequest(t *testing.T) { + fp := &fakeProvider{turns: [][]llm.Event{ + toolTurn("t1", "Echo", `{}`), + textTurn("second turn's reply"), + }} + reg := tool.NewRegistry() + reg.Register(echoTool{}) + a := New(fp, reg, permission.New(permission.ModeDefault, nil)) + a.Budget = NewBudget(1, 0) + + drain(t, a.Run(context.Background(), "first"), 2*time.Second) + if a.LastOutcome == nil || a.LastOutcome.Outcome != OutcomeExhausted { + t.Fatalf("LastOutcome after the first Run = %+v, want OutcomeExhausted", a.LastOutcome) + } + + // A fresh budget for the second call -- otherwise it would immediately + // halt again, and there would be no second request to inspect. + a.Budget = nil + drain(t, a.Run(context.Background(), "second"), 2*time.Second) + + if len(fp.requests) < 2 { + t.Fatalf("got %d requests, want at least 2 (first call + second call)", len(fp.requests)) + } + secondReq := fp.requests[1] + found := false + for _, m := range secondReq.Messages { + for _, c := range m.Content { + if strings.Contains(c.Text, "previous-turn-outcome") && strings.Contains(c.Text, "exhausted") { + found = true + } + } + } + if !found { + t.Fatalf("second request does not carry the previous-outcome block: %+v", secondReq.Messages) + } +} + +// TestPreviousOutcomeStoredOnceNotReinjectedPerIteration confirms the +// previous-outcome note lands on the opening user message of the next turn +// as ordinary, non-volatile history -- not re-injected on every request a +// multi-iteration turn makes, and not marked Volatile (which would keep it +// out of the cached prefix on every subsequent request for no reason, since +// unlike ProjectContext it never changes within a turn). +func TestPreviousOutcomeStoredOnceNotReinjectedPerIteration(t *testing.T) { + fp := &fakeProvider{turns: [][]llm.Event{ + toolTurn("t1", "Echo", `{}`), // first Run: halts on budget + toolTurn("t2", "Echo", `{}`), // second Run, iter 0: tool call + textTurn("final reply"), // second Run, iter 1: finishes + }} + reg := tool.NewRegistry() + reg.Register(echoTool{}) + a := New(fp, reg, permission.New(permission.ModeDefault, nil)) + a.Budget = NewBudget(1, 0) + + drain(t, a.Run(context.Background(), "first"), 2*time.Second) + if a.LastOutcome == nil || a.LastOutcome.Outcome != OutcomeExhausted { + t.Fatalf("LastOutcome after the first Run = %+v, want OutcomeExhausted", a.LastOutcome) + } + + // A fresh budget so the second call's two iterations both go through. + a.Budget = nil + drain(t, a.Run(context.Background(), "second"), 2*time.Second) + + if len(fp.requests) < 3 { + t.Fatalf("got %d requests, want at least 3 (first call + second call's 2 iterations)", len(fp.requests)) + } + + for _, reqIdx := range []int{1, 2} { + req := fp.requests[reqIdx] + count := 0 + for _, m := range req.Messages { + for _, c := range m.Content { + if strings.Contains(c.Text, "previous-turn-outcome") { + count++ + if c.Volatile { + t.Errorf("request %d: previous-outcome block is marked Volatile, want it stored as ordinary history", reqIdx) + } + } + } + } + if count != 1 { + t.Errorf("request %d carries the previous-outcome block %d times, want exactly 1", reqIdx, count) + } + } +} + +// TestRunDoesNotInjectPreviousOutcomeAfterACleanFinish confirms an ordinary +// successful turn leaves nothing for the next one to carry forward -- the +// common case, and the one that must generate zero extra tokens. +func TestRunDoesNotInjectPreviousOutcomeAfterACleanFinish(t *testing.T) { + fp := &fakeProvider{turns: [][]llm.Event{ + textTurn("first turn's reply"), + textTurn("second turn's reply"), + }} + a := New(fp, tool.NewRegistry(), permission.New(permission.ModeDefault, nil)) + + drain(t, a.Run(context.Background(), "first"), 2*time.Second) + if a.LastOutcome == nil || a.LastOutcome.Outcome != OutcomeCompleted { + t.Fatalf("LastOutcome after a clean finish = %+v, want OutcomeCompleted", a.LastOutcome) + } + + drain(t, a.Run(context.Background(), "second"), 2*time.Second) + + secondReq := fp.requests[1] + for _, m := range secondReq.Messages { + for _, c := range m.Content { + if strings.Contains(c.Text, "previous-turn-outcome") { + t.Fatalf("second request unexpectedly carries a previous-outcome block after a clean finish: %q", c.Text) + } + } + } +} diff --git a/internal/agent/planning/protocol.go b/internal/agent/planning/protocol.go new file mode 100644 index 0000000..9f4fe02 --- /dev/null +++ b/internal/agent/planning/protocol.go @@ -0,0 +1,54 @@ +// Package planning holds the response-format contract for the planning +// pass: a separate completion, run once at the start of a turn (see +// protocol.Agent's Planning field), that states the turn's goal, its todo +// list, and which items (if any) should be delegated to a sub-agent -- +// PLAN.md's Goals and plans milestone. See internal/agent/react and +// internal/agent/explorer for the sibling protocols this package's own +// shape mirrors. +package planning + +import "github.com/mchalapuk/coded/internal/agent/protocol" + +// PromptSuffix explains what the three sections are for, appended after +// the generated per-section sentence Protocol.PromptText() produces from +// Sections' own Description fields -- the same split react.Protocol() +// makes between what the template can say mechanically and what needs +// stating in the model's own terms. +const PromptSuffix = `Read the goal against the whole conversation so far, not just the latest message -- ` + + `re-deriving it from only what was just said is how "add caching to the repository layer" quietly ` + + `becomes "make the test pass". State it once; it does not change for the rest of this turn. The todo ` + + `list is soft: a genuinely one-step turn can say "none". For delegate, name any todo items broad ` + + `enough to hand to a sub-agent via Spawn (many candidate files, an unknown location, an answer that's ` + + `a pointer rather than something to read in full) -- and if none qualify, say so plainly rather than ` + + `leaving the question unanswered.` + +// Protocol is the planning pass's response-format contract: three +// KindDecision sections, goal/todo/delegate, in that order. Unlike +// react.Protocol() or explorer.Protocol(), this declares no KindResult +// section -- the planning pass is never run through protocol.NewAgent's +// full enforcement loop (mid-stream violation retry, MissingSections), so +// Protocol.Validate's one-KindResult invariant doesn't apply to it; it's +// parsed directly (see protocol.Protocol.Parse) from a single best-effort +// completion instead. See protocol.Agent's Planning field doc comment for +// why that's a deliberate, smaller commitment than fully enforcing a +// second protocol per iteration would be. +func Protocol() protocol.Protocol { + return protocol.Protocol{ + Name: "planning", + Sections: []protocol.Section{ + { + Name: "goal", Kind: protocol.KindDecision, + Description: "one sentence stating what this turn is to achieve", + }, + { + Name: "todo", Kind: protocol.KindDecision, + Description: "a numbered list of steps toward the goal, or \"none\" for a genuinely one-step turn", + }, + { + Name: "delegate", Kind: protocol.KindDecision, + Description: "which todo items (if any) should be handed to a sub-agent via Spawn, and why -- or state plainly why none should be", + }, + }, + PromptSuffix: PromptSuffix, + } +} diff --git a/internal/agent/planning/protocol_test.go b/internal/agent/planning/protocol_test.go new file mode 100644 index 0000000..b85bacd --- /dev/null +++ b/internal/agent/planning/protocol_test.go @@ -0,0 +1,40 @@ +package planning + +import ( + "strings" + "testing" +) + +func TestProtocolSectionNamesAndKinds(t *testing.T) { + p := Protocol() + want := []struct { + name string + }{{"goal"}, {"todo"}, {"delegate"}} + if len(p.Sections) != len(want) { + t.Fatalf("Protocol().Sections = %+v, want %d sections", p.Sections, len(want)) + } + for i, w := range want { + if p.Sections[i].Name != w.name { + t.Errorf("Sections[%d].Name = %q, want %q", i, p.Sections[i].Name, w.name) + } + } +} + +// TestProtocolHasNoResultSection guards a deliberate property: unlike +// react.Protocol()/explorer.Protocol(), planning's own protocol declares no +// KindResult section, since it's never run through protocol.NewAgent's +// Validate-enforcing turn loop -- see Protocol()'s own doc comment. +func TestProtocolHasNoResultSection(t *testing.T) { + if _, ok := Protocol().ResultSection(); ok { + t.Fatalf("Protocol().ResultSection() found one, want none") + } +} + +func TestProtocolPromptTextMentionsAllThreeTags(t *testing.T) { + got := Protocol().PromptText() + for _, tag := range []string{"", "", ""} { + if !strings.Contains(got, tag) { + t.Errorf("PromptText() = %q, want it to mention %q", got, tag) + } + } +} diff --git a/internal/agent/protocol/agent.go b/internal/agent/protocol/agent.go index fc2ac33..8ab5e86 100644 --- a/internal/agent/protocol/agent.go +++ b/internal/agent/protocol/agent.go @@ -48,9 +48,34 @@ type Agent struct { // README.md found" the same way as one that simply never asked. Readme func() ReadmeInfo + // Planning, if set, runs once at the start of every turn (before the + // main loop's own iteration 0) as a separate, best-effort completion + // under this Protocol -- typically planning.Protocol() -- producing a + // goal, todo list, and delegation decision (see PlanState) injected + // into every later request this same turn (see RequestMessages, + // planBlock). Nil (the default -- no existing built-in definition sets + // this) disables the pass entirely; every agent's behavior is + // unchanged from before this field existed. + // + // Deliberately not enforced the way a.proto is: no live mid-stream + // guard, no violation retry, no tool dispatch even if the model tries + // one. A malformed or missing response just leaves a.plan nil, and the + // turn proceeds exactly as if Planning were unset -- see + // runPlanningPass's own doc comment for why that's a smaller, safer + // commitment than generalizing the retry loop below to a second + // protocol. + Planning *Protocol + // proto is the response-format contract this Agent enforces, fixed for // the Agent's lifetime by whoever called NewAgent. proto Protocol + + // plan is this turn's PlanState, set by runPlanningPass if Planning is + // non-nil and produced a usable goal; nil otherwise (including for + // every turn before this field existed). Turn-scoped like + // agent.Agent.LastOutcome: read by RequestMessages, overwritten (not + // merged) at the start of the next turn that runs a planning pass. + plan *PlanState } // NewAgent creates an Agent enforcing p. prov, tools, and perm must not be @@ -76,15 +101,43 @@ func (a *Agent) Protocol() Protocol { } // RequestMessages returns the conversation as it goes out on the wire: -// agent.Agent.RequestMessagesParts' prefix with a.proto's reminder appended, -// plus its suffix unchanged. Shadows the embedded method so it's what Run -// (below) actually sends to StreamTurn -- see StreamTurn's doc comment for -// why shadowing, not the embedding, is what makes that true. +// agent.Agent.RequestMessagesParts' prefix with a.plan's turn-plan block +// (if any -- see planBlock) and then a.proto's reminder appended, plus its +// suffix unchanged. Shadows the embedded method so it's what Run (below) +// actually sends to StreamTurn -- see StreamTurn's doc comment for why +// shadowing, not the embedding, is what makes that true. func (a *Agent) RequestMessages() []llm.Message { prefix, suffix := a.Agent.RequestMessagesParts() + prefix = withPlanBlock(prefix, a.plan) return append(a.proto.WithReminder(prefix), suffix...) } +// withPlanBlock appends planBlock(p)'s text to a copy of msgs' trailing +// message, the same per-request, never-stored-in-history treatment +// withProjectContext gives ProjectContext's own block -- a package-local +// copy of that append shape (msgs is prefix here, already ending with the +// last user-role message by RequestMessagesParts' own contract) rather +// than a shared call to it, since withProjectContext is unexported in +// package agent. +func withPlanBlock(msgs []llm.Message, p *PlanState) []llm.Message { + block := planBlock(p) + if block == "" || len(msgs) == 0 { + return msgs + } + last := &msgs[len(msgs)-1] + if last.Role != llm.RoleUser { + return msgs + } + out := append([]llm.Message(nil), msgs...) + copyLast := *last + copyLast.Content = append( + append([]llm.ContentBlock(nil), last.Content...), + llm.ContentBlock{Type: llm.ContentText, Text: agent.WrapHarnessMessage(block), Volatile: true}, + ) + out[len(out)-1] = copyLast + return out +} + // SystemPrompt builds from agent.Agent.SystemPreamble (identity, // message-format text) instead of calling agent.Agent.SystemPrompt: it // inserts a.proto.PromptText() -- the paragraph explaining the section @@ -120,15 +173,14 @@ func joinNonEmpty(parts ...string) string { } // Run starts a new turn with userInput and returns a channel of Events. The -// channel is closed when the agent has produced a final answer -// (EventTurnComplete) or hit a terminal error (EventError). The caller must -// drain the channel; for any EventPermissionRequested the caller must -// eventually call Respond, including after ctx cancellation, or the agent -// goroutine will leak. +// channel is closed after exactly one EventTurnEnded, whatever its Outcome. +// The caller must drain the channel; for any EventPermissionRequested the +// caller must eventually call Respond, including after ctx cancellation, or +// the agent goroutine will leak. func (a *Agent) Run(ctx context.Context, userInput string) <-chan agent.Event { - out := make(chan agent.Event, 16) - go a.run(ctx, userInput, out) - return out + raw := make(chan agent.Event, 16) + go a.run(ctx, userInput, raw) + return agent.StampIdentity(raw, a.Name, a.Depth) } func (a *Agent) run(ctx context.Context, userInput string, out chan<- agent.Event) { @@ -137,10 +189,7 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- agent.Even // turnStart marks the state to roll back to if the very first provider // call of this Run invocation fails -- see the iter == 0 branch below. turnStart := a.MessageCount() - a.AppendMessage(llm.Message{ - Role: llm.RoleUser, - Content: []llm.ContentBlock{{Type: llm.ContentText, Text: agent.WrapUserMessage(userInput)}}, - }) + a.AppendMessage(agent.NewTurnMessage(userInput, a.LastOutcome)) if a.Readme != nil && a.proto.SyntheticRead != nil && !hasReadmeRead(a.RequestMessages()) { assistantMsg, resultMsg := SyntheticReadmeExchange(a.proto, a.Readme()) @@ -148,6 +197,8 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- agent.Even a.AppendMessage(resultMsg) } + a.runPlanningPass(ctx, out) + maxIter := a.MaxIterations if maxIter == 0 { maxIter = defaultMaxIterations @@ -161,6 +212,10 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- agent.Even violationStart := -1 for iter := 0; iter < maxIter; iter++ { + if a.Budget.Exhausted() { + a.EndTurn(out, agent.ErrBudgetExhausted, &agent.TurnResult{Outcome: agent.OutcomeExhausted, Reason: agent.ErrBudgetExhausted.Error()}) + return + } idx := a.MessageCount() guard := a.proto.NewGuard() assistantMsg, stopReason, verdict, err := a.StreamTurn(ctx, out, a.SystemPrompt(), a.RequestMessages(), guard) @@ -175,7 +230,7 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- agent.Even // valid exchange. a.PruneMessages(turnStart, a.MessageCount()) } - out <- agent.Event{Type: agent.EventError, Err: err} + a.EndTurn(out, err, &agent.TurnResult{Outcome: agent.OutcomeForError(err), Reason: err.Error()}) return } a.AppendMessage(assistantMsg) @@ -250,7 +305,7 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- agent.Even } if isFinal { - out <- agent.Event{Type: agent.EventTurnComplete} + a.EndTurn(out, nil, &agent.TurnResult{Outcome: agent.OutcomeCompleted}) return } @@ -261,11 +316,75 @@ func (a *Agent) run(ctx context.Context, userInput string, out chan<- agent.Even // drop it rather than leave it dangling in history. n := a.MessageCount() a.PruneMessages(n-1, n) - out <- agent.Event{Type: agent.EventError, Err: err} + a.EndTurn(out, err, &agent.TurnResult{Outcome: agent.OutcomeForError(err), Reason: err.Error()}) return } a.AppendMessage(resultMsg) } - out <- agent.Event{Type: agent.EventError, Err: fmt.Errorf("agent: exceeded max iterations (%d) without reaching a final answer", maxIter)} + maxIterErr := fmt.Errorf("%w (%d) without reaching a final answer", agent.ErrMaxIterationsExceeded, maxIter) + a.EndTurn(out, maxIterErr, &agent.TurnResult{Outcome: agent.OutcomeExhausted, Reason: maxIterErr.Error()}) +} + +// runPlanningPass runs the planning pass, if a.Planning is set: one +// best-effort completion under a.Planning's own protocol (not a.proto), +// parsed into a.plan and injected into every later request this same turn +// (see RequestMessages, withPlanBlock). The exchange is appended to +// history for the transcript's sake, then immediately excluded from +// future requests (see Agent.ExcludeMessages) -- the same mechanism a +// resolved protocol-violation episode uses, since a.proto's own turn loop +// below has no idea how to interpret // tags and +// must never see them again. +// +// Deliberately not run through the same live mid-stream enforcement a.proto +// gets: no guard, no retry on a missing goal, no tool-call dispatch even +// if the model tries one (any tool_use blocks in the response are simply +// never executed -- StreamTurn still advertises the full tool list, since +// there's no per-call override for that, and a model reasoning about +// delegation benefits from seeing Spawn exists anyway). A failed or +// malformed planning completion just leaves a.plan nil, and the turn +// proceeds exactly as if Planning were unset, rather than costing a +// second call to redo it -- a deliberately smaller commitment than +// generalizing a.proto's own retry loop to a second protocol, which +// PLAN.md's "hard-enforced, same reject-and-retry path as a missing react +// tag" describes but this does not implement. +func (a *Agent) runPlanningPass(ctx context.Context, out chan<- agent.Event) { + if a.Planning == nil { + return + } + idx := a.MessageCount() + prefix, suffix := a.Agent.RequestMessagesParts() + msgs := make([]llm.Message, 0, len(prefix)+len(suffix)) + msgs = append(msgs, prefix...) + msgs = append(msgs, suffix...) + system := joinNonEmpty(a.Agent.SystemPreamble(), a.Planning.PromptText()) + + assistantMsg, _, _, err := a.StreamTurn(ctx, out, system, msgs, nil) + if err != nil { + return // best-effort: proceed without a plan rather than failing the turn + } + a.AppendMessage(assistantMsg) + a.ExcludeMessages(idx, idx+1) + + var text strings.Builder + for _, b := range assistantMsg.Content { + if b.Type == llm.ContentText { + text.WriteString(b.Text) + } + } + sections, _ := a.Planning.Parse(text.String()) + plan := &PlanState{} + for _, sc := range sections { + switch sc.Section.Name { + case "goal": + plan.Goal = sc.Text + case "todo": + plan.Todo = ParseTodoItems(sc.Text, nil) + case "delegate": + plan.Delegate = sc.Text + } + } + if plan.Goal != "" { + a.plan = plan + } } diff --git a/internal/agent/protocol/agent_test.go b/internal/agent/protocol/agent_test.go index 7b4d3f5..e2acfdb 100644 --- a/internal/agent/protocol/agent_test.go +++ b/internal/agent/protocol/agent_test.go @@ -3,6 +3,7 @@ package protocol import ( "context" "encoding/json" + "errors" "os" "strings" "testing" @@ -86,6 +87,12 @@ func textTurn(s string) []llm.Event { } } +// isCompleted reports whether ev is an EventTurnEnded with OutcomeCompleted +// -- the replacement for the old bare agent.EventTurnComplete check. +func isCompleted(ev agent.Event) bool { + return ev.Type == agent.EventTurnEnded && ev.Result != nil && ev.Result.Outcome == agent.OutcomeCompleted +} + func toolTurn(id, name, input string) []llm.Event { return []llm.Event{ {Type: llm.EventTextDelta, Text: reactTags}, @@ -171,7 +178,7 @@ func TestRunSimpleTextTurn(t *testing.T) { if e.Type == agent.EventTextDelta { gotText += e.Text } - if e.Type == agent.EventTurnComplete { + if isCompleted(e) { gotComplete = true } } @@ -180,7 +187,7 @@ func TestRunSimpleTextTurn(t *testing.T) { t.Errorf("text = %q, want %q", gotText, want) } if !gotComplete { - t.Errorf("expected agent.EventTurnComplete") + t.Errorf("expected an EventTurnEnded with OutcomeCompleted") } if len(a.Messages()) != 2 { t.Errorf("expected 2 messages (user+assistant), got %d", len(a.Messages())) @@ -190,7 +197,7 @@ func TestRunSimpleTextTurn(t *testing.T) { // TestRunFinalAnswerWithoutReactTagsIsRejectedAndRetried confirms a // tool-free final answer missing the required tags is not accepted as the // turn's end: the agent loops back for a redo (rather than emitting -// agent.EventTurnComplete) instead of surfacing the malformed answer as done. +// OutcomeCompleted) instead of surfacing the malformed answer as done. func TestRunFinalAnswerWithoutReactTagsIsRejectedAndRetried(t *testing.T) { fp := &fakeProvider{turns: [][]llm.Event{ { @@ -204,12 +211,12 @@ func TestRunFinalAnswerWithoutReactTagsIsRejectedAndRetried(t *testing.T) { ch := a.Run(context.Background(), "hi") var completions int for ev := range ch { - if ev.Type == agent.EventTurnComplete { + if isCompleted(ev) { completions++ } } if completions != 1 { - t.Fatalf("expected exactly 1 agent.EventTurnComplete (after the retry), got %d", completions) + t.Fatalf("expected exactly 1 EventTurnEnded with OutcomeCompleted (after the retry), got %d", completions) } if fp.calls != 2 { t.Fatalf("expected the provider to be called twice (reject + retry), got %d", fp.calls) @@ -433,12 +440,12 @@ func TestRunCutsTurnAtReopenedReactTag(t *testing.T) { } var completions int for _, e := range events { - if e.Type == agent.EventTurnComplete { + if isCompleted(e) { completions++ } } if completions != 1 { - t.Errorf("expected the turn to end on the truncated text (1 agent.EventTurnComplete), got %d", completions) + t.Errorf("expected the turn to end on the truncated text (1 EventTurnEnded with OutcomeCompleted), got %d", completions) } if fp.calls != 1 { t.Errorf("expected no follow-up provider call, got %d", fp.calls) @@ -658,8 +665,8 @@ func TestRunKeepsUnresolvedViolationEpisode(t *testing.T) { a.MaxIterations = 2 events := drain(t, a.Run(context.Background(), "go"), 2*time.Second) - if last := events[len(events)-1]; last.Type != agent.EventError { - t.Fatalf("expected agent.EventError after exhausting MaxIterations, got %v", last.Type) + if last := events[len(events)-1]; last.Type != agent.EventTurnEnded || last.Result == nil || last.Result.Outcome != agent.OutcomeExhausted { + t.Fatalf("expected agent.EventTurnEnded with OutcomeExhausted after exhausting MaxIterations, got %+v", last) } msgs := a.Messages() @@ -668,6 +675,35 @@ func TestRunKeepsUnresolvedViolationEpisode(t *testing.T) { } } +// TestRunHaltsOnBudgetExhaustion confirms a Budget's round-trip ceiling +// halts the enforced loop with agent.ErrBudgetExhausted -- checked before +// each StreamTurn call, same as the bare agent.Agent.run loop (see +// agent.TestRunHaltsOnBudgetExhaustion), so an exhausted budget stops the +// turn before it spends one more round-trip rather than after. +func TestRunHaltsOnBudgetExhaustion(t *testing.T) { + fp := &fakeProvider{turns: [][]llm.Event{ + toolTurn("t1", "Bash", `{"command":"echo hi"}`), + toolTurn("t2", "Bash", `{"command":"echo hi"}`), + }} + reg := tool.NewRegistry() + reg.Register(bash.Tool{}) + a := New(fp, reg, permission.New(permission.ModeBypass, nil)) + a.Budget = agent.NewBudget(1, 0) + + events := drain(t, a.Run(context.Background(), "go"), 2*time.Second) + + last := events[len(events)-1] + if last.Type != agent.EventTurnEnded || last.Result == nil || last.Result.Outcome != agent.OutcomeExhausted { + t.Fatalf("last event = %+v, want agent.EventTurnEnded with OutcomeExhausted", last) + } + if !errors.Is(last.Err, agent.ErrBudgetExhausted) { + t.Errorf("last.Err = %v, want it to wrap agent.ErrBudgetExhausted", last.Err) + } + if fp.calls != 1 { + t.Errorf("expected exactly 1 provider call (bounded by the round-trip budget), got %d", fp.calls) + } +} + // TestRunInjectsReactReminderOnlyIntoOutgoingRequest confirms the per-turn // reinforcement reminder is added to the request sent to the provider but // never persisted into a.Messages(), so it can't accumulate once per tool @@ -1100,7 +1136,7 @@ func TestRunRememberWithSuggestedSpecCoversLaterMatchingCommand(t *testing.T) { if ev.Type == agent.EventPermissionRequested { t.Fatalf("second, differently-flagged git status command should not need asking again") } - if ev.Type == agent.EventTurnComplete { + if isCompleted(ev) { break } } @@ -1109,11 +1145,11 @@ func TestRunRememberWithSuggestedSpecCoversLaterMatchingCommand(t *testing.T) { func drainToTurnComplete(t *testing.T, ch <-chan agent.Event) { t.Helper() for ev := range ch { - if ev.Type == agent.EventTurnComplete { + if isCompleted(ev) { return } } - t.Fatal("channel closed before agent.EventTurnComplete") + t.Fatal("channel closed before an EventTurnEnded with OutcomeCompleted") } // TestRunPersistRuleWritesToConfigAndReloadEnforcesIt drives the full stack @@ -1165,12 +1201,12 @@ func TestRunPersistRuleWritesToConfigAndReloadEnforcesIt(t *testing.T) { // to the agent goroutine), so any assertion about PersistRule's effect // must wait for a subsequent event -- not run right after Respond // returns -- to avoid a race with the agent goroutine actually calling - // it; draining to agent.EventTurnComplete gives that ordering for free. + // it; draining to an EventTurnEnded with OutcomeCompleted gives that ordering for free. for ev := range ch { if ev.Type == agent.EventPermissionRequested { t.Fatalf("second git command should be covered by the persisted rule, got another prompt") } - if ev.Type == agent.EventTurnComplete { + if isCompleted(ev) { break } } @@ -1313,7 +1349,7 @@ func TestDispatchBashRunsEachCommandBeforeMovingToTheNext(t *testing.T) { agent.EventToolStepStarted, agent.EventToolStepResult, agent.EventToolStepStarted, agent.EventToolStepResult, agent.EventToolCallResult, - agent.EventTextDelta, agent.EventTurnComplete, + agent.EventTextDelta, agent.EventTurnEnded, } if len(seq) != len(wantSeq) { t.Fatalf("event sequence = %v, want %v", seq, wantSeq) @@ -1470,7 +1506,8 @@ func TestRunIgnoresToolCallsWhenResultAlreadyPresent(t *testing.T) { switch ev.Type { case agent.EventToolCallStarted, agent.EventToolCallResult: sawToolActivity = true - case agent.EventTurnComplete: + } + if isCompleted(ev) { completions++ } } @@ -1479,7 +1516,7 @@ func TestRunIgnoresToolCallsWhenResultAlreadyPresent(t *testing.T) { t.Fatal("expected the tool call to never run once the response already gave its result") } if completions != 1 { - t.Fatalf("expected exactly 1 agent.EventTurnComplete, got %d", completions) + t.Fatalf("expected exactly 1 EventTurnEnded with OutcomeCompleted, got %d", completions) } if sawToolActivity { t.Fatal("expected no tool call activity at all -- the stream is cut before the tool call is ever reached") diff --git a/internal/agent/protocol/plan.go b/internal/agent/protocol/plan.go new file mode 100644 index 0000000..3402889 --- /dev/null +++ b/internal/agent/protocol/plan.go @@ -0,0 +1,155 @@ +package protocol + +import ( + "fmt" + "strconv" + "strings" + + "github.com/mchalapuk/coded/internal/agent" +) + +// TodoItem is one step toward a turn's goal, with a stable ID so later +// reference -- closing it, once amendment lands (see PLAN.md's Goals and +// plans milestone) -- refers to something durable rather than a position +// in a list that can reflow. +type TodoItem struct { + ID int + Text string + Done bool +} + +// PlanState is a turn's goal, todo list, and delegation decision, produced +// by the planning pass (see Agent's Planning field) and injected into +// every later request within the same turn (see planBlock) -- PLAN.md's +// Goals and plans milestone. Held as turn state, never stored in +// a.messages: the planning pass's own raw completion is what's persisted +// (excluded from future requests, not deleted -- see Agent.ExcludeMessages), +// and this is a parsed, structured view of it recomputed fresh each turn +// rather than a second copy to keep in sync. +type PlanState struct { + Goal string + // Todo is empty for a genuinely one-step turn (the planning pass said + // "none"), which is not the same as planning never having run at all + // (see Agent.plan being nil) -- PLAN.md's "a genuinely one-step turn + // has no list, and the harness synthesizes a single item and notes it + // rather than burning a round-trip on ceremony" describes a caller's + // own fallback when Goal is non-empty but Todo came back empty; this + // type doesn't synthesize anything for itself. + Todo []TodoItem + // Delegate is the planning pass's own section text + // verbatim: which todo items (if any) it named for Spawn, or its + // stated reason none qualified. Empty only if the planning pass + // produced no delegate section at all. + Delegate string +} + +// ParseTodoItems parses raw (the planning pass's own section text) +// into items, assigning each a stable ID: text that matches a previous +// item's Text verbatim keeps that item's ID (and Done state), so a later +// reference to "item 3" still refers to the same thing across a +// re-parse/amendment; new text gets the next unused ID. "none" (any case, +// alone on its own line) is skipped, not turned into an item -- see +// PlanState.Todo's doc comment for why a genuinely empty list and "planning +// didn't run" are different states this parser doesn't conflate. +func ParseTodoItems(raw string, previous []TodoItem) []TodoItem { + nextID := 1 + byText := make(map[string]TodoItem, len(previous)) + for _, it := range previous { + byText[it.Text] = it + if it.ID >= nextID { + nextID = it.ID + 1 + } + } + + var items []TodoItem + for _, line := range strings.Split(raw, "\n") { + text := stripListMarker(line) + if text == "" || strings.EqualFold(text, "none") { + continue + } + if prev, ok := byText[text]; ok { + items = append(items, prev) + continue + } + item := TodoItem{ID: nextID, Text: text} + items = append(items, item) + byText[text] = item + nextID++ + } + return items +} + +// stripListMarker trims line down to its content, dropping a leading +// "N. ", "N) ", "- ", or "* " list marker and surrounding whitespace -- +// the handful of shapes a model asked for "a numbered list" plausibly +// produces. Returns "" for a blank line. +func stripListMarker(line string) string { + line = strings.TrimSpace(line) + if line == "" { + return "" + } + if idx := strings.IndexAny(line, ".)"); idx > 0 && idx < 4 { + if _, err := strconv.Atoi(line[:idx]); err == nil { + return strings.TrimSpace(line[idx+1:]) + } + } + if strings.HasPrefix(line, "- ") || strings.HasPrefix(line, "* ") { + return strings.TrimSpace(line[2:]) + } + return line +} + +// planBlock renders p as a per-request note reminding the model of its own +// stated goal, todo list, and delegation decision for the rest of the turn +// -- injected the same way agent.Agent.ProjectContext is (per-request, +// Volatile, never stored in history). Nil p, or a p with no Goal (planning +// disabled, or produced nothing usable), renders "". +func planBlock(p *PlanState) string { + if p == nil || p.Goal == "" { + return "" + } + var b strings.Builder + b.WriteString("\nGoal: ") + b.WriteString(p.Goal) + if len(p.Todo) > 0 { + b.WriteString("\nTodo:") + for _, item := range p.Todo { + status := "[ ]" + if item.Done { + status = "[x]" + } + fmt.Fprintf(&b, "\n %s #%d %s", status, item.ID, item.Text) + } + } + if p.Delegate != "" { + b.WriteString("\nDelegation: ") + b.WriteString(p.Delegate) + } + b.WriteString("\n") + return b.String() +} + +// FinalOutcome computes the outcome a turn should actually report given its +// natural one and whether its plan (if any) still has open todo items: a +// natural agent.OutcomeCompleted downgrades to agent.OutcomeIncomplete when +// items are open -- PLAN.md's "a final answer with open items requires a +// declared outcome... undeclared is incomplete, and it's a protocol +// violation, not a circumstance." Any other natural outcome (already a +// harness- or agent-caused halt) passes through unchanged: this only ever +// downgrades a claimed clean finish, it never upgrades a real failure into +// something else. +// +// Not called from Agent.run automatically. Doing so today would mark every +// turn Incomplete the moment planning produces more than a one-item todo +// list, since there is no in-band way yet for the main loop to mark an +// item Done -- live todo amendment needs a change to the main protocol +// itself (react's own tags), not just an optional side-pass, and is +// explicitly deferred (see PlanState's own doc comment). A caller with its +// own way of tracking which items were actually addressed can call this +// directly; nothing in this codebase does yet. +func FinalOutcome(natural agent.Outcome, hasOpenItems bool) agent.Outcome { + if natural == agent.OutcomeCompleted && hasOpenItems { + return agent.OutcomeIncomplete + } + return natural +} diff --git a/internal/agent/protocol/plan_test.go b/internal/agent/protocol/plan_test.go new file mode 100644 index 0000000..bae3a40 --- /dev/null +++ b/internal/agent/protocol/plan_test.go @@ -0,0 +1,186 @@ +package protocol + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/mchalapuk/coded/internal/agent" + "github.com/mchalapuk/coded/internal/llm" + "github.com/mchalapuk/coded/internal/permission" + "github.com/mchalapuk/coded/internal/tool" +) + +func TestParseTodoItemsAssignsStableIDs(t *testing.T) { + first := ParseTodoItems("1. read the file\n2. write the fix", nil) + if len(first) != 2 { + t.Fatalf("first parse = %+v, want 2 items", first) + } + if first[0].ID == first[1].ID { + t.Fatalf("first parse assigned the same ID to two different items: %+v", first) + } + + // Re-parsing the same two lines, plus a new third one, must keep the + // first two items' IDs unchanged and give the new one a fresh ID. + second := ParseTodoItems("1. read the file\n2. write the fix\n3. add a test", first) + if len(second) != 3 { + t.Fatalf("second parse = %+v, want 3 items", second) + } + if second[0].ID != first[0].ID { + t.Errorf("item 0's ID changed across re-parse: %d -> %d", first[0].ID, second[0].ID) + } + if second[1].ID != first[1].ID { + t.Errorf("item 1's ID changed across re-parse: %d -> %d", first[1].ID, second[1].ID) + } + if second[2].ID == first[0].ID || second[2].ID == first[1].ID { + t.Errorf("the new third item reused an existing ID: %+v", second) + } +} + +func TestParseTodoItemsPreservesDoneAcrossReparse(t *testing.T) { + items := ParseTodoItems("1. read the file", nil) + items[0].Done = true + + reparsed := ParseTodoItems("1. read the file\n2. write the fix", items) + if !reparsed[0].Done { + t.Errorf("Done was lost across re-parse: %+v", reparsed) + } + if reparsed[1].Done { + t.Errorf("the new item started Done, want false: %+v", reparsed) + } +} + +func TestParseTodoItemsNoneYieldsNoItems(t *testing.T) { + if got := ParseTodoItems("none", nil); len(got) != 0 { + t.Errorf("ParseTodoItems(\"none\") = %v, want no items", got) + } + if got := ParseTodoItems("None", nil); len(got) != 0 { + t.Errorf("ParseTodoItems(\"None\") = %v, want no items (case-insensitive)", got) + } +} + +func TestPlanBlockNilOrEmptyGoalIsEmpty(t *testing.T) { + if got := planBlock(nil); got != "" { + t.Errorf("planBlock(nil) = %q, want \"\"", got) + } + if got := planBlock(&PlanState{}); got != "" { + t.Errorf("planBlock(no goal) = %q, want \"\"", got) + } +} + +func TestPlanBlockRendersGoalTodoAndDelegation(t *testing.T) { + p := &PlanState{ + Goal: "add an email validator", + Todo: []TodoItem{{ID: 1, Text: "read validators.go"}, {ID: 2, Text: "write the test", Done: true}}, + Delegate: "none of these need a sub-agent", + } + got := planBlock(p) + for _, want := range []string{"add an email validator", "#1 read validators.go", "[x] #2 write the test", "none of these need a sub-agent"} { + if !strings.Contains(got, want) { + t.Errorf("planBlock() = %q, want it to contain %q", got, want) + } + } +} + +// planningTestProtocol mirrors internal/agent/planning.Protocol()'s shape +// without importing that package -- matching this codebase's convention +// (see internal/agent/definition's own reactLikeProtocol) of keeping a +// package's tests free of a dependency on a sibling package whose own +// shape could drift independently of what these tests actually check. +var planningTestProtocol = Protocol{ + Name: "planning-test", + Sections: []Section{ + {Name: "goal", Kind: KindDecision}, + {Name: "todo", Kind: KindDecision}, + {Name: "delegate", Kind: KindDecision}, + }, +} + +func planningTurn(goal, todo, delegate string) []llm.Event { + text := planningTestProtocol.WrapSections(goal, todo, delegate) + return []llm.Event{ + {Type: llm.EventTextDelta, Text: text}, + {Type: llm.EventMessageStop, StopReason: llm.StopEndTurn}, + } +} + +// TestRunPlanningPassPopulatesPlanAndExcludesItsOwnExchange drives a full +// turn with Planning set: the first scripted completion is the planning +// pass's own response (parsed into a.plan), the second is the main +// protocol's ordinary final answer. Checks both that a.plan is populated +// correctly and that the planning exchange never reaches the main +// protocol's own request -- a.proto has no idea how to read // +// tags, so if it leaked through, MissingSections would reject +// it as a violation. +func TestRunPlanningPassPopulatesPlanAndExcludesItsOwnExchange(t *testing.T) { + fp := &fakeProvider{turns: [][]llm.Event{ + planningTurn("add an email validator", "1. read validators.go\n2. write the test", "none of these need a sub-agent"), + textTurn("done"), + }} + a := New(fp, tool.NewRegistry(), permission.New(permission.ModeDefault, nil)) + a.Planning = &planningTestProtocol + + events := drain(t, a.Run(context.Background(), "add email validation"), 2*time.Second) + var last agent.Event + if len(events) > 0 { + last = events[len(events)-1] + } + if last.Type != agent.EventTurnEnded || last.Result == nil || last.Result.Outcome != agent.OutcomeCompleted { + t.Fatalf("last event = %+v, want a completed EventTurnEnded (the main protocol's own second turn)", last) + } + + if a.plan == nil { + t.Fatalf("a.plan is nil, want it populated from the planning pass's response") + } + if a.plan.Goal != "add an email validator" { + t.Errorf("a.plan.Goal = %q, want %q", a.plan.Goal, "add an email validator") + } + if len(a.plan.Todo) != 2 { + t.Fatalf("a.plan.Todo = %+v, want 2 items", a.plan.Todo) + } + if a.plan.Delegate != "none of these need a sub-agent" { + t.Errorf("a.plan.Delegate = %q, want the planning pass's own text", a.plan.Delegate) + } + + // The planning exchange must be in the persisted transcript (in + // a.Messages()) but excluded from what actually went to the provider + // on the second call. + if len(fp.requests) < 2 { + t.Fatalf("got %d provider requests, want at least 2 (planning + main)", len(fp.requests)) + } + for _, m := range fp.requests[1].Messages { + for _, c := range m.Content { + if strings.Contains(c.Text, "") { + t.Fatalf("the main protocol's own request still contains the planning pass's tag: %+v", fp.requests[1].Messages) + } + } + } +} + +func TestFinalOutcomeDowngradesCompletedWithOpenItems(t *testing.T) { + if got := FinalOutcome(agent.OutcomeCompleted, true); got != agent.OutcomeIncomplete { + t.Errorf("FinalOutcome(Completed, open items) = %v, want OutcomeIncomplete", got) + } +} + +func TestFinalOutcomeLeavesCompletedWithNoOpenItemsAlone(t *testing.T) { + if got := FinalOutcome(agent.OutcomeCompleted, false); got != agent.OutcomeCompleted { + t.Errorf("FinalOutcome(Completed, no open items) = %v, want OutcomeCompleted unchanged", got) + } +} + +// TestFinalOutcomeNeverChangesANonCompletedOutcome guards the "only ever +// downgrades a claimed clean finish" property: an outcome that's already +// a harness- or agent-caused halt must never be relabeled by this +// function, regardless of hasOpenItems. +func TestFinalOutcomeNeverChangesANonCompletedOutcome(t *testing.T) { + for _, o := range []agent.Outcome{agent.OutcomeExhausted, agent.OutcomeErrored, agent.OutcomeCancelled, agent.OutcomeAbandoned, agent.OutcomeIncomplete} { + if got := FinalOutcome(o, true); got != o { + t.Errorf("FinalOutcome(%v, true) = %v, want unchanged %v", o, got, o) + } + if got := FinalOutcome(o, false); got != o { + t.Errorf("FinalOutcome(%v, false) = %v, want unchanged %v", o, got, o) + } + } +} diff --git a/internal/agent/protocol/prompt.go b/internal/agent/protocol/prompt.go index 8f8b491..4edc4f2 100644 --- a/internal/agent/protocol/prompt.go +++ b/internal/agent/protocol/prompt.go @@ -59,11 +59,21 @@ func requiredSentence(sections []Section, result Section, hasResult bool) string } } - all := "both" - if n != 2 { + var all string + switch n { + case 1: + all = "it" + case 2: + all = "both" + default: all = "all " + numberWord(n) } + blockWord := "tagged blocks" + if n == 1 { + blockWord = "tagged block" + } + closing := "the tool call or calls, or your final answer" endClause := "" if hasResult { @@ -72,7 +82,7 @@ func requiredSentence(sections []Section, result Section, hasResult bool) string } return "IMPORTANT: Every message you write must open with " + numberWord(n) + - " tagged blocks in this order: " + strings.Join(steps, ", then ") + + " " + blockWord + " in this order: " + strings.Join(steps, ", then ") + " — followed by " + closing + ". Be sure to start your message with " + strings.Join(tags, " followed by ") + " but use those sections only once per your message." + endClause + " This format is required for all your responses. " + diff --git a/internal/agent/protocol/retry_event_test.go b/internal/agent/protocol/retry_event_test.go index f080233..532c68b 100644 --- a/internal/agent/protocol/retry_event_test.go +++ b/internal/agent/protocol/retry_event_test.go @@ -44,10 +44,12 @@ func TestRunForwardsRetryingEvent(t *testing.T) { retrying = append(retrying, e) case agent.EventTextDelta: gotText += e.Text - case agent.EventTurnComplete: - gotComplete = true - case agent.EventError: - t.Fatalf("unexpected agent.EventError: %v", e.Err) + case agent.EventTurnEnded: + if isCompleted(e) { + gotComplete = true + } else { + t.Fatalf("unexpected non-completed EventTurnEnded: %+v", e.Result) + } } } @@ -65,7 +67,7 @@ func TestRunForwardsRetryingEvent(t *testing.T) { } if !gotComplete { - t.Error("expected agent.EventTurnComplete once the stream recovers") + t.Error("expected an EventTurnEnded with OutcomeCompleted once the stream recovers") } want := reactTags + "recovered" if gotText != want { diff --git a/internal/agent/protocol/scheme.go b/internal/agent/protocol/scheme.go new file mode 100644 index 0000000..a155550 --- /dev/null +++ b/internal/agent/protocol/scheme.go @@ -0,0 +1,78 @@ +package protocol + +// Visibility controls whether a Pass's output crosses back to whoever +// invoked the turn. Private output still persists in the transcript (so it +// can be rendered on request, e.g. the reviewer's checklist answers) but is +// excluded from the request history any later pass, or the caller, sees -- +// the same mechanism Protocol.Agent already uses to exclude a resolved +// protocol-violation episode from future requests (see Agent.ExcludeMessages), +// applied here by construction rather than after the fact. +type Visibility string + +const ( + // VisibilityPublic exposes a pass's output the way a single-pass + // scheme's only pass always has: it's the turn's answer, or feeds + // straight into history for the next pass/iteration to see. + VisibilityPublic Visibility = "public" + // VisibilityPrivate keeps a pass's output out of what any later pass or + // the caller sees -- e.g. the reviewer's pass-1 checklist answers, + // which only pass 2's action-item list (VisibilityPublic) is meant to + // surface. + VisibilityPrivate Visibility = "private" +) + +// Pass is one provider call within a Scheme: its own prompt fragment (via +// its own Protocol's PromptSuffix), its own response-format contract (its +// own Protocol), and how much of the turn it can see and how far its +// output travels. +type Pass struct { + // Name identifies the pass for logging/rendering and for Sees + // references from a later pass in the same Scheme. + Name string + // Protocol is this pass's own response-format contract -- a different + // Protocol per pass is the whole point (see PLAN.md's Thinking schemes + // milestone): the reviewer's pass 1 wants a checklist protocol, pass 2 + // wants an action-item-list protocol, neither is the react protocol + // the passes around them might be using. + Protocol Protocol + // When, if set, gates this pass to firing only on that Situation -- + // e.g. a planning pass with When: SituationUserInput runs once, at + // turn open, and not on every later tool-round. The zero value ("") + // means every iteration, which is what makes a single-pass Scheme + // (see Default) exactly today's per-iteration behavior: one pass, no + // gate, every time. + When Situation + // Visibility controls whether this pass's output crosses back to a + // later pass or the caller -- see Visibility's doc comment. + Visibility Visibility + // Sees names the earlier passes (by Name) in the same Scheme whose + // private output this pass's own request history may include, in + // addition to every public pass's output, which is always visible + // (it's already in history, or was the previous iteration's answer). + // Empty means this pass sees only public output, same as any external + // caller. + Sees []string +} + +// Scheme is an ordered list of Passes making up one agent's turn structure. +// A Scheme with exactly one Pass (no When gate, VisibilityPublic) is the +// default and is exactly today's single-completion-per-iteration behavior +// -- see Default. +type Scheme struct { + Name string + Passes []Pass +} + +// Default returns the single-pass Scheme equivalent to running p directly, +// the way every agent in this codebase runs today: one Pass, no When gate +// (fires every iteration), VisibilityPublic (its output is the turn's +// answer, unchanged from today). Named Default rather than requiring every +// existing caller to construct this by hand, and the reference a +// multi-pass Scheme's own zero-Pass-count validation (once a turn loop +// actually executes one) can fall back to. +func Default(p Protocol) Scheme { + return Scheme{ + Name: p.Name, + Passes: []Pass{{Name: p.Name, Protocol: p, Visibility: VisibilityPublic}}, + } +} diff --git a/internal/agent/protocol/scheme_test.go b/internal/agent/protocol/scheme_test.go new file mode 100644 index 0000000..6f161cc --- /dev/null +++ b/internal/agent/protocol/scheme_test.go @@ -0,0 +1,22 @@ +package protocol + +import "testing" + +func TestDefaultProducesOneUngatedPublicPass(t *testing.T) { + p := Protocol{Name: "react", Sections: []Section{{Name: "result", Kind: KindResult}}} + s := Default(p) + + if len(s.Passes) != 1 { + t.Fatalf("Default(p).Passes = %v, want exactly 1 pass", s.Passes) + } + pass := s.Passes[0] + if pass.When != "" { + t.Errorf("Default pass.When = %q, want \"\" (fires every iteration)", pass.When) + } + if pass.Visibility != VisibilityPublic { + t.Errorf("Default pass.Visibility = %q, want VisibilityPublic", pass.Visibility) + } + if pass.Protocol.Name != "react" { + t.Errorf("Default pass.Protocol = %+v, want the same Protocol passed in", pass.Protocol) + } +} diff --git a/internal/agent/protocol/situation.go b/internal/agent/protocol/situation.go new file mode 100644 index 0000000..e6a908a --- /dev/null +++ b/internal/agent/protocol/situation.go @@ -0,0 +1,51 @@ +package protocol + +// Situation is what the harness has just put in front of the model, the +// input that selects which Scheme passes fire this iteration (see Pass.When). +// Distinct from a Pass because it does not cost a call on its own: a +// situation is a property of one iteration, and interpreting a tool result +// while deciding the next action is one completion, not two (see PLAN.md's +// Situation entry). +// +// Only the two situations an ordinary turn loop can actually produce are +// declared here. GateFailure (v0.4's stage gates) has no producer in this +// codebase yet; ProtocolViolation is deliberately not modelled as a +// Situation at all -- the existing rejection/retry path (Protocol. +// MissingSections, Agent.rejectToolCalls) already handles it without one, +// and inventing a Situation value with no reader would just be dead data. +type Situation string + +const ( + // SituationUserInput is the turn's opening iteration: fresh input from + // the user (or, for a sub-agent, its spawn prompt) with no tool result + // yet. Fires exactly once per turn, at open -- see + // SituationForIteration's doc comment for why that is an invariant of + // this package's turn loop, not a coincidence a future change could + // silently break. + SituationUserInput Situation = "user-input" + // SituationToolResult is every iteration after the first: the model is + // deciding its next action (or final answer) with at least one tool + // result already in the request. + SituationToolResult Situation = "tool-result" +) + +// SituationForIteration reports which Situation applies to iteration iter +// (0-based) of a turn loop: SituationUserInput at iter == 0, +// SituationToolResult afterward. +// +// SituationUserInput firing exactly once, at iter == 0, is an invariant, +// not an accident: it holds only because Ask's answer returns to the loop +// as a tool result (see agent.PermissionRequest), not as a new user-input +// situation -- if that ever changed, a mid-turn Ask would need to re-fire +// SituationUserInput, and any pass gated on it (see Pass.When) would run +// again mid-turn, silently breaking "the goal is fixed once the turn +// starts" (PLAN.md's Goals and plans milestone). Anything that depends on +// user-input firing once per turn should call this function rather than +// re-deriving the same iter == 0 check, so the invariant has exactly one +// place to hold. +func SituationForIteration(iter int) Situation { + if iter == 0 { + return SituationUserInput + } + return SituationToolResult +} diff --git a/internal/agent/protocol/situation_test.go b/internal/agent/protocol/situation_test.go new file mode 100644 index 0000000..7d9bac1 --- /dev/null +++ b/internal/agent/protocol/situation_test.go @@ -0,0 +1,14 @@ +package protocol + +import "testing" + +func TestSituationForIterationFiresUserInputOnlyOnce(t *testing.T) { + if got := SituationForIteration(0); got != SituationUserInput { + t.Errorf("SituationForIteration(0) = %v, want SituationUserInput", got) + } + for _, iter := range []int{1, 2, 3, 49} { + if got := SituationForIteration(iter); got != SituationToolResult { + t.Errorf("SituationForIteration(%d) = %v, want SituationToolResult (user-input must fire exactly once, at iter 0)", iter, got) + } + } +} diff --git a/internal/agent/protocol/turn_failure_test.go b/internal/agent/protocol/turn_failure_test.go index 6473dc3..8c76a11 100644 --- a/internal/agent/protocol/turn_failure_test.go +++ b/internal/agent/protocol/turn_failure_test.go @@ -58,8 +58,11 @@ func TestFailedFirstTurnRollsBackUserMessage(t *testing.T) { ch := a.Run(context.Background(), "hi") events := drain(t, ch, 2*time.Second) - if len(events) != 1 || events[0].Type != agent.EventError || events[0].Err != wantErr { - t.Fatalf("events = %+v, want a single agent.EventError wrapping %v", events, wantErr) + if len(events) != 1 || events[0].Type != agent.EventTurnEnded || events[0].Err != wantErr { + t.Fatalf("events = %+v, want a single agent.EventTurnEnded wrapping %v", events, wantErr) + } + if events[0].Result == nil || events[0].Result.Outcome != agent.OutcomeErrored { + t.Fatalf("events[0].Result = %+v, want OutcomeErrored", events[0].Result) } if got := len(a.Messages()); got != 0 { t.Fatalf("Messages() len = %d, want 0 (the failed user message must be rolled back)", got) @@ -129,8 +132,11 @@ func TestLaterIterationStreamFailureKeepsEarlierExchanges(t *testing.T) { events := drain(t, ch, 2*time.Second) last := events[len(events)-1] - if last.Type != agent.EventError || last.Err != wantErr { - t.Fatalf("last event = %+v, want agent.EventError wrapping %v", last, wantErr) + if last.Type != agent.EventTurnEnded || last.Err != wantErr { + t.Fatalf("last event = %+v, want agent.EventTurnEnded wrapping %v", last, wantErr) + } + if last.Result == nil || last.Result.Outcome != agent.OutcomeErrored { + t.Fatalf("last.Result = %+v, want OutcomeErrored", last.Result) } // user + assistant(tool_use) + tool_result from the first, successful @@ -169,8 +175,8 @@ func TestDispatchToolCallsFailureDropsDanglingAssistantMessage(t *testing.T) { events = append(events, ev) } last := events[len(events)-1] - if last.Type != agent.EventError { - t.Fatalf("last event = %+v, want agent.EventError from the canceled context", last) + if last.Type != agent.EventTurnEnded || last.Result == nil || last.Result.Outcome != agent.OutcomeCancelled { + t.Fatalf("last event = %+v, want agent.EventTurnEnded with OutcomeCancelled from the canceled context", last) } if got := len(a.Messages()); got != 1 { diff --git a/internal/agent/run_test.go b/internal/agent/run_test.go index ea5341e..df37955 100644 --- a/internal/agent/run_test.go +++ b/internal/agent/run_test.go @@ -18,13 +18,15 @@ import ( // protocol_test's helper of the same name -- protocol imports this package, // so the reverse import needed to share one isn't possible. type fakeProvider struct { - turns [][]llm.Event - calls int + turns [][]llm.Event + calls int + requests []llm.Request // every Request this fake has seen, in call order } func (f *fakeProvider) Name() string { return "fake" } func (f *fakeProvider) Stream(ctx context.Context, req llm.Request) (<-chan llm.Event, error) { + f.requests = append(f.requests, req) if f.calls >= len(f.turns) { panic("fakeProvider: ran out of scripted turns") } @@ -112,6 +114,12 @@ func drain(t *testing.T, ch <-chan Event, timeout time.Duration) []Event { } } +// isCompleted reports whether ev is an EventTurnEnded with OutcomeCompleted +// -- the replacement for the old bare EventTurnComplete check. +func isCompleted(ev Event) bool { + return ev.Type == EventTurnEnded && ev.Result != nil && ev.Result.Outcome == OutcomeCompleted +} + // TestRunSimpleTextTurn covers the base case: a text-only reply ends the // turn immediately, with no response-format contract to satisfy. func TestRunSimpleTextTurn(t *testing.T) { @@ -126,7 +134,7 @@ func TestRunSimpleTextTurn(t *testing.T) { if e.Type == EventTextDelta { gotText += e.Text } - if e.Type == EventTurnComplete { + if isCompleted(e) { gotComplete = true } } @@ -134,13 +142,36 @@ func TestRunSimpleTextTurn(t *testing.T) { t.Errorf("text = %q, want %q", gotText, "hello there") } if !gotComplete { - t.Errorf("expected EventTurnComplete, got %+v", events) + t.Errorf("expected an EventTurnEnded with OutcomeCompleted, got %+v", events) } if got := len(a.Messages()); got != 2 { t.Errorf("expected 2 messages (user+assistant), got %d", got) } } +// TestRunStampsAgentIdentityOnEveryEvent confirms Run's self-stamping (see +// stampIdentity): every event on the channel it returns carries whatever +// Name/Depth the Agent had at the moment Run was called, with no per-call +// site inside the turn loop (StreamTurn, EndTurn) needing to set either +// field itself. +func TestRunStampsAgentIdentityOnEveryEvent(t *testing.T) { + fp := &fakeProvider{turns: [][]llm.Event{textTurn("hi")}} + a := New(fp, tool.NewRegistry(), permission.New(permission.ModeDefault, nil)) + a.Name = "main/explorer" + a.Depth = 1 + + events := drain(t, a.Run(context.Background(), "go"), 2*time.Second) + + if len(events) == 0 { + t.Fatal("expected at least one event") + } + for _, ev := range events { + if ev.AgentName != "main/explorer" || ev.AgentDepth != 1 { + t.Errorf("event %v AgentName/AgentDepth = %q/%d, want main/explorer/1", ev.Type, ev.AgentName, ev.AgentDepth) + } + } +} + // TestRunExecutesToolThenFinishes confirms a tool-call turn dispatches and // loops back for the next provider call, without needing any protocol // enforcement to drive that loop. @@ -157,12 +188,12 @@ func TestRunExecutesToolThenFinishes(t *testing.T) { var gotComplete bool for _, e := range events { - if e.Type == EventTurnComplete { + if isCompleted(e) { gotComplete = true } } if !gotComplete { - t.Fatalf("expected EventTurnComplete, got %+v", events) + t.Fatalf("expected an EventTurnEnded with OutcomeCompleted, got %+v", events) } // user, assistant(tool_use), tool_result, assistant(final) if got := len(a.Messages()); got != 4 { @@ -186,14 +217,46 @@ func TestRunExceedsMaxIterations(t *testing.T) { events := drain(t, a.Run(context.Background(), "go"), 2*time.Second) last := events[len(events)-1] - if last.Type != EventError { - t.Fatalf("last event = %+v, want EventError", last) + if last.Type != EventTurnEnded || last.Result == nil || last.Result.Outcome != OutcomeExhausted { + t.Fatalf("last event = %+v, want EventTurnEnded with OutcomeExhausted", last) + } + if !errors.Is(last.Err, ErrMaxIterationsExceeded) { + t.Errorf("last.Err = %v, want it to wrap ErrMaxIterationsExceeded", last.Err) } if fp.calls != 3 { t.Errorf("expected exactly 3 provider calls (bounded by MaxIterations), got %d", fp.calls) } } +// TestRunHaltsOnBudgetExhaustion confirms a Budget's round-trip ceiling +// halts the loop with ErrBudgetExhausted -- checked before each StreamTurn +// call, so an already-exhausted budget prevents even one more provider call +// from being made, distinct from the max-iterations case (which lets the +// in-flight call finish and only stops the next one). +func TestRunHaltsOnBudgetExhaustion(t *testing.T) { + fp := &fakeProvider{turns: [][]llm.Event{ + toolTurn("t1", "Echo", `{}`), + toolTurn("t2", "Echo", `{}`), + }} + reg := tool.NewRegistry() + reg.Register(echoTool{}) + a := New(fp, reg, permission.New(permission.ModeDefault, nil)) + a.Budget = NewBudget(1, 0) + + events := drain(t, a.Run(context.Background(), "go"), 2*time.Second) + + last := events[len(events)-1] + if last.Type != EventTurnEnded || last.Result == nil || last.Result.Outcome != OutcomeExhausted { + t.Fatalf("last event = %+v, want EventTurnEnded with OutcomeExhausted", last) + } + if !errors.Is(last.Err, ErrBudgetExhausted) { + t.Errorf("last.Err = %v, want it to wrap ErrBudgetExhausted", last.Err) + } + if fp.calls != 1 { + t.Errorf("expected exactly 1 provider call (bounded by the round-trip budget), got %d", fp.calls) + } +} + // TestRunFirstTurnProviderErrorRollsBackHistory guards the dangling-message // bug: if the very first provider call of a fresh session fails, the user // message run() appended before streaming must not survive. @@ -203,8 +266,11 @@ func TestRunFirstTurnProviderErrorRollsBackHistory(t *testing.T) { events := drain(t, a.Run(context.Background(), "hi"), 2*time.Second) - if len(events) != 1 || events[0].Type != EventError || events[0].Err != wantErr { - t.Fatalf("events = %+v, want a single EventError wrapping %v", events, wantErr) + if len(events) != 1 || events[0].Type != EventTurnEnded || events[0].Err != wantErr { + t.Fatalf("events = %+v, want a single EventTurnEnded wrapping %v", events, wantErr) + } + if events[0].Result == nil || events[0].Result.Outcome != OutcomeErrored { + t.Fatalf("events[0].Result = %+v, want OutcomeErrored", events[0].Result) } if got := len(a.Messages()); got != 0 { t.Fatalf("Messages() len = %d, want 0 (the failed user message must be rolled back)", got) @@ -226,8 +292,8 @@ func TestRunNeverPersistsContentlessAssistantMessage(t *testing.T) { events := drain(t, a.Run(context.Background(), "hi"), 2*time.Second) last := events[len(events)-1] - if last.Type != EventTurnComplete { - t.Fatalf("last event = %+v, want EventTurnComplete", last) + if !isCompleted(last) { + t.Fatalf("last event = %+v, want an EventTurnEnded with OutcomeCompleted", last) } for _, m := range a.Messages() { if m.Role == llm.RoleAssistant && len(m.Content) == 0 { @@ -263,8 +329,8 @@ func TestRunDispatchToolCallsFailureDropsDanglingAssistantMessage(t *testing.T) events = append(events, ev) } last := events[len(events)-1] - if last.Type != EventError { - t.Fatalf("last event = %+v, want EventError from the canceled context", last) + if last.Type != EventTurnEnded || last.Result == nil || last.Result.Outcome != OutcomeCancelled { + t.Fatalf("last event = %+v, want EventTurnEnded with OutcomeCancelled from the canceled context", last) } if got := len(a.Messages()); got != 1 { t.Fatalf("Messages() len = %d, want 1 (only the user message; the dangling assistant tool_use must be dropped)", got) diff --git a/internal/memory/session/session.go b/internal/memory/session/session.go index f7a2fd9..7cb2810 100644 --- a/internal/memory/session/session.go +++ b/internal/memory/session/session.go @@ -34,6 +34,17 @@ type Meta struct { Title string `json:"title,omitempty"` StartedAt time.Time `json:"started_at"` UpdatedAt time.Time `json:"updated_at"` + + // ParentID is the ID of the session that spawned this one (see + // Session.NewChild), "" for an ordinary top-level session. A + // sub-agent's transcript is its own file, referenced by its parent + // rather than interleaved into it -- see ChildIDs -- so List can keep + // showing conversations, not fragments (PLAN.md's Sub-agents section). + ParentID string `json:"parent_id,omitempty"` + // ChildIDs holds the IDs of every child session spawned from this one, + // in spawn order, so a full replay can find and walk them without + // scanning every session directory for a matching ParentID. + ChildIDs []string `json:"child_ids,omitempty"` } // Session manages on-disk persistence for one conversation. @@ -91,6 +102,27 @@ func New(cwd, providerName, model string) (*Session, error) { return s, nil } +// NewChild creates a fresh session for a sub-agent spawned from s (see the +// Spawn tool): same cwd as s, its own transcript file, ParentID set to +// s.ID(). s's own meta is updated (ChildIDs gains the new session's ID) and +// persisted immediately, so the link survives even if the child's own run +// never gets far enough to write anything itself. +func (s *Session) NewChild(providerName, model string) (*Session, error) { + child, err := New(s.meta.Cwd, providerName, model) + if err != nil { + return nil, err + } + child.meta.ParentID = s.meta.ID + if err := child.writeMeta(); err != nil { + return nil, err + } + s.meta.ChildIDs = append(s.meta.ChildIDs, child.meta.ID) + if err := s.writeMeta(); err != nil { + return nil, err + } + return child, nil +} + // Open loads an existing session by ID. func Open(id string) (*Session, error) { root, err := RootDir() @@ -120,11 +152,14 @@ func Continue(cwd string) (*Session, error) { return nil, fmt.Errorf("session: no existing session for %s: %w", cwd, os.ErrNotExist) } -// List returns metadata for all sessions, sorted by UpdatedAt descending -// (most recent first). It reads only meta.json files and never opens a -// transcript, so listing costs O(number of sessions) rather than O(total -// transcript bytes) -- which is why the metadata lives in a sibling file -// instead of being recovered from the transcript itself. +// List returns metadata for every top-level session (ParentID == "") -- +// see NewChild -- sorted by UpdatedAt descending (most recent first). A +// sub-agent's own session is deliberately excluded: it's reachable from +// its parent's ChildIDs, not a conversation of its own for `coded sessions +// list` to show alongside real ones. Reads only meta.json files and never +// opens a transcript, so listing costs O(number of sessions) rather than +// O(total transcript bytes) -- which is why the metadata lives in a +// sibling file instead of being recovered from the transcript itself. func List() ([]Meta, error) { root, err := RootDir() if err != nil { @@ -147,6 +182,9 @@ func List() ([]Meta, error) { if err := s.readMeta(); err != nil { continue // skip corrupt/partial session dirs } + if s.meta.ParentID != "" { + continue + } metas = append(metas, s.meta) } sort.Slice(metas, func(i, j int) bool { return metas[i].UpdatedAt.After(metas[j].UpdatedAt) }) diff --git a/internal/memory/session/session_test.go b/internal/memory/session/session_test.go index 3d2d5e7..807ec33 100644 --- a/internal/memory/session/session_test.go +++ b/internal/memory/session/session_test.go @@ -149,3 +149,60 @@ func TestListSortedNewestFirst(t *testing.T) { t.Fatalf("unexpected order: %+v", list) } } + +// TestNewChildLinksParentAndChild confirms NewChild records the +// relationship on both sides: the child's own ParentID, and the parent's +// ChildIDs, persisted immediately rather than only in memory. +func TestNewChildLinksParentAndChild(t *testing.T) { + setHome(t) + parent, err := New("/repo", "anthropic", "m") + if err != nil { + t.Fatal(err) + } + + child, err := parent.NewChild("anthropic", "m") + if err != nil { + t.Fatal(err) + } + if child.Meta().ParentID != parent.ID() { + t.Errorf("child.ParentID = %q, want %q", child.Meta().ParentID, parent.ID()) + } + if child.Meta().Cwd != parent.Meta().Cwd { + t.Errorf("child.Cwd = %q, want the same as parent's %q", child.Meta().Cwd, parent.Meta().Cwd) + } + if got := parent.Meta().ChildIDs; len(got) != 1 || got[0] != child.ID() { + t.Fatalf("parent.ChildIDs = %v, want [%s]", got, child.ID()) + } + + // Persisted, not just held in memory -- reopening the parent from disk + // must see the same link. + reopened, err := Open(parent.ID()) + if err != nil { + t.Fatal(err) + } + if got := reopened.Meta().ChildIDs; len(got) != 1 || got[0] != child.ID() { + t.Fatalf("reopened parent.ChildIDs = %v, want [%s]", got, child.ID()) + } +} + +// TestListExcludesChildSessions confirms a sub-agent's own session never +// shows up in the top-level listing -- it's a fragment of its parent's +// conversation, not a conversation of its own. +func TestListExcludesChildSessions(t *testing.T) { + setHome(t) + parent, err := New("/repo", "anthropic", "m") + if err != nil { + t.Fatal(err) + } + if _, err := parent.NewChild("anthropic", "m"); err != nil { + t.Fatal(err) + } + + list, err := List() + if err != nil { + t.Fatal(err) + } + if len(list) != 1 || list[0].ID != parent.ID() { + t.Fatalf("List() = %+v, want only the parent session", list) + } +} diff --git a/internal/terminal/oneshot/oneshot.go b/internal/terminal/oneshot/oneshot.go index 71bc6de..c5e4e32 100644 --- a/internal/terminal/oneshot/oneshot.go +++ b/internal/terminal/oneshot/oneshot.go @@ -7,55 +7,194 @@ import ( "encoding/json" "fmt" "io" + "strings" "github.com/mchalapuk/coded/internal/agent" "github.com/mchalapuk/coded/internal/permission" "github.com/mchalapuk/coded/internal/terminal/render" ) +// indentWidth is how many spaces one level of Event.AgentDepth indents by. +const indentWidth = 2 + +// TurnError is what Render returns for a non-completed root turn: the +// underlying error (nil for OutcomeAbandoned/OutcomeIncomplete, which state +// a Reason rather than carry an error -- see agent.TurnResult) alongside +// the Outcome that produced it, so a caller like cmd/coded's exitCodeFor +// can pick a process exit code from the outcome even when there's no +// classified *llm.Error to inspect (e.g. OutcomeAbandoned). Implements +// error/Unwrap so existing "if err != nil" and errors.As/Is callers work +// unchanged; only a caller that specifically wants the Outcome needs to +// know this type exists. +type TurnError struct { + Outcome agent.Outcome + Err error +} + +func (e *TurnError) Error() string { + if e.Err != nil { + return e.Err.Error() + } + return string(e.Outcome) +} + +func (e *TurnError) Unwrap() error { return e.Err } + // Render consumes events until the channel closes, writing assistant text -// to out and tool/permission activity to errOut. Permission prompts are +// to out and tool/permission activity to errOut. A sub-agent's events (see +// Event.AgentName/Event.AgentDepth) are indented by their depth via +// out/errOut's own prefixWriter, bracketed by an "[agent] : started/done" line on +// EventAgentStarted/EventAgentComplete -- there is no collapsing in this +// renderer the way the TUI can offer one (see internal/terminal/tui), so +// indentation is the whole mechanism for telling a sub-agent's activity +// apart from its parent's in a plain-text stream. Permission prompts are // always denied since there is no interactive user to ask; callers that // want tools to run unattended should configure --permission-mode -// accept-edits or bypass instead. Returns the first terminal error, if any. +// accept-edits or bypass instead. Returns a *TurnError wrapping the root +// turn's outcome and error, if it didn't end on OutcomeCompleted (see +// agent.TurnResult.Outcome); nil otherwise. func Render(events <-chan agent.Event, out, errOut io.Writer) error { + outW := newPrefixWriter(out) + errW := newPrefixWriter(errOut) var runErr error + var runOutcome agent.Outcome for ev := range events { + outW.setPrefix(indentFor(ev.AgentDepth)) + errW.setPrefix(indentFor(ev.AgentDepth)) switch ev.Type { case agent.EventTextDelta: - fmt.Fprint(out, ev.Text) + fmt.Fprint(outW, ev.Text) case agent.EventToolCallStarted: - fmt.Fprintf(errOut, "\n[tool] %s(%s)\n", ev.ToolCall.Name, compact(ev.ToolCall.Input)) + fmt.Fprintf(errW, "\n[tool] %s(%s)\n", ev.ToolCall.Name, compact(ev.ToolCall.Input)) case agent.EventToolCallResult: r := ev.ToolResult switch { case r.Denied: - fmt.Fprintf(errOut, "[tool] %s: denied\n", r.Name) + fmt.Fprintf(errW, "[tool] %s: denied\n", r.Name) case r.Result.IsError: - fmt.Fprintf(errOut, "[tool] %s: error: %s\n", r.Name, r.Result.Content) + fmt.Fprintf(errW, "[tool] %s: error: %s\n", r.Name, r.Result.Content) default: - fmt.Fprintf(errOut, "[tool] %s: ok\n", r.Name) + fmt.Fprintf(errW, "[tool] %s: ok\n", r.Name) } case agent.EventPermissionRequested: pr := ev.PermissionRequest - fmt.Fprintf(errOut, "\n[permission] %s requires approval; denying in non-interactive mode "+ + fmt.Fprintf(errW, "\n[permission] %s requires approval; denying in non-interactive mode "+ "(rerun with --permission-mode=accept-edits or --permission-mode=bypass to allow)\n", pr.ToolCall.Name) pr.Respond(agent.PermissionResponse{Decision: permission.Deny}) - case agent.EventTurnComplete: - fmt.Fprintln(out) + case agent.EventAgentStarted: + fmt.Fprintf(errW, "\n[agent] %s: started (%s)\n", ev.Agent.Name, ev.Agent.PromptSummary) + case agent.EventAgentComplete: + fmt.Fprintf(errW, "[agent] %s: done\n", ev.AgentName) + case agent.EventTurnEnded: + // A sub-agent's own EventTurnEnded (AgentName != "main") is + // just another forwarded event, informational at most -- Spawn + // already turns a failed child into an error tool.Result, so + // its outcome must never set runErr, or a sub-agent that + // failed mid-investigation would flip the exit code of an + // otherwise-successful root turn that went on to recover from + // it (e.g. by trying something else after reading the + // failure). + completed := ev.Result != nil && ev.Result.Outcome == agent.OutcomeCompleted + if completed { + if ev.AgentName == "main" { + fmt.Fprintln(outW) + } + break + } + if ev.AgentName == "main" { + runErr = ev.Err + if ev.Result != nil { + runOutcome = ev.Result.Outcome + } + } + fmt.Fprintln(errW, describeOutcome(ev.Result, ev.Err)) case agent.EventRetrying: - fmt.Fprintf(errOut, "\n[retry] %s\n", render.DescribeRetry(ev.Err, ev.RetryAttempt, ev.RetryMaxAttempts, ev.RetryDelay)) - case agent.EventError: - runErr = ev.Err - cause, next := render.DescribeError(ev.Err) - msg := "\nError: " + cause - if next != "" { - msg += "\n " + next + fmt.Fprintf(errW, "\n[retry] %s\n", render.DescribeRetry(ev.Err, ev.RetryAttempt, ev.RetryMaxAttempts, ev.RetryDelay)) + } + } + if runOutcome == "" { + return nil + } + return &TurnError{Outcome: runOutcome, Err: runErr} +} + +// describeOutcome renders a non-completed TurnResult: the classified cause +// plus a suggested next step (via render.DescribeError, unchanged from the +// old EventError case -- it already degrades to err.Error() for a plain +// error like ErrBudgetExhausted/ErrMaxIterationsExceeded) when err is set, +// or a plain ": " line for an outcome that carries no +// error at all -- OutcomeAbandoned and OutcomeIncomplete aren't reachable +// yet (see TurnResult's doc comment) but render sensibly regardless. +func describeOutcome(result *agent.TurnResult, err error) string { + if err != nil { + cause, next := render.DescribeError(err) + msg := "\nError: " + cause + if next != "" { + msg += "\n " + next + } + return msg + } + if result.Reason != "" { + return fmt.Sprintf("\n%s: %s", result.Outcome, result.Reason) + } + return "\n" + string(result.Outcome) +} + +// indentFor returns the whitespace prefix for depth: "" at the root, +// indentWidth spaces per additional level. +func indentFor(depth int) string { + if depth == 0 { + return "" + } + return strings.Repeat(" ", indentWidth*depth) +} + +// prefixWriter indents every line written to it by whatever prefix is +// currently set, inserted right after each newline (and before the first +// byte written after construction or after a prefix change lands on a fresh +// line) -- so Render's fmt.Fprint/Fprintf calls never need to know about +// indentation themselves. State (atBOL) persists across SetPrefix calls +// deliberately: events for different agents interleave over the life of one +// Render call (see Event.AgentName/Event.AgentDepth), and which prefix +// applies to a given line is decided by whichever event's text started that +// line, not by whichever event happens to be current when the newline +// before it was written. +type prefixWriter struct { + w io.Writer + prefix string + atBOL bool +} + +func newPrefixWriter(w io.Writer) *prefixWriter { + return &prefixWriter{w: w, atBOL: true} +} + +func (p *prefixWriter) setPrefix(prefix string) { p.prefix = prefix } + +func (p *prefixWriter) Write(b []byte) (int, error) { + written := 0 + for len(b) > 0 { + if p.atBOL && p.prefix != "" { + if _, err := io.WriteString(p.w, p.prefix); err != nil { + return written, err } - fmt.Fprintln(errOut, msg) } + p.atBOL = false + idx := bytes.IndexByte(b, '\n') + if idx == -1 { + n, err := p.w.Write(b) + written += n + return written, err + } + n, err := p.w.Write(b[:idx+1]) + written += n + if err != nil { + return written, err + } + b = b[idx+1:] + p.atBOL = true } - return runErr + return written, nil } func compact(raw json.RawMessage) string { diff --git a/internal/terminal/oneshot/oneshot_test.go b/internal/terminal/oneshot/oneshot_test.go index c178ee3..83b209a 100644 --- a/internal/terminal/oneshot/oneshot_test.go +++ b/internal/terminal/oneshot/oneshot_test.go @@ -2,6 +2,7 @@ package oneshot import ( "bytes" + "errors" "strings" "testing" "time" @@ -12,9 +13,9 @@ import ( func TestRenderPrintsTextAndTurnComplete(t *testing.T) { events := make(chan agent.Event, 4) - events <- agent.Event{Type: agent.EventTextDelta, Text: "hello"} - events <- agent.Event{Type: agent.EventTextDelta, Text: " world"} - events <- agent.Event{Type: agent.EventTurnComplete} + events <- agent.Event{Type: agent.EventTextDelta, Text: "hello", AgentName: "main"} + events <- agent.Event{Type: agent.EventTextDelta, Text: " world", AgentName: "main"} + events <- agent.Event{Type: agent.EventTurnEnded, AgentName: "main", Result: &agent.TurnResult{Outcome: agent.OutcomeCompleted}} close(events) var out, errOut bytes.Buffer @@ -32,9 +33,10 @@ func TestRenderPrintsTextAndTurnComplete(t *testing.T) { // rendering so the two front ends don't diverge. func TestRenderPrintsClassifiedErrorAndNextStep(t *testing.T) { events := make(chan agent.Event, 1) + wireErr := &llm.Error{Kind: llm.ErrAuth, Provider: "anthropic", Message: "invalid x-api-key"} events <- agent.Event{ - Type: agent.EventError, - Err: &llm.Error{Kind: llm.ErrAuth, Provider: "anthropic", Message: "invalid x-api-key"}, + Type: agent.EventTurnEnded, AgentName: "main", Err: wireErr, + Result: &agent.TurnResult{Outcome: agent.OutcomeErrored, Reason: wireErr.Error()}, } close(events) @@ -48,6 +50,66 @@ func TestRenderPrintsClassifiedErrorAndNextStep(t *testing.T) { } } +// TestRenderIndentsSubAgentEvents checks a sub-agent's events (AgentDepth > +// 0) are indented, bracketed by an "[agent] ... started/done" pair in +// errOut, while the parent's own text stays unindented -- the whole +// mechanism this plain-text renderer has for telling a sub-agent's activity +// apart from its parent's (see prefixWriter's doc comment). +func TestRenderIndentsSubAgentEvents(t *testing.T) { + events := make(chan agent.Event, 6) + events <- agent.Event{Type: agent.EventAgentStarted, AgentName: "main/explorer", AgentDepth: 1, + Agent: &agent.AgentInfo{Name: "explorer", PromptSummary: "find the retry logic"}} + events <- agent.Event{Type: agent.EventTextDelta, AgentName: "main/explorer", AgentDepth: 1, Text: "line one\nline two"} + events <- agent.Event{Type: agent.EventAgentComplete, AgentName: "main/explorer", AgentDepth: 1} + events <- agent.Event{Type: agent.EventTextDelta, AgentName: "main", Text: "back to root"} + events <- agent.Event{Type: agent.EventTurnEnded, AgentName: "main", Result: &agent.TurnResult{Outcome: agent.OutcomeCompleted}} + close(events) + + var out, errOut bytes.Buffer + if err := Render(events, &out, &errOut); err != nil { + t.Fatalf("Render() error = %v", err) + } + + wantOut := " line one\n line twoback to root\n" + if got := out.String(); got != wantOut { + t.Errorf("out = %q, want %q", got, wantOut) + } + + errGot := errOut.String() + if !strings.Contains(errGot, "[agent] explorer: started (find the retry logic)") { + t.Errorf("errOut = %q, want an agent-started line", errGot) + } + if !strings.Contains(errGot, "[agent] main/explorer: done") { + t.Errorf("errOut = %q, want an agent-done line", errGot) + } +} + +// TestRenderSubAgentFailureDoesNotSetProcessExitError guards a real bug: a +// sub-agent's own non-completed EventTurnEnded (AgentName != "main") is +// informational, not the root turn's own outcome -- Spawn already turns a +// failed child into an error tool.Result the root agent can react to, so +// the child's raw failure must never flip Render's return value (and +// therefore the process exit code) even though the root turn itself goes on +// to complete successfully. +func TestRenderSubAgentFailureDoesNotSetProcessExitError(t *testing.T) { + events := make(chan agent.Event, 4) + events <- agent.Event{ + Type: agent.EventTurnEnded, AgentName: "main/explorer", AgentDepth: 1, Err: errors.New("explorer blew up"), + Result: &agent.TurnResult{Outcome: agent.OutcomeErrored, Reason: "explorer blew up"}, + } + events <- agent.Event{Type: agent.EventTextDelta, AgentName: "main", Text: "recovered and answered anyway"} + events <- agent.Event{Type: agent.EventTurnEnded, AgentName: "main", Result: &agent.TurnResult{Outcome: agent.OutcomeCompleted}} + close(events) + + var out, errOut bytes.Buffer + if err := Render(events, &out, &errOut); err != nil { + t.Fatalf("Render() error = %v, want nil -- the sub-agent's failure must not become the run's own error", err) + } + if !strings.Contains(errOut.String(), "explorer blew up") { + t.Errorf("errOut = %q, want the sub-agent's failure still reported informationally", errOut.String()) + } +} + // TestRenderPrintsRetryStatus checks an EventRetrying is surfaced to // errOut, so a rate-limit wait during a scripted or automated -p run is // visible instead of silent. @@ -58,7 +120,7 @@ func TestRenderPrintsRetryStatus(t *testing.T) { Err: &llm.Error{Kind: llm.ErrRateLimit, Provider: "anthropic"}, RetryAttempt: 1, RetryMaxAttempts: 3, RetryDelay: 5 * time.Second, } - events <- agent.Event{Type: agent.EventTurnComplete} + events <- agent.Event{Type: agent.EventTurnEnded, AgentName: "main", Result: &agent.TurnResult{Outcome: agent.OutcomeCompleted}} close(events) var out, errOut bytes.Buffer diff --git a/internal/terminal/tui/retry_error_test.go b/internal/terminal/tui/retry_error_test.go index 5175ce5..7f677ab 100644 --- a/internal/terminal/tui/retry_error_test.go +++ b/internal/terminal/tui/retry_error_test.go @@ -43,9 +43,10 @@ func TestHandleAgentEventErrorRendersCauseAndNextStep(t *testing.T) { a := newTestAgent() m := newModel(a, nil) + wireErr := &llm.Error{Kind: llm.ErrAuth, Provider: "anthropic", Message: "invalid x-api-key"} m.handleAgentEvent(agent.Event{ - Type: agent.EventError, - Err: &llm.Error{Kind: llm.ErrAuth, Provider: "anthropic", Message: "invalid x-api-key"}, + Type: agent.EventTurnEnded, Err: wireErr, + Result: &agent.TurnResult{Outcome: agent.OutcomeErrored, Reason: wireErr.Error()}, }) transcript := m.transcript.String() @@ -65,7 +66,11 @@ func TestHandleAgentEventErrorClearsRetryStatus(t *testing.T) { m := newModel(a, nil) m.retryStatus = "stale status" - m.handleAgentEvent(agent.Event{Type: agent.EventError, Err: errors.New("boom")}) + boom := errors.New("boom") + m.handleAgentEvent(agent.Event{ + Type: agent.EventTurnEnded, AgentName: "main", Err: boom, + Result: &agent.TurnResult{Outcome: agent.OutcomeErrored, Reason: boom.Error()}, + }) if m.retryStatus != "" { t.Errorf("retryStatus = %q, want cleared after a terminal error", m.retryStatus) diff --git a/internal/terminal/tui/scroll_pin_test.go b/internal/terminal/tui/scroll_pin_test.go index 67e4243..289050e 100644 --- a/internal/terminal/tui/scroll_pin_test.go +++ b/internal/terminal/tui/scroll_pin_test.go @@ -39,7 +39,7 @@ func TestScrollPinsUserMessageAtTop(t *testing.T) { if strings.Contains(m.viewport.View(), "padding line") { t.Fatalf("prior history must not be visible once pinned to the new message") } - // Mirrors the trailing newline EventTurnComplete appends once a turn finishes. + // Mirrors the trailing newline a completed EventTurnEnded appends once a turn finishes. m.transcript.WriteString("\n") m.refreshViewport() diff --git a/internal/terminal/tui/spawn_command_test.go b/internal/terminal/tui/spawn_command_test.go new file mode 100644 index 0000000..7f2ec44 --- /dev/null +++ b/internal/terminal/tui/spawn_command_test.go @@ -0,0 +1,110 @@ +package tui + +import ( + "strings" + "testing" + "time" + + "github.com/mchalapuk/coded/internal/llm" +) + +func TestPromptAfterFields(t *testing.T) { + cases := []struct { + text string + n int + want string + }{ + {"/spawn explorer find the retry logic", 2, "find the retry logic"}, + {"/spawn explorer find it over there", 2, "find it over there"}, // internal spacing preserved + {"/spawn explorer", 2, ""}, + {"/spawn", 2, ""}, + } + for _, c := range cases { + if got := promptAfterFields(c.text, c.n); got != c.want { + t.Errorf("promptAfterFields(%q, %d) = %q, want %q", c.text, c.n, got, c.want) + } + } +} + +// TestSpawnCommandUsageWithoutPrompt confirms too few fields is a usage +// message, not a call with an empty prompt. +func TestSpawnCommandUsageWithoutPrompt(t *testing.T) { + m := newSubAgentTestModel() + m.handleSlash("/spawn explorer") + + if !strings.Contains(m.transcript.String(), "Usage: /spawn") { + t.Errorf("transcript = %q, want a usage message", m.transcript.String()) + } + if m.mode == uiRunning { + t.Errorf("mode = uiRunning, want unchanged -- a usage error must not start a run") + } +} + +// TestSpawnCommandUnknownAgentReportsAvailableOnes confirms an unknown +// agent name is rejected before anything runs, naming what IS available +// rather than leaving the user to guess. +func TestSpawnCommandUnknownAgentReportsAvailableOnes(t *testing.T) { + m := newSubAgentTestModel() + m.handleSlash("/spawn does-not-exist find something") + + transcript := m.transcript.String() + if !strings.Contains(transcript, `Unknown agent "does-not-exist"`) { + t.Errorf("transcript = %q, want it to name the unknown agent", transcript) + } + if !strings.Contains(transcript, "explorer") { + t.Errorf("transcript = %q, want it to list \"explorer\" as available", transcript) + } + if m.mode == uiRunning { + t.Errorf("mode = uiRunning, want unchanged -- an unknown agent must not start a run") + } +} + +// TestSpawnCommandStartsARunForAKnownAgent confirms a valid /spawn call +// switches into running mode and sets m.events to a channel that will +// eventually close, mirroring how submitting ordinary text starts a turn. +// +// The spawned child shares m.agent.Provider with the root model (see +// runSpawnCommand), so the scripted turn given to newTestAgent here has to +// be a compliant response under *explorer's* protocol +// (/), not react's -- it's the child's own +// protocol.Agent that parses whatever this fake provider hands back, +// regardless of which protocol the root model nominally runs. +func TestSpawnCommandStartsARunForAKnownAgent(t *testing.T) { + explorerCompliantTurn := []llm.Event{ + {Type: llm.EventTextDelta, Text: "lookingdone"}, + {Type: llm.EventMessageStop, StopReason: llm.StopEndTurn}, + } + m := &model{width: 80, agent: newTestAgent(explorerCompliantTurn)} + m.viewport.Width = 80 + m.viewport.Height = 20 + + cmd := m.handleSlash("/spawn explorer find something") + + if m.mode != uiRunning { + t.Fatalf("mode = %v, want uiRunning", m.mode) + } + if m.events == nil { + t.Fatalf("m.events not set") + } + if cmd == nil { + t.Fatalf("handleSlash(\"/spawn ...\") returned a nil tea.Cmd, want one that waits on the event stream") + } + + // Drain to completion so the test doesn't leak the spawn goroutine -- + // explorer has no scripted turns, so its own agent.Run will error out + // (fakeProvider panics on an unscripted call, recovered as a Go test + // failure only if the panic escapes the goroutine -- draining the + // channel here is what surfaces that promptly instead of the test just + // hanging). + deadline := time.After(2 * time.Second) + for { + select { + case _, ok := <-m.events: + if !ok { + return + } + case <-deadline: + t.Fatal("timed out draining the spawn's event channel") + } + } +} diff --git a/internal/terminal/tui/subagent_event_test.go b/internal/terminal/tui/subagent_event_test.go new file mode 100644 index 0000000..8cff4bb --- /dev/null +++ b/internal/terminal/tui/subagent_event_test.go @@ -0,0 +1,146 @@ +package tui + +import ( + "strings" + "testing" + + "github.com/mchalapuk/coded/internal/agent" + "github.com/mchalapuk/coded/internal/llm" + "github.com/mchalapuk/coded/internal/tool" +) + +func newSubAgentTestModel() *model { + m := &model{width: 80, agent: newTestAgent()} + m.viewport.Width = 80 + m.viewport.Height = 20 + return m +} + +// TestSubAgentEventsAreIndentedAndBracketed drives a full sub-agent event +// sequence -- EventAgentStarted, its own text, a tool result, then +// EventAgentComplete -- through handleAgentEvent, and checks the resulting +// transcript brackets the run, indents everything inside it via indentLines +// (whose 2-space margin is prepended outside any ANSI styling -- see its +// doc comment -- so a plain HasPrefix check is reliable here), and keeps +// that content in the right order relative to what comes after. +func TestSubAgentEventsAreIndentedAndBracketed(t *testing.T) { + m := newSubAgentTestModel() + + m.handleAgentEvent(agent.Event{ + Type: agent.EventAgentStarted, AgentName: "main/explorer", AgentDepth: 1, + Agent: &agent.AgentInfo{Name: "explorer", PromptSummary: "find the retry logic"}, + }) + m.handleAgentEvent(agent.Event{Type: agent.EventTextDelta, AgentName: "main/explorer", AgentDepth: 1, Text: "looking"}) + m.handleAgentEvent(agent.Event{ + Type: agent.EventToolCallResult, AgentName: "main/explorer", AgentDepth: 1, + ToolResult: &agent.ToolResultInfo{Name: "Grep", Result: tool.Result{Content: "retry.go:12"}}, + }) + m.handleAgentEvent(agent.Event{Type: agent.EventAgentComplete, AgentName: "main/explorer", AgentDepth: 1}) + m.handleAgentEvent(agent.Event{Type: agent.EventTextDelta, AgentName: "main", Text: "back at the root"}) + m.handleAgentEvent(agent.Event{Type: agent.EventTurnEnded, AgentName: "main", Result: &agent.TurnResult{Outcome: agent.OutcomeCompleted}}) + + transcript := m.transcript.String() + + startIdx := strings.Index(transcript, "→ explorer: find the retry logic") + resultIdx := strings.Index(transcript, "retry.go:12") + completeIdx := strings.Index(transcript, "← main/explorer: done") + rootIdx := strings.Index(transcript, "back at the root") + if startIdx == -1 || resultIdx == -1 || completeIdx == -1 || rootIdx == -1 { + t.Fatalf("transcript missing an expected line:\n%s", transcript) + } + if !(startIdx < resultIdx && resultIdx < completeIdx && completeIdx < rootIdx) { + t.Fatalf("transcript lines out of order (want started < result < complete < root text):\n%s", transcript) + } + + var sawIndentedSearch, sawIndentedResult bool + for _, l := range strings.Split(transcript, "\n") { + if strings.Contains(l, "looking") && strings.HasPrefix(l, " ") { + sawIndentedSearch = true + } + if strings.Contains(l, "retry.go:12") && strings.HasPrefix(l, " ") { + sawIndentedResult = true + } + } + if !sawIndentedSearch { + t.Errorf("sub-agent text was not indented:\n%s", transcript) + } + if !sawIndentedResult { + t.Errorf("sub-agent tool result was not indented:\n%s", transcript) + } +} + +func TestSubAgentIndent(t *testing.T) { + if got := subAgentIndent(0); got != "" { + t.Errorf("subAgentIndent(0) = %q, want \"\"", got) + } + if got := subAgentIndent(1); got != " " { + t.Errorf("subAgentIndent(1) = %q, want 2 spaces", got) + } + if got := subAgentIndent(2); got != " " { + t.Errorf("subAgentIndent(2) = %q, want 4 spaces", got) + } +} + +func TestIndentLines(t *testing.T) { + if got := indentLines("a\nb\nc", " "); got != " a\n b\n c" { + t.Errorf("indentLines multi-line = %q", got) + } + if got := indentLines("no indent", ""); got != "no indent" { + t.Errorf("indentLines with empty indent should be a no-op, got %q", got) + } + if got := indentLines("", " "); got != "" { + t.Errorf("indentLines(\"\", ...) = %q, want \"\"", got) + } +} + +// TestSubAgentUsageCountsTowardTotalAndBreakdown guards the gap +// handleSubAgentEvent's first cut left open: a sub-agent's EventUsage has +// no matching case unless one is added, so its tokens would silently never +// reach the banner at all. This checks both the session-wide total (m.usage) +// and the per-agent breakdown pick up a sub-agent's usage correctly. +func TestSubAgentUsageCountsTowardTotalAndBreakdown(t *testing.T) { + m := newSubAgentTestModel() + + m.handleAgentEvent(agent.Event{Type: agent.EventUsage, AgentName: "main", Usage: &llm.Usage{InputTokens: 100, OutputTokens: 20}}) + m.handleAgentEvent(agent.Event{ + Type: agent.EventUsage, AgentName: "main/explorer", AgentDepth: 1, + Usage: &llm.Usage{InputTokens: 30, OutputTokens: 5}, + }) + + if m.usage.InputTokens != 130 || m.usage.OutputTokens != 25 { + t.Fatalf("m.usage = %+v, want combined root+sub-agent totals (130, 25)", m.usage) + } + + root := m.usageByAgent["main"] + if root.InputTokens != 100 || root.OutputTokens != 20 { + t.Errorf("usageByAgent[\"main\"] = %+v, want the root's own (100, 20)", root) + } + sub := m.usageByAgent["main/explorer"] + if sub.InputTokens != 30 || sub.OutputTokens != 5 { + t.Errorf("usageByAgent[\"main/explorer\"] = %+v, want (30, 5)", sub) + } +} + +func TestFormatUsageBreakdownSilentWithOnlyRoot(t *testing.T) { + byAgent := map[string]llm.Usage{"main": {InputTokens: 10}} + if got := formatUsageBreakdown(byAgent); got != nil { + t.Errorf("formatUsageBreakdown with only the root entry = %v, want nil", got) + } +} + +func TestFormatUsageBreakdownRootFirst(t *testing.T) { + byAgent := map[string]llm.Usage{ + "main": {InputTokens: 100, OutputTokens: 20}, + "main/explorer": {InputTokens: 30, OutputTokens: 5}, + } + got := formatUsageBreakdown(byAgent) + if len(got) != 2 { + t.Fatalf("formatUsageBreakdown() = %v, want 2 lines", got) + } + if !strings.HasPrefix(got[0], "main:") { + t.Errorf("first line = %q, want the root (\"main:\") first", got[0]) + } + if !strings.HasPrefix(got[1], "main/explorer:") { + t.Errorf("second line = %q, want the sub-agent's own", got[1]) + } +} diff --git a/internal/terminal/tui/tui.go b/internal/terminal/tui/tui.go index 6ca2fbd..fddab2c 100644 --- a/internal/terminal/tui/tui.go +++ b/internal/terminal/tui/tui.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "regexp" + "sort" "strings" "github.com/charmbracelet/bubbles/spinner" @@ -20,11 +21,13 @@ import ( "github.com/charmbracelet/x/ansi" "github.com/mchalapuk/coded/internal/agent" + "github.com/mchalapuk/coded/internal/agent/definition" "github.com/mchalapuk/coded/internal/agent/protocol" "github.com/mchalapuk/coded/internal/llm" "github.com/mchalapuk/coded/internal/memory/session" "github.com/mchalapuk/coded/internal/permission" "github.com/mchalapuk/coded/internal/terminal/render" + "github.com/mchalapuk/coded/internal/tool/builtin/spawn" ) var ( @@ -225,6 +228,20 @@ type model struct { // once the run is interrupted by a tool call or the turn completes. pendingAssistant string + // pendingSubText/pendingSubIndent mirror pendingAssistant for a + // sub-agent's text (see Event.AgentName): buffered raw text plus the + // left margin it renders with, flushed into transcript on + // EventAgentComplete or the next tool event at the same path (see + // flushSubText). Not run through renderAssistantBlock's + // m.agent.Protocol()-based section parsing -- that protocol is the + // root agent's, and a sub-agent's raw text is tagged under its own, + // different protocol (e.g. explorer's /), so + // parsing it with the wrong section names would silently misrender + // it. This is deliberately the plain, ungrouped first cut (PLAN.md's + // Sub-agents section); a collapsible group with per-agent-aware + // rendering is follow-up work, not this one. + pendingSubText, pendingSubIndent string + // wrapped is the transcript word-wrapped to wrapWidth; it's what's // actually handed to the viewport. Cached so sticky-header rendering // (which runs on every View, not just on content changes) doesn't have @@ -236,10 +253,21 @@ type model struct { savedIdx int // usage accumulates token counts across every provider turn seen so far - // this session, so the tokens banner reads as a running total rather - // than resetting each turn -- see handleAgentEvent's EventUsage case. + // this session -- the root agent's own and every sub-agent's combined + // -- so the tokens banner reads as a running session-wide total rather + // than resetting each turn. See handleAgentEvent's and + // handleSubAgentEvent's EventUsage cases. usage llm.Usage + // usageByAgent breaks the same totals down by contributing agent, keyed + // by its Event.AgentName ("main" for the root). Kept alongside + // usage rather than replacing it: most sessions only ever have one key + // (the root's own), and recomputing a sum from the map on every render + // would be needless work for that overwhelmingly common case -- see + // formatUsageBreakdown, which only has anything to say once a second + // key appears. + usageByAgent map[string]llm.Usage + permReq *agent.PermissionRequest permCursor int @@ -384,16 +412,19 @@ func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m.handleAgentEvent(agent.Event(msg)) case agentDoneMsg: - // The agent stopped -- whether it finished cleanly or errored out, - // the events channel closed and this fired -- so this is the one - // place a per-stop usage summary belongs, rather than duplicating it - // across EventTurnComplete and EventError. + // The agent stopped -- whatever EventTurnEnded's Outcome was, the + // events channel closed and this fired -- so this is the one place + // a per-stop usage summary belongs, rather than duplicating it + // across every possible Outcome. if summary := formatUsage(m.usage); summary != "" { // 2-column left margin, matching the assistant text column // (renderAssistantBlock's PaddingLeft(2)) this summary sits // directly under -- left outside styleSystem's Render so the // margin itself isn't painted with the style's background. m.writeRaw(" " + styleSystem.Render(summary)) + for _, line := range formatUsageBreakdown(m.usageByAgent) { + m.writeRaw(" " + styleSystem.Render(line)) + } } m.mode = uiInput m.retryStatus = "" @@ -566,6 +597,7 @@ func (m *model) handleSlash(text string) tea.Cmd { " /login [provider] log in and store an API key\n" + " /logout [provider] remove a stored API key\n" + " /model [family] show the model menu, or set it directly (e.g. /model Sonnet)\n" + + " /spawn run a sub-agent directly, without the model\n" + " /quit quit coded") case "/clear": m.clearConversation() @@ -579,6 +611,8 @@ func (m *model) handleSlash(text string) tea.Cmd { } else { m.openModelMenu() } + case "/spawn": + return m.runSpawnCommand(text, fields) case "/quit": m.quitting = true return tea.Quit @@ -588,6 +622,114 @@ func (m *model) handleSlash(text string) tea.Cmd { return nil } +// runSpawnCommand implements "/spawn ": runs the named +// sub-agent directly, bypassing the model entirely -- see PLAN.md's +// Sub-agents section, "so the user can divide work into chats by hand +// without a workflow". This constructs the exact same spawn.Tool the model +// would call as the Spawn tool, so the resulting event stream (and +// therefore the transcript) is identical either way; only how the call +// gets triggered differs. +func (m *model) runSpawnCommand(text string, fields []string) tea.Cmd { + if len(fields) < 3 { + m.writeSystem("Usage: /spawn ") + return nil + } + agentName := fields[1] + prompt := promptAfterFields(text, 2) + + defs := definition.Spawnable() + if _, ok := defs.Get(agentName); !ok { + m.writeSystem(fmt.Sprintf("Unknown agent %q. Available: %s", agentName, strings.Join(spawnableNames(defs), ", "))) + return nil + } + + spawnTool := &spawn.Tool{ + Provider: m.agent.Provider, + Tools: m.agent.Tools, + Perm: m.agent.Permission, + Definitions: defs, + Session: m.sess, + } + + m.mode = uiRunning + m.retryStatus = "" + ch := runSpawnTool(spawnTool, agentName, prompt) + m.events = ch + return waitForEvent(ch) +} + +// promptAfterFields returns text with its first n whitespace-delimited +// fields (and the whitespace immediately after them) removed, preserving +// whatever spacing the rest of the original text had -- unlike +// strings.Fields()[n:], which would collapse internal whitespace in a +// multi-line or multiply-spaced prompt when rejoined. +func promptAfterFields(text string, n int) string { + rest := text + for i := 0; i < n; i++ { + rest = strings.TrimLeft(rest, " \t") + idx := strings.IndexAny(rest, " \t") + if idx == -1 { + return "" + } + rest = rest[idx:] + } + return strings.TrimLeft(rest, " \t") +} + +// spawnableNames lists defs' definitions by name, for the /spawn command's +// "unknown agent" error message. +func spawnableNames(defs *definition.Registry) []string { + list := defs.List() + names := make([]string, len(list)) + for i, d := range list { + names[i] = d.Name + } + return names +} + +// runSpawnTool runs t.ExecuteReporting on its own goroutine, relaying +// everything it reports onto the returned channel exactly the way +// m.agent.Run's own channel behaves -- closed after exactly one +// EventTurnEnded, whatever its Outcome, so handleAgentEvent's existing +// agentDoneMsg handling (the usage-summary line, returning focus to the +// input box) needs no /spawn-specific case at all. +func runSpawnTool(t *spawn.Tool, agentName, prompt string) <-chan agent.Event { + out := make(chan agent.Event, 16) + go func() { + defer close(out) + input, err := json.Marshal(map[string]string{"agent": agentName, "prompt": prompt}) + if err != nil { + out <- agent.Event{Type: agent.EventTurnEnded, Err: err, Result: &agent.TurnResult{Outcome: agent.OutcomeForError(err), Reason: err.Error()}} + return + } + res, err := t.ExecuteReporting(context.Background(), input, chanReporter{out}) + if err != nil { + out <- agent.Event{Type: agent.EventTurnEnded, Err: err, Result: &agent.TurnResult{Outcome: agent.OutcomeForError(err), Reason: err.Error()}} + return + } + if res.IsError { + resErr := fmt.Errorf("%s", res.Content) + out <- agent.Event{Type: agent.EventTurnEnded, Err: resErr, Result: &agent.TurnResult{Outcome: agent.OutcomeErrored, Reason: res.Content}} + return + } + out <- agent.Event{Type: agent.EventTurnEnded, Result: &agent.TurnResult{Outcome: agent.OutcomeCompleted}} + }() + return out +} + +// chanReporter implements tool.Reporter over an Event channel -- the +// /spawn command's own copy of the same bridge agent.DispatchToolCalls +// uses internally (see its unexported reporterAdapter) to let a +// tool.EventReportingTool emit agent.Event values without the tool package +// importing this one. +type chanReporter struct{ out chan<- agent.Event } + +func (r chanReporter) Report(v any) { + if ev, ok := v.(agent.Event); ok { + r.out <- ev + } +} + // clearConversation implements "/clear": wipes the agent's message history // and the rendered transcript, then starts a brand new on-disk session so // old and new conversations never share a transcript file -- mirroring how @@ -601,6 +743,7 @@ func (m *model) clearConversation() { m.reflowSpans = nil m.pendingAssistant = "" m.usage = llm.Usage{} + m.usageByAgent = nil m.savedIdx = 0 cwd, err := os.Getwd() @@ -768,6 +911,9 @@ func (m *model) runAuthLogout(fields []string) { } func (m *model) handleAgentEvent(ev agent.Event) (tea.Model, tea.Cmd) { + if ev.AgentName != "main" { + return m.handleSubAgentEvent(ev) + } switch ev.Type { case agent.EventTextDelta: m.appendAssistant(ev.Text) @@ -780,12 +926,7 @@ func (m *model) handleAgentEvent(ev agent.Event) (tea.Model, tea.Cmd) { case agent.EventToolStepResult: m.writeToolStepResult(ev.ToolStepResult) case agent.EventUsage: - if ev.Usage != nil { - m.usage.InputTokens += ev.Usage.InputTokens - m.usage.OutputTokens += ev.Usage.OutputTokens - m.usage.CacheReadInputTokens += ev.Usage.CacheReadInputTokens - m.usage.CacheCreationInputTokens += ev.Usage.CacheCreationInputTokens - } + m.recordUsage(ev.AgentName, ev.Usage) case agent.EventPermissionRequested: m.permReq = ev.PermissionRequest m.permCursor = 0 @@ -794,18 +935,90 @@ func (m *model) handleAgentEvent(ev agent.Event) (tea.Model, tea.Cmd) { return m, nil // wait for handleKey to resolve; do not re-arm waitForEvent yet case agent.EventRetrying: m.retryStatus = render.DescribeRetry(ev.Err, ev.RetryAttempt, ev.RetryMaxAttempts, ev.RetryDelay) - case agent.EventError: + case agent.EventTurnEnded: m.retryStatus = "" - cause, next := render.DescribeError(ev.Err) - msg := "Error: " + cause - if next != "" { - msg += "\n " + next + if ev.Result != nil && ev.Result.Outcome == agent.OutcomeCompleted { + m.flushAssistant() + m.transcript.WriteString("\n") + m.refreshViewport() + break } - m.writeSystem(msg) - case agent.EventTurnComplete: + m.writeSystem(describeTurnOutcome(ev.Result, ev.Err)) + } + if m.mode == uiPermission { + return m, nil + } + return m, waitForEvent(m.events) +} + +// handleSubAgentEvent is handleAgentEvent's branch for any event whose +// AgentName isn't "main" (see Event.AgentName): a flat, indented rendering, +// bracketed by "→ name: summary" / "← name: done" lines -- no collapsing, no +// protocol-aware section parsing (see pendingSubText's doc comment for why +// not). Permission requests and retry status are handled exactly like a +// root-level event: the same prompt UI and mode switch apply regardless of +// which agent in the tree asked, since the permission engine is shared +// across the whole tree (see PLAN.md's Sub-agents section). +func (m *model) handleSubAgentEvent(ev agent.Event) (tea.Model, tea.Cmd) { + indent := subAgentIndent(ev.AgentDepth) + switch ev.Type { + case agent.EventAgentStarted: m.flushAssistant() - m.transcript.WriteString("\n") + name, summary := "", "" + if ev.Agent != nil { + name, summary = ev.Agent.Name, ev.Agent.PromptSummary + } + m.writeRaw(indent + styleSystem.Render(fmt.Sprintf("→ %s: %s", name, summary))) + case agent.EventTextDelta: + m.pendingSubText += ev.Text + m.pendingSubIndent = indent m.refreshViewport() + case agent.EventToolCallStarted: + m.flushSubText() + m.writeRaw(indentLines(styleTool.Render(formatToolCall(ev.ToolCall)), indent)) + case agent.EventToolCallResult: + m.flushSubText() + r := ev.ToolResult + style := styleToolOutput + if r.Denied || r.Result.IsError { + style = styleToolError + } + m.writeRaw(indentLines(style.Render(r.Result.Content), indent)) + case agent.EventToolStepStarted: + m.flushSubText() + m.writeRaw(indentLines(styleTool.Render(fmt.Sprintf("%s[%s]", ev.ToolStep.ToolName, ev.ToolStep.Label)), indent)) + case agent.EventToolStepResult: + m.flushSubText() + style := styleToolOutput + if ev.ToolStepResult.Denied || ev.ToolStepResult.Outcome.IsError { + style = styleToolError + } + m.writeRaw(indentLines(style.Render(ev.ToolStepResult.Outcome.Output), indent)) + case agent.EventAgentComplete: + m.flushSubText() + m.writeRaw(indent + styleSystem.Render(fmt.Sprintf("← %s: done", ev.AgentName))) + case agent.EventUsage: + m.recordUsage(ev.AgentName, ev.Usage) + case agent.EventPermissionRequested: + m.permReq = ev.PermissionRequest + m.permCursor = 0 + m.mode = uiPermission + m.scrollToBottom() + return m, nil + case agent.EventRetrying: + m.retryStatus = render.DescribeRetry(ev.Err, ev.RetryAttempt, ev.RetryMaxAttempts, ev.RetryDelay) + case agent.EventTurnEnded: + // A sub-agent's own EventTurnEnded is just another event on its + // forwarded stream (see ForwardChild) -- Spawn's own + // EventAgentComplete, not this, is what brackets its run visibly + // (see handleSubAgentEvent's EventAgentComplete case above). A + // clean finish is silent here; anything else surfaces the same way + // a root-level failure does, just indented. + if ev.Result != nil && ev.Result.Outcome == agent.OutcomeCompleted { + break + } + m.flushSubText() + m.writeRaw(indentLines(styleSystem.Render(describeTurnOutcome(ev.Result, ev.Err)), indent)) } if m.mode == uiPermission { return m, nil @@ -813,6 +1026,96 @@ func (m *model) handleAgentEvent(ev agent.Event) (tea.Model, tea.Cmd) { return m, waitForEvent(m.events) } +// describeTurnOutcome renders a non-completed TurnResult: the classified +// cause plus a suggested next step (via render.DescribeError, which +// degrades to err.Error() for a plain error like +// agent.ErrBudgetExhausted/agent.ErrMaxIterationsExceeded) when err is +// set, or a plain ": " line for an outcome that carries +// no error at all -- agent.OutcomeAbandoned and agent.OutcomeIncomplete +// aren't reachable yet (see agent.TurnResult's doc comment) but render +// sensibly regardless. A package-local twin of oneshot.describeOutcome: +// same shape, different rendering target (no leading "\n" -- writeSystem +// and writeRaw already separate blocks with their own blank lines). +func describeTurnOutcome(result *agent.TurnResult, err error) string { + if err != nil { + cause, next := render.DescribeError(err) + msg := "Error: " + cause + if next != "" { + msg += "\n " + next + } + return msg + } + if result == nil { + return string(agent.OutcomeErrored) + } + if result.Reason != "" { + return fmt.Sprintf("%s: %s", result.Outcome, result.Reason) + } + return string(result.Outcome) +} + +// recordUsage accumulates u into both m.usage (the session-wide total -- see +// its doc comment) and m.usageByAgent[key], the per-agent breakdown +// formatUsageBreakdown renders from. key is ev.AgentName ("main" for the +// root agent's own events) -- see handleAgentEvent and +// handleSubAgentEvent's EventUsage cases, this method's only two callers. +func (m *model) recordUsage(key string, u *llm.Usage) { + if u == nil { + return + } + m.usage.InputTokens += u.InputTokens + m.usage.OutputTokens += u.OutputTokens + m.usage.CacheReadInputTokens += u.CacheReadInputTokens + m.usage.CacheCreationInputTokens += u.CacheCreationInputTokens + + if m.usageByAgent == nil { + m.usageByAgent = make(map[string]llm.Usage) + } + agentUsage := m.usageByAgent[key] + agentUsage.InputTokens += u.InputTokens + agentUsage.OutputTokens += u.OutputTokens + agentUsage.CacheReadInputTokens += u.CacheReadInputTokens + agentUsage.CacheCreationInputTokens += u.CacheCreationInputTokens + m.usageByAgent[key] = agentUsage +} + +// flushSubText renders pendingSubText (see its doc comment) into transcript +// and clears it. Called whenever a tool event or EventAgentComplete at the +// same path needs to follow it, mirroring flushAssistant's role for +// pendingAssistant. +func (m *model) flushSubText() { + if m.pendingSubText == "" { + return + } + m.writeRaw(indentLines(styleAssistant.Render(m.pendingSubText), m.pendingSubIndent)) + m.pendingSubText = "" + m.pendingSubIndent = "" +} + +// subAgentIndent returns the left margin for a sub-agent event at depth: +// two columns per level, so nested spawns (once possible) step in further +// than a first-level child. +func subAgentIndent(depth int) string { + if depth == 0 { + return "" + } + return strings.Repeat(" ", depth) +} + +// indentLines prefixes every line of text with indent -- text may already +// be ANSI-styled (see handleSubAgentEvent's callers), so this only ever +// touches line boundaries, never the content between them. +func indentLines(text, indent string) string { + if indent == "" || text == "" { + return text + } + lines := strings.Split(text, "\n") + for i, l := range lines { + lines[i] = indent + l + } + return strings.Join(lines, "\n") +} + func (m *model) resolvePermission(optionIdx int) { if m.permReq == nil { return @@ -1108,6 +1411,36 @@ func formatUsage(u llm.Usage) string { return text } +// formatUsageBreakdown renders one line per contributing agent in byAgent, +// sorted by key with "main" (the root) always first -- deliberately silent +// (returns nil) when there's only the root's own entry, since that's every +// session before a sub-agent ever ran, and repeating the exact total +// formatUsage already printed would be noise rather than a breakdown. +func formatUsageBreakdown(byAgent map[string]llm.Usage) []string { + if len(byAgent) < 2 { + return nil + } + keys := make([]string, 0, len(byAgent)) + for k := range byAgent { + keys = append(keys, k) + } + sort.Slice(keys, func(i, j int) bool { + if keys[i] == "main" { + return true + } + if keys[j] == "main" { + return false + } + return keys[i] < keys[j] + }) + + lines := make([]string, 0, len(keys)) + for _, k := range keys { + lines = append(lines, fmt.Sprintf("%s: %s", k, formatUsage(byAgent[k]))) + } + return lines +} + // formatTokenCount renders a token count the way Claude Code's own status // output does: exact below 1k, one decimal of k/m above it. func formatTokenCount(n int) string { diff --git a/internal/tool/builtin/spawn/spawn.go b/internal/tool/builtin/spawn/spawn.go new file mode 100644 index 0000000..d8b6714 --- /dev/null +++ b/internal/tool/builtin/spawn/spawn.go @@ -0,0 +1,329 @@ +// Package spawn implements the built-in Spawn tool: invokes a named agent +// definition with a prompt, runs it to completion in an isolated +// conversation, and returns its final answer as the tool result. See +// PLAN.md's Sub-agents section: fresh context is the point, so the caller +// gets the conclusion, not the sub-agent's own tool traffic. +package spawn + +import ( + "context" + "encoding/json" + "fmt" + "os" + "strings" + + "github.com/mchalapuk/coded/internal/agent" + "github.com/mchalapuk/coded/internal/agent/definition" + "github.com/mchalapuk/coded/internal/agent/protocol" + "github.com/mchalapuk/coded/internal/llm" + "github.com/mchalapuk/coded/internal/memory/project" + "github.com/mchalapuk/coded/internal/memory/session" + "github.com/mchalapuk/coded/internal/permission" + "github.com/mchalapuk/coded/internal/tool" +) + +// maxResultChars bounds how much of a sub-agent's final answer Spawn +// returns to its caller -- every tool bounds its own output (see +// internal/tool's package doc comment), and a sub-agent's whole point is to +// come back with a pointer-sized answer, not a second copy of whatever it +// read. Generous relative to a file-read tool's cap since the content here +// is already distilled, not raw source. +const maxResultChars = 8000 + +// description is the tool schema's Description -- the one piece of this +// whole feature that decides whether any of the rest of it ever fires (see +// PLAN.md's Sub-agents section). It states the breadth heuristic +// explicitly rather than leaving "when should I delegate" for the model to +// guess: delegate when the search spans many files, the location is +// genuinely unknown, or the answer is a pointer rather than something to +// read in full -- not when a single Grep or Read would already answer the +// question. The failure signature to watch for is the same shape as an +// unnecessary tool call anywhere else: an explorer spawned for something +// one direct call would have answered, or a parent that re-reads every +// file the explorer already named instead of trusting the pointer. +const description = `Runs a named sub-agent to completion in its own, isolated conversation and returns ` + + `only its final answer -- not its tool calls, not its intermediate reasoning. Use this to delegate a ` + + `broad or open-ended search (many candidate files, an unknown location, a question whose answer is a ` + + `pointer rather than something to read in full) to a sub-agent that reports back file:line pointers with ` + + `short claims, so you don't have to hold every file it looked at in your own context. Do not use this for ` + + `something a single Read or Grep call would already answer directly -- that costs a whole extra ` + + `conversation for no benefit over calling the tool yourself. Trust the pointers a sub-agent reports back; ` + + `re-reading every file it names defeats the reason to have spawned it at all.` + +// Tool is the built-in Spawn tool. Provider, Tools, and Budget mirror the +// spawning agent's own -- passed as plain fields rather than a live +// *agent.Agent reference, since Tool is constructed and registered into a +// tool.Registry before the agent that will hold that registry exists (see +// cmd/coded's wiring order); there is no back-reference available to hold +// at construction time, only the pieces a child actually needs to inherit. +type Tool struct { + // Provider is the LLM provider a spawned child streams against -- the + // same one the spawning agent itself uses. + Provider llm.Provider + // Tools is the spawning agent's own effective tool registry -- what a + // child's own Tools restriction narrows against (see + // definition.NewChild), so a child can never gain a tool its parent + // doesn't have. + Tools *tool.Registry + // Perm is the single permission.Engine shared across the whole tree + // (see PLAN.md's Sub-agents section) -- prompts serialize through it + // regardless of which agent asked. + Perm *permission.Engine + // Budget, if set, is the spawning agent's own budget; a child shares it + // via Budget.Sub rather than getting a fresh allocation. Nil (the + // common case today, since nothing sets a root Budget yet) means + // unlimited, same as for any agent. + Budget *agent.Budget + // Definitions holds the agent definitions Spawn may target -- a + // deliberately narrower registry than definition.Builtins() at the + // call site that constructs this Tool: "main" itself is never a valid + // target, since a nested unrestricted agent isn't a capability this + // milestone means to expose, only the built-ins meant to be spawned + // (explorer today). + Definitions *definition.Registry + // Session, if set, is the spawning agent's own session -- used to + // create a child session (see session.Session.NewChild) that the + // spawned agent's messages are persisted into, so `coded sessions + // list` can find it via the parent's ChildIDs even though it never + // appears as a top-level entry itself. Nil (e.g. in tests, or + // cmd/promptdump's throwaway runs) skips persistence entirely rather + // than failing the spawn over it. + Session *session.Session +} + +func (t *Tool) Name() string { return "Spawn" } + +func (t *Tool) Description() string { return description } + +func (t *Tool) InputSchema() json.RawMessage { + names := make([]string, 0) + if t.Definitions != nil { + for _, d := range t.Definitions.List() { + names = append(names, d.Name) + } + } + schema, err := json.Marshal(map[string]any{ + "type": "object", + "properties": map[string]any{ + "agent": map[string]any{ + "type": "string", + "description": "Which sub-agent to run.", + "enum": names, + }, + "prompt": map[string]any{ + "type": "string", + "description": "The task for the sub-agent to do, stated as its entire goal -- it has no other context from this conversation.", + }, + }, + "required": []string{"agent", "prompt"}, + }) + if err != nil { + panic(err) // programmer error: the literal map above is always valid + } + return schema +} + +// Risk is informational only: Spawn is deliberately unpermissioned (see +// ExecuteReporting's doc comment), so this value is never consulted by +// permission.Engine.Check for the call itself -- only for whatever a +// renderer wants to show. RiskExec is the closest fit: a spawned agent may +// end up running the same range of actions a direct tool call could, +// mediated through its own turn loop rather than this call. +func (t *Tool) Risk() tool.Risk { return tool.RiskExec } + +// Subject returns the zero value: Spawn is never checked against a +// permission rule (see ExecuteReporting), so there is nothing here worth +// matching on -- the same allowance Tool.Subject's own doc comment +// describes for a call with nothing to report. +func (t *Tool) Subject(json.RawMessage) tool.Subject { return tool.Subject{} } + +// Execute exists to satisfy tool.Tool; ExecuteReporting is what actually +// runs, since Spawn is a tool.EventReportingTool (see agent.DispatchToolCalls, +// which type-asserts for that interface and calls ExecuteReporting instead +// of Execute for any tool that implements it). +func (t *Tool) Execute(ctx context.Context, input json.RawMessage) (tool.Result, error) { + return t.ExecuteReporting(ctx, input, noopReporter{}) +} + +type noopReporter struct{} + +func (noopReporter) Report(any) {} + +type spawnInput struct { + Agent string `json:"agent"` + Prompt string `json:"prompt"` +} + +// ExecuteReporting runs the named sub-agent to completion, relaying its +// entire event stream to r as it happens (see agent.ForwardChild) bracketed +// by EventAgentStarted/EventAgentComplete, and returns its final answer, +// bounded to maxResultChars. +// +// Deliberately unpermissioned: agent.DispatchToolCalls skips +// permission.Engine.Check entirely for an EventReportingTool call (see its +// own doc comment on that case). Every action the spawned child actually +// takes -- each of its own tool calls -- still goes through that same Check +// individually, exactly as if the user had typed the child's actions +// themselves; asking about the Spawn call on top of that would be asking +// permission for a decision the user is about to be asked about anyway, +// once per real action rather than once for the wrapper around them. The +// Budget ceiling, not a permission prompt, is what bounds how much a spawn +// can cost. +func (t *Tool) ExecuteReporting(ctx context.Context, input json.RawMessage, r tool.Reporter) (tool.Result, error) { + var in spawnInput + if err := json.Unmarshal(input, &in); err != nil { + return tool.Result{Content: "invalid input: " + err.Error(), IsError: true}, nil + } + if in.Agent == "" || in.Prompt == "" { + return tool.Result{Content: "both \"agent\" and \"prompt\" are required", IsError: true}, nil + } + def, ok := t.Definitions.Get(in.Agent) + if !ok { + return tool.Result{Content: fmt.Sprintf("unknown agent %q", in.Agent), IsError: true}, nil + } + + callerName, callerDepth, _ := agent.CallerIdentityFromContext(ctx) + parentLike := &agent.Agent{Provider: t.Provider, Tools: t.Tools, Budget: t.Budget, Name: callerName, Depth: callerDepth} + child, err := definition.NewChild(def, parentLike, t.Perm) + if err != nil { + return tool.Result{}, err + } + wireProjectContext(child, def) + + childSess := t.newChildSession(child.Model) + + r.Report(agent.Event{ + Type: agent.EventAgentStarted, + AgentName: child.Name, + AgentDepth: child.Depth, + Agent: &agent.AgentInfo{ + Name: def.Name, + PromptSummary: summarize(in.Prompt), + }, + }) + + var childErr error + agent.ForwardChild(child.Run(ctx, in.Prompt), func(ev agent.Event) { + if ev.Type == agent.EventTurnEnded && ev.Result != nil && ev.Result.Outcome != agent.OutcomeCompleted { + childErr = ev.Err + if childErr == nil { + childErr = fmt.Errorf("%s: %s", ev.Result.Outcome, ev.Result.Reason) + } + } + r.Report(ev) + }) + + r.Report(agent.Event{Type: agent.EventAgentComplete, AgentName: child.Name, AgentDepth: child.Depth}) + persistChildMessages(childSess, child) + + if childErr != nil { + return tool.Result{Content: "sub-agent failed: " + childErr.Error(), IsError: true}, nil + } + return tool.Result{Content: bound(resultText(child))}, nil +} + +// newChildSession creates a session nested under t.Session (see +// session.Session.NewChild) for this spawn, or nil if t.Session is unset or +// creating one fails -- session persistence is a record of what happened, +// never a precondition for the spawn itself to proceed. model is the +// child agent's actually-resolved Model (post-construction), not +// necessarily def.Model, which may be "". +func (t *Tool) newChildSession(model string) *session.Session { + if t.Session == nil { + return nil + } + child, err := t.Session.NewChild(t.Provider.Name(), model) + if err != nil { + return nil + } + return child +} + +// persistChildMessages appends every message the child produced to sess, a +// no-op if sess is nil (see newChildSession). Best-effort, matching how +// cmd/coded's own persistNewMessages treats session writes: a failure here +// doesn't unwind an otherwise-successful spawn. +func persistChildMessages(sess *session.Session, child *protocol.Agent) { + if sess == nil { + return + } + for _, m := range child.Messages() { + _ = sess.AppendMessage(m) + } +} + +// wireProjectContext gives child the same project-context (and, if def +// asks for it, synthetic README read) wiring cmd/coded gives the root +// agent -- see project.Block/Readme and Definition.SeedReadme's doc +// comment. Best-effort: os.Getwd failing leaves the child without it +// rather than failing the whole spawn over something this incidental. +func wireProjectContext(child *protocol.Agent, def definition.Definition) { + cwd, err := os.Getwd() + if err != nil { + return + } + child.ProjectContext = func() string { return project.Block(cwd) } + if def.SeedReadme { + child.Readme = func() protocol.ReadmeInfo { + content, found := project.Readme(cwd) + return protocol.ReadmeInfo{Content: content, Found: found} + } + } +} + +// summarize renders prompt as a single line for EventAgentStarted's display +// summary, so a long multi-line prompt doesn't blow out a collapsed +// group's header. +func summarize(prompt string) string { + line := strings.SplitN(strings.TrimSpace(prompt), "\n", 2)[0] + const maxLen = 80 + if len(line) > maxLen { + return line[:maxLen] + "…" + } + return line +} + +// resultText extracts a completed child's final answer: the last assistant +// message's result-kind section (see protocol.Protocol.ResultSection), or +// its trailing unwrapped text if the protocol has no result section +// closed, or the raw text as a last resort. Returns "" if the child never +// produced an assistant message at all (e.g. it errored on its very first +// call). +func resultText(child *protocol.Agent) string { + msgs := child.Messages() + for i := len(msgs) - 1; i >= 0; i-- { + if msgs[i].Role != llm.RoleAssistant { + continue + } + var text strings.Builder + for _, b := range msgs[i].Content { + if b.Type == llm.ContentText { + text.WriteString(b.Text) + } + } + raw := text.String() + sections, trailing := child.Protocol().Parse(raw) + if resultSec, ok := child.Protocol().ResultSection(); ok { + for _, sc := range sections { + if sc.Section.Name == resultSec.Name && sc.Closed { + return sc.Text + } + } + } + if trailing != "" { + return trailing + } + return raw + } + return "" +} + +// bound caps s to maxResultChars, announcing the truncation -- see +// internal/tool's package doc comment on why every tool does this rather +// than trusting the model to ask for less. +func bound(s string) string { + if len(s) <= maxResultChars { + return s + } + return s[:maxResultChars] + fmt.Sprintf("\n\n[truncated: %d more characters]", len(s)-maxResultChars) +} diff --git a/internal/tool/builtin/spawn/spawn_test.go b/internal/tool/builtin/spawn/spawn_test.go new file mode 100644 index 0000000..fd50374 --- /dev/null +++ b/internal/tool/builtin/spawn/spawn_test.go @@ -0,0 +1,512 @@ +package spawn + +import ( + "context" + "encoding/json" + "sync" + "testing" + "time" + + "github.com/mchalapuk/coded/internal/agent" + "github.com/mchalapuk/coded/internal/agent/definition" + "github.com/mchalapuk/coded/internal/agent/protocol" + "github.com/mchalapuk/coded/internal/llm" + "github.com/mchalapuk/coded/internal/memory/session" + "github.com/mchalapuk/coded/internal/permission" + "github.com/mchalapuk/coded/internal/tool" +) + +// fakeProvider replays one scripted turn per Stream call -- a package-local +// copy of the convention every other package in this codebase keeps for the +// same reason (see agent/run_test.go's doc comment on its own copy). +type fakeProvider struct { + turns [][]llm.Event + calls int +} + +func (f *fakeProvider) Name() string { return "fake" } + +func (f *fakeProvider) Stream(ctx context.Context, req llm.Request) (<-chan llm.Event, error) { + if f.calls >= len(f.turns) { + panic("fakeProvider: ran out of scripted turns") + } + turn := f.turns[f.calls] + f.calls++ + ch := make(chan llm.Event, len(turn)) + for _, e := range turn { + ch <- e + } + close(ch) + return ch, nil +} + +// collectingReporter records every reported value in order, for tests that +// assert on the sequence and shape of what ExecuteReporting emits. +// Mutex-protected: a test that inspects events while ExecuteReporting is +// still running concurrently (e.g. polling for a permission request while +// the child is blocked waiting on it) reads and writes this from two +// goroutines at once. +type collectingReporter struct { + mu sync.Mutex + events []agent.Event +} + +func (r *collectingReporter) Report(v any) { + ev, ok := v.(agent.Event) + if !ok { + return + } + r.mu.Lock() + defer r.mu.Unlock() + r.events = append(r.events, ev) +} + +// snapshot returns a copy of events recorded so far, safe to range over +// while Report may still be appending concurrently. +func (r *collectingReporter) snapshot() []agent.Event { + r.mu.Lock() + defer r.mu.Unlock() + return append([]agent.Event(nil), r.events...) +} + +// testExplorerDef mirrors definition.Explorer's shape without depending on +// the builtin registry's exact tool set, so these tests don't have to seed +// a real read-only registry to exercise Spawn's own logic. +func testExplorerDef() definition.Definition { + return definition.Definition{ + Name: "explorer", + Identity: "you are a test explorer", + Protocol: explorerLikeProtocol(), + Loop: definition.LoopEnforced, + } +} + +func explorerLikeProtocol() protocol.Protocol { + return protocol.Protocol{ + Name: "test-explorer-like", + Sections: []protocol.Section{ + {Name: "search", Kind: protocol.KindThinking}, + {Name: "findings", Kind: protocol.KindResult}, + }, + } +} + +func compliantFinalTurn(findings string) []llm.Event { + return []llm.Event{ + {Type: llm.EventTextDelta, Text: "looking" + findings + ""}, + {Type: llm.EventMessageStop, StopReason: llm.StopEndTurn}, + } +} + +// toolCallTurn scripts a non-final turn: a search preamble plus one tool +// call, leaving uncovered -- used to force a second loop +// iteration, e.g. to exercise a budget that only trips starting on that +// second round-trip (see TestSpawnSurfacesChildErrorAsErrorResult). +func toolCallTurn(id, name, input string) []llm.Event { + return []llm.Event{ + {Type: llm.EventTextDelta, Text: "looking"}, + {Type: llm.EventToolUseStart, ToolID: id, ToolName: name}, + {Type: llm.EventToolUseDelta, ToolID: id, ToolDelta: input}, + {Type: llm.EventToolUseEnd, ToolID: id}, + {Type: llm.EventMessageStop, StopReason: llm.StopToolUse}, + } +} + +func newTool(defs *definition.Registry, prov llm.Provider, perm *permission.Engine) *Tool { + return &Tool{ + Provider: prov, + Tools: tool.NewRegistry(), + Perm: perm, + Definitions: defs, + } +} + +// rootCallerCtx stands in for the context DispatchToolCalls always wraps a +// real call with (see agent.WithCallerIdentity) -- ExecuteReporting reads +// the calling agent's identity from ctx rather than from Tool's own fields +// (see Tool's doc comment on why), so a test calling it directly needs to +// supply one itself, as if "main" were the one spawning. +func rootCallerCtx() context.Context { + return agent.WithCallerIdentity(context.Background(), "main", 0) +} + +func TestSpawnUnknownAgentReturnsErrorResult(t *testing.T) { + defs := definition.NewRegistry() + tl := newTool(defs, &fakeProvider{}, permission.New(permission.ModeDefault, nil)) + + input, _ := json.Marshal(spawnInput{Agent: "does-not-exist", Prompt: "find something"}) + res, err := tl.Execute(context.Background(), input) + if err != nil { + t.Fatalf("Execute: %v", err) + } + if !res.IsError { + t.Fatalf("res = %+v, want IsError for an unknown agent", res) + } +} + +func TestSpawnMissingFieldsReturnsErrorResult(t *testing.T) { + defs := definition.NewRegistry() + defs.Register(testExplorerDef()) + tl := newTool(defs, &fakeProvider{}, permission.New(permission.ModeDefault, nil)) + + input, _ := json.Marshal(spawnInput{Agent: "explorer"}) // no prompt + res, err := tl.Execute(context.Background(), input) + if err != nil { + t.Fatalf("Execute: %v", err) + } + if !res.IsError { + t.Fatalf("res = %+v, want IsError when prompt is missing", res) + } +} + +// TestSpawnRunsChildAndReturnsItsFindings covers the whole happy path: the +// child runs to completion, the tool result is its section +// (not its scratch or a raw dump), and the reporter sees a +// correctly bracketed event sequence with the right AgentName/AgentDepth +// stamped on every event. +func TestSpawnRunsChildAndReturnsItsFindings(t *testing.T) { + defs := definition.NewRegistry() + defs.Register(testExplorerDef()) + fp := &fakeProvider{turns: [][]llm.Event{compliantFinalTurn("retry.go:12 has the retry loop")}} + tl := newTool(defs, fp, permission.New(permission.ModeDefault, nil)) + + input, _ := json.Marshal(spawnInput{Agent: "explorer", Prompt: "find the retry logic"}) + rep := &collectingReporter{} + res, err := tl.ExecuteReporting(rootCallerCtx(), input, rep) + if err != nil { + t.Fatalf("ExecuteReporting: %v", err) + } + if res.IsError { + t.Fatalf("res = %+v, want a successful result", res) + } + if res.Content != "retry.go:12 has the retry loop" { + t.Errorf("res.Content = %q, want just the findings section", res.Content) + } + + if len(rep.events) < 2 { + t.Fatalf("got %d events, want at least start+complete", len(rep.events)) + } + first, last := rep.events[0], rep.events[len(rep.events)-1] + if first.Type != agent.EventAgentStarted { + t.Errorf("first event = %v, want EventAgentStarted", first.Type) + } + if first.Agent == nil || first.Agent.Name != "explorer" { + t.Errorf("first event Agent = %+v, want Name \"explorer\"", first.Agent) + } + if last.Type != agent.EventAgentComplete { + t.Errorf("last event = %v, want EventAgentComplete", last.Type) + } + for _, ev := range rep.events { + if ev.AgentName != "main/explorer" || ev.AgentDepth != 1 { + t.Errorf("event %v AgentName/AgentDepth = %q/%d, want main/explorer/1", ev.Type, ev.AgentName, ev.AgentDepth) + } + } +} + +func TestSpawnBoundsLongResult(t *testing.T) { + defs := definition.NewRegistry() + defs.Register(testExplorerDef()) + long := make([]byte, maxResultChars+500) + for i := range long { + long[i] = 'x' + } + fp := &fakeProvider{turns: [][]llm.Event{compliantFinalTurn(string(long))}} + tl := newTool(defs, fp, permission.New(permission.ModeDefault, nil)) + + input, _ := json.Marshal(spawnInput{Agent: "explorer", Prompt: "find something huge"}) + res, err := tl.ExecuteReporting(rootCallerCtx(), input, &collectingReporter{}) + if err != nil { + t.Fatalf("ExecuteReporting: %v", err) + } + if len(res.Content) >= len(long) { + t.Fatalf("res.Content len = %d, want truncated well below the %d-byte input", len(res.Content), len(long)) + } + if !contains(res.Content, "truncated") { + t.Errorf("res.Content = %q, want a truncation notice", res.Content[:80]) + } +} + +// TestSpawnSurfacesChildErrorAsErrorResult covers a child that never +// reaches a final answer -- here, a round-trip budget of 1 lets its first +// (tool-calling) turn through but blocks the second, forced by +// toolCallTurn leaving uncovered -- surfacing as an IsError +// tool.Result rather than silently returning an empty string as if the +// child had succeeded. A ceiling only ever trips starting on the +// round-trip *after* the one that reaches it (see Budget.Exhausted's doc +// comment: checked before spending, so the first round-trip a positive +// ceiling allows always gets to happen), which is why this needs a +// tool-calling first turn rather than a budget of 0 (0 means unlimited, +// not "immediately exhausted" -- see Budget's own doc comment). +func TestSpawnSurfacesChildErrorAsErrorResult(t *testing.T) { + defs := definition.NewRegistry() + def := testExplorerDef() + defs.Register(def) + fp := &fakeProvider{turns: [][]llm.Event{toolCallTurn("t1", "Read", "{}")}} + tl := newTool(defs, fp, permission.New(permission.ModeDefault, nil)) + tl.Budget = agent.NewBudget(1, 0) + + input, _ := json.Marshal(spawnInput{Agent: "explorer", Prompt: "find something"}) + res, err := tl.ExecuteReporting(rootCallerCtx(), input, &collectingReporter{}) + if err != nil { + t.Fatalf("ExecuteReporting: %v", err) + } + if !res.IsError { + t.Fatalf("res = %+v, want IsError once the child's round-trip budget is exhausted", res) + } +} + +func TestSpawnIsUnpermissioned(t *testing.T) { + if _, ok := any(&Tool{}).(tool.EventReportingTool); !ok { + t.Fatalf("*Tool must implement tool.EventReportingTool so agent.DispatchToolCalls skips permission.Check for it") + } +} + +// TestSpawnDoesNotSwitchModeDuringChildRun guards the property that +// replaced per-definition mode tightening (see the singleton-permission-mode +// ADR): the engine's mode is written only in response to explicit user +// input, never as a side effect of a spawn. It mirrors +// TestSpawnForwardsChildPermissionRequestAndUnblocksOnRespond's setup -- a +// child suspended mid-turn on a pending permission request -- and checks the +// mode mid-flight rather than only before and after. +func TestSpawnDoesNotSwitchModeDuringChildRun(t *testing.T) { + defs := definition.NewRegistry() + defs.Register(testExplorerDef()) + registry := tool.NewRegistry() + registry.Register(mutatingTool{}) + + fp := &fakeProvider{turns: [][]llm.Event{ + toolCallTurn("t1", "Mutate", "{}"), + compliantFinalTurn("done after the mutate"), + }} + perm := permission.New(permission.ModeDefault, nil) + tl := &Tool{ + Provider: fp, + Tools: registry, + Perm: perm, + Definitions: defs, + } + + input, _ := json.Marshal(spawnInput{Agent: "explorer", Prompt: "mutate something"}) + rep := &collectingReporter{} + resultCh := make(chan tool.Result, 1) + go func() { + res, _ := tl.ExecuteReporting(rootCallerCtx(), input, rep) + resultCh <- res + }() + + deadline := time.After(2 * time.Second) + var req *agent.PermissionRequest + for req == nil { + select { + case <-deadline: + t.Fatal("timed out waiting for the child's permission request to reach the reporter") + default: + for _, ev := range rep.snapshot() { + if ev.Type == agent.EventPermissionRequested { + req = ev.PermissionRequest + break + } + } + } + } + if perm.Mode() != permission.ModeDefault { + t.Fatalf("perm.Mode() while child is suspended on a permission request = %v, want unchanged ModeDefault", perm.Mode()) + } + req.Respond(agent.PermissionResponse{Decision: permission.Allow}) + + select { + case <-resultCh: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for the child to finish after Respond") + } +} + +func TestSpawnCancellationPropagatesToChild(t *testing.T) { + defs := definition.NewRegistry() + defs.Register(testExplorerDef()) + tl := newTool(defs, &blockingProvider{}, permission.New(permission.ModeDefault, nil)) + + ctx, cancel := context.WithCancel(rootCallerCtx()) + cancel() // already cancelled before the call starts + + input, _ := json.Marshal(spawnInput{Agent: "explorer", Prompt: "find something"}) + done := make(chan struct{}) + go func() { + tl.ExecuteReporting(ctx, input, &collectingReporter{}) + close(done) + }() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("ExecuteReporting did not return promptly after its context was cancelled") + } +} + +// blockingProvider's Stream blocks until ctx is done, then reports that as +// an error -- standing in for a real provider that would otherwise hang +// forever on a cancelled call. +type blockingProvider struct{} + +func (blockingProvider) Name() string { return "blocking" } + +func (blockingProvider) Stream(ctx context.Context, _ llm.Request) (<-chan llm.Event, error) { + <-ctx.Done() + return nil, ctx.Err() +} + +// mutatingTool is a stub RiskMutate tool, registered directly (bypassing +// the real internal/tool/builtin/write package) so +// TestSpawnForwardsChildPermissionRequestAndUnblocksOnRespond doesn't need +// a real filesystem write to exercise the permission.Ask path. +type mutatingTool struct{} + +func (mutatingTool) Name() string { return "Mutate" } +func (mutatingTool) Description() string { return "stub mutate tool" } +func (mutatingTool) InputSchema() json.RawMessage { return json.RawMessage(`{"type":"object"}`) } +func (mutatingTool) Risk() tool.Risk { return tool.RiskMutate } +func (mutatingTool) Subject(json.RawMessage) tool.Subject { return tool.Subject{} } +func (mutatingTool) Execute(context.Context, json.RawMessage) (tool.Result, error) { + return tool.Result{Content: "mutated"}, nil +} + +// TestSpawnForwardsChildPermissionRequestAndUnblocksOnRespond exercises the +// real Ask path -- a mutate call inside the child hits ModeDefault's "ask" +// fallback -- and checks two things at once: the resulting +// EventPermissionRequested is relayed to the top-level reporter with the +// child's AgentName/AgentDepth already stamped on it (not swallowed inside +// Spawn's own synchronous call), and calling its Respond lets the child's own goroutine +// continue and finish normally. This is what "one permission engine for +// the whole tree, prompts serialize globally" (PLAN.md's Sub-agents +// section) actually requires: the same Ask/Respond machinery a root-level +// call uses works unchanged for a spawned child's calls, through the one +// shared *permission.Engine both get constructed with. +func TestSpawnForwardsChildPermissionRequestAndUnblocksOnRespond(t *testing.T) { + defs := definition.NewRegistry() + defs.Register(testExplorerDef()) // Tools: nil -- inherits whatever the parent registry has + registry := tool.NewRegistry() + registry.Register(mutatingTool{}) + + fp := &fakeProvider{turns: [][]llm.Event{ + toolCallTurn("t1", "Mutate", "{}"), + compliantFinalTurn("done after the mutate"), + }} + tl := &Tool{ + Provider: fp, + Tools: registry, + Perm: permission.New(permission.ModeDefault, nil), + Definitions: defs, + } + + input, _ := json.Marshal(spawnInput{Agent: "explorer", Prompt: "mutate something"}) + rep := &collectingReporter{} + + resultCh := make(chan tool.Result, 1) + go func() { + res, err := tl.ExecuteReporting(rootCallerCtx(), input, rep) + if err != nil { + t.Errorf("ExecuteReporting: %v", err) + } + resultCh <- res + }() + + deadline := time.After(2 * time.Second) + var req *agent.PermissionRequest + var reqName string + var reqDepth int + for req == nil { + select { + case <-deadline: + t.Fatal("timed out waiting for the child's permission request to reach the reporter") + default: + for _, ev := range rep.snapshot() { + if ev.Type == agent.EventPermissionRequested { + req = ev.PermissionRequest + reqName, reqDepth = ev.AgentName, ev.AgentDepth + break + } + } + } + } + if reqName != "main/explorer" || reqDepth != 1 { + t.Fatalf("EventPermissionRequested AgentName/AgentDepth = %q/%d, want main/explorer/1", reqName, reqDepth) + } + req.Respond(agent.PermissionResponse{Decision: permission.Allow}) + + select { + case res := <-resultCh: + if res.IsError { + t.Fatalf("res = %+v, want success once the mutate call was allowed", res) + } + if res.Content != "done after the mutate" { + t.Errorf("res.Content = %q, want the child's final findings", res.Content) + } + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for the child to finish after Respond") + } +} + +// TestSpawnPersistsChildSession confirms a spawn creates a session nested +// under the parent's (see session.Session.NewChild), persists the child +// agent's messages into it, and links it into the parent's ChildIDs -- so +// `coded sessions list` can still find it (via the parent) even though it +// never appears as a top-level entry itself. +func TestSpawnPersistsChildSession(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + defs := definition.NewRegistry() + defs.Register(testExplorerDef()) + fp := &fakeProvider{turns: [][]llm.Event{compliantFinalTurn("retry.go:12")}} + + parentSess, err := session.New("/repo", "fake", "test-model") + if err != nil { + t.Fatal(err) + } + tl := newTool(defs, fp, permission.New(permission.ModeDefault, nil)) + tl.Session = parentSess + + input, _ := json.Marshal(spawnInput{Agent: "explorer", Prompt: "find the retry logic"}) + if _, err := tl.ExecuteReporting(rootCallerCtx(), input, &collectingReporter{}); err != nil { + t.Fatalf("ExecuteReporting: %v", err) + } + + reopened, err := session.Open(parentSess.ID()) + if err != nil { + t.Fatal(err) + } + childIDs := reopened.Meta().ChildIDs + if len(childIDs) != 1 { + t.Fatalf("parent.ChildIDs = %v, want exactly one child session", childIDs) + } + + childSess, err := session.Open(childIDs[0]) + if err != nil { + t.Fatal(err) + } + if childSess.Meta().ParentID != parentSess.ID() { + t.Errorf("child.ParentID = %q, want %q", childSess.Meta().ParentID, parentSess.ID()) + } + msgs, err := childSess.LoadMessages() + if err != nil { + t.Fatal(err) + } + if len(msgs) == 0 { + t.Fatalf("child session has no persisted messages") + } + + list, err := session.List() + if err != nil { + t.Fatal(err) + } + if len(list) != 1 || list[0].ID != parentSess.ID() { + t.Fatalf("session.List() = %+v, want only the parent (child excluded)", list) + } +} + +func contains(s, substr string) bool { + for i := 0; i+len(substr) <= len(s); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} diff --git a/internal/tool/registry.go b/internal/tool/registry.go index 58e1c9f..a2ea200 100644 --- a/internal/tool/registry.go +++ b/internal/tool/registry.go @@ -79,6 +79,25 @@ func (r *Registry) Schemas() []llm.ToolSchema { return out } +// Subset returns a new Registry containing only the named tools, in the +// order given (not r's registration order -- the caller's list, typically a +// Definition's Tools field, is itself the deliberate ordering here), with +// Prompt carried over unchanged. Returns an error naming the first unknown +// tool rather than silently omitting it, so a typo'd definition fails loudly +// instead of quietly granting fewer tools than intended. +func (r *Registry) Subset(names []string) (*Registry, error) { + out := NewRegistry() + out.Prompt = r.Prompt + for _, name := range names { + t, ok := r.tools[name] + if !ok { + return nil, fmt.Errorf("tool: unknown tool %q in subset", name) + } + out.Register(t) + } + return out, nil +} + // Execute looks up name in the registry and runs it with input, returning a // Result even on lookup failure (so the agent loop can always feed something // back to the model instead of needing special-case handling for "tool not found"). diff --git a/internal/tool/registry_test.go b/internal/tool/registry_test.go index 03819c7..bb16739 100644 --- a/internal/tool/registry_test.go +++ b/internal/tool/registry_test.go @@ -13,3 +13,47 @@ func TestRegistryExecuteUnknownTool(t *testing.T) { t.Fatalf("expected error for unknown tool") } } + +// stubTool is a minimal Tool implementation for registry tests that don't +// care about execution behavior, only identity and registration. +type stubTool struct{ name string } + +func (s stubTool) Name() string { return s.name } +func (s stubTool) Description() string { return "stub" } +func (s stubTool) InputSchema() json.RawMessage { return json.RawMessage(`{"type":"object"}`) } +func (s stubTool) Risk() Risk { return RiskReadOnly } +func (s stubTool) Subject(json.RawMessage) Subject { return Subject{} } +func (s stubTool) Execute(context.Context, json.RawMessage) (Result, error) { + return Result{}, nil +} + +func TestRegistrySubsetPreservesPromptAndOrder(t *testing.T) { + r := NewRegistry() + r.Prompt = "usage guidance" + r.Register(stubTool{name: "A"}) + r.Register(stubTool{name: "B"}) + r.Register(stubTool{name: "C"}) + + sub, err := r.Subset([]string{"C", "A"}) + if err != nil { + t.Fatalf("Subset: %v", err) + } + if sub.Prompt != "usage guidance" { + t.Errorf("Subset.Prompt = %q, want carried over from the parent registry", sub.Prompt) + } + got := sub.List() + if len(got) != 2 || got[0].Name() != "C" || got[1].Name() != "A" { + t.Fatalf("Subset order = %v, want [C A] (caller's list order, not registration order)", got) + } + if _, ok := sub.Get("B"); ok { + t.Errorf("Subset unexpectedly contains B") + } +} + +func TestRegistrySubsetUnknownNameErrors(t *testing.T) { + r := NewRegistry() + r.Register(stubTool{name: "A"}) + if _, err := r.Subset([]string{"A", "DoesNotExist"}); err == nil { + t.Fatalf("Subset with unknown name: got nil error, want one naming the typo") + } +} diff --git a/internal/tool/reporter.go b/internal/tool/reporter.go new file mode 100644 index 0000000..ffd5e7f --- /dev/null +++ b/internal/tool/reporter.go @@ -0,0 +1,33 @@ +package tool + +import ( + "context" + "encoding/json" +) + +// Reporter lets a tool emit intermediate activity while it executes, +// instead of only returning one Result at the end -- for a tool whose call +// takes long enough, or does enough of its own thing, that a UI watching +// live has something worth seeing before the call returns (see +// EventReportingTool). The reported values are opaque to this package on +// purpose: this package is a dependency of internal/agent (Tool, Registry, +// Subject), so it cannot import agent.Event without an import cycle. A +// caller that constructs a Reporter (see internal/agent's dispatch) knows +// the concrete type it's really passing through. +type Reporter interface { + Report(v any) +} + +// EventReportingTool is implemented by a Tool whose Execute wants a +// Reporter for the duration of one call. The agent package's dispatch +// type-asserts for this the same way it already does for SteppedTool: an +// optional capability a tool opts into, not a change to the base Tool +// interface every existing implementation would otherwise have to grow a +// method for. +type EventReportingTool interface { + Tool + // ExecuteReporting runs the call, calling r.Report for whatever + // intermediate activity is worth surfacing live, and returns the same + // Result an ordinary Tool.Execute would once it's done. + ExecuteReporting(ctx context.Context, input json.RawMessage, r Reporter) (Result, error) +} diff --git a/prompts/explorer-run.txt b/prompts/explorer-run.txt new file mode 100644 index 0000000..972a67f --- /dev/null +++ b/prompts/explorer-run.txt @@ -0,0 +1,195 @@ +coded prompt dump + +================================================================================ +TOOLS (schemas advertised to the model) +================================================================================ + +--- Read --- +Reads a file from the local filesystem. Returns the content with line numbers. Supports an optional offset/limit to read a slice of large files. + +IMPORTANT: Prefer this tool over invoking cat (or head/tail) through the Bash tool to read a file's contents. + +Schema: + { + "properties": { + "file_path": { + "description": "Absolute or cwd-relative path to the file", + "type": "string" + }, + "limit": { + "description": "Maximum number of lines to read", + "type": "integer" + }, + "offset": { + "description": "1-based line number to start reading from", + "type": "integer" + } + }, + "required": [ + "file_path" + ], + "type": "object" + } + +--- Grep --- +Searches file contents for a regular expression (RE2 syntax) under a path, optionally filtered by a glob. Returns matching path:line:text entries. Returns 50 matches by default; pass limit (max 200) for more. + +IMPORTANT: Prefer this tool over invoking grep through the Bash tool. + +Schema: + { + "properties": { + "case_insensitive": { + "description": "Case-insensitive match", + "type": "boolean" + }, + "glob": { + "description": "Optional filename glob filter, e.g. '*.go'", + "type": "string" + }, + "limit": { + "description": "Maximum number of matches to return (default 50, max 200)", + "type": "integer" + }, + "path": { + "description": "Directory to search (default: current directory)", + "type": "string" + }, + "pattern": { + "description": "RE2 regular expression to search for", + "type": "string" + } + }, + "required": [ + "pattern" + ], + "type": "object" + } + +--- Glob --- +Finds files matching a glob pattern (supports ** for recursive matching) under a path, sorted by most recently modified first. Returns 20 matches by default; pass limit (max 100) for more. + +Schema: + { + "properties": { + "limit": { + "description": "Maximum number of matches to return (default 20, max 100)", + "type": "integer" + }, + "path": { + "description": "Directory to search from (default: current directory)", + "type": "string" + }, + "pattern": { + "description": "Glob pattern, e.g. '**/*.go'", + "type": "string" + } + }, + "required": [ + "pattern" + ], + "type": "object" + } + +================================================================================ +SYSTEM PROMPT +================================================================================ +You are an explorer sub-agent: you search a codebase for something specific and report back where it is, not a summary of what you read. You have read-only tools (Read, Grep, Glob) -- no edits, no shell commands. Your entire answer becomes another agent's only view of what you found: it will not re-run your searches or re-read the files you looked at, so an answer written as prose forces it to redo your work to verify anything you claim. Report file:line pointers with a one-line claim about what's there, not a narrative of your investigation. Report explicitly what you searched for and did not find -- that negative result is what stops the caller from repeating a search that already came up empty. + +Every text content block you receive from the user role is wrapped in either or . is exactly what the user typed, verbatim. is text the harness itself injected — never something the user wrote or said. Treat only the content inside as the user's own words. The harness appends a block to the last user message. It contains current workdir, git repository path, and contents of project-local agent instruction files (CODED.md, AGENTS.md, CLAUDE.md). Trust the contents of block, follow the instructions it carries as if the user had given them, and never read a file it already reports on. + +IMPORTANT: Every message you write must open with one tagged block in this order: ... stating what you're looking for, where you've already looked, and what you still expect to check — followed by either the tool call or calls, or your final answer wrapped in .... Be sure to start your message with ... but use those sections only once per your message. End your message after ... section. This format is required for all your responses. Responses not following the format will be rejected. Always include it, exactly once each, in every response. + +You have a set of tools at your disposal. Every tool call runs in the project's working directory (given in the block described below). Always use paths relative to current dir. Never use absolute paths. Prefer small, verifiable changes, and run relevant build/test commands when available before declaring a task done. When several actions are independent — reading or searching multiple files, running unrelated commands — issue them as multiple tool calls in the same response; they run in order and their results come back together, which is faster than one call per turn. Keep a call to its own response only when it depends on an earlier call's result. + +================================================================================ +CONVERSATION +================================================================================ + +-------------------------------------------------------------------------------- +[1] USER +-------------------------------------------------------------------------------- + +--- Content Block [type: text] --- + +Find where retry backoff is implemented. + + +-------------------------------------------------------------------------------- +[2] ASSISTANT +-------------------------------------------------------------------------------- + +--- Content Block [type: text] --- + +I have not yet read README.md. + + +--- Content Block [type: tool_use, id: coded_builtin_read_1, name: Read] --- +{ + "file_path": "README.md" +} + +-------------------------------------------------------------------------------- +[3] USER (tool results) +-------------------------------------------------------------------------------- + +--- Content Block [type: tool_result, id: coded_builtin_read_1, status: ok] --- +# sample + +A throwaway package used to demonstrate validators. + +-------------------------------------------------------------------------------- +[4] ASSISTANT +-------------------------------------------------------------------------------- + +--- Content Block [type: text] --- + +The task is to find where retry backoff is computed. I haven't looked at this workspace yet, so I'll grep for "backoff" across it before concluding anything. + + +--- Content Block [type: tool_use, id: coded_builtin_grep_1, name: Grep] --- +{ + "path": "/workspace", + "pattern": "backoff" +} + +-------------------------------------------------------------------------------- +[5] USER (tool results) +-------------------------------------------------------------------------------- + +--- Content Block [type: tool_result, id: coded_builtin_grep_1, status: ok] --- +(no matches) + +--- Content Block [type: text] --- + + +/workspace +/workspace + + +Prefer table-driven tests. Keep exported functions documented with a doc comment. + + + + + + + +--- Content Block [type: text] --- + + +IMPORTANT: Your response must start with a content block containing ... section, in that order, before either your tool calls or your final answer, wrapped in .... + + + +-------------------------------------------------------------------------------- +[6] ASSISTANT +-------------------------------------------------------------------------------- + +--- Content Block [type: text] --- + +Grep found no matches for "backoff" anywhere in the workspace. + + +No backoff-related code exists in this workspace -- searched for "backoff" (case-sensitive, whole tree) and found nothing. Not found: any retry/backoff implementation. +