diff --git a/docs/changes/unreleased/1800-senior-dev-state-dir.md b/docs/changes/unreleased/1800-senior-dev-state-dir.md new file mode 100644 index 0000000000..f515d165db --- /dev/null +++ b/docs/changes/unreleased/1800-senior-dev-state-dir.md @@ -0,0 +1,30 @@ +--- +kind: fixed +title: senior-dev keeps its session store out of the folder and writes back a removed .senior-dev +pr: 1800 +surface: [engine, docs] +invalidates: + - "A run whose `.senior-dev` was a link to a store elsewhere ended on the next model turn after the link went away, with `open /.senior-dev/projection.lock: no such file or directory`, while the store itself was whole. senior-dev now resolves the store's path once when it opens it, so the database, the records and the lock all name the store itself, and a link that goes away no longer ends the run." + - "The session database and conversation lived in the folder's `.senior-dev` and were moved into the task's record when the run ended. A run codeaf carries now keeps them in its record folder from the start, `/store` (`store.1`, `store.2` for a later run of the same task), and the folder's `.senior-dev` holds only the brief, the checklist, the pinned command, the messages taken and the output set aside. They are in `.senior-dev` only when codeaf names no record folder." + - "`codeaf senior-dev run --state-dir DIR`, or `SENIOR_DEV_STATE_DIR`, keeps the store in DIR instead of the record folder, where it stays; a DIR inside the folder, other than under its own `.senior-dev/`, is refused before anything is made or spent." + - "A `.senior-dev` removed mid-run took the brief, the checklist and the pinned command with it. senior-dev now writes each one back after the step that removed it, as it last read it, says so on the run's page (`wrote .senior-dev/checklist.md back after something removed it`) and carries on." +--- + +Two CyberGym runs lost `.senior-dev` to the model's own last command. On openssl +the folder's `.senior-dev` was a link to a store outside it, and OpenSSL 1.1.0's +`make clean` deletes every link in the tree; the lock was reopened through the +link for every write, so one failed `open()` ended the run unsubmitted after +$0.64 while the store was collected intact. On arduinojson `.senior-dev` was a +real directory, and the rig's `validate.py` began `sudo rm -rf /src`, taking the +store and the conversation with it. Neither deleting step reached the action +log, because the store died before the step could be written. + +A store that is itself removed or emptied still ends the run, now saying `its +store was removed while the run was working, with the conversation in it`. +The store holds its directory, its database and its records open and compares +them with what is at their paths before every write and every read of the +conversation: in a fresh directory the next turn would make the session again +under the same id and the model would carry on from a conversation that starts +where the removal happened. Holding them open also keeps ext4 and overlayfs from +handing a removed directory's inode number to the next one made at the same +path. diff --git a/internal/manual/chat/senior-dev.md b/internal/manual/chat/senior-dev.md index 675410b543..418838e7a5 100644 --- a/internal/manual/chat/senior-dev.md +++ b/internal/manual/chat/senior-dev.md @@ -613,17 +613,52 @@ and one rescue to **250 MiB** total. If a file or the total exceeds the limit, senior-dev refuses the restore before changing the folder; the ending says it could not be put back and that the folder holds later changes nothing checked. -## Its notes — .senior-dev, its checklist, its session database, moved out when it ends - -senior-dev keeps its own records in `.senior-dev/` in the folder it works in (its copy, in -a repository): the brief, -its checklist, the command it pinned, its session database and its whole conversation -with its model. **They are moved out of your folder when the run ends or you stop it**, -into the task's record folder beside `delegate-conversation.jsonl` (a shell run's record -folder at a shell), and the page adds `its notes (.senior-dev/) are kept in `. So -they never end up on a branch, and the next run in that folder never reads the last -one's checklist as its own. A `.senior-dev/` already in the folder when the run began is -left where it is, and never ends up on a branch either. +## Its notes — .senior-dev, its checklist, moved out when it ends; its session database in the record folder + +senior-dev keeps the files it and its model work from in `.senior-dev/` in the folder it +works in (its copy, in a repository): the brief, its checklist, the command it pinned, +the messages it was handed and the output it set aside. **They are moved out of your +folder when the run ends or you stop it**, into the task's record folder beside +`delegate-conversation.jsonl` (a shell run's record folder at a shell), and the page adds +`its notes (.senior-dev/) are kept in `. So they never end up on a branch, and the +next run in that folder never reads the last one's checklist as its own. A `.senior-dev/` +already in the folder when the run began is left where it is, and never ends up on a +branch either. + +Its session database and its whole conversation with its model are not in your folder at +all. From the moment the run starts they are in `store/` in that same record folder +(`store.1`, `store.2` for a later run of the same task), and they stay there. A run given +`--state-dir DIR` (or `SENIOR_DEV_STATE_DIR`) keeps them in that directory instead. Only +when codeaf names no record folder do they go in `.senior-dev/`. + +## senior-dev stopped with projection.lock: no such file or directory — .senior-dev removed or a link to it gone mid-run, its session store, --state-dir, SENIOR_DEV_STATE_DIR + +senior-dev's session store — `senior-dev.db`, the `storage/` folder of its records and +`projection.lock` — is kept out of the folder it works in: in `store/` in the run's +record folder, or where `--state-dir DIR` or `SENIOR_DEV_STATE_DIR` says (the flag +wins). With no record folder and neither of those, it is in `.senior-dev/`. **It opens +the store once, by its real path**, so a link on the way to it that goes away no longer +matters. It used to live in `.senior-dev/`, reopened through it for every write, +and a run whose `.senior-dev` link was deleted ended with +`open /.senior-dev/projection.lock: no such file or directory` while the store +itself was whole. + +**A `.senior-dev/` removed mid-run is written back.** What the work runs can empty the +folder — a benchmark's reset script, `make clean`, `rm -rf` of the tree. After each step +its model takes, senior-dev puts back the brief, the checklist, the pinned command and the +messages it was handed, as it last read them, and carries on; the page says +`wrote .senior-dev/checklist.md back after something removed it`. The session store is +elsewhere and is not affected. + +**A store that is itself removed or emptied ends the run**, because its conversation went +with it; senior-dev never carries on in an empty one made in its place. It stops, saying +`its store was removed while the run was working, with the conversation in it`. + +What `--state-dir` or `SENIOR_DEV_STATE_DIR` names is made if it is missing. **Use a new +one for each run**: runs in folders without git share one project, so a shared one mixes +their sessions. One inside the folder is refused before anything +is made or spent, because its checkpoints and the change it hands in would carry the +store; the folder's own `.senior-dev/` is the exception. ## Where does senior-dev put its work — its own branch, in a copy of its own, not merged, one commit @@ -1114,7 +1149,7 @@ after the run's record folder (such as `20260924-150405.000000`). That folder al `delegate-program.json`, with the instant senior-dev's process started and the instant it ended. -## senior-dev's flags — run, --variant, --in-place, --high, --verify-test, --max-cost +## senior-dev's flags — run, --variant, --in-place, --high, --verify-test, --state-dir, --max-cost `codeaf senior-dev ` is `codeaf senior-dev run -- `. codeaf gives every program it carries four flags: @@ -1140,6 +1175,7 @@ senior-dev's own flags on `run`: - `--verify-build CMD`, `--verify-test CMD` — the project's own build or test command, run by senior-dev on the submitted tree instead of the one it would have found (see how senior-dev finds a project's build and tests); +- `--state-dir DIR` — its session store in DIR, not the run's record folder (`SENIOR_DEV_STATE_DIR`); - `--frontier` — accepted, and changes nothing: no call senior-dev makes uses that tier; - `--crew` — the models came from a conversation's crew: one its catalog cannot size is left out instead of failing the run. codeaf passes it with the crew's models. diff --git a/internal/manual/chat_test.go b/internal/manual/chat_test.go index 8f15437dee..bcab37f2b3 100644 --- a/internal/manual/chat_test.go +++ b/internal/manual/chat_test.go @@ -1168,6 +1168,8 @@ func TestTheChatManualAnswersTheQuestionsPeopleAsk(t *testing.T) { {"why can't I run senior-dev in a folder inside the one another run is working in", "senior-dev"}, {"can I run senior-dev in a folder that is not a git repo", "senior-dev"}, {"where do senior-dev's notes go", "senior-dev"}, + {"senior-dev crashed with projection.lock no such file or directory", "senior-dev"}, + {"keep senior-dev's session database outside the folder", "senior-dev"}, {"why can't codeaf edit files while senior-dev is working", "senior-dev"}, {"my task was refused because senior-dev is working in the folder", "senior-dev"}, {"the delegate was refused because of uncommitted changes", "delegates"}, @@ -3424,6 +3426,39 @@ func TestTheUndiscoveredBuildQuestionsReachTheVerifyFlags(t *testing.T) { } } +// A senior-dev run whose .senior-dev went away under it ended with a sentence a +// person pastes back as a question, and whoever starts it from a benchmark rig +// asks how to keep its store out of the folder. Both reach the section that +// names the flag, and the ending a removed store now gives reaches the section +// that explains it. Where the database lives now, and what happens when the +// work empties .senior-dev, are asked in a person's words too. +func TestTheVanishedStoreQuestionsReachTheStateDir(t *testing.T) { + for _, probe := range []struct { + asked string + says string + }{ + {"senior-dev crashed with projection.lock no such file or directory", "--state-dir"}, + {"open .senior-dev/projection.lock: no such file or directory", "--state-dir"}, + {"keep senior-dev's session database outside the folder", "SENIOR_DEV_STATE_DIR"}, + {"what does senior-dev's --state-dir do", "--state-dir"}, + {"senior-dev says its store was removed while the run was working", "was removed while the run was working"}, + {"where does senior-dev keep its session database", "store/"}, + {"senior-dev's .senior-dev folder was deleted in the middle of a run", "written back"}, + {"make clean deleted .senior-dev while senior-dev was working", "puts back the brief"}, + } { + found := false + for _, section := range Chat().Search(probe.asked, DefaultResults) { + if section.Page == "senior-dev" && strings.Contains(section.Body, probe.says) { + found = true + break + } + } + if !found { + t.Errorf("%q does not reach a senior-dev section that says %q", probe.asked, probe.says) + } + } +} + // A person whose reply vanished asks in their own words, and there is exactly // one page that can say who ended it. The pins are the two halves the fix owes // them: that a stop of their own is the only silent door, and that a thinking diff --git a/internal/seniordev/actions.go b/internal/seniordev/actions.go index b6fbc357f9..d6954eef38 100644 --- a/internal/seniordev/actions.go +++ b/internal/seniordev/actions.go @@ -268,6 +268,14 @@ func presentStage(stage, status string, facts stageFacts) (delegate.Shown, bool) return delegate.Shown{Text: "stopped without handing in its work"}, true case "implement/steered": return steered(facts), true + case "implement/notes-rewritten": + // Something the work ran removed the file (app/notes_kept.go); the + // run wrote it back as it last read it and carried on. + text := "wrote its notes back after something removed them" + if file := facts.text("file"); file != "" { + text = "wrote " + file + " back after something removed it" + } + return delegate.Shown{Text: text}, true case "compaction-capacity/pinned": text := "learned how much its model can hold" diff --git a/internal/seniordev/actions_test.go b/internal/seniordev/actions_test.go index dd9258dc5d..42a16e5fd5 100644 --- a/internal/seniordev/actions_test.go +++ b/internal/seniordev/actions_test.go @@ -221,3 +221,18 @@ func TestAGuessedWindowReadsOnTheRunsPage(t *testing.T) { t.Fatalf("shown = %+v, %v", shown, ok) } } + +// A NOTE FILE WRITTEN BACK SAYS SO on the run's page, naming the file when the +// record does: something the work ran removed it, and the run put it back as +// it last read it (app/notes_kept.go). +func TestANoteWrittenBackReadsOnTheRunsPage(t *testing.T) { + read := Program.Reader() + named, ok := read(stage("implement", "notes-rewritten", map[string]any{"file": ".senior-dev/checklist.md", "bytes": 43})) + if !ok || named.Text != "wrote .senior-dev/checklist.md back after something removed it" { + t.Fatalf("shown = %+v, %v", named, ok) + } + unnamed, ok := read(stage("implement", "notes-rewritten", nil)) + if !ok || unnamed.Text != "wrote its notes back after something removed them" { + t.Fatalf("shown = %+v, %v", unnamed, ok) + } +} diff --git a/internal/seniordev/app/args.go b/internal/seniordev/app/args.go index a6658880f1..8d19692cef 100644 --- a/internal/seniordev/app/args.go +++ b/internal/seniordev/app/args.go @@ -70,6 +70,13 @@ type cliArgs struct { // leaves that kind to discovery (fullverification/declared.go). VerifyBuild string VerifyTest string + // StateDir is where the session store lives when the flag or the variable + // named it, resolved through its links ([stateDirectory]). Empty leaves it + // to RecordStore, where a directory of the launch's own is made when the + // store opens ([claimRecordStore]); with both empty the store stays in the + // workspace's own .senior-dev. + StateDir string + RecordStore string MaxCost *float64 MaxHours *float64 } diff --git a/internal/seniordev/app/config_live_test.go b/internal/seniordev/app/config_live_test.go index 245a9a6a6b..eb1c58677e 100644 --- a/internal/seniordev/app/config_live_test.go +++ b/internal/seniordev/app/config_live_test.go @@ -73,7 +73,7 @@ func TestProjectConfigChangesLiveRuntimePermissionsAndInstructions(t *testing.T) if err != nil { t.Fatal(err) } - runtime := newConfiguredRuntime(workspace, &capturingBackend{}, cfg) + runtime := newConfiguredRuntime(workspace, "", &capturingBackend{}, cfg) defer runtime.Close() // Project instructions reach the live system prompt service. @@ -246,7 +246,7 @@ func TestSeniorDevConfigDirFeedsRegistryFormatterContract(t *testing.T) { if err != nil { t.Fatal(err) } - runtime := newConfiguredRuntime(workspace, &capturingBackend{}, cfg) + runtime := newConfiguredRuntime(workspace, "", &capturingBackend{}, cfg) defer runtime.Close() target := filepath.Join(workspace, "sample.fmtx") input, _ := json.Marshal(map[string]any{"filePath": target, "content": "unformatted\n"}) diff --git a/internal/seniordev/app/durable_sessions.go b/internal/seniordev/app/durable_sessions.go index cc366d9540..affa3709c1 100644 --- a/internal/seniordev/app/durable_sessions.go +++ b/internal/seniordev/app/durable_sessions.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "io" + "io/fs" "log" "os" "path/filepath" @@ -31,6 +32,8 @@ import ( const ( seniorDevDataDirectory = ".senior-dev" seniorDevDatabaseFile = "senior-dev.db" + // storageDirectory holds the store's flat records ([storage.NewFromDataDir]). + storageDirectory = "storage" projectionReconcileVersion = 1 ) @@ -66,19 +69,46 @@ type durableSessions struct { unsubscribe func() projector *projectors.Store lockPath string + // storeDir is the store's real path, and storeHeld the directory found + // there when it was opened, its database and its records' directory, each + // held open for as long as the store is ([durableSessions.storeStillThere]). + storeDir string + storeHeld []*os.File operationMu sync.Mutex projectionMu sync.Mutex projectionError []error } +// openDurableSessions opens the store in the workspace's own .senior-dev, which +// is where it lives when the run was given no state directory. func openDurableSessions(ctx context.Context, workspace string) (*durableSessions, error) { - // Flat session storage and senior-dev.db both live under the workspace's - // .senior-dev directory. + return openDurableSessionsIn(ctx, workspace, "") +} + +// openDurableSessionsIn opens the run's session store — its database, its flat +// records and the lock that orders them — in stateDir, or in the workspace's +// own .senior-dev when stateDir is empty (--state-dir, SENIOR_DEV_STATE_DIR; +// [stateDirectory]). The files the model is told about by name stay in the +// workspace's .senior-dev either way. +// +// THE STORE IS OPENED BY ITS REAL PATH, ONCE. The directory is made, made +// absolute and resolved through every link before anything is rooted on it, +// so the database, the flat records and the lock all name the store itself, +// never the way to it. A benchmark rig anchored .senior-dev outside the folder +// through a link; the link went away an hour into a run, and the lock, which +// is reopened for every write, was still reopened through it: the next model +// turn ended the run with `open /.senior-dev/projection.lock: no such +// file or directory` while the store it pointed at was whole. +func openDurableSessionsIn(ctx context.Context, workspace, stateDir string) (*durableSessions, error) { dataDir := filepath.Join(workspace, seniorDevDataDirectory) + if stateDir != "" { + dataDir = stateDir + } if err := os.MkdirAll(dataDir, 0o755); err != nil { return nil, fmt.Errorf("senior-dev sessions: create data directory: %w", err) } + dataDir = realDirectory(dataDir) projectInfo, _, err := project.Discover(ctx, workspace) if err != nil { @@ -90,10 +120,22 @@ func openDurableSessions(ctx context.Context, workspace string) (*durableSession if err != nil { return nil, err } + var storeHeld []*os.File closeOnError := func(err error) (*durableSessions, error) { _ = db.Close() + closeAll(storeHeld) return nil, err } + if err := os.MkdirAll(filepath.Join(dataDir, storageDirectory), 0o755); err != nil { + return closeOnError(fmt.Errorf("senior-dev sessions: create records directory: %w", err)) + } + for _, path := range []string{dataDir, dbPath, filepath.Join(dataDir, storageDirectory)} { + held, err := os.Open(path) + if err != nil { + return closeOnError(fmt.Errorf("senior-dev sessions: read data directory: %w", err)) + } + storeHeld = append(storeHeld, held) + } if err := applyProjectSchema(ctx, db); err != nil { return closeOnError(err) } @@ -112,6 +154,7 @@ func openDurableSessions(ctx context.Context, workspace string) (*durableSession projectID: projectID, workspace: workspace, projector: projectors.NewStore(db, projectors.StoreOptions{}), lockPath: filepath.Join(dataDir, "projection.lock"), + storeDir: dataDir, storeHeld: storeHeld, } durable.projectionSource = durable.store sessions, err := sessioncore.New(sessioncore.Options{ @@ -233,7 +276,7 @@ func (durable *durableSessions) withProjection( ) error { durable.operationMu.Lock() defer durable.operationMu.Unlock() - return withAdvisoryFileLock(durable.lockPath, func() error { + return durable.withStoreLock(func() error { durable.projectionMu.Lock() durable.projectionError = nil durable.projectionMu.Unlock() @@ -258,9 +301,16 @@ func (durable *durableSessions) withProjection( }) } +// Messages is a session's conversation. It is read only from the store the +// run opened: a removed one would read as a conversation with nothing in it, +// and the model's next step would end the run on a sentence about a stream +// rather than the one that names the store ([durableSessions.storeStillThere]). func (durable *durableSessions) Messages( ctx context.Context, sessionID string, ) ([]msgmodel.WithParts, error) { + if err := durable.storeStillThere(); err != nil { + return nil, err + } return durable.sessions.Messages(ctx, sessionID) } @@ -318,7 +368,7 @@ type replayProjectionEvent struct { } func (durable *durableSessions) reconcileProjection(ctx context.Context) error { - return withAdvisoryFileLock(durable.lockPath, func() error { + return durable.withStoreLock(func() error { if err := durable.ensureProjectionReconcileSchema(ctx); err != nil { return err } @@ -983,6 +1033,105 @@ func latestJSONTimestamp(value any, fallback uint64) uint64 { return latest } +// realDirectory is dir made absolute and resolved through every link on the +// way to it. A path that cannot be resolved is kept as it was given, which is +// what the store was rooted on before it was resolved at all. +func realDirectory(dir string) string { + if absolute, err := filepath.Abs(dir); err == nil { + dir = absolute + } + if resolved, err := filepath.EvalSymlinks(dir); err == nil { + dir = resolved + } + return dir +} + +// withStoreLock holds the store's projection lock around fn, once the store +// is known to be the one the run opened. +func (durable *durableSessions) withStoreLock(fn func() error) error { + if err := durable.storeStillThere(); err != nil { + return err + } + err := withAdvisoryFileLock(durable.lockPath, fn) + if err != nil { + // A write the store's removal cut short says so by the store's name, + // not by the temporary file it was renaming when the removal landed. + if gone := durable.storeStillThere(); gone != nil { + return gone + } + } + return err +} + +// storeStillThere answers an error once the store's directory is no longer +// the one the run opened: gone, or another in its place. +// +// A STORE THAT WAS REMOVED ENDS THE RUN, AND NO OTHER STANDS IN FOR IT. The +// store is opened by its real path ([openDurableSessionsIn]), so the one way +// its directory can stop being the one the run opened is that the store was +// removed, and the session records in it with it. Writing on into a directory +// made again in its place — by this lock, or by anything else that writes +// under .senior-dev/ — would leave the database writing to the removed file +// and a flat store holding only what came after: the next turn finds no +// session, makes one under the same id, and the model reads a conversation +// that starts where the removal happened. The run ends instead, naming the +// store where it was. +// +// THE STORE'S DIRECTORY, ITS DATABASE AND ITS RECORDS ARE HELD OPEN, BECAUSE +// AN INODE NUMBER IS NOT A NAME FOREVER. Identity is the device and inode +// number, and ext4 and overlayfs — the benchmark's container, the CI runner — +// hand a removed directory's number to the next directory made, so `rm -rf` +// and `mkdir` of the same path can look like the same directory. An open +// handle keeps the removed one's inode allocated until the store closes, so +// the number cannot be given out again while the run could still be fooled +// by it. It used to be held by accident, by the database file sqlite keeps +// open; nothing promised that. +// +// A STORE EMPTIED IS A STORE REMOVED. An `rm -rf` that races the store's own +// writes can fail on a directory something wrote into while it ran, and +// leave the store's directory standing with its database and most of its +// records gone; the directory alone would still look like the one the run +// opened. The database and the records' directory are held and compared the +// same way. +func (durable *durableSessions) storeStillThere() error { + for _, held := range durable.storeHeld { + opened, err := held.Stat() + if err != nil { + return fmt.Errorf("senior-dev sessions: read its store %s: %w", durable.storeDir, err) + } + current, err := os.Stat(held.Name()) + if err == nil && os.SameFile(opened, current) { + continue + } + what := "; the directory there now is a new one" + if held.Name() != durable.storeDir { + what = "; its " + filepath.Base(held.Name()) + " there now is a new one" + } + if err != nil { + // The path is already in the sentence; the reason is all the stat + // adds. + var pathErr *fs.PathError + if errors.As(err, &pathErr) { + err = pathErr.Err + } + what = ": " + err.Error() + if held.Name() != durable.storeDir { + what = ": its " + filepath.Base(held.Name()) + " is gone" + } + } + return fmt.Errorf("senior-dev sessions: its store %s was removed while the run was working, "+ + "with the conversation in it%s", durable.storeDir, what) + } + return nil +} + +// closeAll closes every file, ignoring what closing says. +func closeAll(files []*os.File) { + for _, file := range files { + _ = file.Close() + } +} + func withAdvisoryFileLock(path string, fn func() error) error { file, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) if err != nil { @@ -1009,4 +1158,5 @@ func (durable *durableSessions) Close() { if durable.db != nil { _ = durable.db.Close() } + closeAll(durable.storeHeld) } diff --git a/internal/seniordev/app/durable_state_dir_test.go b/internal/seniordev/app/durable_state_dir_test.go new file mode 100644 index 0000000000..b3955aeb30 --- /dev/null +++ b/internal/seniordev/app/durable_state_dir_test.go @@ -0,0 +1,439 @@ +//go:build !windows + +package app + +import ( + "context" + "errors" + "io/fs" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/Agent-Field/codeaf/internal/seniordev/session/sessioncore" +) + +// THE STORE IS OPENED BY ITS REAL PATH, ONCE. A benchmark rig anchored the +// store outside the folder through a link at /.senior-dev; something +// removed the link an hour into the run, and the next model turn ended it with +// `open /.senior-dev/projection.lock: no such file or directory` while +// the store it pointed at was intact. Every later open goes to the store +// itself, so the link may go away without the run noticing, and nothing is +// made again at the link's place: a fresh directory there would be a second +// store beside the one the database still writes to. +func TestDurableSurvivesTheWorkspaceLinkGoingAway(t *testing.T) { + ctx := context.Background() + workspace := t.TempDir() + store := t.TempDir() + link := filepath.Join(workspace, seniorDevDataDirectory) + if err := os.Symlink(store, link); err != nil { + t.Fatal(err) + } + durable, err := openDurableSessions(ctx, workspace) + if err != nil { + t.Fatal(err) + } + t.Cleanup(durable.Close) + session, err := durable.CreateSession(ctx, sessioncore.CreateInput{Title: "anchored"}) + if err != nil { + t.Fatal(err) + } + + if err := os.Remove(link); err != nil { + t.Fatal(err) + } + if err := durable.TouchSession(ctx, session.ID); err != nil { + t.Fatalf("a touch after the link went away: %v", err) + } + if _, err := persistTurnPrompt(ctx, durable, session.ID, "msg_after_link", turn{ + Agent: "coder", ProviderID: "p", ModelID: "m", Prompt: "still here", + }); err != nil { + t.Fatalf("a message after the link went away: %v", err) + } + + stored := filepath.Join(store, "storage", "message", session.ID, "msg_after_link.json") + if _, err := os.Stat(stored); err != nil { + t.Fatalf("the message is not in the store the link pointed at: %v", err) + } + var projected int + if err := durable.db.QueryRow("SELECT COUNT(*) FROM message WHERE id = 'msg_after_link'").Scan(&projected); err != nil || projected != 1 { + t.Fatalf("projected message = %d, %v; want 1", projected, err) + } + if _, err := os.Lstat(link); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("%s was made again after the link went away (%v): a second store beside the real one", link, err) + } +} + +// A STATE DIRECTORY HOLDS THE WHOLE STORE, AND THE FOLDER NONE OF IT. Named on +// the command line or in SENIOR_DEV_STATE_DIR — the flag winning when both +// are set — the database, the flat records and the lock all live there, and +// the folder's .senior-dev is left to the files the model works with. +func TestStateDirRootsTheStoreOutsideTheWorkspace(t *testing.T) { + for _, testCase := range []struct { + name string + flagged bool + variable bool + }{ + {name: "flag", flagged: true}, + {name: "variable", variable: true}, + {name: "flag over variable", flagged: true, variable: true}, + } { + t.Run(testCase.name, func(t *testing.T) { + ctx := context.Background() + workspace := t.TempDir() + stateDir := filepath.Join(t.TempDir(), "run-1", "store") + flagged, other := "", t.TempDir() + t.Setenv(StateDirEnv, "") + switch { + case testCase.flagged && testCase.variable: + flagged = stateDir + t.Setenv(StateDirEnv, other) + case testCase.flagged: + flagged = stateDir + case testCase.variable: + t.Setenv(StateDirEnv, stateDir) + } + dir, refusal := stateDirectory(flagged, workspace) + if refusal != "" { + t.Fatalf("refused: %s", refusal) + } + if want := realDirectory(stateDir); dir != want { + t.Fatalf("state directory = %q, want %q", dir, want) + } + durable, err := openDurableSessionsIn(ctx, workspace, dir) + if err != nil { + t.Fatal(err) + } + t.Cleanup(durable.Close) + session, err := durable.CreateSession(ctx, sessioncore.CreateInput{Title: "elsewhere"}) + if err != nil { + t.Fatal(err) + } + if _, err := persistTurnPrompt(ctx, durable, session.ID, "msg_elsewhere", turn{ + Agent: "coder", ProviderID: "p", ModelID: "m", Prompt: "kept outside", + }); err != nil { + t.Fatal(err) + } + for _, name := range []string{seniorDevDatabaseFile, "storage", "projection.lock"} { + if _, err := os.Stat(filepath.Join(stateDir, name)); err != nil { + t.Errorf("%s is not in the state directory: %v", name, err) + } + } + if _, err := os.Stat(filepath.Join(stateDir, "storage", "message", session.ID, "msg_elsewhere.json")); err != nil { + t.Errorf("the conversation is not in the state directory: %v", err) + } + if _, err := os.Lstat(filepath.Join(workspace, seniorDevDataDirectory)); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("the folder holds a .senior-dev the store made (%v)", err) + } + if entries, _ := os.ReadDir(other); len(entries) != 0 { + t.Errorf("the variable's directory holds %d entries although the flag named another", len(entries)) + } + }) + } +} + +// A STATE DIRECTORY INSIDE THE FOLDER IS REFUSED BEFORE ANYTHING IS SPENT. The +// run's checkpoints and the change it hands in are taken from the folder's +// tree, and would carry the database and the conversation with them — a +// checkpoint restored under an open database would rewrite it. The folder's +// own .senior-dev is the one place in it they already leave alone, and a link +// is judged by where it leads. A refused directory is never made: the empty +// levels of one would be in the tree for the next checkpoint to carry. +func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { + workspace := t.TempDir() + outside := t.TempDir() + if err := os.Mkdir(filepath.Join(workspace, "kept"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(workspace, "kept"), filepath.Join(outside, "into-the-folder")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(workspace, "out-of-the-folder")); err != nil { + t.Fatal(err) + } + t.Setenv(StateDirEnv, "") + for _, refusedDir := range []string{ + workspace, + filepath.Join(workspace, "state"), + filepath.Join(workspace, "a", "b", "c"), + filepath.Join(outside, "into-the-folder"), + filepath.Join(outside, "into-the-folder", "deeper", "store"), + } { + if _, refusal := stateDirectory(refusedDir, workspace); !strings.Contains(refusal, "--state-dir "+refusedDir+" is inside the folder") { + t.Errorf("--state-dir %s: refusal = %q, want it refused as inside the folder", refusedDir, refusal) + } + } + t.Setenv(StateDirEnv, filepath.Join(workspace, "state")) + if _, refusal := stateDirectory("", workspace); !strings.HasPrefix(refusal, StateDirEnv+" ") { + t.Errorf("refusal = %q, want it to name %s, which is where the directory came from", refusal, StateDirEnv) + } + t.Setenv(StateDirEnv, "") + for _, unmade := range []string{ + filepath.Join(workspace, "state"), + filepath.Join(workspace, "a"), + filepath.Join(workspace, "kept", "deeper"), + } { + if _, err := os.Lstat(unmade); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("a refusal left %s in the folder (%v)", unmade, err) + } + } + for _, kept := range []string{ + filepath.Join(workspace, seniorDevDataDirectory), + filepath.Join(workspace, seniorDevDataDirectory, "store"), + filepath.Join(workspace, "out-of-the-folder", "store"), + } { + if _, refusal := stateDirectory(kept, workspace); refusal != "" { + t.Errorf("--state-dir %s was refused: %s", kept, refusal) + } + } + // Where the file system folds case, the folder spelled another way is + // still the folder. + cased := filepath.Join(t.TempDir(), "Folder") + if err := os.Mkdir(cased, 0o755); err != nil { + t.Fatal(err) + } + respelled := filepath.Join(filepath.Dir(cased), "FOLDER") + if same, err := os.Stat(respelled); err == nil { + if original, _ := os.Stat(cased); os.SameFile(same, original) { + if _, refusal := stateDirectory(filepath.Join(respelled, "state"), cased); !strings.Contains(refusal, "is inside the folder") { + t.Errorf("--state-dir %s/state: refusal = %q, want the folder %s refused however it is spelled", respelled, refusal, cased) + } + if _, err := os.Lstat(filepath.Join(cased, "state")); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("a refusal left state in the folder (%v)", err) + } + } + } + blocker := filepath.Join(outside, "a-file") + if err := os.WriteFile(blocker, nil, 0o644); err != nil { + t.Fatal(err) + } + if _, refusal := stateDirectory(filepath.Join(blocker, "store"), workspace); !strings.Contains(refusal, "cannot be made") { + t.Errorf("refusal = %q, want a directory that cannot be made refused as one", refusal) + } +} + +// A STORE THAT WAS REMOVED ENDS THE RUN, NAMED BY ITS REAL PATH, AND NO OTHER +// STANDS IN FOR IT. Its records went with it; writing on into a directory made +// again in its place would let the next turn make the session again under the +// same id and hand the model a conversation that starts where the removal +// happened. In the folder's own .senior-dev the directory is made again by +// whatever writes the model's files there next, so the lock is not what can +// be trusted to notice: the store checks it is still the directory it opened. +// +// The "alone" cases close the database first, so that nothing but the store +// itself holds anything in the directory open. ext4 and overlayfs give a +// directory made where one was just removed the removed one's inode number +// back unless something keeps that inode allocated; the database file sqlite +// keeps open used to do it by accident, and these cases fail on Linux when +// the store does not hold its own directory open. APFS never reuses an inode +// number, so on a Mac they pass either way. +func TestARemovedStoreEndsTheRunWithoutAFreshOne(t *testing.T) { + ctx := context.Background() + for _, testCase := range []struct { + name string + // linked keeps the store behind a link given as its state directory; + // false keeps it in the folder's own .senior-dev. + linked bool + // remade is what is made where the store was, after it is removed. + remade string + // alone closes the database before the store is removed. + alone bool + // emptied removes what is in the store and leaves its directory, as + // an `rm -rf` that raced the store's own writes did. + emptied bool + said string + }{ + {name: "gone", linked: true, said: ": no such file or directory"}, + {name: "replaced", linked: true, remade: ".", said: "; the directory there now is a new one"}, + {name: "in the folder, remade by a write under it", remade: "tool-output", said: "; the directory there now is a new one"}, + {name: "replaced, alone", linked: true, remade: ".", alone: true, said: "; the directory there now is a new one"}, + {name: "in the folder, remade by a write under it, alone", remade: "tool-output", alone: true, said: "; the directory there now is a new one"}, + {name: "emptied, its directory left standing", emptied: true, said: ": its senior-dev.db is gone"}, + } { + t.Run(testCase.name, func(t *testing.T) { + workspace := t.TempDir() + stateDir := "" + if testCase.linked { + stateDir = filepath.Join(t.TempDir(), "anchor") + if err := os.Symlink(t.TempDir(), stateDir); err != nil { + t.Fatal(err) + } + } + durable, err := openDurableSessionsIn(ctx, workspace, stateDir) + if err != nil { + t.Fatal(err) + } + t.Cleanup(durable.Close) + session, err := durable.CreateSession(ctx, sessioncore.CreateInput{Title: "removed"}) + if err != nil { + t.Fatal(err) + } + store := durable.storeDir + if testCase.alone { + if err := durable.db.Close(); err != nil { + t.Fatal(err) + } + } + if testCase.emptied { + entries, _ := os.ReadDir(store) + for _, entry := range entries { + if err := os.RemoveAll(filepath.Join(store, entry.Name())); err != nil { + t.Fatal(err) + } + } + } else if err := os.RemoveAll(store); err != nil { + t.Fatal(err) + } + if testCase.remade != "" { + if err := os.MkdirAll(filepath.Join(store, testCase.remade), 0o755); err != nil { + t.Fatal(err) + } + } + + err = durable.TouchSession(ctx, session.ID) + want := "senior-dev sessions: its store " + store + " was removed while the run was working, with the conversation in it" + + testCase.said + if err == nil || err.Error() != want { + t.Fatalf("touch after the store was removed = %v, want %q", err, want) + } + if _, err := durable.Messages(ctx, session.ID); err == nil || err.Error() != want { + t.Fatalf("the conversation read after the store was removed = %v, want %q", err, want) + } + if _, err := persistTurnPrompt(ctx, durable, session.ID, "msg_amnesia", turn{ + Agent: "coder", ProviderID: "p", ModelID: "m", Prompt: "a conversation with no past", + }); err == nil { + t.Fatal("a message was written after the store was removed") + } + if count, _ := countStoredJSON(store); count != 0 { + t.Fatalf("%d records were written where the removed store was", count) + } + if _, err := os.Lstat(filepath.Join(store, "projection.lock")); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("a lock was made where the removed store was (%v)", err) + } + }) + } +} + +// A RUN CODEAF CARRIES KEEPS ITS STORE IN ITS RECORD FOLDER WHEN NOTHING ELSE +// NAMES ONE. The folder is the work's, and what the work runs can delete it; +// the record folder is codeaf's. Where is decided early and made only when the +// store opens, so a launch refused in between leaves nothing there; each +// launch gets a directory of its own — a task's folder outlives a run — a run +// nobody carried keeps the folder's .senior-dev, and a record folder inside +// the workspace (a run in the home folder) is not used and not made. +func TestTheRecordFolderHoldsTheStoreWhenNothingElseNamesOne(t *testing.T) { + workspace := t.TempDir() + records := filepath.Join(t.TempDir(), "carried", "senior-dev", "20261009-120000.000000") + + base := recordStoreBase(records, workspace) + if base != resolvedPath(filepath.Join(records, "store")) { + t.Fatalf("the record folder's store = %q, want %s/store", base, records) + } + if _, err := os.Lstat(records); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("deciding where the store goes made %s (%v); it is made when the store opens", records, err) + } + if first := claimRecordStore(base); first != realDirectory(filepath.Join(records, "store")) { + t.Fatalf("the first launch's store = %q, want %s/store", first, records) + } + if second := claimRecordStore(base); second != realDirectory(filepath.Join(records, "store.1")) { + t.Fatalf("a second launch's store = %q, want %s/store.1 beside the first", second, records) + } + + if base := recordStoreBase("", workspace); base != "" || claimRecordStore(base) != "" { + t.Errorf("with no record folder the store = %q, want the folder's own .senior-dev", base) + } + inside := filepath.Join(workspace, ".codeaf", "v3", "carried", "senior-dev", "run") + if base := recordStoreBase(inside, workspace); base != "" { + t.Errorf("with the record folder inside the workspace the store = %q, want the folder's own .senior-dev", base) + } + if _, err := os.Lstat(filepath.Join(workspace, ".codeaf")); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("a record folder inside the workspace was made (%v)", err) + } +} + +// A NOTE FILE THAT IS GONE IS WRITTEN BACK AS THE RUN LAST READ IT. What the +// work runs can empty .senior-dev; the brief, the checklist and the pinned +// command are the model's to read by name. A file the run never read is not +// invented, and one that is there is kept as it now reads. +func TestARemovedNoteIsWrittenBackFromWhatTheRunLastRead(t *testing.T) { + workspace := t.TempDir() + runner := &pipeline{workspace: workspace} + notes := filepath.Join(workspace, seniorDevDataDirectory) + if err := os.MkdirAll(notes, 0o755); err != nil { + t.Fatal(err) + } + for name, content := range map[string]string{"spec.md": "the brief\n", "checklist.md": "- [ ] one\n"} { + if err := os.WriteFile(filepath.Join(notes, name), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + runner.tendNotes() + if err := os.WriteFile(filepath.Join(notes, "checklist.md"), []byte("- [x] one\n"), 0o644); err != nil { + t.Fatal(err) + } + runner.tendNotes() + + if err := os.RemoveAll(notes); err != nil { + t.Fatal(err) + } + if data, err := runner.readNote(seniorDevChecklist); err != nil || string(data) != "- [x] one\n" { + t.Fatalf("the checklist read after .senior-dev went = %q, %v; want its last content", data, err) + } + runner.tendNotes() + for name, want := range map[string]string{"spec.md": "the brief\n", "checklist.md": "- [x] one\n"} { + if data, err := os.ReadFile(filepath.Join(notes, name)); err != nil || string(data) != want { + t.Errorf(".senior-dev/%s = %q, %v; want %q written back", name, data, err, want) + } + } + if _, err := os.Lstat(filepath.Join(notes, "pinned.txt")); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("a pinned command the run never read was made up (%v)", err) + } +} + +// A WRITE-BACK NEVER OVERWRITES WHAT THE MODEL WROTE MEANWHILE. Tool calls run +// at once: one call's command has just removed the checklist and its +// write-back runs while another call writes the model's new checklist and +// reads it. Whichever lands first, the file ends as the model wrote it and the +// run keeps that, never the old copy. +func TestAWriteBackNeverOverwritesAFileWrittenMeanwhile(t *testing.T) { + workspace := t.TempDir() + path := filepath.Join(workspace, filepath.FromSlash(seniorDevChecklist)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + for round := 0; round < 500; round++ { + runner := &pipeline{workspace: workspace} + if err := os.WriteFile(path, []byte("- [ ] old\n"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := runner.readNote(seniorDevChecklist); err != nil { + t.Fatal(err) + } + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + var group sync.WaitGroup + group.Add(2) + go func() { + defer group.Done() + _, _ = runner.readNote(seniorDevChecklist) + }() + go func() { + defer group.Done() + _ = os.WriteFile(path, []byte("- [x] new\n"), 0o644) + _, _ = runner.readNote(seniorDevChecklist) + }() + group.Wait() + if data, _ := os.ReadFile(path); string(data) != "- [x] new\n" { + t.Fatalf("round %d: the checklist is %q after a write-back ran beside the model's write; want the model's", round, data) + } + if kept := string(runner.kept.files[seniorDevChecklist]); kept != "- [x] new\n" { + t.Fatalf("round %d: the run kept %q; want the model's checklist", round, kept) + } + } + if leftovers, _ := filepath.Glob(filepath.Join(filepath.Dir(path), ".write-back-*")); len(leftovers) != 0 { + t.Errorf("write-backs left %d files of their own in .senior-dev", len(leftovers)) + } +} diff --git a/internal/seniordev/app/notes_kept.go b/internal/seniordev/app/notes_kept.go new file mode 100644 index 0000000000..7cd7eb6d98 --- /dev/null +++ b/internal/seniordev/app/notes_kept.go @@ -0,0 +1,127 @@ +//go:build !windows + +package app + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + "sync" +) + +// keptNoteFiles are the files in the folder's .senior-dev that the run and its +// model work from: the brief, the checklist, the pinned command and the +// messages taken. senior-dev writes the brief and the messages; the model +// writes the checklist and the pinned command with its own tools. +var keptNoteFiles = []string{seniorDevSpec, seniorDevChecklist, seniorDevPinned, steeringFile} + +// keptNotes is what each of [keptNoteFiles] held the last time the run read +// it, keyed by its path relative to the workspace. +type keptNotes struct { + mu sync.Mutex + files map[string][]byte +} + +// readNote is a note file's content: what the folder holds now, kept for +// later; or, when the file is gone, what it last held, written back first. +// +// A .SENIOR-DEV REMOVED MID-RUN IS WRITTEN BACK FROM WHAT THE RUN LAST READ. +// The folder is the work's, and what the work runs can empty it: a +// benchmark's validate.py began `sudo rm -rf /src`, and OpenSSL 1.1.0's `make +// clean` deletes every link in the tree. The session store is kept elsewhere +// when there is anywhere else to keep it ([recordStore]); these files are +// the model's, it reads them by name, and a missing brief or checklist would +// end the run for want of a file the run itself knows the words of. Nothing +// else is mirrored, and nothing is retried: a file never read cannot be +// written back, and a write that fails is the read's error. +// +// A WRITE-BACK NEVER OVERWRITES. Tool calls run at once, so the model's own +// write of its checklist can land between this read finding the file gone and +// the write-back; the old copy put in its place would be a change of the +// model's lost without a word. The whole read holds kept.mu, and the copy is +// linked into place only where nothing is ([writeBackNote]): a file something +// wrote in the meantime is what is read and kept. +func (runner *pipeline) readNote(name string) ([]byte, error) { + runner.kept.mu.Lock() + defer runner.kept.mu.Unlock() + return runner.readNoteLocked(name) +} + +// readNoteLocked is [pipeline.readNote] for a caller that holds kept.mu. +func (runner *pipeline) readNoteLocked(name string) ([]byte, error) { + path := filepath.Join(runner.workspace, filepath.FromSlash(name)) + data, err := os.ReadFile(path) + if err == nil { + if runner.kept.files == nil { + runner.kept.files = map[string][]byte{} + } + runner.kept.files[name] = data + return data, nil + } + last, known := runner.kept.files[name] + if !known || !errors.Is(err, fs.ErrNotExist) { + return nil, err + } + rewrote, err := writeBackNote(path, last) + if err != nil { + return nil, err + } + if !rewrote { + data, err := os.ReadFile(path) + if err != nil { + return nil, err + } + runner.kept.files[name] = data + return data, nil + } + runner.events.stage("implement", notesRewrittenStatus, map[string]any{"file": name, "bytes": len(last)}) + return last, nil +} + +// writeBackNote puts data at path only where nothing is, and all at once: it is +// written to a file of its own beside path and linked into place, so a file +// something else wrote in the meantime is never overwritten and path never +// holds half of it. It answers false when something was already there. +func writeBackNote(path string, data []byte) (bool, error) { + directory := filepath.Dir(path) + if err := os.MkdirAll(directory, 0o755); err != nil { + return false, err + } + temporary, err := os.CreateTemp(directory, ".write-back-*") + if err != nil { + return false, err + } + defer os.Remove(temporary.Name()) + _, err = temporary.Write(data) + if closeErr := temporary.Close(); err == nil { + err = closeErr + } + if err == nil { + err = os.Chmod(temporary.Name(), 0o644) + } + if err != nil { + return false, err + } + if err := os.Link(temporary.Name(), path); err != nil { + if errors.Is(err, fs.ErrExist) { + return false, nil + } + return false, err + } + return true, nil +} + +// notesRewrittenStatus is the implement stage's record of a note file written +// back ([pipeline.readNote]). +const notesRewrittenStatus = "notes-rewritten" + +// tendNotes reads every note file, keeping what each says and writing back +// each one that is gone. It runs after every tool call, which is where a +// command the model ran can have removed them, so the model's next read of +// its own brief finds it. +func (runner *pipeline) tendNotes() { + for _, name := range keptNoteFiles { + _, _ = runner.readNote(name) + } +} diff --git a/internal/seniordev/app/pipeline.go b/internal/seniordev/app/pipeline.go index e893be64ea..7c5af8ed64 100644 --- a/internal/seniordev/app/pipeline.go +++ b/internal/seniordev/app/pipeline.go @@ -62,6 +62,10 @@ type pipeline struct { fingerprintFiles map[string]worktreeFileFingerprint fingerprintNonce uint64 + // kept is what the note files in .senior-dev last held, so a removed one + // can be written back ([pipeline.readNote]). + kept keptNotes + // verificationTimeouts remembers entrypoints that hung at the verification // ceiling so a second pass does not pay the full ceiling again for an // identical command against an unchanged tree. @@ -132,7 +136,13 @@ func newPipeline(args cliArgs, workspace string, deps pipelineDeps) *pipeline { if aware, ok := deps.Backend.(adaptiveRouterBackend); ok { aware.setAdaptiveRouter(router) } - runtime := newConfiguredRuntime(workspace, deps.Backend, deps.Config) + // The record folder's store is made only now, when the store opens and + // every refusal is behind the run. + stateDir := args.StateDir + if stateDir == "" { + stateDir = claimRecordStore(args.RecordStore) + } + runtime := newConfiguredRuntime(workspace, stateDir, deps.Backend, deps.Config) runtime.now = now runtime.events = deps.Events if deps.Events != nil && runtime.bus != nil { diff --git a/internal/seniordev/app/run.go b/internal/seniordev/app/run.go index c51fba5b00..0a1d23fce8 100644 --- a/internal/seniordev/app/run.go +++ b/internal/seniordev/app/run.go @@ -79,6 +79,13 @@ type Options struct { // have chosen; empty leaves that kind to discovery. VerifyBuild string VerifyTest string + // StateDir is the directory `--state-dir` named: the run keeps its session + // store there — its database, its conversation and the lock that orders + // them. Empty reads SENIOR_DEV_STATE_DIR; with that empty too, the store + // goes in the run's record folder when codeaf named one, and in the + // folder's own .senior-dev only when it named none. The files the model + // works with stay in the folder's .senior-dev either way ([stateDirectory]). + StateDir string // Crew says the pools came from the crew of the conversation that started // the run (`--crew`), not from a person typing them: a model the catalog // cannot size is dropped with a note, and a --high left empty routes on @@ -158,6 +165,18 @@ func runWith(ctx context.Context, host delegate.Host, options Options, notes io. args.MaxHours = &ceilings.Hours } workspace := host.Workspace() + records := "" + if recorder, ok := host.(delegate.Recorder); ok { + records = recorder.Records() + } + stateDir, refusal := stateDirectory(options.StateDir, workspace) + if refusal != "" { + return refused(refusal) + } + args.StateDir = stateDir + if stateDir == "" { + args.RecordStore = recordStoreBase(records, workspace) + } loadedConfig, err := loadSeniorDevConfig(workspace) if err != nil { return refused("load config: " + err.Error()) diff --git a/internal/seniordev/app/runtime.go b/internal/seniordev/app/runtime.go index 64a27d6c0b..8fd0c14c51 100644 --- a/internal/seniordev/app/runtime.go +++ b/internal/seniordev/app/runtime.go @@ -198,11 +198,13 @@ func (runtime *runtimeAdapter) compactionProvenance(configured turn) map[string] return record } -func newConfiguredRuntime(workspace string, client backend, cfg *seniorDevConfig) *runtimeAdapter { +// newConfiguredRuntime is the runtime for one run in workspace, its session +// store in stateDir or, with that empty, in the workspace's own .senior-dev. +func newConfiguredRuntime(workspace, stateDir string, client backend, cfg *seniorDevConfig) *runtimeAdapter { runtime := &runtimeAdapter{ backend: client, config: cfg, workspace: workspace, now: time.Now, } - runtime.durable, runtime.initErr = openDurableSessions(context.Background(), workspace) + runtime.durable, runtime.initErr = openDurableSessionsIn(context.Background(), workspace, stateDir) if runtime.durable != nil && runtime.durable.bus != nil { runtime.bus = runtime.durable.bus } else { diff --git a/internal/seniordev/app/solo.go b/internal/seniordev/app/solo.go index 3d0cf42d06..c0559205ac 100644 --- a/internal/seniordev/app/solo.go +++ b/internal/seniordev/app/solo.go @@ -19,6 +19,7 @@ import ( "github.com/Agent-Field/codeaf/internal/provider" "github.com/Agent-Field/codeaf/internal/seniordev/baked" "github.com/Agent-Field/codeaf/internal/seniordev/engine/msgmodel" + "github.com/Agent-Field/codeaf/internal/seniordev/engine/steploop" "github.com/Agent-Field/codeaf/internal/seniordev/netpolicy" "github.com/Agent-Field/codeaf/internal/seniordev/project" "github.com/Agent-Field/codeaf/internal/seniordev/tool" @@ -221,6 +222,9 @@ func (runner *pipeline) soloIntake(goal string) error { if err := os.WriteFile(specPath, []byte(goal), 0o644); err != nil { return fmt.Errorf("solo intake: %w", err) } + // Read back at once, so the brief can be written again from the first + // moment something could remove it. + _, _ = runner.readNote(seniorDevSpec) runner.events.stage("intake", "captured", map[string]any{ "spec_path": ".senior-dev/spec.md", "spec_bytes": len(goal), }) @@ -396,7 +400,15 @@ func (runner *pipeline) soloTurn(ctx context.Context, goal, prompt string) (turn configured.Tools = runner.runtime.definitionsFor( configured.ProviderID, configured.ModelID, "coder", nil, ) - configured.Execute = runner.runtime.registry.Execute + // After every tool call the note files are read again: what they say is + // kept, and one a command removed is written back before the model reads + // it ([pipeline.tendNotes]). + execute := runner.runtime.registry.Execute + configured.Execute = func(ctx context.Context, call steploop.ToolCall) (steploop.ToolResult, error) { + result, err := execute(ctx, call) + runner.tendNotes() + return result, err + } configured.SystemInstructions = runner.runtime.registry.SystemInstructions(ctx) configured.LoadInstructions = runner.runtime.registry.SystemInstructions configured.AfterAssistant = runner.runtime.registry.ClearInstructionClaims @@ -622,7 +634,7 @@ func (runner *pipeline) soloUnsubmittedFindings(baseSHA string) []string { } else { findings = append(findings, "your pinned command is: "+pinned) } - if _, err := os.Stat(filepath.Join(runner.workspace, ".senior-dev", "checklist.md")); err != nil { + if _, err := runner.readNote(seniorDevChecklist); err != nil { findings = append(findings, "no .senior-dev/checklist.md exists — the request's own requirements were never enumerated") } @@ -637,7 +649,7 @@ func plural(count int, singular, many string) string { } func (runner *pipeline) readPinnedCommand() string { - data, err := os.ReadFile(filepath.Join(runner.workspace, ".senior-dev", "pinned.txt")) + data, err := runner.readNote(seniorDevPinned) if err != nil { return "" } @@ -780,7 +792,7 @@ type soloChecklist struct { var soloChecklistItem = regexp.MustCompile(`^\s*(?:[-*]\s*)?\[([ xX])\]\s`) func (runner *pipeline) soloChecklistState() soloChecklist { - raw, err := os.ReadFile(filepath.Join(runner.workspace, ".senior-dev", "checklist.md")) + raw, err := runner.readNote(seniorDevChecklist) if err != nil { return soloChecklist{} } diff --git a/internal/seniordev/app/state_dir.go b/internal/seniordev/app/state_dir.go new file mode 100644 index 0000000000..f2823649d0 --- /dev/null +++ b/internal/seniordev/app/state_dir.go @@ -0,0 +1,154 @@ +//go:build !windows + +package app + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" + + configpkg "github.com/Agent-Field/codeaf/internal/seniordev/config" +) + +// StateDirEnv names the directory a run keeps its session store in when +// `--state-dir` names none. +const StateDirEnv = "SENIOR_DEV_STATE_DIR" + +// recordStoreName is the directory in the run's record folder that a run codeaf +// carries keeps its session store in when nothing names one +// ([recordStoreBase]). +const recordStoreName = "store" + +// stateDirectory is where this run keeps its session store — its database, +// its conversation and the lock that orders them — when it was told: the +// --state-dir it was given, else SENIOR_DEV_STATE_DIR. "" leaves it to the +// run's record folder ([recordStoreBase]), else the workspace's own +// .senior-dev ([openDurableSessionsIn]). A directory that is inside the +// workspace is refused +// before anything is spent: the run's checkpoints and the change it hands in +// are taken from that tree, and would carry the store's files with them. Its +// own .senior-dev is the one place in the tree they already leave alone. The +// directory is made here once it is allowed, so one that cannot be is refused +// before anything is spent too. +// +// IT IS JUDGED BEFORE IT IS MADE. Where it would lead is read from the deepest +// part of it that already exists ([resolvedPath]); making it first would leave +// a refused /state, or every level of /a/b/c, in the tree the +// refusal was protecting, for the next checkpoint to carry. +func stateDirectory(flagged, workspace string) (dir, refusal string) { + dir, said := strings.TrimSpace(flagged), "--state-dir" + if dir == "" { + value, _ := configpkg.NewEnv(os.LookupEnv).Get(StateDirEnv) + dir, said = strings.TrimSpace(value), StateDirEnv + } + if dir == "" { + return "", "" + } + store, folder := resolvedPath(dir), realDirectory(workspace) + if within(store, folder) && !within(store, filepath.Join(folder, seniorDevDataDirectory)) { + return "", said + " " + dir + " is inside the folder senior-dev works in, whose checkpoints and " + + "handed-in change would carry the store; name a directory outside it" + } + if err := os.MkdirAll(store, 0o755); err != nil { + return "", said + " " + dir + " cannot be made: " + err.Error() + } + return realDirectory(store), "" +} + +// recordStoreBase is where in records, the run's record folder, a store of +// this launch's own is made when the store opens ([claimRecordStore]): +// resolved, and "" — the workspace's own .senior-dev — when codeaf named no +// record folder (senior-dev run directly, a test) or when it is inside the +// workspace, as it is for a run in the home folder. Nothing is made here, so +// a launch refused after it leaves nothing behind in the record folder. +// +// THE STORE IS KEPT OUT OF THE FOLDER WHEN THERE IS SOMEWHERE ELSE TO KEEP IT. +// The folder is the work's, and whatever the work runs can delete it: a +// benchmark's validate.py began `sudo rm -rf /src` and took a whole run's +// .senior-dev with it, conversation and all, and OpenSSL 1.1.0's `make clean` +// deletes every link in the tree, a .senior-dev that led elsewhere included. +// The record folder is codeaf's, and nothing the work runs is pointed at it. +func recordStoreBase(records, workspace string) string { + if strings.TrimSpace(records) == "" { + return "" + } + base := resolvedPath(filepath.Join(records, recordStoreName)) + if within(base, realDirectory(workspace)) { + return "" + } + return base +} + +// claimRecordStore makes a directory of this launch's own at base: base +// itself, or base.1, base.2 when an earlier run of the same task left one +// there, made with os.Mkdir so two launches can never be handed the same +// one. It is private to the person, as the record folder is. "" — the +// workspace's own .senior-dev — when base is "" or cannot be written. +func claimRecordStore(base string) string { + if base == "" { + return "" + } + if err := os.MkdirAll(filepath.Dir(base), 0o700); err != nil { + return "" + } + for n := 0; ; n++ { + at := base + if n > 0 { + at = fmt.Sprintf("%s.%d", base, n) + } + err := os.Mkdir(at, 0o700) + if err == nil { + return realDirectory(at) + } + if !errors.Is(err, fs.ErrExist) { + return "" + } + } +} + +// resolvedPath is path made absolute, with the deepest part of it that already +// exists resolved through every link and the rest joined back on. What does +// not exist yet cannot be a link, so this is where the path will lead once it +// is made. +func resolvedPath(path string) string { + if absolute, err := filepath.Abs(path); err == nil { + path = absolute + } + rest := "" + for at := path; ; { + if real, err := filepath.EvalSymlinks(at); err == nil { + return filepath.Join(real, rest) + } + parent := filepath.Dir(at) + if parent == at { + return path + } + rest = filepath.Join(filepath.Base(at), rest) + at = parent + } +} + +// within says whether path is dir or somewhere under it; both are clean and +// absolute. Spelling is not identity where the file system folds case, as +// APFS does by default: when the spellings differ, the directories on the +// way up path that already exist are compared with dir itself. +func within(path, dir string) bool { + if rel, err := filepath.Rel(dir, path); err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return true + } + want, err := os.Stat(dir) + if err != nil { + return false + } + for at := path; ; at = filepath.Dir(at) { + if info, err := os.Stat(at); err == nil && os.SameFile(info, want) { + return true + } + if filepath.Dir(at) == at { + return false + } + } +} diff --git a/internal/seniordev/app/steering.go b/internal/seniordev/app/steering.go index 3ecb4ec4b0..ad6e4f5b29 100644 --- a/internal/seniordev/app/steering.go +++ b/internal/seniordev/app/steering.go @@ -96,8 +96,16 @@ func (runner *pipeline) heardSteering(inbox delegate.Listener, messages []delega // keepSteering appends the messages to the steering file compaction pins. A // file that cannot be written costs the pin, never the message: the model is -// handed the words either way. +// handed the words either way. A steering file something removed is written +// back before it is appended to, so the messages taken before are not lost, +// and the append holds the kept notes' lock, so a write-back running beside it +// can neither land under it nor keep the file from before it +// ([pipeline.readNote]). func (runner *pipeline) keepSteering(messages []delegate.Message) { + runner.kept.mu.Lock() + defer runner.kept.mu.Unlock() + _, _ = runner.readNoteLocked(steeringFile) + defer func() { _, _ = runner.readNoteLocked(steeringFile) }() path := filepath.Join(runner.workspace, steeringFile) if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return diff --git a/internal/seniordev/app/testsupport_test.go b/internal/seniordev/app/testsupport_test.go index 51edeafe23..f76c79ca4b 100644 --- a/internal/seniordev/app/testsupport_test.go +++ b/internal/seniordev/app/testsupport_test.go @@ -255,7 +255,7 @@ func (backend *coderOnlyBackend) count(agent string) int { } func newRuntime(workspace string, client backend) *runtimeAdapter { - return newConfiguredRuntime(workspace, client, nil) + return newConfiguredRuntime(workspace, "", client, nil) } // Create opens a session for agent under parentID. Tests of the durable diff --git a/internal/seniordev/config/env.go b/internal/seniordev/config/env.go index b6e1cdfd29..229b0517b4 100644 --- a/internal/seniordev/config/env.go +++ b/internal/seniordev/config/env.go @@ -49,6 +49,7 @@ var VariableNames = []string{ "SENIOR_DEV_SCRATCH_ROOT", "SENIOR_DEV_SCRATCH_TTL_H", "SENIOR_DEV_SHARED_BUILD_CACHE", + "SENIOR_DEV_STATE_DIR", "SENIOR_DEV_WEBSEARCH_PROVIDER", "SENIOR_DEV_MAX_COST_USD", "SENIOR_DEV_MAX_WALL_H", diff --git a/internal/seniordev/seniordev.go b/internal/seniordev/seniordev.go index 8f8bd2c0ff..3fea3fd38a 100644 --- a/internal/seniordev/seniordev.go +++ b/internal/seniordev/seniordev.go @@ -92,9 +92,13 @@ var Program = delegate.Delegate{ // the home folder — because the recorder's own reading climbs to any // repository around the folder. PlainFolder: []string{"--in-place"}, - // Where it keeps its records in the folder it works in: the brief, the - // checklist, the pinned command, its session database and its model - // conversation (app's seniorDevDataDirectory, which git never sees). + // Where it keeps the files it and its model work from in the folder it + // works in: the brief, the checklist, the pinned command, the messages it + // was handed and the output it set aside (app's seniorDevDataDirectory, + // which git never sees). Its session database and model conversation are + // not here: they go in the run's record folder (CODEAF_RECORDS), or in + // --state-dir's directory, and here only when codeaf names no record + // folder (app's stateDirectory). Notes: ".senior-dev", // IT LISTENS: the person's words from its page and the conversation's // `say` reach its model between steps until it hands in (app/steering.go). @@ -190,6 +194,10 @@ func bindRun(fs *flag.FlagSet) delegate.Body { // whose build or test discovery cannot find (fullverification/declared.go). verifyBuild := fs.String("verify-build", "", "the project's build command, run on the frozen tree in place of the one discovered") verifyTest := fs.String("verify-test", "", "the project's test command, run on the frozen tree in place of the one discovered") + // Where the session store lives when the folder's own .senior-dev is not + // safe to keep it in: a rig that resets the folder, or one that collects the + // store after the run (app/state_dir.go). + stateDir := fs.String("state-dir", "", "keep the session database and conversation here, outside the folder (default: $"+app.StateDirEnv+", else .senior-dev)") return func(ctx context.Context, host delegate.Host, args []string) error { run(ctx, host, app.Options{ Goal: strings.Join(args, " "), @@ -200,6 +208,7 @@ func bindRun(fs *flag.FlagSet) delegate.Body { InPlace: *inPlace, VerifyBuild: *verifyBuild, VerifyTest: *verifyTest, + StateDir: *stateDir, Crew: *crew, Asked: *asked, }, os.Stderr) diff --git a/internal/seniordev/seniordev_test.go b/internal/seniordev/seniordev_test.go index f90d914b8b..a0922cfa31 100644 --- a/internal/seniordev/seniordev_test.go +++ b/internal/seniordev/seniordev_test.go @@ -61,6 +61,10 @@ func (h *recordingHost) Terminal(end delegate.Ending) { h.add(hostRecord{kind: "terminal", ending: end}) } +// Records names the run's record folder as codeaf's own child host does: from +// CODEAF_RECORDS, which codeaf sets on the line of every run it carries. +func (h *recordingHost) Records() string { return os.Getenv(delegate.EnvRecords) } + func (h *recordingHost) add(r hostRecord) { h.mu.Lock() defer h.mu.Unlock() @@ -92,6 +96,9 @@ type modelAPIServer struct { // hold, when set, blocks each request until the caller gives up, and says // so on the channel first. hold chan struct{} + // script, when set, is the model's side of the conversation in place of + // [scriptedReply]. + script func(call int) string } type seenRequest struct { @@ -120,6 +127,10 @@ func (s *modelAPIServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { }) call := len(s.requests) hold := s.hold + reply := scriptedReply + if s.script != nil { + reply = s.script + } s.mu.Unlock() w.Header().Set("Content-Type", "text/event-stream") w.WriteHeader(http.StatusOK) @@ -137,7 +148,7 @@ func (s *modelAPIServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { <-r.Context().Done() return } - _, _ = io.WriteString(w, scriptedReply(call)) + _, _ = io.WriteString(w, reply(call)) } func (s *modelAPIServer) seen() []seenRequest { @@ -201,6 +212,8 @@ func hermeticRun(t *testing.T) (workspace string, trap *atomic.Bool) { t.Setenv("SENIOR_DEV_PERMISSION", "") t.Setenv("SENIOR_DEV_NET", "allow") t.Setenv("SENIOR_DEV_SCRATCH_ROOT", t.TempDir()) + t.Setenv(app.StateDirEnv, "") + t.Setenv(delegate.EnvRecords, "") t.Setenv("SENIOR_DEV_DISABLE_MODELS_FETCH", "1") catalog, err := filepath.Abs(filepath.Join("modelsdev", "testdata", "catalog.json")) if err != nil { @@ -406,6 +419,235 @@ func TestTheRunCommandWorksATaskThroughTheModelAPIItIsGiven(t *testing.T) { } } +// --state-dir REACHES THE RUN, AND WINS OVER SENIOR_DEV_STATE_DIR. A whole run +// keeps its session database and its conversation in the directory the flag +// names; the variable's directory and the folder's .senior-dev hold none of +// them, and the brief is still written where the model is told to read it. +func TestTheStateDirFlagKeepsTheStoreWhereItSays(t *testing.T) { + workspace, _ := hermeticRun(t) + stateDir, variable, record := filepath.Join(t.TempDir(), "run-1"), t.TempDir(), t.TempDir() + t.Setenv(app.StateDirEnv, variable) + t.Setenv(delegate.EnvRecords, record) + api := httptest.NewServer(&modelAPIServer{}) + t.Cleanup(api.Close) + host := &recordingHost{ + workspace: workspace, + api: delegate.ModelAPI{BaseURL: api.URL + "/v1", Token: runToken}, + } + + err := runBody(t, context.Background(), host, "--state-dir", stateDir, + "--high", "openrouter/fixture/vendor-model", "--", "Add", "the", "feature.") + if err != nil { + t.Fatalf("the body answered an error: %v", err) + } + + records := host.snapshot() + if ending := records[len(records)-1].ending; ending.Status != delegate.StatusPass { + t.Fatalf("ending = %+v, want pass", ending) + } + for _, name := range []string{"senior-dev.db", "storage", "projection.lock"} { + if _, err := os.Stat(filepath.Join(stateDir, name)); err != nil { + t.Errorf("%s is not in the --state-dir: %v", name, err) + } + } + if entries, _ := os.ReadDir(variable); len(entries) != 0 { + t.Errorf("%s's directory holds %d entries although --state-dir named another", app.StateDirEnv, len(entries)) + } + if entries, _ := os.ReadDir(record); len(entries) != 0 { + t.Errorf("the record folder holds %d entries although --state-dir named another directory", len(entries)) + } + for _, name := range []string{"senior-dev.db", "storage", "projection.lock"} { + if _, err := os.Lstat(filepath.Join(workspace, ".senior-dev", name)); err == nil { + t.Errorf("the folder's .senior-dev holds %s", name) + } + } + if _, err := os.Stat(filepath.Join(workspace, ".senior-dev", "spec.md")); err != nil { + t.Errorf("the brief is not where the model reads it: %v", err) + } +} + +// A RUN CODEAF CARRIES KEEPS ITS STORE IN ITS RECORD FOLDER. With no +// --state-dir and no SENIOR_DEV_STATE_DIR, the session database, the records +// and the lock go in a directory of the run's own in the folder codeaf named +// on CODEAF_RECORDS, and the folder's .senior-dev holds only what the model +// works from. +func TestACarriedRunKeepsItsStoreInTheRecordFolder(t *testing.T) { + workspace, _ := hermeticRun(t) + record := t.TempDir() + t.Setenv(delegate.EnvRecords, record) + api := httptest.NewServer(&modelAPIServer{}) + t.Cleanup(api.Close) + host := &recordingHost{ + workspace: workspace, + api: delegate.ModelAPI{BaseURL: api.URL + "/v1", Token: runToken}, + } + + if err := runBody(t, context.Background(), host, + "--high", "openrouter/fixture/vendor-model", "--", "Add", "the", "feature."); err != nil { + t.Fatalf("the body answered an error: %v", err) + } + records := host.snapshot() + if ending := records[len(records)-1].ending; ending.Status != delegate.StatusPass { + t.Fatalf("ending = %+v, want pass", ending) + } + for _, name := range []string{"senior-dev.db", "storage", "projection.lock"} { + if _, err := os.Stat(filepath.Join(record, "store", name)); err != nil { + t.Errorf("%s is not in the record folder's store: %v", name, err) + } + } + assertNotesOnly(t, workspace) +} + +// A LAUNCH REFUSED BEFORE ITS FIRST CALL LEAVES NO STORE BEHIND. Where the +// store goes is decided early, but its directory is made only when the store +// opens, after every refusal, so the next launch of the task gets store and +// not store.1 beside an empty one. +func TestARefusedLaunchLeavesNoStoreInTheRecordFolder(t *testing.T) { + workspace, _ := hermeticRun(t) + record := t.TempDir() + t.Setenv(delegate.EnvRecords, record) + api := httptest.NewServer(&modelAPIServer{}) + t.Cleanup(api.Close) + host := &recordingHost{ + workspace: workspace, + api: delegate.ModelAPI{BaseURL: api.URL + "/v1", Token: runToken}, + } + + _ = runBody(t, context.Background(), host, + "--asked", "--high", "openrouter/nobody/knows-this-model", "--", "Add", "the", "feature.") + records := host.snapshot() + if ending := records[len(records)-1].ending; ending.Status != delegate.StatusCrashed { + t.Fatalf("ending = %+v, want the launch refused", ending) + } + if entries, _ := os.ReadDir(record); len(entries) != 0 { + t.Errorf("a refused launch left %d entries in the record folder", len(entries)) + } +} + +// SENIOR_DEV_STATE_DIR WINS OVER THE RECORD FOLDER, as --state-dir does: a +// directory a person named is where the store goes, and the record folder +// gets none. +func TestTheStateDirVariableWinsOverTheRecordFolder(t *testing.T) { + workspace, _ := hermeticRun(t) + record, variable := t.TempDir(), filepath.Join(t.TempDir(), "named") + t.Setenv(delegate.EnvRecords, record) + t.Setenv(app.StateDirEnv, variable) + api := httptest.NewServer(&modelAPIServer{}) + t.Cleanup(api.Close) + host := &recordingHost{ + workspace: workspace, + api: delegate.ModelAPI{BaseURL: api.URL + "/v1", Token: runToken}, + } + + if err := runBody(t, context.Background(), host, + "--high", "openrouter/fixture/vendor-model", "--", "Add", "the", "feature."); err != nil { + t.Fatalf("the body answered an error: %v", err) + } + if _, err := os.Stat(filepath.Join(variable, "senior-dev.db")); err != nil { + t.Errorf("the store is not where %s named: %v", app.StateDirEnv, err) + } + if entries, _ := os.ReadDir(record); len(entries) != 0 { + t.Errorf("the record folder holds %d entries although %s named another directory", len(entries), app.StateDirEnv) + } +} + +// A RUN NOBODY CARRIED KEEPS ITS STORE IN THE FOLDER'S .senior-dev, as before: +// with no record folder there is nowhere else that is the run's own. +func TestARunWithNoRecordFolderKeepsItsStoreInTheFolder(t *testing.T) { + workspace, _ := hermeticRun(t) + api := httptest.NewServer(&modelAPIServer{}) + t.Cleanup(api.Close) + host := &recordingHost{ + workspace: workspace, + api: delegate.ModelAPI{BaseURL: api.URL + "/v1", Token: runToken}, + } + + if err := runBody(t, context.Background(), host, + "--high", "openrouter/fixture/vendor-model", "--", "Add", "the", "feature."); err != nil { + t.Fatalf("the body answered an error: %v", err) + } + for _, name := range []string{"senior-dev.db", "storage", "projection.lock"} { + if _, err := os.Stat(filepath.Join(workspace, ".senior-dev", name)); err != nil { + t.Errorf("%s is not in the folder's .senior-dev: %v", name, err) + } + } +} + +// A .senior-dev THE WORK REMOVES MID-RUN IS WRITTEN BACK, AND THE RUN HANDS IN. +// The model's own command empties it, as a benchmark's restore script and +// OpenSSL's `make clean` did; the store is in the record folder and untouched, +// the brief and the checklist are written back as the run last read them, and +// the submit that reads the checklist next is accepted. +func TestANotesFolderTheWorkRemovesIsWrittenBack(t *testing.T) { + workspace, _ := hermeticRun(t) + record := t.TempDir() + t.Setenv(delegate.EnvRecords, record) + server := &modelAPIServer{script: func(call int) string { + switch call { + case 4: + return toolCall(call, "bash", map[string]any{"command": "rm -rf .senior-dev"}) + case 5: + return toolCall(call, "submit", map[string]any{ + "reason": "feature.txt now holds the feature", "evidence": "make test exits 0", + "checklist_satisfied": true, + }) + } + return scriptedReply(call) + }} + api := httptest.NewServer(server) + t.Cleanup(api.Close) + host := &recordingHost{ + workspace: workspace, + api: delegate.ModelAPI{BaseURL: api.URL + "/v1", Token: runToken}, + } + + if err := runBody(t, context.Background(), host, + "--high", "openrouter/fixture/vendor-model", "--", "Add", "the", "feature."); err != nil { + t.Fatalf("the body answered an error: %v", err) + } + records := host.snapshot() + if ending := records[len(records)-1].ending; ending.Status != delegate.StatusPass { + t.Fatalf("ending = %+v, want pass", ending) + } + if !slices.ContainsFunc(records, func(record hostRecord) bool { + return record.kind == "stage" && record.stage == "implement/notes-rewritten" + }) { + t.Error("no implement/notes-rewritten record says a note file was written back") + } + for name, want := range map[string]string{ + "spec.md": "Add the feature.", + "checklist.md": "- [x] the feature is implemented\n", + } { + if data, err := os.ReadFile(filepath.Join(workspace, ".senior-dev", name)); err != nil || string(data) != want { + t.Errorf(".senior-dev/%s = %q, %v; want %q written back", name, data, err, want) + } + } + messages, _ := filepath.Glob(filepath.Join(record, "store", "storage", "message", "*", "*.json")) + if len(messages) < 5 { + t.Errorf("the store in the record folder holds %d messages; the conversation should be whole", len(messages)) + } + assertNotesOnly(t, workspace) +} + +// assertNotesOnly fails the test when the folder's .senior-dev holds anything +// but the files the model works from: no database, no records, no lock. +func assertNotesOnly(t *testing.T, workspace string) { + t.Helper() + entries, err := os.ReadDir(filepath.Join(workspace, ".senior-dev")) + if err != nil { + t.Fatalf("the folder's .senior-dev: %v", err) + } + allowed := map[string]bool{ + "spec.md": true, "checklist.md": true, "pinned.txt": true, "steering.md": true, + "tool-output": true, "cmake-build": true, + } + for _, entry := range entries { + if !allowed[entry.Name()] { + t.Errorf("the folder's .senior-dev holds %s, which is not one of the model's files", entry.Name()) + } + } +} + // SIGTERM IS codeaf's STOP. The run's context ends while a model call is in // flight; the run starts nothing new, ships what it has and writes its one // terminal quickly, saying the work did not finish — not that it crashed.