From 66575561817a44622f196a22c5710a4491539a62 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Fri, 9 Oct 2026 10:04:04 -0400 Subject: [PATCH 1/4] senior-dev: open the store by its real path, and --state-dir to keep it outside the folder A CyberGym run had anchored /.senior-dev to a store elsewhere through a link. The projection lock was reopened through that link on every write, so when the link stopped resolving an hour in, the next model turn ended the run unsubmitted with `open /.senior-dev/projection.lock: no such file or directory` while the store itself was intact. The store's directory is now made, made absolute and resolved through every link once when it is opened; the database, the flat records and the lock are all rooted on that real path. `codeaf senior-dev run --state-dir DIR`, or SENIOR_DEV_STATE_DIR, keeps the store in a directory of its own (the flag wins); one inside the folder, other than under its own .senior-dev, is refused before the run starts. The model's files stay in the folder's .senior-dev either way. A store that is itself removed still ends the run, now naming it. It remembers which directory it opened and writes into no other: a fresh empty directory in its place (made by the next tool output under .senior-dev, or by anything else) 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. Co-Authored-By: Claude Opus 5.5 --- .../unreleased/1800-senior-dev-state-dir.md | 25 ++ internal/manual/chat/senior-dev.md | 48 +++- internal/manual/chat_test.go | 31 +++ internal/seniordev/app/args.go | 7 +- internal/seniordev/app/config_live_test.go | 4 +- internal/seniordev/app/durable_sessions.go | 91 ++++++- .../seniordev/app/durable_state_dir_test.go | 256 ++++++++++++++++++ internal/seniordev/app/pipeline.go | 2 +- internal/seniordev/app/run.go | 12 + internal/seniordev/app/runtime.go | 6 +- internal/seniordev/app/state_dir.go | 50 ++++ internal/seniordev/app/testsupport_test.go | 2 +- internal/seniordev/config/env.go | 1 + internal/seniordev/seniordev.go | 8 +- internal/seniordev/seniordev_test.go | 44 +++ 15 files changed, 566 insertions(+), 21 deletions(-) create mode 100644 docs/changes/unreleased/1800-senior-dev-state-dir.md create mode 100644 internal/seniordev/app/durable_state_dir_test.go create mode 100644 internal/seniordev/app/state_dir.go 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..e73a108b2e --- /dev/null +++ b/docs/changes/unreleased/1800-senior-dev-state-dir.md @@ -0,0 +1,25 @@ +--- +kind: fixed +title: senior-dev opens its store by its real path, and takes --state-dir to keep it outside the folder +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." + - "senior-dev's session database and conversation always lived in the folder's `.senior-dev/` and were moved into the task's record when the run ended. `codeaf senior-dev run --state-dir DIR`, or `SENIOR_DEV_STATE_DIR`, keeps them in DIR instead, where they stay; a DIR inside the folder, other than under its own `.senior-dev/`, is refused before the run starts." +--- + +A CyberGym run on openssl had anchored `/src/openssl/.senior-dev` to +`/home/agent/senior-dev-store` through a link. Sixty-four minutes in the link +stopped resolving, and because the projection lock was reopened through it for +every write, one failed `open()` ended the run unsubmitted after $0.64; the store +was collected intact afterwards. The same ending appeared earlier on arduinojson. + +A store that is itself removed still ends the run, now saying `its store +was removed while the run was working, with the conversation in it`, and it +does so even when something has made the directory again in the meantime — in +the folder's own `.senior-dev/`, the next tool output written there does. The +store remembers which directory it opened and writes into no other: in a fresh +one 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. Runs +in folders without git all share the project `global`, so a rig should give +each run a state directory of its own. diff --git a/internal/manual/chat/senior-dev.md b/internal/manual/chat/senior-dev.md index 675410b543..0f722a0c36 100644 --- a/internal/manual/chat/senior-dev.md +++ b/internal/manual/chat/senior-dev.md @@ -625,6 +625,38 @@ they never end up on a branch, and the next run in that folder never reads the l 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. +A run given `--state-dir DIR` (or `SENIOR_DEV_STATE_DIR`) keeps its session database and +its conversation in that directory instead, and they stay there: only what is in +`.senior-dev/` — the brief, the checklist, the pinned command, the output it set aside — +is moved into the record. + +## 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 keeps its session store — `senior-dev.db`, the `storage/` folder of its records +and `projection.lock` — in `.senior-dev/` in the folder it works in, unless it is given a +directory of its own. **It opens that store once, by its real path**: a `.senior-dev` that +is a link to somewhere else is followed when the run starts, and every later read and +write goes where the link led. So a link that goes away while it works no longer ends the +run. It used to: the lock was reopened through the link for every write, and the next +model turn ended the run with `open /.senior-dev/projection.lock: no such file or +directory` while the store itself was whole. + +**A store that is itself removed still ends the run**, because its conversation went with +it. senior-dev does not carry on in an empty directory made in its place, with a model +that has lost its work; it stops, saying +`its store was removed while the run was working, with the conversation in it`. +Whatever clears or restores the folder — a benchmark's reset script, `rm -rf` of the +tree — removes a store kept in it. + +To keep the store out of the folder, start `codeaf senior-dev run` with `--state-dir DIR`, +or set `SENIOR_DEV_STATE_DIR`; the flag wins when both are set, and the directory is made +if it is missing. **Use a new one for each run**: runs in folders without git share one +project, so a directory shared between them mixes their sessions. A directory inside the +folder is refused before anything is spent, because its checkpoints and the change it +hands in would carry the store; the folder's own `.senior-dev/` is the exception. The +brief, the checklist and the other files the model reads stay in `.senior-dev/` in the +folder either way. + ## Where does senior-dev put its work — its own branch, in a copy of its own, not merged, one commit In a git repository, codeaf cuts a branch of its own for the run (`task/-<id>`) from @@ -1114,7 +1146,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 <brief>` is `codeaf senior-dev run -- <brief>`. codeaf gives every program it carries four flags: @@ -1127,20 +1159,20 @@ program it carries four flags: senior-dev's own flags on `run`: - `--variant NAME` — how hard the coder thinks: `low`, `medium`, `high`, `xhigh`, `max`, - or `none` for the model's own default; `high` when unset. Its history summaries send - none; + or `none` for the model's own default; `high` when unset; - `--in-place` — work without git even inside a repository: no commits, and its - checkpoints kept outside the folder. A folder with no git history is worked that way - without it; codeaf passes it itself under a repository at your home folder; + checkpoints kept outside the folder. A folder with no git history needs no flag; + codeaf passes it itself under a repository at your home folder; - `--high`, `--low` — comma-separated models it routes among; `--low` (its history summaries) falls back to `--high`. On a shell run, each `--high` entry accepts a bare OpenRouter id, service-prefixed id, or short `/crew` model word; - `--asked` — the `--high` models were chosen by name, so one senior-dev cannot size ends the run before its first call rather than being skipped; - `--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); -- `--frontier` — accepted, and changes nothing: no call senior-dev makes uses that tier; + run by senior-dev on the submitted tree instead of the one it would have found; +- `--state-dir DIR` — keep its session database and conversation in DIR, outside the + folder (`SENIOR_DEV_STATE_DIR` when unset); a new one for each run; +- `--frontier` — accepted; no call 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..5a8b6c8f3e 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,35 @@ 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. +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"}, + } { + 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/app/args.go b/internal/seniordev/app/args.go index a6658880f1..f17b8213fb 100644 --- a/internal/seniordev/app/args.go +++ b/internal/seniordev/app/args.go @@ -70,8 +70,11 @@ type cliArgs struct { // leaves that kind to discovery (fullverification/declared.go). VerifyBuild string VerifyTest string - MaxCost *float64 - MaxHours *float64 + // StateDir is where the session store lives, resolved through its links; + // empty keeps it in the workspace's own .senior-dev ([stateDirectory]). + StateDir string + MaxCost *float64 + MaxHours *float64 } // CrewModel is a crew seat's model as a pool entry: the id filed under the 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..0655de93c7 100644 --- a/internal/seniordev/app/durable_sessions.go +++ b/internal/seniordev/app/durable_sessions.go @@ -66,19 +66,49 @@ type durableSessions struct { unsubscribe func() projector *projectors.Store lockPath string + // storeDir is the store's real path, and storeInfo the directory found + // there when it was opened ([durableSessions.storeStillThere]). + storeDir string + storeInfo os.FileInfo 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 <folder>/.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) + storeInfo, err := os.Stat(dataDir) + if err != nil { + return nil, fmt.Errorf("senior-dev sessions: read data directory: %w", err) + } projectInfo, _, err := project.Discover(ctx, workspace) if err != nil { @@ -112,6 +142,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, storeInfo: storeInfo, } durable.projectionSource = durable.store sessions, err := sessioncore.New(sessioncore.Options{ @@ -233,7 +264,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() @@ -318,7 +349,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 +1014,58 @@ 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 + } + return withAdvisoryFileLock(durable.lockPath, fn) +} + +// 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. +func (durable *durableSessions) storeStillThere() error { + if durable.storeInfo == nil { + return nil + } + current, err := os.Stat(durable.storeDir) + switch { + case err == nil && os.SameFile(durable.storeInfo, current): + return nil + case err == nil: + return fmt.Errorf("senior-dev sessions: its store %s was removed while the run was working, "+ + "with the conversation in it; the directory there now is a new one", durable.storeDir) + default: + return fmt.Errorf("senior-dev sessions: its store %s was removed while the run was working, "+ + "with the conversation in it: %w", durable.storeDir, err) + } +} + func withAdvisoryFileLock(path string, fn func() error) error { file, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) if err != nil { 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..9dccc2b99e --- /dev/null +++ b/internal/seniordev/app/durable_state_dir_test.go @@ -0,0 +1,256 @@ +//go:build !windows + +package app + +import ( + "context" + "errors" + "io/fs" + "os" + "path/filepath" + "strings" + "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 <folder>/.senior-dev; something +// removed the link an hour into the run, and the next model turn ended it with +// `open <folder>/.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. +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(outside, "into-the-folder"), + } { + 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 _, 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) + } + } + 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. +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 + said string + }{ + {name: "gone", linked: true, said: ": stat {store}: 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"}, + } { + 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 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" + + strings.ReplaceAll(testCase.said, "{store}", store) + if err == nil || err.Error() != want { + t.Fatalf("touch 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) + } + }) + } +} diff --git a/internal/seniordev/app/pipeline.go b/internal/seniordev/app/pipeline.go index e893be64ea..3aec203ba5 100644 --- a/internal/seniordev/app/pipeline.go +++ b/internal/seniordev/app/pipeline.go @@ -132,7 +132,7 @@ 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) + runtime := newConfiguredRuntime(workspace, args.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..37bfe43a09 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 — in place of the folder's own .senior-dev. Empty reads + // SENIOR_DEV_STATE_DIR, and with that empty too the store stays in the + // folder. 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,11 @@ func runWith(ctx context.Context, host delegate.Host, options Options, notes io. args.MaxHours = &ceilings.Hours } workspace := host.Workspace() + stateDir, refusal := stateDirectory(options.StateDir, workspace) + if refusal != "" { + return refused(refusal) + } + args.StateDir = stateDir 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/state_dir.go b/internal/seniordev/app/state_dir.go new file mode 100644 index 0000000000..3b1ae5766c --- /dev/null +++ b/internal/seniordev/app/state_dir.go @@ -0,0 +1,50 @@ +//go:build !windows + +package app + +import ( + "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" + +// stateDirectory is where this run keeps its session store — its database, +// its conversation and the lock that orders them: the --state-dir it was +// given, else SENIOR_DEV_STATE_DIR, else "" for the workspace's own +// .senior-dev ([openDurableSessionsIn]). The directory is made here, so one +// that cannot be is refused before anything is spent, and so is one inside +// the workspace: 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. +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 "", "" + } + if err := os.MkdirAll(dir, 0o755); err != nil { + return "", said + " " + dir + " cannot be made: " + err.Error() + } + store, folder := realDirectory(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" + } + return store, "" +} + +// within says whether path is dir or somewhere under it; both are clean and +// absolute. +func within(path, dir string) bool { + rel, err := filepath.Rel(dir, path) + return err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} 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..2c96a54d20 100644 --- a/internal/seniordev/seniordev.go +++ b/internal/seniordev/seniordev.go @@ -94,7 +94,8 @@ var Program = delegate.Delegate{ 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). + // conversation (app's seniorDevDataDirectory, which git never sees). A run + // given --state-dir keeps the database and conversation there instead. 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 +191,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 +205,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..74f79b952c 100644 --- a/internal/seniordev/seniordev_test.go +++ b/internal/seniordev/seniordev_test.go @@ -201,6 +201,7 @@ 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("SENIOR_DEV_DISABLE_MODELS_FETCH", "1") catalog, err := filepath.Abs(filepath.Join("modelsdev", "testdata", "catalog.json")) if err != nil { @@ -406,6 +407,49 @@ 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 := filepath.Join(t.TempDir(), "run-1"), t.TempDir() + 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}, + } + + 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)) + } + 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) + } +} + // 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. From 29f1be2a7909fca32f3fd012871035fc2b054d39 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Fri, 9 Oct 2026 12:03:18 -0400 Subject: [PATCH 2/4] senior-dev: hold the store's directory open, and judge --state-dir before making it Review of #1800. The store's identity check compared a Stat taken when it opened with one taken now, and ext4 and overlayfs give a directory made where one was just removed the old inode number back. It held only because sqlite's pooled connection kept senior-dev.db open; with the database closed, the check was fooled 3 out of 3 times in an alpine container and the next write went into the new directory. The store now keeps its directory open and compares through that handle. --state-dir made the directory before judging it, so a refused <folder>/state or <folder>/a/b/c was left in the tree. It is now judged from the deepest part that exists, and made only once allowed; a folder spelled with other case on APFS is refused as the folder. The "gone" ending no longer prints the path twice, and the four flag bullets reworded in the manual are restored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- internal/manual/chat/senior-dev.md | 15 ++-- internal/seniordev/app/durable_sessions.go | 42 ++++++++--- .../seniordev/app/durable_state_dir_test.go | 54 +++++++++++++-- internal/seniordev/app/state_dir.go | 69 +++++++++++++++---- 4 files changed, 148 insertions(+), 32 deletions(-) diff --git a/internal/manual/chat/senior-dev.md b/internal/manual/chat/senior-dev.md index 0f722a0c36..7c9865de63 100644 --- a/internal/manual/chat/senior-dev.md +++ b/internal/manual/chat/senior-dev.md @@ -1159,20 +1159,21 @@ program it carries four flags: senior-dev's own flags on `run`: - `--variant NAME` — how hard the coder thinks: `low`, `medium`, `high`, `xhigh`, `max`, - or `none` for the model's own default; `high` when unset; + or `none` for the model's own default; `high` when unset. Its history summaries send + none; - `--in-place` — work without git even inside a repository: no commits, and its - checkpoints kept outside the folder. A folder with no git history needs no flag; - codeaf passes it itself under a repository at your home folder; + checkpoints kept outside the folder. A folder with no git history is worked that way + without it; codeaf passes it itself under a repository at your home folder; - `--high`, `--low` — comma-separated models it routes among; `--low` (its history summaries) falls back to `--high`. On a shell run, each `--high` entry accepts a bare OpenRouter id, service-prefixed id, or short `/crew` model word; - `--asked` — the `--high` models were chosen by name, so one senior-dev cannot size ends the run before its first call rather than being skipped; - `--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; -- `--state-dir DIR` — keep its session database and conversation in DIR, outside the - folder (`SENIOR_DEV_STATE_DIR` when unset); a new one for each run; -- `--frontier` — accepted; no call uses that tier; + 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, outside the 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/seniordev/app/durable_sessions.go b/internal/seniordev/app/durable_sessions.go index 0655de93c7..a52b3a0c50 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" @@ -66,10 +67,11 @@ type durableSessions struct { unsubscribe func() projector *projectors.Store lockPath string - // storeDir is the store's real path, and storeInfo the directory found - // there when it was opened ([durableSessions.storeStillThere]). - storeDir string - storeInfo os.FileInfo + // storeDir is the store's real path, and storeHandle the directory found + // there when it was opened, held open for as long as the store is + // ([durableSessions.storeStillThere]). + storeDir string + storeHandle *os.File operationMu sync.Mutex projectionMu sync.Mutex @@ -105,23 +107,26 @@ func openDurableSessionsIn(ctx context.Context, workspace, stateDir string) (*du return nil, fmt.Errorf("senior-dev sessions: create data directory: %w", err) } dataDir = realDirectory(dataDir) - storeInfo, err := os.Stat(dataDir) + storeHandle, err := os.Open(dataDir) if err != nil { return nil, fmt.Errorf("senior-dev sessions: read data directory: %w", err) } projectInfo, _, err := project.Discover(ctx, workspace) if err != nil { + _ = storeHandle.Close() return nil, fmt.Errorf("senior-dev sessions: discover project: %w", err) } projectID := string(projectInfo.ID) dbPath := filepath.Join(dataDir, seniorDevDatabaseFile) db, err := projectors.Open(ctx, dbPath, projectors.BusyRetryOptions{Log: io.Discard}) if err != nil { + _ = storeHandle.Close() return nil, err } closeOnError := func(err error) (*durableSessions, error) { _ = db.Close() + _ = storeHandle.Close() return nil, err } if err := applyProjectSchema(ctx, db); err != nil { @@ -142,7 +147,7 @@ func openDurableSessionsIn(ctx context.Context, workspace, stateDir string) (*du projectID: projectID, workspace: workspace, projector: projectors.NewStore(db, projectors.StoreOptions{}), lockPath: filepath.Join(dataDir, "projection.lock"), - storeDir: dataDir, storeInfo: storeInfo, + storeDir: dataDir, storeHandle: storeHandle, } durable.projectionSource = durable.store sessions, err := sessioncore.New(sessioncore.Options{ @@ -1049,18 +1054,36 @@ func (durable *durableSessions) withStoreLock(fn func() error) error { // 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 IS 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 directory'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. func (durable *durableSessions) storeStillThere() error { - if durable.storeInfo == nil { + if durable.storeHandle == nil { return nil } + opened, err := durable.storeHandle.Stat() + if err != nil { + return fmt.Errorf("senior-dev sessions: read its store %s: %w", durable.storeDir, err) + } current, err := os.Stat(durable.storeDir) switch { - case err == nil && os.SameFile(durable.storeInfo, current): + case err == nil && os.SameFile(opened, current): return nil case err == nil: return fmt.Errorf("senior-dev sessions: its store %s was removed while the run was working, "+ "with the conversation in it; the directory there now is a new one", durable.storeDir) default: + // 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 + } return fmt.Errorf("senior-dev sessions: its store %s was removed while the run was working, "+ "with the conversation in it: %w", durable.storeDir, err) } @@ -1092,4 +1115,7 @@ func (durable *durableSessions) Close() { if durable.db != nil { _ = durable.db.Close() } + if durable.storeHandle != nil { + _ = durable.storeHandle.Close() + } } diff --git a/internal/seniordev/app/durable_state_dir_test.go b/internal/seniordev/app/durable_state_dir_test.go index 9dccc2b99e..2bb001ee73 100644 --- a/internal/seniordev/app/durable_state_dir_test.go +++ b/internal/seniordev/app/durable_state_dir_test.go @@ -138,7 +138,8 @@ func TestStateDirRootsTheStoreOutsideTheWorkspace(t *testing.T) { // 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. +// 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() @@ -155,7 +156,9 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { 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) @@ -166,6 +169,15 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { 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"), @@ -175,6 +187,23 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { 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) @@ -191,6 +220,14 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { // 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 { @@ -200,11 +237,15 @@ func TestARemovedStoreEndsTheRunWithoutAFreshOne(t *testing.T) { linked bool // remade is what is made where the store was, after it is removed. remade string - said string + // alone closes the database before the store is removed. + alone bool + said string }{ - {name: "gone", linked: true, said: ": stat {store}: no such file or directory"}, + {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"}, } { t.Run(testCase.name, func(t *testing.T) { workspace := t.TempDir() @@ -225,6 +266,11 @@ func TestARemovedStoreEndsTheRunWithoutAFreshOne(t *testing.T) { t.Fatal(err) } store := durable.storeDir + if testCase.alone { + if err := durable.db.Close(); err != nil { + t.Fatal(err) + } + } if err := os.RemoveAll(store); err != nil { t.Fatal(err) } @@ -236,7 +282,7 @@ func TestARemovedStoreEndsTheRunWithoutAFreshOne(t *testing.T) { 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" + - strings.ReplaceAll(testCase.said, "{store}", store) + testCase.said if err == nil || err.Error() != want { t.Fatalf("touch after the store was removed = %v, want %q", err, want) } diff --git a/internal/seniordev/app/state_dir.go b/internal/seniordev/app/state_dir.go index 3b1ae5766c..3b49580d54 100644 --- a/internal/seniordev/app/state_dir.go +++ b/internal/seniordev/app/state_dir.go @@ -17,11 +17,17 @@ const StateDirEnv = "SENIOR_DEV_STATE_DIR" // stateDirectory is where this run keeps its session store — its database, // its conversation and the lock that orders them: the --state-dir it was // given, else SENIOR_DEV_STATE_DIR, else "" for the workspace's own -// .senior-dev ([openDurableSessionsIn]). The directory is made here, so one -// that cannot be is refused before anything is spent, and so is one inside -// the workspace: 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. +// .senior-dev ([openDurableSessionsIn]). One 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 <folder>/state, or every level of <folder>/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 == "" { @@ -31,20 +37,57 @@ func stateDirectory(flagged, workspace string) (dir, refusal string) { if dir == "" { return "", "" } - if err := os.MkdirAll(dir, 0o755); err != nil { - return "", said + " " + dir + " cannot be made: " + err.Error() - } - store, folder := realDirectory(dir), realDirectory(workspace) + 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" } - return store, "" + if err := os.MkdirAll(store, 0o755); err != nil { + return "", said + " " + dir + " cannot be made: " + err.Error() + } + return realDirectory(store), "" +} + +// 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. +// 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 { - rel, err := filepath.Rel(dir, path) - return err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) + 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 + } + } } From ba163d3d3461d7cf7cdd0f9b13cd96057691b623 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Fri, 9 Oct 2026 13:38:31 -0400 Subject: [PATCH 3/4] senior-dev: keep the store in the run's record folder, and write back a removed .senior-dev Both CyberGym runs lost .senior-dev to the model's own last command: OpenSSL 1.1.0's make clean deletes every link in the tree, and the rig's validate.py begins sudo rm -rf /src. The store is now kept out of the folder by default. - With no --state-dir and no SENIOR_DEV_STATE_DIR, a run codeaf carries keeps its store in a directory of its own in the record folder codeaf names on CODEAF_RECORDS: <record>/store, or store.1, store.2 for a later run of the same task. Only with no record folder does it stay in .senior-dev. - The brief, the checklist, the pinned command and the steering file are read again after every tool call and by senior-dev's own reads; one that is gone is written back as the run last read it, with an implement/notes-rewritten record and a line on the run's page. - The removed-store check now also holds the database and the records' directory open: an rm -rf that raced the store's own writes left the directory standing with its contents gone, and the directory alone still matched. Conversation reads are checked too, and a write cut short by the removal names the store instead of the temporary file it was renaming. Manual, the Notes comment and the change entry say where the store is now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .../unreleased/1800-senior-dev-state-dir.md | 36 +++-- internal/manual/chat/senior-dev.md | 79 +++++----- internal/manual/chat_test.go | 6 +- internal/seniordev/actions.go | 8 + internal/seniordev/app/args.go | 5 +- internal/seniordev/app/durable_sessions.go | 131 +++++++++------ .../seniordev/app/durable_state_dir_test.go | 115 +++++++++++++- internal/seniordev/app/notes_kept.go | 77 +++++++++ internal/seniordev/app/pipeline.go | 4 + internal/seniordev/app/run.go | 14 +- internal/seniordev/app/solo.go | 20 ++- internal/seniordev/app/state_dir.go | 56 ++++++- internal/seniordev/app/steering.go | 6 +- internal/seniordev/seniordev.go | 11 +- internal/seniordev/seniordev_test.go | 149 +++++++++++++++++- 15 files changed, 587 insertions(+), 130 deletions(-) create mode 100644 internal/seniordev/app/notes_kept.go diff --git a/docs/changes/unreleased/1800-senior-dev-state-dir.md b/docs/changes/unreleased/1800-senior-dev-state-dir.md index e73a108b2e..167b0186bc 100644 --- a/docs/changes/unreleased/1800-senior-dev-state-dir.md +++ b/docs/changes/unreleased/1800-senior-dev-state-dir.md @@ -1,25 +1,29 @@ --- kind: fixed -title: senior-dev opens its store by its real path, and takes --state-dir to keep it outside the folder +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 <folder>/.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." - - "senior-dev's session database and conversation always lived in the folder's `.senior-dev/` and were moved into the task's record when the run ended. `codeaf senior-dev run --state-dir DIR`, or `SENIOR_DEV_STATE_DIR`, keeps them in DIR instead, where they stay; a DIR inside the folder, other than under its own `.senior-dev/`, is refused before the run starts." + - "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, `<record>/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." --- -A CyberGym run on openssl had anchored `/src/openssl/.senior-dev` to -`/home/agent/senior-dev-store` through a link. Sixty-four minutes in the link -stopped resolving, and because the projection lock was reopened through it for -every write, one failed `open()` ended the run unsubmitted after $0.64; the store -was collected intact afterwards. The same ending appeared earlier on arduinojson. +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 still ends the run, now saying `its store <dir> -was removed while the run was working, with the conversation in it`, and it -does so even when something has made the directory again in the meantime — in -the folder's own `.senior-dev/`, the next tool output written there does. The -store remembers which directory it opened and writes into no other: in a fresh -one 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. Runs -in folders without git all share the project `global`, so a rig should give -each run a state directory of its own. +A store that is itself removed or emptied still ends the run, now saying `its +store <dir> 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 read and write: 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 7c9865de63..641a543b4c 100644 --- a/internal/manual/chat/senior-dev.md +++ b/internal/manual/chat/senior-dev.md @@ -615,47 +615,50 @@ 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 <path>`. 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. - -A run given `--state-dir DIR` (or `SENIOR_DEV_STATE_DIR`) keeps its session database and -its conversation in that directory instead, and they stay there: only what is in -`.senior-dev/` — the brief, the checklist, the pinned command, the output it set aside — -is moved into the record. +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 <path>`. 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 keeps its session store — `senior-dev.db`, the `storage/` folder of its records -and `projection.lock` — in `.senior-dev/` in the folder it works in, unless it is given a -directory of its own. **It opens that store once, by its real path**: a `.senior-dev` that -is a link to somewhere else is followed when the run starts, and every later read and -write goes where the link led. So a link that goes away while it works no longer ends the -run. It used to: the lock was reopened through the link for every write, and the next -model turn ended the run with `open <folder>/.senior-dev/projection.lock: no such file or -directory` while the store itself was whole. - -**A store that is itself removed still ends the run**, because its conversation went with -it. senior-dev does not carry on in an empty directory made in its place, with a model -that has lost its work; it stops, saying +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 <folder>/.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 <dir> was removed while the run was working, with the conversation in it`. -Whatever clears or restores the folder — a benchmark's reset script, `rm -rf` of the -tree — removes a store kept in it. - -To keep the store out of the folder, start `codeaf senior-dev run` with `--state-dir DIR`, -or set `SENIOR_DEV_STATE_DIR`; the flag wins when both are set, and the directory is made -if it is missing. **Use a new one for each run**: runs in folders without git share one -project, so a directory shared between them mixes their sessions. A directory inside the -folder is refused before anything is spent, because its checkpoints and the change it -hands in would carry the store; the folder's own `.senior-dev/` is the exception. The -brief, the checklist and the other files the model reads stay in `.senior-dev/` in the -folder either way. + +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 @@ -1172,7 +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, outside the folder (`SENIOR_DEV_STATE_DIR`); +- `--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 5a8b6c8f3e..bcab37f2b3 100644 --- a/internal/manual/chat_test.go +++ b/internal/manual/chat_test.go @@ -3430,7 +3430,8 @@ func TestTheUndiscoveredBuildQuestionsReachTheVerifyFlags(t *testing.T) { // 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. +// 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 @@ -3441,6 +3442,9 @@ func TestTheVanishedStoreQuestionsReachTheStateDir(t *testing.T) { {"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) { 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/app/args.go b/internal/seniordev/app/args.go index f17b8213fb..83d39339be 100644 --- a/internal/seniordev/app/args.go +++ b/internal/seniordev/app/args.go @@ -70,8 +70,9 @@ type cliArgs struct { // leaves that kind to discovery (fullverification/declared.go). VerifyBuild string VerifyTest string - // StateDir is where the session store lives, resolved through its links; - // empty keeps it in the workspace's own .senior-dev ([stateDirectory]). + // StateDir is where the session store lives, resolved through its links: + // the flag's, the variable's or the run's record folder's; empty keeps it + // in the workspace's own .senior-dev ([stateDirectory]). StateDir string MaxCost *float64 MaxHours *float64 diff --git a/internal/seniordev/app/durable_sessions.go b/internal/seniordev/app/durable_sessions.go index a52b3a0c50..affa3709c1 100644 --- a/internal/seniordev/app/durable_sessions.go +++ b/internal/seniordev/app/durable_sessions.go @@ -32,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 ) @@ -67,11 +69,11 @@ type durableSessions struct { unsubscribe func() projector *projectors.Store lockPath string - // storeDir is the store's real path, and storeHandle the directory found - // there when it was opened, held open for as long as the store is - // ([durableSessions.storeStillThere]). - storeDir string - storeHandle *os.File + // 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 @@ -107,28 +109,33 @@ func openDurableSessionsIn(ctx context.Context, workspace, stateDir string) (*du return nil, fmt.Errorf("senior-dev sessions: create data directory: %w", err) } dataDir = realDirectory(dataDir) - storeHandle, err := os.Open(dataDir) - if err != nil { - return nil, fmt.Errorf("senior-dev sessions: read data directory: %w", err) - } projectInfo, _, err := project.Discover(ctx, workspace) if err != nil { - _ = storeHandle.Close() return nil, fmt.Errorf("senior-dev sessions: discover project: %w", err) } projectID := string(projectInfo.ID) dbPath := filepath.Join(dataDir, seniorDevDatabaseFile) db, err := projectors.Open(ctx, dbPath, projectors.BusyRetryOptions{Log: io.Discard}) if err != nil { - _ = storeHandle.Close() return nil, err } + var storeHeld []*os.File closeOnError := func(err error) (*durableSessions, error) { _ = db.Close() - _ = storeHandle.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) } @@ -147,7 +154,7 @@ func openDurableSessionsIn(ctx context.Context, workspace, stateDir string) (*du projectID: projectID, workspace: workspace, projector: projectors.NewStore(db, projectors.StoreOptions{}), lockPath: filepath.Join(dataDir, "projection.lock"), - storeDir: dataDir, storeHandle: storeHandle, + storeDir: dataDir, storeHeld: storeHeld, } durable.projectionSource = durable.store sessions, err := sessioncore.New(sessioncore.Options{ @@ -294,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) } @@ -1038,7 +1052,15 @@ func (durable *durableSessions) withStoreLock(fn func() error) error { if err := durable.storeStillThere(); err != nil { return err } - return withAdvisoryFileLock(durable.lockPath, fn) + 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 @@ -1055,37 +1077,58 @@ func (durable *durableSessions) withStoreLock(fn func() error) error { // that starts where the removal happened. The run ends instead, naming the // store where it was. // -// THE STORE'S DIRECTORY IS 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 directory'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. +// 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 { - if durable.storeHandle == nil { - return nil - } - opened, err := durable.storeHandle.Stat() - if err != nil { - return fmt.Errorf("senior-dev sessions: read its store %s: %w", durable.storeDir, err) - } - current, err := os.Stat(durable.storeDir) - switch { - case err == nil && os.SameFile(opened, current): - return nil - case err == nil: - return fmt.Errorf("senior-dev sessions: its store %s was removed while the run was working, "+ - "with the conversation in it; the directory there now is a new one", durable.storeDir) - default: - // 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 + 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: %w", durable.storeDir, err) + "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() } } @@ -1115,7 +1158,5 @@ func (durable *durableSessions) Close() { if durable.db != nil { _ = durable.db.Close() } - if durable.storeHandle != nil { - _ = durable.storeHandle.Close() - } + closeAll(durable.storeHeld) } diff --git a/internal/seniordev/app/durable_state_dir_test.go b/internal/seniordev/app/durable_state_dir_test.go index 2bb001ee73..40bb70b3a8 100644 --- a/internal/seniordev/app/durable_state_dir_test.go +++ b/internal/seniordev/app/durable_state_dir_test.go @@ -94,7 +94,7 @@ func TestStateDirRootsTheStoreOutsideTheWorkspace(t *testing.T) { case testCase.variable: t.Setenv(StateDirEnv, stateDir) } - dir, refusal := stateDirectory(flagged, workspace) + dir, refusal := stateDirectory(flagged, workspace, "") if refusal != "" { t.Fatalf("refused: %s", refusal) } @@ -160,12 +160,12 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { 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") { + 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+" ") { + 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, "") @@ -183,7 +183,7 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { filepath.Join(workspace, seniorDevDataDirectory, "store"), filepath.Join(workspace, "out-of-the-folder", "store"), } { - if _, refusal := stateDirectory(kept, workspace); refusal != "" { + if _, refusal := stateDirectory(kept, workspace, ""); refusal != "" { t.Errorf("--state-dir %s was refused: %s", kept, refusal) } } @@ -196,7 +196,7 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { 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") { + 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) { @@ -208,7 +208,7 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { 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") { + 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) } } @@ -239,13 +239,17 @@ func TestARemovedStoreEndsTheRunWithoutAFreshOne(t *testing.T) { remade string // alone closes the database before the store is removed. alone bool - said string + // 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() @@ -271,7 +275,14 @@ func TestARemovedStoreEndsTheRunWithoutAFreshOne(t *testing.T) { t.Fatal(err) } } - if err := os.RemoveAll(store); err != nil { + 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 != "" { @@ -286,6 +297,9 @@ func TestARemovedStoreEndsTheRunWithoutAFreshOne(t *testing.T) { 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 { @@ -300,3 +314,88 @@ func TestARemovedStoreEndsTheRunWithoutAFreshOne(t *testing.T) { }) } } + +// 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. Each launch gets a directory of its own there +// — a task's folder outlives a run — the flag and the variable still win, 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") + t.Setenv(StateDirEnv, "") + + first, refusal := stateDirectory("", workspace, records) + if refusal != "" || first != realDirectory(filepath.Join(records, "store")) { + t.Fatalf("store = %q (%q), want %s/store", first, refusal, records) + } + second, _ := stateDirectory("", workspace, records) + if second != realDirectory(filepath.Join(records, "store.1")) { + t.Fatalf("a second launch's store = %q, want %s/store.1 beside the first", second, records) + } + + flagged := filepath.Join(t.TempDir(), "flagged") + if dir, _ := stateDirectory(flagged, workspace, records); dir != realDirectory(flagged) { + t.Errorf("with --state-dir the store = %q, want %s", dir, flagged) + } + variable := t.TempDir() + t.Setenv(StateDirEnv, variable) + if dir, _ := stateDirectory("", workspace, records); dir != realDirectory(variable) { + t.Errorf("with %s the store = %q, want %s", StateDirEnv, dir, variable) + } + t.Setenv(StateDirEnv, "") + if _, err := os.Lstat(filepath.Join(records, "store.2")); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("the flag or the variable still made a store in the record folder (%v)", err) + } + + if dir, _ := stateDirectory("", workspace, ""); dir != "" { + t.Errorf("with no record folder the store = %q, want the folder's own .senior-dev", dir) + } + inside := filepath.Join(workspace, ".codeaf", "v3", "carried", "senior-dev", "run") + if dir, _ := stateDirectory("", workspace, inside); dir != "" { + t.Errorf("with the record folder inside the workspace the store = %q, want the folder's own .senior-dev", dir) + } + 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) + } +} diff --git a/internal/seniordev/app/notes_kept.go b/internal/seniordev/app/notes_kept.go new file mode 100644 index 0000000000..7797a8f82e --- /dev/null +++ b/internal/seniordev/app/notes_kept.go @@ -0,0 +1,77 @@ +//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. +func (runner *pipeline) readNote(name string) ([]byte, error) { + path := filepath.Join(runner.workspace, filepath.FromSlash(name)) + data, err := os.ReadFile(path) + runner.kept.mu.Lock() + if err == nil { + if runner.kept.files == nil { + runner.kept.files = map[string][]byte{} + } + runner.kept.files[name] = data + runner.kept.mu.Unlock() + return data, nil + } + last, known := runner.kept.files[name] + runner.kept.mu.Unlock() + if !known || !errors.Is(err, fs.ErrNotExist) { + return nil, err + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return nil, err + } + if err := os.WriteFile(path, last, 0o644); err != nil { + return nil, err + } + runner.events.stage("implement", notesRewrittenStatus, map[string]any{"file": name, "bytes": len(last)}) + return last, 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 3aec203ba5..176f4e4b7c 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. diff --git a/internal/seniordev/app/run.go b/internal/seniordev/app/run.go index 37bfe43a09..f100f83631 100644 --- a/internal/seniordev/app/run.go +++ b/internal/seniordev/app/run.go @@ -81,10 +81,10 @@ type Options struct { 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 — in place of the folder's own .senior-dev. Empty reads - // SENIOR_DEV_STATE_DIR, and with that empty too the store stays in the - // folder. The files the model works with stay in the folder's .senior-dev - // either way ([stateDirectory]). + // 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 @@ -165,7 +165,11 @@ func runWith(ctx context.Context, host delegate.Host, options Options, notes io. args.MaxHours = &ceilings.Hours } workspace := host.Workspace() - stateDir, refusal := stateDirectory(options.StateDir, workspace) + records := "" + if recorder, ok := host.(delegate.Recorder); ok { + records = recorder.Records() + } + stateDir, refusal := stateDirectory(options.StateDir, workspace, records) if refusal != "" { return refused(refusal) } 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 index 3b49580d54..73ca0d8b50 100644 --- a/internal/seniordev/app/state_dir.go +++ b/internal/seniordev/app/state_dir.go @@ -3,6 +3,9 @@ package app import ( + "errors" + "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -14,10 +17,16 @@ import ( // `--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 ([recordStore]). +const recordStoreName = "store" + // stateDirectory is where this run keeps its session store — its database, // its conversation and the lock that orders them: the --state-dir it was -// given, else SENIOR_DEV_STATE_DIR, else "" for the workspace's own -// .senior-dev ([openDurableSessionsIn]). One inside the workspace is refused +// given, else SENIOR_DEV_STATE_DIR, else a directory of its own in records, +// the run's record folder codeaf named ([recordStore]), else "" for the +// workspace's own .senior-dev ([openDurableSessionsIn]). A directory named on +// the command line or in the variable 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 @@ -28,14 +37,14 @@ const StateDirEnv = "SENIOR_DEV_STATE_DIR" // part of it that already exists ([resolvedPath]); making it first would leave // a refused <folder>/state, or every level of <folder>/a/b/c, in the tree the // refusal was protecting, for the next checkpoint to carry. -func stateDirectory(flagged, workspace string) (dir, refusal string) { +func stateDirectory(flagged, workspace, records 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 "", "" + return recordStore(records, workspace), "" } store, folder := resolvedPath(dir), realDirectory(workspace) if within(store, folder) && !within(store, filepath.Join(folder, seniorDevDataDirectory)) { @@ -48,6 +57,45 @@ func stateDirectory(flagged, workspace string) (dir, refusal string) { return realDirectory(store), "" } +// recordStore is a directory of this launch's own in records, the run's record +// folder: store, or store.1, store.2 when an earlier run of the same task left +// one there, made here so two launches can never be handed the same one. It +// is "" — the workspace's own .senior-dev — when codeaf named no record folder +// (senior-dev run directly, a test), when that folder cannot be written, or +// when it is inside the workspace, as it is for a run in the home 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 recordStore(records, workspace string) string { + if strings.TrimSpace(records) == "" { + return "" + } + base := resolvedPath(filepath.Join(records, recordStoreName)) + if within(base, realDirectory(workspace)) { + 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 diff --git a/internal/seniordev/app/steering.go b/internal/seniordev/app/steering.go index 3ecb4ec4b0..6d376881f2 100644 --- a/internal/seniordev/app/steering.go +++ b/internal/seniordev/app/steering.go @@ -96,8 +96,12 @@ 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 +// ([pipeline.readNote]). func (runner *pipeline) keepSteering(messages []delegate.Message) { + _, _ = runner.readNote(steeringFile) + defer func() { _, _ = runner.readNote(steeringFile) }() path := filepath.Join(runner.workspace, steeringFile) if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return diff --git a/internal/seniordev/seniordev.go b/internal/seniordev/seniordev.go index 2c96a54d20..3fea3fd38a 100644 --- a/internal/seniordev/seniordev.go +++ b/internal/seniordev/seniordev.go @@ -92,10 +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). A run - // given --state-dir keeps the database and conversation there instead. + // 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). diff --git a/internal/seniordev/seniordev_test.go b/internal/seniordev/seniordev_test.go index 74f79b952c..2e852ff97b 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 { @@ -202,6 +213,7 @@ func hermeticRun(t *testing.T) (workspace string, trap *atomic.Bool) { 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 { @@ -413,8 +425,9 @@ func TestTheRunCommandWorksATaskThroughTheModelAPIItIsGiven(t *testing.T) { // 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 := filepath.Join(t.TempDir(), "run-1"), t.TempDir() + 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{ @@ -440,6 +453,9 @@ func TestTheStateDirFlagKeepsTheStoreWhereItSays(t *testing.T) { 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) @@ -450,6 +466,135 @@ func TestTheStateDirFlagKeepsTheStoreWhereItSays(t *testing.T) { } } +// 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 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. From 53310c0bc22bca438cdb7e4e18f42082c22fbaa3 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:03:38 -0400 Subject: [PATCH 4/4] senior-dev: a write-back never overwrites, and the record folder's store is made when it opens Review of #1800. Tool calls run at once (steploop's processor runs each in its own goroutine but the question), so a write-back could land after the model's own write of its checklist and put the old copy over it. readNote now holds the kept notes' lock throughout, and the copy is written to a file of its own and linked into place, which fails where anything already is; the file then read is the one kept. keepSteering appends under the same lock. Against the old write-back the new test fails in its first round. The record folder's store was made before the config, --asked and window refusals, so a refused launch left an empty store and the next got store.1. Where it goes is still decided early (recordStoreBase); the directory is made in newPipeline when the store opens (claimRecordStore). Also: the notes section's heading says where the database is now, the change entry says the store is checked before every write and every read of the conversation, and the page's notes-rewritten line has a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .../unreleased/1800-senior-dev-state-dir.md | 15 +-- internal/manual/chat/senior-dev.md | 2 +- internal/seniordev/actions_test.go | 15 +++ internal/seniordev/app/args.go | 15 ++- .../seniordev/app/durable_state_dir_test.go | 104 ++++++++++++------ internal/seniordev/app/notes_kept.go | 62 ++++++++++- internal/seniordev/app/pipeline.go | 8 +- internal/seniordev/app/run.go | 5 +- internal/seniordev/app/state_dir.go | 43 +++++--- internal/seniordev/app/steering.go | 10 +- internal/seniordev/seniordev_test.go | 53 +++++++++ 11 files changed, 259 insertions(+), 73 deletions(-) diff --git a/docs/changes/unreleased/1800-senior-dev-state-dir.md b/docs/changes/unreleased/1800-senior-dev-state-dir.md index 167b0186bc..f515d165db 100644 --- a/docs/changes/unreleased/1800-senior-dev-state-dir.md +++ b/docs/changes/unreleased/1800-senior-dev-state-dir.md @@ -20,10 +20,11 @@ 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 <dir> 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 read and write: 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. +store <dir> 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 641a543b4c..418838e7a5 100644 --- a/internal/manual/chat/senior-dev.md +++ b/internal/manual/chat/senior-dev.md @@ -613,7 +613,7 @@ 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 +## 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, 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 83d39339be..8d19692cef 100644 --- a/internal/seniordev/app/args.go +++ b/internal/seniordev/app/args.go @@ -70,12 +70,15 @@ type cliArgs struct { // leaves that kind to discovery (fullverification/declared.go). VerifyBuild string VerifyTest string - // StateDir is where the session store lives, resolved through its links: - // the flag's, the variable's or the run's record folder's; empty keeps it - // in the workspace's own .senior-dev ([stateDirectory]). - StateDir string - MaxCost *float64 - MaxHours *float64 + // 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 } // CrewModel is a crew seat's model as a pool entry: the id filed under the diff --git a/internal/seniordev/app/durable_state_dir_test.go b/internal/seniordev/app/durable_state_dir_test.go index 40bb70b3a8..b3955aeb30 100644 --- a/internal/seniordev/app/durable_state_dir_test.go +++ b/internal/seniordev/app/durable_state_dir_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "github.com/Agent-Field/codeaf/internal/seniordev/session/sessioncore" @@ -94,7 +95,7 @@ func TestStateDirRootsTheStoreOutsideTheWorkspace(t *testing.T) { case testCase.variable: t.Setenv(StateDirEnv, stateDir) } - dir, refusal := stateDirectory(flagged, workspace, "") + dir, refusal := stateDirectory(flagged, workspace) if refusal != "" { t.Fatalf("refused: %s", refusal) } @@ -160,12 +161,12 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { 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") { + 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+" ") { + 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, "") @@ -183,7 +184,7 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { filepath.Join(workspace, seniorDevDataDirectory, "store"), filepath.Join(workspace, "out-of-the-folder", "store"), } { - if _, refusal := stateDirectory(kept, workspace, ""); refusal != "" { + if _, refusal := stateDirectory(kept, workspace); refusal != "" { t.Errorf("--state-dir %s was refused: %s", kept, refusal) } } @@ -196,7 +197,7 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { 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") { + 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) { @@ -208,7 +209,7 @@ func TestAStateDirInsideTheFolderIsRefused(t *testing.T) { 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") { + 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) } } @@ -317,44 +318,35 @@ func TestARemovedStoreEndsTheRunWithoutAFreshOne(t *testing.T) { // 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. Each launch gets a directory of its own there -// — a task's folder outlives a run — the flag and the variable still win, 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. +// 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") - t.Setenv(StateDirEnv, "") - first, refusal := stateDirectory("", workspace, records) - if refusal != "" || first != realDirectory(filepath.Join(records, "store")) { - t.Fatalf("store = %q (%q), want %s/store", first, refusal, records) - } - second, _ := stateDirectory("", workspace, records) - if second != realDirectory(filepath.Join(records, "store.1")) { - t.Fatalf("a second launch's store = %q, want %s/store.1 beside the first", second, records) + base := recordStoreBase(records, workspace) + if base != resolvedPath(filepath.Join(records, "store")) { + t.Fatalf("the record folder's store = %q, want %s/store", base, records) } - - flagged := filepath.Join(t.TempDir(), "flagged") - if dir, _ := stateDirectory(flagged, workspace, records); dir != realDirectory(flagged) { - t.Errorf("with --state-dir the store = %q, want %s", dir, flagged) + 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) } - variable := t.TempDir() - t.Setenv(StateDirEnv, variable) - if dir, _ := stateDirectory("", workspace, records); dir != realDirectory(variable) { - t.Errorf("with %s the store = %q, want %s", StateDirEnv, dir, variable) + if first := claimRecordStore(base); first != realDirectory(filepath.Join(records, "store")) { + t.Fatalf("the first launch's store = %q, want %s/store", first, records) } - t.Setenv(StateDirEnv, "") - if _, err := os.Lstat(filepath.Join(records, "store.2")); !errors.Is(err, fs.ErrNotExist) { - t.Errorf("the flag or the variable still made a store in the record folder (%v)", err) + 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 dir, _ := stateDirectory("", workspace, ""); dir != "" { - t.Errorf("with no record folder the store = %q, want the folder's own .senior-dev", dir) + 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 dir, _ := stateDirectory("", workspace, inside); dir != "" { - t.Errorf("with the record folder inside the workspace the store = %q, want the folder's own .senior-dev", dir) + 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) @@ -399,3 +391,49 @@ func TestARemovedNoteIsWrittenBackFromWhatTheRunLastRead(t *testing.T) { 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 index 7797a8f82e..7cd7eb6d98 100644 --- a/internal/seniordev/app/notes_kept.go +++ b/internal/seniordev/app/notes_kept.go @@ -35,33 +35,83 @@ type keptNotes struct { // 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) - runner.kept.mu.Lock() if err == nil { if runner.kept.files == nil { runner.kept.files = map[string][]byte{} } runner.kept.files[name] = data - runner.kept.mu.Unlock() return data, nil } last, known := runner.kept.files[name] - runner.kept.mu.Unlock() if !known || !errors.Is(err, fs.ErrNotExist) { return nil, err } - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + rewrote, err := writeBackNote(path, last) + if err != nil { return nil, err } - if err := os.WriteFile(path, last, 0o644); 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" diff --git a/internal/seniordev/app/pipeline.go b/internal/seniordev/app/pipeline.go index 176f4e4b7c..7c5af8ed64 100644 --- a/internal/seniordev/app/pipeline.go +++ b/internal/seniordev/app/pipeline.go @@ -136,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, args.StateDir, 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 f100f83631..0a1d23fce8 100644 --- a/internal/seniordev/app/run.go +++ b/internal/seniordev/app/run.go @@ -169,11 +169,14 @@ func runWith(ctx context.Context, host delegate.Host, options Options, notes io. if recorder, ok := host.(delegate.Recorder); ok { records = recorder.Records() } - stateDir, refusal := stateDirectory(options.StateDir, workspace, 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/state_dir.go b/internal/seniordev/app/state_dir.go index 73ca0d8b50..f2823649d0 100644 --- a/internal/seniordev/app/state_dir.go +++ b/internal/seniordev/app/state_dir.go @@ -18,15 +18,16 @@ import ( 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 ([recordStore]). +// 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: the --state-dir it was -// given, else SENIOR_DEV_STATE_DIR, else a directory of its own in records, -// the run's record folder codeaf named ([recordStore]), else "" for the -// workspace's own .senior-dev ([openDurableSessionsIn]). A directory named on -// the command line or in the variable that is inside the workspace is refused +// 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 @@ -37,14 +38,14 @@ const recordStoreName = "store" // part of it that already exists ([resolvedPath]); making it first would leave // a refused <folder>/state, or every level of <folder>/a/b/c, in the tree the // refusal was protecting, for the next checkpoint to carry. -func stateDirectory(flagged, workspace, records string) (dir, refusal string) { +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 recordStore(records, workspace), "" + return "", "" } store, folder := resolvedPath(dir), realDirectory(workspace) if within(store, folder) && !within(store, filepath.Join(folder, seniorDevDataDirectory)) { @@ -57,12 +58,12 @@ func stateDirectory(flagged, workspace, records string) (dir, refusal string) { return realDirectory(store), "" } -// recordStore is a directory of this launch's own in records, the run's record -// folder: store, or store.1, store.2 when an earlier run of the same task left -// one there, made here so two launches can never be handed the same one. It -// is "" — the workspace's own .senior-dev — when codeaf named no record folder -// (senior-dev run directly, a test), when that folder cannot be written, or -// when it is inside the workspace, as it is for a run in the home folder. +// 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 @@ -70,7 +71,7 @@ func stateDirectory(flagged, workspace, records string) (dir, refusal string) { // .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 recordStore(records, workspace string) string { +func recordStoreBase(records, workspace string) string { if strings.TrimSpace(records) == "" { return "" } @@ -78,6 +79,18 @@ func recordStore(records, workspace string) string { 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 "" } diff --git a/internal/seniordev/app/steering.go b/internal/seniordev/app/steering.go index 6d376881f2..ad6e4f5b29 100644 --- a/internal/seniordev/app/steering.go +++ b/internal/seniordev/app/steering.go @@ -97,11 +97,15 @@ 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. A steering file something removed is written -// back before it is appended to, so the messages taken before are not lost +// 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.readNote(steeringFile) - defer func() { _, _ = runner.readNote(steeringFile) }() + 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/seniordev_test.go b/internal/seniordev/seniordev_test.go index 2e852ff97b..a0922cfa31 100644 --- a/internal/seniordev/seniordev_test.go +++ b/internal/seniordev/seniordev_test.go @@ -498,6 +498,59 @@ func TestACarriedRunKeepsItsStoreInTheRecordFolder(t *testing.T) { 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) {