diff --git a/docs/changes/unreleased/1517-task-room-paths.md b/docs/changes/unreleased/1517-task-room-paths.md new file mode 100644 index 000000000..74722004d --- /dev/null +++ b/docs/changes/unreleased/1517-task-room-paths.md @@ -0,0 +1,16 @@ +--- +kind: fixed +title: runs land every file by its real name, --one-model reaches every run seat, caches stay out +pr: 1517 +surface: [chat, engine] +invalidates: + - "A run's landing read `git status --porcelain` without `-z`, so a name git quotes (an accent, a quote, a newline, `a -> b`) was dropped from the commit without a word. Every reader of paths on the landing road now reads git's NUL form, and those files land." + - "The work tab took a file's name from the last ` b/` in its diff header, so `x b/plandb.db` was hidden as codeaf's own store and an accented name drew as octal escapes. One reader (`session.PatchSectionPath`) now reads the name the way git's own apply does, for both the filter and the room." + - "Under `--one-model` a `/task` run's worker, planner, checker and probe were seated from the profile's crew rows and `CODEAF_CHECK_MODEL`. Every seat now rides the conversation's live model, and the environment rung stands down under the flag." + - "A run's landing committed every untracked file, including `__pycache__/*.pyc`. An untracked file matching `__pycache__/`, `*.pyc`, `.pytest_cache/`, `.mypy_cache/`, `.ruff_cache/` or `.DS_Store` now stays out; a tracked or worker-staged one still lands." + - "`the worker reads a note at its next step` was two constants in two packages. It is one, `session.RunNotePickupWord`." +--- + +The belt worker's page also tells a worker to delete its scratch files before +`plandb done`, because a fresh-install run landed a `calc_work.py` it had made to +compare against HEAD. diff --git a/internal/manual/chat/models-and-cost.md b/internal/manual/chat/models-and-cost.md index 0d16fe18a..6ecd6af8d 100644 --- a/internal/manual/chat/models-and-cost.md +++ b/internal/manual/chat/models-and-cost.md @@ -1157,7 +1157,9 @@ other, timing a benchmark cell, or attributing a cost. It settles four things on your model: the crew's three seats and the two small rows, any role you pinned, the model that work leaving the conversation runs on, and the fallback chain codeaf would otherwise -move to when a model cannot answer. Under this flag **nothing hops** — not on a refusal, +move to when a model cannot answer. That includes every seat of a `/task` run — its worker, +planner, checker and probe — whatever your crew rows say, and `CODEAF_CHECK_MODEL` stands +down too. Under this flag **nothing hops** — not on a refusal, not on a reply that keeps stalling, not on rate limiting that will not clear — because a run whose cost is being attributed to one model cannot have finished a single reply on another. That includes the catalog's own guess: with no `fallback models` row written, an diff --git a/internal/manual/chat/reading-a-task-page.md b/internal/manual/chat/reading-a-task-page.md index 47ed4f7dc..5a238095b 100644 --- a/internal/manual/chat/reading-a-task-page.md +++ b/internal/manual/chat/reading-a-task-page.md @@ -193,6 +193,9 @@ changed. `the files this task changes are listed here when it lands`. - On a run's task, `work` is the run's working copy read against the commit it was cut from: the difference, and the files added, with the files codeaf keeps there left out. + Every file is listed under its own name, including spaces, quotes and accents. Only + codeaf's own files are left out: `plandb.db` and the files beside it at the copy's + top, the `.codeaf` folder and the `bin/plandb` shim. The whole run shares that one copy, and the tab says so at its top. Once the run has ended its copy is given back, and the tab reads the work off the run's branch instead. With that branch gone too, it says `the run's working copy is not here any more`. diff --git a/internal/manual/chat/worker-harness.md b/internal/manual/chat/worker-harness.md index 40f4d53e4..fc8fa1084 100644 --- a/internal/manual/chat/worker-harness.md +++ b/internal/manual/chat/worker-harness.md @@ -42,6 +42,10 @@ that will not go in is never forced: the branch is kept in your repository and t names it, for example `its branch was kept`, when your checkout moved on after the copy was cut. A run whose workers committed everything still names its branch and changed files; codeaf signs those commits before the work comes home. +A file lands under its own name whatever characters it holds, including spaces, quotes, +accents or a newline; only codeaf's own `plandb.db` and the files beside it, `.codeaf/` +files, and untracked build caches stay out of the landing commit (see *Why a task's +commit has no __pycache__* below). A run that only read says `nothing to land: the run's working copy holds no change` and changes no file. The landing card says `merged` when the work is in your folder and `branch kept` only for a @@ -54,6 +58,26 @@ row the run was published under settles `done` when the run finished whole and `CODEAF_TASK_BELT=node` to use the older session tree road instead. See *How to turn it off*. +## Why a task's commit has no __pycache__ or .pyc files, and no scratch files — what a landing leaves out + +A run's landing commits the files its copy holds, except three kinds: + +- codeaf's own files: `plandb.db` and the files beside it, and `.codeaf/`; +- **untracked build caches**, the ones the interpreter and the test tools write while a + worker runs your tests: `__pycache__/`, `*.pyc`, `.pytest_cache/`, `.mypy_cache/`, + `.ruff_cache/` and `.DS_Store`. These are matched by exact name, never by resemblance, so + `poetry.lock`, `bench-results/` or `pycache_notes.md` still land; +- nothing else. + +**A cache your repository already tracks still lands when it changes**, and so does one the +worker staged with `git add` or committed itself. Only a cache file git has never been told +about stays out. The task's `work` tab leaves the same files out of its list, so it never +shows a file that will not come home. + +**Scratch copies are the worker's to clean up.** A worker is told to delete every scratch +file it made in its copy before it reports done, because anything left there lands on your +branch. + ## The tasks pane and a task's page While a run is live the task pane draws its **plan**: one row per task in the @@ -714,7 +738,9 @@ named on a door or in the profile: The seat a person names is the seat **every** launch takes — a task launched after the door resolved the seats still runs on them, not on whichever row the -profile happens to hold. Read it back with `plandb spend --by seat`. +profile happens to hold. Read it back with `plandb spend --by seat`. A conversation +started with `--one-model` seats all four — work, plan, check and probe — on the model you +are talking to, and neither the crew rows nor `CODEAF_CHECK_MODEL` moves them. ## Headless: codeaf do — the exit code it leaves with diff --git a/internal/manual/chat_test.go b/internal/manual/chat_test.go index c29e0bc80..ebfe19835 100644 --- a/internal/manual/chat_test.go +++ b/internal/manual/chat_test.go @@ -2881,6 +2881,7 @@ func TestTheChatManualAnswersTheQuestionsPeopleAsk(t *testing.T) { // second road for a task and the plan pane a run draws, each asked the // way somebody meets it rather than in the machinery's own words. {"how do I pause a task", "worker-harness"}, + {"why did __pycache__ files not land in my task's commit", "worker-harness"}, {"how do I open one part of a run from the side list", "worker-harness"}, {"can I add a note to a running task", "worker-harness"}, {"why did the task refuse my cancel", "worker-harness"}, diff --git a/internal/run/chat_check_seat_test.go b/internal/run/chat_check_seat_test.go index e385ace8f..ffd5429a8 100644 --- a/internal/run/chat_check_seat_test.go +++ b/internal/run/chat_check_seat_test.go @@ -66,3 +66,84 @@ func TestTheChatDoorsCheckRidesTheCheckModelVariable(t *testing.T) { asked, config.CheckModelEnv) } } + +// UNDER `--one-model` THE RUN HAS ONE SEAT (contract 3a). The chat's door names +// the conversation's model as every seat, and the engine seats every role on it +// — the root's planning, a leaf, a check and a probe — whatever the profile's +// crew rows say and whatever CODEAF_CHECK_MODEL says. The profile rows below +// name OTHER models for every tier, so a seat that fell back to the profile or +// the environment would be seen here. +func TestUnderOneModelEveryRunSeatIsTheConversationsModel(t *testing.T) { + t.Setenv(config.CheckModelEnv, "vendor/env-check") + store := runOpenStore(t) + if _, err := store.AddMany([]plandb.TaskSpec{ + {ID: "one", Title: "One", ParentID: store.RootID()}, + {ID: "review", Title: "Review", Role: plandb.RoleCheck}, + {ID: "discriminate", Title: "Discriminate", Role: plandb.RoleProbe}, + }); err != nil { + t.Fatal(err) + } + dir := crewProfile(t, map[string]string{ + config.KeyTierLowModel: "vendor/profile-small", + config.KeyTierWorkerModel: "vendor/profile-worker", + config.KeyTierHighModel: "vendor/profile-careful", + config.KeyTierMastermindModel: "vendor/profile-thinking", + }) + recorder := &recordingCompleter{} + factory := run.CrewFactory(store, t.TempDir(), dir, run.Seats{One: "vendor/one"}, recorder.forModel) + for _, id := range []string{store.RootID(), "one", "review", "discriminate"} { + recorder.models = nil + factory(*store.Task(id)) + if len(recorder.models) != 1 || recorder.models[0] != "vendor/one" { + t.Errorf("task %s was seated on %v under one model, want vendor/one", id, recorder.models) + } + } +} + +// THE CHAT'S DOOR CARRIES THE ONE SEAT TO THE ENGINE (contract 3a). The spec +// names only OneModel — no work, plan or check seat — so a launch that rode +// anything else read it from the profile or the environment, which is the leak +// the tagged suite billed claude-fable-5.1 through. +func TestTheChatDoorsOneModelReachesEveryLaunch(t *testing.T) { + t.Setenv("CODEAF_TASK_BELT", "bash") + t.Setenv("CODEAF_PLANDB_BIN", stubCLI(t)) + t.Setenv(config.CheckModelEnv, "vendor/env-check") + store := runOpenStore(t) + if _, err := store.AddMany([]plandb.TaskSpec{{ID: "review", Title: "Review", Role: plandb.RoleCheck}}); err != nil { + t.Fatal(err) + } + dir := crewProfile(t, map[string]string{ + config.KeyTierHighModel: "vendor/profile-careful", + config.KeyTierWorkerModel: "vendor/profile-worker", + config.KeyTierMastermindModel: "vendor/profile-thinking", + }) + var mu sync.Mutex + var asked []string + refuse := &seat{ever: func(context.Context, []ai.Message) (*ai.Response, error) { + return nil, errors.New("scripted: no provider behind this seat") + }} + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + run.ChatEngine.Start(ctx, session.RunSpec{ + Store: store, + Workspace: t.TempDir(), + ProfileDir: dir, + OneModel: "vendor/one", + CompleterFor: func(model string) session.Completer { + mu.Lock() + asked = append(asked, model) + mu.Unlock() + return refuse + }, + }) + mu.Lock() + defer mu.Unlock() + if len(asked) == 0 { + t.Fatal("the run launched nothing, so nothing was seated") + } + for _, model := range asked { + if model != "vendor/one" { + t.Fatalf("a launch under one model was seated on %s (all asked: %v)", model, asked) + } + } +} diff --git a/internal/run/crew.go b/internal/run/crew.go index be4cd7ac6..8827bb6ba 100644 --- a/internal/run/crew.go +++ b/internal/run/crew.go @@ -21,6 +21,13 @@ package run // comes from the profile's tiers, and an empty seat falls exactly where an empty // tier always fell. // +// UNDER `--one-model` THE PROFILE DOES NOT ANSWER. A conversation started with +// the flag names its own model as every seat ([Seats.One]), and the factory +// seats every role on it without reading a crew row or the check seat's +// environment rung: the flag promises every text call rides the model the +// person is talking to, and a crew row answering for an empty seat is how a +// run under it once billed two models nobody named. +// // THE READ IS AT LAUNCH, NEVER CACHED. The factory asks the profile again for // every task it seats — for the check and probe rows, and for any seat the door // left empty — so a crew change between two launches (a /crew run in the @@ -80,6 +87,14 @@ type Seats struct { Work string Plan string Check string + // One is the conversation's model under `--one-model`, and when it is set + // it is EVERY seat: the three above, the probe no door names, and any role + // this build has not learned. Nothing here asks the profile or the check + // seat's environment rung while it is set, because the flag promises that + // every text call rides the model the person is talking to — and an empty + // seat falling to the crew row is exactly how a run under it billed models + // nobody named. + One string } // CrewFactory is the run's WorkerFactory: it seats each task in the model its @@ -109,6 +124,12 @@ type Seats struct { // profile is old. func CrewFactory(store *plandb.Store, workspace, profileDir string, seats Seats, completerFor func(model string) session.Completer) WorkerFactory { return func(task plandb.Task) Worker { + // UNDER `--one-model` THERE IS NO TIER TO READ. The door named one model + // for every seat ([Seats.One]), so the role does not matter and neither + // the profile nor the environment is asked. + if seats.One != "" { + return NewBashWorker(store, workspace, seats.One, completerFor(seats.One)) + } // A task the store cannot name — which the supervisor never hands over — // reads as the work seat, the same fallback SeatFor gives an unknown // role, so RoleOf's error needs no reader here. diff --git a/internal/run/enginewire.go b/internal/run/enginewire.go index 16abf3b1d..45dc5bc56 100644 --- a/internal/run/enginewire.go +++ b/internal/run/enginewire.go @@ -50,6 +50,9 @@ func (engine) Start(ctx context.Context, spec session.RunSpec) session.RunSummar Work: spec.WorkModel, Plan: spec.PlanModel, Check: chatCheckSeat(spec.CheckModel), + // AND UNDER `--one-model` ONE MODEL IS EVERY SEAT, the probe and the + // check's environment rung included ([Seats.One]). + One: spec.OneModel, }, spec.CompleterFor), OnSpend: spec.OnSpend, Gate: spec.Admission, diff --git a/internal/run/land_test.go b/internal/run/land_test.go index 434f64f4d..ae8018671 100644 --- a/internal/run/land_test.go +++ b/internal/run/land_test.go @@ -13,6 +13,7 @@ import ( "os/exec" "path/filepath" "reflect" + "slices" "strings" "testing" @@ -97,6 +98,148 @@ func TestLandCommitsTheRunsWorkOntoItsBranch(t *testing.T) { } } +// Contract 4a and 4b: the real landing must carry git-quoted names as their +// exact bytes while leaving the harness's own files outside the commit. +func TestLandKeepsEveryRealFilename(t *testing.T) { + t.Setenv("CODEAF_TASK_BELT", "bash") + repo := t.TempDir() + runGit(t, repo, "init") + runGit(t, repo, "checkout", "-b", "work") + writeRunFile(t, filepath.Join(repo, "x b", "plandb.db"), "one\n") + runGit(t, repo, "add", "x b/plandb.db") + runGit(t, repo, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-m", "seed") + base := strings.TrimSpace(runGitOut(t, repo, "rev-parse", "work")) + work := filepath.Join(t.TempDir(), "run") + runGit(t, repo, "worktree", "add", "-b", "run-work", work, "HEAD") + store := runOpenStore(t) + files := map[string]string{ + "x b/plandb.db": "one\ntwo\n", + " lead.txt": "lead\n", + "odd name é'q.txt": "accent\n", + "has\"quote.txt": "quote\n", + "new\nline.txt": "newline\n", + "a -> b.txt": "arrow\n", + } + seat := newFakeSeat() + seat.actions["root"] = func(_ context.Context, _ plandb.Task) (run.Report, error) { + for path, content := range files { + writeRunFile(t, filepath.Join(work, path), content) + } + writeRunFile(t, filepath.Join(work, "plandb.db"), "private\n") + writeRunFile(t, filepath.Join(work, ".codeaf", "jobs", "x.log"), "private\n") + return run.Report{Result: "wrote six files"}, nil + } + ctx := runContext(t) + if got := run.NewSupervisor(store, work, 1, run.Limits{}, seat.workerFor).Run(ctx); got != run.OutcomeDone { + t.Fatalf("run outcome = %q", got) + } + landing, err := run.Land(ctx, store, work, base, store.RootID()) + if err != nil || landing.Refused != "" || landing.Branch != "run-work" { + t.Fatalf("landing = %+v, error = %v", landing, err) + } + want := make([]string, 0, len(files)) + for name := range files { + want = append(want, name) + } + slices.Sort(want) + if !reflect.DeepEqual(landing.Changed, want) { + t.Fatalf("landing.Changed = %q, want %q", landing.Changed, want) + } + listed := strings.Split(runGitOut(t, repo, "ls-tree", "-r", "-z", "--name-only", landing.Branch), "\x00") + for name, content := range files { + if !slices.Contains(listed, name) { + t.Errorf("landing tree omits %q: %q", name, listed) + } + if got := runGitOut(t, repo, "show", landing.Branch+":"+name); got != content { + t.Errorf("landed %q = %q, want %q", name, got, content) + } + } + for _, name := range []string{"plandb.db", ".codeaf/jobs/x.log"} { + if slices.Contains(listed, name) { + t.Errorf("landing tree includes harness file %q", name) + } + } + if notes := store.Notes(store.RootID(), 0); len(notes) == 0 || notes[len(notes)-1].Body != "landed on run-work: 6 files" { + t.Fatalf("root notes = %v, want six-file landing", notes) + } +} + +// A LANDING LEAVES OUT UNTRACKED BUILD CACHES AND NOTHING ELSE (contract 5a, 5b +// and 5c). A fresh-install run in a Python repository with no .gitignore landed +// the interpreter's new `__pycache__/*.pyc` files beside its one-line fix. The +// caches a run leaves untracked stay out; a cache file the repository already +// tracks, and one the worker staged itself, are work and land; and every name +// that merely resembles a cache — a lockfile, a folder the project owns, a file +// whose name contains the word — lands as it always did. +func TestLandLeavesOutUntrackedBuildCachesOnly(t *testing.T) { + t.Setenv("CODEAF_TASK_BELT", "bash") + repo := t.TempDir() + runGit(t, repo, "init") + runGit(t, repo, "checkout", "-b", "work") + writeRunFile(t, filepath.Join(repo, "calc.py"), "def add(a, b):\n return a - b\n") + writeRunFile(t, filepath.Join(repo, "__pycache__", "calc.cpython-310.pyc"), "tracked cache\n") + runGit(t, repo, "add", "calc.py", "__pycache__/calc.cpython-310.pyc") + runGit(t, repo, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-m", "seed") + base := strings.TrimSpace(runGitOut(t, repo, "rev-parse", "work")) + work := filepath.Join(t.TempDir(), "run") + runGit(t, repo, "worktree", "add", "-b", "run-work", work, "HEAD") + store := runOpenStore(t) + lands := map[string]string{ + "calc.py": "def add(a, b):\n return a + b\n", + "__pycache__/calc.cpython-310.pyc": "recompiled\n", + "notes/.DS_Store": "staged on purpose\n", + "poetry.lock": "lock\n", + "package-lock.json": "{}\n", + "bench-results/x": "result\n", + "pycache_notes.md": "notes\n", + "src/__pycache__helper.py": "helper\n", + "my.pyc.txt": "text\n", + "DS_Store.md": "doc\n", + } + caches := []string{ + "__pycache__/test_calc.cpython-310.pyc", + "pkg/__pycache__/m.pyc", + ".pytest_cache/v/cache/nodeids", + ".mypy_cache/x", + ".ruff_cache/x", + ".DS_Store", + "lib/old.pyc", + } + seat := newFakeSeat() + seat.actions["root"] = func(_ context.Context, _ plandb.Task) (run.Report, error) { + for path, content := range lands { + writeRunFile(t, filepath.Join(work, path), content) + } + for _, path := range caches { + writeRunFile(t, filepath.Join(work, path), "cache\n") + } + runGit(t, work, "add", "notes/.DS_Store") + return run.Report{Result: "fixed add"}, nil + } + ctx := runContext(t) + if got := run.NewSupervisor(store, work, 1, run.Limits{}, seat.workerFor).Run(ctx); got != run.OutcomeDone { + t.Fatalf("run outcome = %q", got) + } + landing, err := run.Land(ctx, store, work, base, store.RootID()) + if err != nil || landing.Refused != "" { + t.Fatalf("landing = %+v, error = %v", landing, err) + } + want := make([]string, 0, len(lands)) + for name := range lands { + want = append(want, name) + } + slices.Sort(want) + if !reflect.DeepEqual(landing.Changed, want) { + t.Fatalf("landing.Changed = %q, want %q", landing.Changed, want) + } + listed := strings.Split(runGitOut(t, repo, "ls-tree", "-r", "-z", "--name-only", landing.Branch), "\x00") + for _, name := range caches { + if slices.Contains(listed, name) { + t.Errorf("the landing carried the untracked build cache %q", name) + } + } +} + // TestLandRefusesARunThatWroteNothing is the other ending of the same door: a // run that only read is a refusal with no branch, and the root says so. func TestLandRefusesARunThatWroteNothing(t *testing.T) { diff --git a/internal/session/agent.go b/internal/session/agent.go index e260c7c0c..37f1f9860 100644 --- a/internal/session/agent.go +++ b/internal/session/agent.go @@ -1617,13 +1617,17 @@ const ( // sent, and the sentence says that rather than reporting a second delivery // that did not happen. steerAgainWord = "already on the task's record from the same message — nothing was sent a second time" - // steerRunNoteWord is what a note on a run's own row answers. A run's task - // has no worker to splice a line into. The words are a note on the task, - // and the worker reads a note at its next step, which is the same sentence - // the task room says once the store has the note. Saying the note arrived - // now would claim a read that has not happened (stoprun.go's - // [Agent.sayToRunRow]). - steerRunNoteWord = "the worker reads a note at its next step" + // RunNotePickupWord is WHEN a note on a run's task is read, and it is ONE + // SENTENCE IN TWO PLACES: the receipt a note typed at a run's own row + // answers in the chat (stoprun.go's [Agent.sayToRunRow]), and the line the + // task room writes under the note once the store has it (internal/tui3's + // taskPlanPickupWord takes it from here). A run's task has no worker to + // splice a line into; a worker is a separate loop, so the words wait in the + // store as a note until the worker asks for its next step. Saying the note + // arrived now would claim a read that has not happened. It is exported + // because the room must say the same thing about the same note, and the + // manual quotes it exactly (worker-harness.md, task-controls.md). + RunNotePickupWord = "the worker reads a note at its next step" ) // steerRecord is what the JOURNAL keeps about this line when it is a correction diff --git a/internal/session/bashbelt_worker_test.go b/internal/session/bashbelt_worker_test.go index 8d8fee1f6..b955244e0 100644 --- a/internal/session/bashbelt_worker_test.go +++ b/internal/session/bashbelt_worker_test.go @@ -184,3 +184,21 @@ func TestBashWorkerPageRequiresChecksForDelegatedTasks(t *testing.T) { t.Fatalf("the worker page permits a delegated task without Checks:\n%s", doc) } } + +// THE WORKER CLEANS UP ITS OWN SCRATCH BEFORE IT REPORTS DONE (contract 5e). A +// run's landing commits what the copy holds, and a worker that compared its fix +// against the commit by copying calc.py to calc_work.py inside the copy put that +// copy on the person's branch. The page names the deletion owed before +// `plandb done`, in the few bytes the belt's prompt law leaves it. +func TestBashWorkerPageSaysToDeleteScratchBeforeDone(t *testing.T) { + // The page is wrapped for reading, so a sentence is compared with its + // line breaks folded to the spaces they stand for. + doc := strings.Join(strings.Fields(bashWorkerPage()), " ") + for _, want := range []string{ + "Delete every scratch file you made in your copy before `plandb done`: it would land.", + } { + if !strings.Contains(doc, want) { + t.Fatalf("the worker page does not say %q:\n%s", want, doc) + } + } +} diff --git a/internal/session/build_caches.go b/internal/session/build_caches.go new file mode 100644 index 000000000..f4ff752e7 --- /dev/null +++ b/internal/session/build_caches.go @@ -0,0 +1,66 @@ +package session + +// build_caches.go is the one list of BUILD CACHES a run's landing leaves out +// when the run leaves them untracked, and the rule that reads it. +// +// A RUN'S LANDING COMMITS WHAT THE COPY HOLDS ([beltTreeWork]), because a shell +// worker keeps no ledger and git's status is the record. In a repository with +// no .gitignore, that record includes what the interpreter and the tools wrote +// while the worker ran the tests: a fresh-install run landed its one-line fix +// with two new `__pycache__/*.pyc` files beside it, on the person's branch. +// Those files are never anybody's work. The machine made them, the next run +// makes them again, and no reviewer wants them in a diff. +// +// IT IS NOT [harnessWrote], AND IT IS NARROWER ON PURPOSE. That rule answers +// which paths are codeaf's own, and its law is that a path is the harness's +// only because the harness wrote it there. A rule that went by what a file of +// that kind is usually called dropped every lockfile a run changed. This list +// is a different question, which files are caches, answered by exact names +// and never by a resemblance: +// - an entry ending in `/` is a directory, matched as a whole path segment +// anywhere in the path; +// - an entry starting with `*` is a file's ending, matched on the basename; +// - any other entry is a whole basename. +// And it applies ONLY to an untracked file. A cache the repository already +// tracks lands when it changes, and one the worker staged or committed itself +// lands too: that is the only way a shell worker says it meant a file. + +import "strings" + +// buildCacheNames is THE LIST, spelled the way the manual quotes it. +var buildCacheNames = []string{ + "__pycache__/", + "*.pyc", + ".pytest_cache/", + ".mypy_cache/", + ".ruff_cache/", + ".DS_Store", +} + +// buildCache says whether a slash-separated path inside a working copy is one +// of the caches [buildCacheNames] lists. A path whose last segment only +// shares a cache's name as part of a longer one — `pycache_notes.md`, +// `my.pyc.txt`, `DS_Store.md` — is not. +func buildCache(path string) bool { + segments := strings.Split(path, "/") + base := segments[len(segments)-1] + folders := segments[:len(segments)-1] + for _, name := range buildCacheNames { + switch { + case strings.HasSuffix(name, "/"): + for _, folder := range folders { + if folder == strings.TrimSuffix(name, "/") { + return true + } + } + case strings.HasPrefix(name, "*"): + ending := strings.TrimPrefix(name, "*") + if len(base) > len(ending) && strings.HasSuffix(base, ending) { + return true + } + case base == name: + return true + } + } + return false +} diff --git a/internal/session/build_caches_test.go b/internal/session/build_caches_test.go new file mode 100644 index 000000000..0f4bc74de --- /dev/null +++ b/internal/session/build_caches_test.go @@ -0,0 +1,65 @@ +package session + +import ( + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/manual" +) + +// THE BUILD-CACHE RULE MATCHES WHOLE NAMES AND NOTHING THAT RESEMBLES THEM +// (contract 5a and 5c). The harness-files rule beside it once dropped every +// lockfile for matching a suffix, so the near-misses here are the half that +// matters: each one is a project's own file and must stay work. +func TestBuildCacheMatchesOnlyTheNamedCaches(t *testing.T) { + for path, want := range map[string]bool{ + "__pycache__/calc.cpython-310.pyc": true, + "pkg/__pycache__/m.cpython-310.pyc": true, + "lib/old.pyc": true, + ".pytest_cache/v/cache/nodeids": true, + "a/.mypy_cache/3.12/x.json": true, + ".ruff_cache/0.5/abc": true, + ".DS_Store": true, + "docs/.DS_Store": true, + "calc.py": false, + "poetry.lock": false, + "package-lock.json": false, + "bench-results/x": false, + "pycache_notes.md": false, + "src/__pycache__helper.py": false, + "my.pyc.txt": false, + "DS_Store.md": false, + "__pycache__": false, + "notes/.pytest_cache.md": false, + } { + if got := buildCache(path); got != want { + t.Errorf("buildCache(%q) = %v, want %v", path, got, want) + } + } +} + +// THE WORK TAB DOES NOT LIST A CACHE THE LANDING WILL LEAVE OUT (contract 5d), +// because the tab is the person's preview of what comes home. +func TestReadPlanWorkLeavesOutUntrackedBuildCaches(t *testing.T) { + repo := newTestRepo(t) + base := strings.TrimSpace(gitOut(t, repo, "rev-parse", "HEAD")) + writeFile(t, filepath.Join(repo, "__pycache__", "calc.cpython-310.pyc"), "cache\n") + writeFile(t, filepath.Join(repo, ".DS_Store"), "cache\n") + writeFile(t, filepath.Join(repo, "pycache_notes.md"), "work\n") + work := readPlanWork(&TaskCopyRecord{Dir: repo, HomeSha: base}) + if !slices.Equal(work.Added, []string{"pycache_notes.md"}) { + t.Fatalf("added = %q, want only pycache_notes.md", work.Added) + } +} + +// THE MANUAL NAMES THE LIST AS THE CODE SPELLS IT (contract 5f), so a person +// asking why a file did not land reads the same names the landing leaves out. +func TestTheManualNamesEveryBuildCache(t *testing.T) { + for _, name := range buildCacheNames { + if !manual.Chat().Mentions("`" + name + "`") { + t.Errorf("no chat manual page names the build cache `%s`", name) + } + } +} diff --git a/internal/session/git_patch_path.go b/internal/session/git_patch_path.go new file mode 100644 index 000000000..79a932744 --- /dev/null +++ b/internal/session/git_patch_path.go @@ -0,0 +1,169 @@ +package session + +// git_patch_path.go is THE ONE READER OF WHICH FILE A PATCH SECTION IS ABOUT, +// shared by the work tab's filter here ([planWorkPatch]) and the task room that +// draws the same patch (internal/tui3's planWorkLines). +// +// A PATCH HAS NO NUL FORM. `-z` changes git's name lists and never a unified +// diff, so the name has to be read off the section itself, and it is read the +// way git's own `apply` reads it. Taking the text after the last ` b/` in the +// `diff --git` line was the old reading, and it read a person's +// `x b/plandb.db` as `plandb.db`: the harness filter then dropped the file as +// codeaf's own store, and the room drew the wrong name. A name git quoted came +// through as its octal escapes. +// +// ONE READER, BECAUSE TWO WOULD DISAGREE. The filter that decides a section is +// codeaf's own and the label a person reads over it are the same question, and +// the day they were answered twice was the day a file could be hidden under +// one name and drawn under another. + +import "strings" + +// PatchSections splits a unified patch into one piece per file, each starting +// at its `diff --git` line, so a file's header travels with its body. +func PatchSections(patch string) []string { + var sections []string + start := -1 + for at := 0; at < len(patch); { + end := strings.IndexByte(patch[at:], '\n') + line := patch[at:] + next := len(patch) + if end >= 0 { + line = patch[at : at+end] + next = at + end + 1 + } + if strings.HasPrefix(line, "diff --git ") { + if start >= 0 { + sections = append(sections, patch[start:at]) + } + start = at + } + at = next + } + if start >= 0 { + sections = append(sections, patch[start:]) + } else if strings.TrimSpace(patch) != "" { + sections = append(sections, patch) + } + return sections +} + +// PatchSectionPath is the path one section is about, as the file stands now. +// +// A rename or copy says its new name on a line of its own (`rename to`, +// `copy to`), which is never ambiguous, so that line wins. Otherwise the +// `diff --git` line names the file twice. When git quoted it, the second +// quoted token is the name. When it did not, the two halves are the same +// name, so the line is measured rather than split: `a/P b/P` is two plus P, +// three, and P again, which finds P even when P itself holds ` b/`. +func PatchSectionPath(section string) string { + head, rest, _ := strings.Cut(section, "\n") + for rest != "" { + line, tail, _ := strings.Cut(rest, "\n") + if strings.HasPrefix(line, "--- ") || strings.HasPrefix(line, "+++ ") || + strings.HasPrefix(line, "@@") || strings.HasPrefix(line, "Binary files") { + break + } + for _, prefix := range []string{"rename to ", "copy to "} { + if strings.HasPrefix(line, prefix) { + name := strings.TrimPrefix(line, prefix) + if decoded, ok := unquoteGitPath(name); ok { + return decoded + } + return name + } + } + rest = tail + } + name := strings.TrimPrefix(head, "diff --git ") + if strings.HasPrefix(name, `"`) { + if end := quotedGitTokenEnd(name); end > 0 && end < len(name) && name[end] == ' ' { + destination := name[end+1:] + if strings.HasPrefix(destination, `"`) { + if tokenEnd := quotedGitTokenEnd(destination); tokenEnd > 0 { + destination = destination[:tokenEnd] + } + } + if decoded, ok := unquoteGitPath(destination); ok { + return strings.TrimPrefix(decoded, "b/") + } + return strings.TrimPrefix(destination, "b/") + } + } + if len(name) >= 5 && (len(name)-5)%2 == 0 { + n := (len(name) - 5) / 2 + if strings.HasPrefix(name, "a/") && name[2+n:5+n] == " b/" && name[2:2+n] == name[5+n:] { + return name[5+n:] + } + } + // Older patch engines can write two unequal bare halves without extended + // rename headers; their last destination prefix is the best available path. + if at := strings.LastIndex(name, " b/"); at >= 0 { + return name[at+3:] + } + return name +} + +// quotedGitTokenEnd finds a C-quoted token's closing quote without mistaking +// an escaped quote for the end of the path. +func quotedGitTokenEnd(s string) int { + if len(s) == 0 || s[0] != '"' { + return -1 + } + for i := 1; i < len(s); i++ { + if s[i] == '\\' { + i++ + continue + } + if s[i] == '"' { + return i + 1 + } + } + return -1 +} + +// unquoteGitPath decodes Git's C quoting byte for byte, including octal UTF-8 +// bytes, so the returned string is the filename Git actually saw. +func unquoteGitPath(s string) (string, bool) { + if len(s) < 2 || s[0] != '"' || s[len(s)-1] != '"' { + return "", false + } + var out strings.Builder + for i := 1; i < len(s)-1; i++ { + if s[i] != '\\' { + out.WriteByte(s[i]) + continue + } + i++ + if i >= len(s)-1 { + return "", false + } + switch s[i] { + case '"', '\\': + out.WriteByte(s[i]) + case 'a': + out.WriteByte('\a') + case 'b': + out.WriteByte('\b') + case 'f': + out.WriteByte('\f') + case 'n': + out.WriteByte('\n') + case 'r': + out.WriteByte('\r') + case 't': + out.WriteByte('\t') + case 'v': + out.WriteByte('\v') + case '0', '1', '2', '3', '4', '5', '6', '7': + if i+2 >= len(s)-1 || s[i+1] < '0' || s[i+1] > '7' || s[i+2] < '0' || s[i+2] > '7' { + return "", false + } + out.WriteByte((s[i]-'0')*64 + (s[i+1]-'0')*8 + s[i+2] - '0') + i += 2 + default: + return "", false + } + } + return out.String(), true +} diff --git a/internal/session/git_patch_path_test.go b/internal/session/git_patch_path_test.go new file mode 100644 index 000000000..4941d37ab --- /dev/null +++ b/internal/session/git_patch_path_test.go @@ -0,0 +1,24 @@ +package session + +import "testing" + +// Contract 1c: the patch reader must use git's destination path for both +// quoted and bare headers, including names with a destination prefix inside. +func TestPatchSectionPathReadsRealName(t *testing.T) { + cases := []struct{ name, patch, want string }{ + {"embedded destination", "diff --git a/x b/plandb.db b/x b/plandb.db\nindex 1..2\n", "x b/plandb.db"}, + {"octal", "diff --git \"a/odd name \\303\\251'q.txt\" \"b/odd name \\303\\251'q.txt\"\nindex 1..2\n", "odd name é'q.txt"}, + {"quote", "diff --git \"a/has\\\"quote.txt\" \"b/has\\\"quote.txt\"\nindex 1..2\n", "has\"quote.txt"}, + {"newline", "diff --git \"a/new\\nline.txt\" \"b/new\\nline.txt\"\nindex 1..2\n", "new\nline.txt"}, + {"rename", "diff --git a/old.txt b/new.txt\nsimilarity index 100%\nrename from old.txt\nrename to new.txt\n", "new.txt"}, + {"deletion", "diff --git a/gone.txt b/gone.txt\ndeleted file mode 100644\n--- a/gone.txt\n+++ /dev/null\n", "gone.txt"}, + {"older engine", "diff --git a/old.txt b/new.txt\n--- a/old.txt\n+++ b/new.txt\n", "new.txt"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := PatchSectionPath(tc.patch); got != tc.want { + t.Fatalf("PatchSectionPath = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/internal/session/git_paths.go b/internal/session/git_paths.go new file mode 100644 index 000000000..d1b59ac60 --- /dev/null +++ b/internal/session/git_paths.go @@ -0,0 +1,66 @@ +package session + +// git_paths.go is THE ONE READER OF THE PATHS GIT NAMES IN ITS NUL-TERMINATED +// ANSWERS, for every question on the landing road about which files are the +// work. +// +// GIT'S HUMAN OUTPUT IS FOR PEOPLE, AND IT QUOTES. `git status --porcelain` +// without `-z` writes a name holding a space, a quote, an accent or a newline +// inside double quotes, with C escapes: `odd name é'q.txt` arrives as +// `"odd name \303\251'q.txt"`. A reader that stripped the outer quotes handed +// `git add` a literal pathspec naming no file on disk, the add refused it, the +// per-path retry read it as a file that had never existed, and the landing +// commit went out without it and without a word. The `-z` form writes every +// name as its own bytes, so it is the only form read here. +// +// A NAME IS NEVER TRIMMED. A leading or trailing space is part of a file's name +// on every filesystem git runs on, and a trim is how the work tab listed +// ` lead.txt` as `lead.txt`, a file that was not there. + +import "strings" + +// gitNULPaths reads a `-z` name list (`--name-only -z`, `ls-files -z`) into +// the names it holds. Only the empty fields are dropped, which are the list's +// own terminator and never a file. +func gitNULPaths(out string) []string { + var paths []string + for _, path := range strings.Split(out, "\x00") { + if path != "" { + paths = append(paths, path) + } + } + return paths +} + +// porcelainEntry is one record of `git status --porcelain -z`: its two status +// columns, the path as it stands now, and for a rename or a copy the path it +// came from. Both ends are kept because the questions differ: what to stage +// wants the name that exists now, and what a run touched wants both. +type porcelainEntry struct { + Code string + Path string + From string +} + +// porcelainEntries reads a `git status --porcelain -z` answer. The two status +// columns are read by position, never after a trim, because the first column +// is a space for a change that is not staged, and a record whose third byte is +// not the separating space is not one git writes. A rename or copy in either +// column is followed by one more field, the name it came from. +func porcelainEntries(out string) []porcelainEntry { + fields := strings.Split(out, "\x00") + var entries []porcelainEntry + for i := 0; i < len(fields); i++ { + entry := fields[i] + if len(entry) < 4 || entry[2] != ' ' { + continue + } + item := porcelainEntry{Code: entry[:2], Path: entry[3:]} + if strings.ContainsAny(item.Code, "RC") && i+1 < len(fields) { + i++ + item.From = fields[i] + } + entries = append(entries, item) + } + return entries +} diff --git a/internal/session/git_paths_test.go b/internal/session/git_paths_test.go new file mode 100644 index 000000000..ac2f1d2f7 --- /dev/null +++ b/internal/session/git_paths_test.go @@ -0,0 +1,69 @@ +package session + +import ( + "path/filepath" + "reflect" + "testing" +) + +// Contract 4a: the belt must hand staging the real path, including whitespace +// and bytes that git quotes in its human status output. +func TestBeltTreeWorkKeepsGitQuotedNames(t *testing.T) { + repo := newTestRepo(t) + names := []string{" lead.txt", "odd name é'q.txt", "has\"quote.txt", "new\nline.txt", "a -> b.txt"} + for _, name := range names { + writeFile(t, filepath.Join(repo, name), "work\n") + } + want := make(map[string]bool, len(names)) + for _, name := range names { + want[literalPathspec+name] = true + } + got := beltTreeWork(repo) + if len(got) != len(want) { + t.Fatalf("beltTreeWork = %q, want %q", got, names) + } + for _, path := range got { + if !want[path] { + t.Errorf("beltTreeWork contains wrong path %q", path) + } + } +} + +// Contract 4c: a leftover's name must be the same bytes as the file on disk. +func TestLeftBehindKeepsGitQuotedName(t *testing.T) { + repo := newTestRepo(t) + writeFile(t, filepath.Join(repo, "odd name é'q.txt"), "work\n") + if got := leftBehind(repo); !reflect.DeepEqual(got, []string{"odd name é'q.txt"}) { + t.Fatalf("leftBehind = %q", got) + } +} + +// Contract 4c: the staged path list must keep git-quoted names unchanged. +func TestStagedPathsKeepsGitQuotedName(t *testing.T) { + repo := newTestRepo(t) + writeFile(t, filepath.Join(repo, "odd name é'q.txt"), "work\n") + mustGit(t, repo, "add", "odd name é'q.txt") + got, problem := stagedPaths(repo) + if problem != "" || !reflect.DeepEqual(got, []string{"odd name é'q.txt"}) { + t.Fatalf("stagedPaths = %q, %q", got, problem) + } +} + +// Contract 4c: the porcelain reader must preserve the two names of a rename +// and must treat an arrow inside a filename as ordinary bytes. +func TestPorcelainEntriesReadRenameAndArrow(t *testing.T) { + entries := porcelainEntries("R new\x00old\x00?? a -> b.txt\x00") + want := []porcelainEntry{{Code: "R ", Path: "new", From: "old"}, {Code: "??", Path: "a -> b.txt"}} + if !reflect.DeepEqual(entries, want) { + t.Fatalf("porcelainEntries = %#v, want %#v", entries, want) + } +} + +// Contract 4a: paths separated by NUL must retain spaces at either edge. +func TestGitNULPathsKeepWhitespace(t *testing.T) { + got := gitNULPaths(" lead.txt\x00trail.txt \x00\x00") + want := []string{" lead.txt", "trail.txt "} + if !reflect.DeepEqual(got, want) { + t.Fatalf("gitNULPaths = %q, want %q", got, want) + } +} diff --git a/internal/session/land_run_tree.go b/internal/session/land_run_tree.go index 7996d9b09..e78055ba8 100644 --- a/internal/session/land_run_tree.go +++ b/internal/session/land_run_tree.go @@ -63,11 +63,7 @@ func LandRunTree(dir, base, title, model string) (branch string, changed []strin if err != nil { return "", nil, "", err } - for _, name := range strings.Split(out, "\x00") { - if name != "" { - changed = append(changed, name) - } - } + changed = append(changed, gitNULPaths(out)...) head := runTreeHead(dir) if head == "" { return "", nil, "", errors.New("read the run's branch head after landing") @@ -114,10 +110,8 @@ func runTouchedPaths(dir, base, head string) ([]string, error) { if err != nil { return nil, fmt.Errorf("read run commit paths %s: %w", sha, err) } - for _, path := range strings.Split(paths, "\x00") { - if path != "" { - seen[path] = true - } + for _, path := range gitNULPaths(paths) { + seen[path] = true } } changed := make([]string, 0, len(seen)) diff --git a/internal/session/plandb_work.go b/internal/session/plandb_work.go index cc61d0a66..f1ce38c36 100644 --- a/internal/session/plandb_work.go +++ b/internal/session/plandb_work.go @@ -144,14 +144,17 @@ func readPlanWork(copied *TaskCopyRecord) PlanTaskWork { if base == "" { base = "HEAD" } - patch, err := git(work.Dir, "diff", "--no-color", "--no-ext-diff", base, "--", ".") + patch, err := git(work.Dir, planWorkDiffArgs(base, "--", ".")...) if err != nil { return work } work.Patch, work.Cut = planWorkPatch(patch) if out, err := git(work.Dir, "ls-files", "--others", "--exclude-standard", "-z", "--", "."); err == nil { - for _, name := range strings.Split(out, "\x00") { - if name = strings.TrimSpace(name); name != "" && !harnessWrote(filepath.ToSlash(name)) { + // An untracked build cache is left out as the landing leaves it + // out ([buildCache]), so the tab never previews a file that will + // not come home. + for _, name := range gitNULPaths(out) { + if !harnessWrote(filepath.ToSlash(name)) && !buildCache(filepath.ToSlash(name)) { work.Added = append(work.Added, name) } } @@ -163,7 +166,7 @@ func readPlanWork(copied *TaskCopyRecord) PlanTaskWork { if root == "" || branch == "" || base == "" { return work } - patch, err := git(root, "diff", "--no-color", "--no-ext-diff", base, "refs/heads/"+branch, "--") + patch, err := git(root, planWorkDiffArgs(base, "refs/heads/"+branch, "--")...) if err != nil { return work } @@ -172,13 +175,27 @@ func readPlanWork(copied *TaskCopyRecord) PlanTaskWork { return work } +// planWorkDiffArgs is the one spelling of the difference the work tab reads, +// on the live copy and off a given-back copy's branch alike, with the caller's +// revisions and pathspec after it. +// +// THE HEADER'S SHAPE IS PINNED HERE, NOT LEFT TO THE PERSON'S GIT CONFIG, because +// [PatchSectionPath] reads the file's name off it. `diff.noprefix` would write +// `diff --git P P` and `diff.mnemonicPrefix` `diff --git i/P w/P`, so the two +// prefixes are said outright. `core.quotePath=false` lets an accent arrive as +// itself rather than as octal escapes; a name with a quote or a newline is still +// quoted, and the reader unquotes it. +func planWorkDiffArgs(rest ...string) []string { + return append([]string{"-c", "core.quotePath=false", "diff", "--no-color", "--no-ext-diff", "--src-prefix=a/", "--dst-prefix=b/"}, rest...) +} + // planWorkPatch drops every file section the harness wrote and caps what is // left. The sections are cut on git's own header line, so a file the harness // owns is left out whole and never half. func planWorkPatch(patch string) (string, bool) { var kept strings.Builder - for _, section := range planPatchSections(patch) { - if harnessWrote(planPatchPath(section)) { + for _, section := range PatchSections(patch) { + if harnessWrote(PatchSectionPath(section)) { continue } kept.WriteString(section) @@ -193,43 +210,3 @@ func planWorkPatch(patch string) (string, bool) { } return cut, true } - -// planPatchSections splits a unified patch into one piece per file, each -// starting at its `diff --git` line. -func planPatchSections(patch string) []string { - var out []string - start := -1 - for at := 0; at < len(patch); { - end := strings.IndexByte(patch[at:], '\n') - line := patch[at:] - next := len(patch) - if end >= 0 { - line = patch[at : at+end] - next = at + end + 1 - } - if strings.HasPrefix(line, "diff --git ") { - if start >= 0 { - out = append(out, patch[start:at]) - } - start = at - } - at = next - } - if start >= 0 { - out = append(out, patch[start:]) - } else if strings.TrimSpace(patch) != "" { - out = append(out, patch) - } - return out -} - -// planPatchPath is the path one file section is about, read off its header's -// `b/` side, which is the file as it stands now. -func planPatchPath(section string) string { - head, _, _ := strings.Cut(section, "\n") - head = strings.TrimPrefix(head, "diff --git ") - if at := strings.LastIndex(head, " b/"); at >= 0 { - return head[at+3:] - } - return head -} diff --git a/internal/session/plandb_work_test.go b/internal/session/plandb_work_test.go index 5b08f3f4c..c9e3f9a5c 100644 --- a/internal/session/plandb_work_test.go +++ b/internal/session/plandb_work_test.go @@ -4,6 +4,8 @@ import ( "os" "os/exec" "path/filepath" + "reflect" + "sort" "strings" "testing" ) @@ -53,6 +55,44 @@ func TestReadPlanWorkIsTheCopysOwnChangesWithoutTheHarnessFiles(t *testing.T) { } } +// Contract 1a and 1b: the live copy must keep unusual project filenames and +// exclude only the harness's files, even when a project path ends in plandb.db. +func TestReadPlanWorkKeepsRealNamesInLiveCopy(t *testing.T) { + repo := newTestRepo(t) + for _, name := range []string{"x b/plandb.db", "sub/plandb.db"} { + writeFile(t, filepath.Join(repo, name), "one\n") + mustGit(t, repo, "add", name) + } + mustGit(t, repo, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-m", "seed names") + base := strings.TrimSpace(gitOut(t, repo, "rev-parse", "HEAD")) + for _, name := range []string{"x b/plandb.db", "sub/plandb.db"} { + writeFile(t, filepath.Join(repo, name), "two\n") + } + added := []string{" lead.txt", "odd name é'q.txt", "has\"quote.txt", "new\nline.txt"} + for _, name := range added { + writeFile(t, filepath.Join(repo, name), "new\n") + } + writeFile(t, filepath.Join(repo, "plandb.db"), "private\n") + writeFile(t, filepath.Join(repo, ".codeaf", "x"), "private\n") + work := readPlanWork(&TaskCopyRecord{Dir: repo, HomeSha: base}) + if !work.Read || work.Cut { + t.Fatalf("work read = %v, cut = %v", work.Read, work.Cut) + } + for _, name := range []string{"x b/plandb.db", "sub/plandb.db"} { + if !strings.Contains(work.Patch, "diff --git a/"+name+" b/"+name) { + t.Errorf("patch omits %q:\n%s", name, work.Patch) + } + } + if strings.Contains(work.Patch, "diff --git a/plandb.db b/plandb.db") || strings.Contains(work.Patch, "diff --git a/.codeaf/") { + t.Errorf("patch includes harness files:\n%s", work.Patch) + } + sort.Strings(added) + sort.Strings(work.Added) + if !reflect.DeepEqual(work.Added, added) { + t.Errorf("added = %q, want %q", work.Added, added) + } +} + // A PATCH WITH A FILE THE HARNESS WROTE LOSES THAT FILE WHOLE, never half. func TestPlanWorkPatchDropsAHarnessFileWhole(t *testing.T) { patch := "diff --git a/load.go b/load.go\n--- a/load.go\n+++ b/load.go\n@@ -1 +1 @@\n-a\n+b\n" + @@ -115,3 +155,38 @@ func TestReadPlanWorkReadsAGivenBackCopyOffItsBranch(t *testing.T) { t.Fatalf("the branch's work is not in the patch:\n%s", work.Patch) } } + +// Contract 1a and 1b: a given-back copy reads project paths from its branch +// with the same header spelling and harness exclusions as a live copy. +func TestReadPlanWorkKeepsRealNamesOnGivenBackBranch(t *testing.T) { + repo := newTestRepo(t) + writeFile(t, filepath.Join(repo, "x b", "plandb.db"), "one\n") + mustGit(t, repo, "add", "x b/plandb.db") + mustGit(t, repo, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-m", "seed") + base := strings.TrimSpace(gitOut(t, repo, "rev-parse", "HEAD")) + mustGit(t, repo, "checkout", "-b", "task/given-back") + writeFile(t, filepath.Join(repo, "x b", "plandb.db"), "two\n") + writeFile(t, filepath.Join(repo, "odd name é'q.txt"), "accent\n") + writeFile(t, filepath.Join(repo, "plandb.db"), "private\n") + mustGit(t, repo, "add", "x b/plandb.db", "odd name é'q.txt", "plandb.db") + mustGit(t, repo, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-m", "work") + mustGit(t, repo, "checkout", "--detach", base) + work := readPlanWork(&TaskCopyRecord{Dir: t.TempDir(), Root: repo, Branch: "task/given-back", HomeSha: base}) + if !work.Read { + t.Fatal("given-back branch was not read") + } + for _, name := range []string{"x b/plandb.db", "odd name é'q.txt"} { + found := false + for _, section := range PatchSections(work.Patch) { + if PatchSectionPath(section) == name { + found = true + } + } + if !found { + t.Errorf("given-back patch omits %q:\n%s", name, work.Patch) + } + } + if strings.Contains(work.Patch, "diff --git a/plandb.db b/plandb.db") { + t.Errorf("given-back patch includes harness store:\n%s", work.Patch) + } +} diff --git a/internal/session/principal_delivery.go b/internal/session/principal_delivery.go index a84cfd64e..fcf1d142b 100644 --- a/internal/session/principal_delivery.go +++ b/internal/session/principal_delivery.go @@ -85,7 +85,7 @@ func anythingToCompare(root, branch string, changed []string) bool { pathspec := ":(literal)" + path branchArgs = append(branchArgs, pathspec) workspaceArgs = append(workspaceArgs, pathspec) - wanted[strings.TrimSpace(path)] = true + wanted[path] = true } branchNames, branchErr := git(root, branchArgs...) if namesOne(wanted, branchNames, branchErr) { @@ -107,8 +107,8 @@ func namesOne(wanted map[string]bool, out string, err error) bool { if err != nil { return false } - for _, name := range strings.Split(out, "\x00") { - if wanted[strings.TrimSpace(name)] { + for _, name := range gitNULPaths(out) { + if wanted[name] { return true } } diff --git a/internal/session/prompts/bashworker.md b/internal/session/prompts/bashworker.md index b11c9dccd..9400b321a 100644 --- a/internal/session/prompts/bashworker.md +++ b/internal/session/prompts/bashworker.md @@ -97,7 +97,8 @@ work. BEFORE `plandb done`, walk every requirement sentence of your work order and of the ask it serves, one per line, and beside each name the command or test that proved it in THIS run. A requirement with no proof is not done — prove it now, or -report it undone. The walk is the last check, not a summary. +report it undone. The walk is the last check, not a summary. Delete every +scratch file you made in your copy before `plandb done`: it would land. THREE VERBS END OR HOLD A TASK, and none of them is a reply. You ACT with a bash call; you FINISH with `plandb done` on your own task, and only after the diff --git a/internal/session/run_tree_changes.go b/internal/session/run_tree_changes.go index 42bf85f42..54588480f 100644 --- a/internal/session/run_tree_changes.go +++ b/internal/session/run_tree_changes.go @@ -112,10 +112,8 @@ func (s RunTreeSnapshot) Changed() []string { } } } else if out, err := git(s.root, "diff", "--name-only", "-z", s.head, head); err == nil { - for _, name := range strings.Split(out, "\x00") { - if name = strings.TrimSpace(name); name != "" { - add(filepath.Join(s.root, filepath.FromSlash(name))) - } + for _, name := range gitNULPaths(out) { + add(filepath.Join(s.root, filepath.FromSlash(name))) } } } @@ -195,20 +193,10 @@ func runTreeStatus(root string) []string { return nil } var paths []string - fields := strings.Split(out, "\x00") - for i := 0; i < len(fields); i++ { - entry := fields[i] - if len(entry) < 4 { - continue - } - code, name := entry[:2], entry[3:] - paths = append(paths, filepath.Join(root, filepath.FromSlash(name))) - if code[0] == 'R' || code[0] == 'C' { - // The next field is the name it came from. - if i+1 < len(fields) && fields[i+1] != "" { - paths = append(paths, filepath.Join(root, filepath.FromSlash(fields[i+1]))) - } - i++ + for _, entry := range porcelainEntries(out) { + paths = append(paths, filepath.Join(root, filepath.FromSlash(entry.Path))) + if entry.From != "" { + paths = append(paths, filepath.Join(root, filepath.FromSlash(entry.From))) } } return paths diff --git a/internal/session/stoprun.go b/internal/session/stoprun.go index ffad50f8e..a5e80f9a3 100644 --- a/internal/session/stoprun.go +++ b/internal/session/stoprun.go @@ -275,7 +275,7 @@ func (a *Agent) sayToRunRow(id uint64, text string, origin messageOrigin) (Steer if err != nil { return SteerReceipt{}, true, err } - return SteerReceipt{Landing: steerRunNoteWord}, true, nil + return SteerReceipt{Landing: RunNotePickupWord}, true, nil } g := a.graph() if g == nil { diff --git a/internal/session/task_landing_test.go b/internal/session/task_landing_test.go index cc17cdd95..c89b152ad 100644 --- a/internal/session/task_landing_test.go +++ b/internal/session/task_landing_test.go @@ -157,13 +157,16 @@ func TestLeftBehindReadsThePorcelainColumnsWhole(t *testing.T) { // The tree git status reads here is the ordinary shape the landing reads: // one modified-not-staged line with the leading-space padding and one // untracked line beside it. - status := gitOut(t, repo, "status", "--porcelain", "--untracked-files=all", "--", ".") - if !strings.Contains(status, " M a/b.go") || !strings.Contains(status, "?? c.go") { + status := gitOut(t, repo, "status", "--porcelain", "-z", "--untracked-files=all", "--", ".") + if !strings.Contains(status, " M a/b.go\x00") || !strings.Contains(status, "?? c.go\x00") { t.Fatalf("the fixture reads:\n%s, want a modified and an untracked path", status) } - left := porcelainPaths(status) + var left []string + for _, entry := range porcelainEntries(status) { + left = append(left, entry.Path) + } if !containsString(left, "a/b.go") || !containsString(left, "c.go") { - t.Fatalf("porcelainPaths = %v, want a/b.go whole and c.go", left) + t.Fatalf("porcelainEntries paths = %v, want a/b.go whole and c.go", left) } } diff --git a/internal/session/task_run.go b/internal/session/task_run.go index 07515bb84..dd6b75477 100644 --- a/internal/session/task_run.go +++ b/internal/session/task_run.go @@ -8902,11 +8902,11 @@ const ( // while the merge is still in progress. An empty answer is a merge that failed // before it touched the index. func conflictedPaths(root string) []string { - out, err := git(root, "diff", "--name-only", "--diff-filter=U") + out, err := git(root, "diff", "--name-only", "-z", "--diff-filter=U") if err != nil { return nil } - return nonEmptyLines(out) + return gitNULPaths(out) } // abandonMerge takes the person's checkout back out of a merge, and it exists @@ -8946,7 +8946,7 @@ func leftBehind(dir string) []string { return paths } } - args := []string{"status", "--porcelain", "--untracked-files=all", "--", "."} + args := []string{"status", "--porcelain", "-z", "--untracked-files=all", "--", "."} for _, dropping := range taskDroppingNames() { args = append(args, ":(exclude)"+dropping) } @@ -8954,7 +8954,11 @@ func leftBehind(dir string) []string { if err != nil { return nil } - return porcelainPaths(out) + var paths []string + for _, entry := range porcelainEntries(out) { + paths = append(paths, entry.Path) + } + return paths } const leftBehindRecord = "left-behind.json" @@ -9166,15 +9170,13 @@ func unheldLedgerPaths(dir string, wrote []string) []string { if len(paths) == 0 { return nil } - out, err := git(dir, append([]string{"status", "--porcelain", "--untracked-files=all", "--"}, paths...)...) + out, err := git(dir, append([]string{"status", "--porcelain", "-z", "--untracked-files=all", "--"}, paths...)...) if err != nil { return nil } var unheld []string - for _, line := range nonEmptyLines(out) { - if len(line) > 3 { - unheld = append(unheld, strings.TrimSpace(line[3:])) - } + for _, entry := range porcelainEntries(out) { + unheld = append(unheld, entry.Path) } return unheld } @@ -9186,11 +9188,11 @@ func unheldLedgerPaths(dir string, wrote []string) []string { // otherwise the same one a node that only read gives — and its caller merges and // then removes the only other copy of the work on the strength of it. func stagedPaths(dir string) ([]string, string) { - out, err := git(dir, "diff", "--cached", "--name-only") + out, err := git(dir, "diff", "--cached", "--name-only", "-z") if err != nil { return nil, firstLine(out) } - return nonEmptyLines(out), "" + return gitNULPaths(out), "" } // stagedDiffStat is the node's change AS A SHAPE: one line per file with how @@ -9329,8 +9331,8 @@ func stageableWork(dir string, wrote []string) []string { } // beltTreeWork reads every change git sees in the working copy that the -// ledger did not name — modified, added and untracked alike, one path per -// line — and takes out the paths the harness itself writes, which are +// ledger did not name — modified, added and untracked alike — and takes out +// the paths the harness itself writes, which are // machinery and never the work. A belt worker's landing stages this whole // answer ([stageTaskWork]), so what a person gets on the branch is what the // shell did, and nothing else. @@ -9341,12 +9343,13 @@ func stageableWork(dir string, wrote []string) []string { // folder or an ancestor of it — never a path inside the working copy git // could name. func beltTreeWork(dir string) []string { - out, err := git(dir, "status", "--porcelain", "--untracked-files=all", "--", ".") + out, err := git(dir, "status", "--porcelain", "-z", "--untracked-files=all", "--", ".") if err != nil { return nil } var paths []string - for _, path := range porcelainPaths(out) { + for _, entry := range porcelainEntries(out) { + path := entry.Path // WHAT IS MACHINERY IS ANSWERED IN ONE PLACE ([harnessWrote]), by where // the harness itself writes, and never by a name project files share: a // `.lock` suffix here once kept every lockfile a run changed off the @@ -9354,31 +9357,14 @@ func beltTreeWork(dir string) []string { if harnessWrote(path) { continue } - paths = append(paths, literalPathspec+path) - } - return paths -} - -// porcelainPaths reads the paths out of one `git status --porcelain` answer, -// taken from the fixed columns rather than trimmed off the front: a porcelain -// line is two status letters, a space, then the path, and a line that was -// trimmed first has lost the status columns' own padding — the staged ' M -// a/b.go' reads as 'M a/b.go', and the slice past the third column then cuts -// the first character of the path. A rename carries both names and the one -// that exists now is the second. -func porcelainPaths(out string) []string { - var paths []string - for _, line := range strings.Split(out, "\n") { - if strings.TrimSpace(line) == "" || len(line) < 4 { + // AN UNTRACKED BUILD CACHE IS NOT THE WORK EITHER, and it is a + // separate, narrower question ([buildCache]): exact cache names, and + // only for a file git has never been told about. A tracked cache that + // changed, or one the worker staged itself, is not `??` and lands. + if entry.Code == "??" && buildCache(path) { continue } - path := strings.TrimSpace(line[3:]) - if _, renamed, found := strings.Cut(path, " -> "); found { - path = renamed - } - if path = strings.Trim(path, `"`); path != "" { - paths = append(paths, path) - } + paths = append(paths, literalPathspec+path) } return paths } diff --git a/internal/session/task_run_belt.go b/internal/session/task_run_belt.go index 36fdf6224..524df5348 100644 --- a/internal/session/task_run_belt.go +++ b/internal/session/task_run_belt.go @@ -119,6 +119,14 @@ type RunSpec struct { // environment rung and the profile's checker row, and never the plan // seat's model. CheckModel string + // OneModel is the conversation's model when the conversation runs under + // `--one-model` ([Config.OneModel]), and empty otherwise. EVERY SEAT OF THE + // RUN RIDES IT: work, plan, check and the probe no door names. The flag + // withholds the roles ladder and the crew router, so without this the + // engine's crew factory found three empty seats and filled them from the + // profile's crew rows — a run under a flag that promises one model billed + // the crew's. + OneModel string // CompleterFor answers the provider a worker is seated on. The door hands // the conversation's own — a run worker's calls go out the way the // conversation's do — and a nil one lets the engine build each worker's @@ -603,6 +611,19 @@ func (a *Agent) beltRunSpec(run *beltRun, brief string) RunSpec { planSeat = d.Seat(crewroute.Planner).Send checkSeat = d.Seat(crewroute.Checker).Send } + // AND UNDER `--one-model` THERE IS ONE SEAT, the conversation's own model, + // read live as every errand's floor is read ([Agent.callRoleChecked]), so a + // /model typed before the run moves it. The flag withholds the ladder and + // the router, which left all three seats above empty, and an empty seat is + // one the engine fills from the profile's crew rows: the run billed models + // the person had just said to leave alone. It is carried as its own field + // too, because the probe seat and the check seat's environment rung are the + // engine's to read and only a named "every seat" stands them down. + oneModel := "" + if a.config.OneModel { + oneModel = a.Model() + workSeat, planSeat, checkSeat = oneModel, oneModel, oneModel + } wallLeft, _ := a.config.Budget.Left() if a.config.Budget.Wall > 0 && !a.startedAt.IsZero() { @@ -630,6 +651,7 @@ func (a *Agent) beltRunSpec(run *beltRun, brief string) RunSpec { WorkModel: workSeat, PlanModel: planSeat, CheckModel: checkSeat, + OneModel: oneModel, CompleterFor: func(string) Completer { return a.crewRunCompleter(run) }, } } diff --git a/internal/session/task_run_belt_test.go b/internal/session/task_run_belt_test.go index 62d6e2bcb..77b3fd3a4 100644 --- a/internal/session/task_run_belt_test.go +++ b/internal/session/task_run_belt_test.go @@ -505,6 +505,58 @@ func TestStartTaskBashBeltPassesTheConversationWallLeftToTheRun(t *testing.T) { endBeltRun(t, agent, double) } +// UNDER `--one-model` EVERY SEAT OF A RUN RIDES THE CONVERSATION'S MODEL +// (contract 3a, 3b and 3c). The flag withholds the roles ladder and the crew +// router, and the spec used to hand the engine three empty seats, which the +// engine's crew factory filled from the profile's crew rows and from +// CODEAF_CHECK_MODEL: a run under the flag billed models nobody named. The +// seats are read at the run's start, so a /model typed before it moves them, +// and a conversation without the flag hands the engine what it always did. +func TestUnderOneModelEveryRunSeatRidesTheConversationsModel(t *testing.T) { + t.Setenv("CODEAF_TASK_BELT", "bash") + t.Setenv("CODEAF_CHECK_MODEL", "vendor/env-check") + for _, test := range []struct { + name string + oneModel bool + switchTo string + want string + }{ + {"the launch model", true, "", "test/model"}, + {"the model switched to before the run", true, "vendor/switched", "vendor/switched"}, + {"no flag leaves the seats to the engine", false, "", ""}, + } { + t.Run(test.name, func(t *testing.T) { + double := newBeltRunDouble("done") + registerBeltRunEngine(t, double) + dir := t.TempDir() + agent, _ := newTestAgent(t, beltRunCompleter{text: "done"}, func(config *Config) { + config.Workspace = newTestRepo(t) + config.Place = Place{Dir: dir} + config.AskConsent = false + config.OneModel = test.oneModel + }) + if test.switchTo != "" { + agent.SetModel(test.switchTo) + } + if _, _, _, err := agent.StartTask(context.Background(), "write the file", false); err != nil { + t.Fatalf("StartTask: %v", err) + } + <-double.entered + double.mu.Lock() + spec := double.spec + double.mu.Unlock() + for seat, got := range map[string]string{ + "work": spec.WorkModel, "plan": spec.PlanModel, "check": spec.CheckModel, "every": spec.OneModel, + } { + if got != test.want { + t.Errorf("the %s seat the engine was handed is %q, want %q", seat, got, test.want) + } + } + endBeltRun(t, agent, double) + }) + } +} + func TestDriveBeltRunLimitUsesTheOrdinaryLandingRoad(t *testing.T) { agent, _, run, _ := landingSummaryFixture(t, &scriptedCompleter{}) landCalls := 0 diff --git a/internal/tui3/note_pickup_law_test.go b/internal/tui3/note_pickup_law_test.go new file mode 100644 index 000000000..612f4d93b --- /dev/null +++ b/internal/tui3/note_pickup_law_test.go @@ -0,0 +1,59 @@ +package tui3 + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "strconv" + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/manual" + "github.com/Agent-Field/codeaf/internal/session" +) + +// THE NOTE'S PICKUP SENTENCE IS SPELLED ONCE (contract 2a and 2b). The chat's +// receipt for a note on a run's row and the room's line under the same stored +// note are one promise, and two string literals of it would each pass their own +// test on the day one of them changed. So the sources themselves are read: in +// this package and the session's, exactly one literal says it, the room's +// constant is that one, and the manual quotes it exactly. +func TestTheNotePickupSentenceIsSpelledOnce(t *testing.T) { + count := 0 + for _, dir := range []string{".", "../session"} { + packages, err := parser.ParseDir(token.NewFileSet(), dir, func(info os.FileInfo) bool { + return !strings.HasSuffix(info.Name(), "_test.go") + }, 0) + if err != nil { + t.Fatal(err) + } + for _, pkg := range packages { + for _, file := range pkg.Files { + ast.Inspect(file, func(node ast.Node) bool { + literal, ok := node.(*ast.BasicLit) + if !ok || literal.Kind != token.STRING { + return true + } + value, err := strconv.Unquote(literal.Value) + if err != nil { + t.Errorf("unquote %s: %v", literal.Value, err) + } + if strings.Contains(value, "reads a note at its next step") { + count++ + } + return true + }) + } + } + } + if count != 1 { + t.Fatalf("note pickup sentence has %d Go definitions, want one", count) + } + if taskPlanPickupWord != session.RunNotePickupWord { + t.Fatal("the room's note pickup words differ from the chat receipt") + } + if !manual.Chat().Mentions(session.RunNotePickupWord) { + t.Fatal("the manual does not quote the note pickup sentence") + } +} diff --git a/internal/tui3/planroom.go b/internal/tui3/planroom.go index 2cd6fdc5b..3345e768b 100644 --- a/internal/tui3/planroom.go +++ b/internal/tui3/planroom.go @@ -565,45 +565,52 @@ func (a *app) roomWorkRows(width int) []row { // ([app.questionDiffLines]): the mark from the vocabulary and the hue from the // ramp, with each file's header in ink so the eye finds where one file ends. // +// THE FILE'S NAME IS READ OFF THE RAW SECTION, BEFORE ANYTHING IS MADE +// DRAWABLE, and by the session's one reader of it ([session.PatchSectionPath]). +// A name can hold ` b/`, a quote, an accent or a newline, and git writes some of +// those quoted; reading the header here a second way is what once drew a +// person's `x b/plandb.db` as `plandb.db` while the session's filter judged the +// same header, so both sides now ask the one reader. +// +// A SECTION'S HEADER AND ITS HUNKS ARE TOLD APART BY POSITION, never by what a +// line starts with. Everything before the first `@@` is git's own account of +// the file (the index line, the modes, the `---`/`+++` names) and is left out +// but for a binary file's one sentence; everything after it is the file's +// bytes, where a removed line that read `-- note` arrives as `--- note` and is +// work to draw, not a header to skip. +// // EVERY LINE IS SOMEBODY ELSE'S BYTES, whatever a worker wrote into a file, and // goes through [drawableLine] as a call's output does: a carriage return from a // file with Windows line ends or an escape inside one would repaint rows this // surface owns. func (a *app) planWorkLines(patch string, width int) []string { - lines := strings.Split(strings.TrimRight(patch, "\n"), "\n") - out := make([]string, 0, len(lines)) - for _, line := range lines { - line = drawableLine(line) - switch { - case strings.HasPrefix(line, "diff --git "): - if len(out) > 0 { - out = append(out, "") + var out []string + for _, section := range session.PatchSections(patch) { + path := drawableLine(session.PatchSectionPath(section)) + inHunk := false + for _, raw := range strings.Split(strings.TrimRight(section, "\n"), "\n") { + line := drawableLine(raw) + switch { + case !inHunk && strings.HasPrefix(raw, "diff --git "): + if len(out) > 0 { + out = append(out, "") + } + out = append(out, a.pal.ink(fit(path, width))) + case strings.HasPrefix(line, "@@"): + inHunk = true + out = append(out, a.pal.dim(fit(line, width))) + case !inHunk && strings.HasPrefix(line, "Binary files"): + out = append(out, a.pal.dim(fit(line, width))) + case !inHunk: + continue + case strings.HasPrefix(line, "+"): + out = append(out, a.pal.add(fit(a.icon(tokens.GDiffAdd)+strings.TrimPrefix(line, "+"), width))) + case strings.HasPrefix(line, "-"): + out = append(out, a.pal.del(fit(a.icon(tokens.GDiffDel)+strings.TrimPrefix(line, "-"), width))) + default: + out = append(out, a.pal.dim(fit(line, width))) } - out = append(out, a.pal.ink(fit(planPatchHeadPath(line), width))) - case strings.HasPrefix(line, "+++"), strings.HasPrefix(line, "---"), - strings.HasPrefix(line, "index "), strings.HasPrefix(line, "new file"), - strings.HasPrefix(line, "deleted file"), strings.HasPrefix(line, "similarity"), - strings.HasPrefix(line, "rename "), strings.HasPrefix(line, "old mode"), - strings.HasPrefix(line, "new mode"): - continue - case strings.HasPrefix(line, "@@"): - out = append(out, a.pal.dim(fit(line, width))) - case strings.HasPrefix(line, "+"): - out = append(out, a.pal.add(fit(a.icon(tokens.GDiffAdd)+strings.TrimPrefix(line, "+"), width))) - case strings.HasPrefix(line, "-"): - out = append(out, a.pal.del(fit(a.icon(tokens.GDiffDel)+strings.TrimPrefix(line, "-"), width))) - default: - out = append(out, a.pal.dim(fit(line, width))) } } return out } - -// planPatchHeadPath is the file a patch section is about, off its header. -func planPatchHeadPath(line string) string { - head := strings.TrimPrefix(line, "diff --git ") - if at := strings.LastIndex(head, " b/"); at >= 0 { - return head[at+3:] - } - return head -} diff --git a/internal/tui3/planroom_test.go b/internal/tui3/planroom_test.go index 3ec9c5b21..200d45327 100644 --- a/internal/tui3/planroom_test.go +++ b/internal/tui3/planroom_test.go @@ -204,6 +204,39 @@ func TestTheWorkTabOfARunsTaskDrawsTheRunsWorkingCopyDifference(t *testing.T) { } } +// THE WORK TAB NAMES EVERY FILE AS ITSELF (contract 1a and 1c). A header whose +// name holds ` b/` once drew only the part after the last one, so a person's +// `x b/plandb.db` read as codeaf's own `plandb.db`; a header git quoted drew its +// octal escapes. And a removed line that read `-- note` arrives as `--- note`, +// which is the file's bytes and not a header to skip. +func TestTheWorkTabDrawsRealGitFilenames(t *testing.T) { + row := session.PlanTaskRow{ID: "t-6", Title: "fix the loader", Status: "running"} + fake := &planWorkFake{work: session.PlanTaskWork{Read: true, Patch: "diff --git a/x b/plandb.db b/x b/plandb.db\n--- a/x b/plandb.db\n+++ b/x b/plandb.db\n@@ -1,2 +1 @@\n-one\n--- dropped note\n+two\n" + + "diff --git \"a/odd name \\303\\251'q.txt\" \"b/odd name \\303\\251'q.txt\"\n--- /dev/null\n+++ \"b/odd name \\303\\251'q.txt\"\n@@ -0,0 +1 @@\n+new\n"}} + a, plan := planAppWith(t, []session.PlanTaskRow{row}, map[string]session.PlanTaskPage{row.ID: {Row: row, Description: "b"}}) + fake.planFake = plan + a.agent = fake + openPlanRoomNow(t, a, row.ID) + drive(t, a, tea.KeyPressMsg{Code: tea.KeyTab}) + view := planRoomText(t, a) + for _, name := range []string{"x b/plandb.db", "odd name é'q.txt", "-- dropped note"} { + if !strings.Contains(view, name) { + t.Errorf("work tab omits %q:\n%s", name, view) + } + } + if strings.Contains(view, `\303`) { + t.Errorf("work tab draws git's quoted spelling of a name:\n%s", view) + } + // The rows are padded and the side list sits beside them, so a row is read + // up to the list's rule and trimmed before it is compared. + for _, line := range strings.Split(view, "\n") { + text, _, _ := strings.Cut(line, "│") + if strings.TrimSpace(text) == "plandb.db" { + t.Errorf("work tab drew a row that is only plandb.db, the tail of a longer name:\n%s", view) + } + } +} + // `x` OVER AN EMPTY BOX STOPS A RUN'S TASK THROUGH THE PLAN'S DOOR, after the // card; on a task that has ended it is the letter it is. func TestStopInARunsTaskRoomGoesThroughThePlansDoor(t *testing.T) { diff --git a/internal/tui3/taskplan.go b/internal/tui3/taskplan.go index 16b21ba1f..5229eefb7 100644 --- a/internal/tui3/taskplan.go +++ b/internal/tui3/taskplan.go @@ -668,12 +668,12 @@ const ( // a run's task: what the words become, which is a note on the task // (planroom.go's [app.planRoomSteer]). taskPlanNoteWord = "a note for this task" - // taskPlanPickupWord is WHEN a note is read. A worker is a separate loop, so - // a note waits in the store until the worker asks for its next step — the - // manual's own account of a note (worker-harness.md, "Steering a task"), - // said under the note in the task's room once the store has it - // (planroom.go's [app.planRoomSteer]). - taskPlanPickupWord = "the worker reads a note at its next step" + // taskPlanPickupWord is WHEN a note is read, said under the note in the + // task's room once the store has it (planroom.go's [app.planRoomSteer]). It + // is TAKEN from the session's receipt rather than spelled again, because the + // chat row and the room answer the same note and two spellings of one + // promise were two sentences waiting to disagree. + taskPlanPickupWord = session.RunNotePickupWord // taskPlanRefusedWord leads the line a refused action draws in a step's // place. It is the permissions page's own word for a call that was refused, // taken from that constant so the two places cannot come to disagree.