diff --git a/cmd/codeaf/carried.go b/cmd/codeaf/carried.go index 5915b6e12..90ed2e2e4 100644 --- a/cmd/codeaf/carried.go +++ b/cmd/codeaf/carried.go @@ -44,6 +44,7 @@ import ( "github.com/Agent-Field/codeaf/internal/delegate/builtin" "github.com/Agent-Field/codeaf/internal/home" lanes "github.com/Agent-Field/codeaf/internal/lane" + "github.com/Agent-Field/codeaf/internal/modelsource" "github.com/Agent-Field/codeaf/internal/provider" "github.com/Agent-Field/codeaf/internal/provider/modelapi" "github.com/Agent-Field/codeaf/internal/roles" @@ -122,6 +123,7 @@ func carriedSignals() (context.Context, context.CancelFunc) { type carriedRoad struct { completerFor func(model string) modelapi.Completer serves func(model string) bool + resolveModel func(word string) (string, error) modelPrice func(model string) (input, output float64, known bool) seat string defaultSeat func() (string, error) @@ -150,7 +152,14 @@ func profileRoad() (carriedRoad, error) { return carriedRoad{ completerFor: adapters.forModel, serves: func(model string) bool { return session.ServesModel(sources, model) }, - modelPrice: settings.Models.PriceNow, + resolveModel: func(word string) (string, error) { + // A shell has no model picker already warming in the background. + // Wait for the same catalog the chat's task matcher reads before + // deciding what a person's short model word means. + settings.Models.Warmed(context.Background()) + return session.ResolveProgramShellModels(word, v3TaskModels(settings.Models)(), sources) + }, + modelPrice: settings.Models.PriceNow, defaultSeat: func() (string, error) { // A shell run needs the profile's work seat only when nobody pinned // a model for this invocation. A fresh profile can still use --high. @@ -226,8 +235,29 @@ func runCarriedHost(ctx context.Context, inv *delegate.Invocation) error { } road, err := carriedModels() if err != nil { + if word := strings.TrimSpace(inv.ExplicitFlags["high"]); word != "" && errors.Is(err, config.ErrNoAPIKey) { + return session.ProgramShellModelRefusal(word) + } return err } + if word := strings.TrimSpace(inv.ExplicitFlags["high"]); word != "" && road.resolveModel != nil { + resolved, err := road.resolveModel(word) + if err != nil { + return err + } + if inv.Program.Name == "senior-dev" { + // Its pool is addressed through the loopback API's OpenRouter + // dialect, just as the chat road's crewFlags spells every seat. + models := strings.Split(resolved, ",") + for i, model := range models { + if !strings.HasPrefix(model, modelsource.DefaultID+"/") { + models[i] = modelsource.DefaultID + "/" + model + } + } + resolved = strings.Join(models, ",") + } + inv.Line = carriedResolvedHigh(inv.Line, resolved) + } if strings.TrimSpace(inv.ExplicitFlags["high"]) == "" && road.defaultSeat != nil { road.seat, err = road.defaultSeat() if err != nil { @@ -485,6 +515,28 @@ func carriedChildLine(inv *delegate.Invocation) []string { return append(head, line...) } +// carriedResolvedHigh replaces only the model flag's value on the person's +// line. Every other program flag and every word of the brief stays as typed. +func carriedResolvedHigh(line []string, model string) []string { + resolved := append([]string(nil), line...) + for i, word := range resolved { + if word == "--" { + break + } + switch { + case word == "--high" || word == "-high": + if i+1 < len(resolved) { + resolved[i+1] = model + } + case strings.HasPrefix(word, "--high="): + resolved[i] = "--high=" + model + case strings.HasPrefix(word, "-high="): + resolved[i] = "-high=" + model + } + } + return resolved +} + // carriedExit is an ending on the exit ladder: the work stands, a limit you // set stopped it, or it ran and did not finish. func carriedExit(status string) error { diff --git a/cmd/codeaf/carried_fresh_profile_test.go b/cmd/codeaf/carried_fresh_profile_test.go index d802b8cb2..27537a9e4 100644 --- a/cmd/codeaf/carried_fresh_profile_test.go +++ b/cmd/codeaf/carried_fresh_profile_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "os" + "path/filepath" "strings" "sync/atomic" "testing" @@ -19,6 +20,41 @@ import ( // The real shell parser must let a model named by --high start on a fresh // profile with only a provider key. The local server sees actual chat calls. func TestSeniorDevExplicitShellModelRunsOnFreshProfile(t *testing.T) { + testSeniorDevExplicitShellModel(t, shellModelCase{word: "openrouter/fixture/vendor-model"}) +} + +// The shell's real host and child must finish work in a folder without git. +func TestSeniorDevShellWorksInAPlainFolder(t *testing.T) { + testSeniorDevExplicitShellModel(t, shellModelCase{word: "openrouter/fixture/vendor-model", plain: true}) +} + +func TestSeniorDevShellResolvesBareAndCrewModelWords(t *testing.T) { + for _, tc := range []shellModelCase{ + {word: "fixture/vendor-model"}, + {word: "vendor-model"}, + {word: "fixture/vendor-model", asked: true}, + } { + name := tc.word + if tc.asked { + name += "-asked" + } + t.Run(name, func(t *testing.T) { testSeniorDevExplicitShellModel(t, tc) }) + } +} + +func TestSeniorDevShellRefusesAServiceThatCannotServeItsModel(t *testing.T) { + testSeniorDevExplicitShellModel(t, shellModelCase{word: "openrouter/fixture/vendor-model", noKey: true, refused: true}) +} + +type shellModelCase struct { + word string + plain bool + asked bool + refused bool + noKey bool +} + +func testSeniorDevExplicitShellModel(t *testing.T, tc shellModelCase) { if testing.Short() { t.Skip("drives the real senior-dev child") } @@ -27,8 +63,20 @@ func TestSeniorDevExplicitShellModelRunsOnFreshProfile(t *testing.T) { t.Skip("senior-dev is unavailable in this build") } workspace := seniorDevWorkspace(t) + previousCatalog := sharedCatalog + sharedCatalog = newSharedCatalog() + t.Cleanup(func() { sharedCatalog = previousCatalog }) + if tc.plain { + if err := os.RemoveAll(workspace + "/.git"); err != nil { + t.Fatal(err) + } + } t.Setenv("CODEAF_HOME", t.TempDir()) - t.Setenv("OPENROUTER_API_KEY", "sk-or-v1-fixture") + if tc.noKey { + t.Setenv("OPENROUTER_API_KEY", "") + } else { + t.Setenv("OPENROUTER_API_KEY", "sk-or-v1-fixture") + } t.Setenv(carriedChildEnv, "real") t.Setenv("DO_NOT_TRACK", "1") t.Setenv("CODEAF_NO_UPDATE_CHECK", "1") @@ -78,13 +126,33 @@ func TestSeniorDevExplicitShellModelRunsOnFreshProfile(t *testing.T) { output := &lockedBuffer{} carriedStdout = output t.Cleanup(func() { carriedStdout = previous }) - err := runCarried(program, []string{"run", "--high", "openrouter/fixture/vendor-model", "--dir", workspace, "--", "Add", "the", "feature."}) + line := []string{"run", "--high", tc.word} + if tc.asked { + line = append(line, "--asked") + } + line = append(line, "--dir", workspace, "--", "Add", "the", "feature.") + err := runCarried(program, line) + if tc.refused { + if err == nil || calls.Load() != 0 || !strings.Contains(err.Error(), tc.word) || + !strings.Contains(err.Error(), "/crew") || !strings.Contains(err.Error(), "codeaf connect") || output.String() != "" { + t.Fatalf("unserved model error = %v after %d calls, want model and doors before child", err, calls.Load()) + } + if records, _ := filepath.Glob(filepath.Join(carriedRecordRoot("senior-dev"), "*")); len(records) != 0 { + t.Fatalf("the refused shell model started a child record: %q", records) + } + return + } if code := exitCodeOf(err); code != 0 || calls.Load() == 0 { t.Fatalf("fresh explicit shell run exited %d after %d chat calls: %s", code, calls.Load(), output.String()) } if content, err := os.ReadFile(workspace + "/feature.txt"); err != nil || string(content) != "implemented by stub\n" { t.Fatalf("the shell did not make the feature: %q, %v", content, err) } + if tc.plain { + if _, err := os.Stat(workspace + "/.git"); !os.IsNotExist(err) { + t.Fatalf("the plain folder acquired a repository: %v", err) + } + } } // With no explicit model, the shell still asks the profile for a work seat. diff --git a/cmd/codeaf/carried_seniordev_worker_test.go b/cmd/codeaf/carried_seniordev_worker_test.go index 4639ee19c..8c073ce90 100644 --- a/cmd/codeaf/carried_seniordev_worker_test.go +++ b/cmd/codeaf/carried_seniordev_worker_test.go @@ -171,6 +171,14 @@ func TestSeniorDevWorksAPlainFolderAsTheChatsRunWorker(t *testing.T) { if err != nil { t.Fatal(err) } + folder, err := session.PrepareProgramFolder(session.ProgramFolderOrder{ + Program: program, Dir: workspace, Brief: "Add the feature.", + Holder: "the chat's run", Keep: filepath.Join(t.TempDir(), "run"), + }) + if err != nil { + t.Fatal(err) + } + defer folder.Finish("done") model := &seniorDevModel{} worker := runengine.NewDelegateWorker(store, workspace, program, runengine.DelegateSetup{ Exe: self, @@ -178,6 +186,7 @@ func TestSeniorDevWorksAPlainFolderAsTheChatsRunWorker(t *testing.T) { CompleterFor: func(string) session.Completer { return model }, Ledger: filepath.Join(t.TempDir(), "usage.jsonl"), PlainFolder: true, + IgnoredFile: folder.IgnoredFile(), }, 1.0, 0) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) diff --git a/docs/changes/unreleased/1530-senior-dev-plain-folder.md b/docs/changes/unreleased/1530-senior-dev-plain-folder.md new file mode 100644 index 000000000..459dff285 --- /dev/null +++ b/docs/changes/unreleased/1530-senior-dev-plain-folder.md @@ -0,0 +1,9 @@ +--- +kind: fixed +title: senior-dev works in plain folders and resolves shell model names +pr: 1530 +surface: [chat] +invalidates: + - "On dd0fcc654, senior-dev crashed before its first model call in a plain folder because the child was given a missing start-time ignore list. Shell and chat runs now receive a readable empty list there, work in place, and commit nothing." + - "On dd0fcc654, a shell --high value had to include the openrouter service prefix or senior-dev could fail in models.dev. Bare OpenRouter ids, service-prefixed ids, and short crew model words now resolve before the child starts; an unserved model is refused with the /crew and codeaf connect doors." +--- diff --git a/internal/manual/chat/senior-dev.md b/internal/manual/chat/senior-dev.md index 853a02484..48dfb9260 100644 --- a/internal/manual/chat/senior-dev.md +++ b/internal/manual/chat/senior-dev.md @@ -633,7 +633,12 @@ glm-5.1 and minimax-m2.7. A run with no usable crew model uses it. A shell run without `--high` asks the profile for a worker recommendation; if no connected model can fill that seat, it says to widen or pin `/crew` models. An explicit `--high ` runs on a fresh profile with a provider key and no crew rows; -that model is used without resolving a profile seat. +that model is used without resolving a profile seat. It accepts a bare OpenRouter id +(`z-ai/glm-5.3-flash`), a service-prefixed id (`openrouter/z-ai/glm-5.3-flash`), or +a short model word from the same list `/crew` uses (`glm-5.3-flash`). Each word is +resolved before senior-dev starts. A word that is ambiguous, unknown, or cannot be +served by a connected service is refused with `cannot use model "" here; +choose one this service serves with /crew or add its service with codeaf connect`. **At a shell you choose**: `--high` replaces the list, `--low` sets the summaries' models, and `--variant` sets the reasoning effort every call asks for. @@ -689,7 +694,10 @@ senior-dev's own flags on `run`: 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`; + 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; - `--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/run/delegate_child_test.go b/internal/run/delegate_child_test.go index 771c2b712..4bb190816 100644 --- a/internal/run/delegate_child_test.go +++ b/internal/run/delegate_child_test.go @@ -50,6 +50,12 @@ func childProgram() delegate.Delegate { } func childBody(ctx context.Context, host delegate.Host, args []string) error { + if os.Getenv("FAKE_READ_IGNORED") == "1" { + if _, err := os.ReadFile(os.Getenv("SENIOR_DEV_IGNORED_AT_START")); err != nil { + host.Terminal(delegate.Ending{Status: delegate.StatusCrashed, Message: "read start-time ignore list: " + err.Error()}) + return nil + } + } if path := os.Getenv("FAKE_API_FILE"); path != "" { api := host.Models() _ = os.WriteFile(path, []byte(api.BaseURL+"\n"+api.Token+"\n"), 0o600) diff --git a/internal/run/delegateworker_test.go b/internal/run/delegateworker_test.go index a94dcba18..aaeb8d9c4 100644 --- a/internal/run/delegateworker_test.go +++ b/internal/run/delegateworker_test.go @@ -367,6 +367,34 @@ func TestDelegateWorkerPassesTheRunBranchAndIgnoreRecordToItsChild(t *testing.T) } } +// The chat readies a plain folder before the worker starts a real child. The +// child must be able to read the safety list it was handed before model work. +func TestDelegateWorkerChildReadsPlainFoldersStartTimeIgnoreList(t *testing.T) { + t.Setenv("CODEAF_HOME", t.TempDir()) + t.Setenv("FAKE_READ_IGNORED", "1") + store := runOpenStore(t) + workspace := t.TempDir() + program, setup, calling, _ := realChild(t, 0, "1") + program.Lands = delegate.LandsTree + folder, err := session.PrepareProgramFolder(session.ProgramFolderOrder{ + Program: program, Dir: workspace, Brief: "Make the feature", Holder: "the chat's run", + Keep: filepath.Join(t.TempDir(), "run"), + }) + if err != nil { + t.Fatal(err) + } + defer folder.Finish("done") + setup.PlainFolder = true + setup.IgnoredFile = folder.IgnoredFile() + report, err := run.NewDelegateWorker(store, workspace, program, setup, 1, 0).Run(runContext(t), *store.Task(store.RootID())) + if err != nil || report.Steps != 1 || len(calling.seen()) != 1 { + t.Fatalf("plain-folder child = %+v, %v, calls %q; want one answered call", report, err, calling.seen()) + } + if _, err := os.Stat(filepath.Join(workspace, ".git")); !os.IsNotExist(err) { + t.Fatalf("the plain folder acquired a repository: %v", err) + } +} + // A program that ended without finishing says why, and the run carries its // words whole to whoever drew the row: its status word, its sentence and its // account, not only the run's one word for every unfinished ending. diff --git a/internal/session/programfolder.go b/internal/session/programfolder.go index 8dd0d4f1a..7a50925d2 100644 --- a/internal/session/programfolder.go +++ b/internal/session/programfolder.go @@ -246,6 +246,13 @@ func PrepareProgramFolder(order ProgramFolderOrder) (*ProgramFolder, error) { folder.NotesWereThere = err == nil } if !repo { + // A PLAIN FOLDER HAS NO GIT IGNORE RULES, but both launch roads hand + // the child this path. An empty, readable list keeps the child's + // unreadable-list safety rule intact without aborting a plain run. + if err := folder.writeIgnoredAtStart(nil); err != nil { + folder.release() + return nil, err + } if outer != "" && !holdsHomeFolder(outer) { folder.IgnoredOuter = outer folder.Outer = "" @@ -259,15 +266,9 @@ func PrepareProgramFolder(order ProgramFolderOrder) (*ProgramFolder, error) { return nil, fmt.Errorf("read paths ignored at the start in %s: %s", dir, firstLine(ignored)) } folder.IgnoredAtStart = strings.Split(strings.TrimSuffix(ignored, "\x00"), "\x00") - if path := folder.IgnoredFile(); path != "" { - if err := os.MkdirAll(folder.Keep, 0o700); err != nil { - folder.release() - return nil, err - } - if err := os.WriteFile(path, []byte(ignored), 0o600); err != nil { - folder.release() - return nil, err - } + if err := folder.writeIgnoredAtStart([]byte(ignored)); err != nil { + folder.release() + return nil, err } carried, err := folder.carryOn() if !carried && err == nil { @@ -287,6 +288,19 @@ func PrepareProgramFolder(order ProgramFolderOrder) (*ProgramFolder, error) { return folder, nil } +// writeIgnoredAtStart makes the child's frozen safety list before either road +// can launch it. The empty file in a plain folder means no git rules existed. +func (f *ProgramFolder) writeIgnoredAtStart(body []byte) error { + path := f.IgnoredFile() + if path == "" { + return nil + } + if err := os.MkdirAll(f.Keep, 0o700); err != nil { + return err + } + return os.WriteFile(path, body, 0o600) +} + // carryOn takes up the branch an earlier finished run of the same program left // checked out in this folder, and answers false when there is none to take up. // diff --git a/internal/session/programfolder_safety_test.go b/internal/session/programfolder_safety_test.go index 3c589715a..82f1779ab 100644 --- a/internal/session/programfolder_safety_test.go +++ b/internal/session/programfolder_safety_test.go @@ -38,6 +38,9 @@ func TestProgramFolderNeverCommitsPathsIgnoredAtStartOrRunCaches(t *testing.T) { writeFile(t, filepath.Join(repo, ".pytest_cache", "state"), "cache") writeFile(t, filepath.Join(repo, "made.txt"), "work\n") folder.Finish("done") + if frozen, err := os.ReadFile(folder.IgnoredFile()); err != nil || string(frozen) != string(ignoredRecord) { + t.Fatalf("the repository's frozen ignore list changed during the run: %q, %v", frozen, err) + } paths := gitOut(t, repo, "ls-tree", "-r", "--name-only", "HEAD") for _, want := range []string{".gitignore", "made.txt"} { if !strings.Contains(paths, want) { @@ -68,6 +71,9 @@ func TestProgramFolderInsideIgnoredDirectoryStaysPlain(t *testing.T) { t.Fatal(err) } folder := prepareIn(t, testPrograms("fake")[0], inside, "Build here") + if ignored, err := os.ReadFile(folder.IgnoredFile()); err != nil || len(ignored) != 0 { + t.Fatalf("the ignored subfolder needs a readable empty safety list: %q, %v", ignored, err) + } writeFile(t, filepath.Join(inside, "result.txt"), "made\n") end := folder.Finish("done") if !folder.Plain() || folder.Dir != inside || strings.TrimSpace(gitOut(t, repo, "rev-parse", "main")) != before || currentBranch(repo) != "main" { diff --git a/internal/session/taskmodel.go b/internal/session/taskmodel.go index 62a5cb27b..46cabb7fe 100644 --- a/internal/session/taskmodel.go +++ b/internal/session/taskmodel.go @@ -36,6 +36,7 @@ package session // caller had before this file existed. import ( + "fmt" "slices" "sort" "strings" @@ -449,14 +450,62 @@ func (a *Agent) resolveProgramModels(word string) taskModelChoice { // resolveProgramWord is one word of [Agent.resolveProgramModels]: a model, a // shortlist of the ones a service serves, or the refusal. func (a *Agent) resolveProgramWord(word string) taskModelChoice { - sources := a.programSources() + return resolveProgramWordWithSources(word, a.programSources(), a.resolveTaskModel) +} + +// ResolveProgramShellModels gives a shell program the same model-word matcher +// and service check as a chat proposal. A shell has no card for a shortlist, +// so it asks for a more precise word before starting the child. +func ResolveProgramShellModels(words string, available []string, sources modelsource.Set) (string, error) { + var resolved []string + for _, part := range strings.Split(words, ",") { + word := strings.TrimSpace(part) + if word == "" { + continue + } + choice := resolveProgramWordWithSources(word, sources, func(word string) taskModelChoice { + if len(available) == 0 { + return taskModelChoice{model: word} + } + matches := matchTaskModel(word, available) + if len(matches) == 1 { + return taskModelChoice{model: matches[0]} + } + if len(matches) > 1 { + return taskModelChoice{options: matches} + } + return taskModelChoice{problem: "unknown model"} + }) + if choice.problem != "" || len(choice.options) > 0 || choice.model == "" { + return "", ProgramShellModelRefusal(word) + } + if !slices.Contains(resolved, choice.model) { + resolved = append(resolved, choice.model) + } + } + if len(resolved) == 0 { + return "", fmt.Errorf("--high names no model; choose one with /crew or add its service with codeaf connect") + } + return strings.Join(resolved, ","), nil +} + +// ProgramShellModelRefusal is the shell's one sentence for a model it cannot +// hand to a child, including a model named before any service key is present. +func ProgramShellModelRefusal(word string) error { + return fmt.Errorf("cannot use model %q here; choose one this service serves with /crew or add its service with codeaf connect", word) +} + +// resolveProgramWordWithSources is the common decision for a chat proposal +// and a shell flag: a connected service prefix wins, then the task's model +// matcher resolves the person's word, and the selected service must answer. +func resolveProgramWordWithSources(word string, sources modelsource.Set, resolveTask func(string) taskModelChoice) taskModelChoice { if segment, bare := modelsource.Split(word, sources.Written()); segment != "" && bare != "" { if !ServesModel(sources, word) { return taskModelChoice{problem: programUnservedProblem(word)} } return taskModelChoice{model: word} } - choice := a.resolveTaskModel(word) + choice := resolveTask(word) serves := func(model string) bool { return sources.Empty() || ServesModel(sources, model) } switch { case choice.problem != "":