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,18 +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 {
-		if len(out) == 0 {
-			cmd = exec.CommandContext(runCtx, executablePath, "models")
-			hideAgentWindow(cmd)
-			out, err = cmd.Output()
-			if err != nil && len(out) == 0 {
-				return []Model{}, 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, _ = 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
@@ -589,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
@@ -608,11 +622,9 @@ func parsePiModels(output string) []Model {
 		} else {
 			continue
 		}
-		// A real row resolves to `provider/model` with both halves present.
-		// Drop anything else — e.g. a stray `Warning: No models match pattern
-		// "..."` line, which pi can interleave with the catalog when an agent
-		// config holds stale patterns (#3729). Without this guard the leading
-		// `Warning:` token becomes a bogus `Warning/` model in the picker.
+		// 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
 		}
@@ -629,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

server/pkg/agent/models_test.go

@@ -465,7 +465,11 @@ func TestDiscoverPiModelsNonZeroExit(t *testing.T) {
 
 	const table = "provider         model        context  max-out  thinking  images\n" +
 		"glm-coding-plan  glm-4.7      202.8K   16.4K    no        no"
-	const warning = `Warning: No models match pattern "opencode-go/mimo-v2-omni"`
+	// 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
@@ -477,15 +481,23 @@ func TestDiscoverPiModelsNonZeroExit(t *testing.T) {
 			name: "catalog on stdout",
 			script: "#!/bin/sh\n" +
 				"cat <<'EOF'\n" + table + "\nEOF\n" +
-				"echo " + strconv.Quote(warning) + " >&2\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 on stderr",
+			name: "catalog and prefixed warning on stderr",
 			script: "#!/bin/sh\n" +
-				"cat >&2 <<'EOF'\n" + table + "\n" + warning + "\nEOF\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",
 		},
 	}
@@ -499,22 +511,46 @@ func TestDiscoverPiModelsNonZeroExit(t *testing.T) {
 			if err != nil {
 				t.Fatalf("discoverPiModels: %v", err)
 			}
-			var found bool
-			for _, m := range models {
-				if m.ID == "glm-coding-plan/glm-4.7" {
-					found = true
-				}
-				if m.ID == "Warning/" || m.Provider == "Warning" {
-					t.Errorf("warning line leaked into the catalog as a bogus model: %+v", m)
-				}
-			}
-			if !found {
-				t.Fatalf("expected glm-coding-plan/glm-4.7 despite non-zero exit, got %+v", models)
+			// 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