diff --git a/CHANGELOG.md b/CHANGELOG.md index d4ea1637..69b8dc61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,10 +12,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **oh-my-pi runs the lich plugin.** Settings › lich plugin now offers omp the same install as the others, writing the released extension into omp's own `extensions/` directory — so an omp card shows what its session is doing, - renames itself from the conversation's title, and refreshes git status as - files change. The install also registers lich's MCP server in omp's - `mcp.json`, merged in beside whatever is already there, which puts the tools - for reaching the other sessions in an omp agent's own tool list. Two gaps are + refreshes git status as files change, and takes its name from the + conversation's title once omp has written one. The install also registers + lich's MCP server in omp's `mcp.json`, merged in beside whatever is already + there, which puts the tools for reaching the other sessions in an omp agent's + own tool list. Two gaps are the harness's own and Settings names them: omp has no observed approval event, so a session waiting on your permission shows a spinner rather than a bell. diff --git a/internal/agentplugin/agentplugin.go b/internal/agentplugin/agentplugin.go index b8554348..61644339 100644 --- a/internal/agentplugin/agentplugin.go +++ b/internal/agentplugin/agentplugin.go @@ -83,6 +83,11 @@ type Service struct { // lookPath resolves a binary on PATH, injected so tests decide which // provider CLIs the machine has. lookPath func(string) (string, error) + // lichBin names this lich for the registrations written into a harness's + // config. Injected for the same reason lookPath is: a test has to be able to + // say "this binary cannot be resolved", which is the branch that decides + // whether a session is promised tools it does not have. + lichBin func() string } // New returns a service that shells out through bins. @@ -93,6 +98,7 @@ func New(bins BinResolver) *Service { latestURL: latestReleaseURL, rawBase: rawBaseURL, lookPath: exec.LookPath, + lichBin: lichBinary, } } @@ -155,10 +161,26 @@ func (s *Service) HasTools(provider string) bool { if !slices.Contains(supported, provider) || !s.available(provider) { return false } + if registersServerAtInstall(provider) && s.lichBin() == "" { + return false + } version, installed := s.installedVersion(provider) return installed && !semver.Less(version, toolsMinVersion) } +// registersServerAtInstall reports whether a provider's tools come from the MCP +// server the install writes into its config, rather than from the plugin itself. +// +// It is what stops HasTools promising tools the install could not register: both +// of these write the lich binary's own path, and both leave the registration out +// when it cannot be resolved (crushrcBlock, ompMCPDocument) — so a session there +// has the plugin's reports and no tool list, and must be told the command line +// instead. opencode is absent because its plugin defines the tools itself, with +// no binary to name. +func registersServerAtInstall(provider string) bool { + return provider == providers.Crush || provider == providers.OMP +} + // Installed reports whether the lich plugin is installed for a provider — that // its sessions report what they are doing (docs/hooks/). The relay asks before // reading a session's silence as an answer: a provider that never reports is diff --git a/internal/agentplugin/agentplugin_test.go b/internal/agentplugin/agentplugin_test.go index 9172dfea..137fcee1 100644 --- a/internal/agentplugin/agentplugin_test.go +++ b/internal/agentplugin/agentplugin_test.go @@ -244,6 +244,7 @@ func serveBody(t *testing.T, status int, body string) *Service { latestURL: srv.URL, bins: stubBins{}, lookPath: func(name string) (string, error) { return "/usr/bin/" + name, nil }, + lichBin: lichBinary, } } @@ -417,7 +418,7 @@ func fakeCLI(t *testing.T, failOn string) (*Service, func() []string) { } return strings.Split(strings.TrimSpace(string(data)), "\n") } - return &Service{bins: stubBin(self)}, calls + return &Service{bins: stubBin(self), lichBin: lichBinary}, calls } // TestInstallAddsMarketplaceThenPlugin pins the order and the exact targets per @@ -603,6 +604,71 @@ func TestHasToolsIsAlwaysTrueForTheHarnessesToldAtSpawn(t *testing.T) { } } +// Which providers depend on a registration lich writes with its own path — the +// two whose installs leave that registration out when the path cannot be +// resolved (crushrcBlock, ompMCPDocument). Pinned as a list rather than derived, +// because a provider added to one side and not the other is exactly the drift +// that promises tools nobody registered. +func TestRegistersServerAtInstall(t *testing.T) { + for _, provider := range []string{providers.Crush, providers.OMP} { + if !registersServerAtInstall(provider) { + t.Errorf("%s writes lich's server at install, but is not treated as doing so", provider) + } + } + for _, provider := range []string{providers.Claude, providers.Codex, providers.OpenCode} { + if registersServerAtInstall(provider) { + t.Errorf("%s does not get its tools from a registration lich writes", provider) + } + } +} + +// And the decision that rule feeds: an installed oh-my-pi whose lich cannot name +// its own binary has the plugin's reports and no tool list, so a relayed message +// must name the shell command instead of a tool that is not there. +func TestHasToolsIsFalseWhenTheServerCouldNotBeRegistered(t *testing.T) { + agent := ompHome(t) + writeMarkedModule(t, filepath.Join(agent, ompExtensionsDir, ompFile), toolsMinVersion) + s := New(stubBins{}) + s.lookPath = func(string) (string, error) { return "/usr/bin/omp", nil } + + if !s.HasTools(providers.OMP) { + t.Fatal("HasTools(omp) = false with the plugin installed and a binary to register") + } + + s.lichBin = func() string { return "" } + if s.HasTools(providers.OMP) { + t.Error("HasTools(omp) = true, but no server could be registered for it") + } +} + +// opencode is the other side of that rule: its plugin defines the tools itself, +// so there is no binary to name and an unresolvable one changes nothing. +func TestHasToolsIgnoresTheBinaryForOpencode(t *testing.T) { + config := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", config) + writeMarkedModule(t, filepath.Join(config, "opencode", "plugin", opencodeFile), toolsMinVersion) + s := New(stubBins{}) + s.lookPath = func(string) (string, error) { return "/usr/bin/opencode", nil } + s.lichBin = func() string { return "" } + + if !s.HasTools(providers.OpenCode) { + t.Error("HasTools(opencode) = false, but its tools come from the plugin, not from a server") + } +} + +// writeMarkedModule lays down what an install of that version leaves behind: the +// module with lich's marker line, which is where the version is read back from. +func writeMarkedModule(t *testing.T, path, version string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + marker := jsComment + " " + markerName + " v" + version + " — installed by lich\n" + if err := os.WriteFile(path, []byte(marker), 0o644); err != nil { + t.Fatal(err) + } +} + func TestHasToolsFollowsThePluginVersionElsewhere(t *testing.T) { for _, tt := range []struct { name string diff --git a/internal/agentplugin/crush.go b/internal/agentplugin/crush.go index 44263b26..ae98e133 100644 --- a/internal/agentplugin/crush.go +++ b/internal/agentplugin/crush.go @@ -86,7 +86,7 @@ func (s *Service) crushInstall() error { return fmt.Errorf("write %s: %w", path, err) } } - return s.writeCrushrc(version, dir, lichBinary()) + return s.writeCrushrc(version, dir, s.lichBin()) } // writeCrushrc replaces lich's block in Crush's crushrc, creating the file when diff --git a/internal/agentplugin/files_test.go b/internal/agentplugin/files_test.go index 243a65b7..0698a9ef 100644 --- a/internal/agentplugin/files_test.go +++ b/internal/agentplugin/files_test.go @@ -52,6 +52,7 @@ func fileServer(t *testing.T, files map[string]string) (*Service, func() []strin rawBase: srv.URL, bins: stubBins{}, lookPath: func(name string) (string, error) { return "/usr/bin/" + name, nil }, + lichBin: lichBinary, }, func() []string { return asked } } @@ -192,6 +193,43 @@ func ompHome(t *testing.T) string { return dir } +// The profile beats the explicit override, which is the order `omp config path` +// applies and the opposite of how the two read. Asserted through a real install +// rather than on the resolver alone: getting it backwards writes the extension +// into a directory omp never scans, and every report goes missing in silence. +// (internal/terminal resolves the same rule for resume, and pins it separately.) +func TestOMPInstallFollowsTheProfileOverTheOverride(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + // Asserted, not assumed: a platform whose home this redirect misses would + // send the install into the real user's directory and still pass. + if dir, err := os.UserHomeDir(); err != nil || dir != home { + t.Fatalf("home redirect missed: %q (err %v)", dir, err) + } + t.Setenv("PI_CODING_AGENT_DIR", t.TempDir()) + t.Setenv("OMP_PROFILE", "work") + s, _ := fileServer(t, map[string]string{tagged(ompSource): "export default () => {}\n"}) + + if err := s.Install(providers.OMP); err != nil { + t.Fatalf("Install: %v", err) + } + + profile := filepath.Join(home, ".omp", "profiles", "work", "agent") + if _, err := os.Stat(filepath.Join(profile, ompExtensionsDir, ompFile)); err != nil { + t.Fatalf("the extension is not in the profile's directory: %v", err) + } + if override := os.Getenv("PI_CODING_AGENT_DIR"); override != "" { + if _, err := os.Stat(filepath.Join(override, ompExtensionsDir, ompFile)); !os.IsNotExist(err) { + t.Errorf("the override was written too (stat err = %v)", err) + } + } + // And the version reads back from the same place it was written to. + if got, ok := s.installedVersion(providers.OMP); !ok || got != testVersion { + t.Errorf("installedVersion = (%q,%v), want (%q,true)", got, ok, testVersion) + } +} + func TestOMPInstallWritesTheExtensionAndRegistersTheServer(t *testing.T) { agent := ompHome(t) s, asked := fileServer(t, map[string]string{ @@ -202,7 +240,7 @@ func TestOMPInstallWritesTheExtensionAndRegistersTheServer(t *testing.T) { t.Fatalf("Install: %v", err) } - data, err := os.ReadFile(filepath.Join(agent, "extensions", ompFile)) + data, err := os.ReadFile(filepath.Join(agent, ompExtensionsDir, ompFile)) if err != nil { t.Fatalf("read the installed extension: %v", err) } @@ -292,7 +330,7 @@ func TestOMPInstallRefusesAnUnreadableDocument(t *testing.T) { } // And nothing else was written either: an extension reporting into a session // that never got the tools is the half-install this ordering exists to avoid. - if _, err := os.Stat(filepath.Join(agent, "extensions", ompFile)); !os.IsNotExist(err) { + if _, err := os.Stat(filepath.Join(agent, ompExtensionsDir, ompFile)); !os.IsNotExist(err) { t.Errorf("the refused install wrote the extension anyway (stat err = %v)", err) } } @@ -306,7 +344,7 @@ func TestOMPInstallWritesNothingWhenTheFetchFails(t *testing.T) { if err := s.Install(providers.OMP); err == nil { t.Fatal("Install: want an error when the release has no extension, got nil") } - if _, err := os.Stat(filepath.Join(agent, "extensions", ompFile)); !os.IsNotExist(err) { + if _, err := os.Stat(filepath.Join(agent, ompExtensionsDir, ompFile)); !os.IsNotExist(err) { t.Errorf("a failed install left an extension behind (stat err = %v)", err) } if _, err := os.Stat(filepath.Join(agent, ompMCPFile)); !os.IsNotExist(err) { diff --git a/internal/agentplugin/omp.go b/internal/agentplugin/omp.go index d32f1234..f2be40d5 100644 --- a/internal/agentplugin/omp.go +++ b/internal/agentplugin/omp.go @@ -24,9 +24,11 @@ import ( const ( // ompSource is the module's path inside the plugin repository, and ompFile - // the name it takes in omp's extensions directory. - ompSource = "omp/lich.js" - ompFile = "lich.js" + // the name it takes in omp's extensions directory — the directory omp scans + // unprompted, which is what makes the file being there the whole install. + ompSource = "omp/lich.js" + ompFile = "lich.js" + ompExtensionsDir = "extensions" // ompMCPFile is the document omp reads MCP servers from, in the same agent // directory as the extensions. @@ -53,11 +55,11 @@ func (s *Service) ompInstall() error { // Merged before anything is written, because the only way this step fails is // a document lich must not replace — and an install that leaves an extension // behind while refusing the registration is worse than one the user retries. - registration, err := ompMCPDocument(dir, lichBinary()) + registration, err := ompMCPDocument(dir, s.lichBin()) if err != nil { return err } - extensions := filepath.Join(dir, "extensions") + extensions := filepath.Join(dir, ompExtensionsDir) if err := os.MkdirAll(extensions, 0o755); err != nil { return fmt.Errorf("create %s: %w", extensions, err) } @@ -84,7 +86,7 @@ func (s *Service) ompInstalledVersion() (string, bool) { if err != nil { return "", false } - data, err := os.ReadFile(filepath.Join(dir, "extensions", ompFile)) + data, err := os.ReadFile(filepath.Join(dir, ompExtensionsDir, ompFile)) if err != nil { return "", false } diff --git a/internal/spawn/spawn.go b/internal/spawn/spawn.go index b2afe84e..4769e029 100644 --- a/internal/spawn/spawn.go +++ b/internal/spawn/spawn.go @@ -216,17 +216,21 @@ func (s *Service) Open(fromID, projectName, kind, worktree, base, model string) if err := s.sessions.AddSession(target.ID, id, label, kind, stored, opened.NextSeq); err != nil { return Session{}, err } - if model != "" { - if err := s.sessions.SetSessionModel(id, model); err != nil { - return Session{}, err - } - } // Announced before the spawn, and regardless of how it goes: the row exists // either way, so the card has to exist either way too — a session only the // database knows about is one the user cannot reach to see what went wrong. if s.events != nil { s.events.Emit(OpenedEventName, opened) } + // After the card, for that same reason: the model is an override on a row + // that already exists, so a write that fails must not be what hides the + // session. The spawn below reads the model back from the row, so a failure + // here costs the provider's default and nothing else. + if model != "" { + if err := s.sessions.SetSessionModel(id, model); err != nil { + return Session{}, err + } + } if err := s.term.Start(id, target.ID, cwd, kind, "", opened.Name, setup, startCols, startRows); err != nil { return Session{}, fmt.Errorf("session %q was created but its terminal did not start: %w", label, err) } diff --git a/internal/spawn/spawn_test.go b/internal/spawn/spawn_test.go index 602b5f44..2c65a88f 100644 --- a/internal/spawn/spawn_test.go +++ b/internal/spawn/spawn_test.go @@ -25,7 +25,8 @@ type fakeSessions struct { rows []added addErr error // models records the model written on each session row, keyed by session id. - models map[string]string + models map[string]string + modelErr error // deleted/parked/purged record what a close asked the store to do, and // against which active session it left the project. deleted []closedRow @@ -68,6 +69,9 @@ func (f *fakeSessions) AddSession(projectID, sessionID, label, kind, path string } func (f *fakeSessions) SetSessionModel(sessionID, model string) error { + if f.modelErr != nil { + return f.modelErr + } if f.models == nil { f.models = map[string]string{} } @@ -338,6 +342,25 @@ func TestOpenRecordsTheModelOnTheRow(t *testing.T) { } } +// The model is written after the card is announced, and this is why: a row with +// no card is a session the user cannot reach to see what went wrong, so the one +// write that can fail after the row exists must not be what hides it. The model +// is an override — losing it costs the provider's default and nothing more. +func TestOpenAnnouncesTheCardEvenWhenTheModelCannotBeRecorded(t *testing.T) { + svc, sessions, _, _, events := newService(t) + sessions.modelErr = errors.New("database is locked") + + if _, err := svc.Open("s1", "", "claude", "", "", "opus"); err == nil { + t.Fatal("Open: want the write error reported, got nil") + } + if len(sessions.rows) != 1 { + t.Fatalf("wrote %d rows, want the session's own", len(sessions.rows)) + } + if len(events.events) != 1 || events.events[0].name != OpenedEventName { + t.Errorf("events = %+v, want the card announced for the row that exists", events.events) + } +} + // A session opened with no model leaves the column alone: "" is what every // session the window opens carries, and what leaves the provider on its default. func TestOpenWithoutAModelWritesNone(t *testing.T) { diff --git a/internal/terminal/command.go b/internal/terminal/command.go index 0c6d2e1f..f1725aae 100644 --- a/internal/terminal/command.go +++ b/internal/terminal/command.go @@ -119,13 +119,26 @@ func nameArgs(kind, name string) []string { if kind != providers.Claude { return nil } - name = strings.TrimSpace(name) - if name == "" || strings.HasPrefix(name, "-") { + name, ok := flagValue(name) + if !ok { return nil } return []string{claudeNameFlag, name} } +// flagValue trims a value lich is about to hand a provider flag and reports +// whether it can be passed at all. Nothing to pass is one answer; a value that +// starts with a dash is the other — the provider would read it as another flag, +// and the one thing lich must never do is turn a name or a model somebody typed +// into an argument that changes how the agent runs. +func flagValue(value string) (string, bool) { + value = strings.TrimSpace(value) + if value == "" || strings.HasPrefix(value, "-") { + return "", false + } + return value, true +} + // providerArgs assembles the whole argument list for one spawn. Ordering is // decided here rather than by appending in call order, because the two // providers constrain it in opposite directions: Codex spells resume as a @@ -152,9 +165,9 @@ func providerArgs(kind, name, resume, model, lichBin string, skipPermissions boo // can act on. A value that would be read as a flag is dropped instead, as it is // for a session name. func modelArgs(kind, model string) []string { - flag, ok := modelFlags[kind] - model = strings.TrimSpace(model) - if !ok || model == "" || strings.HasPrefix(model, "-") { + flag, wired := modelFlags[kind] + model, usable := flagValue(model) + if !wired || !usable { return nil } return []string{flag, model}