Files
multica/server/pkg/agent/codebuddy_discovery_fallback_test.go
3b08423684 MUL-6260 fix(daemon): treat custom runtime fixed_args as the argv prefix (#7052)
* fix(daemon): treat custom runtime fixed_args as the argv prefix (MUL-6260)

A custom runtime profile configured as `ccms start q36` failed every task
with `unknown option '-p'`. The profile's fixed_args were merged into
ExecOptions.ExtraArgs, which each backend appends *after* the protocol
flags it hardcodes, so the wrapper saw `-p` before its own `start`
subcommand had been selected and rejected it (GH #7046).

Ordering was only half the defect. ExtraArgs is opt-in — six of the
twenty-one backends read it — so on the other fifteen families, including
every ACP one, fixed_args were silently discarded rather than misordered.
Model discovery and `--version` probing carried the profile's path but not
its arguments, so even a fixed launch would have left the model picker and
the reported version describing the wrapper instead of the CLI behind it.

fixed_args now travel as agent.Config.LaunchPrefix and are spliced in
directly after the executable, ahead of everything a backend builds. The
prefix is applied by a new Command boundary in launch.go through which
every runtime process in the package is constructed — launches, version
probes and discovery alike — so a backend cannot forget it. A guard test
fails the build if any file outside launch.go calls os/exec directly;
distributed opt-in is what let ExtraArgs rot, and repeating it would rot
the same way.

Flag-style commands (`agent --model composer-2.5`) are unaffected: a CLI
takes its own flags in any position, which is why prefix-first can be the
single order rather than a per-runtime setting.

One deliberate behaviour change. The prefix used to sit last and win every
last-wins parse, so a profile pinning `--model` silently overrode the model
chosen on the agent — the UI showed one model and ran another. Prefix-first
inverts that: the more specific per-agent choice is the later token and
wins. Overlaps are logged. Protocol-critical flags are filtered out of the
prefix once in New(), which knows the family; positional tokens always pass
through. Codex additionally strips `-c` overrides aimed at daemon-managed
config namespaces, since those beat config.toml from any argv position.

The order was never documented, and every shipped fixed_args example is
flag-style, so this fills in a contract rather than breaking one.

Co-authored-by: multica-agent <[email protected]>

* fix(daemon): close the launch-prefix gaps found in review (MUL-6260)

Four defects from the first round of review on the fixed_args-as-argv-prefix
change.

Hermes could escape its own task overlay. The profile selection was resolved
from custom_args alone and stripped from custom_args alone, so a profile
configured as `hermes -p research` kept that flag in the launch prefix and
re-pointed HERMES_HOME past the overlay the daemon had just built. Resolution
now parses the argv hermes actually sees — launch prefix then custom args, in
process order — and the strip covers both regions. It also removes every
selection rather than the first: taking out the first promotes the second, and
two separately-configured argv regions make "two selections" an ordinary
configuration rather than a user mistake.

Positional tokens were not passing through as documented. filterLaunchPrefix
delegated to filterCustomArgs, whose blocklists deliberately carry positional
tokens — hermes and qwenpaw block `acp`, traecli blocks `acp`/`serve`, grok
blocks `agent`/`stdio`/`serve` — because in custom_args those are someone
re-issuing the backend's own subcommand. In a launch prefix the same token is
the command's identity, so `hermes-wrapper acp tenant` was being rewritten to
`tenant`. Only a leading `-` now makes a token eligible for the blocklist; a
blocked flag still consumes its separate value so no stray value survives to be
read as a subcommand.

Codex fast mode lost to the prefix. `--disable fast_mode` beats `--enable
fast_mode` regardless of argv order and `-c features.fast_mode=false` beats
config.toml from any position, so prefix-first does not protect an explicitly
selected priority tier. The removal half of enforceCodexFastMode is now a
separate helper applied to the prefix too; the managed enable is still appended
once, on the final args.

Windows CI could not compile the daemon tests: the Command migration left an
unused `path := runtimeCmd.Path` in a hook whose body never referenced it.
Verified with GOOS=windows and GOOS=linux vet, which is what should have caught
it the first time.

Also softens the docs and the Command doc comment: flag-style prefixes usually
parse the same in either position, but a CLI that separates global from
subcommand flags can care, so the runtime docs now name the position change and
tell operators with existing fixed_args to re-run one task after upgrading
instead of promising the move is invisible.

Co-authored-by: multica-agent <[email protected]>

* fix(daemon): resolve and strip the Hermes profile from the real launch argv (MUL-6260)

Second-round review found the Hermes profile chain still reasoning about a
hand-assembled approximation of the command line instead of the command line
itself. The backend launches `<prefix> acp <filtered custom args>`, but the
daemon assembled `<prefix> <custom args>` — no `acp`, no filtering — and that
token participates in Hermes' own argv scan.

With fixed_args `--model` and custom_args `-p research`, the approximation
reads `-p` as `--model`'s value and finds no selection, so the overlay is
seeded from the default home; the real `--model acp -p research` skips
`--model acp` and selects `research`. The selector was then stripped, so the
task stayed inside the overlay — but an overlay built from the wrong profile,
silently.

Stripping had the matching defect, and this one was still an escape rather than
a mismatch. A launch prefix ending in a bare `-p` captures the backend's `acp`
as its profile value, so neither region holds a complete selection and
per-region stripping removes nothing: `-p acp research --yolo` reaches Hermes
intact and re-points HERMES_HOME straight out of the overlay.

Both halves now go through one assembly. agent.HermesLaunchArgv builds exactly
what hermesBackend.Execute builds — the backend calls the same helper, so they
cannot drift — and the daemon resolves from it. StripHermesProfileSelectors
strips against that same argv and hands surviving tokens back to the region
they came from, iterating to a fixed point because removing one selection
promotes the next. The `acp` token is never removed; the backend re-adds it,
and dropping the flag that captured it is what breaks the selection.

StripAllHermesProfileArgs, hermesLaunchArgs and hermesEffectiveArgs are gone:
each was only ever correct on a complete argv, and keeping them would leave the
same footgun for the next caller.

Tests cover the case the review named (prefix ending in a value-taking flag,
custom args opening with a selector), the straddling-selection escape, multiple
occurrences across both regions, and an invalid value left untouched because
Hermes discards it anyway.

Co-authored-by: multica-agent <[email protected]>

---------

Co-authored-by: J <[email protected]>
Co-authored-by: multica-agent <[email protected]>
2026-08-17 15:11:36 +08:00

321 lines
12 KiB
Go

//go:build !windows
package agent
import (
"context"
"encoding/json"
"os"
"path/filepath"
"strings"
"testing"
)
// codebuddyACPSessionResult is the shape CodeBuddy 2.130.0 actually returns from
// `session/new` over `codebuddy --acp`, trimmed to four models. Captured from the
// real CLI: the catalog lives under models.availableModels with an advertised
// currentModelId, and the effort catalog rides along in configOptions as
// thought_level — including the `enabled` choice that is a session toggle rather
// than a valid `--effort` argument.
const codebuddyACPSessionResult = `{"sessionId":"ses-codebuddy","models":{"currentModelId":"hy3",` +
`"availableModels":[` +
`{"modelId":"hy3","name":"Hy3","description":"x0.00 credits"},` +
`{"modelId":"glm-5.2","name":"GLM-5.2","description":"x0.79 credits"},` +
`{"modelId":"kimi-k3-1","name":"Kimi-K3","description":"x1.62 credits"},` +
`{"modelId":"deepseek-v3-2-volc","name":"DeepSeek-V3.2","description":"x0.29 credits"}]},` +
`"configOptions":[` +
`{"type":"select","id":"mode","name":"Permission Mode","currentValue":"default","options":[{"value":"default","name":"Default"}]},` +
`{"type":"select","id":"thought_level","name":"Deep Thinking","category":"thought_level","currentValue":"enabled","options":[` +
`{"value":"minimal","name":"Minimal"},{"value":"low","name":"Low"},{"value":"medium","name":"Medium"},` +
`{"value":"high","name":"High"},{"value":"xhigh","name":"X-High"},{"value":"max","name":"Max"},` +
`{"value":"enabled","name":"On (default)"}]}]}`
// writeCodebuddyACPStub writes an executable stub that speaks just enough ACP for
// discovery. sessionResult is returned from session/new; when it is empty the
// stub fails that call, which is how a not-logged-in CLI is simulated.
func writeCodebuddyACPStub(t *testing.T, sessionResult string) string {
t.Helper()
dir := t.TempDir()
path := filepath.Join(dir, "codebuddy")
sessionReply := `printf '{"jsonrpc":"2.0","id":%s,"result":` + sessionResult + `}\n' "$id"`
if sessionResult == "" {
sessionReply = `printf '{"jsonrpc":"2.0","id":%s,"error":{"code":-32000,"message":"not authenticated"}}\n' "$id"`
}
script := `#!/bin/sh
# Minimal CodeBuddy ACP stub: initialize + session/new only, which is all model
# discovery drives. --version answers so the runtime-registration probe is happy.
case "$1" in
--version) echo '2.130.0'; exit 0 ;;
esac
while IFS= read -r line; do
id=$(printf '%s' "$line" | sed -n 's/.*"id":\([0-9]*\).*/\1/p')
case "$line" in
*'"method":"initialize"'*)
printf '{"jsonrpc":"2.0","id":%s,"result":{"protocolVersion":1,"agentCapabilities":{"loadSession":true},"authMethods":[{"id":"external","name":"Login with Google/Github"}]}}\n' "$id"
;;
*'"method":"session/new"'*)
` + sessionReply + `
exit 0
;;
esac
done
`
if err := os.WriteFile(path, []byte(script), 0o755); err != nil {
t.Fatalf("write codebuddy ACP stub: %v", err)
}
return path
}
func resetCodebuddyDiscoveryCaches(t *testing.T) {
t.Helper()
clear := func() {
modelCacheMu.Lock()
delete(modelCache, "codebuddy")
modelCacheMu.Unlock()
resetThinkingCacheForTests()
}
clear()
t.Cleanup(clear)
}
// TestDiscoverCodebuddyModelsFromACP is the core of the migration (MUL-5549):
// the catalog now comes from the ACP handshake, so IDs, display names AND the
// default model all come from CodeBuddy instead of being guessed from the ID.
func TestDiscoverCodebuddyModelsFromACP(t *testing.T) {
resetCodebuddyDiscoveryCaches(t)
path := writeCodebuddyACPStub(t, codebuddyACPSessionResult)
catalog, err := discoverCodebuddyModels(context.Background(), Command{Path: path})
if err != nil {
t.Fatalf("discoverCodebuddyModels: %v", err)
}
if catalog.Fallback {
t.Fatal("a successful ACP handshake must not be marked Fallback")
}
if len(catalog.Models) != 4 {
t.Fatalf("expected 4 models, got %d: %+v", len(catalog.Models), catalog.Models)
}
// Labels are the CLI's own names. The old --help path had to derive these
// from the ID and got them wrong: kimi-k3-1 became "Kimi K3 1" and
// deepseek-v3-2-volc became "Deepseek V3 2 Volc".
wantLabel := map[string]string{
"hy3": "Hy3",
"glm-5.2": "GLM-5.2",
"kimi-k3-1": "Kimi-K3",
"deepseek-v3-2-volc": "DeepSeek-V3.2",
}
for _, m := range catalog.Models {
if want, ok := wantLabel[m.ID]; !ok {
t.Errorf("unexpected model %q", m.ID)
} else if m.Label != want {
t.Errorf("label(%q) = %q, want the CLI's own %q", m.ID, m.Label, want)
}
}
// The default is the advertised currentModelId, not "whichever came first".
var defaults []string
for _, m := range catalog.Models {
if m.Default {
defaults = append(defaults, m.ID)
}
}
if len(defaults) != 1 || defaults[0] != "hy3" {
t.Errorf("default models = %v, want exactly [hy3] from currentModelId", defaults)
}
// The ACP payload has no vendor field and CodeBuddy's ids are bare, so
// without the prefix post-pass every model would land in one unlabelled
// group instead of the picker's per-vendor sections.
wantProvider := map[string]string{
"hy3": "hunyuan",
"glm-5.2": "zhipu",
"kimi-k3-1": "kimi",
"deepseek-v3-2-volc": "deepseek",
}
for _, m := range catalog.Models {
if want := wantProvider[m.ID]; m.Provider != want {
t.Errorf("provider(%q) = %q, want %q — the picker groups on this", m.ID, m.Provider, want)
}
}
}
// TestCodebuddyModelProviderCoversRealCatalog pins vendor inference against every
// ID shape CodeBuddy 2.130.0 actually advertises. A miss here is invisible in the
// backend but collapses the picker into one unlabelled list.
func TestCodebuddyModelProviderCoversRealCatalog(t *testing.T) {
t.Parallel()
for id, want := range map[string]string{
// Live ACP catalog, all 16 ids.
"hy3": "hunyuan", "glm-5.2": "zhipu", "glm-5.1": "zhipu", "glm-5.0": "zhipu",
"glm-5.0-turbo": "zhipu", "glm-5v-turbo": "zhipu", "glm-4.7": "zhipu",
"minimax-m3": "minimax", "minimax-m2.7": "minimax",
"kimi-k3-1": "kimi", "kimi-k2.7": "kimi", "kimi-k2.6": "kimi", "kimi-k2.5": "kimi",
"deepseek-v4-pro": "deepseek", "deepseek-v4-flash": "deepseek", "deepseek-v3-2-volc": "deepseek",
// Static fallback ids, which must group too.
"claude-sonnet-4.6": "anthropic", "claude-opus-4.7": "anthropic",
"gemini-3.1-pro": "google", "gpt-5.5": "openai",
"deepseek-v3-2-volc-ioa": "deepseek",
} {
if got := codebuddyModelProvider(id); got != want {
t.Errorf("codebuddyModelProvider(%q) = %q, want %q", id, got, want)
}
}
}
// TestCodebuddyStaticModelsCarryProviders guards the fallback path's grouping:
// those entries hardcode Provider rather than going through the post-pass, so a
// new entry added without one would silently break grouping there instead.
func TestCodebuddyStaticModelsCarryProviders(t *testing.T) {
t.Parallel()
for _, m := range codebuddyStaticModels() {
if m.Provider == "" {
t.Errorf("static fallback model %q has no Provider; the picker would not group it", m.ID)
}
if want := codebuddyModelProvider(m.ID); m.Provider != want {
t.Errorf("static fallback %q has Provider %q but prefix inference says %q", m.ID, m.Provider, want)
}
}
}
// TestDiscoverCodebuddyModelsACPEffort pins the effort catalog coming out of the
// same handshake — no second process — and the `enabled` filtering.
func TestDiscoverCodebuddyModelsACPEffort(t *testing.T) {
resetCodebuddyDiscoveryCaches(t)
path := writeCodebuddyACPStub(t, codebuddyACPSessionResult)
catalog, err := discoverCodebuddyModels(context.Background(), Command{Path: path})
if err != nil {
t.Fatalf("discoverCodebuddyModels: %v", err)
}
thinking := catalog.Models[0].Thinking
if thinking == nil {
t.Fatal("expected the effort catalog to be annotated from the same session/new response")
}
var got []string
for _, lvl := range thinking.SupportedLevels {
got = append(got, lvl.Value)
}
want := []string{"minimal", "low", "medium", "high", "xhigh", "max"}
if strings.Join(got, ",") != strings.Join(want, ",") {
t.Errorf("levels = %v, want %v", got, want)
}
// `enabled` is advertised by ACP but `--effort enabled` is not a valid
// command line, so it must not reach the picker...
for _, lvl := range thinking.SupportedLevels {
if lvl.Value == "enabled" {
t.Error("`enabled` is a session toggle, not an --effort value; it must be filtered out")
}
}
// ...and since it is the advertised currentValue, DefaultLevel must stay
// empty ("runtime decides") rather than echo a value we cannot pass.
if thinking.DefaultLevel != "" {
t.Errorf("DefaultLevel = %q, want empty because currentValue was the unusable `enabled`", thinking.DefaultLevel)
}
}
// TestParseACPCodebuddyEffortDefault covers the other branch: when CodeBuddy
// advertises a real level as current, we echo it.
func TestParseACPCodebuddyEffortDefault(t *testing.T) {
raw := json.RawMessage(`{"configOptions":[{"id":"thought_level","currentValue":"high","options":[` +
`{"value":"low"},{"value":"high"},{"value":"enabled"}]}]}`)
levels, def := parseACPCodebuddyEffort(raw)
if strings.Join(levels, ",") != "low,high" {
t.Errorf("levels = %v, want [low high]", levels)
}
if def != "high" {
t.Errorf("DefaultLevel = %q, want high", def)
}
}
// TestDiscoverCodebuddyModelsFallsBackOnACPFailure covers the not-logged-in /
// unreachable-CLI cases. The stand-in is still offered so the picker stays
// usable, but it must be marked Fallback so the server can never cache it as
// this runtime's real catalog (MUL-5549), and the effort picker must still work.
func TestDiscoverCodebuddyModelsFallsBackOnACPFailure(t *testing.T) {
for _, tc := range []struct {
name string
path func(t *testing.T) string
}{
{
name: "binary missing",
path: func(t *testing.T) string { return missingAgentExecutable(t, "codebuddy") },
},
{
name: "session/new refused (not logged in)",
path: func(t *testing.T) string { return writeCodebuddyACPStub(t, "") },
},
{
name: "session/new carries no catalog",
path: func(t *testing.T) string { return writeCodebuddyACPStub(t, `{"sessionId":"ses-empty"}`) },
},
} {
t.Run(tc.name, func(t *testing.T) {
resetCodebuddyDiscoveryCaches(t)
catalog, err := discoverCodebuddyModels(context.Background(), Command{Path: tc.path(t)})
if err != nil {
t.Fatalf("discoverCodebuddyModels: %v", err)
}
if !catalog.Fallback {
t.Error("a degraded discovery must be marked Fallback")
}
if len(catalog.Models) == 0 {
t.Error("expected the static stand-in to still be offered to the UI")
}
if catalog.Models[0].Thinking == nil || len(catalog.Models[0].Thinking.SupportedLevels) == 0 {
t.Error("the fallback path must still annotate effort levels")
}
})
}
}
// TestCodebuddyStaticEffortFallbackCoversFlagValues keeps the offline fallback
// honest against the flag it feeds: every level offered must be one `--effort`
// accepts, and the set should not silently shrink below what the CLI supports.
func TestCodebuddyStaticEffortFallbackCoversFlagValues(t *testing.T) {
t.Parallel()
for _, level := range codebuddyStaticEffortFallback {
if !codebuddyFlagEffortValues[level] {
t.Errorf("static fallback offers %q, which `--effort` does not accept", level)
}
if !IsKnownThinkingValue("codebuddy", level) {
t.Errorf("static fallback offers %q, which the server-side gate rejects", level)
}
}
if len(codebuddyStaticEffortFallback) != len(codebuddyFlagEffortValues) {
t.Errorf("static fallback has %d levels but --effort accepts %d; they drifted apart",
len(codebuddyStaticEffortFallback), len(codebuddyFlagEffortValues))
}
}
// TestCachedDiscoveryDoesNotCacheFallback pins that a fallback never occupies
// the daemon's 60s discovery cache: the next request must be free to retry.
func TestCachedDiscoveryDoesNotCacheFallback(t *testing.T) {
const key = "test-cache-fallback"
reset := func() {
modelCacheMu.Lock()
delete(modelCache, key)
modelCacheMu.Unlock()
}
reset()
t.Cleanup(reset)
calls := 0
fn := func() (Catalog, error) {
calls++
return Catalog{Models: []Model{{ID: "stand-in"}}, Fallback: true}, nil
}
for i := 0; i < 2; i++ {
got, err := cachedDiscovery(key, fn)
if err != nil {
t.Fatalf("cachedDiscovery: %v", err)
}
if !got.Fallback {
t.Error("cachedDiscovery must preserve the Fallback marker")
}
}
if calls != 2 {
t.Fatalf("a fallback must not be cached: expected fn called 2x, got %d", calls)
}
}