fix: apply the omp audit — card before model, tools only when registered

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.
 

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

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

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

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) {

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
 	}

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)
 	}

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) {

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}