mirror of
https://gh-proxy.org/https://github.com/klinsmaya/multica.git
synced 2026-11-09 09:56:47 +08:00
3b08423684
* 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]>
72 lines
2.8 KiB
Go
72 lines
2.8 KiB
Go
package agent
|
|
|
|
import "testing"
|
|
|
|
// TestDiscoveryCacheKeyScopesByExecutable pins the key shape: an empty path
|
|
// keeps the bare provider key (what a built-in on PATH resolves to), and two
|
|
// executables of the same protocol family never collapse onto one key.
|
|
func TestDiscoveryCacheKeyScopesByExecutable(t *testing.T) {
|
|
if got, want := discoveryCacheKey("hermes", Command{}), "hermes"; got != want {
|
|
t.Fatalf("discoveryCacheKey(hermes, \"\") = %q, want %q", got, want)
|
|
}
|
|
builtin := discoveryCacheKey("hermes", Command{Path: "/usr/local/bin/hermes"})
|
|
custom := discoveryCacheKey("hermes", Command{Path: "/usr/local/bin/jcode"})
|
|
if builtin == custom {
|
|
t.Fatalf("same key %q for two different executables", builtin)
|
|
}
|
|
if builtin == "hermes" || custom == "hermes" {
|
|
t.Fatalf("a path-scoped key must differ from the bare provider key: %q / %q", builtin, custom)
|
|
}
|
|
}
|
|
|
|
// TestCachedDiscoveryDoesNotShareMemoAcrossExecutables is the reason the key
|
|
// carries the path (MUL-5789). A host can run a built-in runtime and a custom
|
|
// runtime profile of the same protocol family at once — the reporter's own
|
|
// `MULTICA_HERMES_PATH` workaround produces exactly that pair. With a
|
|
// provider-only key the first discovery to land would answer for both for the
|
|
// full TTL, reintroducing the catalog/binary mismatch the daemon-side fix
|
|
// removes.
|
|
func TestCachedDiscoveryDoesNotShareMemoAcrossExecutables(t *testing.T) {
|
|
builtinKey := discoveryCacheKey("hermes", Command{Path: "/usr/local/bin/hermes"})
|
|
customKey := discoveryCacheKey("hermes", Command{Path: "/usr/local/bin/jcode"})
|
|
reset := func() {
|
|
modelCacheMu.Lock()
|
|
delete(modelCache, builtinKey)
|
|
delete(modelCache, customKey)
|
|
modelCacheMu.Unlock()
|
|
}
|
|
reset()
|
|
t.Cleanup(reset)
|
|
|
|
discover := func(id string, calls *int) func() (Catalog, error) {
|
|
return func() (Catalog, error) {
|
|
*calls++
|
|
return Catalog{Models: []Model{{ID: id}}}, nil
|
|
}
|
|
}
|
|
|
|
var builtinCalls, customCalls int
|
|
if _, err := cachedDiscovery(builtinKey, discover("hermes-model", &builtinCalls)); err != nil {
|
|
t.Fatalf("cachedDiscovery(builtin): %v", err)
|
|
}
|
|
|
|
got, err := cachedDiscovery(customKey, discover("jcode-model", &customCalls))
|
|
if err != nil {
|
|
t.Fatalf("cachedDiscovery(custom): %v", err)
|
|
}
|
|
if customCalls != 1 {
|
|
t.Fatalf("custom executable must run its own discovery, got %d calls", customCalls)
|
|
}
|
|
if len(got.Models) != 1 || got.Models[0].ID != "jcode-model" {
|
|
t.Fatalf("custom key served the built-in catalog: %+v", got.Models)
|
|
}
|
|
|
|
// The built-in entry is still memoized under its own key.
|
|
if _, err := cachedDiscovery(builtinKey, discover("hermes-model", &builtinCalls)); err != nil {
|
|
t.Fatalf("cachedDiscovery(builtin, second): %v", err)
|
|
}
|
|
if builtinCalls != 1 {
|
|
t.Fatalf("built-in discovery should have been served from cache, got %d calls", builtinCalls)
|
|
}
|
|
}
|