From b39f85266850a3f70d0a6a3b382f1e1330153c4b Mon Sep 17 00:00:00 2001 From: Franco Conturso Date: Fri, 13 Nov 2026 16:54:20 -0300 Subject: [PATCH] agents: the flow names the profile of each role; spawn refuses other profiles (#758, D294) --- cmd/contr/agents.go | 14 +++++++--- cmd/contr/agents_test.go | 8 ++++++ cmd/contr/mcp.go | 9 ++++++- cmd/contr/mcp_test.go | 16 +++++++++++- docs/DECISIONS.md | 1 + internal/agents/agents.go | 3 +++ internal/agents/flow.go | 50 ++++++++++++++++++++++++++++++++++++ internal/agents/flow_test.go | 44 +++++++++++++++++++++++++++++-- kit/README.md | 7 ++++- 9 files changed, 144 insertions(+), 8 deletions(-) diff --git a/cmd/contr/agents.go b/cmd/contr/agents.go index a9b587fc..3e46bbfb 100644 --- a/cmd/contr/agents.go +++ b/cmd/contr/agents.go @@ -23,14 +23,16 @@ import ( const agentsUsage = `Usage: contr agents ls [--all] [--json] list the workers contr agents spawn --name N --profile P [--issue I] [--worktree DIR] [--project P] - [--role R] [--review-pr PR] [--no-parent] [--json] + [--role R] [--review-pr PR] [--no-parent] [--json] [--any-profile] [--task-file F | [--] task…] start a worker; --task-file reads the task from a file; --review-pr makes a detached worktree at the PR's head (
-wt/review-) of the repository at --worktree (default: here) and starts - the worker there + the worker there; the profile must be + one the flow names for a role, unless + --any-profile contr agents send type text and press Enter contr agents interrupt press the stop key contr agents stop [--keep] end the worker's program and remove its @@ -150,6 +152,7 @@ func runAgents(args []string, stdout, stderr io.Writer, dial func(context.Contex keep = fs.Bool("keep", false, "keep the finished terminal") ended = fs.Bool("ended", false, "remove all finished workers started by this terminal") lines = fs.Int("lines", 0, "only the last lines") + anyProf = fs.Bool("any-profile", false, "spawn with a profile that the flow does not name") ) need := map[string]int{"ls": 0, "watch": 0, "spawn": 0, "send": 2, "interrupt": 1, "stop": 1, "rm": 0, "read": 1, "prompt": 1} n, ok := need[sub] @@ -163,7 +166,7 @@ func runAgents(args []string, stdout, stderr io.Writer, dial func(context.Contex } allowed := map[string][]string{ "ls": {"all", "json"}, - "spawn": {"name", "profile", "issue", "worktree", "project", "role", "no-parent", "json", "task-file", "review-pr"}, + "spawn": {"name", "profile", "issue", "worktree", "project", "role", "no-parent", "json", "task-file", "review-pr", "any-profile"}, "read": {"lines"}, "stop": {"keep"}, "rm": {"ended"}, @@ -226,6 +229,11 @@ func runAgents(args []string, stdout, stderr io.Writer, dial func(context.Contex if err != nil { return err } + if !*anyProf { + if err := cfg.CheckProfile(*profile); err != nil { + return err + } + } spec := agents.Spec{Name: *name, Profile: *profile, Role: *role, Issue: *issue, Worktree: *worktree, Project: *project, Task: task, Colors: storedColors()} if *reviewPR != 0 { if spec.Worktree, err = m.ReviewWorktree(ctx, cmp.Or(*worktree, "."), *reviewPR); err != nil { diff --git a/cmd/contr/agents_test.go b/cmd/contr/agents_test.go index 3a490ff7..fdc3bcc5 100644 --- a/cmd/contr/agents_test.go +++ b/cmd/contr/agents_test.go @@ -161,6 +161,14 @@ func TestAgentsSpawn(t *testing.T) { if code, _, errs := run(t, core, nil, "spawn", "--name", "x", "--profile", "nope"); code != 1 || !strings.Contains(errs, "no profile") { t.Errorf("unknown profile: %d %q", code, errs) } + // orchestrator is a profile but no worker role in the flow (#758). + core.params = nil + if code, _, errs := run(t, core, nil, "spawn", "--name", "x", "--profile", "orchestrator"); code != 1 || !strings.Contains(errs, "allowed: builder, planner, reviewer, debugger, explorer") || len(core.params) != 0 { + t.Errorf("profile not in the flow: %d %q %v", code, errs, core.params) + } + if code, _, errs := run(t, core, nil, "spawn", "--name", "x", "--profile", "orchestrator", "--any-profile"); code != 0 { + t.Errorf("--any-profile: %d %q", code, errs) + } } func TestAgentsSendInterruptStopRead(t *testing.T) { diff --git a/cmd/contr/mcp.go b/cmd/contr/mcp.go index 9ece641a..4dfd42e3 100644 --- a/cmd/contr/mcp.go +++ b/cmd/contr/mcp.go @@ -179,6 +179,7 @@ func agentTools(m *managers, parent string) []mcp.Tool { All bool `json:"all"` Keep bool `json:"keep"` ReviewPR int `json:"review_pr"` + AnyProf bool `json:"any_profile"` } run := func(f func(context.Context, *agents.Manager, args) (string, error)) func(context.Context, json.RawMessage) (string, error) { return func(ctx context.Context, raw json.RawMessage) (string, error) { @@ -198,12 +199,18 @@ func agentTools(m *managers, parent string) []mcp.Tool { Schema: schema([]string{"name", "profile"}, map[string]prop{ "name": str("Unique worker name: 1-32 of a-z 0-9 - _ (for example b-158)."), "profile": str("builder, planner, reviewer or debugger, or a profile from the profiles file."), "task": str("The self-contained brief: the first message. Give task or task_file."), "task_file": str("An absolute path to a file that holds the brief, instead of task."), "issue": str("Issue number."), "worktree": str("The worker's working folder (its git worktree)."), "role": str("Role label; the profile name by default."), "project": str("Project name; groups the worker with its project's questions."), - "review_pr": {"integer", "A pull request number: start the worker in a detached worktree at the PR's head,
-wt/review-, of the repository at worktree (default: the coordinator's folder)."}}), + "any_profile": {"boolean", "Allow a profile that the flow does not name for a role. Use only when the owner asks."}, + "review_pr": {"integer", "A pull request number: start the worker in a detached worktree at the PR's head,
-wt/review-, of the repository at worktree (default: the coordinator's folder)."}}), Run: run(func(ctx context.Context, mgr *agents.Manager, a args) (string, error) { mgr, err := m.spawnManager(mgr) if err != nil { return "", err } + if !a.AnyProf { + if err := mgr.Config().CheckProfile(a.Profile); err != nil { + return "", err + } + } task, err := spawnTask(a.Task, a.TaskFile) if err != nil { return "", err diff --git a/cmd/contr/mcp_test.go b/cmd/contr/mcp_test.go index 36fac413..420bdb7c 100644 --- a/cmd/contr/mcp_test.go +++ b/cmd/contr/mcp_test.go @@ -352,8 +352,22 @@ func TestMCPSpawnReadsProfilesEachTime(t *testing.T) { if err := os.WriteFile(file, []byte(`{"profiles":{"planner-high":{"argv":["claude","--effort","high"]}}}`), 0o600); err != nil { t.Fatal(err) } + // The flow does not name it yet: refused, with the allowed profiles. + if _, err := spawn(context.Background(), args); err == nil || !strings.Contains(err.Error(), "allowed: builder, planner, reviewer, debugger, explorer") { + t.Fatalf("profile not in the flow: %v", err) + } + if _, err := spawn(context.Background(), json.RawMessage(`{"name":"p","profile":"planner-high","task":"plan","any_profile":true}`)); err != nil { + t.Fatalf("any_profile: %v", err) + } + if err := os.WriteFile(file, []byte(`{"profiles":{"planner-high":{"argv":["claude","--effort","high"]}},"flow":{"profiles":{"planner":"planner-high"}}}`), 0o600); err != nil { + t.Fatal(err) + } if _, err := spawn(context.Background(), args); err != nil { - t.Fatalf("after the profile was added: %v", err) + t.Fatalf("after the flow named the profile: %v", err) + } + // Now the flow names planner-high for the planner, so planner is refused. + if _, err := spawn(context.Background(), json.RawMessage(`{"name":"q","profile":"planner","task":"plan"}`)); err == nil || !strings.Contains(err.Error(), "allowed: builder, planner-high,") { + t.Errorf("planner after the change: %v", err) } // A broken file fails the spawn with its error, not the other tools. if err := os.WriteFile(file, []byte(`{`), 0o600); err != nil { diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 861552ff..43b46675 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -268,3 +268,4 @@ replaces it. | D291 | 2026-11-11 | An empty project can start something and can be closed (#746; owner; docs/spec/ProjectRow.md). The menu of a project row has "New orchestrator" while the project has no orchestrator, "New terminal", a separator, "Close all agents" only while the project has agents, and "Close project". The two starts run in that project's folder (the main checkout, else the first worktree, else the stored folder), through the paths of the Projects `+` menu (`NewOrchestrator.StartIn`) and of New tab (`NewTerminal.Start`). "Hide project" is now "Close project": the behavior does not change (the project leaves the sidebar, a toast " closed" with Undo, nothing stops, Open projects brings it back). The projects modal says "Close" and "⌘⌫ close project" for the same action. A click on an empty project selects it; the empty terminal then shows "New orchestrator" and "New terminal ⌘T" for that project, and ⌘T opens there too. Before, "New orchestrator in " showed only for a stored project with no terminal at all; a project whose terminals held no agent got only a disabled "Close all agents". It replaces those parts of D219, D207, D171 and D174. | | D292 | 2026-11-11 | Chat links are ink with an underline (#747; ChatView.md, Claude's text and Chips). PR, issue, worker and file chips and Markdown links draw their text in `ink-88`, the ink of the text around them, with an underline, so the state icon is the one color and an answer has one kind of link. The worker chip has no dot: it shows the mark of its model family (D285) in `WorkerInk` of its state, the size of the Octicon; another program has no icon. The chip's family travels in its address (`worker.claude`). The underline is in the text's ink: MyGo's markdown has no decoration color for links, so a lower step (`ink-48`) is not possible without a MyGo change. The pre-pass wraps the text in ``; a bare URL is not wrapped (tags would end it) and shows its underline on hover only. Replaces the `content-link` of D275, D286 and D288 for text. | | D293 | 2026-11-12 | A coordinator picks up flow and model edits in agents.json during a session (#753, owner). The `contr hook prompt` and `contr hook post-tool` hooks hash the flow text and each profile's `--model` and `--effort`, and compare the hash with the last one that terminal saw (`flow-seen/` in the state folder). The session-start hook saves the hash, so the first prompt says nothing. After a change, only a terminal with the label `contr.orchestrating` or role orchestrator gets the context "contr: the owner changed the flow and models in agents.json; follow these from now on." with the flow and one line per profile. hc is asked for the role only after a change, so the hooks do no hc call in the usual case. Errors go to stderr and never block the hook. | Profiles are read at each spawn, but the coordinator only knew the flow from session start and compaction. | +| D294 | 2026-11-13 | The flow names the profile of each worker role (#758). `Flow.Profiles` is saved as `flow.profiles` in agents.json, with the keys builder, planner, reviewer, debugger and explorer. A missing key is the role's own name, so an old file loads as before. The flow text ends with one line "Profiles: builder: …, planner: …". `contr agents spawn` and the MCP `spawn` tool refuse a profile that the flow names for no role, and the error lists the allowed profiles; `--any-profile` and `any_profile: true` are the override. A profile that agents.json does not have still gets the "no profile" error. The orchestrator spawn from the app and `contr new-orchestrator` are not checked. | A coordinator spawned builders on another profile than the owner set, and nothing stopped it. | diff --git a/internal/agents/agents.go b/internal/agents/agents.go index c5246c6a..70964aea 100644 --- a/internal/agents/agents.go +++ b/internal/agents/agents.go @@ -755,6 +755,9 @@ func (m *Manager) Send(ctx context.Context, name, text string) error { return nil } +// Config is the config that m spawns with. +func (m *Manager) Config() Config { return m.cfg } + // WithConfig returns a copy of m on the same core with the profiles of cfg. // A relative kit path is made absolute, as in New. func (m *Manager) WithConfig(cfg Config) *Manager { diff --git a/internal/agents/flow.go b/internal/agents/flow.go index 939aa14d..b344a4a4 100644 --- a/internal/agents/flow.go +++ b/internal/agents/flow.go @@ -24,6 +24,51 @@ type Flow struct { Workers int `json:"workers"` // Merge is ask (the owner's OK each time) or auto (when CI and review pass). Merge string `json:"merge"` + // Profiles is the profile that each worker role spawns with (D294). + Profiles RoleProfiles `json:"profiles,omitzero"` +} + +// RoleProfiles names the profile of each worker role. An empty name is the +// role's own name, so an agents.json without this key loads as before. +type RoleProfiles struct { + Builder string `json:"builder,omitempty"` + Planner string `json:"planner,omitempty"` + Reviewer string `json:"reviewer,omitempty"` + Debugger string `json:"debugger,omitempty"` + Explorer string `json:"explorer,omitempty"` +} + +// Roles gives the worker roles and their profiles, in a fixed order. +func (r RoleProfiles) Roles() [][2]string { + or := func(v, role string) [2]string { + if v == "" { + v = role + } + return [2]string{role, v} + } + return [][2]string{ + or(r.Builder, "builder"), or(r.Planner, "planner"), or(r.Reviewer, "reviewer"), + or(r.Debugger, "debugger"), or(r.Explorer, "explorer"), + } +} + +// CheckProfile gives an error when the flow names profile for no worker +// role. The error names the profiles that the flow allows (#758). A profile +// that c does not have gives no error here: Spawn reports it. +func (c Config) CheckProfile(profile string) error { + if _, ok := c.Profiles[profile]; !ok { + return nil + } + var allowed []string + for _, rp := range c.Flow.Profiles.Roles() { + if rp[1] == profile { + return nil + } + if !slices.Contains(allowed, rp[1]) { + allowed = append(allowed, rp[1]) + } + } + return fmt.Errorf("agents: the flow names no role with profile %q; allowed: %s (override: --any-profile, or any_profile in the spawn tool)", profile, strings.Join(allowed, ", ")) } // DefaultFlow holds the rules of the kit's orchestrate playbook. @@ -75,12 +120,17 @@ func (f Flow) Lines() []string { "ask": "Merge: ask the owner each time.", "auto": "Merge: merge yourself when CI and review pass.", }[f.Merge] + var roles []string + for _, rp := range f.Profiles.Roles() { + roles = append(roles, rp[0]+": "+rp[1]) + } return []string{ plan, review, fmt.Sprintf("Debugger: spawn debugger after %d failed fix rounds.", f.DebuggerAfter), fmt.Sprintf("Workers at once: at most %d, reviewers included.", f.Workers), merge, + "Profiles: " + strings.Join(roles, ", ") + ". Spawn each role with its profile only.", } } diff --git a/internal/agents/flow_test.go b/internal/agents/flow_test.go index a31d8bc1..b25fe974 100644 --- a/internal/agents/flow_test.go +++ b/internal/agents/flow_test.go @@ -101,12 +101,12 @@ func TestSaveRoundTripKeepsUnknownKeysAndProfiles(t *testing.T) { func TestFlowText(t *testing.T) { got := DefaultFlow().Text() - for _, want := range []string{"The flow:\n", "- Plan first: when needed.", "- Review: code PRs.", "after 2 failed fix rounds", "at most 4", "- Merge: ask"} { + for _, want := range []string{"The flow:\n", "- Plan first: when needed.", "- Review: code PRs.", "after 2 failed fix rounds", "at most 4", "- Merge: ask", "- Profiles: builder: builder, planner: planner, reviewer: reviewer, debugger: debugger, explorer: explorer."} { if !strings.Contains(got, want) { t.Errorf("text lacks %q:\n%s", want, got) } } - if n := strings.Count(got, "\n- "); n != 5 { + if n := strings.Count(got, "\n- "); n != 6 { t.Errorf("%d rule lines", n) } } @@ -220,3 +220,43 @@ func TestLoadConfigOwnOrchestratorWins(t *testing.T) { t.Fatalf("%v %+v", err, cfg.Profiles["orchestrator"]) } } + +func TestFlowProfilesLoadAndCheck(t *testing.T) { + path := filepath.Join(t.TempDir(), "agents.json") + if err := os.WriteFile(path, []byte(`{"flow":{"plan":"never"}}`), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := LoadConfig(path) + if err != nil || cfg.Flow.Profiles != (RoleProfiles{}) { + t.Fatalf("old file: %v %+v", err, cfg.Flow.Profiles) + } + if err := cfg.CheckProfile("builder"); err != nil { + t.Errorf("builder: %v", err) + } + if err := os.WriteFile(path, []byte(`{"profiles":{"b2":{"argv":["claude"]}},"flow":{"profiles":{"builder":"b2"}}}`), 0o600); err != nil { + t.Fatal(err) + } + if cfg, err = LoadConfig(path); err != nil { + t.Fatal(err) + } + if err := cfg.CheckProfile("b2"); err != nil { + t.Errorf("b2: %v", err) + } + err = cfg.CheckProfile("builder") + if err == nil || !strings.Contains(err.Error(), "allowed: b2, planner, reviewer, debugger, explorer") { + t.Errorf("builder after the change: %v", err) + } + if !strings.Contains(cfg.Flow.Text(), "- Profiles: builder: b2, planner: planner,") { + t.Errorf("text:\n%s", cfg.Flow.Text()) + } + if err := cfg.CheckProfile("unknown"); err != nil { + t.Errorf("a profile c lacks is left to Spawn: %v", err) + } + if err := cfg.Save(path); err != nil { + t.Fatal(err) + } + data, _ := os.ReadFile(path) + if !strings.Contains(string(data), `"builder": "b2"`) { + t.Errorf("saved: %s", data) + } +} diff --git a/kit/README.md b/kit/README.md index 2ee712f1..42a6dc3e 100644 --- a/kit/README.md +++ b/kit/README.md @@ -32,7 +32,12 @@ The `--plugin-dir` sessions below still work with the plugin installed. agent: `claude --plugin-dir --agent contr: …`. A variant names its agent: `"planner-high": {"argv": ["claude", "--model", "opus", "--effort", "high"], "agent": "planner"}`. The `spawn` tool reads the - profiles file on each spawn, so a new profile works at once. + profiles file on each spawn, so a new profile works at once. To spawn a + worker with it, name it for its role in the flow: + `"flow": {"profiles": {"planner": "planner-high"}}`. A role with no name + uses the profile of the same name. Spawn refuses a profile that the flow + names for no role; `--any-profile` (`any_profile: true` in the tool) lets + it through (D294). To prepare new worktrees of a project, add its main checkout and a shell command: `"setup": {"~/code/app": "cp -R \"$CONTR_MAIN/.husky/_\" .husky/ && pnpm install --frozen-lockfile --offline"}`. Spawn runs it