diff --git a/cmd/codeaf/do.go b/cmd/codeaf/do.go index 334dfbb045..63fb6813ac 100644 --- a/cmd/codeaf/do.go +++ b/cmd/codeaf/do.go @@ -323,11 +323,18 @@ func runDo(args []string) error { completionReserve := flags.Int("completion-reserve", 0, "tokens every call keeps free for its answer and its reasoning "+ "(default "+strconv.Itoa(ctxbudget.DefaultCompletionReserveTokens)+")") + slotsRaw := flags.String("slots", "", + "how many workers may run at once for this run; 0 is no limit "+ + "(default: your task.parallel setting, which is no limit)") debug := flags.Bool("debug", false, debugFlagHelp()) if err := parseCommandFlags(flags, reorder(flags, args)); err != nil { return err } noteRenamedFlags(flags) + slots, err := parseSlots(*slotsRaw) + if err != nil { + return err + } // THE RUN ID IS MINTED AT THE DOOR, once per invocation and before anything // can make a call, so that every record this errand leaves names the same // run. The folder is announced on the way out and only when something was @@ -351,7 +358,7 @@ func runDo(args []string) error { task: task, run: run, database: *database, keep: *keep, workspace: *workspace, timeout: wall.wall, asJSON: *asJSON, yesSpend: *yesSpend, model: *model, planModel: *planModel, checkModel: *checkModel, - contextFill: *contextFill, completionReserve: *completionReserve, + contextFill: *contextFill, completionReserve: *completionReserve, slots: slots, stdout: os.Stdout, stderr: os.Stderr, }) } @@ -398,9 +405,11 @@ type doRequest struct { // a run to a price can say so, and a ceiling of nothing is a run that may // spend nothing: the limit stopped it before a worker did. costCap *float64 - // slots bounds how many run-engine workers run at once. Zero is the door's - // own default ([defaultRunSlots]); a test names one it can watch. - slots int + // slots bounds how many run-engine workers run at once. Nil is the + // person's own `task.parallel` setting, the same row the chat door reads, + // and a named 0 is no bound at all (the `--slots` flag); a test names one + // it can watch. + slots *int // newBeltCompleter scripts the run road's worker, the way newClient scripts // the legacy road's. Nil builds a real provider client per seat model, which // is what a live run does; a test hands back a [session.Completer] that @@ -3356,17 +3365,36 @@ func errandStatus(outcome headlessOutcome) error { // ── the run road ──────────────────────────────────────────────────────────── -// defaultRunSlots is how many run-engine workers `codeaf do` starts at once -// when no caller names a number. Four is the same width the resident's own -// dispatcher runs a job at, and it is a bound rather than a target: a brief -// that needs one worker uses one. -const defaultRunSlots = 4 +// slotsFor answers how many run-engine workers this errand may run at once, +// where 0 is no bound. A caller who named a figure gets it; anyone else gets +// the profile's `task.parallel`, which is the ONE row that answers this +// question for the chat door too (internal/session's task_run_belt.go reads +// the same setting) and is no limit out of the box. This door used to carry +// a constant of its own, four, beside a setting that promised no limit — two +// answers to one question, and the person who had set the row found `codeaf +// do` ignoring it. +func (r doRequest) slotsFor(profileDir string) int { + if r.slots != nil { + return *r.slots + } + return config.TaskParallelAt(profileDir) +} -func (r doRequest) slotsOrDefault() int { - if r.slots > 0 { - return r.slots +// parseSlots reads the `--slots` flag. Blank is the flag unset, which leaves +// the answer to the profile; anything else is a whole number of workers, and +// 0 is no bound. The flag is a string rather than an int so that an unset +// flag and a named 0 are two different things, which an int's zero value +// cannot say. +func parseSlots(raw string) (*int, error) { + raw = strings.TrimSpace(raw) + if raw == "" { + return nil, nil + } + n, err := strconv.Atoi(raw) + if err != nil || n < 0 { + return nil, fmt.Errorf("--slots wants a whole number of workers, 0 for no limit; got %q", raw) } - return defaultRunSlots + return &n, nil } // runErrand is `codeaf do` on the run engine: the same errand as the road above @@ -3440,7 +3468,7 @@ func runErrand(request doRequest, seats config.Seats) (headlessOutcome, error) { Workspace: workspace, Title: title, Brief: request.task, - Slots: request.slotsOrDefault(), + Slots: request.slotsFor(settings.ProfileDir), Limits: limits, Factory: runengine.CrewFactory(store, workspace, settings.ProfileDir, runengine.Seats{ Work: seats.Work.Model, diff --git a/cmd/codeaf/do_engine_test.go b/cmd/codeaf/do_engine_test.go index 1b900d550d..16cf436dcc 100644 --- a/cmd/codeaf/do_engine_test.go +++ b/cmd/codeaf/do_engine_test.go @@ -272,7 +272,7 @@ func TestDoOnTheRunEngineRootStoreFinishBeforeWorkerReturnNamesRootResult(t *tes var stdout, stderr strings.Builder err := doErrand(doRequest{ task: "write out.txt and say what you did", workspace: workspace, asJSON: true, - timeout: 60 * time.Second, slots: 1, stdout: &stdout, stderr: &stderr, + timeout: 60 * time.Second, slots: bound(1), stdout: &stdout, stderr: &stderr, newBeltCompleter: func(string) session.Completer { return seat }, }) if err != nil { @@ -313,7 +313,7 @@ func TestDoOnTheRunEngineCompletesABriefAndNamesTheRootResult(t *testing.T) { var stdout, stderr strings.Builder err := doErrand(doRequest{ task: "write out.txt and say what you did", workspace: workspace, asJSON: true, - timeout: 60 * time.Second, slots: 1, stdout: &stdout, stderr: &stderr, + timeout: 60 * time.Second, slots: bound(1), stdout: &stdout, stderr: &stderr, newBeltCompleter: func(string) session.Completer { return seat }, }) if err != nil { @@ -419,7 +419,7 @@ func TestDoOnTheRunEngineSeatsEveryLaunchOnTheDoorsModels(t *testing.T) { var stdout, stderr strings.Builder err = doErrand(doRequest{ task: "write out.txt and say what you did", workspace: workspace, asJSON: true, - timeout: 60 * time.Second, slots: 1, model: workModel, planModel: planModel, + timeout: 60 * time.Second, slots: bound(1), model: workModel, planModel: planModel, stdout: &stdout, stderr: &stderr, newBeltCompleter: newBelt, }) if err != nil { @@ -485,7 +485,7 @@ func TestDoOnTheRunEngineChecksALeafAndExitsZeroWhenItHolds(t *testing.T) { var stdout, stderr strings.Builder if err := doErrand(doRequest{ task: "write out.txt and say what you did", workspace: workspace, asJSON: true, - timeout: 60 * time.Second, slots: 1, stdout: &stdout, stderr: &stderr, + timeout: 60 * time.Second, slots: bound(1), stdout: &stdout, stderr: &stderr, newBeltCompleter: func(string) session.Completer { return seat }, }); err != nil { t.Fatalf("a run whose check held left with %v, want 0\nstdout:\n%s\nstderr:\n%s", @@ -525,7 +525,7 @@ func TestDoOnTheRunEngineChecksASelfFinishedRootAndExitsZeroWhenItHolds(t *testi var stdout, stderr strings.Builder if err := doErrand(doRequest{ task: "do the work alone and say what you did", workspace: workspace, asJSON: true, - timeout: 60 * time.Second, slots: 1, stdout: &stdout, stderr: &stderr, + timeout: 60 * time.Second, slots: bound(1), stdout: &stdout, stderr: &stderr, newBeltCompleter: func(string) session.Completer { return seat }, }); err != nil { t.Fatalf("a self-finished root whose check held left with %v, want 0\nstdout:\n%s\nstderr:\n%s", @@ -620,7 +620,7 @@ func TestDoOnTheRunEngineLeavesTheUsageLedgerToTheSession(t *testing.T) { var stdout, stderr strings.Builder if err := doErrand(doRequest{ task: "write out.txt and say what you did", workspace: workspace, asJSON: true, - timeout: 60 * time.Second, slots: 1, stdout: &stdout, stderr: &stderr, + timeout: 60 * time.Second, slots: bound(1), stdout: &stdout, stderr: &stderr, newBeltCompleter: func(string) session.Completer { return seat }, }); err != nil { t.Fatalf("errand: %v\n%s", err, stderr.String()) @@ -705,7 +705,7 @@ func TestDoOnTheRunEngineSeatsACheckOnTheCheckModel(t *testing.T) { var stdout, stderr strings.Builder err = doErrand(doRequest{ task: "write out.txt and say what you did", workspace: workspace, asJSON: true, - timeout: 60 * time.Second, slots: 1, + timeout: 60 * time.Second, slots: bound(1), model: workModel, planModel: planModel, checkModel: checkModel, stdout: &stdout, stderr: &stderr, newBeltCompleter: newBelt, }) @@ -796,7 +796,7 @@ func TestDoOnTheRunEngineSeatsAnUnpinnedCheckOnTheCrewsChecker(t *testing.T) { var stdout, stderr strings.Builder err = doErrand(doRequest{ task: "write out.txt and say what you did", workspace: workspace, asJSON: true, - timeout: 60 * time.Second, slots: 1, + timeout: 60 * time.Second, slots: bound(1), stdout: &stdout, stderr: &stderr, newBeltCompleter: newBelt, }) if err != nil { @@ -825,3 +825,6 @@ func TestDoOnTheRunEngineSeatsAnUnpinnedCheckOnTheCrewsChecker(t *testing.T) { t.Fatalf("the completer was never asked for the crew's careful row; the check was seated elsewhere (built %v)", models) } } + +// bound is a named slot count for a request, the way the flag would name one. +func bound(n int) *int { return &n } diff --git a/cmd/codeaf/do_slots_test.go b/cmd/codeaf/do_slots_test.go new file mode 100644 index 0000000000..e11f32e3f7 --- /dev/null +++ b/cmd/codeaf/do_slots_test.go @@ -0,0 +1,62 @@ +package main + +import ( + "testing" + + "github.com/Agent-Field/codeaf/internal/config" +) + +// ONE ROW ANSWERS HOW MANY WORKERS RUN AT ONCE, and `codeaf do` reads it the +// way the chat door does. The door used to carry a constant of its own, four, +// so a person who had set `task.parallel` found this command ignoring it and a +// person who had set nothing got a bound the setting said did not exist. +func TestCodeafDoRunsAsManyWorkersAsTaskParallelSays(t *testing.T) { + dir := t.TempDir() + if got := (doRequest{}).slotsFor(dir); got != config.DefaultTaskParallel { + t.Fatalf("an unset row resolves %d slots, want the setting's own default %d (no limit)", got, config.DefaultTaskParallel) + } + registry := config.NewSettings(config.SettingsOptions{ + ProfileDir: dir, + ModelValue: func(slot string) string { return slot + "/model" }, + SetModel: func(string, string) error { return nil }, + SplitPct: func() int { return 0 }, + }) + row, found := registry.Row(config.KeyTaskParallel) + if !found { + t.Fatalf("no settings row %q", config.KeyTaskParallel) + } + if err := row.Apply("3"); err != nil { + t.Fatal(err) + } + if got := (doRequest{}).slotsFor(dir); got != 3 { + t.Fatalf("a row of 3 resolves %d slots for codeaf do, want 3", got) + } + // A figure named on the command outranks the row, and a named 0 is no + // bound rather than "unset": the two are different requests. + if got := (doRequest{slots: bound(2)}).slotsFor(dir); got != 2 { + t.Fatalf("--slots 2 resolves %d, want 2 over a row of 3", got) + } + if got := (doRequest{slots: bound(0)}).slotsFor(dir); got != 0 { + t.Fatalf("--slots 0 resolves %d, want 0 (no bound) over a row of 3", got) + } +} + +// The flag is read as text so that a blank and a zero stay two different +// things; everything that is not a whole number of workers is refused with the +// value it was handed. +func TestTheSlotsFlagTellsBlankFromZeroAndRefusesTheRest(t *testing.T) { + if got, err := parseSlots(""); err != nil || got != nil { + t.Fatalf("blank parses (%v, %v), want (nil, nil): the flag was not given", got, err) + } + if got, err := parseSlots(" 0 "); err != nil || got == nil || *got != 0 { + t.Fatalf("0 parses (%v, %v), want a named zero", got, err) + } + if got, err := parseSlots("12"); err != nil || got == nil || *got != 12 { + t.Fatalf("12 parses (%v, %v), want 12", got, err) + } + for _, bad := range []string{"-1", "four", "1.5"} { + if _, err := parseSlots(bad); err == nil { + t.Fatalf("--slots %q was accepted, want a refusal", bad) + } + } +} diff --git a/cmd/codeaf/logs_test.go b/cmd/codeaf/logs_test.go index d66b56e993..304e8366f9 100644 --- a/cmd/codeaf/logs_test.go +++ b/cmd/codeaf/logs_test.go @@ -46,6 +46,16 @@ func TestMain(m *testing.M) { os.Setenv("CODEAF_MODEL_POOL_SUBMIT_URL", "http://127.0.0.1:1/v1/rows") } restore := isolateTestEnvironment() + // AND THIS BINARY'S SUITE IS THE OLDER BELT'S SUITE, for the reason + // internal/session's TestMain gives at length: these tests drive `codeaf do` + // and the task doors down the node road they were written against, and they + // said which road by saying nothing. The pin is unconditional and it is set + // AFTER the isolation above, which clears this variable when the binary was + // launched by a plan worker — a suite whose answer depends on what the + // person running it exported is the one thing a test may not be, and a pin + // that a later unset undoes is not a pin. A test that means the harness sets + // "bash" for itself and wins. + os.Setenv("CODEAF_TASK_BELT", "node") code := m.Run() restore() os.Exit(code) diff --git a/cmd/codeaf/runwire.go b/cmd/codeaf/runwire.go index ceafe1f44f..fcf3614373 100644 --- a/cmd/codeaf/runwire.go +++ b/cmd/codeaf/runwire.go @@ -5,6 +5,6 @@ package main // what puts the door's second road — a `/task` under the bash belt starting a // RUN rather than a node of the session's own tree — into this binary, so the // import is a blank one: nothing here calls the package, and the registration is -// its one effect. With CODEAF_TASK_BELT unset the door answers the road it always -// had, and this changes nothing a person sees. +// its one effect. With CODEAF_TASK_BELT naming the older belt the door answers +// the road it always had, and the import still changes nothing a person sees. import _ "github.com/Agent-Field/codeaf/internal/run" diff --git a/docs/changes/unreleased/1355-the-worker-harness-is-the-default-belt-again.md b/docs/changes/unreleased/1355-the-worker-harness-is-the-default-belt-again.md new file mode 100644 index 0000000000..e5b21da3fa --- /dev/null +++ b/docs/changes/unreleased/1355-the-worker-harness-is-the-default-belt-again.md @@ -0,0 +1,48 @@ +--- +kind: changed +title: the worker harness is the default belt again, and a run has no worker bound +pr: 1355 +surface: [engine, chat] +invalidates: + - "#1340 made the node belt the default again, with the exact word `bash` as the only way onto the harness. It is the other way round once more: unset `CODEAF_TASK_BELT` is the harness, and `node`, `legacy` and `off` are the words that reach the older engine. Anything else, a blank included, is the harness." + - "A task the chat put on the harness ran its parts ONE AT A TIME whatever `task.parallel` said, because the run engine read the setting's 0 as 1. It reads 0 as no bound now, which is what the setting has always promised." + - "`codeaf do` started four workers at once and read no setting. It reads `task.parallel` like the chat door, and `--slots ` names a figure for one run; there is no constant of four anywhere." +--- +#1340 sent the default back to the node belt on a comparison whose cells were +launched in two unequal waves, and its own entry said the quality gap was one +task. The comparison that answers it is a 32-cell shuffled single batch on +Spark: eight DeepSWE tasks, two replicates, both belts interleaved so they saw +the same load, one model on every seat, each cell graded by its fixture's own +suite with the official test patch applied. The harness passed 3 of 14 scored +cells at a median `$0.239` and 1010 seconds; the node belt passed 2 of 13 at +`$0.384` and 2924 seconds. Pooled over every scored cell of the week, 61 of +them, the two belts pass at the same rate and the harness is cheaper. The wall +gap is the node belt running its parts one after another. + +So the default is the harness, as #1335 had it, and every line #1340 changed +goes back: the predicate, the two `TestMain` pins that name the older engine's +suite once, the manual's three pages and the truth table in +`bashbelt_default_test.go`. + +**The bound is gone too.** `task.parallel` is 0 out of the box and its hint says +0 is no limit. The chat door handed that 0 to the run engine, and the engine's +supervisor read a count below one as one, so a conversation's harness task ran +one part at a time while the setting beside it promised no limit. `codeaf do` +never asked the setting and carried a constant of four. Both roads read the one +row now and a 0 is no bound; `codeaf do --slots ` names a figure for one +run, where `0` is no limit and blank is the setting. The manual says so under +*How many tasks run at once* and in the `codeaf do` flag table. + +**The tmux suite names its belt now.** Every scenario `start` launches says +`CODEAF_TASK_BELT=node`, the road it was written for, and the launcher drops the +runner's own value of the variable before the child starts, so a word exported +in a developer's shell cannot choose what the suite tests. Before this the suite +relied on the absence of a word, which is exactly the instrument fault that +would have had a benchmark comparing the harness to itself. One new subtest, +`TaskOnTheDefaultBelt`, launches with the variable absent and reads the run +road off the screen, which is the only test of the default there has ever been. + +The one piece of machinery that had to move for this: the supervisor's drain +reads every outstanding return before it waits for the worker goroutines. With +no bound there can be more workers out than the return channel is deep, and a +worker blocked on handing in its return never reaches the wait. diff --git a/docs/changes/unreleased/1359-run-row-opens-its-plan.md b/docs/changes/unreleased/1359-run-row-opens-its-plan.md new file mode 100644 index 0000000000..5b7ea3a207 --- /dev/null +++ b/docs/changes/unreleased/1359-run-row-opens-its-plan.md @@ -0,0 +1,15 @@ +--- +kind: fixed +title: a run's row on the tasks place is the store's task, wears its state and opens its page +pr: 1359 +surface: [chat, engine] +invalidates: + - "A `/task` on the run engine drew a row that wore `working` and whose `enter` opened an empty room. The row was the one the run's door publishes for work the graph holds no node for, and the tasks place had dropped the store's own row for it because the two share a title. The store's row is drawn now: it wears the word its store status maps to (`running`, `queued`, `done`, `incomplete`, `stopped`, `your call`) and `enter` opens the task's page — the work order, the notes, and the trajectory of every command its worker ran." + - "internal/session's TaskNotice carried no way to tell a row the store answers for from a node of the session's own tree, and internal/tui3's planRowShown said so in its own comment: THIS SURFACE CANNOT SEE THE STORE ID. TaskNotice.PlanTask now names the store task a run's row is, in planStoreID's spelling — the same one PlanTaskRow.ID carries and PlanTaskPage is asked for — set by the door that mints both halves, carried forward by publishRunRow, and kept in the checkpoint. The title match is left to the node road, where a plan-born node's store ids are not the row's number and the node is the half with a room behind it." +--- + +Found by a real-model tmux drive, not by a unit test: this shipped with +`internal/tui3` and `internal/session` green, because every piece worked and the +place drew the wrong one of two rows. The defect was already reachable under +`CODEAF_TASK_BELT=bash`; making the harness the default belt only promotes it +from opt-in to default, which is why it blocks that change. diff --git a/internal/config/settings.go b/internal/config/settings.go index 82d8b91857..7bc0e4ef97 100644 --- a/internal/config/settings.go +++ b/internal/config/settings.go @@ -1312,18 +1312,15 @@ var OperatorEnvPins = []string{ // shape, under `make demo-home`'s terms. A row offering to persist a // fixture would put a demo question in front of a person every morning. "CODEAF_QUESTION_DEMO", - // CODEAF_TASK_BELT builds a task worker on the bash belt instead of the - // shipped belt (internal/session's bashbelt.go, - // docs/design/bash-task-loop/DESIGN.md): the one `bash` tool plus the - // hands that cannot be a shell command, so both arms of the comparison - // run from one binary. It is plumbing for the reason CODEAF_SWARM and - // CODEAF_SPLITGATE are — it picks which belt an experiment runs, not - // something the product has an opinion about — and it shares their - // lifetime: it disappears when the experiment has won or lost, which is - // exactly the lifetime a persisted setting must not have. A row would - // also be wrong the way the exit-code hatch is: it would put every future - // task worker on an experiment's belt on a machine where the variable is - // nowhere in sight. Unset, every worker is where it was. + // CODEAF_TASK_BELT sends a task worker BACK to the older node belt + // (internal/session's bashbelt.go, docs/design/worker-harness/DESIGN.md). + // The bash belt is the shipped default, so the variable is an escape + // hatch rather than the way in: `node`, `legacy` and `off` are the only + // words that turn it off, and one binary still runs both roads. It stays + // plumbing rather than a settings row for the reason the exit-code hatch + // is — a persisted row would pin a machine to the older engine long after + // whoever set it had forgotten, and an escape hatch must be as easy to + // stop using as it was to start. It disappears when the older belt does. "CODEAF_TASK_BELT", // CODEAF_PLANDB_BIN names the binary a bash-belt worker's `plandb` shim // execs (internal/session's plandb_plan.go) when the running program is diff --git a/internal/e2e/tmux_test.go b/internal/e2e/tmux_test.go index 0b83372971..bdcaaa4cdf 100644 --- a/internal/e2e/tmux_test.go +++ b/internal/e2e/tmux_test.go @@ -212,7 +212,18 @@ func start(t *testing.T, name, home, ws string, cols, rows int, args ...string) // THE RIG IS HANDED THE KEY THE PRODUCT WOULD HAVE FOUND, whichever road it // came down: a key that lives only in the profile reaches the child through // the variable here, exactly as a key exported in the shell does. - r := startWithEnv(t, []string{config.APIKeyEnv + "=" + liveKey(t)}, + // + // AND IT NAMES ITS BELT. Every scenario that starts here was written against + // the node road and reads that road's words, and it said so by saying + // nothing while unset meant node. The default is the worker harness now, and + // a scenario that relied on the absence of a word would have moved to the + // other road with every assertion still green — the fault that would have + // had both belt benchmarks comparing the harness to itself. `node` is the + // word because it reaches the older engine on this binary and is simply not + // `bash` on an older one. A scenario that wants the harness says `bash` + // itself through [startWithEnv], and the one that tests the default says no + // word at all ([testTaskOnTheDefaultBelt]). + r := startWithEnv(t, []string{config.APIKeyEnv + "=" + liveKey(t), "CODEAF_TASK_BELT=node"}, name, home, ws, cols, rows, args...) r.skipSetup(t) return r @@ -322,7 +333,13 @@ func startWithEnv(t *testing.T, env []string, name, home, ws string, cols, rows // The pid LEADS the name: tmux falls back to prefix matching on -t, so a // sibling's `kill-session -t afe2e_a` would still reach `afe2e_a-`. name = fmt.Sprintf("p%d-%s", os.Getpid(), name) - command := []string{"env"} + // THE RUNNER'S OWN BELT WORD DOES NOT REACH THE CHILD. `env` without -i + // hands the child everything this process has, so a developer with + // CODEAF_TASK_BELT exported in their shell would be choosing which road + // every scenario tests. The variable is dropped first; an assignment in env + // follows the -u and wins, so a scenario that names a word still gets it, + // and a scenario that names none really runs with the variable absent. + command := []string{"env", "-u", "CODEAF_TASK_BELT"} command = append(command, env...) command = append(command, "CODEAF_HOME="+home, diff --git a/internal/e2e/tui_e2e_test.go b/internal/e2e/tui_e2e_test.go index d6ae37232f..b9e5cb396d 100644 --- a/internal/e2e/tui_e2e_test.go +++ b/internal/e2e/tui_e2e_test.go @@ -114,6 +114,7 @@ func TestTUIE2E(t *testing.T) { t.Run("a_refused_task_proposal_draws_no_schema_sentence", testRefusedTaskProposal) t.Run("space_in_the_task_room_pages_the_card", testTaskRoomKeepsSpace) t.Run("TaskOnTheRunEngine", testTaskOnTheRunEngine) + t.Run("TaskOnTheDefaultBelt", testTaskOnTheDefaultBelt) } // testPlainLaunchConnectionsAndHarnesses is the engine-road regression: the @@ -2232,15 +2233,17 @@ func waitForRecord(t *testing.T, home string, within time.Duration) string { // place that moves with the store's own state, and the page that row opens, // whose trajectory is the worker's record of every command it ran. // -// THE BELT IS ASKED FOR IN THE BINARY'S OWN ENVIRONMENT. CODEAF_TASK_BELT=bash -// is the one switch that makes the door take the run road at all (internal/run's -// engine is linked and registered for it, cmd/codeaf/runwire.go); with the -// variable unset the same `/task` starts an ordinary node of this session's -// tree, which the roster subtests already read. [startWithEnv] is how this suite -// hands a variable to the launched process. +// THE BELT IS NAMED IN THE BINARY'S OWN ENVIRONMENT. The harness is the +// default, and this subtest still says CODEAF_TASK_BELT=bash outright, for the +// reason [start] says `node`: a scenario that names its road keeps testing that +// road when the default moves. The default itself — that a `/task` with the +// variable absent takes this same road — is what [testTaskOnTheDefaultBelt] +// proves, on the same screens, with no word given. `node`, `legacy` and `off` +// are the words that send the same `/task` to an ordinary node of this +// session's tree instead, which the roster subtests read. // // IT COSTS A FEW CENTS AND LANDS IN ABOUT THIRTY SECONDS, the shape and the -// price [testStatesDone] pays for the same brief on the shipped belt. +// price [testStatesDone] pays for the same brief on the node belt. // // THE STATE WORD IS READ OFF THE ROW AND NOT OFF THE SCREEN. The place files its // rows under headings that are state words themselves — everything working stands @@ -2249,11 +2252,29 @@ func waitForRecord(t *testing.T, home string, within time.Duration) string { // care [statesHeadLine] takes on a landing card, spent on a row of the list // ([planRowWearing]). func testTaskOnTheRunEngine(t *testing.T) { + taskOnTheRunEngine(t, "afe2e_task_run", "CODEAF_TASK_BELT=bash") +} + +// testTaskOnTheDefaultBelt is the one scenario in this suite that launches the +// binary with NO belt word and asserts the road it takes. It is the only thing +// here that tests the default: every other scenario names its road, so the +// default could move without one of them noticing, and it did, twice, in +// #1335 and #1340. [startWithEnv] drops the runner's own variable before the +// child starts, so absent here means absent in the process and not merely +// unmentioned by the test. +func testTaskOnTheDefaultBelt(t *testing.T) { + taskOnTheRunEngine(t, "afe2e_task_default") +} + +// taskOnTheRunEngine is the body the two subtests above share: launch with the +// key and whatever belt words the caller names, put one `/task` on the run +// engine, and read the run off the tasks place, the plan page and the thread. +func taskOnTheRunEngine(t *testing.T, rigName string, beltWords ...string) { home := newHome(t, nil) ws := newWorkspace(t, "runws", false) r := startWithEnv(t, - []string{config.APIKeyEnv + "=" + liveKey(t), "CODEAF_TASK_BELT=bash"}, - "afe2e_task_run", home, ws, tuiWide, 45, "chat", "--one-model") + append([]string{config.APIKeyEnv + "=" + liveKey(t)}, beltWords...), + rigName, home, ws, tuiWide, 45, "chat", "--one-model") r.skipSetup(t) // runRowWord is the run's own words on the tasks place, and one word of the @@ -2275,9 +2296,8 @@ func testTaskOnTheRunEngine(t *testing.T) { // store saying a task is deliverable and a worker has it — both read as work // in flight, and a task whose root has landed reads done. openTasksPlace(t, r) - running := r.waitFor(40*time.Second, say(t, "planRunningWord")) + running := planRowWaitsToWear(t, r, 40*time.Second, runRowWord, say(t, "planRunningWord")) t.Logf("the run on the tasks place, while a worker holds its task:\n%s", running) - planRowWearing(t, running, runRowWord, say(t, "planRunningWord")) // ── the page mid-run: the live step at the live edge ──────────────────── // @@ -2288,22 +2308,40 @@ func testTaskOnTheRunEngine(t *testing.T) { // taskPlanBody). It is read here, before the root lands, because the live step // is gone the moment its command ends — the page after the landing is the // settled page the section below reads. + // + // AND THE CURSOR IS MOVED ONTO THE ROW FIRST ([tasksPlaceRunRow]). Enter on + // the conversation row is the door into the conversation and always was, so + // a press made without this one read the chat and said nothing about a plan + // page at all — and passed, because the mark it waits for is drawn on the + // conversation too. + tasksPlaceRunRow(t, r) r.keys("Enter") - live, sawLive := r.glimpse(20*time.Second, say(t, "planLiveGlyph")) - if !sawLive { - t.Fatalf("the plan page open on a running task never drew its live step (%q beside the "+ - "command):\n%s", say(t, "planLiveGlyph"), r.capture()) - } - if !strings.Contains(live, say(t, "planLiveClockWord")) { - t.Errorf("the plan page's live line has no clock under it (%q):\n%s", - say(t, "planLiveClockWord"), live) + // THE PAGE COMING UP IS THE ASSERTION AND THE LIVE STEP IS AN OBSERVATION, + // and the two are separated here because only one of them is a fact about + // the surface. That the press over the row opens the STORE'S page rather + // than a room is true every time, and the page's own note box says it is + // the page. Which command a worker happens to be part-way through when the + // key lands is a moment: this brief is four steps and about half a minute, + // so a page opened a second after the last one ends is an honest page of a + // task that has finished — [rig.glimpse]'s own bargain, which never fails + // on a thing that is only on screen while work runs. Asserting it made a + // red out of a fast run and said nothing about any defect. + page := r.waitFor(40*time.Second, say(t, "planNoteBoxWord")) + t.Logf("the page the press over the run's row opened:\n%s", page) + if live, sawLive := r.glimpse(15*time.Second, say(t, "planLiveGlyph")); sawLive { + if !strings.Contains(live, say(t, "planLiveClockWord")) { + t.Errorf("the plan page's live line has no clock under it (%q):\n%s", + say(t, "planLiveClockWord"), live) + } + t.Logf("the plan page mid-run, carrying the live step:\n%s", live) + } else { + t.Logf("the run finished before a live step could be caught on the page, which is this " + + "brief on a fast worker and not a defect") } - t.Logf("the plan page mid-run, carrying the live step:\n%s", live) r.keys("Escape") - done := r.waitFor(runPatience, say(t, "planDoneWord")) + done := planRowWaitsToWear(t, r, runPatience, runRowWord, say(t, "planDoneWord")) t.Logf("the run on the tasks place once its root landed:\n%s", done) - planRowWearing(t, done, runRowWord, say(t, "planDoneWord")) // ── the landing, in the thread ────────────────────────────────────────── // @@ -2328,20 +2366,29 @@ func testTaskOnTheRunEngine(t *testing.T) { // command the worker ran, and the run's own finish among them. `esc` backs out // one layer to the list, the card's own bargain. openTasksPlace(t, r) + tasksPlaceRunRow(t, r) r.keys("Enter") - // glimpse AND NOT waitFor, BECAUSE THE PAGE NOT COMING UP IS NOT A TIMEOUT. A - // wait that ran out would report a screen the suite never saw and leave the - // reader to work out which of two pages answered the key; the answer is a fact - // about the row that was under the cursor, and it is said as one. - page, saw := r.glimpse(40*time.Second, say(t, "planFinishCommand")) - if !saw { - t.Fatalf("Enter over the run's row never opened the store's plan page, so no screen this suite "+ - "can reach carries the %q line its worker finishes with. The row under the cursor is the "+ - "run's node row and not its plan row — planRowWearing says why — and a node row opens a "+ - "room, which the engine holds no node for, so it is empty. The screen after Enter was:\n%s", - say(t, "planFinishCommand"), r.capture()) - } - t.Logf("the plan page, carrying the worker's own finish command:\n%s", page) + // THE PAGE IS READ BY TWO OF ITS OWN WORDS, and neither is the model's. The + // note box stands on this page and on nothing else, and the head's figures + // are the store's count of the steps its worker took and what they cost — + // so the pair says the press opened THE STORE'S PAGE and not the room a + // record row opens, which is the whole of what this scenario came to prove. + // + // WHICH COMMANDS ARE ON IT IS THE WORKER'S BUSINESS. The finish is the + // worker's own `plandb done` when the worker writes one, and the RUN's when + // it does not (internal/run's worker.go), and a brief this small on a fast + // model is regularly the second — measured twice on this lane, where the + // page carried `echo`, `cat` and the run's own ending note. So the finish + // command is observed and logged, never waited out: asserting it made a red + // out of a model's choice and said nothing about the surface. + stored := r.waitFor(40*time.Second, say(t, "planNoteBoxWord"), say(t, "planStepsSpend")) + t.Logf("the page the run's row opens, with the store's own figures on it:\n%s", stored) + if finish, saw := r.glimpse(5*time.Second, say(t, "planFinishCommand")); saw { + t.Logf("and this worker wrote its own finish into the trajectory:\n%s", finish) + } else { + t.Logf("this worker left the ending to the run, so no %q step is on the page", + say(t, "planFinishCommand")) + } r.keys("Escape") back := r.waitFor(30*time.Second, say(t, "planDoneWord")) t.Logf("esc backed out of the page to the list:\n%s", back) @@ -2358,10 +2405,59 @@ func openTasksPlace(t *testing.T, r *rig) { time.Sleep(700 * time.Millisecond) r.keys("Enter") time.Sleep(700 * time.Millisecond) - // THE FOLD IS OPENED UNDER THE CURSOR. The place groups its rows by - // conversation and opens every group shut, so the run's row is not drawn until - // its conversation is unfolded. + // THE FOLD IS OPENED UNDER THE CURSOR, AND THE PRESS IS WAITED OUT. The place + // groups its rows by conversation and opens every group shut, so the run's row + // is not drawn until its conversation is unfolded — and a `→` that reached the + // program before the place was up is a key nothing answered, which left the + // page holding no row for the work at all. The foot says which way the fold + // is, so both halves of the gesture are read off the screen rather than slept + // through: measured on two runs of one binary, one opened and one did not. + r.waitFor(30*time.Second, say(t, "tasksFoldShutWord")) r.keys("Right") + r.waitFor(30*time.Second, say(t, "tasksFoldOpenWord")) +} + +// planRowWaitsToWear polls until the run's OWN ROW on the tasks place wears this +// state word, and answers the screen it was read on. +// +// IT IS A WAIT ON THE ROW AND NOT ON THE SCREEN, which is this scenario's own law +// ([planRowWearing]) spelled as a wait rather than only as an assertion. The +// place files its rows under headings that are state words themselves, so a +// screen-wide wait returns the instant a HEADING says `running` — which on a real +// screen can be before the store's own read has landed and before the fold has +// opened, and the assertion then reads whichever line happens to carry the title. +// Two runs of one binary split on exactly that: one read the row and passed, the +// next read the side list's line and failed. +func planRowWaitsToWear(t *testing.T, r *rig, within time.Duration, words, state string) string { + t.Helper() + deadline := time.Now().Add(within) + for { + screen := r.capture() + if tasksRowWearing(screen, words, state) != "" { + return screen + } + if time.Now().After(deadline) { + // THE RED IS THE ASSERTION'S OWN, so a row wearing the wrong word reads + // as what that means and not as a timeout. + planRowWearing(t, screen, words, state) + return screen + } + time.Sleep(250 * time.Millisecond) + } +} + +// tasksPlaceRunRow steps the cursor off the conversation group and onto the +// first row inside it, which is the run's own. +// +// THE PLACE SELECTS THE CONVERSATION FIRST and `enter` over that row opens the +// conversation ([app.openConversationRow]), which is not a mistake in the +// surface: a group row's door is the group. So a press over a piece of work is +// a press over the row, and the walk down onto it is part of the gesture — +// the same `↓` [testStatesDone] takes before it reads a task's own door. +func tasksPlaceRunRow(t *testing.T, r *rig) { + t.Helper() + r.keys("Down") + time.Sleep(400 * time.Millisecond) } // tasksRowOf is the one line of the tasks place carrying these words, or "" when @@ -2380,6 +2476,26 @@ func tasksRowOf(screen, words string) string { return "" } +// tasksRowWearing is the one line of the tasks place carrying these words AND +// this state word, or "" when no line carries both. +// +// IT IS BOTH WORDS ON ONE LINE AND NOT THE FIRST LINE WITH THE TITLE. At the +// width this suite runs the place draws the record pane beside the list, on the +// same rows, and the pane LEADS WITH THE SELECTED ROW'S OWN TITLE — so the first +// line carrying the work's name is the pane's heading, which wears no state at +// all. A search that stopped there read `⌕ type to filter … │ write HELLO.md …` +// off a screen whose row said `done · 4 steps · $0.03` two lines below, and +// reported the row as bare. +func tasksRowWearing(screen, words, state string) string { + for _, line := range strings.Split(screen, "\n") { + row := strings.TrimSpace(line) + if strings.Contains(row, words) && strings.Contains(row, state) { + return row + } + } + return "" +} + // planRowWearing asserts that the run's row is on the tasks place wearing this // state word, and answers the row for the log. // @@ -2395,6 +2511,9 @@ func tasksRowOf(screen, words string) string { // Enter over that row opens a room the engine holds no node for. func planRowWearing(t *testing.T, screen, words, state string) string { t.Helper() + if row := tasksRowWearing(screen, words, state); row != "" { + return row + } row := tasksRowOf(screen, words) if row == "" { t.Errorf("the tasks place draws no row for the run (%q), so nothing on it can wear %q:\n%s", @@ -2403,9 +2522,11 @@ func planRowWearing(t *testing.T, screen, words, state string) string { } if !strings.Contains(row, state) { t.Errorf("the run's row does not wear %q, so the row the place drew is not the store's plan "+ - "row: planRowShown (internal/tui3/taskplan.go) drops a plan row whose title a node row of "+ - "this conversation already wears, and the run's door publishes its own row with the store "+ - "root's title on it. The row drawn is the node's, in the engine's own word:\n\t%s", state, row) + "row. The run's door publishes a row for work the graph holds no node for and says which "+ + "store task it is (session's TaskNotice.PlanTask); the place takes those rows out by that "+ + "identity and draws the store's own (internal/tui3's planStoreDraws). A row wearing the "+ + "engine's `working` instead is the node half, which means the identity did not join — it is "+ + "dropped on the way, or the two ends spell the store id differently:\n\t%s", state, row) } return row } diff --git a/internal/e2e/tuiwords_test.go b/internal/e2e/tuiwords_test.go index 718cdbd54c..3cb708ddb8 100644 --- a/internal/e2e/tuiwords_test.go +++ b/internal/e2e/tuiwords_test.go @@ -573,6 +573,27 @@ var tuiWords = map[string]tuiWord{ "(questiondelivery.go's questionWaitingLine). It stands in the roster's foot where the " + "enter-door would be, so a task that landed `your call` and asked something is read here", }, + // ── the tasks place's own fold ─────────────────────────────────────────── + // + // The place groups its rows by conversation and draws every group SHUT, so a + // scenario that reads a piece of work inside one has to open it — and has to + // know the press landed. These two words are the two answers the foot gives, + // and they are the suite's only honest way to tell a page with nothing in it + // from a page whose fold has not opened yet. + "tasksFoldShutWord": { + screen: "→ what ran under it", + why: "the tasks place's foot over a SHUT conversation group (place_tasks.go's tasksOpenWord). " + + "It is waited for before the `→` is pressed, because a key that reached the program before " + + "the place was up is a key nothing answered — and the page then held no row for the work, " + + "which a screen-wide wait for a state word read straight past", + }, + "tasksFoldOpenWord": { + screen: "← fold it back up", + why: "the same foot once the group is OPEN (place_tasks.go's tasksShutWord), which is what says " + + "the `→` landed and the rows inside are drawn. Waiting on it rather than sleeping is what " + + "made the run-engine scenarios repeatable: two runs of one binary split on whether the fold " + + "had opened by the time the assertion read a row", + }, "tasksEnterInsideWord": { screen: "enter go inside it", why: "the roster's other door, over work no window is holding any more — it is the one that opens the " + @@ -602,8 +623,9 @@ var tuiWords = map[string]tuiWord{ // ── the run engine's plan, on the tasks place ──────────────────────────── // - // A `/task` under CODEAF_TASK_BELT=bash starts a RUN rather than a node of - // this session's own tree: the conversation seeds a plan store, the engine + // A `/task` on the worker harness — the default, and what CODEAF_TASK_BELT=bash + // names outright — starts a RUN rather than a node of this session's own + // tree: the conversation seeds a plan store, the engine // drives it, and the store's root lands on the tasks place beside the record // (internal/tui3's taskplan.go). These rows are what the tmux suite reads to // prove the run happened, moved, and left a page of its own. @@ -629,7 +651,19 @@ var tuiWords = map[string]tuiWord{ why: "the command a bash-belt worker finishes its store task with, recorded in the task's own " + "trajectory and drawn as a step line on the plan page (taskplan.go's taskPlanBody reads " + "PlanTaskPage.Steps). THE PAGE DOES NOT SPELL IT — the worker runs it — so the gate looks where " + - "it is written: internal/session's plandb_plan.go, the sentence that teaches the finish", + "it is written: internal/session's plandb_plan.go, the sentence that teaches the finish. " + + "IT IS OBSERVED AND NEVER WAITED OUT, because whether it is on a page is the WORKER'S " + + "choice: the run writes the ending itself for a task whose worker stopped calling tools " + + "without writing one (internal/run's worker.go), which a small brief on a fast model " + + "regularly is. What the suite asserts about that page instead is that it is the store's " + + "page at all — planNoteBoxWord and planStepsSpend, two words no room draws", + }, + "planNoteBoxWord": { + screen: "a note for this task", + why: "the plan page's own note box (taskplan.go's taskPlanNoteWord), and the one word on it " + + "that is there whatever state the task is in. It is what says the press over a run's row " + + "opened THE STORE'S PAGE rather than a room — the assertion that defect #1359 was about — " + + "where the live step beside it is a moment and is only ever observed", }, "planLiveGlyph": { screen: tokens.GlyphStepRunning, diff --git a/internal/manual/chat/adaptive-runs.md b/internal/manual/chat/adaptive-runs.md index 151c4a290f..7dfa7a3d33 100644 --- a/internal/manual/chat/adaptive-runs.md +++ b/internal/manual/chat/adaptive-runs.md @@ -157,6 +157,7 @@ keyboard decides for itself and says on the record that it decided. | `--timeout` | a hard wall on the whole run | | `--json` | print one machine-readable object instead of the deliverable | | `--yes-spend` | approve a plan whose price crosses the consent threshold | +| `--slots ` | how many workers may run at once for this run; `0` is no limit. Unset, it is your `task.parallel` setting, which is no limit out of the box | | `--model ` | the work model for this run | | `--plan-model ` | the model that plans, when it should differ from the work model | | `--check-model ` | the model that checks finished work; then `CODEAF_CHECK_MODEL`, then a plan seat pinned by flag or environment, then the crew's careful row | diff --git a/internal/manual/chat/how-tasks-run.md b/internal/manual/chat/how-tasks-run.md index 9434bad6b0..73d49c8829 100644 --- a/internal/manual/chat/how-tasks-run.md +++ b/internal/manual/chat/how-tasks-run.md @@ -629,11 +629,11 @@ where it stands. A task is the same agent you talk to, with the same tools, in a quieter place. -**On the worker harness road its belt is not the conversation's.** With -`CODEAF_TASK_BELT=bash` set, a worker carries one shell and the plan CLI rather than -these tools, and the verbs for handing work out come off it; the *worker harness* page -names what that belt carries. Everything below is the belt the older road composes, -which is what a build without the switch gives every task. +**On the worker harness road its belt is not the conversation's.** That road is the +default: a worker carries one shell and the plan CLI rather than these tools, and the +verbs for handing work out come off it; the *worker harness* page names what that belt +carries. Everything below is the belt the older road composes, which is what a build +reaches only when `CODEAF_TASK_BELT` is set to `node`, `legacy` or `off`. It inherits the conversation's provider client, context window, image support, roles source, search provider and fetcher, **connected accounts**, image-generation model and @@ -2520,6 +2520,11 @@ By default, **no limit**. `task.parallel` is 0 (blank) out of the box, and 0 mea A cap, if you set one, is a **queue and never a refusal**: a ready task past the cap sits and starts when a slot frees. +The same row answers for every road work runs on: a task this conversation puts on the +worker harness runs as many of its parts at once as `task.parallel` allows, and so does +`codeaf do`. That command can also name a figure for one run with `--slots `, where +`0` is no limit, and a figure named there outranks the setting for that run only. + The real ceiling is the machine. Before starting **each** task, codeaf asks whether one more may start: diff --git a/internal/manual/chat/tasks.md b/internal/manual/chat/tasks.md index f30c032074..b1fe8a4af7 100644 --- a/internal/manual/chat/tasks.md +++ b/internal/manual/chat/tasks.md @@ -5568,8 +5568,7 @@ and breadcrumbs remain available. ## will the chat do it itself or start a task? One read, one edit or one command the chat does itself. Anything with parts goes -out as tasks. With `CODEAF_TASK_BELT=bash` set there is **one way** the chat puts -work out, a task: +out as tasks. There is **one way** the chat puts work out, a task: - **hand off:** the chat proposes a task; approving the card, or letting its countdown run out, starts it as a run in the conversation's plan. @@ -5581,17 +5580,17 @@ work out, a task: - **ask about:** the chat reads the run's rows and the task's own steps and answers from them. It never redoes the work. -## is there a quick task with the bash belt on? can the chat still start quick tasks? +## is there a quick task? can the chat still start quick tasks? -**No. With `CODEAF_TASK_BELT=bash` set the conversation has no quick task.** The +**No. The conversation has no quick task.** The chat's only verb for putting work out is a task, and every task it starts is part of the conversation's plan, where the tree shows it and a check reads it. A quick task ran outside the plan, in the folder you stand in, with no check, so it was left off rather than kept as a second road. Asked to parallelize, the chat proposes several tasks at once. Small work it simply does itself. -With the variable unset nothing changes: quick tasks work as *What a quick task is* -describes. +With `CODEAF_TASK_BELT` set to `node`, `legacy` or `off` the older engine returns +and quick tasks work as *What a quick task is* describes. ## what are the what, since, now and next lines? diff --git a/internal/manual/chat/worker-harness.md b/internal/manual/chat/worker-harness.md index 2881956473..7c5f6b1174 100644 --- a/internal/manual/chat/worker-harness.md +++ b/internal/manual/chat/worker-harness.md @@ -571,19 +571,25 @@ which must land before the run is over. The fix is checked in turn, but only once: a finding on a `fix:` task is a note and no second fix task, so a run cannot loop. -## How to turn it on +## How to turn it off -**This page describes machinery that is not the shipped default.** The harness runs -when the environment variable `CODEAF_TASK_BELT=bash` is set. With it unset, the -build is unchanged and the older engine serves every road: +**This page describes the shipped default.** The harness is what a `/task` and a +`codeaf do` run on, on every machine, with nothing set. The environment variable +`CODEAF_TASK_BELT` is now the way *out* of it rather than the way in: set it to +`node`, `legacy` or `off` and the older engine serves every road again: - a `/task` is a node of this session's own tree, not a run on the plan store; - a task worker carries the conversation's own tools, not one shell; - `codeaf do` is dispatched by the resident's reconciler, not the run engine. +Those three words are the only ones that turn it off. Any other value, including +an empty one, leaves you on the harness, so a typo cannot quietly move you to a +different engine. + The switch is read where the belt is composed, where a person's `/task` is -admitted, and where `codeaf do` chooses its road — and **with it unset, not one -byte of any prompt, belt or landing moves**. +admitted, and where `codeaf do` chooses its road — and **with one of those three +words set, not one byte of any prompt, belt or landing moves from the older +road**. Everything behind the switch is a seam. A build with no run engine linked answers the older road, and every refusal on the run road falls back to it rather than diff --git a/internal/run/bashworker.go b/internal/run/bashworker.go index ebfedbc049..ef97370339 100644 --- a/internal/run/bashworker.go +++ b/internal/run/bashworker.go @@ -47,10 +47,10 @@ import ( // the ordinary plan-spend path, so this is the one writer of a run task's row, // and the model it names is the model every call the worker made went out on. // -// THE FLAG IS THE DOOR'S. CODEAF_TASK_BELT=bash is what makes a run wire this -// worker at all; the seat's constructor reads the switch once and refuses -// without it, so with the flag unset not one byte of any prompt, belt or -// landing changes — nothing constructs this worker. +// THE FLAG IS THE DOOR'S. The bash belt is what makes a run wire this worker at +// all; the seat's constructor reads the switch once and refuses when +// CODEAF_TASK_BELT names the older belt, so on that road not one byte of any +// prompt, belt or landing changes — nothing constructs this worker. type BashWorker struct { store *plandb.Store workspace string diff --git a/internal/run/run.go b/internal/run/run.go index 5dea1bc590..6b386d60a9 100644 --- a/internal/run/run.go +++ b/internal/run/run.go @@ -183,12 +183,20 @@ type Supervisor struct { // NewSupervisor builds a run over store. The workspace is the run's own // working copy, carried for the worker seat and the landing that follow this -// loop; slots bounds how many workers run at once (a slot count below one -// runs one at a time, which keeps a misconfigured run alive rather than -// dead); limits bound the run's cost and, per task, its steps. +// loop; slots bounds how many workers run at once; limits bound the run's +// cost and, per task, its steps. +// +// A SLOT COUNT BELOW ONE IS NO BOUND AT ALL. That is the word the setting the +// chat door reads gives it — `task.parallel` is 0 out of the box and 0 means no +// limit (internal/config's DefaultTaskParallel) — and the door passes the +// figure through as given. This constructor used to read the same 0 as 1 "to +// keep a misconfigured run alive", which quietly ran every task a conversation +// put on the harness one worker at a time while the setting beside it promised +// no limit. What runs out is the machine and the provider's rate, and both are +// governed elsewhere; the number of workers is not the resource. func NewSupervisor(store *plandb.Store, workspace string, slots int, limits Limits, factory WorkerFactory) *Supervisor { - if slots < 1 { - slots = 1 + if slots < 0 { + slots = 0 } return &Supervisor{ Owner: ownerName(), @@ -198,7 +206,7 @@ func NewSupervisor(store *plandb.Store, workspace string, slots int, limits Limi limits: limits, factory: factory, after: time.After, - finished: make(chan workerReturn, slots+1), + finished: make(chan workerReturn, returnsBuffer(slots)), liveMoved: make(chan struct{}, 1), cancels: make(map[string]context.CancelFunc), cut: make(map[string]bool), @@ -386,7 +394,7 @@ func (s *Supervisor) pass(ctx context.Context, rootID string) Outcome { } return OutcomeIncomplete } - if s.inFlight < s.slots && !s.dispatchedRoot { + if !s.full() && !s.dispatchedRoot { // The root is the first worker of the run. It is claimed by the runtime // in the store, so it needs no claim here — only a seat. s.dispatchedRoot = true @@ -394,7 +402,7 @@ func (s *Supervisor) pass(ctx context.Context, rootID string) Outcome { } if s.limitHit == "" { for _, ready := range s.store.ReadySet().Runnable { - if s.inFlight >= s.slots { + if s.full() { break } task := *ready @@ -486,34 +494,55 @@ func (s *Supervisor) launch(ctx context.Context, task plandb.Task, wake string) // // THE DOLLARS ARE NOT DROPPED, for absorb's own reason: what a run counts as // spent includes every paid call whatever way its task ended, and a worker the -// run outlived was paid for like any other. Every worker has returned by the -// time the wait is over and each left exactly one return in the channel, which -// is deep enough to hold them all, so they are read here without waiting and -// only their spend is settled. It also leaves the channel empty, so a return -// from this run can never be read as one of the next. +// run outlived was paid for like any other. Every worker leaves exactly one +// return in the channel on its way out, and inFlight is the count of those not +// yet read, so exactly that many are read here and only their spend is +// settled. It also leaves the channel empty, so a return from this run can +// never be read as one of the next. +// +// THE RETURNS ARE READ BEFORE THE GOROUTINES ARE WAITED FOR. A run with no +// slot bound can have more workers out than the channel has room for, and a +// worker blocked on handing its return in never reaches Done; waiting first +// would wait forever. Reading first is right on a bounded run too, where it +// changes nothing but the order. func (s *Supervisor) drain() { for id, cancel := range s.cancels { cancel() delete(s.cancels, id) } - s.workers.Wait() - for { - select { - case ret := <-s.finished: - s.inFlight-- - // THE ENDING IS DROPPED, BUT THE FACT OF WHO IT CUT IS NOT: a - // worker that came home with the run's own ending as its error was - // taken down by that ending, and the run records it where it knows - // ([Summary.Cut]). The ending itself stays dropped, for the reason - // the comment above gives. - if errors.Is(ret.err, context.Canceled) { - s.cut[ret.task.ID] = true - } - s.settleSpend(ret) - default: - return + for s.inFlight > 0 { + ret := <-s.finished + s.inFlight-- + // THE ENDING IS DROPPED, BUT THE FACT OF WHO IT CUT IS NOT: a worker + // that came home with the run's own ending as its error was taken down + // by that ending, and the run records it where it knows ([Summary.Cut]). + // The ending itself stays dropped, for the reason the comment above + // gives. + if errors.Is(ret.err, context.Canceled) { + s.cut[ret.task.ID] = true } + s.settleSpend(ret) + } + s.workers.Wait() +} + +// full says whether the run may launch nothing more right now: a bounded run +// with every slot taken. An unbounded run is never full. +func (s *Supervisor) full() bool { + return s.slots > 0 && s.inFlight >= s.slots +} + +// returnsBuffer sizes the channel workers hand their returns through. A +// bounded run can have at most slots workers out, so slots+1 holds every +// return without a sender ever waiting; an unbounded run has no such figure, +// so it gets a modest depth and the loop's habit of reading the channel in +// every select — and [drain]'s order — is what keeps a sender from waiting +// long. +func returnsBuffer(slots int) int { + if slots < 1 { + return 16 } + return slots + 1 } // bankLive is the one thing a worker's goroutine does to the run's account: it @@ -997,7 +1026,7 @@ func (s *Supervisor) launchWakes(ctx context.Context, rootID string) { if !needsWake(tasks, &task, s.cancels, s.wakes, s.reported) { continue } - if s.inFlight >= s.slots { + if s.full() { return } // THE WOKEN-PARENT LAW: every non-root launch holds the task under its @@ -1050,7 +1079,7 @@ func (s *Supervisor) launchWaits(ctx context.Context, rootID string) { if len(moved) == 0 { continue } - if s.inFlight >= s.slots { + if s.full() { return } // A READY LEAF IS CLAIMED, the same claim every dispatch makes, so its @@ -1519,9 +1548,9 @@ type Spec struct { // assignment the root worker reads. Title string Brief string - // Slots bounds how many workers run at once, and Limits bound the run's - // cost and its per-task steps. Both pass through to the supervisor as - // given. + // Slots bounds how many workers run at once, and 0 is no bound; Limits + // bound the run's cost and its per-task steps. Both pass through to the + // supervisor as given. Slots int Limits Limits // Factory makes the worker for every task the run dispatches. Start holds diff --git a/internal/run/unbounded_test.go b/internal/run/unbounded_test.go new file mode 100644 index 0000000000..1b1b823004 --- /dev/null +++ b/internal/run/unbounded_test.go @@ -0,0 +1,85 @@ +package run_test + +import ( + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/plandb" + "github.com/Agent-Field/codeaf/internal/run" +) + +// A SLOT COUNT OF ZERO IS NO BOUND. It is the figure the chat door hands the +// engine out of the box — `task.parallel` is 0 and 0 means no limit — and the +// supervisor used to read it as one, so every ready leaf of a conversation's +// task ran after the one before it while the setting promised otherwise. The +// test hands the run more ready leaves than any small bound would admit and +// asks the seat how many it held at once. +func TestSupervisorLaunchesEveryReadyLeafAtOnceWithNoSlotBound(t *testing.T) { + store := runOpenStore(t) + ctx := runContext(t) + seat := newFakeSeat() + var leaves []plandb.TaskSpec + for _, id := range []string{"l1", "l2", "l3", "l4", "l5", "l6"} { + leaves = append(leaves, plandb.TaskSpec{ID: id, Title: id}) + // Each leaf holds its seat long enough that six launched together are + // six on the seat together; a serial run would show a peak of one. + seat.actions[id] = holdSeat(150 * time.Millisecond) + } + seat.actions["root"] = splitRoot(t, store, leaves...) + supervisor := run.NewSupervisor(store, t.TempDir(), 0, run.Limits{}, seat.workerFor) + + outcome := supervisor.Run(ctx) + + if outcome != run.OutcomeDone { + t.Fatalf("outcome = %q, want %q", outcome, run.OutcomeDone) + } + if peak := seat.peakConcurrency(); peak != len(leaves) { + t.Fatalf("peak concurrency = %d under no slot bound, want all %d leaves on the seat together", peak, len(leaves)) + } +} + +// MORE WORKERS OUT THAN THE RETURN CHANNEL HOLDS MUST STILL DRAIN. The channel +// workers hand their returns through has a fixed depth on an unbounded run, so +// a drain that waited for every goroutine before reading a return would wait +// on a worker that is itself waiting to be read. Two cheap leaves come home +// fast and spend the run past its dollar limit while the other thirty-eight +// are still on their seats, which is the road that reaches drain with workers +// in flight; the test is that Run comes back. It carries a wall of its own +// because a deadlock inside drain never looks at the run's context. +func TestAnUnboundedRunEndedEarlyStillDrainsEveryWorker(t *testing.T) { + store := runOpenStore(t) + ctx := runContext(t) + seat := newFakeSeat() + var leaves []plandb.TaskSpec + for i := 0; i < 40; i++ { + id := "l" + string(rune('a'+i%26)) + string(rune('a'+i/26)) + leaves = append(leaves, plandb.TaskSpec{ID: id, Title: id}) + hold := 3 * time.Second + if i < 2 { + hold = 300 * time.Millisecond + } + seat.actions[id] = holdSeat(hold) + } + seat.actions["root"] = splitRoot(t, store, leaves...) + // holdSeat banks $0.30 a leaf, so the two fast ones alone cross the limit. + supervisor := run.NewSupervisor(store, t.TempDir(), 0, run.Limits{CostUSD: 0.50}, seat.workerFor) + + outcomes := make(chan run.Outcome, 1) + go func() { outcomes <- supervisor.Run(ctx) }() + var outcome run.Outcome + select { + case outcome = <-outcomes: + case <-time.After(8 * time.Second): + t.Fatal("Run did not come back: drain is waiting on workers that are waiting to hand in their returns") + } + + if outcome != run.OutcomeLimit { + t.Fatalf("outcome = %q, want %q", outcome, run.OutcomeLimit) + } + // The property under test is that more workers were out than the channel + // could hold, not the exact count, which is why the bar is the depth and + // not the number of leaves. + if peak := seat.peakConcurrency(); peak <= 16 { + t.Fatalf("peak concurrency = %d, want more workers out than the return channel holds", peak) + } +} diff --git a/internal/session/bashbelt.go b/internal/session/bashbelt.go index 7f04287a28..ab7132523c 100644 --- a/internal/session/bashbelt.go +++ b/internal/session/bashbelt.go @@ -32,10 +32,10 @@ import ( // // internal/exec/bare IS UNTOUCHED. The hands stay bare's; the experiment is a // wire change — which tools the model can name — and the conversation belt and -// every subharness leaf keep pi's tools byte for byte. Unset -// CODEAF_TASK_BELT, and not one byte of a worker is where it was: [Agent.belt] -// delegates only when the predicate holds, which is what lets both arms of the -// experiment run from one binary. +// every subharness leaf keep pi's tools byte for byte. Turn CODEAF_TASK_BELT +// off, and not one byte of a worker is where it was on the older road: +// [Agent.belt] delegates only when the predicate holds, which is what lets both +// roads run from one binary. // bashBeltSourceCaps is the cap the branch bash hand is BUILT with. bare cuts // a bash result tail-only inside its own accumulator, from the caps the tool @@ -244,14 +244,26 @@ func branchBashDescription(caps bare.Caps) string { return fmt.Sprintf("Execute a bash command in the current working directory. Returns stdout and stderr. Output over %dKB is cut to its first half and its last half, and the whole output is filed as a file the result names. Optionally provide a timeout in seconds.", kb) } -// bashBeltAsked reads THE EXPERIMENT'S SWITCH, and it is the ONE reader of +// beltOffWords are the spellings of CODEAF_TASK_BELT that send a task back to +// the node belt. They are the ONLY way off the bash belt, and the list is +// deliberately short and closed: an unrecognised word leaves a person on the +// belt they were promised rather than quietly moving them off it, because a +// typo in an environment variable must not be able to change which engine runs +// the work. The empty string is not on the list — an exported-but-empty +// variable is the same as an unset one, which is the default, which is bash. +var beltOffWords = map[string]bool{"node": true, "legacy": true, "off": true} + +// bashBeltAsked reads THE BELT SWITCH, and it is the ONE reader of // CODEAF_TASK_BELT in this package: the belt a task worker is built on // (newTaskAgentOn) and the landing that judges it (workTaskNode's gate) are // two halves of one fact, and two readers could disagree about which belt a -// node is on. Unset — every machine not running the experiment — it is false, -// and every byte of every worker is where it was. +// node is on. Unset — every machine that has asked for nothing — it is TRUE: +// the bash belt is the belt a task runs on. The variable was the way IN to an +// experiment and is now the way OUT of the default, so the sense of every +// reader below is unchanged while the answer they get when nobody has spoken +// is the opposite of what it was. func bashBeltAsked() bool { - return strings.TrimSpace(env.Get("CODEAF_TASK_BELT")) == "bash" + return !beltOffWords[strings.ToLower(strings.TrimSpace(env.Get("CODEAF_TASK_BELT")))] } // BashBeltAsked is [bashBeltAsked] as a door outside this package reads it: a diff --git a/internal/session/bashbelt_default_test.go b/internal/session/bashbelt_default_test.go index a15ae40eae..0e59746359 100644 --- a/internal/session/bashbelt_default_test.go +++ b/internal/session/bashbelt_default_test.go @@ -2,42 +2,45 @@ package session import "testing" -// THE DEFAULT IS A FACT WORTH PINNING WHICHEVER WAY IT POINTS. +// THIS FILE IS THE ONLY THING IN THE PACKAGE THAT TESTS THE DEFAULT. // -// The bash belt was made the default in #1335 and the default was moved back -// here, on a measured comparison rather than on anybody's reading. What that -// episode showed is that the default had never been asserted anywhere: it was -// carried only by the absence of a value in other tests, so it could move, and -// did, without one test in the tree saying a word about it. +// hermetic_test.go's TestMain pins CODEAF_TASK_BELT to the older belt for the +// whole suite, because this package's tests were written against that engine +// and named it by saying nothing. That pin is correct and it has a cost: with +// it in place, nothing in the package would notice if the default went back to +// the older belt, because every test would keep passing and the suite's green +// would be computed over a road no person runs. // -// So the switch's whole answer is written out. An unset or blank variable is -// the older node belt; the exact word `bash` is the harness; nothing else -// reaches the harness, because the predicate is an equality and not a list. -func TestTheBeltIsTheNodeBeltUnlessTheWordIsExactlyBash(t *testing.T) { +// So the truth table is asserted here, by name, with the variable set for each +// row through t.Setenv — which outranks the package pin and is restored on the +// way out. An empty value is a row, and a deliberate one: an exported but blank +// variable must read as an unset one, which is the default, which is bash. +func TestTheBeltIsBashUnlessOneOfThreeWordsSaysOtherwise(t *testing.T) { for _, row := range []struct { value string want bool why string }{ - {"", false, "unset or blank is the default, and the default is the node belt"}, - {"bash", true, "the one word that asks for the harness"}, - {"node", false, "a word naming the older belt is not the word for the harness"}, - {"legacy", false, "and neither is this one"}, - {"off", false, "nor this"}, - {"BASH", false, "the match is exact: an upper-case spelling does not reach the harness"}, - {"bashx", false, "and neither does a longer word that starts the same way"}, - {"true", false, "a word that means nothing here moves nobody"}, + {"", true, "unset or blank is the default, and the default is the harness"}, + {"bash", true, "the word that used to be the way in still names the harness"}, + {"node", false, "the older belt, named"}, + {"legacy", false, "the older belt, under its other name"}, + {"off", false, "the older belt, for a person who reads the switch as a switch"}, + {"NODE", false, "the words are matched without case, as a person types them"}, + {" node ", false, "surrounding space is a person's typing, not a different word"}, + {"nodes", true, "an unrecognised word leaves a person on the belt they were promised"}, + {"true", true, "a word that means nothing here does not move anybody"}, + {"no", true, "and neither does one that looks like it should"}, } { t.Run(row.value, func(t *testing.T) { t.Setenv("CODEAF_TASK_BELT", row.value) if got := bashBeltAsked(); got != row.want { - t.Fatalf("CODEAF_TASK_BELT=%q: harness on = %v, want %v — %s", row.value, got, row.want, row.why) + t.Fatalf("CODEAF_TASK_BELT=%q: belt on = %v, want %v — %s", row.value, got, row.want, row.why) } // The exported door and the package's own reader are two halves of - // one fact. A change that flipped one and not the other would leave - // a run and the landing that judges it on different belts, which is - // the exact thing the single-reader rule above bashBeltAsked exists - // to prevent. + // one fact, and the whole point of the door is that they cannot + // disagree. A change that flipped one and not the other would leave + // a run and the landing that judges it on different belts. if BashBeltAsked() != bashBeltAsked() { t.Fatalf("CODEAF_TASK_BELT=%q: the exported door and the package reader disagree", row.value) } @@ -45,13 +48,12 @@ func TestTheBeltIsTheNodeBeltUnlessTheWordIsExactlyBash(t *testing.T) { } } -// SURROUNDING SPACE IS A PERSON'S TYPING. The predicate trims, so a value a -// shell carried a space into still asks for the harness; this is separate from -// the table above because it is a property of the reader rather than a row of -// the switch. -func TestSpaceAroundTheWordStillAsksForTheHarness(t *testing.T) { - t.Setenv("CODEAF_TASK_BELT", " bash ") - if !bashBeltAsked() { - t.Fatal("a value with surrounding space did not reach the harness, but the predicate trims") +// The suite's own pin is a fact worth asserting, because it is what makes every +// other test in this package a test of the older engine. If it is ever dropped, +// a hundred and forty tests change what they are testing without one line of +// them changing, which is exactly what happened when the default moved. +func TestThePackageSuiteRunsOnTheOlderBelt(t *testing.T) { + if bashBeltAsked() { + t.Fatal("this package's TestMain no longer pins the older belt: every test here is now testing the harness road it was not written for") } } diff --git a/internal/session/bashbelt_plandb_test.go b/internal/session/bashbelt_plandb_test.go index 8b81b013e1..717975b86e 100644 --- a/internal/session/bashbelt_plandb_test.go +++ b/internal/session/bashbelt_plandb_test.go @@ -1022,7 +1022,7 @@ func TestPlandbCliTheBeltAndPageFollowTheSwitch(t *testing.T) { // WITHOUT THE SWITCH: the plain worker the door builds when the // experiment is off — same shape, no bash belt — carries the graph verbs, // and its page is the one it always read. - t.Setenv("CODEAF_TASK_BELT", "") + t.Setenv("CODEAF_TASK_BELT", "node") plain, _ := newTestAgent(t, &scriptedCompleter{}, func(config *Config) { config.InTask = true config.tasker = graphForShape(t) diff --git a/internal/session/bashbelt_test.go b/internal/session/bashbelt_test.go index 2a8df2ac87..ff0be7f0ef 100644 --- a/internal/session/bashbelt_test.go +++ b/internal/session/bashbelt_test.go @@ -242,7 +242,7 @@ func TestBashBeltWorkerThinksFromTheWorkSeat(t *testing.T) { } }) t.Run("on the shipped belt", func(t *testing.T) { - t.Setenv("CODEAF_TASK_BELT", "") + t.Setenv("CODEAF_TASK_BELT", "node") if got := beltWorkerAgent(t, effort.None, effort.None).ResolvedEffort(); got != "" { t.Fatalf("the shipped-belt worker resolves to %q, want absence", got) } diff --git a/internal/session/bashbelt_worker.go b/internal/session/bashbelt_worker.go index 3840eae072..d3cfd9b15b 100644 --- a/internal/session/bashbelt_worker.go +++ b/internal/session/bashbelt_worker.go @@ -50,7 +50,7 @@ import ( // real client itself. func NewBeltWorker(config Config, completer Completer, task *plandb.Task, storePath string) (*Agent, error) { if !bashBeltAsked() { - return nil, errors.New("the bash belt is off: CODEAF_TASK_BELT is not bash") + return nil, errors.New("the bash belt is off: CODEAF_TASK_BELT names the node belt") } if task == nil { return nil, errors.New("no store task for the worker seat") diff --git a/internal/session/chat_role_prompt_test.go b/internal/session/chat_role_prompt_test.go index 25339feb41..d3170eb968 100644 --- a/internal/session/chat_role_prompt_test.go +++ b/internal/session/chat_role_prompt_test.go @@ -63,7 +63,7 @@ func TestTheConversationUnderTheBeltHasOneWayToPutWorkOut(t *testing.T) { func TestTheConversationWithoutTheBeltKeepsBothVerbs(t *testing.T) { now := time.Date(2026, 9, 18, 12, 0, 0, 0, time.FixedZone("EDT", -4*60*60)) - t.Setenv("CODEAF_TASK_BELT", "") + t.Setenv("CODEAF_TASK_BELT", "node") registerBeltRunEngine(t, newBeltRunDouble("unused")) agent, _ := newTestAgent(t, beltRunCompleter{text: "unused"}, func(config *Config) { config.Workspace = t.TempDir() diff --git a/internal/session/hermetic_test.go b/internal/session/hermetic_test.go index 993b54fcbb..65e7913db9 100644 --- a/internal/session/hermetic_test.go +++ b/internal/session/hermetic_test.go @@ -96,6 +96,30 @@ func runTests(m *testing.M) int { fmt.Fprintf(os.Stderr, "session tests: could not pin furrow away: %v\n", err) return 1 } + // AND THIS PACKAGE'S SUITE IS THE OLDER BELT'S SUITE, SAID ONCE HERE. + // + // The bash belt is the product's default. Most of the tests in this package + // were written against the node belt and describe its behaviour — its + // landings, its auditor, its own task tree — and they said so by saying + // nothing, because the default used to agree with them. When the default + // moved they did not become wrong, they became silent about which road they + // meant, and a hundred and forty of them changed what they were testing + // without one line of them changing. + // + // So the road they were written for is named here rather than a hundred and + // forty times, and a test that means the harness says so with + // t.Setenv(..., "bash"), which outranks this for that test and is restored + // on the way out. Every harness test in this package already does. + // + // THIS IS NOT A PIN THAT MAKES THE DEFAULT UNTESTED. The harness road has + // its own suites and they run on the real default: internal/run, + // internal/plandb, and this package's own bashbelt, plandb and land_run + // files. What is pinned here is the older engine's suite, which is the only + // thing it ever tested. + if err := os.Setenv("CODEAF_TASK_BELT", "node"); err != nil { + fmt.Fprintf(os.Stderr, "session tests: could not pin the belt: %v\n", err) + return 1 + } code := m.Run() diff --git a/internal/session/land_run_tree.go b/internal/session/land_run_tree.go index 2525479246..d8d5035450 100644 --- a/internal/session/land_run_tree.go +++ b/internal/session/land_run_tree.go @@ -25,11 +25,11 @@ import "errors" // THE SWITCH IS THE BELT'S, read here for the reason [NewBeltWorker] reads it: // the belt road stages the tree's own status minus what the harness itself // writes, which is not what the default landing does, so a landing that ran -// with CODEAF_TASK_BELT unset would change a landing for a belt nobody +// with CODEAF_TASK_BELT naming the node belt would change a landing for one nobody // composed. With the flag off this refuses and touches nothing. func LandRunTree(dir, title string, sign bool) (branch string, changed []string, refusal string, err error) { if !bashBeltAsked() { - return "", nil, "", errors.New("the bash belt is off: CODEAF_TASK_BELT is not bash") + return "", nil, "", errors.New("the bash belt is off: CODEAF_TASK_BELT names the node belt") } saved, problem, why := commitTaskWork(dir, title, nil, sign, true) if problem != "" { diff --git a/internal/session/land_run_tree_test.go b/internal/session/land_run_tree_test.go index b1b7e39eac..4c786c8893 100644 --- a/internal/session/land_run_tree_test.go +++ b/internal/session/land_run_tree_test.go @@ -65,7 +65,7 @@ func TestLandRunTreeRefusesATreeWithNothingToLand(t *testing.T) { // THE SWITCH IS THE BELT'S. With CODEAF_TASK_BELT off the door refuses and not // one byte of the tree moves: no commit, no index, the work still on the floor. func TestLandRunTreeLeavesTheTreeAloneWithTheFlagOff(t *testing.T) { - t.Setenv("CODEAF_TASK_BELT", "") + t.Setenv("CODEAF_TASK_BELT", "node") repo := newTestRepo(t) before := gitOut(t, repo, "rev-parse", "HEAD") writeFile(t, filepath.Join(repo, "second.txt"), "two\n") diff --git a/internal/session/plandb_spend_test.go b/internal/session/plandb_spend_test.go index 283e888f65..10e335c237 100644 --- a/internal/session/plandb_spend_test.go +++ b/internal/session/plandb_spend_test.go @@ -259,7 +259,7 @@ func TestSpendRowsFollowThePlanNode(t *testing.T) { // AND SO DOES THE FLAG-OFF WORKER, whatever its node carries: the switch // gates the wiring whole, so with it off nothing is armed and not one row // is written anywhere — not by this worker, and not by the store road. - t.Setenv("CODEAF_TASK_BELT", "") + t.Setenv("CODEAF_TASK_BELT", "node") osession, _ := newTestAgent(t, &scriptedCompleter{}, nil) ograph := osession.graph() path := filepath.Join(t.TempDir(), planStoreFilename) diff --git a/internal/session/plandbcli_e2e_test.go b/internal/session/plandbcli_e2e_test.go index 3ee76cac66..96568baaaa 100644 --- a/internal/session/plandbcli_e2e_test.go +++ b/internal/session/plandbcli_e2e_test.go @@ -659,7 +659,7 @@ func TestPlandbCliWorkerOwnDone(t *testing.T) { // session folder, arms no shim, and hands the worker the ordinary belt with // the graph's own verbs on it — every byte where it was. func TestPlandbCliFlagOffTouchesNothing(t *testing.T) { - t.Setenv("CODEAF_TASK_BELT", "") + t.Setenv("CODEAF_TASK_BELT", "node") place := Place{Dir: t.TempDir()} repo := newTestRepo(t) t.Setenv("HOME", t.TempDir()) diff --git a/internal/session/planrow_identity_test.go b/internal/session/planrow_identity_test.go new file mode 100644 index 0000000000..835bf84ee3 --- /dev/null +++ b/internal/session/planrow_identity_test.go @@ -0,0 +1,66 @@ +package session + +// planrow_identity_test.go pins the IDENTITY THE RUN'S DOOR PUBLISHES: the row a +// person is answered with says which task of the plan store it is, and that id +// is the one the store's own read answers under. +// +// THE SURFACE CANNOT WORK THIS OUT FOR ITSELF. The row and the store task wear +// the same title, and a title cannot tell a run's row — which the graph holds no +// node for — from a node that merely says the same words; the tasks place drew +// the wrong half of the pair and its Enter opened an empty room (#1355). So the +// door that mints both halves in one breath states the link +// ([TaskNotice.PlanTask]), and this test is the two ends of it meeting. + +import ( + "context" + "testing" +) + +func TestARunsRowNamesTheStoreTaskThePlanReadAnswersUnder(t *testing.T) { + t.Setenv("CODEAF_TASK_BELT", "bash") + double := newBeltRunDouble("done") + registerBeltRunEngine(t, double) + conversation := newTestRepo(t) + sessionDir := t.TempDir() + agent, _ := newTestAgent(t, beltRunCompleter{text: "done"}, func(config *Config) { + config.Workspace = conversation + config.Place = Place{Dir: sessionDir} + config.AskConsent = false + }) + stand := taskStand{dir: conversation, mode: TaskModeWorktree} + if err := agent.startKnownTaskRun(context.Background(), 41, "write HELLO.md", "brief", nil, stand, ""); err != nil { + t.Fatalf("start run: %v", err) + } + <-double.entered + + // THE ROW SAYS WHICH TASK IT IS. Without this the place is back to matching + // the pair on the title they share. + row, found := runRowOf(agent.graph(), 41) + if !found { + t.Fatal("the run's door published no row for the work it started") + } + if row.PlanTask == "" { + t.Fatal("the run's row names no store task, so nothing can tell it from a node of this " + + "session's own tree wearing the same title (TaskNotice.PlanTask)") + } + + // AND THE STORE ANSWERS UNDER THAT ID. The two halves are one piece of work + // read from two ends only if the id the row names is the id the plan read + // files its own row under — a row naming an id nothing answers for would + // leave the place with an identity it cannot use. + rows := agent.PlanTasks() + if len(rows) == 0 { + t.Fatal("the conversation's plan read answers no rows while its run is live") + } + var titles []string + for _, task := range rows { + if task.ID == row.PlanTask { + close(double.release) + return + } + titles = append(titles, task.ID+" "+task.Title) + } + close(double.release) + t.Fatalf("the run's row names store task %q and the plan read answers under %v: the place cannot "+ + "join the pair by an id only one of them uses", row.PlanTask, titles) +} diff --git a/internal/session/task_contract.go b/internal/session/task_contract.go index da0d298baf..f3350a2b32 100644 --- a/internal/session/task_contract.go +++ b/internal/session/task_contract.go @@ -602,6 +602,28 @@ type TaskNotice struct { // carries these facts in its own record. Nil is a row whose copy was never // recorded, which is a run that cannot be carried on. Copy *TaskCopyRecord + // PlanTask is WHICH TASK OF THE PLAN STORE THIS ROW IS, and it is the one + // fact that tells a row the store answers for from a row the graph holds a + // node for. It is set on a RUN's row and nowhere else, by the door that + // minted both halves in one breath (task_run_belt.go's + // [Agent.startKnownTaskRun] names the store task with the number the row + // wears), and it is empty on every node of the session's own tree. + // + // IT IS SPELLED THE ONE WAY A STORE ID CROSSES THIS SEAM ([planStoreID]): + // the same spelling [PlanTaskRow.ID] carries and [Agent.PlanTaskPage] is + // asked for. The store's own bare id is answered under by nothing a surface + // can reach, so a row carrying that instead would name an identity no read + // in this package joins. + // + // IT IS AN IDENTITY AND NOT A DESCRIPTION. A surface reading it knows this + // row and that store task are one piece of work read from two ends, so it + // can draw the one of them the store is the authority for — its state word, + // and the page carrying its worker's trajectory. Before this field existed + // the only link was the TITLE the two halves happened to share, which + // cannot tell a run's row from a node that merely wears the same words, and + // the place drew the row whose Enter opened a room the engine holds no node + // for (internal/tui3's taskplan.go). + PlanTask string // Merge is how the branch came home: "merged", "kept" (finished but left // on its branch), "conflicted" (branch kept), "inplace" (a non-git // workspace ran in the person's tree), or "" while running. diff --git a/internal/session/task_proposal_belt_test.go b/internal/session/task_proposal_belt_test.go index d42b2e5954..4fae3ee2d0 100644 --- a/internal/session/task_proposal_belt_test.go +++ b/internal/session/task_proposal_belt_test.go @@ -118,7 +118,7 @@ func TestApprovedProposalBashBeltJoinsLiveRunWithPlanDependencies(t *testing.T) } func TestApprovedProposalWithoutBeltKeepsSessionTreeRoad(t *testing.T) { - t.Setenv("CODEAF_TASK_BELT", "") + t.Setenv("CODEAF_TASK_BELT", "node") double := newBeltRunDouble("must not run") registerBeltRunEngine(t, double) dir := t.TempDir() diff --git a/internal/session/task_run_belt.go b/internal/session/task_run_belt.go index d2ad1012d3..0782fe1451 100644 --- a/internal/session/task_run_belt.go +++ b/internal/session/task_run_belt.go @@ -78,7 +78,8 @@ type RunSpec struct { // and the brief is the assignment the root worker reads. Title string Brief string - // Slots is how many workers run at once. CostUSD is what is left of the + // Slots is how many workers run at once, and 0 is no limit, which is + // the word `task.parallel` itself uses. CostUSD is what is left of the // smaller dollar limit the person set on the conversation, so the run and // conversation spend from the same finite allowance. Slots int @@ -304,7 +305,20 @@ func (a *Agent) startKnownTaskRun(ctx context.Context, id uint64, title, brief s a.beltMu.Lock() live.joined = append(live.joined, id) a.beltMu.Unlock() - a.publishRunRow(g, TaskNotice{ID: id, Title: title, State: TaskRunning, Parent: live.row, StartedAt: a.taskClockNow()}) + a.publishRunRow(g, TaskNotice{ + ID: id, Title: title, State: TaskRunning, Parent: live.row, StartedAt: a.taskClockNow(), + // AND THE ROW SAYS WHICH STORE TASK IT IS, from its first breath, for + // the reason the copy is written down in the same breath below: the + // store is the authority for this work's state and for the page + // carrying its worker's trajectory, and a row that could not name its + // task left a surface guessing from the title + // ([TaskNotice.PlanTask]). IT IS SPELLED THE ONE WAY A STORE ID + // CROSSES THIS SEAM — [planStoreID], which is what + // [PlanTaskRow.ID] carries and what [Agent.PlanTaskPage] is asked + // for — so the id the row names is the id the plan read answers + // under. The bare stored id is answered under by nothing. + PlanTask: planStoreID(storeID), + }) return nil } @@ -345,6 +359,12 @@ func (a *Agent) startKnownTaskRun(ctx context.Context, id uint64, title, brief s a.publishRunRow(g, TaskNotice{ ID: id, Title: title, State: TaskRunning, StartedAt: born, Copy: runCopyOf(tree), + // THE ROOT'S ROW NAMES THE STORE'S ROOT, which is this same number: the + // store was seeded under `storeID` a few lines up, so the row the person + // was answered with and the task the store drives are one identity said + // twice rather than two pieces of work ([TaskNotice.PlanTask]). In + // [planStoreID]'s spelling, which is the one the plan read answers under. + PlanTask: planStoreID(storeID), }) go a.driveBeltRun(runCtx, engine, run, a.beltRunSpec(run, brief)) @@ -452,13 +472,26 @@ func (a *Agent) installBeltRun(g *TaskGraph, run *beltRun) { // Carrying it forward in the one function every publisher goes through is what // keeps that from depending on each of them remembering. A notice that names a // copy of its own wins, because it is the more recent reading. +// +// AND THE STORE TASK IS CARRIED THE SAME WAY, for the same reason: which task +// of the plan this row IS was settled when the row was minted and is true for +// its whole life, so a settle or a stop that publishes a fresh notice must not +// be able to drop it ([TaskNotice.PlanTask]). A row that lost its identity +// halfway through would send the place back to guessing by title exactly when +// the work ended, which is the moment a person goes looking for its page. func (a *Agent) publishRunRow(g *TaskGraph, notice TaskNotice) { - if notice.Copy == nil { + if notice.Copy == nil || notice.PlanTask == "" { for _, kept := range g.runRows(notice.ID) { - if kept.ID == notice.ID && kept.Copy != nil { + if kept.ID != notice.ID { + continue + } + if notice.Copy == nil && kept.Copy != nil { notice.Copy = kept.Copy - break } + if notice.PlanTask == "" && kept.PlanTask != "" { + notice.PlanTask = kept.PlanTask + } + break } } a.emitTaskUpdate(notice) diff --git a/internal/session/task_shape_test.go b/internal/session/task_shape_test.go index 0217bf1518..6334cd2790 100644 --- a/internal/session/task_shape_test.go +++ b/internal/session/task_shape_test.go @@ -150,7 +150,7 @@ func TestAPersonsTaskIsAdmittedOnTheirOwnWords(t *testing.T) { // This law covers the legacy task tree. An ambient bash-belt setting takes // StartTask through the plan-backed road, whose worker brief also carries // the plan identity and lifecycle instructions. - t.Setenv("CODEAF_TASK_BELT", "") + t.Setenv("CODEAF_TASK_BELT", "node") client := &scriptedCompleter{steps: []step{func(context.Context, []ai.Message) (*ai.Response, error) { return textResponse(shapedAnswer), nil }}} diff --git a/internal/session/task_store.go b/internal/session/task_store.go index 48e23bd469..2ecb0f506f 100644 --- a/internal/session/task_store.go +++ b/internal/session/task_store.go @@ -747,6 +747,15 @@ type runRecord struct { // loud rather than repairing. Copy *TaskCopyRecord `json:"copy,omitempty"` + // PlanTask is WHICH TASK OF THE PLAN STORE THIS ROW IS + // ([TaskNotice.PlanTask]), carried across a restart for the same reason the + // copy is: the conversation reopened tomorrow reads its store off the disk + // and has to know which of its tasks the row it is redrawing already + // answers for. A record written before this field existed decodes with "", + // which is the honest reading — that row carries no identity and the title + // is all the place has. + PlanTask string `json:"planTask,omitempty"` + // ElapsedMS is whatever age the row was last published with, frozen. A run's // rows do not carry one today — the family publishes no Elapsed — so it is // absent on every record this code writes, and it is here rather than left @@ -1066,6 +1075,7 @@ func runRowRecord(notice TaskNotice) runRecord { StartedAt: notice.StartedAt, EndedAt: notice.EndedAt, Copy: notice.Copy, + PlanTask: notice.PlanTask, } } @@ -1104,6 +1114,7 @@ func runRowNotice(record runRecord) TaskNotice { StartedAt: record.StartedAt, EndedAt: record.EndedAt, Copy: record.Copy, + PlanTask: record.PlanTask, } if !notice.State.settled() { // WORK NOTHING IS DRIVING IS INTERRUPTED, NOT FAILED. This row was live diff --git a/internal/tui3/place_tasks.go b/internal/tui3/place_tasks.go index 5d3626dac8..e32889df99 100644 --- a/internal/tui3/place_tasks.go +++ b/internal/tui3/place_tasks.go @@ -455,6 +455,10 @@ func (a *app) taskSheetMine() tasksMine { if node := a.taskSheetNodeFor(&entry); node != nil { status := a.taskStatus(node) row.live = &status + // AND WHICH STORE TASK THE ROW IS, when the node is one the run's door + // published. It is read off the node and never off the entry, because + // the node is the half that was told ([taskNode.planTask]). + row.planTask = node.planTask } mine.rows = append(mine.rows, row) } diff --git a/internal/tui3/planrow_identity_test.go b/internal/tui3/planrow_identity_test.go new file mode 100644 index 0000000000..c509063d35 --- /dev/null +++ b/internal/tui3/planrow_identity_test.go @@ -0,0 +1,198 @@ +package tui3 + +// planrow_identity_test.go pins WHICH HALF OF ONE PIECE OF WORK THE TASKS PLACE +// DRAWS when the store and a row of this conversation are both talking about it. +// +// THERE ARE TWO ROADS AND THE ANSWER IS NOT THE SAME ON BOTH. A plan-born node +// is a real node of this session's tree that happens to have a store task +// beside it, and the node is the half with a room behind it. A row the run's +// door published is not a node at all — the door seeds a store, names the +// store's task with the number the person was answered with, and publishes a +// row under it (internal/session's task_run_belt.go) — so the store is the only +// half with a state that moves and a page that opens. +// +// THE DEFECT (#1355's two tmux failures) WAS ONE HEURISTIC ANSWERING BOTH. The +// place matched the halves on the TITLE they share, which cannot tell a run's +// row from a node wearing the same words, and so drew the run as a node row: it +// wore `working`, the engine's word for a node nobody is driving, while the +// store said `running`, and Enter over it opened a room the engine holds no +// node for. The row says which store task it is now +// ([session.TaskNotice.PlanTask]), and these two tests are the two roads. + +import ( + "strings" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/session" +) + +// planRowFixtureNow is the one clock both roads are read on. +func planRowFixtureNow() time.Time { return time.Date(2026, 9, 21, 12, 0, 0, 0, time.UTC) } + +// planRowChat is the conversation both halves of the work belong to. The dedupe +// is restricted to this window's own rows, so both the node row and the plan +// row have to wear it or the test would be proving nothing. +const planRowChat = "a-conversation" + +// planRowReading reads one reading for a run whose row and whose store task are +// both in front of the place: `storeID` is what the store answered under and +// `planTask` is what the row says it is ("" for a node of this session's own +// tree). BOTH ARE SPELLED THE WAY A STORE ID CROSSES THE SEAM — `t-`, +// which is what [session.PlanTaskRow.ID] carries — because an id spelled two +// ways is an identity that joins nothing, and that is the second half of what +// this defect was. +func planRowReading(t *testing.T, planTask string, storeID string) tasksReading { + t.Helper() + const title = "write HELLO.md containing the word hello" + // The row as the surface holds it: running, because the row the run's door + // published says so, and `working` is the word that reading comes out as. + live := session.ProjectTask(session.TaskFacts{State: session.TaskRunning, Liveness: session.TaskLivenessHeld}) + node := tasksMineRow{ + entry: session.TaskIndexEntry{ + ID: "7", Label: title, Title: title, + Status: string(session.TaskRunning), SessionID: planRowChat, + StartedAt: planRowFixtureNow().Add(-2 * time.Minute), + }, + runs: true, + live: &live, + planTask: planTask, + } + // And the store's own read of the same work: `claimed`, which is a worker + // holding the task, and the word that maps onto `running`. + store := session.PlanTaskRow{ + ID: storeID, Title: title, Status: "claimed", + Started: planRowFixtureNow().Add(-2 * time.Minute), + } + mine := tasksMine{ + row: session.SessionRow{ID: planRowChat, Title: "the conversation", At: planRowFixtureNow()}, + rows: []tasksMineRow{node}, + plan: []session.PlanTaskRow{store}, + } + return readTasks(session.World{}, mine, session.LastDays(planRowFixtureNow(), 10), tasksSort{}, + time.Time{}, planRowFixtureNow()) +} + +// planRowItemOf is the one item of a reading whose title carries these words, or +// the zero item and false when the place read none. +func planRowItemOf(reading tasksReading, words string) (tasksItem, int) { + var found tasksItem + count := 0 + for _, item := range reading.items { + if strings.Contains(item.entry.Title, words) { + found, count = item, count+1 + } + } + return found, count +} + +// THE RUN'S ROW IS DRAWN AS THE STORE'S TASK, because the store is the half +// that answers for it: the state word it wears is the store's, and the page +// Enter opens is the store's own — the description its worker was given and the +// trajectory of every command it ran. +// +// THE ROW WEARING `working` IS THE WHOLE DEFECT. It is the reading of a +// [session.TaskNode] nobody is driving, on a row the graph holds no node for, +// and it is what the tmux drive read off the real screen. +func TestARunsRowIsDrawnAsTheStoreTaskItSaysItIs(t *testing.T) { + reading := planRowReading(t, "t-7", "t-7") + item, drawn := planRowItemOf(reading, "HELLO.md") + if drawn != 1 { + t.Fatalf("the place draws the run %d times, want once: the row and its store task are one "+ + "piece of work read from two ends", drawn) + } + if item.plan == nil { + t.Fatalf("the run's row is the node row and not the store's plan row, so enter over it opens a "+ + "room the engine holds no node for. The row drawn is %+v", item.entry) + } + if got := item.status().Word; got != "running" { + t.Fatalf("the run's row wears %q, want %q — the word its store status maps to (planStateWord); "+ + "`working` is the engine's reading of a node nobody is driving", got, "running") + } + // AND THE DRAWN LINE SAYS IT, because a word on a reading nothing paints is + // not a word a person reads. + row := tasksDrawnRow(tasksPage(reading, 140), "HELLO.md") + if !strings.Contains(row, "running") { + t.Fatalf("the run's drawn row reads\n %s\nand it must wear `running`", row) + } +} + +// AND A PLAN-BORN NODE IS STILL DRAWN AS THE NODE, which is the dedupe this +// change must not break. Its row says no store task — the node road's link +// lives on the node's own spec and never on the row — so the place matches the +// two halves on the title they share and keeps the half with a room behind it. +func TestAPlanBornNodeIsStillDrawnAsItsNodeRow(t *testing.T) { + reading := planRowReading(t, "", "t-kq3f7a") + item, drawn := planRowItemOf(reading, "HELLO.md") + if drawn != 1 { + t.Fatalf("the place draws the plan-born node %d times, want once", drawn) + } + if item.plan != nil { + t.Fatalf("the plan-born node is drawn as its store task, and the node is the half with a room " + + "behind it: the title dedupe (planRowShown) is what keeps the place drawing it once") + } +} + +// A ROW WHOSE STORE TASK THE PLAN READ DOES NOT HOLD IS STILL DRAWN. The read +// may not have landed, and a finished plan is archived out from under its own +// rows — dropping the row on the strength of an identity nothing answers for +// would take the run off the page altogether. +func TestARunsRowSurvivesAPlanReadThatDoesNotHoldIt(t *testing.T) { + reading := planRowReading(t, "t-7", "t-someother") + item, drawn := planRowItemOf(reading, "HELLO.md") + if drawn != 1 { + t.Fatalf("the place draws the run %d times, want once", drawn) + } + if item.plan != nil { + t.Fatalf("the run's row was replaced by a store task that is not the one it named") + } +} + +// AND THE IDENTITY SURVIVES THE WHOLE SURFACE, which is the half a reading built +// by hand cannot prove: the notice the run's door publishes reaches +// [app.taskUpdate], the node keeps the store task it named, [app.taskSheetMine] +// carries it onto the row, and the place reads it there. Every one of those four +// is a place the fact can be dropped, and the tmux drive that found this defect +// is the only other thing that walks all four. +func TestTheRunsRowCarriesItsStoreTaskThroughTheWholeSurface(t *testing.T) { + const title = "write HELLO.md containing the word hello" + root := session.PlanTaskRow{ID: "t-1", Title: title, Status: "claimed"} + a, _ := planAppWith(t, []session.PlanTaskRow{root}, nil) + drive(t, a, streamEventMsg{gen: a.gen, ev: update(1, title, session.TaskRunning, + session.TaskNotice{PlanTask: "t-1"})}) + if node := a.tasks[1]; node == nil || node.planTask != "t-1" { + t.Fatalf("the node kept planTask=%q, want \"t-1\": the run's row said which store task it is", + func() string { + if a.tasks[1] == nil { + return "" + } + return a.tasks[1].planTask + }()) + } + mine := a.taskSheetMine() + found := false + for _, row := range mine.rows { + if strings.Contains(row.entry.Title, "HELLO.md") { + found = true + if row.planTask != "t-1" { + t.Fatalf("the place's own row carries planTask=%q, want \"t-1\"", row.planTask) + } + } + } + if !found { + t.Fatalf("the place holds no row for the run at all; it holds %d rows and %d plan rows", + len(mine.rows), len(mine.plan)) + } + if len(mine.plan) != 1 { + t.Fatalf("the place holds %d plan rows, want the one the store answered", len(mine.plan)) + } + reading := readTasks(session.World{}, mine, session.LastDays(a.now(), 10), tasksSort{}, time.Time{}, a.now()) + item, drawn := planRowItemOf(reading, "HELLO.md") + if drawn != 1 || item.plan == nil { + t.Fatalf("the place draws the run %d times and item.plan=%v, want once as the store's own row", + drawn, item.plan != nil) + } + if got := item.status().Word; got != "running" { + t.Fatalf("the run's row wears %q, want %q", got, "running") + } +} diff --git a/internal/tui3/task.go b/internal/tui3/task.go index 0c04a78db2..8eb2375482 100644 --- a/internal/tui3/task.go +++ b/internal/tui3/task.go @@ -177,6 +177,18 @@ type taskNode struct { // only run; a child carries both, so the same door opens the run's page at // the card this row represents. Empty is an ordinary task, unchanged. run, node string + // planTask is WHICH TASK OF THE RUN'S PLAN STORE this row is + // (session's TaskNotice.PlanTask), and it is set on the rows the run's door + // publishes and on nothing else. Empty is an ordinary node of this + // session's own tree, which is every row until a `/task` takes the run road. + // + // IT IS AN IDENTITY THIS SURFACE COULD NOT WORK OUT FOR ITSELF. A row the + // run's door published wears the store root's own title, so the tasks place + // matched the two halves on those words and drew the row the engine holds no + // node for — whose Enter opens an empty room. The store says which task it + // is now, so the place can draw the half the store answers for + // (taskplan.go's [planStoreDraws]). + planTask string // stopped says a PERSON ended this node rather than the work ending on its // own (session's TaskNotice.Stopped). It rides beside the state rather than // replacing it — a stopped node still settles as failed — and it is what the @@ -5914,6 +5926,13 @@ func (a *app) taskUpdate(ev session.Event) tea.Cmd { if notice.Node != "" { node.node = notice.Node } + // AND WHICH STORE TASK THIS ROW IS, kept on the run door's own rule: the + // identity was settled when the row was minted and is true for the row's + // whole life, so an update quiet about it has not turned a run's row back + // into a node of this session's tree ([taskNode.planTask]). + if notice.PlanTask != "" { + node.planTask = notice.PlanTask + } if notice.Branch != "" { node.branch = notice.Branch } diff --git a/internal/tui3/taskplan.go b/internal/tui3/taskplan.go index 80e79dd280..5cfef1407b 100644 --- a/internal/tui3/taskplan.go +++ b/internal/tui3/taskplan.go @@ -755,12 +755,18 @@ func planTitleFor(title string) string { return strings.ToLower(strings.TrimSpac // back over it. So the store row and the node row are one piece of work read // from two ends, and the place draws it ONCE. // -// THIS SURFACE CANNOT SEE THE STORE ID, and it does not need to: it can see -// this conversation's own node rows, and a plan-born node wears the store task's -// own title — the store is seeded with the node's title and every later task is -// added under it. So the two are matched on the title the pair cannot disagree -// about, restricted to this conversation's rows so another chat's work wearing -// the same words cannot hide a plan row. +// A PLAN-BORN NODE DOES NOT SAY WHICH STORE TASK IT IS, and it does not need +// to: this surface can see this conversation's own node rows, and a plan-born +// node wears the store task's own title — the store is seeded with the node's +// title and every later task is added under it. So the two are matched on the +// title the pair cannot disagree about, restricted to this conversation's rows +// so another chat's work wearing the same words cannot hide a plan row. +// +// A ROW THAT DOES NAME ITS STORE TASK IS NOT MATCHED HERE AT ALL. The run's +// door publishes a row for work the graph holds no node for, and it says which +// task of the store that row is ([session.TaskNotice.PlanTask]) — so those rows +// are taken out by identity before this runs ([planStoreDraws]), and the title +// guess is left to the road that has nothing better. func planRowShown(names map[string]bool, title string) bool { if len(names) == 0 { return false @@ -787,6 +793,52 @@ func planNamesOf(rows []tasksMineRow, chat string) map[string]bool { return out } +// planStoreDraws is this conversation's own rows with the ones THE STORE IS THE +// AUTHORITY FOR taken out, and it is the first thing the tasks place's reading +// does with them. +// +// A ROW THE RUN'S DOOR PUBLISHED IS NOT A NODE. The door that takes the run +// road never admits a node into the graph — it seeds a plan store, names the +// store's task with the number the person was answered with, and publishes a +// row under that number ([session.Agent.startKnownTaskRun]). So the store and +// that row are one piece of work read from two ends, and unlike the node road +// the surface is TOLD which two ([session.TaskNotice.PlanTask], carried onto +// the row by place_tasks.go). +// +// AND THE HALF THAT IS DRAWN IS THE STORE'S, for two reasons that are the same +// reason. The store's status is what the run actually moves — the published row +// wears the engine's own word for a node nobody is driving, so the pair could +// not even agree on the state — and Enter over a plan row opens the page with +// the worker's trajectory on it ([app.taskSheetPlan]), where Enter over the +// published row opens a room the engine holds no node for and so draws nothing. +// A CAPABILITY THAT CANNOT WORK IS ABSENT, NOT BROKEN: the row that opens an +// empty room is not drawn. +// +// A ROW WHOSE TASK THE PLAN READ DOES NOT HOLD STAYS. The read may not have +// landed yet, and the run's store is archived the moment a finished plan is +// replaced ([session.Agent.openBeltRunStore]) — dropping a row on the strength +// of an identity nothing answers for would take the run off the page +// altogether, which is worse than the row it replaces. +func planStoreDraws(rows []tasksMineRow, plan []session.PlanTaskRow) []tasksMineRow { + if len(rows) == 0 || len(plan) == 0 { + return rows + } + held := make(map[string]bool, len(plan)) + for _, task := range plan { + if id := strings.TrimSpace(task.ID); id != "" { + held[id] = true + } + } + kept := make([]tasksMineRow, 0, len(rows)) + for _, row := range rows { + if held[strings.TrimSpace(row.planTask)] { + continue + } + kept = append(kept, row) + } + return kept +} + // ── THE PAGE ONE PLAN ROW OPENS ───────────────────────────────────────────── // taskSheetPlan opens the page over one plan row: the description the worker was diff --git a/internal/tui3/tasksplace.go b/internal/tui3/tasksplace.go index 43a2dd8378..e07c4afe4f 100644 --- a/internal/tui3/tasksplace.go +++ b/internal/tui3/tasksplace.go @@ -165,6 +165,12 @@ type tasksMineRow struct { entry session.TaskIndexEntry runs bool live *session.TaskStatus + // planTask is WHICH TASK OF THE RUN'S PLAN STORE this row is, when the row + // is one the run's door published ([taskNode.planTask]). It rides the row + // rather than the entry because it is a fact about the LIVE row and not + // about the project's record: the index has never carried a store id and a + // row written to it would be claiming one for work whose store has gone. + planTask string } // tasksReading is everything drawing and routing need from one world reading. @@ -262,6 +268,14 @@ func tasksKeyOf(entry session.TaskIndexEntry) tasksKey { // seconds ago and are the only authority for work that has not landed. func readTasks(world session.World, mine tasksMine, win session.UsageWindow, by tasksSort, seen, now time.Time) tasksReading { r := tasksReading{win: win.Normalized(), seen: seen, now: now, summaryNow: strings.TrimSpace(mine.now), tilde: mine.tilde, order: by} + // THE ROWS THE RUN'S STORE ANSWERS FOR ARE TAKEN OUT FIRST, before anything + // reads them: they are drawn as the store's own plan rows below, and this is + // the one line that keeps the pair from being drawn twice or drawn as the + // wrong half (taskplan.go's [planStoreDraws] says which half and why). It is + // done here rather than in either loop so the title dedupe further down sees + // the same set of rows the page does — a run's row left in the name set + // would hide its own plan row. + mine.rows = planStoreDraws(mine.rows, mine.plan) // order keeps the pass stable: a map alone would re-order the page on every // frame it was rebuilt, and the sections below are drawn in the order the // rows arrived within each one.