From d077b5766c0fa15504fe98bf1b2f284adb493515 Mon Sep 17 00:00:00 2001 From: joyqi Date: Fri, 21 Aug 2026 23:35:37 +0800 Subject: [PATCH 01/11] tool: whether a call may run in parallel can depend on the call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parallel opt-in was answered per tool NAME, which works only while a tool's calls are all alike. A delegation tool breaks that: every call is named the same and differs only in which configured agent it names, and those agents differ in exactly the three properties the opt-in is about — whether the call writes, prompts, or opens a surface. One answer per name would have to lie in one direction, either serializing a read-only fan-out or letting a write-capable call into a batch. SupportsParallel now takes the call's arguments. parallelRun already held the whole ToolCall and was throwing everything but the name away, so the data was at the call site all along; the four read-only file tools ignore the parameter and are otherwise untouched. The contract widens from "this tool's calls never conflict" to "this call does not", and that needs a limit. The arguments must be LOOKED UP, never interpreted: a key into a table the user wrote resolves to a static fact they already authorized, a free-form string does not. That is the line bash stays on the wrong side of even now that the gate exists — a shell command's effects cannot be known from its declaration, and a model-supplied "this one only reads" is the constrained party signing its own certificate. The test dispatcher gains a compile-time interface assertion. ParallelReporter is optional, so a signature drifting out of step here would not fail to compile — it would silently stop being detected, serialize everything, and pass most of the file while proving nothing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- chat/chat.go | 12 ++++++---- chat/parallel.go | 19 +++++++++++---- chat/parallel_test.go | 44 ++++++++++++++++++++++++++++++++-- tool/code.go | 12 ++++++---- tool/parallel_test.go | 55 +++++++++++++++++++++++++++++++++++++++---- tool/tool.go | 46 +++++++++++++++++++++++------------- 6 files changed, 151 insertions(+), 37 deletions(-) diff --git a/chat/chat.go b/chat/chat.go index 77b64d7..bc7a3f6 100644 --- a/chat/chat.go +++ b/chat/chat.go @@ -374,12 +374,14 @@ func headerSummaryOf(dispatch tool.Dispatcher, name string, args map[string]any) return hr.HeaderSummary(name, args) } -// supportsParallel reports whether the named tool's calls may run -// concurrently (the optional tool.ParallelReporter capability; dispatchers -// without it serialize everything, which is the safe answer). -func supportsParallel(dispatch tool.Dispatcher, name string) bool { +// supportsParallel reports whether THIS call may run concurrently with the +// round's others (the optional tool.ParallelReporter capability; dispatchers +// without it serialize everything, which is the safe answer). The arguments +// travel because the answer can differ between two calls to one tool — see +// tool.parallelizer. +func supportsParallel(dispatch tool.Dispatcher, call provider.ToolCall) bool { pr, ok := dispatch.(tool.ParallelReporter) - return ok && pr.SupportsParallel(name) + return ok && pr.SupportsParallel(call.Name, call.Arguments) } // isInteractive reports whether the named tool runs its own user surface: diff --git a/chat/parallel.go b/chat/parallel.go index 4e2203b..0c25b64 100644 --- a/chat/parallel.go +++ b/chat/parallel.go @@ -10,9 +10,12 @@ import ( "chatchain/tool" ) -// Parallel tool execution. A round's calls arrive as a list, and the ones a -// tool declares safe to run concurrently (tool.ParallelReporter — today the -// read-only file tools) execute together instead of one after another. +// Parallel tool execution. A round's calls arrive as a list, and the ones +// declared safe to run concurrently (tool.ParallelReporter — today the +// read-only file tools) execute together instead of one after another. The +// declaration is per CALL: a tool whose calls differ in kind answers per +// call, so the batches follow what is actually safe rather than what a tool +// name can promise. // // Two orderings are kept regardless of who finishes first: // @@ -28,9 +31,17 @@ import ( // parallelRun returns the end of the run of consecutive parallel-capable // calls starting at i (i itself when the call at i is not one). +// +// Capability is asked per CALL, not per tool name, so a round that mixes +// concurrent-safe and serial calls to the SAME tool splits at the boundaries +// without any special handling: [a a b a] batches [a a], runs b alone, then +// [a]. That falls out of scanning for consecutive runs — and it drops the +// serial one into the path that already has approval gates, surfaces and +// expanded rendering, which is exactly where a call needing any of them +// belongs. func parallelRun(dispatch tool.Dispatcher, calls []provider.ToolCall, i int) int { j := i - for j < len(calls) && supportsParallel(dispatch, calls[j].Name) { + for j < len(calls) && supportsParallel(dispatch, calls[j]) { j++ } return j diff --git a/chat/parallel_test.go b/chat/parallel_test.go index 6880ded..2a17e3a 100644 --- a/chat/parallel_test.go +++ b/chat/parallel_test.go @@ -12,18 +12,33 @@ import ( ) // parallelDispatch is a Dispatcher whose named tools are parallel-capable and -// whose calls block until released, so a test can prove they overlap. +// whose calls block until released, so a test can prove they overlap. When +// byAgent is set the answer comes from the call's "agent" argument instead of +// its name — the delegation shape, where one name covers calls that differ. type parallelDispatch struct { parallel map[string]bool + byAgent map[string]bool enter chan struct{} // one token per started call release chan struct{} // closed to let every call finish peak int64 live int64 } +// ParallelReporter is an OPTIONAL interface, so a signature that drifts out of +// step here would not fail to compile — it would silently stop being detected +// and serialize everything, passing most of this file while proving nothing. +// The assertion turns that into a build error. +var _ tool.ParallelReporter = (*parallelDispatch)(nil) + func (d *parallelDispatch) Tools() []provider.ToolDef { return nil } -func (d *parallelDispatch) SupportsParallel(name string) bool { return d.parallel[name] } +func (d *parallelDispatch) SupportsParallel(name string, args map[string]any) bool { + if d.byAgent != nil { + agent, _ := args["agent"].(string) + return d.byAgent[agent] + } + return d.parallel[name] +} func (d *parallelDispatch) CallTool(ctx context.Context, name string, args map[string]any) (string, bool, error) { n := atomic.AddInt64(&d.live, 1) @@ -69,6 +84,31 @@ func TestParallelRunBoundaries(t *testing.T) { } } +// The same boundaries hold when the calls share a NAME and differ only in +// their arguments — the delegation shape. A per-name answer could not split +// this sequence at all: it would either serialize the fan-out or let the +// write-capable call join a batch. +func TestParallelRunSplitsCallsToOneTool(t *testing.T) { + d := ¶llelDispatch{byAgent: map[string]bool{"search": true, "implement": false}} + delegate := func(id, agent string) provider.ToolCall { + return provider.ToolCall{ID: id, Name: "delegate", Arguments: map[string]any{"agent": agent}} + } + calls := []provider.ToolCall{ + delegate("1", "search"), delegate("2", "search"), + delegate("3", "implement"), + delegate("4", "search"), + } + for _, tc := range []struct{ from, want int }{ + {0, 2}, // the two searches batch + {2, 2}, // the write-capable one runs alone + {3, 4}, // and the search after it batches again + } { + if got := parallelRun(d, calls, tc.from); got != tc.want { + t.Errorf("parallelRun(from=%d) = %d, want %d", tc.from, got, tc.want) + } + } +} + // The calls in a batch really do overlap — the point of the change. func TestParallelBatchRunsConcurrently(t *testing.T) { const n = 4 diff --git a/tool/code.go b/tool/code.go index 27e7d82..0a56e1f 100644 --- a/tool/code.go +++ b/tool/code.go @@ -611,10 +611,14 @@ type codeEditFile struct{ cs *codeSet } // so a round that reads four files can read them at once. edit_file and // write_file are deliberately absent — they write — and so is glob's and // grep's mutating sibling set, which does not exist. -func (t *codeReadFile) SupportsParallel() bool { return true } -func (t *codeGlob) SupportsParallel() bool { return true } -func (t *codeGrep) SupportsParallel() bool { return true } -func (t *codeListDir) SupportsParallel() bool { return true } +// +// These four answer the same for every call, so the arguments go unread. The +// parameter exists for tools whose calls differ in kind (see parallelizer); +// reading it here would only invite a per-call exception none of them wants. +func (t *codeReadFile) SupportsParallel(map[string]any) bool { return true } +func (t *codeGlob) SupportsParallel(map[string]any) bool { return true } +func (t *codeGrep) SupportsParallel(map[string]any) bool { return true } +func (t *codeListDir) SupportsParallel(map[string]any) bool { return true } // HeaderSummary: the path IS the call for the file tools — the header reads // "[edit_file internal/ui/model.go]". edit_file and write_file especially diff --git a/tool/parallel_test.go b/tool/parallel_test.go index 6c82b29..f6e9370 100644 --- a/tool/parallel_test.go +++ b/tool/parallel_test.go @@ -1,6 +1,11 @@ package tool -import "testing" +import ( + "context" + "testing" + + "chatchain/provider" +) // A tool may run concurrently only if it needs nothing that concurrency // would break: no writes, no approval prompt, no interactive surface. This @@ -24,7 +29,9 @@ func TestOnlySafeToolsOptIntoParallel(t *testing.T) { "glob": true, "grep": true, "list_dir": true, "read_file": true, } for name, tl := range all { - got := r.SupportsParallel(name) + // These four answer the same for every call, so the arguments are + // irrelevant here — nil is the honest thing to pass. + got := r.SupportsParallel(name, nil) if got != wantParallel[name] { t.Errorf("%s: SupportsParallel = %v, want %v", name, got, wantParallel[name]) } @@ -51,16 +58,54 @@ func TestOnlySafeToolsOptIntoParallel(t *testing.T) { // is the real case: its servers make no promise about concurrent calls. func TestParallelDefaultsToNo(t *testing.T) { var r *Registry - if r.SupportsParallel("read_file") { + if r.SupportsParallel("read_file", nil) { t.Fatal("a nil registry claimed parallel support") } empty := &Registry{index: map[string]Tool{}} - if empty.SupportsParallel("anything") { + if empty.SupportsParallel("anything", nil) { t.Fatal("an unknown tool claimed parallel support") } // A tool that simply does not implement the interface. plain := &Registry{index: map[string]Tool{"x": &codeEditFile{&codeSet{}}}} - if plain.SupportsParallel("x") { + if plain.SupportsParallel("x", nil) { t.Fatal("edit_file claimed parallel support") } } + +// perCallTool answers from a key into a fixed table — the delegation shape, +// where every call shares a name and differs only in which configured entry +// it selects. Nothing here interprets a free-form argument; that distinction +// is the whole reason the arguments are allowed to decide (see parallelizer). +type perCallTool struct{ safe map[string]bool } + +func (perCallTool) Def() provider.ToolDef { return provider.ToolDef{Name: "delegate"} } +func (perCallTool) Call(context.Context, map[string]any) (string, bool, error) { + return "", false, nil +} +func (p perCallTool) SupportsParallel(args map[string]any) bool { + key, _ := args["agent"].(string) + return p.safe[key] +} + +// The capability is a property of the CALL. One answer per tool name would +// have to lie in one direction: serializing a read-only fan-out, or letting a +// write-capable call into a batch. +func TestSupportsParallelIsPerCall(t *testing.T) { + r := &Registry{index: map[string]Tool{ + "delegate": perCallTool{safe: map[string]bool{"search": true, "implement": false}}, + }} + if !r.SupportsParallel("delegate", map[string]any{"agent": "search"}) { + t.Error("the read-only entry must be allowed to run concurrently") + } + if r.SupportsParallel("delegate", map[string]any{"agent": "implement"}) { + t.Error("the write-capable entry must stay serialized") + } + // An entry that is not in the table at all, and a call with no argument: + // unknown resolves to the safe answer, never to the permissive one. + if r.SupportsParallel("delegate", map[string]any{"agent": "nonesuch"}) { + t.Error("an unknown entry must default to serialized") + } + if r.SupportsParallel("delegate", nil) { + t.Error("a call with no entry named must default to serialized") + } +} diff --git a/tool/tool.go b/tool/tool.go index 4d25e77..aebfc12 100644 --- a/tool/tool.go +++ b/tool/tool.go @@ -87,11 +87,11 @@ type PresentationReporter interface { Presentation(name string) Presentation } -// parallelizer is an optional Tool interface: the tool declares that its -// calls may run CONCURRENTLY with other calls in the same round. +// parallelizer is an optional Tool interface: the tool declares that a CALL +// may run concurrently with other calls in the same round. // -// The default is no, and the default is what almost every tool wants. A tool -// may opt in only if all three hold: +// The default is no, and the default is what almost every tool wants. A call +// may be opted in only if all three hold: // // - it does not write — two concurrent writers to one file is a race whose // symptoms are intermittent and whose cause is invisible in a transcript; @@ -99,19 +99,30 @@ type PresentationReporter interface { // once have one screen to share; // - it opens no surface (PresentSurface), for the same reason. // -// Today that is exactly the read-only file tools. Codex arrived at the same -// default (supports_parallel_tool_calls = false) and, notably, does not opt -// its own shell in: a shell command's effects cannot be known from its -// declaration, so the only honest answer for it is "no". +// The question takes the call's ARGUMENTS because for some tools it cannot be +// answered from the name. A delegation tool is the case that forces it: every +// call is named the same and differs only in which configured agent it names, +// and those differ in exactly the three properties above. One answer per name +// would have to lie in one direction — serializing a read-only fan-out, or +// letting a write-capable call into a batch. +// +// The arguments must be LOOKED UP, never interpreted. A key into a table the +// user wrote resolves to a static fact they already authorized; a free-form +// string does not. That is the line bash falls on the wrong side of, and why +// it stays out even though the gate now exists: a shell command's effects +// cannot be known from its declaration, and a model-supplied "this one only +// reads" is the constrained party signing its own certificate. Codex settled +// the same way — supports_parallel_tool_calls defaults false, and it does not +// opt its own shell in either. type parallelizer interface { - SupportsParallel() bool + SupportsParallel(args map[string]any) bool } // ParallelReporter is the Dispatcher-side mirror of parallelizer (as // PresentationReporter mirrors presenter). Parts without the capability // report false, which keeps their calls serialized. type ParallelReporter interface { - SupportsParallel(name string) bool + SupportsParallel(name string, args map[string]any) bool } // headliner is an optional Tool interface: the tool writes the summary that @@ -413,10 +424,11 @@ func (r *Registry) Presentation(name string) Presentation { return PresentGroup } -// SupportsParallel reports whether the named built-in tool's calls may run -// concurrently with others in the same round (the optional parallelizer -// interface; absent means no). -func (r *Registry) SupportsParallel(name string) bool { +// SupportsParallel reports whether THIS call to the named built-in tool may +// run concurrently with others in the same round (the optional parallelizer +// interface; absent means no). args reaches the tool because the answer can +// depend on the call — see parallelizer. +func (r *Registry) SupportsParallel(name string, args map[string]any) bool { if r == nil { return false } @@ -425,7 +437,7 @@ func (r *Registry) SupportsParallel(name string) bool { return false } p, ok := t.(parallelizer) - return ok && p.SupportsParallel() + return ok && p.SupportsParallel(args) } // HeaderSummary reports the named built-in tool's own call summary (via the @@ -549,10 +561,10 @@ func (m *multiDispatcher) Presentation(name string) Presentation { // SupportsParallel routes the question to the part owning the tool name. // Parts without the capability — the MCP manager, whose servers make no such // promise — report false, so their calls stay serialized. -func (m *multiDispatcher) SupportsParallel(name string) bool { +func (m *multiDispatcher) SupportsParallel(name string, args map[string]any) bool { if p := m.owner(name); p != nil { if pr, ok := p.(ParallelReporter); ok { - return pr.SupportsParallel(name) + return pr.SupportsParallel(name, args) } } return false From c06a0807dc05a8f9e36f7ba52f5d2656c37ad61c Mon Sep 17 00:00:00 2001 From: joyqi Date: Fri, 21 Aug 2026 23:36:27 +0800 Subject: [PATCH 02/11] tool: the code set can be granted without its writers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Granting a model the ability to search a codebase meant also granting it edit_file and write_file — the set was all or nothing. That made the two useful shapes unreachable at once: a reviewer that must not touch the tree, and (once delegation exists) a child that can search AND fan out, since the parallel opt-in requires that nothing in the set writes. `read_only: true` withholds the two writers and keeps the four tools that only look. Withholding beats forbidding: a tool the model cannot see is never attempted, never refused, and never argued with. It also needs no new notion of "harmless" — what remains already satisfies the parallel opt-in, so a read-only set answers yes to it for free. read_only together with auto_write is rejected rather than resolved: auto_write approves writes the set does not offer, so saying both is a mistake worth hearing about instead of one silently winning. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- tool/code.go | 25 ++++++++++++++++++++----- tool/code_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/tool/code.go b/tool/code.go index 0a56e1f..6d07e09 100644 --- a/tool/code.go +++ b/tool/code.go @@ -46,6 +46,16 @@ type codeConfig struct { // AutoWrite skips the interactive approval for edit_file/write_file (and // permits them in non-interactive -m runs). AutoWrite bool `yaml:"auto_write"` + // ReadOnly withholds edit_file and write_file entirely, leaving the four + // tools that only look. + // + // Withholding beats forbidding: a tool the model cannot see is never + // attempted, never refused, and never argued with. It also makes the set + // answer yes to the parallel opt-in, since what is left neither writes + // nor prompts — which is what lets a delegated agent search a codebase + // and still fan out. Without it, granting search meant granting writes, + // and granting writes meant giving up concurrency. + ReadOnly bool `yaml:"read_only"` } // newCodeSet builds the "code" toolset. @@ -53,19 +63,24 @@ func newCodeSet(env Env, node yaml.Node) ([]Tool, error) { var cfg codeConfig if !node.IsZero() { if err := node.Decode(&cfg); err != nil { - return nil, fmt.Errorf("config must be a mapping (auto_write): %w", err) + return nil, fmt.Errorf("config must be a mapping (auto_write, read_only): %w", err) } } + if cfg.ReadOnly && cfg.AutoWrite { + return nil, fmt.Errorf("read_only and auto_write contradict each other: auto_write approves writes the set does not offer") + } cwd, _ := os.Getwd() cs := &codeSet{root: env.Root(), cwd: cwd, autoWrite: cfg.AutoWrite, reads: make(map[string]time.Time)} - return []Tool{ + tools := []Tool{ &codeGlob{cs}, &codeGrep{cs}, &codeListDir{cs}, &codeReadFile{cs}, - &codeEditFile{cs}, - &codeWriteFile{cs}, - }, nil + } + if cfg.ReadOnly { + return tools, nil + } + return append(tools, &codeEditFile{cs}, &codeWriteFile{cs}), nil } // codeSet is the state shared by the set's tools for one session: the jail diff --git a/tool/code_test.go b/tool/code_test.go index 700c92a..29d2d39 100644 --- a/tool/code_test.go +++ b/tool/code_test.go @@ -373,3 +373,49 @@ func TestMutationsPostDiffArtifact(t *testing.T) { t.Fatalf("overwrite diff wrong:\n%s", joined) } } + +// read_only withholds the writers rather than refusing them, which is also +// what makes the set answer yes to the parallel opt-in: a delegated agent can +// then search a codebase AND fan out. Before it, granting search granted +// writes, and granting writes cost concurrency. +func TestCodeSetReadOnly(t *testing.T) { + var node yaml.Node + if err := yaml.Unmarshal([]byte("read_only: true\n"), &node); err != nil { + t.Fatal(err) + } + tools, err := newCodeSet(Env{ProjectRoot: t.TempDir()}, *node.Content[0]) + if err != nil { + t.Fatal(err) + } + got := map[string]bool{} + for _, tl := range tools { + name := tl.Def().Name + got[name] = true + p, ok := tl.(parallelizer) + if !ok || !p.SupportsParallel(nil) { + t.Errorf("%s survives read_only but is not parallel-safe", name) + } + } + for _, want := range []string{"glob", "grep", "list_dir", "read_file"} { + if !got[want] { + t.Errorf("read_only dropped %s, which only looks", want) + } + } + for _, gone := range []string{"edit_file", "write_file"} { + if got[gone] { + t.Errorf("read_only kept %s", gone) + } + } +} + +// The two flags describe incompatible intents; saying both is a mistake worth +// hearing about rather than one silently winning. +func TestCodeSetReadOnlyRejectsAutoWrite(t *testing.T) { + var node yaml.Node + if err := yaml.Unmarshal([]byte("read_only: true\nauto_write: true\n"), &node); err != nil { + t.Fatal(err) + } + if _, err := newCodeSet(Env{ProjectRoot: t.TempDir()}, *node.Content[0]); err == nil { + t.Fatal("read_only + auto_write must be rejected") + } +} From dc7f1c15a614bce552660c72e4cc99bb425a220f Mon Sep 17 00:00:00 2001 From: joyqi Date: Fri, 21 Aug 2026 23:37:20 +0800 Subject: [PATCH 03/11] chat,tool,cmd: delegate a task to a child agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A long survey — every call site of a symbol, whether a hypothesis holds across a tree — costs the conversation its own transcript to reach one paragraph of answer. The delegate toolset runs that work as a child agent and brings back the reply ALONE: no tool calls, no reasoning. Claude Code, Codex and pi all settled on that same contract. An agent IS a provider entry: tools: delegate: agents: search: fast-provider review: {provider: careful-provider, description: reads a diff} Nothing about the child is configured twice. Which model, which tools, whether those tools ask for approval, the system prompt, temperature, context window — the referenced entry already says all of it, and saying it again is how the two copies would come to disagree. Only `description` is new, because it is about the ROLE rather than the provider and is the one thing the model has to go on when choosing. The layering follows what each layer already knows. cmd resolves a name to a provider and a toolset, which is what it does for the main session anyway; chat runs the loop; tool decides whether a delegation is allowed and what to call it. The seam between them is tool.Delegator, shaped like Env.Interact — absent, and the set contributes no tools, so the model never sees what it cannot use. Parallelism falls out of the capability above: a delegation may overlap another exactly when its agent grants nothing that changes state, which is the same question as whether every tool in that agent's set may run in parallel. Reusing that keeps one definition of harmless rather than two that could disagree. Two limits are deliberate. The child's toolset never includes delegate — recursive delegation is unbounded in a way no per-run cap describes, and pi's extension disables it for the same reason. And an agent whose provider entry has no `model:` is a startup error: the main session answers a missing model with a picker or by demanding -M, neither of which exists down here, so the alternative is an empty model name reaching the API as a 400 mid-conversation. (It did, once.) executeWithTools gains an approval seam it always needed. It has no user of its own, so it now either forwards the question to one who exists — a child runs inside a parent that owns a terminal — or, with nobody to ask, refuses and says how to enable the call, which is what it did before. Binding a live parent's gate to it is left for its own change; until then a child's writes are refused exactly as a -m run's are. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- chat/chat.go | 40 +++++----- chat/delegate.go | 150 +++++++++++++++++++++++++++++++++++ chat/output.go | 20 ++++- chat/toolloop_test.go | 6 +- cmd/delegate.go | 178 +++++++++++++++++++++++++++++++++++++++++ cmd/delegate_test.go | 128 ++++++++++++++++++++++++++++++ cmd/root.go | 14 ++++ tool/delegate.go | 180 ++++++++++++++++++++++++++++++++++++++++++ tool/tool.go | 67 ++++++++++++++-- 9 files changed, 752 insertions(+), 31 deletions(-) create mode 100644 chat/delegate.go create mode 100644 cmd/delegate.go create mode 100644 cmd/delegate_test.go create mode 100644 tool/delegate.go diff --git a/chat/chat.go b/chat/chat.go index bc7a3f6..1605947 100644 --- a/chat/chat.go +++ b/chat/chat.go @@ -33,7 +33,7 @@ func FetchModels(ctx context.Context, p provider.Provider) ([]string, error) { // still travels either way, so the exit status keeps meaning what it did. func Once(ctx context.Context, p provider.Provider, message string, systemPrompt string, dispatch tool.Dispatcher, agent AgentOptions, maxTurns int, format OutputFormat, w io.Writer) error { rec := newRunRecorder() - reply, images, imageErrs, err := runOnce(ctx, p, message, systemPrompt, dispatch, agent, maxTurns, rec) + reply, images, imageErrs, err := runOnce(ctx, p, message, systemPrompt, dispatch, agent, maxTurns, quietHost{rec: rec}) if format == OutputJSON { if werr := writeReport(w, rec.report(p, reply, images, imageErrs, err)); werr != nil { @@ -59,7 +59,7 @@ func Once(ctx context.Context, p provider.Provider, message string, systemPrompt // runOnce performs the send and reports what came back, writing nothing. Once // owns the formatting; keeping this half output-free is what lets a failed // run still be described. -func runOnce(ctx context.Context, p provider.Provider, message string, systemPrompt string, dispatch tool.Dispatcher, agent AgentOptions, maxTurns int, rec *runRecorder) (reply string, images, imageErrs []string, err error) { +func runOnce(ctx context.Context, p provider.Provider, message string, systemPrompt string, dispatch tool.Dispatcher, agent AgentOptions, maxTurns int, host quietHost) (reply string, images, imageErrs []string, err error) { var messages []provider.Message if systemPrompt != "" { messages = append(messages, provider.Message{Role: "system", Content: systemPrompt}) @@ -94,7 +94,7 @@ func runOnce(ctx context.Context, p provider.Provider, message string, systemPro } if isToolProvider && len(tools) > 0 { - reply, _, err = executeWithTools(ctx, tp, dispatch, &messages, tools, sendOverlay, maxTurns, rec) + reply, _, err = executeWithTools(ctx, tp, dispatch, &messages, tools, sendOverlay, maxTurns, host) if err != nil { return "", nil, nil, err } @@ -107,7 +107,7 @@ func runOnce(ctx context.Context, p provider.Provider, message string, systemPro return "", nil, nil, err } // The tool loop records each of its rounds; this path has exactly one. - rec.observe(p, nil) + host.rec.observe(p, nil) images, imageErrs = saveImagesQuiet(p) return reply, images, imageErrs, nil } @@ -270,7 +270,7 @@ var errToolRoundsExceeded = errors.New("tool loop reached the --max-turns limit // the output format, because the cost of a few ints per round is not worth a // conditional, and because a run that fails mid-loop still owes an account of // the rounds it did pay for. -func executeWithTools(ctx context.Context, tp provider.ToolProvider, dispatch tool.Dispatcher, history *[]provider.Message, tools []provider.ToolDef, overlay string, maxTurns int, rec *runRecorder) (string, string, error) { +func executeWithTools(ctx context.Context, tp provider.ToolProvider, dispatch tool.Dispatcher, history *[]provider.Message, tools []provider.ToolDef, overlay string, maxTurns int, host quietHost) (string, string, error) { for rounds := 0; ; rounds++ { if maxTurns > 0 && rounds == maxTurns { return "", "", fmt.Errorf("%w (%d turns)", errToolRoundsExceeded, maxTurns) @@ -296,7 +296,7 @@ func executeWithTools(ctx context.Context, tp provider.ToolProvider, dispatch to // Read the accounting NOW: LastUsageFull reports the provider's most // recent call, so anything between here and the next round would // silently reassign this round's cost. - rec.observe(tp, toolNames(toolCalls)) + host.rec.observe(tp, toolNames(toolCalls)) if len(toolCalls) == 0 { if content == "" && reasoning != "" { // Reasoning-only response: the reasoning IS the answer. @@ -313,20 +313,22 @@ func executeWithTools(ctx context.Context, tp provider.ToolProvider, dispatch to *history = append(*history, msg) for _, tc := range toolCalls { - // Approval-requiring tools cannot ask anyone here: reject the call - // with a result that tells the model (and the user reading the - // transcript) how to enable it. + // Approval gate. This loop has no user of its own, so it either + // forwards the question to one who exists — a delegated child + // runs inside a parent that owns a terminal — or, with nobody to + // ask, refuses and says how to enable the call. if needsApproval(dispatch, tc.Name) { - *history = append(*history, provider.Message{ - Role: "tool", - Content: fmt.Sprintf("%s was not executed: it requires interactive approval, "+ - "which is unavailable in this non-interactive run. Set the toolset's auto-approve option "+ - "(tools.code.auto_write / tools.shell.auto_run) to permit it here.", tc.Name), - ToolCallID: tc.ID, - ToolCallName: tc.Name, - IsError: true, - }) - continue + allowed, why := host.askApproval(tc) + if !allowed { + *history = append(*history, provider.Message{ + Role: "tool", + Content: why, + ToolCallID: tc.ID, + ToolCallName: tc.Name, + IsError: true, + }) + continue + } } resultText, isError, callErr := dispatch.CallTool(ctx, tc.Name, tc.Arguments) if callErr != nil { diff --git a/chat/delegate.go b/chat/delegate.go new file mode 100644 index 0000000..f8c0991 --- /dev/null +++ b/chat/delegate.go @@ -0,0 +1,150 @@ +package chat + +import ( + "context" + "fmt" + "sort" + "sync" + "time" + + "chatchain/provider" + "chatchain/tool" +) + +// Delegation: running a child agent from inside a parent's tool call. +// +// The child is this same machinery one level down — a provider, a toolset and +// runOnce — which is why there is so little here. What this file owns is the +// three things that only make sense at the boundary: resolving an agent name +// to something runnable, forwarding the child's approval questions to the one +// user who exists, and refusing to let the child delegate in turn. + +// quietHost is what the non-interactive tool loop needs from whoever started +// it: somewhere to record what each round cost, and — when a user exists +// somewhere up the stack — a way to put an approval question to them. +type quietHost struct { + rec *runRecorder + // approve, when set, forwards a state-changing call to whoever owns the + // terminal. A -m run leaves it nil because there is nobody to ask; a + // delegated child sets it because it has no user of its own but runs + // inside a parent that does. + approve func(tc provider.ToolCall) (bool, string) +} + +// askApproval resolves one gated call: allowed, or the refusal to hand back +// as the call's result. +func (h quietHost) askApproval(tc provider.ToolCall) (bool, string) { + if h.approve == nil { + return false, fmt.Sprintf("%s was not executed: it requires interactive approval, "+ + "which is unavailable in this non-interactive run. Set the toolset's auto-approve option "+ + "(tools.code.auto_write / tools.shell.auto_run) to permit it here.", tc.Name) + } + return h.approve(tc) +} + +// Child is everything a delegated run needs, assembled by the host. +// +// The host builds it because config → provider → dispatcher is exactly what +// it already does for the main session. A second implementation here is how +// the two would come to disagree about which knobs a provider entry sets. +type Child struct { + Provider provider.Provider + Dispatch tool.Dispatcher + System string + // AgentMode is the AGENTS.md/skills overlay setting — agent MODE, not + // the delegated agent. The two senses of the word meet in this struct, + // so this one says which it is. + AgentMode AgentOptions + MaxTurns int +} + +// ChildFactory builds a child for a configured agent name. +type ChildFactory func(agent string) (Child, error) + +// Delegator runs child agents for the delegate toolset (tool.Delegator). +type Delegator struct { + agents map[string]tool.AgentInfo + names []string + build ChildFactory + + mu sync.Mutex + approve func(agent string, tc provider.ToolCall) (bool, string) +} + +// NewDelegator prepares the seam. Names are sorted once: the agent list +// reaches the model as a schema enum, and a set that reshuffled between runs +// would defeat prompt caching for no reason. +func NewDelegator(agents map[string]tool.AgentInfo, build ChildFactory) *Delegator { + names := make([]string, 0, len(agents)) + for name := range agents { + names = append(names, name) + } + sort.Strings(names) + return &Delegator{agents: agents, names: names, build: build} +} + +func (d *Delegator) AgentNames() []string { return d.names } + +func (d *Delegator) Agent(name string) (tool.AgentInfo, bool) { + info, ok := d.agents[name] + return info, ok +} + +// SetApprover binds the parent's approval gate, which only exists once there +// is a live UI. Until then a child's state-changing calls are refused, which +// is the same answer any other non-interactive run gives. +func (d *Delegator) SetApprover(fn func(agent string, tc provider.ToolCall) (bool, string)) { + d.mu.Lock() + defer d.mu.Unlock() + d.approve = fn +} + +func (d *Delegator) approver() func(string, provider.ToolCall) (bool, string) { + d.mu.Lock() + defer d.mu.Unlock() + return d.approve +} + +// Run executes one delegation to completion. +func (d *Delegator) Run(ctx context.Context, spec tool.DelegateSpec) (tool.DelegateResult, error) { + if _, ok := d.agents[spec.Agent]; !ok { + return tool.DelegateResult{}, fmt.Errorf("unknown agent %q", spec.Agent) + } + child, err := d.build(spec.Agent) + if err != nil { + return tool.DelegateResult{}, err + } + // The per-task effort override lands on a provider built for this call + // alone, so it cannot leak into the parent's or another child's sampling. + if spec.Effort != "" { + if tun, ok := child.Provider.(provider.Tunable); ok { + tun.SetEffort(spec.Effort) + } + } + + host := quietHost{rec: newRunRecorder()} + if fn := d.approver(); fn != nil { + // Only one child can be waiting on the user at a time — the terminal + // is single-threaded even when the delegations are not. In practice + // concurrent delegations never reach here at all, because a child may + // only run in parallel when its agent grants no state-changing tool + // (tool.delegateTool.SupportsParallel); the lock is what keeps that + // from being load-bearing. + host.approve = func(tc provider.ToolCall) (bool, string) { + d.mu.Lock() + defer d.mu.Unlock() + return fn(spec.Agent, tc) + } + } + + started := time.Now() + reply, _, _, err := runOnce(ctx, child.Provider, spec.Task, child.System, + child.Dispatch, child.AgentMode, child.MaxTurns, host) + res := tool.DelegateResult{ + Reply: reply, + Rounds: len(host.rec.rounds), + Usage: host.rec.usage(), + Duration: time.Since(started), + } + return res, err +} diff --git a/chat/output.go b/chat/output.go index 125efe9..336277a 100644 --- a/chat/output.go +++ b/chat/output.go @@ -16,7 +16,7 @@ import ( // human at a terminal and the wrong one for everything else: what the run // cost, how many round trips it took and why it stopped were recoverable only // by reading prose. Anything driving this binary as a subprocess — CI, a -// pipeline, a parent agent treating it as a sub-agent — needs those as data. +// pipeline, a parent agent running it as a child — needs those as data. // // The shape follows the two CLIs that already settled this. Usage rides on // the round that incurred it (Codex's turn.completed.usage) and the run ends @@ -30,9 +30,9 @@ type OutputFormat string const ( // OutputText prints the reply alone — the historical -m behaviour, and - // still the default. It is also the right choice for a sub-agent: the - // point of delegating is that the answer reaches the caller's context - // without the transcript that produced it. + // still the default. It is also what a delegated child reports: the point + // of delegating is that the answer reaches the caller's context without + // the transcript that produced it. OutputText OutputFormat = "text" // OutputJSON replaces that with a single result object. OutputJSON OutputFormat = "json" @@ -140,6 +140,18 @@ func (r *runRecorder) observe(p any, tools []string) { r.rounds = append(r.rounds, rr) } +// usage is the run's aggregate in provider terms, for callers that report it +// as accounting rather than as JSON (a delegated child's cost). +func (r *runRecorder) usage() provider.Usage { + return provider.Usage{ + Input: r.total.InputTokens, + Output: r.total.OutputTokens, + CacheRead: r.total.CacheReadTokens, + CacheWrite: r.total.CacheWriteTokens, + Total: r.total.TotalTokens, + } +} + // report closes the run. A failed run still reports: the rounds that did // complete were billed, and hiding them would make exactly the runs worth // investigating the ones with no numbers. diff --git a/chat/toolloop_test.go b/chat/toolloop_test.go index f738017..dc12835 100644 --- a/chat/toolloop_test.go +++ b/chat/toolloop_test.go @@ -43,7 +43,7 @@ func TestToolLoopCap(t *testing.T) { tp := &loopingToolProvider{} history := []provider.Message{{Role: "user", Content: "go"}} - _, _, err := executeWithTools(context.Background(), tp, noopDispatcher{}, &history, noopDispatcher{}.Tools(), "", limit, newRunRecorder()) + _, _, err := executeWithTools(context.Background(), tp, noopDispatcher{}, &history, noopDispatcher{}.Tools(), "", limit, quietHost{rec: newRunRecorder()}) if !errors.Is(err, errToolRoundsExceeded) { t.Fatalf("err = %v, want errToolRoundsExceeded", err) } @@ -79,7 +79,7 @@ func TestToolLoopCap(t *testing.T) { func TestToolLoopUnlimitedByDefault(t *testing.T) { tp := &loopingToolProvider{stopAfter: 75} history := []provider.Message{{Role: "user", Content: "go"}} - reply, _, err := executeWithTools(context.Background(), tp, noopDispatcher{}, &history, noopDispatcher{}.Tools(), "", 0, newRunRecorder()) + reply, _, err := executeWithTools(context.Background(), tp, noopDispatcher{}, &history, noopDispatcher{}.Tools(), "", 0, quietHost{rec: newRunRecorder()}) if err != nil { t.Fatalf("unlimited loop errored: %v", err) } @@ -135,7 +135,7 @@ func TestExecuteWithToolsRefreshesPerRound(t *testing.T) { dispatch := &growingDispatcher{} history := []provider.Message{{Role: "user", Content: "go"}} - reply, _, err := executeWithTools(context.Background(), tp, dispatch, &history, dispatch.Tools(), "", 0, newRunRecorder()) + reply, _, err := executeWithTools(context.Background(), tp, dispatch, &history, dispatch.Tools(), "", 0, quietHost{rec: newRunRecorder()}) if err != nil || reply != "done" { t.Fatalf("loop failed: %q %v", reply, err) } diff --git a/cmd/delegate.go b/cmd/delegate.go new file mode 100644 index 0000000..841dc76 --- /dev/null +++ b/cmd/delegate.go @@ -0,0 +1,178 @@ +package cmd + +import ( + "fmt" + "net/http" + "os" + + "chatchain/chat" + "chatchain/config" + "chatchain/provider" + "chatchain/tool" + + "gopkg.in/yaml.v3" +) + +// Wiring for the delegate toolset. Resolving an agent name to something +// runnable lives here because config → provider → toolset is what this +// package already does for the main session; the chat layer runs the child +// and the tool layer decides whether a delegation is allowed, and neither +// needs to learn how a provider entry is spelled. + +// delegateConfig is `tools: delegate:`. An agent IS a provider entry — see +// tool/delegate.go for why nothing about the child is respecified here. +type delegateConfig struct { + Agents map[string]agentRef `yaml:"agents"` + MaxTurns int `yaml:"max_turns"` +} + +// agentRef is a provider name, optionally with the one thing the provider +// entry cannot carry: what this agent is FOR. The bare-string form is the +// common case and stays a bare string. +type agentRef struct { + Provider string `yaml:"provider"` + Description string `yaml:"description"` +} + +func (a *agentRef) UnmarshalYAML(n *yaml.Node) error { + if n.Kind == yaml.ScalarNode { + return n.Decode(&a.Provider) + } + type raw agentRef // shed the custom unmarshaller to avoid recursing + return n.Decode((*raw)(a)) +} + +// delegateDefaultMaxTurns bounds a child that will not stop. The parent's own +// cap is the user pressing ESC; a child has nobody watching it round by +// round, so it gets a number instead. +const delegateDefaultMaxTurns = 30 + +// buildDelegator resolves every configured agent up front — a name that does +// not resolve is a startup error, not a surprise three tool calls into a +// conversation. +func buildDelegator(cfg *config.Config, node yaml.Node, hc httpClientSource, root string, warnf func(string, ...any)) (*chat.Delegator, error) { + var sc delegateConfig + if !node.IsZero() { + if err := node.Decode(&sc); err != nil { + return nil, fmt.Errorf("config must be a mapping (agents, max_turns): %w", err) + } + } + if len(sc.Agents) == 0 { + return nil, fmt.Errorf("no agents configured (add `agents:` mapping agent names to provider names)") + } + maxTurns := sc.MaxTurns + if maxTurns <= 0 { + maxTurns = delegateDefaultMaxTurns + } + + type resolved struct { + ptype string + pc config.ProviderConfig + tools map[string]yaml.Node + } + agents := make(map[string]tool.AgentInfo, len(sc.Agents)) + byName := make(map[string]resolved, len(sc.Agents)) + + for name, ref := range sc.Agents { + if ref.Provider == "" { + return nil, fmt.Errorf("agent %q: no provider named", name) + } + ptype, pc := cfg.Get(ref.Provider) + if err := checkProviderName(cfg, ref.Provider, ptype); err != nil { + return nil, fmt.Errorf("agent %q: %w", name, err) + } + // A child has no way to be asked which model to use: the main session + // answers a missing `model:` with the interactive picker or by + // demanding -M, and neither exists down here. Requiring it at startup + // is the only place the answer can still be useful — otherwise the + // empty name reaches the provider and comes back as a 400 in the + // middle of somebody's conversation. + if pc.Model == "" { + return nil, fmt.Errorf("agent %q: provider %q has no `model:` (a delegated agent cannot be asked to pick one)", name, ref.Provider) + } + tools := childTools(pc.Tools) + // A child's toolset is built once here so its access can be reported + // to the model and, more importantly, so the parallel decision rests + // on what the user configured rather than on what a task claims. + reg := tool.Build(tool.Env{ProjectRoot: root}, tools, func(string, ...any) {}) + agents[name] = tool.AgentInfo{Description: ref.Description, ReadOnly: readOnlyRegistry(reg)} + byName[name] = resolved{ptype: ptype, pc: pc, tools: tools} + } + + build := func(name string) (chat.Child, error) { + r, ok := byName[name] + if !ok { + return chat.Child{}, fmt.Errorf("unknown agent %q", name) + } + key := r.pc.Key + if env := os.Getenv(providerEnvKey(r.ptype)); env != "" { + key = env + } + if key == "" { + return chat.Child{}, fmt.Errorf("agent %q: API key is required (set %s or `key:`)", name, providerEnvKey(r.ptype)) + } + p, err := provider.New(r.ptype, key, r.pc.URL, r.pc.Model, r.pc.Temperature, hc.HTTPClient()) + if err != nil { + return chat.Child{}, fmt.Errorf("agent %q: %w", name, err) + } + if r.pc.Effort != "" { + if tun, ok := p.(provider.Tunable); ok { + tun.SetEffort(r.pc.Effort) + } + } + if r.pc.TopP != nil { + if tun, ok := p.(provider.TopPTunable); ok { + tun.SetTopP(r.pc.TopP) + } + } + sys, err := r.pc.ResolveSystem() + if err != nil { + return chat.Child{}, fmt.Errorf("agent %q: %w", name, err) + } + // The child's own toolset: no ask seam (nobody to question but the + // parent's user, and the child is not the conversation they are in) + // and no Delegate, which is what stops the recursion. + env := tool.Env{ProjectRoot: root} + return chat.Child{ + Provider: p, + Dispatch: tool.Build(env, r.tools, warnf), + System: sys, + AgentMode: chat.AgentOptions{Enabled: r.pc.Agent, Root: root}, + MaxTurns: maxTurns, + }, nil + } + return chat.NewDelegator(agents, build), nil +} + +// httpClientSource is the recording transport, narrowed to the one method a +// child needs — its requests belong in /debug alongside the parent's. +type httpClientSource interface{ HTTPClient() *http.Client } + +// childTools strips the delegate set from a child's toolset. A child that +// could delegate would delegate recursively, and the cost of that is +// unbounded in a way no per-run cap describes. pi's delegate extension +// disables it in the child for the same reason. +func childTools(raw map[string]yaml.Node) map[string]yaml.Node { + out := make(map[string]yaml.Node, len(raw)) + for k, v := range raw { + if k == "delegate" { + continue + } + out[k] = v + } + return out +} + +// readOnlyRegistry reports whether every tool in the set may run +// concurrently, which is the same question as whether the set changes +// anything: the parallel opt-in already means "does not write, needs no +// approval, opens no surface". Reusing it keeps one definition of harmless +// instead of two that could disagree — and an empty set is trivially read-only. +func readOnlyRegistry(reg *tool.Registry) bool { + for _, def := range reg.Tools() { + if !reg.SupportsParallel(def.Name, nil) { + return false + } + } + return true +} diff --git a/cmd/delegate_test.go b/cmd/delegate_test.go new file mode 100644 index 0000000..5c8e465 --- /dev/null +++ b/cmd/delegate_test.go @@ -0,0 +1,128 @@ +package cmd + +import ( + "net/http" + "os" + "path/filepath" + "testing" + + "chatchain/config" + "chatchain/tool" + + "gopkg.in/yaml.v3" +) + +type nopHTTP struct{} + +func (nopHTTP) HTTPClient() *http.Client { return nil } + +func loadConfig(t *testing.T, body string) *config.Config { + t.Helper() + p := filepath.Join(t.TempDir(), "c.yaml") + if err := os.WriteFile(p, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + return config.Load(p) +} + +func agentsNode(t *testing.T, body string) yaml.Node { + t.Helper() + var doc yaml.Node + if err := yaml.Unmarshal([]byte(body), &doc); err != nil { + t.Fatal(err) + } + return *doc.Content[0] // unwrap the document node +} + +// The parallel decision has to survive a whole chain: a provider entry's +// toolset → readOnlyRegistry → AgentInfo.ReadOnly → the answer the tool gives +// for a call naming that agent. Every link was plausible alone, and nothing +// asserted they compose — which is exactly how a fan-out silently serializes. +func TestDelegateParallelFollowsTheAgentsToolset(t *testing.T) { + cfg := loadConfig(t, ` +providers: + worker: {type: openai, key: k, model: m} + scout: {type: openai, key: k, model: m, tools: {code: {read_only: true}}} + codeboy: {type: openai, key: k, model: m, tools: {code: }} +`) + node := agentsNode(t, ` +agents: + fast: {provider: worker, description: has no tools at all} + scout: {provider: scout, description: searches but cannot write} + slow: {provider: codeboy, description: can edit files} +`) + del, err := buildDelegator(cfg, node, nopHTTP{}, t.TempDir(), func(string, ...any) {}) + if err != nil { + t.Fatalf("buildDelegator: %v", err) + } + if info, ok := del.Agent("fast"); !ok || !info.ReadOnly { + t.Error("an agent with no toolset must resolve as read-only") + } + // The case the whole feature was blocked on: an agent that can SEARCH and + // still fan out. Before code's read_only, granting search granted writes, + // so the only parallel-capable agent was one with no tools at all. + if info, ok := del.Agent("scout"); !ok || !info.ReadOnly { + t.Error("an agent with the read-only code set must be read-only") + } + if info, ok := del.Agent("slow"); !ok || info.ReadOnly { + t.Error("an agent holding edit_file/write_file must not be read-only") + } + + reg := tool.Build(tool.Env{ProjectRoot: t.TempDir(), Delegate: del}, + map[string]yaml.Node{"delegate": {}}, func(string, ...any) {}) + if !reg.SupportsParallel("delegate", map[string]any{"agent": "fast"}) { + t.Error("a delegation to a read-only agent must be allowed to run concurrently") + } + if !reg.SupportsParallel("delegate", map[string]any{"agent": "scout"}) { + t.Error("a searching-but-not-writing agent must be allowed to run concurrently") + } + if reg.SupportsParallel("delegate", map[string]any{"agent": "slow"}) { + t.Error("a delegation to a write-capable agent must stay serialized") + } + // Unknown and absent both mean serial: the permissive answer is never the + // one an unresolved name falls back to. + if reg.SupportsParallel("delegate", map[string]any{"agent": "nope"}) || + reg.SupportsParallel("delegate", nil) { + t.Error("an unresolved agent must default to serialized") + } +} + +// A referenced provider without a model cannot be asked to pick one, so it has +// to fail at startup rather than as a 400 mid-conversation. +func TestDelegateRequiresAModel(t *testing.T) { + cfg := loadConfig(t, "providers:\n worker: {type: openai, key: k}\n") + node := agentsNode(t, "agents:\n fast: worker\n") + if _, err := buildDelegator(cfg, node, nopHTTP{}, t.TempDir(), func(string, ...any) {}); err == nil { + t.Fatal("an agent whose provider has no model: must be a startup error") + } +} + +// The bare-string form is the common one and must mean the same as the +// mapping form with only `provider:` set. +func TestAgentRefAcceptsBothForms(t *testing.T) { + var got struct { + Agents map[string]agentRef `yaml:"agents"` + } + if err := yaml.Unmarshal([]byte("agents:\n a: worker\n b: {provider: worker, description: d}\n"), &got); err != nil { + t.Fatal(err) + } + if got.Agents["a"].Provider != "worker" || got.Agents["a"].Description != "" { + t.Errorf("bare string form = %+v", got.Agents["a"]) + } + if got.Agents["b"].Provider != "worker" || got.Agents["b"].Description != "d" { + t.Errorf("mapping form = %+v", got.Agents["b"]) + } +} + +// A child must not be handed the tool that made it: recursive delegation is +// unbounded in a way no per-run cap describes. +func TestChildToolsDropDelegate(t *testing.T) { + raw := map[string]yaml.Node{"code": {}, "delegate": {}, "shell": {}} + got := childTools(raw) + if _, ok := got["delegate"]; ok { + t.Error("the child kept the delegate set") + } + if len(got) != 2 { + t.Errorf("childTools dropped more than delegate: %v", got) + } +} diff --git a/cmd/root.go b/cmd/root.go index 9049d99..fb7e30e 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -229,6 +229,20 @@ var rootCmd = &cobra.Command{ toolEnv.ProjectRoot = agents.ProjectRoot(cwd) } + // The delegate seam, when configured. Every agent resolves here so a + // bad provider name fails at startup rather than three tool calls + // into a conversation; without the seam the set contributes no tools. + if node, ok := pc.Tools["delegate"]; ok && !tool.SetDisabled(pc.Tools, "delegate") { + warnf := func(format string, a ...any) { + chat.ErrorStyle.Fprintf(os.Stderr, "⚠ "+format+"\n", a...) + } + del, derr := buildDelegator(cfg, node, reqLog, toolEnv.ProjectRoot, warnf) + if derr != nil { + return fmt.Errorf("tools.delegate: %w", derr) + } + toolEnv.Delegate = del + } + // --output-format describes a single -m run; the REPL has no such run // to report on. Both an unknown value and a misplaced flag are errors // rather than a quiet fall back to text: a caller that asked for JSON diff --git a/tool/delegate.go b/tool/delegate.go new file mode 100644 index 0000000..d6548a5 --- /dev/null +++ b/tool/delegate.go @@ -0,0 +1,180 @@ +package tool + +import ( + "context" + "fmt" + "strings" + + "chatchain/provider" + + "gopkg.in/yaml.v3" +) + +// The "delegate" set: one tool, delegate, which runs a configured child agent +// on a task and returns its answer. +// +// The child is a full agent — its own context, its own tool loop — and what +// comes back is its final reply ALONE. No tool calls, no reasoning. That is +// the whole point: a search that takes twenty rounds costs the parent one +// paragraph. Claude Code, Codex and pi all landed on the same contract. +// +// An agent is named in config and IS a provider entry: +// +// tools: +// delegate: +// agents: +// search: fast-provider +// review: +// provider: careful-provider +// description: Reads a diff and reports what is wrong with it +// +// Nothing about the child is configured here a second time. Which model, which +// tools, whether those tools ask for approval, the system prompt, temperature, +// context window — the referenced provider entry already says all of it, and +// saying it twice is how the two copies would come to disagree. The only +// field that is not already over there is `description`, which is about the +// ROLE rather than the provider, and is the one thing the model has to go on +// when choosing. + +func newDelegateSet(env Env, _ yaml.Node) ([]Tool, error) { + // Same contract as the ask set: without the host seam the set + // contributes no tools, so the model never sees what it cannot use. + if env.Delegate == nil { + return nil, nil + } + names := env.Delegate.AgentNames() + if len(names) == 0 { + return nil, fmt.Errorf("no agents configured (add `agents:` mapping agent names to provider names)") + } + return []Tool{&delegateTool{d: env.Delegate, names: names}}, nil +} + +type delegateTool struct { + d Delegator + names []string // configured agents, stable order +} + +// SupportsParallel: two delegations may overlap only when neither child can +// change state, and that is a property of the AGENT this call names — not of +// the delegate tool, whose calls are all named the same. +// +// The answer is a lookup into the user's configuration, never an inference +// from the task text. A model that wanted a write-capable child to run +// concurrently could otherwise get one by describing its task as read-only. +// An unconfigured or missing agent resolves to false: unknown means serial. +func (t *delegateTool) SupportsParallel(args map[string]any) bool { + name, _ := args["agent"].(string) + info, ok := t.d.Agent(name) + return ok && info.ReadOnly +} + +// HeaderSummary puts the agent and the head of its brief in the call header — +// "[delegate search: every call site of parallelRun]". The task is the only +// thing that distinguishes two delegations to one agent, and the agent name +// is what says how much the call is about to cost. +func (t *delegateTool) HeaderSummary(args map[string]any) string { + agent, _ := args["agent"].(string) + task, _ := args["task"].(string) + task = headerCommand(strings.TrimSpace(task)) + switch { + case agent == "": + return task + case task == "": + return agent + } + return agent + ": " + task +} + +func (t *delegateTool) Def() provider.ToolDef { + var b strings.Builder + b.WriteString("Delegate a task to a child agent and get back its answer.\n\n" + + "The child starts with NO knowledge of this conversation and reports back only its " + + "final answer, so `task` must be self-contained — state the goal, the context it needs, " + + "and what shape the answer should take. Prefer delegating work whose intermediate steps " + + "you do not need to see (surveying a codebase, checking a hypothesis across many files); " + + "doing it yourself is better when you need to watch it happen.\n\n" + + "Available agents:\n") + for _, name := range t.names { + info, _ := t.d.Agent(name) + desc := info.Description + if desc == "" { + desc = "(no description configured)" + } + access := "read-only" + if !info.ReadOnly { + access = "can modify files" + } + fmt.Fprintf(&b, "- %s (%s): %s\n", name, access, desc) + } + + return provider.ToolDef{ + Name: "delegate", + Description: strings.TrimRight(b.String(), "\n"), + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "agent": map[string]any{ + "type": "string", + "enum": toAny(t.names), + "description": "Which configured agent runs the task.", + }, + "task": map[string]any{ + "type": "string", + "description": "The complete brief. The child sees this and nothing else — " + + "no history, no files you have already read.", + }, + "effort": map[string]any{ + "type": "string", + "enum": []any{"low", "medium", "high", "xhigh", "max"}, + "description": "Optional reasoning effort override for this one task.", + }, + }, + "required": []any{"agent", "task"}, + }, + } +} + +func (t *delegateTool) Call(ctx context.Context, args map[string]any) (string, bool, error) { + agent := strings.TrimSpace(stringArg(args, "agent")) + if agent == "" { + return "missing required argument: agent", true, nil + } + if _, ok := t.d.Agent(agent); !ok { + return fmt.Sprintf("unknown agent %q — configured agents: %s", + agent, strings.Join(t.names, ", ")), true, nil + } + task := strings.TrimSpace(stringArg(args, "task")) + if task == "" { + return "missing required argument: task", true, nil + } + effort := strings.TrimSpace(stringArg(args, "effort")) + if effort != "" && !provider.ValidEffort(effort) { + return fmt.Sprintf("invalid effort %q: want low|medium|high|xhigh|max", effort), true, nil + } + + res, err := t.d.Run(ctx, DelegateSpec{Agent: agent, Task: task, Effort: effort}) + if err != nil { + // The child's failure is the parent's result, not the parent's crash: + // a returned error would abort the whole round, while a tool error + // lets the model try something else. + return fmt.Sprintf("delegation to %q failed: %v", agent, err), true, nil + } + if strings.TrimSpace(res.Reply) == "" { + return fmt.Sprintf("agent %q finished without an answer after %d round(s)", agent, res.Rounds), true, nil + } + return res.Reply, false, nil +} + +// stringArg reads a string argument, tolerating absence. +func stringArg(args map[string]any, key string) string { + s, _ := args[key].(string) + return s +} + +func toAny(ss []string) []any { + out := make([]any, 0, len(ss)) + for _, s := range ss { + out = append(out, s) + } + return out +} diff --git a/tool/tool.go b/tool/tool.go index aebfc12..a27d57e 100644 --- a/tool/tool.go +++ b/tool/tool.go @@ -6,7 +6,8 @@ // config maps a set name to the set's shared raw config; the set factory // decodes that one config instance and hands it to every tool it constructs // (an empty value means defaults). Current sets: "shell" (bash), "code" -// (file tools), and "agent" (load_skill). The Registry aggregates the enabled +// (file tools), "agent" (load_skill), "ask" (choose/confirm) and "delegate" +// (run a child agent). The Registry aggregates the enabled // tools behind the Dispatcher surface, and Merge combines several dispatchers // (e.g. built-ins + an MCP manager) into one. package tool @@ -19,6 +20,7 @@ import ( "sort" "strconv" "sync" + "time" "chatchain/provider" @@ -205,6 +207,10 @@ type Env struct { // (the ask set). nil in non-interactive runs — a set that needs it // returns no tools, so the model never sees what it cannot use. Interact Interactor + // Delegate runs a child agent (the delegate set). nil where delegation + // is not configured — same contract as Interact: the set returns no + // tools, so the model never sees what it cannot use. + Delegate Delegator } // Root is the toolsets' anchor directory: the configured project root, else @@ -231,6 +237,56 @@ type Interactor interface { Ask(ctx context.Context, spec AskSpec) (AskResult, error) } +// Delegator is the host-side seam for running a child agent, as Interactor is +// for asking the user something. A tool decides whether a delegation is +// allowed and what to call it; building a provider, assembling a toolset and +// driving a round loop is the chat layer's job, and none of it belongs in a +// tool. +type Delegator interface { + // AgentNames lists the configured agents in a stable order. The set is + // resolved by the host rather than decoded from this set's own config + // (the usual convention) because the config's values are provider names, + // and only the host can say what a provider name resolves to. + AgentNames() []string + // Agent resolves a configured agent by name. ok=false means the name is + // not configured, which the tool reports as an error rather than + // silently substituting a default. + Agent(name string) (AgentInfo, bool) + // Run executes the child to completion and returns its final answer. + Run(ctx context.Context, spec DelegateSpec) (DelegateResult, error) +} + +// AgentInfo is what the tool needs to know about a configured agent without +// being able to read provider configs itself. +type AgentInfo struct { + // Description tells the model what this agent is for; it is the only + // basis on which the model can choose between them. + Description string + // ReadOnly reports that the agent's toolset grants nothing that changes + // state. It is the whole basis for letting a delegation run in parallel, + // and it is derived from the user's configuration — never from anything + // the model says about the task. + ReadOnly bool +} + +// DelegateSpec is one delegation request. +type DelegateSpec struct { + Agent string // the configured agent name + Task string // the entire brief; the child receives nothing else + Effort string // "" leaves the agent's configured default in place +} + +// DelegateResult is what a finished child hands back. Reply is the only part +// that reaches the model — no tool calls, no reasoning, which is the point of +// delegating. The rest is accounting for the transcript, and travels as an +// Artifact so that showing what a child cost does not itself cost tokens. +type DelegateResult struct { + Reply string + Rounds int + Usage provider.Usage + Duration time.Duration +} + // AskSpec is one interaction: 1–4 questions answered on one surface (Tab // switches, Enter commits all, ESC declines the whole ask). type AskSpec struct { @@ -277,10 +333,11 @@ type SetFactory func(env Env, node yaml.Node) ([]Tool, error) // its factory and one line here; growing a set = its factory returns one more // Tool. Future candidate: "web" (browse/search). var sets = map[string]SetFactory{ - "shell": newShellSet, - "agent": newAgentSet, - "code": newCodeSet, - "ask": newAskSet, + "shell": newShellSet, + "agent": newAgentSet, + "code": newCodeSet, + "ask": newAskSet, + "delegate": newDelegateSet, } // SetDisabled reports an explicit boolean-false config value for a set — From b2d6a8d02503a65880a3e4fa587e188742fae225 Mon Sep 17 00:00:00 2001 From: joyqi Date: Fri, 21 Aug 2026 23:50:05 +0800 Subject: [PATCH 04/11] chat: a child's approval question reaches the one user there is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The delegate seam was built with the gate unbound, so a child's writes met the same refusal a -m run gets — the machinery existed and nothing was plugged into it. A child has no terminal of its own, and giving it one would mean two prompts with two memories in front of one person. The prompt moves out of the tool loop into an approvalGate the loop and the delegator share. A child's question arrives at the same place, labelled with the agent that asked: writer › write_file wants to modify files — allow? Without that label a delegated request is indistinguishable from one the conversation made itself, which is the difference between approving what you asked for and approving what something else did. "Allow for this session" is shared rather than tracked per agent. The grant a user gives is "this session may edit files", and a child running inside the session is part of it; the evidence is symmetric too, since the prompt shows the tool and the file and never the diff, which settles only after the call. Asking twice for one decision is the friction, not the safeguard. The approval callbacks take a context so the prompt can be cancelled with the turn. Concurrency needs no more than the delegator's existing lock: a delegation may only run in parallel when its agent grants no state-changing tool, so two children can never be at this prompt at once. Verified end to end in a real terminal — the child asked, the prompt appeared labelled, the denial went back as the call's result, and the child carried on and reported rather than the turn aborting. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- chat/approval.go | 67 +++++++++++++++++++++++ chat/approval_test.go | 123 ++++++++++++++++++++++++++++++++++++++++++ chat/chat.go | 2 +- chat/delegate.go | 16 +++--- chat/run.go | 46 ++++++++-------- cmd/root.go | 4 +- 6 files changed, 227 insertions(+), 31 deletions(-) create mode 100644 chat/approval.go create mode 100644 chat/approval_test.go diff --git a/chat/approval.go b/chat/approval.go new file mode 100644 index 0000000..0b156df --- /dev/null +++ b/chat/approval.go @@ -0,0 +1,67 @@ +package chat + +import ( + "context" + "fmt" + + "chatchain/internal/host" + "chatchain/internal/ui" +) + +// approvalGate is the one place a state-changing call is put to the user. +// +// Both the conversation's own tool loop and a delegated child arrive here. A +// child has no terminal of its own, and giving it a second gate would mean +// two prompts with two memories in front of one person — so its questions +// travel up to this one instead, labelled with the agent that asked. +// +// The "allow for this session" memory is shared for the same reason: the +// grant a user gives is "this session may edit files", and a child running +// inside the session is part of it. What the prompt shows is symmetric too — +// the tool and the file, never the diff, which settles only after the call — +// so a parent's request and a child's are answered on the same evidence. +type approvalGate struct { + u *ui.UI + tr *transcript + pres *host.Presenter + approved map[string]bool // "allow for this session", keyed by tool name +} + +// ask resolves one gated call. subject names where the request came from when +// it was not this conversation ("search" for a delegation); empty for the +// conversation's own calls, whose origin needs no saying. +// +// The error is reserved for the prompt itself failing. A refusal is (false, +// nil): the caller turns it into a result the model can read, and a turn that +// continues after a denial is the point of asking. +func (g *approvalGate) ask(ctx context.Context, name, subject string) (bool, error) { + if g.approved[name] { + return true, nil + } + label := displayToolName(name) + if subject != "" { + label = subject + " › " + label + } + // The turn is now blocked on the user: needs-input state on every host, + // and a ping if they wandered off. + g.pres.SetState(host.StateNeedsInput) + g.pres.Notify(host.Event{Kind: host.KindNeedsInput, + Text: fmt.Sprintf("%s wants to modify files", label)}) + g.tr.pauseForInput("waiting for approval") + choice, err := g.u.Select(ctx, ui.SelectSpec{ + Title: fmt.Sprintf("%s wants to modify files — allow?", label), + Items: []string{"Allow once", "Allow for this session", "Deny"}, + }) + g.tr.resumeFromInput() + g.pres.SetState(host.StateBusy) // resolved either way; end states override + if err != nil { + return false, err + } + if choice.Cancelled || choice.Index == 2 { + return false, nil + } + if choice.Index == 1 { + g.approved[name] = true + } + return true, nil +} diff --git a/chat/approval_test.go b/chat/approval_test.go new file mode 100644 index 0000000..6dfe8a6 --- /dev/null +++ b/chat/approval_test.go @@ -0,0 +1,123 @@ +package chat + +import ( + "context" + "errors" + "fmt" + "io" + "strings" + "testing" + + "chatchain/provider" +) + +// gatedDispatch owns one tool that always needs approval, and records whether +// it was ever actually executed. +type gatedDispatch struct{ ran int } + +func (d *gatedDispatch) Tools() []provider.ToolDef { + return []provider.ToolDef{{Name: "write_file"}} +} +func (d *gatedDispatch) RequiresApproval(string) bool { return true } +func (d *gatedDispatch) CallTool(context.Context, string, map[string]any) (string, bool, error) { + d.ran++ + return "written", false, nil +} + +// writingProvider asks for the gated tool once, then answers. +type writingProvider struct{ calls int } + +func (p *writingProvider) StreamChatWithTools(ctx context.Context, msgs []provider.Message, tools []provider.ToolDef, w io.Writer, reasoning io.WriteCloser) (string, string, []provider.ToolCall, error) { + reasoning.Close() + p.calls++ + if p.calls == 1 { + return "", "", []provider.ToolCall{{ID: "c1", Name: "write_file", Arguments: map[string]any{}}}, nil + } + // The result of the gated call is the last thing in history; echo it so + // the test can assert on what the model was actually told. + last := msgs[len(msgs)-1] + return "saw: " + last.Content, "", nil, nil +} + +func runGated(t *testing.T, host quietHost) (string, *gatedDispatch, error) { + t.Helper() + d := &gatedDispatch{} + history := []provider.Message{{Role: "user", Content: "go"}} + reply, _, err := executeWithTools(context.Background(), &writingProvider{}, d, + &history, d.Tools(), "", 0, host) + return reply, d, err +} + +// With nobody to ask, the loop refuses and says how to enable the call. That +// is the -m contract and it must survive the seam being added. +func TestQuietLoopRefusesWhenThereIsNobodyToAsk(t *testing.T) { + reply, d, err := runGated(t, quietHost{rec: newRunRecorder()}) + if err != nil { + t.Fatal(err) + } + if d.ran != 0 { + t.Error("a gated tool ran with no approval") + } + if !strings.Contains(reply, "auto_write") { + t.Errorf("the refusal must name the way to enable it, got %q", reply) + } +} + +// A delegated child has no user of its own but runs inside a parent that does, +// so its question travels up and the answer decides the call. +func TestQuietLoopForwardsApproval(t *testing.T) { + var asked []string + host := quietHost{rec: newRunRecorder(), + approve: func(_ context.Context, tc provider.ToolCall) (bool, string) { + asked = append(asked, tc.Name) + return true, "" + }} + reply, d, err := runGated(t, host) + if err != nil { + t.Fatal(err) + } + if len(asked) != 1 || asked[0] != "write_file" { + t.Errorf("gate saw %v, want one write_file", asked) + } + if d.ran != 1 { + t.Errorf("tool ran %d times, want 1", d.ran) + } + if !strings.Contains(reply, "written") { + t.Errorf("the model should have seen the result, got %q", reply) + } +} + +// A denial is a result the model reads, not an aborted turn: the child carries +// on and reports back, which is what the parent's terminal showed. +func TestQuietLoopDenialContinuesTheRun(t *testing.T) { + host := quietHost{rec: newRunRecorder(), + approve: func(_ context.Context, tc provider.ToolCall) (bool, string) { + return false, "The user declined this call." + }} + reply, d, err := runGated(t, host) + if err != nil { + t.Fatal(err) + } + if d.ran != 0 { + t.Error("a denied tool ran anyway") + } + if !strings.Contains(reply, "declined") { + t.Errorf("the model must be told it was declined, got %q", reply) + } +} + +// The refusal text is the call's result either way, so a failing prompt must +// not be mistaken for consent. +func TestQuietHostAskApprovalShapes(t *testing.T) { + var h quietHost + ok, why := h.askApproval(context.Background(), provider.ToolCall{Name: "edit_file"}) + if ok || !strings.Contains(why, "edit_file") { + t.Errorf("nil approver = (%v, %q)", ok, why) + } + h.approve = func(_ context.Context, tc provider.ToolCall) (bool, string) { + return false, fmt.Sprintf("%v", errors.New("prompt broke")) + } + if ok, why := h.askApproval(context.Background(), provider.ToolCall{Name: "edit_file"}); ok || why != "prompt broke" { + t.Errorf("failed prompt = (%v, %q), want a refusal carrying the reason", ok, why) + } +} diff --git a/chat/chat.go b/chat/chat.go index 1605947..719a1a8 100644 --- a/chat/chat.go +++ b/chat/chat.go @@ -318,7 +318,7 @@ func executeWithTools(ctx context.Context, tp provider.ToolProvider, dispatch to // runs inside a parent that owns a terminal — or, with nobody to // ask, refuses and says how to enable the call. if needsApproval(dispatch, tc.Name) { - allowed, why := host.askApproval(tc) + allowed, why := host.askApproval(ctx, tc) if !allowed { *history = append(*history, provider.Message{ Role: "tool", diff --git a/chat/delegate.go b/chat/delegate.go index f8c0991..b2d61ea 100644 --- a/chat/delegate.go +++ b/chat/delegate.go @@ -28,18 +28,18 @@ type quietHost struct { // terminal. A -m run leaves it nil because there is nobody to ask; a // delegated child sets it because it has no user of its own but runs // inside a parent that does. - approve func(tc provider.ToolCall) (bool, string) + approve func(ctx context.Context, tc provider.ToolCall) (bool, string) } // askApproval resolves one gated call: allowed, or the refusal to hand back // as the call's result. -func (h quietHost) askApproval(tc provider.ToolCall) (bool, string) { +func (h quietHost) askApproval(ctx context.Context, tc provider.ToolCall) (bool, string) { if h.approve == nil { return false, fmt.Sprintf("%s was not executed: it requires interactive approval, "+ "which is unavailable in this non-interactive run. Set the toolset's auto-approve option "+ "(tools.code.auto_write / tools.shell.auto_run) to permit it here.", tc.Name) } - return h.approve(tc) + return h.approve(ctx, tc) } // Child is everything a delegated run needs, assembled by the host. @@ -68,7 +68,7 @@ type Delegator struct { build ChildFactory mu sync.Mutex - approve func(agent string, tc provider.ToolCall) (bool, string) + approve func(ctx context.Context, agent string, tc provider.ToolCall) (bool, string) } // NewDelegator prepares the seam. Names are sorted once: the agent list @@ -93,13 +93,13 @@ func (d *Delegator) Agent(name string) (tool.AgentInfo, bool) { // SetApprover binds the parent's approval gate, which only exists once there // is a live UI. Until then a child's state-changing calls are refused, which // is the same answer any other non-interactive run gives. -func (d *Delegator) SetApprover(fn func(agent string, tc provider.ToolCall) (bool, string)) { +func (d *Delegator) SetApprover(fn func(ctx context.Context, agent string, tc provider.ToolCall) (bool, string)) { d.mu.Lock() defer d.mu.Unlock() d.approve = fn } -func (d *Delegator) approver() func(string, provider.ToolCall) (bool, string) { +func (d *Delegator) approver() func(context.Context, string, provider.ToolCall) (bool, string) { d.mu.Lock() defer d.mu.Unlock() return d.approve @@ -130,10 +130,10 @@ func (d *Delegator) Run(ctx context.Context, spec tool.DelegateSpec) (tool.Deleg // only run in parallel when its agent grants no state-changing tool // (tool.delegateTool.SupportsParallel); the lock is what keeps that // from being load-bearing. - host.approve = func(tc provider.ToolCall) (bool, string) { + host.approve = func(ctx context.Context, tc provider.ToolCall) (bool, string) { d.mu.Lock() defer d.mu.Unlock() - return fn(spec.Agent, tc) + return fn(ctx, spec.Agent, tc) } } diff --git a/chat/run.go b/chat/run.go index daf7889..e1f3370 100644 --- a/chat/run.go +++ b/chat/run.go @@ -32,7 +32,7 @@ type SessionFactory func() (*SessionWriter, error) // // Invariant: after ui.New() nothing may write to the terminal except through // the facade — no spinner, no raw OSC/ANSI escapes, no direct stdout. -func Run(p, titleP provider.Provider, systemPrompt string, systemInteractive bool, importedHistory []provider.Message, dispatch tool.Dispatcher, mgr *mcpmgr.Manager, sw *SessionWriter, newSession SessionFactory, interact *Interactor, contextWindow int, agent AgentOptions, notify bool, reqLog *RequestLog) error { +func Run(p, titleP provider.Provider, systemPrompt string, systemInteractive bool, importedHistory []provider.Message, dispatch tool.Dispatcher, mgr *mcpmgr.Manager, sw *SessionWriter, newSession SessionFactory, interact *Interactor, delegator *Delegator, contextWindow int, agent AgentOptions, notify bool, reqLog *RequestLog) error { // ---- pre-Program phase: plain stdout, the Program hasn't claimed the // terminal yet. The OSC background query MUST happen here (during the // Program it would race the event loop's stdin ownership). @@ -59,7 +59,9 @@ func Run(p, titleP provider.Provider, systemPrompt string, systemInteractive boo } // Tools the user approved with "allow for this session" — consulted by the - // approval gate before every call of an approval-requiring tool. + // approval gate before every call of an approval-requiring tool. Shared + // with delegated children: the grant is "this session may edit files", + // and the child is part of this session. approved := make(map[string]bool) var history []provider.Message @@ -166,6 +168,22 @@ func Run(p, titleP provider.Provider, systemPrompt string, systemInteractive boo // block (input, thinking, content, tool calls, notices, echoes) declares // itself and the transcript alone spaces them (transcript.go). tr := newTranscript(u, budget.counter) + gate := &approvalGate{u: u, tr: tr, pres: pres, approved: approved} + if delegator != nil { + // A child has no terminal of its own. Rather than inventing a second + // gate for it — two prompts with two memories for one person — its + // questions arrive at this one, labelled with the agent that asked. + delegator.SetApprover(func(ctx context.Context, agent string, tc provider.ToolCall) (bool, string) { + ok, err := gate.ask(ctx, tc.Name, agent) + switch { + case err != nil: + return false, fmt.Sprintf("%s was not executed: %v", tc.Name, err) + case !ok: + return false, "The user declined this call." + } + return true, "" + }) + } if reqLog != nil { // /debug on doubles as the no-collapse switch: with recording on, the // activity group settles after every event (classic per-call blocks). @@ -1008,7 +1026,7 @@ func Run(p, titleP provider.Provider, systemPrompt string, systemInteractive boo } var err error if isToolProvider && len(tools) > 0 { - reply, thinking, err = toolLoop(turnCtx, u, sink, tr, tp, dispatch, &history, tools, sendOverlay, approved, sw.ImagesDir, ctxm, pres, steer) + reply, thinking, err = toolLoop(turnCtx, u, sink, tr, tp, dispatch, &history, tools, sendOverlay, gate, sw.ImagesDir, ctxm, pres, steer) } else { reply, thinking, err = streamTurn(turnCtx, u, sink, tr, p, ctxm, func(w io.Writer, r io.WriteCloser) (string, string, error) { return p.StreamChat(turnCtx, agents.ComposeSendHistory(history, sendOverlay), w, r) @@ -1362,7 +1380,7 @@ func streamTurn(ctx context.Context, u *ui.UI, sink ui.StreamSink, tr *transcrip // steer (nil-safe) drains mid-turn type-ahead at each round boundary — the // only place a user message can legally enter the conversation (a round's // tool results must directly follow its calls). -func toolLoop(ctx context.Context, u *ui.UI, sink ui.StreamSink, tr *transcript, tp provider.ToolProvider, dispatch tool.Dispatcher, history *[]provider.Message, tools []provider.ToolDef, overlay string, approved map[string]bool, imgDir func() string, ctxm *ctxMeter, pres *host.Presenter, steer func() []provider.Message) (string, string, error) { +func toolLoop(ctx context.Context, u *ui.UI, sink ui.StreamSink, tr *transcript, tp provider.ToolProvider, dispatch tool.Dispatcher, history *[]provider.Message, tools []provider.ToolDef, overlay string, gate *approvalGate, imgDir func() string, ctxm *ctxMeter, pres *host.Presenter, steer func() []provider.Message) (string, string, error) { // No round cap: the user is the brake (ESC cancels the turn; approval // gates cover mutating tools) — industry parity with the major CLIs. interactive := func(name string) bool { return isInteractive(dispatch, name) } @@ -1472,23 +1490,12 @@ func toolLoop(ctx context.Context, u *ui.UI, sink ui.StreamSink, tr *transcript, // only with the user's consent — once, or for the whole session. // The widget header above shows what is being approved; the group // clock pauses while the user deliberates. - if needsApproval(dispatch, tc.Name) && !approved[tc.Name] { - // The turn is now blocked on the user: needs-input state on - // every host, and a ping if they wandered off. - pres.SetState(host.StateNeedsInput) - pres.Notify(host.Event{Kind: host.KindNeedsInput, - Text: fmt.Sprintf("%s wants to modify files", displayToolName(tc.Name))}) - tr.pauseForInput("waiting for approval") - choice, aerr := u.Select(ctx, ui.SelectSpec{ - Title: fmt.Sprintf("%s wants to modify files — allow?", displayToolName(tc.Name)), - Items: []string{"Allow once", "Allow for this session", "Deny"}, - }) - tr.resumeFromInput() - pres.SetState(host.StateBusy) // resolved either way; end states override + if needsApproval(dispatch, tc.Name) { + allowed, aerr := gate.ask(ctx, tc.Name, "") if aerr != nil { return "", "", aerr } - if choice.Cancelled || choice.Index == 2 { + if !allowed { const declined = "The user declined this call." if expanded { tr.settleShowcase(header, nil, declined, true) @@ -1504,9 +1511,6 @@ func toolLoop(ctx context.Context, u *ui.UI, sink ui.StreamSink, tr *transcript, }) continue } - if choice.Index == 1 { - approved[tc.Name] = true - } } toolCtx, cancel := context.WithCancel(ctx) diff --git a/cmd/root.go b/cmd/root.go index fb7e30e..dd9810e 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -221,6 +221,7 @@ var rootCmd = &cobra.Command{ // binds the live UI); -m runs leave it nil, so the ask set // contributes no tools and the model never sees them. var interact *chat.Interactor + var delegator *chat.Delegator if chatMessage == "" { interact = chat.NewInteractor() toolEnv.Interact = interact @@ -241,6 +242,7 @@ var rootCmd = &cobra.Command{ return fmt.Errorf("tools.delegate: %w", derr) } toolEnv.Delegate = del + delegator = del } // --output-format describes a single -m run; the REPL has no such run @@ -412,7 +414,7 @@ var rootCmd = &cobra.Command{ return err } } - return chat.Run(p, titleP, systemPrompt, systemInteractive, importedHistory, dispatch, mgr, sw, newSession, interact, contextWindow, agentOpts, pc.Notify == nil || *pc.Notify, reqLog) + return chat.Run(p, titleP, systemPrompt, systemInteractive, importedHistory, dispatch, mgr, sw, newSession, interact, delegator, contextWindow, agentOpts, pc.Notify == nil || *pc.Notify, reqLog) }, } From 2815c9ffa7dc335b84dea5e4307cfd8cde630729 Mon Sep 17 00:00:00 2001 From: joyqi Date: Sat, 22 Aug 2026 00:04:56 +0800 Subject: [PATCH 05/11] chat,tool: show what a delegated child cost without paying for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A delegation's price was invisible. The parent's token counter moves only by its own rounds — which is correct, and the point of delegating — but it left no way to tell a cheap child from one that spent twenty rounds reading files, and the cost of a fan-out was unknowable in the one place it mattered. The number goes to the user through the artifact side channel, which already exists for exactly this: content meant for the reader and kept out of the result text. Reporting a delegation's price by spending tokens on the number would be its own small joke. [delegate scout: every call site of parallelRun] · 4 rounds · 12.4k tokens · 8s The slot is now injected for every call rather than only expanded ones, so one channel serves both renderers — a diff for the expanded path, a trailing detail for the event row. The batch path gives each concurrent call its own slot; a shared one would be a race resolved by whoever finished last. finishCall takes the note as a variadic trailing argument. The sixteen call sites with nothing to add stay as they are: appending "" to each would be noise that says nothing about them. The settled classic block keeps the note too. Without that a lone delegation was the one case where the cost was visible while it ran and gone the moment it finished. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- chat/approval.go | 13 +++++++++++++ chat/compose_test.go | 24 ++++++++++++------------ chat/parallel.go | 11 ++++++++--- chat/run.go | 11 ++++++----- chat/transcript.go | 29 +++++++++++++++++++++++++---- tool/delegate.go | 20 ++++++++++++++++++++ 6 files changed, 84 insertions(+), 24 deletions(-) diff --git a/chat/approval.go b/chat/approval.go index 0b156df..7dcee63 100644 --- a/chat/approval.go +++ b/chat/approval.go @@ -3,9 +3,11 @@ package chat import ( "context" "fmt" + "strings" "chatchain/internal/host" "chatchain/internal/ui" + "chatchain/tool" ) // approvalGate is the one place a state-changing call is put to the user. @@ -65,3 +67,14 @@ func (g *approvalGate) ask(ctx context.Context, name, subject string) (bool, err } return true, nil } + +// artifactNote renders a "note" artifact into the event row's trailing +// detail: a short fact about the call meant for the user and withheld from +// the model. The "diff" kind belongs to the expanded path and is ignored +// here — one side channel, read differently by the two renderers. +func artifactNote(art *tool.Artifact) string { + if art == nil || art.Kind != "note" || len(art.Lines) == 0 { + return "" + } + return strings.Join(art.Lines, " · ") +} diff --git a/chat/compose_test.go b/chat/compose_test.go index 4b102e3..f485922 100644 --- a/chat/compose_test.go +++ b/chat/compose_test.go @@ -74,11 +74,11 @@ func TestActivityGroupAggregates(t *testing.T) { "call:" + working, "detail:" + tokens, "call:[a …]", "call:[a full]", - "line:" + eventLine("[a full]", "ok", false), + "line:" + eventLine("[a full]", "ok", false, ""), "call:" + working, "detail:1 tool · " + tokens, "call:[b]", - "line:" + eventLine("[b]", "out", false), + "line:" + eventLine("[b]", "out", false, ""), "call:" + working, "detail:2 tools · " + tokens, "settle", @@ -110,7 +110,7 @@ func TestActivityGroupLoneToolClassic(t *testing.T) { want := []string{ "user:x", "print:", "call:[read_file path:a]", - "line:" + eventLine("[read_file path:a]", "line1\nline2", false), + "line:" + eventLine("[read_file path:a]", "line1\nline2", false, ""), "call:" + working, "detail:1 tool", "settle", @@ -163,11 +163,11 @@ func TestActivityGroupFailBreakout(t *testing.T) { summary := dim("◇ ran 2 tools in 2s") + ErrorStyle.Sprintf(" · %d failed", 1) want := []string{ "call:[a]", - "line:" + eventLine("[a]", "fine", false), + "line:" + eventLine("[a]", "fine", false, ""), "call:" + working, "detail:1 tool", "call:[bash cmd:x]", - "line:" + eventLine("[bash cmd:x]", "exit 1\ndetail", true), + "line:" + eventLine("[bash cmd:x]", "exit 1\ndetail", true, ""), "call:" + working, "detail:2 tools", "settle", @@ -290,7 +290,7 @@ func TestThinkingComposingInterleave(t *testing.T) { "detail:", "call:[bash …]", // the pending call raised at settle, same widget "call:[bash cmd:ls]", - "line:" + eventLine("[bash cmd:ls]", "ok", false), + "line:" + eventLine("[bash cmd:ls]", "ok", false, ""), "call:" + working, "detail:1 tool", "settle", @@ -441,11 +441,11 @@ func TestResetTurnSettlesPartialGroup(t *testing.T) { want := []string{ "user:x", "print:", "call:[a]", - "line:" + eventLine("[a]", "ok", false), + "line:" + eventLine("[a]", "ok", false, ""), "call:" + working, "detail:1 tool", "call:[b]", - "line:" + eventLine("[b]", "ok", false), + "line:" + eventLine("[b]", "ok", false, ""), "call:" + working, "detail:2 tools", "settle", @@ -473,7 +473,7 @@ func TestSettleReopensAfterInterleave(t *testing.T) { want := []string{ "call:[bash …]", "print:", "print:" + ErrorStyle.Sprint("⚠ MCP srv failed: boom"), - "line:" + eventLine("[bash cmd:ls]", "ok", false), + "line:" + eventLine("[bash cmd:ls]", "ok", false, ""), "call:" + working, "detail:1 tool", "print:", "settle", @@ -677,7 +677,7 @@ func TestImageWidgetSettlesActivityFirst(t *testing.T) { want := []string{ "user:draw with tools", "print:", "call:[a]", - "line:" + eventLine("[a]", "ok", false), + "line:" + eventLine("[a]", "ok", false, ""), "call:" + working, "detail:1 tool", "settle", "print:" + strings.Join(classic, "|"), // the lone call settles classic @@ -713,7 +713,7 @@ func TestShowcaseSettlesGroupAndExpandsDiff(t *testing.T) { want := []string{ "user:edit it", "print:", "call:[read_file path:a]", - "line:" + eventLine("[read_file path:a]", "ok", false), + "line:" + eventLine("[read_file path:a]", "ok", false, ""), "call:" + working, "detail:1 tool", "settle", "print:" + strings.Join(classic, "|"), // the group settles first @@ -873,7 +873,7 @@ func TestUserSettlesOpenGroup(t *testing.T) { classic := append([]string{"[a]"}, classicResult("ok", false)...) want := []string{ "call:[a]", - "line:" + eventLine("[a]", "ok", false), + "line:" + eventLine("[a]", "ok", false, ""), "call:" + working, "detail:1 tool", "settle", "print:" + strings.Join(classic, "|"), diff --git a/chat/parallel.go b/chat/parallel.go index 0c25b64..a1feaba 100644 --- a/chat/parallel.go +++ b/chat/parallel.go @@ -53,6 +53,7 @@ type batchOutcome struct { text string isError bool dur time.Duration + note string // user-only detail from the artifact side channel } // runParallelBatch executes calls concurrently and returns their results in @@ -90,11 +91,15 @@ func runParallelBatch(ctx context.Context, pushScope func(context.CancelFunc) fu go func(i int, tc provider.ToolCall) { defer wg.Done() started := time.Now() - text, isError, err := dispatch.CallTool(batchCtx, tc.Name, tc.Arguments) + // One artifact slot per call: the batch runs concurrently, so a + // shared slot would be a race with a last-writer-wins result. + callCtx, artifact := tool.WithArtifact(batchCtx) + text, isError, err := dispatch.CallTool(callCtx, tc.Name, tc.Arguments) if err != nil { text, isError = fmt.Sprintf("Error calling tool: %v", err), true } - outcomes[i] = batchOutcome{text: text, isError: isError, dur: time.Since(started)} + outcomes[i] = batchOutcome{text: text, isError: isError, + dur: time.Since(started), note: artifactNote(artifact())} }(i, tc) } wg.Wait() @@ -103,7 +108,7 @@ func runParallelBatch(ctx context.Context, pushScope func(context.CancelFunc) fu msgs := make([]provider.Message, 0, len(calls)) for i, tc := range calls { - tr.finishCall(headers[i], outcomes[i].text, outcomes[i].isError, outcomes[i].dur) + tr.finishCall(headers[i], outcomes[i].text, outcomes[i].isError, outcomes[i].dur, outcomes[i].note) msgs = append(msgs, provider.Message{ Role: "tool", Content: outcomes[i].text, diff --git a/chat/run.go b/chat/run.go index e1f3370..e180302 100644 --- a/chat/run.go +++ b/chat/run.go @@ -1517,10 +1517,11 @@ func toolLoop(ctx context.Context, u *ui.UI, sink ui.StreamSink, tr *transcript, // Expanded calls report their display payload (the diff) through // the artifact side channel — user-facing only, never billed to // the model. - artifact := func() *tool.Artifact { return nil } - if expanded { - toolCtx, artifact = tool.WithArtifact(toolCtx) - } + // The artifact side channel carries a call's user-facing payload — + // an expanded call's diff, a delegated call's accounting — kept + // out of the result text so it is never billed to the model. + // Every call gets a slot; only some post to one. + toolCtx, artifact := tool.WithArtifact(toolCtx) pop := u.PushCancelScope(cancel) started := time.Now() resultText, isError, callErr := dispatch.CallTool(toolCtx, tc.Name, tc.Arguments) @@ -1539,7 +1540,7 @@ func toolLoop(ctx context.Context, u *ui.UI, sink ui.StreamSink, tr *transcript, if expanded { tr.settleShowcase(header, artifact(), resultText, isError) } else { - tr.finishCall(header, resultText, isError, dur) + tr.finishCall(header, resultText, isError, dur, artifactNote(artifact())) } result := provider.Message{ diff --git a/chat/transcript.go b/chat/transcript.go index e9ac60b..dc3609c 100644 --- a/chat/transcript.go +++ b/chat/transcript.go @@ -86,6 +86,7 @@ type activityGroup struct { firstHeader string // the lone call's classic form, valid while tools == 1 firstResult string firstErr bool + firstNote string // the lone call's user-only trailing detail failLines []string // red breakout rows appended under the summary } @@ -411,7 +412,16 @@ func (t *transcript) openCall(label string) { // row scrolling through the widget, the failure breakout, and — while it is // the group's only call — the material for the classic degenerate form. In // verbose mode the group settles immediately, reproducing the classic block. -func (t *transcript) finishCall(header, result string, isError bool, dur time.Duration) { +// note is an optional trailing detail for the event row — a delegated call's +// round and token count, which the user should see and the model should not +// be billed for. It is variadic so the sixteen call sites that have nothing +// to add stay as they are: appending "" to each of them would be noise that +// says nothing. +func (t *transcript) finishCall(header, result string, isError bool, dur time.Duration, note ...string) { + var extra string + if len(note) > 0 { + extra = note[0] + } t.mu.Lock() defer t.mu.Unlock() if !t.grp.up { @@ -422,6 +432,7 @@ func (t *transcript) finishCall(header, result string, isError bool, dur time.Du t.grp.toolsDur += dur if t.grp.tools == 1 { t.grp.firstHeader, t.grp.firstResult, t.grp.firstErr = header, result, isError + t.grp.firstNote = extra } if isError { t.grp.fails++ @@ -431,7 +442,7 @@ func (t *transcript) finishCall(header, result string, isError bool, dur time.Du t.settleGroupLocked() return } - t.u.CallLine(eventLine(header, result, isError)) + t.u.CallLine(eventLine(header, result, isError, extra)) t.ensureWidgetLocked(dim("Working…")) t.callDetailLocked() } @@ -527,7 +538,14 @@ func (t *transcript) groupLinesLocked() []string { return []string{dim(fmt.Sprintf("%s thought for %s", reasoningSymbol, timefmt.Elapsed(g.thinkDur)))} } if g.tools == 1 && g.thinks == 0 { - lines := []string{g.firstHeader} + // The classic block keeps the note too: a lone call that folds away + // would otherwise be the one case where what a delegation cost is + // visible while it runs and gone once it finishes. + head := g.firstHeader + if g.firstNote != "" { + head += dim(" · " + g.firstNote) + } + lines := []string{head} lc := &lineCommitter{commit: func(ls ...string) { lines = append(lines, ls...) }} printToolResult(lc, g.firstResult, g.firstErr) lc.flush() @@ -548,7 +566,7 @@ func (t *transcript) groupLinesLocked() []string { // eventLine is a completed call's body row inside the widget: a glyph, the // header, and a snippet of the first result line. -func eventLine(header, result string, isError bool) string { +func eventLine(header, result string, isError bool, note string) string { glyph := DimStyle.Sprint("✓") if isError { glyph = ErrorStyle.Sprint("✗") @@ -557,6 +575,9 @@ func eventLine(header, result string, isError bool) string { if first := firstResultLine(result); first != "" { line += dim(" · " + truncateRunes(first, 48)) } + if note != "" { + line += dim(" · " + note) + } return line } diff --git a/tool/delegate.go b/tool/delegate.go index d6548a5..46a0c1f 100644 --- a/tool/delegate.go +++ b/tool/delegate.go @@ -5,6 +5,8 @@ import ( "fmt" "strings" + "chatchain/internal/timefmt" + "chatchain/internal/tokfmt" "chatchain/provider" "gopkg.in/yaml.v3" @@ -159,6 +161,15 @@ func (t *delegateTool) Call(ctx context.Context, args map[string]any) (string, b // lets the model try something else. return fmt.Sprintf("delegation to %q failed: %v", agent, err), true, nil } + // What the child cost goes to the USER through the artifact channel, not + // into the result. Reporting a delegation's price by spending tokens on + // the number would be its own small joke; this way the parent's + // transcript shows it and the parent's context never carries it. + PostArtifact(ctx, Artifact{Kind: "note", Lines: []string{ + fmt.Sprintf("%d round%s", res.Rounds, plural(res.Rounds)), + tokfmt.Tokens(res.Usage.ContextTokens()) + " tokens", + timefmt.Elapsed(res.Duration), + }}) if strings.TrimSpace(res.Reply) == "" { return fmt.Sprintf("agent %q finished without an answer after %d round(s)", agent, res.Rounds), true, nil } @@ -178,3 +189,12 @@ func toAny(ss []string) []any { } return out } + +// plural is the "s" a count needs; a header that says "1 rounds" reads as a +// bug in the thing that printed it. +func plural(n int) string { + if n == 1 { + return "" + } + return "s" +} From f0eaaa5194059ca12d5ed1e5b3dc2d9ad11b8c83 Mon Sep 17 00:00:00 2001 From: joyqi Date: Sat, 22 Aug 2026 00:11:28 +0800 Subject: [PATCH 06/11] chat: the -m loop batches concurrent-safe calls too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parallel execution was reachable only from the interactive loop, because the machinery took a transcript. A scripted run — CI, a pipeline, a parent treating this binary as a child — read four files one at a time for no reason other than where the code happened to live. The concurrency itself never needed a terminal, so it moves into runBatch and both loops call it: the interactive one wraps it in a widget and a cancel scope, the quiet one has neither and needs neither. Nothing else about the quiet loop changes — the approval gate cannot be reached from a batch, since a call may only opt into parallel execution if it needs no approval and opens no surface. Measured on a mock whose children sleep 1.5s each: two read-only delegations from `-m` finish in 2.28s where they took 3.09s, and the server-side timeline shows both starting at +0.00s. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- chat/chat.go | 16 +++++++++++- chat/parallel.go | 50 +++++++++++++++++++++++------------ chat/parallel_test.go | 61 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 18 deletions(-) diff --git a/chat/chat.go b/chat/chat.go index 719a1a8..5da6859 100644 --- a/chat/chat.go +++ b/chat/chat.go @@ -312,7 +312,21 @@ func executeWithTools(ctx context.Context, tp provider.ToolProvider, dispatch to } *history = append(*history, msg) - for _, tc := range toolCalls { + for i := 0; i < len(toolCalls); { + // A run of concurrent-safe calls goes out together. Nothing here + // needs the terminal the interactive path wraps this in: a call + // may only opt into parallel execution if it needs no approval + // and opens no surface, so a batch can never want either. + if j := parallelRun(dispatch, toolCalls, i); j-i >= 2 { + batch := toolCalls[i:j] + for k, o := range runBatch(ctx, dispatch, batch) { + *history = append(*history, batchMessage(batch[k], o)) + } + i = j + continue + } + tc := toolCalls[i] + i++ // Approval gate. This loop has no user of its own, so it either // forwards the question to one who exists — a delegated child // runs inside a parent that owns a terminal — or, with nobody to diff --git a/chat/parallel.go b/chat/parallel.go index a1feaba..128994e 100644 --- a/chat/parallel.go +++ b/chat/parallel.go @@ -84,6 +84,26 @@ func runParallelBatch(ctx context.Context, pushScope func(context.CancelFunc) fu pop = pushScope(cancel) } + outcomes := runBatch(batchCtx, dispatch, calls) + pop() + cancel() + + msgs := make([]provider.Message, 0, len(calls)) + for i, tc := range calls { + tr.finishCall(headers[i], outcomes[i].text, outcomes[i].isError, outcomes[i].dur, outcomes[i].note) + msgs = append(msgs, batchMessage(tc, outcomes[i])) + } + if ctx.Err() != nil { + return msgs, errInterrupted + } + return msgs, nil +} + +// runBatch is the concurrency itself, with no terminal in it: calls go out +// together and their outcomes come back in CALL order. Both loops use it — +// the interactive one wraps it in a widget and a cancel scope, the quiet one +// (-m) has neither and needs neither. +func runBatch(ctx context.Context, dispatch tool.Dispatcher, calls []provider.ToolCall) []batchOutcome { outcomes := make([]batchOutcome, len(calls)) var wg sync.WaitGroup for i, tc := range calls { @@ -93,7 +113,7 @@ func runParallelBatch(ctx context.Context, pushScope func(context.CancelFunc) fu started := time.Now() // One artifact slot per call: the batch runs concurrently, so a // shared slot would be a race with a last-writer-wins result. - callCtx, artifact := tool.WithArtifact(batchCtx) + callCtx, artifact := tool.WithArtifact(ctx) text, isError, err := dispatch.CallTool(callCtx, tc.Name, tc.Arguments) if err != nil { text, isError = fmt.Sprintf("Error calling tool: %v", err), true @@ -103,22 +123,18 @@ func runParallelBatch(ctx context.Context, pushScope func(context.CancelFunc) fu }(i, tc) } wg.Wait() - pop() - cancel() + return outcomes +} - msgs := make([]provider.Message, 0, len(calls)) - for i, tc := range calls { - tr.finishCall(headers[i], outcomes[i].text, outcomes[i].isError, outcomes[i].dur, outcomes[i].note) - msgs = append(msgs, provider.Message{ - Role: "tool", - Content: outcomes[i].text, - ToolCallID: tc.ID, - ToolCallName: tc.Name, - IsError: outcomes[i].isError, - }) +// batchMessage is one call's result as history. Results answer calls in CALL +// order regardless of who finished first — a protocol requirement, not a +// preference. +func batchMessage(tc provider.ToolCall, o batchOutcome) provider.Message { + return provider.Message{ + Role: "tool", + Content: o.text, + ToolCallID: tc.ID, + ToolCallName: tc.Name, + IsError: o.isError, } - if ctx.Err() != nil { - return msgs, errInterrupted - } - return msgs, nil } diff --git a/chat/parallel_test.go b/chat/parallel_test.go index 2a17e3a..96973b4 100644 --- a/chat/parallel_test.go +++ b/chat/parallel_test.go @@ -2,6 +2,7 @@ package chat import ( "context" + "io" "strings" "sync/atomic" "testing" @@ -218,3 +219,63 @@ func (noCapDispatch) Tools() []provider.ToolDef { return nil } func (noCapDispatch) CallTool(context.Context, string, map[string]any) (string, bool, error) { return "", false, nil } + +// The quiet (-m) loop batches too. It had no parallelism at all: the +// machinery took a transcript, so only the interactive path could reach it, +// and a scripted run — CI, a pipeline, a parent treating this binary as a +// child — read four files one at a time for no reason. +func TestQuietLoopBatchesParallelCalls(t *testing.T) { + d := ¶llelDispatch{ + parallel: map[string]bool{"read_file": true}, + enter: make(chan struct{}, 4), + release: make(chan struct{}), + } + tp := &batchingProvider{calls: []provider.ToolCall{ + call("1", "read_file"), call("2", "read_file"), call("3", "read_file"), + }} + // Release only once every call has started: if the loop were serial the + // second would never start and this would deadlock into the test timeout. + go func() { + for i := 0; i < 3; i++ { + <-d.enter + } + close(d.release) + }() + history := []provider.Message{{Role: "user", Content: "go"}} + reply, _, err := executeWithTools(context.Background(), tp, d, &history, + nil, "", 0, quietHost{rec: newRunRecorder()}) + if err != nil { + t.Fatalf("quiet loop failed: %v", err) + } + if reply != "done" { + t.Fatalf("reply = %q", reply) + } + if d.peak < 3 { + t.Errorf("peak concurrency = %d, want 3 — the calls did not overlap", d.peak) + } + // Results still answer their calls in call order, batch or not. + var ids []string + for _, m := range history { + if m.Role == "tool" { + ids = append(ids, m.ToolCallID) + } + } + if strings.Join(ids, ",") != "1,2,3" { + t.Errorf("tool results in %v, want call order", ids) + } +} + +// batchingProvider asks for a fixed set of calls once, then answers. +type batchingProvider struct { + calls []provider.ToolCall + round int +} + +func (p *batchingProvider) StreamChatWithTools(ctx context.Context, msgs []provider.Message, tools []provider.ToolDef, w io.Writer, reasoning io.WriteCloser) (string, string, []provider.ToolCall, error) { + reasoning.Close() + p.round++ + if p.round == 1 { + return "", "", p.calls, nil + } + return "done", "", nil, nil +} From c67fb2a4db062d2c1a2a80473d194f69c58b5b0a Mon Sep 17 00:00:00 2001 From: joyqi Date: Sat, 22 Aug 2026 00:15:16 +0800 Subject: [PATCH 07/11] docs: the delegate toolset and the code set's read-only mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither the code set's configuration nor delegation was described anywhere — auto_write had a paragraph, read_only had none, and delegate was absent entirely. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- README.md | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 832bebe..a78169f 100644 --- a/README.md +++ b/README.md @@ -321,8 +321,9 @@ enabled by listing it under that provider's `tools:` key; the value is the set's shared configuration, and an empty value uses its defaults. Available sets: `shell` (running bash commands, sandboxed), `code` (reading, searching, and editing project files), `agent` (skill activation; auto-enabled by agent -mode), and `ask` (interactive questions to the user; enabled by default in -interactive sessions — disable with `ask: false`). +mode), `ask` (interactive questions to the user; enabled by default in +interactive sessions — disable with `ask: false`), and `delegate` (running a +task as a child agent). ```yaml providers: @@ -355,6 +356,38 @@ single yes/no. ESC declines — the model is told and proceeds on its own. Zero side effects, on by default interactively, absent in `-m` runs; opt out per provider with `tools: {ask: false}`. +#### `delegate` — `delegate` + +Runs a task as a **child agent**: its own context, its own tool loop, and +only its final answer comes back — no tool calls, no reasoning. A survey that +takes twenty rounds costs this conversation one paragraph. + +An agent is a name for a provider entry you already have, so nothing about +the child is configured twice — its model, tools, approval settings, system +prompt and sampling all come from the entry it names: + +```yaml +tools: + delegate: + max_turns: 30 # per child; default 30 + agents: + search: fast-provider + review: + provider: careful-provider + description: Reads a diff and reports what is wrong with it +``` + +`description` is the only field that is not already over there, and it is +what the model chooses between agents on. + +Delegations to an agent whose tools cannot change state run **concurrently**; +anything else runs one at a time. A child's write requests surface as an +approval prompt in your terminal, labelled with the agent that asked +(`review › edit_file wants to modify files`), and "allow for this session" +covers parent and children alike. What a child cost appears beside its call +and is never added to this conversation's context. A child is never given +`delegate` itself. + #### `shell` — `bash` Lets the model run real bash command lines — pipes, redirects, `&&` chaining, @@ -405,6 +438,11 @@ Safety model: auto_write: true # optional; default asks before every write ``` + Or withhold the writers entirely with `read_only: true`, leaving `glob`, + `grep`, `list_dir` and `read_file`. A tool the model cannot see is never + attempted and never refused — useful for a reviewer, and required for a + `delegate` agent that should search and still run concurrently. + Design: docs/design/code-toolset.md ### Agent Mode From 3202e7541211aa7cca2a35dc52cd0fd2553d6da1 Mon Sep 17 00:00:00 2001 From: joyqi Date: Sat, 22 Aug 2026 12:35:31 +0800 Subject: [PATCH 08/11] cmd: a delegation runs uncapped, like everything else here MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit max_turns defaulted to 30 on the reasoning that a child has nobody watching it round by round. That is not true: ESC cancels the turn, and the context it cancels reaches the child's every round, so the brake works on a delegation exactly as it does on anything else. Which leaves a default that contradicted the two loops beside it — --max-turns is 0, and the interactive loop says in as many words that it has no round cap because the user is the brake. Unlimited is now the default here too; the key stays, because a cap the user chooses is a different thing from one they were given. It is still the wrong instrument for a wedged child. That failure is measured in wall-clock — bash caps at ten minutes, pi's delegate extension at fifteen — and thirty cheap rounds cost nothing like three expensive ones. A timeout is worth having, and worth having separately. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- README.md | 2 +- cmd/delegate.go | 20 ++++++++++++++------ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index a78169f..0feddf3 100644 --- a/README.md +++ b/README.md @@ -369,7 +369,7 @@ prompt and sampling all come from the entry it names: ```yaml tools: delegate: - max_turns: 30 # per child; default 30 + max_turns: 30 # optional; default unlimited, as ESC stops a child too agents: search: fast-provider review: diff --git a/cmd/delegate.go b/cmd/delegate.go index 841dc76..ac126f9 100644 --- a/cmd/delegate.go +++ b/cmd/delegate.go @@ -42,10 +42,18 @@ func (a *agentRef) UnmarshalYAML(n *yaml.Node) error { return n.Decode((*raw)(a)) } -// delegateDefaultMaxTurns bounds a child that will not stop. The parent's own -// cap is the user pressing ESC; a child has nobody watching it round by -// round, so it gets a number instead. -const delegateDefaultMaxTurns = 30 +// max_turns defaults to unlimited, like --max-turns and like the interactive +// loop, which states the reason at chat/run.go: the user is the brake. +// +// A child is no exception to that. ESC cancels the turn, and the context it +// cancels reaches the child's every round — so the brake works on a +// delegation exactly as it does on anything else. The earlier default of 30 +// rested on the child having nobody watching it, which is not true. +// +// The key stays, because a cap the user chooses is different from one they +// were given. What it is NOT is a guard against a wedged child: that failure +// is measured in wall-clock, the way bash and pi's delegate extension measure +// it, and thirty cheap rounds cost nothing like three expensive ones. // buildDelegator resolves every configured agent up front — a name that does // not resolve is a startup error, not a surprise three tool calls into a @@ -61,8 +69,8 @@ func buildDelegator(cfg *config.Config, node yaml.Node, hc httpClientSource, roo return nil, fmt.Errorf("no agents configured (add `agents:` mapping agent names to provider names)") } maxTurns := sc.MaxTurns - if maxTurns <= 0 { - maxTurns = delegateDefaultMaxTurns + if maxTurns < 0 { + maxTurns = 0 } type resolved struct { From fee5b8d78cd5a58222ac182a1920f62f68271ad8 Mon Sep 17 00:00:00 2001 From: joyqi Date: Sun, 23 Aug 2026 01:09:01 +0800 Subject: [PATCH 09/11] fix: five defects two delegated reviewers found in this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewed by gpt-5.6-sol and glm-5.3, run concurrently through the delegate toolset this branch adds. Their sixth finding — that the unlimited max_turns default is justified by a brake that does not exist in -m — is real and left for its own change. Every one of these is a promise made in a comment and not kept by the code beside it. The approval prompt named only the tool. For a delegated call nothing else on screen identified the operation: the widget above describes the DELEGATION, not what the child asked to do, so the user was approving "edit_file" with no idea which file. The detail now travels with the request, rendered by whichever side holds the dispatcher that owns the tool — a child's tools are its own, and the parent cannot describe them. toolCallDetail is the header's existing logic, split out so both readers share one implementation. An `agent: true` child never got load_skill. AgentMode injects the AGENTS.md/skills text; the tool comes from the agent SET, which the main session enables separately and this did not — so a child was told which skills exist and given no way to open one, while the docs claimed the provider entry decides a child's tools. A malformed toolset passed startup and then wrote ANSI to stderr from under bubbletea. The validation pass used a silent warnf and the per-delegation rebuild used a loud one; now one helper builds both, its complaints are the startup error this function already promised to produce, and the runtime build says nothing because there is nothing left to say. The parallel classification read the raw agent argument while execution read it trimmed, so a call could be admitted to a batch as a read-only agent and then run as a write-capable one. Both resolve it through one helper. A failed child's accounting was discarded — the early return on error came before the artifact — hiding the cost exactly where it is surprising, and contradicting the same principle stated one commit earlier for -m runs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- chat/approval.go | 19 ++++++-- chat/approval_test.go | 73 +++++++++++++++++++++++++++-- chat/chat.go | 25 ++++++---- chat/delegate.go | 20 ++++---- chat/run.go | 6 +-- cmd/delegate.go | 63 +++++++++++++++++++------ cmd/delegate_test.go | 59 +++++++++++++++++++++++- cmd/root.go | 5 +- tool/delegate.go | 37 ++++++++++----- tool/delegate_test.go | 105 ++++++++++++++++++++++++++++++++++++++++++ 10 files changed, 352 insertions(+), 60 deletions(-) create mode 100644 tool/delegate_test.go diff --git a/chat/approval.go b/chat/approval.go index 7dcee63..e251c8a 100644 --- a/chat/approval.go +++ b/chat/approval.go @@ -29,18 +29,29 @@ type approvalGate struct { approved map[string]bool // "allow for this session", keyed by tool name } -// ask resolves one gated call. subject names where the request came from when -// it was not this conversation ("search" for a delegation); empty for the -// conversation's own calls, whose origin needs no saying. +// ask resolves one gated call. +// +// detail is what the call is about — the path it writes, the command it runs. +// Without it the prompt named only the tool, which for a delegated call left +// nothing on screen to identify the operation: the widget above describes the +// DELEGATION, not what the child is asking to do. Approving "edit_file" with +// no idea which file is not consent. +// +// subject names where the request came from when it was not this conversation +// ("search" for a delegation); empty for the conversation's own calls, whose +// origin needs no saying. // // The error is reserved for the prompt itself failing. A refusal is (false, // nil): the caller turns it into a result the model can read, and a turn that // continues after a denial is the point of asking. -func (g *approvalGate) ask(ctx context.Context, name, subject string) (bool, error) { +func (g *approvalGate) ask(ctx context.Context, name, detail, subject string) (bool, error) { if g.approved[name] { return true, nil } label := displayToolName(name) + if detail != "" { + label += " " + detail + } if subject != "" { label = subject + " › " + label } diff --git a/chat/approval_test.go b/chat/approval_test.go index 6dfe8a6..77b5830 100644 --- a/chat/approval_test.go +++ b/chat/approval_test.go @@ -68,7 +68,7 @@ func TestQuietLoopRefusesWhenThereIsNobodyToAsk(t *testing.T) { func TestQuietLoopForwardsApproval(t *testing.T) { var asked []string host := quietHost{rec: newRunRecorder(), - approve: func(_ context.Context, tc provider.ToolCall) (bool, string) { + approve: func(_ context.Context, tc provider.ToolCall, _ string) (bool, string) { asked = append(asked, tc.Name) return true, "" }} @@ -91,7 +91,7 @@ func TestQuietLoopForwardsApproval(t *testing.T) { // on and reports back, which is what the parent's terminal showed. func TestQuietLoopDenialContinuesTheRun(t *testing.T) { host := quietHost{rec: newRunRecorder(), - approve: func(_ context.Context, tc provider.ToolCall) (bool, string) { + approve: func(_ context.Context, tc provider.ToolCall, _ string) (bool, string) { return false, "The user declined this call." }} reply, d, err := runGated(t, host) @@ -110,14 +110,77 @@ func TestQuietLoopDenialContinuesTheRun(t *testing.T) { // not be mistaken for consent. func TestQuietHostAskApprovalShapes(t *testing.T) { var h quietHost - ok, why := h.askApproval(context.Background(), provider.ToolCall{Name: "edit_file"}) + ok, why := h.askApproval(context.Background(), provider.ToolCall{Name: "edit_file"}, "path:x") if ok || !strings.Contains(why, "edit_file") { t.Errorf("nil approver = (%v, %q)", ok, why) } - h.approve = func(_ context.Context, tc provider.ToolCall) (bool, string) { + h.approve = func(_ context.Context, tc provider.ToolCall, _ string) (bool, string) { return false, fmt.Sprintf("%v", errors.New("prompt broke")) } - if ok, why := h.askApproval(context.Background(), provider.ToolCall{Name: "edit_file"}); ok || why != "prompt broke" { + if ok, why := h.askApproval(context.Background(), provider.ToolCall{Name: "edit_file"}, "path:x"); ok || why != "prompt broke" { t.Errorf("failed prompt = (%v, %q), want a refusal carrying the reason", ok, why) } } + +// detailDispatch names the file its gated tool would write, the way the code +// set's headliner does. +type detailDispatch struct{ gatedDispatch } + +func (detailDispatch) HeaderSummary(_ string, args map[string]any) (string, bool) { + p, _ := args["path"].(string) + return p, true +} + +// The prompt has to say what the call is ABOUT. For a delegated call nothing +// else on screen does: the widget above describes the delegation, not the +// operation the child is asking to perform, so a gate naming only the tool +// asks the user to authorize "edit_file" without saying which file. +func TestForwardedApprovalCarriesTheCallDetail(t *testing.T) { + var seen []string + host := quietHost{rec: newRunRecorder(), + approve: func(_ context.Context, _ provider.ToolCall, detail string) (bool, string) { + seen = append(seen, detail) + return false, "The user declined this call." + }} + d := &detailDispatch{} + tp := &pathWritingProvider{} + history := []provider.Message{{Role: "user", Content: "go"}} + if _, _, err := executeWithTools(context.Background(), tp, d, &history, + d.Tools(), "", 0, host); err != nil { + t.Fatal(err) + } + if len(seen) != 1 || seen[0] != "internal/ui/model.go" { + t.Errorf("gate saw detail %v, want the path the call would write", seen) + } +} + +type pathWritingProvider struct{ calls int } + +func (p *pathWritingProvider) StreamChatWithTools(ctx context.Context, msgs []provider.Message, tools []provider.ToolDef, w io.Writer, reasoning io.WriteCloser) (string, string, []provider.ToolCall, error) { + reasoning.Close() + p.calls++ + if p.calls == 1 { + return "", "", []provider.ToolCall{{ID: "c1", Name: "write_file", + Arguments: map[string]any{"path": "internal/ui/model.go"}}}, nil + } + return "done", "", nil, nil +} + +// The header keeps its shape after the detail was split out of it — one +// implementation, two readers. +func TestToolCallHeaderUnchangedBySplit(t *testing.T) { + d := &detailDispatch{} + tc := provider.ToolCall{Name: "write_file", Arguments: map[string]any{"path": "a/b.go"}} + if got := toolCallHeader(d, tc); got != "[write_file a/b.go]" { + t.Errorf("header = %q", got) + } + // A tool with no summary of its own falls back to the argument digest, + // and an empty summary renders as a bare name rather than the digest. + plain := &gatedDispatch{} + if got := toolCallHeader(plain, tc); got != "[write_file path:a/b.go]" { + t.Errorf("digest header = %q", got) + } + if got := toolCallHeader(plain, provider.ToolCall{Name: "x"}); got != "[x]" { + t.Errorf("bare header = %q", got) + } +} diff --git a/chat/chat.go b/chat/chat.go index 5da6859..08d8e52 100644 --- a/chat/chat.go +++ b/chat/chat.go @@ -332,7 +332,7 @@ func executeWithTools(ctx context.Context, tp provider.ToolProvider, dispatch to // runs inside a parent that owns a terminal — or, with nobody to // ask, refuses and says how to enable the call. if needsApproval(dispatch, tc.Name) { - allowed, why := host.askApproval(ctx, tc) + allowed, why := host.askApproval(ctx, tc, toolCallDetail(dispatch, tc)) if !allowed { *history = append(*history, provider.Message{ Role: "tool", @@ -422,15 +422,23 @@ const toolHeaderMaxValue = 15 // to one line and truncated, and arguments past toolHeaderMaxArgs collapse to a // "… +N args" tail. func toolCallHeader(dispatch tool.Dispatcher, tc provider.ToolCall) string { + name := displayToolName(tc.Name) + if detail := toolCallDetail(dispatch, tc); detail != "" { + return "[" + name + " " + detail + "]" + } + return "[" + name + "]" +} + +// toolCallDetail is what a header says about a call BESIDES its name: the +// tool's own summary, else the argument digest. The approval prompt shows it +// too — a gate that names only the tool asks the user to authorize +// "edit_file" without saying which file. +func toolCallDetail(dispatch tool.Dispatcher, tc provider.ToolCall) string { // A tool that writes its own summary takes over completely — an empty // one renders as a bare "[name]", never as the argument digest below // (see tool.headliner: edit_file's new_string must not reach a header). if summary, ok := headerSummaryOf(dispatch, tc.Name, tc.Arguments); ok { - name := displayToolName(tc.Name) - if summary == "" { - return "[" + name + "]" - } - return "[" + name + " " + summary + "]" + return summary } keys := make([]string, 0, len(tc.Arguments)) for k := range tc.Arguments { @@ -443,8 +451,7 @@ func toolCallHeader(dispatch tool.Dispatcher, tc provider.ToolCall) string { shown = shown[:toolHeaderMaxArgs] } - parts := make([]string, 0, len(shown)+2) - parts = append(parts, displayToolName(tc.Name)) + parts := make([]string, 0, len(shown)+1) for _, k := range shown { v := strings.ReplaceAll(fmt.Sprintf("%v", tc.Arguments[k]), "\n", " ") parts = append(parts, k+":"+truncateRunes(v, toolHeaderMaxValue)) @@ -452,7 +459,7 @@ func toolCallHeader(dispatch tool.Dispatcher, tc provider.ToolCall) string { if extra := len(keys) - len(shown); extra > 0 { parts = append(parts, fmt.Sprintf("… +%d args", extra)) } - return "[" + strings.Join(parts, " ") + "]" + return strings.Join(parts, " ") } // toolResultMaxLines is how many lines of a tool result are shown inline; extra diff --git a/chat/delegate.go b/chat/delegate.go index b2d61ea..983dc12 100644 --- a/chat/delegate.go +++ b/chat/delegate.go @@ -28,18 +28,22 @@ type quietHost struct { // terminal. A -m run leaves it nil because there is nobody to ask; a // delegated child sets it because it has no user of its own but runs // inside a parent that does. - approve func(ctx context.Context, tc provider.ToolCall) (bool, string) + approve func(ctx context.Context, tc provider.ToolCall, detail string) (bool, string) } // askApproval resolves one gated call: allowed, or the refusal to hand back // as the call's result. -func (h quietHost) askApproval(ctx context.Context, tc provider.ToolCall) (bool, string) { +// +// detail is what the call is ABOUT — the path, the command — rendered by the +// caller, because only it holds the dispatcher that owns the tool. A child's +// tools are its own, and the parent asking on its behalf cannot describe them. +func (h quietHost) askApproval(ctx context.Context, tc provider.ToolCall, detail string) (bool, string) { if h.approve == nil { return false, fmt.Sprintf("%s was not executed: it requires interactive approval, "+ "which is unavailable in this non-interactive run. Set the toolset's auto-approve option "+ "(tools.code.auto_write / tools.shell.auto_run) to permit it here.", tc.Name) } - return h.approve(ctx, tc) + return h.approve(ctx, tc, detail) } // Child is everything a delegated run needs, assembled by the host. @@ -68,7 +72,7 @@ type Delegator struct { build ChildFactory mu sync.Mutex - approve func(ctx context.Context, agent string, tc provider.ToolCall) (bool, string) + approve func(ctx context.Context, agent string, tc provider.ToolCall, detail string) (bool, string) } // NewDelegator prepares the seam. Names are sorted once: the agent list @@ -93,13 +97,13 @@ func (d *Delegator) Agent(name string) (tool.AgentInfo, bool) { // SetApprover binds the parent's approval gate, which only exists once there // is a live UI. Until then a child's state-changing calls are refused, which // is the same answer any other non-interactive run gives. -func (d *Delegator) SetApprover(fn func(ctx context.Context, agent string, tc provider.ToolCall) (bool, string)) { +func (d *Delegator) SetApprover(fn func(ctx context.Context, agent string, tc provider.ToolCall, detail string) (bool, string)) { d.mu.Lock() defer d.mu.Unlock() d.approve = fn } -func (d *Delegator) approver() func(context.Context, string, provider.ToolCall) (bool, string) { +func (d *Delegator) approver() func(context.Context, string, provider.ToolCall, string) (bool, string) { d.mu.Lock() defer d.mu.Unlock() return d.approve @@ -130,10 +134,10 @@ func (d *Delegator) Run(ctx context.Context, spec tool.DelegateSpec) (tool.Deleg // only run in parallel when its agent grants no state-changing tool // (tool.delegateTool.SupportsParallel); the lock is what keeps that // from being load-bearing. - host.approve = func(ctx context.Context, tc provider.ToolCall) (bool, string) { + host.approve = func(ctx context.Context, tc provider.ToolCall, detail string) (bool, string) { d.mu.Lock() defer d.mu.Unlock() - return fn(ctx, spec.Agent, tc) + return fn(ctx, spec.Agent, tc, detail) } } diff --git a/chat/run.go b/chat/run.go index e180302..cd01bf2 100644 --- a/chat/run.go +++ b/chat/run.go @@ -173,8 +173,8 @@ func Run(p, titleP provider.Provider, systemPrompt string, systemInteractive boo // A child has no terminal of its own. Rather than inventing a second // gate for it — two prompts with two memories for one person — its // questions arrive at this one, labelled with the agent that asked. - delegator.SetApprover(func(ctx context.Context, agent string, tc provider.ToolCall) (bool, string) { - ok, err := gate.ask(ctx, tc.Name, agent) + delegator.SetApprover(func(ctx context.Context, agent string, tc provider.ToolCall, detail string) (bool, string) { + ok, err := gate.ask(ctx, tc.Name, detail, agent) switch { case err != nil: return false, fmt.Sprintf("%s was not executed: %v", tc.Name, err) @@ -1491,7 +1491,7 @@ func toolLoop(ctx context.Context, u *ui.UI, sink ui.StreamSink, tr *transcript, // The widget header above shows what is being approved; the group // clock pauses while the user deliberates. if needsApproval(dispatch, tc.Name) { - allowed, aerr := gate.ask(ctx, tc.Name, "") + allowed, aerr := gate.ask(ctx, tc.Name, toolCallDetail(dispatch, tc), "") if aerr != nil { return "", "", aerr } diff --git a/cmd/delegate.go b/cmd/delegate.go index ac126f9..83fa2ab 100644 --- a/cmd/delegate.go +++ b/cmd/delegate.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "os" + "strings" "chatchain/chat" "chatchain/config" @@ -58,7 +59,7 @@ func (a *agentRef) UnmarshalYAML(n *yaml.Node) error { // buildDelegator resolves every configured agent up front — a name that does // not resolve is a startup error, not a surprise three tool calls into a // conversation. -func buildDelegator(cfg *config.Config, node yaml.Node, hc httpClientSource, root string, warnf func(string, ...any)) (*chat.Delegator, error) { +func buildDelegator(cfg *config.Config, node yaml.Node, hc httpClientSource, root string) (*chat.Delegator, error) { var sc delegateConfig if !node.IsZero() { if err := node.Decode(&sc); err != nil { @@ -99,10 +100,19 @@ func buildDelegator(cfg *config.Config, node yaml.Node, hc httpClientSource, roo return nil, fmt.Errorf("agent %q: provider %q has no `model:` (a delegated agent cannot be asked to pick one)", name, ref.Provider) } tools := childTools(pc.Tools) - // A child's toolset is built once here so its access can be reported - // to the model and, more importantly, so the parallel decision rests - // on what the user configured rather than on what a task claims. - reg := tool.Build(tool.Env{ProjectRoot: root}, tools, func(string, ...any) {}) + // The toolset is built here so its access can be reported to the + // model and, more importantly, so the parallel decision rests on what + // the user configured rather than on what a task claims. + // + // Complaints are collected rather than printed, and a complaint here + // IS the startup error this function promises. The alternative was + // what shipped: validate with a silent warnf, then rebuild per + // delegation with a loud one — so a malformed toolset passed startup + // and later wrote ANSI to stderr while bubbletea owned the screen. + reg, warnings := buildChildTools(root, tools, pc.Agent) + if len(warnings) > 0 { + return nil, fmt.Errorf("agent %q: %s", name, strings.Join(warnings, "; ")) + } agents[name] = tool.AgentInfo{Description: ref.Description, ReadOnly: readOnlyRegistry(reg)} byName[name] = resolved{ptype: ptype, pc: pc, tools: tools} } @@ -137,13 +147,14 @@ func buildDelegator(cfg *config.Config, node yaml.Node, hc httpClientSource, roo if err != nil { return chat.Child{}, fmt.Errorf("agent %q: %w", name, err) } - // The child's own toolset: no ask seam (nobody to question but the - // parent's user, and the child is not the conversation they are in) - // and no Delegate, which is what stops the recursion. - env := tool.Env{ProjectRoot: root} + // The child's own toolset, built the same way it was validated. Its + // complaints are dropped, not printed: startup already refused + // anything that would produce one, and there is no second thing left + // to say from inside a running turn. + dispatch, _ := buildChildTools(root, r.tools, r.pc.Agent) return chat.Child{ Provider: p, - Dispatch: tool.Build(env, r.tools, warnf), + Dispatch: dispatch, System: sys, AgentMode: chat.AgentOptions{Enabled: r.pc.Agent, Root: root}, MaxTurns: maxTurns, @@ -152,6 +163,28 @@ func buildDelegator(cfg *config.Config, node yaml.Node, hc httpClientSource, roo return chat.NewDelegator(agents, build), nil } +// buildChildTools assembles one child's toolset and returns whatever the +// build had to complain about, rather than printing it. +// +// Agent mode is applied HERE, not left to the overlay. AgentMode only injects +// the AGENTS.md/skills text; load_skill comes from the agent SET, which the +// main session enables separately (buildDispatcher). Without this an agent +// configured `agent: true` was told which skills exist and given no way to +// open one — and the promise that a provider entry decides a child's tools +// was quietly untrue. +func buildChildTools(root string, tools map[string]yaml.Node, agentMode bool) (tool.Dispatcher, []string) { + var warnings []string + warn := func(format string, a ...any) { + warnings = append(warnings, fmt.Sprintf(format, a...)) + } + env := tool.Env{ProjectRoot: root} // no Interact: a child is not the conversation the user is in + reg := tool.Build(env, tools, warn) + if agentMode { + reg.EnableSet(env, "agent", warn) + } + return reg, warnings +} + // httpClientSource is the recording transport, narrowed to the one method a // child needs — its requests belong in /debug alongside the parent's. type httpClientSource interface{ HTTPClient() *http.Client } @@ -176,9 +209,13 @@ func childTools(raw map[string]yaml.Node) map[string]yaml.Node { // anything: the parallel opt-in already means "does not write, needs no // approval, opens no surface". Reusing it keeps one definition of harmless // instead of two that could disagree — and an empty set is trivially read-only. -func readOnlyRegistry(reg *tool.Registry) bool { - for _, def := range reg.Tools() { - if !reg.SupportsParallel(def.Name, nil) { +func readOnlyRegistry(d tool.Dispatcher) bool { + pr, ok := d.(tool.ParallelReporter) + if !ok { + return false // cannot say it is harmless, so do not say it + } + for _, def := range d.Tools() { + if !pr.SupportsParallel(def.Name, nil) { return false } } diff --git a/cmd/delegate_test.go b/cmd/delegate_test.go index 5c8e465..6e18534 100644 --- a/cmd/delegate_test.go +++ b/cmd/delegate_test.go @@ -4,6 +4,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "testing" "chatchain/config" @@ -51,7 +52,7 @@ agents: scout: {provider: scout, description: searches but cannot write} slow: {provider: codeboy, description: can edit files} `) - del, err := buildDelegator(cfg, node, nopHTTP{}, t.TempDir(), func(string, ...any) {}) + del, err := buildDelegator(cfg, node, nopHTTP{}, t.TempDir()) if err != nil { t.Fatalf("buildDelegator: %v", err) } @@ -92,7 +93,7 @@ agents: func TestDelegateRequiresAModel(t *testing.T) { cfg := loadConfig(t, "providers:\n worker: {type: openai, key: k}\n") node := agentsNode(t, "agents:\n fast: worker\n") - if _, err := buildDelegator(cfg, node, nopHTTP{}, t.TempDir(), func(string, ...any) {}); err == nil { + if _, err := buildDelegator(cfg, node, nopHTTP{}, t.TempDir()); err == nil { t.Fatal("an agent whose provider has no model: must be a startup error") } } @@ -126,3 +127,57 @@ func TestChildToolsDropDelegate(t *testing.T) { t.Errorf("childTools dropped more than delegate: %v", got) } } + +// AgentMode only injects the AGENTS.md/skills text; load_skill comes from the +// agent SET, which the main session enables separately. A child configured +// `agent: true` was told which skills exist and given no way to open one. +func TestDelegateChildGetsAgentModeTools(t *testing.T) { + has := func(d tool.Dispatcher, name string) bool { + for _, def := range d.Tools() { + if def.Name == name { + return true + } + } + return false + } + on, warn := buildChildTools(t.TempDir(), nil, true) + if len(warn) > 0 { + t.Fatalf("unexpected warnings: %v", warn) + } + if !has(on, "load_skill") { + t.Error("an agent-mode child must be able to open the skills it is told about") + } + off, _ := buildChildTools(t.TempDir(), nil, false) + if has(off, "load_skill") { + t.Error("a child without agent mode gained load_skill") + } + + // And the flag reaches the builder from the provider entry: load_skill is + // not parallel-safe, so an agent-mode child cannot be classified read-only. + cfg := loadConfig(t, "providers:\n skilled: {type: openai, key: k, model: m, agent: true}\n") + del, err := buildDelegator(cfg, agentsNode(t, "agents:\n a: skilled\n"), nopHTTP{}, t.TempDir()) + if err != nil { + t.Fatalf("buildDelegator: %v", err) + } + if info, _ := del.Agent("a"); info.ReadOnly { + t.Error("an agent-mode child holds load_skill and cannot be read-only") + } +} + +// A malformed toolset in an agent's provider entry has to be a startup error. +// It was validated with a silent warnf and then rebuilt with a loud one per +// delegation, so it passed startup and later wrote to stderr mid-turn. +func TestDelegateRejectsAMalformedChildToolset(t *testing.T) { + cfg := loadConfig(t, ` +providers: + broken: {type: openai, key: k, model: m, tools: {code: [not, a, mapping]}} +`) + node := agentsNode(t, "agents:\n a: broken\n") + _, err := buildDelegator(cfg, node, nopHTTP{}, t.TempDir()) + if err == nil { + t.Fatal("a malformed child toolset must fail at startup") + } + if !strings.Contains(err.Error(), "a") { + t.Errorf("the error should name the agent: %v", err) + } +} diff --git a/cmd/root.go b/cmd/root.go index dd9810e..ec6cfce 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -234,10 +234,7 @@ var rootCmd = &cobra.Command{ // bad provider name fails at startup rather than three tool calls // into a conversation; without the seam the set contributes no tools. if node, ok := pc.Tools["delegate"]; ok && !tool.SetDisabled(pc.Tools, "delegate") { - warnf := func(format string, a ...any) { - chat.ErrorStyle.Fprintf(os.Stderr, "⚠ "+format+"\n", a...) - } - del, derr := buildDelegator(cfg, node, reqLog, toolEnv.ProjectRoot, warnf) + del, derr := buildDelegator(cfg, node, reqLog, toolEnv.ProjectRoot) if derr != nil { return fmt.Errorf("tools.delegate: %w", derr) } diff --git a/tool/delegate.go b/tool/delegate.go index 46a0c1f..c30e2fe 100644 --- a/tool/delegate.go +++ b/tool/delegate.go @@ -65,11 +65,18 @@ type delegateTool struct { // concurrently could otherwise get one by describing its task as read-only. // An unconfigured or missing agent resolves to false: unknown means serial. func (t *delegateTool) SupportsParallel(args map[string]any) bool { - name, _ := args["agent"].(string) - info, ok := t.d.Agent(name) + info, ok := t.d.Agent(agentArg(args)) return ok && info.ReadOnly } +// agentArg reads the agent name the way BOTH the parallel classification and +// the execution must read it. They resolved it differently once — one raw, +// one trimmed — which let a call be admitted to a batch as one agent and then +// run as another. Whatever the rule is, the two have to share it. +func agentArg(args map[string]any) string { + return strings.TrimSpace(stringArg(args, "agent")) +} + // HeaderSummary puts the agent and the head of its brief in the call header — // "[delegate search: every call site of parallelRun]". The task is the only // thing that distinguishes two delegations to one agent, and the agent name @@ -137,7 +144,7 @@ func (t *delegateTool) Def() provider.ToolDef { } func (t *delegateTool) Call(ctx context.Context, args map[string]any) (string, bool, error) { - agent := strings.TrimSpace(stringArg(args, "agent")) + agent := agentArg(args) if agent == "" { return "missing required argument: agent", true, nil } @@ -155,21 +162,27 @@ func (t *delegateTool) Call(ctx context.Context, args map[string]any) (string, b } res, err := t.d.Run(ctx, DelegateSpec{Agent: agent, Task: task, Effort: effort}) + // What the child cost goes to the USER through the artifact channel, not + // into the result. Reporting a delegation's price by spending tokens on + // the number would be its own small joke; this way the parent's + // transcript shows it and the parent's context never carries it. + // + // A FAILED child is billed for the rounds it completed, and it is the run + // worth investigating — reporting the cost only on success would hide it + // exactly where it is surprising. + if res.Rounds > 0 { + PostArtifact(ctx, Artifact{Kind: "note", Lines: []string{ + fmt.Sprintf("%d round%s", res.Rounds, plural(res.Rounds)), + tokfmt.Tokens(res.Usage.ContextTokens()) + " tokens", + timefmt.Elapsed(res.Duration), + }}) + } if err != nil { // The child's failure is the parent's result, not the parent's crash: // a returned error would abort the whole round, while a tool error // lets the model try something else. return fmt.Sprintf("delegation to %q failed: %v", agent, err), true, nil } - // What the child cost goes to the USER through the artifact channel, not - // into the result. Reporting a delegation's price by spending tokens on - // the number would be its own small joke; this way the parent's - // transcript shows it and the parent's context never carries it. - PostArtifact(ctx, Artifact{Kind: "note", Lines: []string{ - fmt.Sprintf("%d round%s", res.Rounds, plural(res.Rounds)), - tokfmt.Tokens(res.Usage.ContextTokens()) + " tokens", - timefmt.Elapsed(res.Duration), - }}) if strings.TrimSpace(res.Reply) == "" { return fmt.Sprintf("agent %q finished without an answer after %d round(s)", agent, res.Rounds), true, nil } diff --git a/tool/delegate_test.go b/tool/delegate_test.go new file mode 100644 index 0000000..e0bd226 --- /dev/null +++ b/tool/delegate_test.go @@ -0,0 +1,105 @@ +package tool + +import ( + "context" + "errors" + "strings" + "testing" + "time" +) + +// fakeDelegator answers from a fixed table and records what Run was asked for. +type fakeDelegator struct { + agents map[string]AgentInfo + ran []string + res DelegateResult + err error +} + +func (f *fakeDelegator) AgentNames() []string { + names := make([]string, 0, len(f.agents)) + for n := range f.agents { + names = append(names, n) + } + return names +} +func (f *fakeDelegator) Agent(name string) (AgentInfo, bool) { + info, ok := f.agents[name] + return info, ok +} +func (f *fakeDelegator) Run(_ context.Context, spec DelegateSpec) (DelegateResult, error) { + f.ran = append(f.ran, spec.Agent) + return f.res, f.err +} + +// The parallel classification and the execution must resolve "which agent" the +// same way. They did not: one read the raw argument and the other trimmed it, +// so a call could be admitted to a batch as a read-only agent and then run as +// a write-capable one. +func TestDelegateResolvesTheAgentOnceForBothPaths(t *testing.T) { + d := &fakeDelegator{agents: map[string]AgentInfo{ + "reader": {ReadOnly: true}, + "writer": {ReadOnly: false}, + }, res: DelegateResult{Reply: "done", Rounds: 1}} + tl := &delegateTool{d: d, names: []string{"reader", "writer"}} + + for _, spelling := range []string{"writer", " writer", "writer ", " writer "} { + args := map[string]any{"agent": spelling, "task": "t"} + if tl.SupportsParallel(args) { + t.Errorf("agent %q classified as parallel-safe", spelling) + } + d.ran = nil + if _, isErr, _ := tl.Call(context.Background(), args); isErr { + t.Errorf("agent %q was rejected by Call but accepted by the classifier", spelling) + } + if len(d.ran) != 1 || d.ran[0] != "writer" { + t.Errorf("agent %q ran %v, want the write-capable agent", spelling, d.ran) + } + } + // And the read-only spelling stays parallel-safe with the same padding. + if !tl.SupportsParallel(map[string]any{"agent": " reader "}) { + t.Error("a padded read-only agent must still be parallel-safe") + } +} + +// A failed child is billed for the rounds it completed, and it is the run +// worth investigating — the accounting must not be the thing that goes +// missing exactly when the cost is surprising. +func TestDelegateReportsCostOfAFailedChild(t *testing.T) { + d := &fakeDelegator{ + agents: map[string]AgentInfo{"a": {}}, + res: DelegateResult{Rounds: 3, Duration: 2 * time.Second}, + err: errors.New("upstream exploded"), + } + tl := &delegateTool{d: d, names: []string{"a"}} + + ctx, collect := WithArtifact(context.Background()) + text, isErr, err := tl.Call(ctx, map[string]any{"agent": "a", "task": "t"}) + if err != nil { + t.Fatalf("a child's failure must be the parent's result, not its error: %v", err) + } + if !isErr || !strings.Contains(text, "upstream exploded") { + t.Errorf("Call = (%q, %v), want the failure as an error result", text, isErr) + } + art := collect() + if art == nil || art.Kind != "note" { + t.Fatalf("a failed delegation posted no accounting: %+v", art) + } + if joined := strings.Join(art.Lines, " "); !strings.Contains(joined, "3 rounds") { + t.Errorf("accounting = %q, want the rounds that were billed", joined) + } +} + +// A child that never reached the provider has nothing to account for, and a +// "0 rounds · 0 tokens" row would be noise dressed as information. +func TestDelegateSkipsAccountingWhenNothingRan(t *testing.T) { + d := &fakeDelegator{agents: map[string]AgentInfo{"a": {}}, err: errors.New("no api key")} + tl := &delegateTool{d: d, names: []string{"a"}} + ctx, collect := WithArtifact(context.Background()) + if _, isErr, _ := tl.Call(ctx, map[string]any{"agent": "a", "task": "t"}); !isErr { + t.Error("a build failure must be an error result") + } + if art := collect(); art != nil { + t.Errorf("posted accounting for a child that never ran: %+v", art) + } +} From 62ee811e6dbf4a215ac688b5d536613326c31722 Mon Sep 17 00:00:00 2001 From: joyqi Date: Sun, 23 Aug 2026 14:20:13 +0800 Subject: [PATCH 10/11] chat: --max-turns bounds the run, not just the parent loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --max-turns capped the parent's loop and nothing else, so a delegated child ran an uncapped loop of its own — in -m, on a background context, with no ESC and nobody watching. The flag exists for exactly that unattended case, and delegation moved the spending to the one place it could not see. The number the user wrote had no relationship to what the run could cost. It is now a budget for the run: one pool, published to the context, drawn on by the parent and by every child. That is what "limit this run" means, and it is the shape Claude Code settled on for the same problem — a budget owned by the run and decremented by every agent in it. Verified against a mock where both sides loop forever: --max-turns 6 spends 1 parent round and 5 child rounds, --max-turns 3 spends 1 and 2. Before, the parent stopped at the cap and the child never stopped at all. No number is invented. Without the flag there is no budget, because a cap nobody chose is either too low to be safe or too high to be a cap — which is why Claude Code ships maxBudgetUsd with no default and no timeout at all. Interactive runs stay uncapped for the same reason they always were: the user is the brake, and ESC reaches a child's every round. tools.delegate.max_turns stays as an optional per-child cap beside it. The two are different questions — what this run may spend, versus what this particular agent should ever need — and neither is a guard against a WEDGED child, which is measured in wall-clock and left for a change that has a reason to pick a number. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- README.md | 9 +++- chat/chat.go | 14 +++++- chat/delegate.go | 6 ++- chat/turns.go | 80 +++++++++++++++++++++++++++++++++ chat/turns_test.go | 109 +++++++++++++++++++++++++++++++++++++++++++++ cmd/delegate.go | 22 ++++----- cmd/root.go | 2 +- 7 files changed, 227 insertions(+), 15 deletions(-) create mode 100644 chat/turns.go create mode 100644 chat/turns_test.go diff --git a/README.md b/README.md index 0feddf3..e8953ce 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ chatchain [openai|anthropic|gemini|vertexai|openresponses] [flags] | `--mcp` | | MCP server (command string or URL, repeatable) | | `--resume` | | Resume a saved session (`--resume` to pick interactively, `--resume=` for a specific one) | | `--no-save` | | Start ephemeral — nothing touches disk unless `/save` is run | -| `--max-turns` | | Limit agentic tool turns in non-interactive mode (`-m` only; 0 = unlimited) | +| `--max-turns` | | Limit agentic tool turns for the whole run, delegated children included (`-m` only; 0 = unlimited) | | `--output-format` | | `-m` output: `text` (default, the reply alone) or `json` (one result object with per-round token usage) | | `--context-window` | | Context window size for compaction accounting (e.g. `200k`, `1m`; default 128k) | | `--agent` | | Enable agent mode (AGENTS.md overlay, skills, `load_skill`, project-scoped sessions) | @@ -369,7 +369,7 @@ prompt and sampling all come from the entry it names: ```yaml tools: delegate: - max_turns: 30 # optional; default unlimited, as ESC stops a child too + max_turns: 30 # optional per-child cap; default unlimited agents: search: fast-provider review: @@ -380,6 +380,11 @@ tools: `description` is the only field that is not already over there, and it is what the model chooses between agents on. +In `-m` runs `--max-turns` is a budget for the whole run: the parent and every +child it delegates to draw on one pool, so the number bounds what the run can +cost rather than what each agent can. Interactive runs have no cap — ESC +cancels a delegation the same as anything else. + Delegations to an agent whose tools cannot change state run **concurrently**; anything else runs one at a time. A child's write requests surface as an approval prompt in your terminal, labelled with the agent that asked diff --git a/chat/chat.go b/chat/chat.go index 08d8e52..d5c1f57 100644 --- a/chat/chat.go +++ b/chat/chat.go @@ -33,7 +33,12 @@ func FetchModels(ctx context.Context, p provider.Provider) ([]string, error) { // still travels either way, so the exit status keeps meaning what it did. func Once(ctx context.Context, p provider.Provider, message string, systemPrompt string, dispatch tool.Dispatcher, agent AgentOptions, maxTurns int, format OutputFormat, w io.Writer) error { rec := newRunRecorder() - reply, images, imageErrs, err := runOnce(ctx, p, message, systemPrompt, dispatch, agent, maxTurns, quietHost{rec: rec}) + // --max-turns is the RUN's budget, not the parent loop's: it is published + // to the context so a delegated child draws on the same pool, and the + // local per-loop cap is left off so the two cannot double-count. + budget := newTurnBudget(maxTurns) + ctx = withTurnBudget(ctx, budget) + reply, images, imageErrs, err := runOnce(ctx, p, message, systemPrompt, dispatch, agent, 0, quietHost{rec: rec, turns: budget}) if format == OutputJSON { if werr := writeReport(w, rec.report(p, reply, images, imageErrs, err)); werr != nil { @@ -272,9 +277,16 @@ var errToolRoundsExceeded = errors.New("tool loop reached the --max-turns limit // the rounds it did pay for. func executeWithTools(ctx context.Context, tp provider.ToolProvider, dispatch tool.Dispatcher, history *[]provider.Message, tools []provider.ToolDef, overlay string, maxTurns int, host quietHost) (string, string, error) { for rounds := 0; ; rounds++ { + // Two caps, and they are different things: maxTurns bounds THIS loop + // (a delegated agent's own tools.delegate.max_turns), while the + // budget is the run's, shared with every child. if maxTurns > 0 && rounds == maxTurns { return "", "", fmt.Errorf("%w (%d turns)", errToolRoundsExceeded, maxTurns) } + if !host.turns.take() { + return "", "", fmt.Errorf("%w (%d turns, shared by this run and everything it delegated)", + errToolRoundsExceeded, host.turns.cap()) + } if dispatch != nil && rounds > 0 { // The advertised set is LIVE: tools a search_tools round loaded // (and late-connecting MCP servers) must appear the very next diff --git a/chat/delegate.go b/chat/delegate.go index 983dc12..3be2c35 100644 --- a/chat/delegate.go +++ b/chat/delegate.go @@ -24,6 +24,10 @@ import ( // somewhere up the stack — a way to put an approval question to them. type quietHost struct { rec *runRecorder + // turns is the RUN's budget, shared with every delegated child. The + // recorder beside it is deliberately NOT shared: what a child cost is + // reported per child, while what the run may spend is one pool. + turns *turnBudget // approve, when set, forwards a state-changing call to whoever owns the // terminal. A -m run leaves it nil because there is nobody to ask; a // delegated child sets it because it has no user of its own but runs @@ -126,7 +130,7 @@ func (d *Delegator) Run(ctx context.Context, spec tool.DelegateSpec) (tool.Deleg } } - host := quietHost{rec: newRunRecorder()} + host := quietHost{rec: newRunRecorder(), turns: turnBudgetFrom(ctx)} if fn := d.approver(); fn != nil { // Only one child can be waiting on the user at a time — the terminal // is single-threaded even when the delegations are not. In practice diff --git a/chat/turns.go b/chat/turns.go new file mode 100644 index 0000000..3f3fa12 --- /dev/null +++ b/chat/turns.go @@ -0,0 +1,80 @@ +package chat + +import ( + "context" + "sync" +) + +// The run's turn budget: --max-turns, counted once for everything the run +// does rather than once per agent. +// +// The flag exists for the unattended case — it is -m only — and that is +// exactly where delegation moves the spending somewhere the flag could not +// see. A cap that bounded the parent to five rounds while each of its +// children ran an uncapped loop of its own was not a cap; the number the user +// wrote had no relationship to what the run could cost. +// +// A shared pool is what "limit this run" means, and it is the shape Claude +// Code settled on for the same problem: one budget owned by the run and +// decremented by every agent in it. No number is invented here — a run +// without --max-turns has no budget at all, because a cap nobody chose is +// either too low to be safe or too high to be a cap. + +type turnBudget struct { + mu sync.Mutex + remaining int + total int +} + +// newTurnBudget returns nil for "no cap", so the unlimited case costs nothing +// and needs no branch at the call site (a nil budget always grants). +func newTurnBudget(n int) *turnBudget { + if n <= 0 { + return nil + } + return &turnBudget{remaining: n, total: n} +} + +// take claims one round, reporting false once the run is spent. It is called +// from several goroutines at once: parallel delegations run concurrently, and +// each child's loop draws on this same pool. +func (b *turnBudget) take() bool { + if b == nil { + return true + } + b.mu.Lock() + defer b.mu.Unlock() + if b.remaining <= 0 { + return false + } + b.remaining-- + return true +} + +// cap reports the budget the run was given, for the error that announces it +// is gone. +func (b *turnBudget) cap() int { + if b == nil { + return 0 + } + return b.total +} + +// turnBudgetKey carries the run's budget to a delegated child. The context is +// the only thing that reaches it: the child is started by a tool, and a tool +// must not know what a turn budget is. +type turnBudgetKey struct{} + +func withTurnBudget(ctx context.Context, b *turnBudget) context.Context { + if b == nil { + return ctx + } + return context.WithValue(ctx, turnBudgetKey{}, b) +} + +// turnBudgetFrom recovers the run's budget. Absent — an interactive run, where +// the brake is the user — it returns nil and every round is granted. +func turnBudgetFrom(ctx context.Context) *turnBudget { + b, _ := ctx.Value(turnBudgetKey{}).(*turnBudget) + return b +} diff --git a/chat/turns_test.go b/chat/turns_test.go new file mode 100644 index 0000000..cffb069 --- /dev/null +++ b/chat/turns_test.go @@ -0,0 +1,109 @@ +package chat + +import ( + "context" + "errors" + "sync" + "testing" + + "chatchain/provider" +) + +func TestTurnBudgetUnlimitedWithoutAFlag(t *testing.T) { + // A cap nobody chose is not invented: no --max-turns means no budget. + for _, n := range []int{0, -1} { + if b := newTurnBudget(n); b != nil { + t.Errorf("newTurnBudget(%d) = %+v, want nil (unlimited)", n, b) + } + } + var nilBudget *turnBudget + for i := 0; i < 1000; i++ { + if !nilBudget.take() { + t.Fatal("a nil budget must grant every round") + } + } +} + +func TestTurnBudgetSpendsExactlyItsCap(t *testing.T) { + b := newTurnBudget(3) + for i := 0; i < 3; i++ { + if !b.take() { + t.Fatalf("round %d refused inside the cap", i+1) + } + } + if b.take() { + t.Error("the budget granted a fourth round") + } + if b.cap() != 3 { + t.Errorf("cap() = %d, want the number the user wrote", b.cap()) + } +} + +// Parallel delegations draw on this pool from several goroutines at once, so +// the count has to be exact under contention — a lost decrement is a cap that +// quietly overspends. +func TestTurnBudgetIsExactUnderContention(t *testing.T) { + const cap = 50 + b := newTurnBudget(cap) + var granted int64 + var mu sync.Mutex + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 100; j++ { + if b.take() { + mu.Lock() + granted++ + mu.Unlock() + } + } + }() + } + wg.Wait() + if granted != cap { + t.Errorf("granted %d rounds against a cap of %d", granted, cap) + } +} + +// The point of the change: the budget belongs to the RUN. Two loops sharing +// one pool stop at the total between them, not at the total each. +func TestTurnBudgetIsSharedAcrossLoops(t *testing.T) { + budget := newTurnBudget(5) + spend := func() int { + tp := &loopingToolProvider{} + history := []provider.Message{{Role: "user", Content: "go"}} + _, _, err := executeWithTools(context.Background(), tp, noopDispatcher{}, &history, + noopDispatcher{}.Tools(), "", 0, quietHost{rec: newRunRecorder(), turns: budget}) + if !errors.Is(err, errToolRoundsExceeded) { + t.Fatalf("loop ended with %v, want the budget to stop it", err) + } + return tp.calls + } + first := spend() + second := spend() + if first != 5 { + t.Errorf("first loop ran %d rounds, want the whole budget", first) + } + if second != 0 { + t.Errorf("second loop ran %d rounds after the pool was spent, want 0", second) + } +} + +// A child recovers the pool from the context it was called with; an +// interactive run publishes none and every round is granted. +func TestTurnBudgetTravelsByContext(t *testing.T) { + if got := turnBudgetFrom(context.Background()); got != nil { + t.Errorf("a context with no budget yielded %+v, want nil", got) + } + b := newTurnBudget(2) + ctx := withTurnBudget(context.Background(), b) + if turnBudgetFrom(ctx) != b { + t.Error("the budget did not survive the context") + } + // A nil budget must not put an unlimited marker in the context either. + if turnBudgetFrom(withTurnBudget(context.Background(), nil)) != nil { + t.Error("an absent budget was published as present") + } +} diff --git a/cmd/delegate.go b/cmd/delegate.go index 83fa2ab..4f6eb04 100644 --- a/cmd/delegate.go +++ b/cmd/delegate.go @@ -43,18 +43,20 @@ func (a *agentRef) UnmarshalYAML(n *yaml.Node) error { return n.Decode((*raw)(a)) } -// max_turns defaults to unlimited, like --max-turns and like the interactive -// loop, which states the reason at chat/run.go: the user is the brake. +// max_turns is an optional PER-CHILD cap and defaults to unlimited, like +// --max-turns and like the interactive loop, which states the reason at +// chat/run.go: the user is the brake. Interactively that holds for a child +// too — ESC cancels the turn and the context it cancels reaches the child's +// every round. // -// A child is no exception to that. ESC cancels the turn, and the context it -// cancels reaches the child's every round — so the brake works on a -// delegation exactly as it does on anything else. The earlier default of 30 -// rested on the child having nobody watching it, which is not true. +// What bounds an unattended run is --max-turns, which is a budget for the RUN +// (chat/turns.go): the parent and every child draw on one pool. This key sits +// beside it as "this particular agent should never need more than N", which +// is the user's own estimate rather than a number invented for them. // -// The key stays, because a cap the user chooses is different from one they -// were given. What it is NOT is a guard against a wedged child: that failure -// is measured in wall-clock, the way bash and pi's delegate extension measure -// it, and thirty cheap rounds cost nothing like three expensive ones. +// Neither is a guard against a WEDGED child. That failure is measured in +// wall-clock, and no turn count describes it — thirty cheap rounds cost +// nothing like three expensive ones. // buildDelegator resolves every configured agent up front — a name that does // not resolve is a startup error, not a surprise three tool calls into a diff --git a/cmd/root.go b/cmd/root.go index ec6cfce..fe57739 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -429,7 +429,7 @@ func init() { rootCmd.Flags().StringVar(&resumeID, "resume", "", "Resume a saved session: --resume to pick interactively, or --resume=") rootCmd.Flags().Lookup("resume").NoOptDefVal = " " // allow bare --resume (interactive picker) rootCmd.Flags().BoolVar(&noSave, "no-save", false, "Start ephemeral: nothing persists unless you run /save in the chat") - rootCmd.Flags().IntVar(&maxTurns, "max-turns", 0, "Limit agentic tool turns in non-interactive mode (-m only; 0 = unlimited)") + rootCmd.Flags().IntVar(&maxTurns, "max-turns", 0, "Limit agentic tool turns for the whole run, delegated children included (-m only; 0 = unlimited)") rootCmd.Flags().StringVar(&outputFormat, "output-format", "", "Non-interactive output: text (default, the reply alone) or json (one result object with token usage)") rootCmd.Flags().StringVar(&contextWindowFlag, "context-window", "", "Context window size for compaction accounting (e.g. 200k, 1m); default 128k") rootCmd.Flags().BoolVar(&agentFlag, "agent", false, "Enable agent mode (AGENTS.md system-prompt overlay)") From 302eb0ee51fa7830ab57f144bb97ffadd228c0ee Mon Sep 17 00:00:00 2001 From: joyqi Date: Sun, 23 Aug 2026 20:33:07 +0800 Subject: [PATCH 11/11] fix: three more defects a review found in this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The JSON report did not account for what a run delegated. --max-turns already charges the run for a child's rounds, so the two accountings contradicted each other: the same rounds belonged to the run when it came to bounding them and not when it came to reporting them. A caller running this binary as a child — the consumer chat/output.go names first — was billed for rounds the report never mentioned. Children now report through a run-level ledger, travelling by context the way the turn budget does, and land under `delegated`: "usage": { "rounds-worth for this agent" }, "delegated": { "rounds": 4, "usage": { … } } Kept beside the parent's own figures rather than folded into them. They answer different questions, and merging them produces the puzzle of two rounds costing four thousand tokens — which, measured on a mock, is exactly the shape of a delegating run: 2 rounds and 20 tokens of its own beside 4 rounds and 4000 delegated. A child's provider entry reached the wire unvalidated. The main session rejects a bad effort, temperature or top_p at startup; a delegated one accepted `effort: turbo` silently and failed with an API 400 partway through a conversation — the same failure the model: check three lines above exists to prevent, arriving by a different door. Image settings are the one part of a provider entry a child does not adopt, and that is now said out loud in the code and the README. It was neither applied nor documented, and an image a child generated would land on disk where the parent never learns of it. And the call header was a third reader of the agent argument, still taking it raw after the classifier and the executor were unified on the trimmed one. Display-only, but that commit's own comment said all readers have to share the rule, and there were three. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0169txuMy5yPuGc8ZovqGaGU --- README.md | 9 +++++- chat/chat.go | 5 +++ chat/delegate.go | 4 +++ chat/output.go | 38 +++++++++++++++------- chat/turns.go | 61 +++++++++++++++++++++++++++++++++++ chat/turns_test.go | 75 ++++++++++++++++++++++++++++++++++++++++++++ cmd/delegate.go | 21 +++++++++++++ cmd/delegate_test.go | 28 +++++++++++++++++ tool/delegate.go | 11 ++++--- 9 files changed, 235 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index e8953ce..ecb57a1 100644 --- a/README.md +++ b/README.md @@ -378,13 +378,20 @@ tools: ``` `description` is the only field that is not already over there, and it is -what the model chooses between agents on. +what the model chooses between agents on. Image settings (`image`, +`json_edits`, `aspect_ratio`, `image_size`, `negative_prompt`) are the one +part a child does not adopt — a delegation returns text, and an image a child +generated would land on disk where the parent never learns of it. In `-m` runs `--max-turns` is a budget for the whole run: the parent and every child it delegates to draw on one pool, so the number bounds what the run can cost rather than what each agent can. Interactive runs have no cap — ESC cancels a delegation the same as anything else. +`--output-format json` reports what the children cost under `delegated`, +beside the parent's own `usage` rather than inside it: one figure says what +this agent spent, the other what it spent by delegating. + Delegations to an agent whose tools cannot change state run **concurrently**; anything else runs one at a time. A child's write requests surface as an approval prompt in your terminal, labelled with the agent that asked diff --git a/chat/chat.go b/chat/chat.go index d5c1f57..f766044 100644 --- a/chat/chat.go +++ b/chat/chat.go @@ -38,6 +38,11 @@ func Once(ctx context.Context, p provider.Provider, message string, systemPrompt // local per-loop cap is left off so the two cannot double-count. budget := newTurnBudget(maxTurns) ctx = withTurnBudget(ctx, budget) + // What the run delegates is billed to the caller too, so the report has + // to state it. The ledger travels the same way the budget does: a child + // is started by a tool, and a tool must not know what either of these is. + rec.delegated = &delegationLedger{} + ctx = withDelegationLedger(ctx, rec.delegated) reply, images, imageErrs, err := runOnce(ctx, p, message, systemPrompt, dispatch, agent, 0, quietHost{rec: rec, turns: budget}) if format == OutputJSON { diff --git a/chat/delegate.go b/chat/delegate.go index 3be2c35..96cf34e 100644 --- a/chat/delegate.go +++ b/chat/delegate.go @@ -154,5 +154,9 @@ func (d *Delegator) Run(ctx context.Context, spec tool.DelegateSpec) (tool.Deleg Usage: host.rec.usage(), Duration: time.Since(started), } + // The run's own report has to account for this: --max-turns already + // charges the run for a child's rounds, and the tokens they cost belong + // to the same run. A failed child counts — its rounds were billed. + delegationLedgerFrom(ctx).add(res.Rounds, res.Usage) return res, err } diff --git a/chat/output.go b/chat/output.go index 336277a..c4d834d 100644 --- a/chat/output.go +++ b/chat/output.go @@ -100,17 +100,29 @@ type RoundReport struct { // reading a mixed stream can branch on it the same way it will when the // streaming format arrives. type RunReport struct { - Type string `json:"type"` // always "result" - Provider string `json:"provider"` - Model string `json:"model"` - Reply string `json:"reply"` - Error string `json:"error,omitempty"` - Rounds int `json:"rounds"` - DurationMS int64 `json:"duration_ms"` - Usage TokenUsage `json:"usage"` - RoundUsage []RoundReport `json:"round_usage,omitempty"` - Images []string `json:"images,omitempty"` - ImageErrors []string `json:"image_errors,omitempty"` + Type string `json:"type"` // always "result" + Provider string `json:"provider"` + Model string `json:"model"` + Reply string `json:"reply"` + Error string `json:"error,omitempty"` + Rounds int `json:"rounds"` + DurationMS int64 `json:"duration_ms"` + Usage TokenUsage `json:"usage"` + // Delegated is what this run's child agents cost, kept beside Usage + // rather than inside it: one says what this agent spent, the other what + // it spent by delegating. Absent when nothing was delegated. + Delegated *DelegatedReport `json:"delegated,omitempty"` + RoundUsage []RoundReport `json:"round_usage,omitempty"` + Images []string `json:"images,omitempty"` + ImageErrors []string `json:"image_errors,omitempty"` +} + +// DelegatedReport is the run's delegation total: how many rounds its children +// ran, and what they cost. The per-child figures reach the terminal through +// the artifact channel; this is the machine-readable aggregate. +type DelegatedReport struct { + Rounds int `json:"rounds"` + Usage TokenUsage `json:"usage"` } // runRecorder accumulates what the tool loop learns as it runs. The loop @@ -120,6 +132,9 @@ type runRecorder struct { started time.Time rounds []RoundReport total TokenUsage + // delegated is the run's shared ledger, filled by children rather than + // by this loop. nil where delegation cannot happen. + delegated *delegationLedger } func newRunRecorder() *runRecorder { return &runRecorder{started: time.Now()} } @@ -164,6 +179,7 @@ func (r *runRecorder) report(p provider.Provider, reply string, images, imageErr Rounds: len(r.rounds), DurationMS: time.Since(r.started).Milliseconds(), Usage: r.total, + Delegated: r.delegated.report(), RoundUsage: r.rounds, Images: images, ImageErrors: imageErrs, diff --git a/chat/turns.go b/chat/turns.go index 3f3fa12..52d449d 100644 --- a/chat/turns.go +++ b/chat/turns.go @@ -3,6 +3,8 @@ package chat import ( "context" "sync" + + "chatchain/provider" ) // The run's turn budget: --max-turns, counted once for everything the run @@ -78,3 +80,62 @@ func turnBudgetFrom(ctx context.Context) *turnBudget { b, _ := ctx.Value(turnBudgetKey{}).(*turnBudget) return b } + +// The run's delegation ledger: what everything this run delegated to cost, +// aggregated so the report can state it. +// +// It exists because the two accountings disagreed. turnBudget already counts +// a child's rounds against --max-turns — the run pays for them — while the +// JSON report counted only the parent's own API calls. A caller running this +// binary as a child was billed for rounds the report did not mention, and +// that caller is the consumer chat/output.go names first. +// +// The total stays SEPARATE from the parent's own usage rather than folded +// into it. They answer different questions — what this agent spent, and what +// it spent by delegating — and merging them produces the puzzle of two rounds +// costing twenty thousand tokens. +type delegationLedger struct { + mu sync.Mutex + rounds int + usage TokenUsage +} + +// add records one finished child. Called from parallel delegations, so it +// locks; a failed child counts too, because its rounds were billed. +func (l *delegationLedger) add(rounds int, u provider.Usage) { + if l == nil || rounds == 0 { + return + } + l.mu.Lock() + defer l.mu.Unlock() + l.rounds += rounds + l.usage.add(u) +} + +// report returns nil when nothing was delegated, so a run that delegated +// nothing carries no empty section. +func (l *delegationLedger) report() *DelegatedReport { + if l == nil { + return nil + } + l.mu.Lock() + defer l.mu.Unlock() + if l.rounds == 0 { + return nil + } + return &DelegatedReport{Rounds: l.rounds, Usage: l.usage} +} + +type delegationLedgerKey struct{} + +func withDelegationLedger(ctx context.Context, l *delegationLedger) context.Context { + if l == nil { + return ctx + } + return context.WithValue(ctx, delegationLedgerKey{}, l) +} + +func delegationLedgerFrom(ctx context.Context) *delegationLedger { + l, _ := ctx.Value(delegationLedgerKey{}).(*delegationLedger) + return l +} diff --git a/chat/turns_test.go b/chat/turns_test.go index cffb069..f9af25e 100644 --- a/chat/turns_test.go +++ b/chat/turns_test.go @@ -107,3 +107,78 @@ func TestTurnBudgetTravelsByContext(t *testing.T) { t.Error("an absent budget was published as present") } } + +// --max-turns already charges the run for a child's rounds; the report has to +// charge it for their tokens. A caller running this binary as a child was +// billed for rounds the report never mentioned. +func TestDelegationLedgerAggregates(t *testing.T) { + var l *delegationLedger + if l.report() != nil { + t.Error("a nil ledger reported something") + } + l = &delegationLedger{} + if l.report() != nil { + t.Error("a run that delegated nothing carries an empty section") + } + l.add(3, provider.Usage{Input: 900, Output: 100, Total: 1000}) + l.add(2, provider.Usage{Input: 400, Output: 50, Total: 450}) + rep := l.report() + if rep == nil || rep.Rounds != 5 { + t.Fatalf("report = %+v, want 5 rounds", rep) + } + if rep.Usage.TotalTokens != 1450 || rep.Usage.InputTokens != 1300 { + t.Errorf("usage = %+v, want the sum of both children", rep.Usage) + } + // A child that never reached the provider adds nothing. + l.add(0, provider.Usage{Input: 999}) + if l.report().Rounds != 5 || l.report().Usage.InputTokens != 1300 { + t.Error("a child with no rounds moved the totals") + } +} + +// Parallel delegations finish on their own goroutines and land here at once. +func TestDelegationLedgerIsExactUnderContention(t *testing.T) { + l := &delegationLedger{} + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 50; j++ { + l.add(1, provider.Usage{Input: 2, Total: 3}) + } + }() + } + wg.Wait() + rep := l.report() + if rep.Rounds != 400 || rep.Usage.TotalTokens != 1200 || rep.Usage.InputTokens != 800 { + t.Errorf("ledger = %+v after 400 concurrent adds", rep) + } +} + +// The delegated total stays out of the parent's own figures: one says what +// this agent spent, the other what it spent by delegating. +func TestReportKeepsDelegatedSeparate(t *testing.T) { + rec := newRunRecorder() + rec.total.add(provider.Usage{Input: 9, Output: 1, Total: 10}) + rec.rounds = []RoundReport{{Round: 1}} + rec.delegated = &delegationLedger{} + rec.delegated.add(4, provider.Usage{Input: 3600, Output: 400, Total: 4000}) + + rep := rec.report(&reportingProvider{}, "done", nil, nil, nil) + if rep.Usage.TotalTokens != 10 { + t.Errorf("own usage = %d, want the parent's own calls only", rep.Usage.TotalTokens) + } + if rep.Rounds != 1 { + t.Errorf("own rounds = %d, want the parent's own rounds only", rep.Rounds) + } + if rep.Delegated == nil || rep.Delegated.Rounds != 4 || rep.Delegated.Usage.TotalTokens != 4000 { + t.Errorf("delegated = %+v, want the children's total", rep.Delegated) + } + // And it is omitted entirely when nothing was delegated. + bare := newRunRecorder() + bare.delegated = &delegationLedger{} + if bare.report(&reportingProvider{}, "x", nil, nil, nil).Delegated != nil { + t.Error("a run that delegated nothing carries a delegated section") + } +} diff --git a/cmd/delegate.go b/cmd/delegate.go index 4f6eb04..54688d7 100644 --- a/cmd/delegate.go +++ b/cmd/delegate.go @@ -101,6 +101,20 @@ func buildDelegator(cfg *config.Config, node yaml.Node, hc httpClientSource, roo if pc.Model == "" { return nil, fmt.Errorf("agent %q: provider %q has no `model:` (a delegated agent cannot be asked to pick one)", name, ref.Provider) } + // The same reasoning covers every other value that only the API can + // reject. The main session validates these at startup; a child's + // entry reached the wire unchecked, so `effort: turbo` was a config + // typo that surfaced as a 400 partway through a conversation — the + // exact failure the check above exists to prevent. + if pc.Effort != "" && !provider.ValidEffort(pc.Effort) { + return nil, fmt.Errorf("agent %q: provider %q has effort %q: want low|medium|high|xhigh|max", name, ref.Provider, pc.Effort) + } + if pc.Temperature != nil && (*pc.Temperature < 0 || *pc.Temperature > 2) { + return nil, fmt.Errorf("agent %q: provider %q has temperature %v: want 0.0-2.0", name, ref.Provider, *pc.Temperature) + } + if pc.TopP != nil && (*pc.TopP < 0 || *pc.TopP > 1) { + return nil, fmt.Errorf("agent %q: provider %q has top_p %v: want 0.0-1.0", name, ref.Provider, *pc.TopP) + } tools := childTools(pc.Tools) // The toolset is built here so its access can be reported to the // model and, more importantly, so the parallel decision rests on what @@ -145,6 +159,13 @@ func buildDelegator(cfg *config.Config, node yaml.Node, hc httpClientSource, roo tun.SetTopP(r.pc.TopP) } } + // Image settings (image, json_edits, aspect_ratio, image_size, + // negative_prompt) are deliberately NOT applied: a delegation returns + // text, and a child that generated an image would write it to disk + // where the parent never learns of it. This is the one part of a + // provider entry a child does not adopt, and it is documented rather + // than rejected — a provider used for images elsewhere should not have + // to be duplicated to be delegated to. sys, err := r.pc.ResolveSystem() if err != nil { return chat.Child{}, fmt.Errorf("agent %q: %w", name, err) diff --git a/cmd/delegate_test.go b/cmd/delegate_test.go index 6e18534..adfe981 100644 --- a/cmd/delegate_test.go +++ b/cmd/delegate_test.go @@ -181,3 +181,31 @@ providers: t.Errorf("the error should name the agent: %v", err) } } + +// A child's provider entry reaches the wire unchecked unless it is checked +// here. The main session validates these at startup; without the same pass a +// typo like `effort: turbo` was a config mistake that surfaced as an API 400 +// partway through a conversation — the failure the model: check exists to +// prevent, arriving by a different door. +func TestDelegateValidatesTheChildsProviderEntry(t *testing.T) { + for _, tc := range []struct{ name, entry, want string }{ + {"effort", "{type: openai, key: k, model: m, effort: turbo}", "effort"}, + {"temperature", "{type: openai, key: k, model: m, temperature: 3.5}", "temperature"}, + {"top_p", "{type: openai, key: k, model: m, top_p: 2}", "top_p"}, + } { + cfg := loadConfig(t, "providers:\n bad: "+tc.entry+"\n") + _, err := buildDelegator(cfg, agentsNode(t, "agents:\n a: bad\n"), nopHTTP{}, t.TempDir()) + if err == nil { + t.Errorf("%s: an invalid value passed startup", tc.name) + continue + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("%s: error should name the field: %v", tc.name, err) + } + } + // Valid values still build. + cfg := loadConfig(t, "providers:\n ok: {type: openai, key: k, model: m, effort: high, temperature: 0.7, top_p: 0.9}\n") + if _, err := buildDelegator(cfg, agentsNode(t, "agents:\n a: ok\n"), nopHTTP{}, t.TempDir()); err != nil { + t.Errorf("a valid entry was rejected: %v", err) + } +} diff --git a/tool/delegate.go b/tool/delegate.go index c30e2fe..840e771 100644 --- a/tool/delegate.go +++ b/tool/delegate.go @@ -69,10 +69,11 @@ func (t *delegateTool) SupportsParallel(args map[string]any) bool { return ok && info.ReadOnly } -// agentArg reads the agent name the way BOTH the parallel classification and -// the execution must read it. They resolved it differently once — one raw, -// one trimmed — which let a call be admitted to a batch as one agent and then -// run as another. Whatever the rule is, the two have to share it. +// agentArg reads the agent name the way every reader of it must. There are +// three — the parallel classification, the execution, and the call header — +// and they resolved it differently once, which let a call be admitted to a +// batch as one agent and then run as another. Whatever the rule is, all of +// them have to share it. func agentArg(args map[string]any) string { return strings.TrimSpace(stringArg(args, "agent")) } @@ -82,7 +83,7 @@ func agentArg(args map[string]any) string { // thing that distinguishes two delegations to one agent, and the agent name // is what says how much the call is about to cost. func (t *delegateTool) HeaderSummary(args map[string]any) string { - agent, _ := args["agent"].(string) + agent := agentArg(args) // the same reading the classifier and the run use task, _ := args["task"].(string) task = headerCommand(strings.TrimSpace(task)) switch {