From 5744e080ef7b4ae07d8b1dae992f9382c777f5fc Mon Sep 17 00:00:00 2001 From: J Date: Thu, 4 Jun 2026 14:19:12 +0800 Subject: [PATCH] fix(agent): harden pi catalog parsing and opencode fallback (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- server/pkg/agent/models.go | 64 ++++++++++++++++++++++++-------- server/pkg/agent/models_test.go | 66 +++++++++++++++++++++++++-------- 2 files changed, 99 insertions(+), 31 deletions(-) diff --git a/server/pkg/agent/models.go b/server/pkg/agent/models.go index 5c0fe3ed7..faded3e5d 100644 --- a/server/pkg/agent/models.go +++ b/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 diff --git a/server/pkg/agent/models_test.go b/server/pkg/agent/models_test.go index a1611c2dc..47c12dce1 100644 --- a/server/pkg/agent/models_test.go +++ b/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