fix(agent): harden pi catalog parsing and opencode fallback (review)

Addresses review feedback on the discovery hardening:

- parsePiModels: the previous guard only dropped the `Warning:`-prefixed
  form (which collapses to an empty `Warning/` half). The real pattern
  warning can arrive without that prefix — `No models match pattern "..."` —
  and the table branch would turn it into a bogus `No/models` model. Filter
  pi's diagnostic lines (unmatched-pattern message, `Warning:`/`Error:`/`Info:`
  prefixes) before field-splitting, with the empty-half check kept as a
  structural backstop.

- discoverOpenCodeModels: parse the verbose output even on a non-zero exit and
  only fall back to plain `opencode models` when nothing parses. The earlier
  version skipped the fallback whenever verbose printed any bytes, so
  unparseable noise on a non-zero exit returned empty — worse than before.

Tests: pi non-zero-exit cases now assert the result is exactly the resolvable
model (no junk row) and cover the un-prefixed warning; new OpenCode test covers
verbose-noise-then-fallback.

Co-authored-by: multica-agent <github@multica.ai>

server/pkg/agent/models.go

@@ -360,16 +360,24 @@ func discoverOpenCodeModels(ctx context.Context, executablePath string) ([]Model
 	defer cancel()
 	cmd := exec.CommandContext(runCtx, executablePath, "models", "--verbose")
 	hideAgentWindow(cmd)
-	out, err := cmd.Output()
-	if err != nil {
+	// Parse whatever the verbose command printed, even on a non-zero exit — a
+	// stale config entry can make `opencode models` exit non-zero while still
+	// listing the resolvable catalog (mirrors the pi path; see #3729/#3627).
+	out, _ := cmd.Output()
+	models := parseOpenCodeModels(string(out))
+	if len(models) == 0 {
+		// Verbose yielded nothing usable (unsupported flag, error text, or an
+		// empty list). Retry the plain command, which omits the per-model JSON
+		// but still prints the IDs.
 		cmd = exec.CommandContext(runCtx, executablePath, "models")
 		hideAgentWindow(cmd)
-		out, err = cmd.Output()
-		if err != nil {
-			return []Model{}, nil
-		}
+		out, _ = cmd.Output()
+		models = parseOpenCodeModels(string(out))
 	}
-	return parseOpenCodeModels(string(out)), nil
+	if len(models) == 0 {
+		return []Model{}, nil
+	}
+	return models, nil
 }
 
 // parseOpenCodeModels accepts the `opencode models` text output and
@@ -562,7 +570,7 @@ func discoverPiModels(ctx context.Context, executablePath string) ([]Model, erro
 	var stderr strings.Builder
 	cmd.Stderr = &stderr
 	stdout, err := cmd.Output()
-	if err != nil {
+	if err != nil && len(stdout) == 0 && stderr.Len() == 0 {
 		return []Model{}, nil
 	}
 	text := string(stdout)
@@ -587,6 +595,14 @@ func parsePiModels(output string) []Model {
 		if line == "" {
 			continue
 		}
+		// pi interleaves human-readable diagnostics with the catalog when an
+		// agent config references stale patterns — e.g.
+		//   Warning: No models match pattern "opencode-go/mimo-v2-omni"
+		// Skip them before field-splitting; otherwise prose tokens are coined
+		// into bogus models like `No/models` or `Warning/`. See #3729.
+		if isPiDiscoveryNoise(line) {
+			continue
+		}
 		fields := strings.Fields(line)
 		if len(fields) == 0 {
 			continue
@@ -606,6 +622,12 @@ func parsePiModels(output string) []Model {
 		} else {
 			continue
 		}
+		// A real id has a non-empty provider and model on both sides of the
+		// slash. Drop anything that doesn't (e.g. a stray `something:` token),
+		// a cheap structural backstop on top of the diagnostic filter above.
+		if slash := strings.Index(id, "/"); slash <= 0 || slash == len(id)-1 {
+			continue
+		}
 		if seen[id] {
 			continue
 		}
@@ -619,6 +641,26 @@ func parsePiModels(output string) []Model {
 	return models
 }
 
+// isPiDiscoveryNoise reports whether a `pi --list-models` line is a diagnostic
+// message rather than a catalog row. pi prints these alongside the table when
+// an agent config references stale provider/model patterns, e.g.
+//
+//	Warning: No models match pattern "opencode-go/mimo-v2-omni"
+//
+// The `Warning:` prefix is not guaranteed across versions, so the unmatched-
+// pattern message is also matched on its own. These are prose, not
+// `provider model` rows; without skipping them the field splitter coins bogus
+// models like `No/models`. See #3729.
+func isPiDiscoveryNoise(line string) bool {
+	lower := strings.ToLower(line)
+	if strings.Contains(lower, "no models match pattern") {
+		return true
+	}
+	return strings.HasPrefix(lower, "warning:") ||
+		strings.HasPrefix(lower, "error:") ||
+		strings.HasPrefix(lower, "info:")
+}
+
 // discoverHermesModels spins up a throwaway `hermes acp` process,
 // drives just enough of the protocol to receive the model list
 // advertised in the `session/new` response, and shuts it down. The
@@ -942,7 +984,7 @@ func discoverCursorModels(ctx context.Context, executablePath string) ([]Model,
 	cmd := exec.CommandContext(runCtx, executablePath, "--list-models")
 	hideAgentWindow(cmd)
 	out, err := cmd.Output()
-	if err != nil {
+	if err != nil && len(out) == 0 {
 		return cursorStaticModels(), nil
 	}
 	models := parseCursorModels(string(out))
@@ -1039,7 +1081,7 @@ func discoverOpenclawAgents(ctx context.Context, executablePath string) ([]Model
 		cmd := exec.CommandContext(runCtx, executablePath, jsonArgs...)
 		hideAgentWindow(cmd)
 		out, err := cmd.Output()
-		if err != nil {
+		if err != nil && len(out) == 0 {
 			continue
 		}
 		if models, ok := parseOpenclawAgentsJSON(out); ok {
@@ -1053,7 +1095,7 @@ func discoverOpenclawAgents(ctx context.Context, executablePath string) ([]Model
 	cmd := exec.CommandContext(runCtx, executablePath, "agents", "list")
 	hideAgentWindow(cmd)
 	out, err := cmd.Output()
-	if err != nil {
+	if err != nil && len(out) == 0 {
 		return []Model{}, nil
 	}
 	return parseOpenclawAgents(string(out)), nil

server/pkg/agent/models_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"path/filepath"
 	"runtime"
+	"strconv"
 	"strings"
 	"testing"
 )
@@ -450,6 +451,106 @@ bareword-only-line
 	}
 }
 
+// TestDiscoverPiModelsNonZeroExit verifies that discoverPiModels still returns
+// the resolvable catalog when `pi --list-models` exits non-zero. Pi exits
+// non-zero (and warns) when an agent config references stale provider/model
+// patterns that no longer match the local catalog. Before the fix the daemon
+// discarded the populated output on any non-zero exit and returned an empty
+// list, so the UI model picker was blank even though the runtime was online and
+// agents ran fine. See GitHub #3729.
+func TestDiscoverPiModelsNonZeroExit(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("fake pi binary is a /bin/sh script")
+	}
+
+	const table = "provider         model        context  max-out  thinking  images\n" +
+		"glm-coding-plan  glm-4.7      202.8K   16.4K    no        no"
+	// The unmatched-pattern warning, with and without the `Warning:` prefix —
+	// the prefix is not guaranteed across pi versions, and the bare form is
+	// what slips past a naive guard into a bogus `No/models` model.
+	const prefixed = `Warning: No models match pattern "opencode-go/mimo-v2-omni"`
+	const bare = `No models match pattern "opencode-go/mimo-v2-pro"`
+
+	cases := []struct {
+		name   string
+		script string
+	}{
+		{
+			// Newer pi prints the catalog to stdout; the stale-pattern
+			// warning goes to stderr and the process exits non-zero.
+			name: "catalog on stdout",
+			script: "#!/bin/sh\n" +
+				"cat <<'EOF'\n" + table + "\nEOF\n" +
+				"echo " + strconv.Quote(prefixed) + " >&2\n" +
+				"exit 1\n",
+		},
+		{
+			// Older pi prints the catalog (and the warning) to stderr; same
+			// non-zero exit. The stderr fallback must still parse the catalog.
+			name: "catalog and prefixed warning on stderr",
+			script: "#!/bin/sh\n" +
+				"cat >&2 <<'EOF'\n" + table + "\n" + prefixed + "\nEOF\n" +
+				"exit 1\n",
+		},
+		{
+			// Same, but the warning has no `Warning:` prefix — must not leak in
+			// as a `No/models` row.
+			name: "catalog and bare warning on stderr",
+			script: "#!/bin/sh\n" +
+				"cat >&2 <<'EOF'\n" + table + "\n" + bare + "\nEOF\n" +
+				"exit 1\n",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			fakePath := filepath.Join(t.TempDir(), "pi")
+			writeTestExecutable(t, fakePath, []byte(tc.script))
+
+			models, err := discoverPiModels(context.Background(), fakePath)
+			if err != nil {
+				t.Fatalf("discoverPiModels: %v", err)
+			}
+			// Exactly the resolvable model — no warning line coined into a
+			// bogus entry, no header row.
+			if len(models) != 1 || models[0].ID != "glm-coding-plan/glm-4.7" {
+				t.Fatalf("expected exactly [glm-coding-plan/glm-4.7] despite non-zero exit, got %+v", models)
+			}
+		})
+	}
+}
+
+// TestDiscoverOpenCodeModelsFallsBackOnVerboseNoise verifies that a non-zero
+// `opencode models --verbose` whose stdout is unparseable noise still falls
+// back to the plain `opencode models` command instead of returning empty. The
+// earlier fix skipped the fallback whenever verbose printed any bytes, which
+// regressed this case. Mirrors the pi hardening in #3729.
+func TestDiscoverOpenCodeModelsFallsBackOnVerboseNoise(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("fake opencode binary is a /bin/sh script")
+	}
+
+	// `opencode models --verbose` => $2 == "--verbose": emit noise + exit 1.
+	// `opencode models`           => no $2: print the plain catalog.
+	script := "#!/bin/sh\n" +
+		"if [ \"$2\" = \"--verbose\" ]; then\n" +
+		"  echo 'panic: catalog sync failed'\n" +
+		"  exit 1\n" +
+		"fi\n" +
+		"echo 'openai/gpt-4o'\n"
+
+	fakePath := filepath.Join(t.TempDir(), "opencode")
+	writeTestExecutable(t, fakePath, []byte(script))
+
+	models, err := discoverOpenCodeModels(context.Background(), fakePath)
+	if err != nil {
+		t.Fatalf("discoverOpenCodeModels: %v", err)
+	}
+	if len(models) != 1 || models[0].ID != "openai/gpt-4o" {
+		t.Fatalf("expected fallback to plain `opencode models` to yield [openai/gpt-4o], got %+v", models)
+	}
+}
+
 func TestParseOpenclawAgents(t *testing.T) {
 	input := `deepseek-v4   deepseek-v4
 claude-sonnet claude-sonnet-4-6