mirror of
https://gh-proxy.org/https://github.com/klinsmaya/multica.git
synced 2026-11-09 09:56:47 +08:00
3df7c38ed1
* fix(agent): normalise the provider prefix in the Hermes set_model skip-gate The gate added in #5690 (MUL-5029) compares the configured model against the model the session reports as current with a raw `==`: if opts.Model != "" && effectiveModel == sessionCurrentModel { Those two values come from different id namespaces, so for an agent whose `model` is stored bare the comparison can never be true. The gate is dead code for those agents and `session/set_model` is replayed on every turn — which is the exact situation #5690 was opened to prevent. Why the two sides cannot match: - Hermes always reports a provider-encoded id. `_encode_model_choice` in its ACP adapter returns `f"{provider}:{model}"` whenever a provider resolves, and the provider falls back to `detect_provider()` or `"openrouter"`, so it is effectively never absent. Confirmed against a live `hermes acp` session: session/new populates `models.currentModelId` from that encoder, so a bare configured `claude-sonnet-4-5` on a `custom` provider is reported back as `custom:claude-sonnet-4-5`. - `agent.model` is persisted verbatim. `handler/agent.go` writes `req.Model` straight into the column on create and update; nothing normalises or validates a `provider:` prefix on that path. - The bare spelling is a documented, first-class input. `cmd_agent.go`'s own flag help is `--model Model identifier (e.g. claude-sonnet-4-6, openai/gpt-4o)`. Only the ACP-catalog dropdown produces the prefixed form, because `acpModelEntry` uses the advertised id as `Model.ID`. So dropdown-configured agents match and skip; every agent configured through the CLI, the API, a seed, or a row predating the catalog does not, and never will. What the replay costs, per turn, in the Hermes runtime: `set_session_model` replaces the session's agent with `_make_agent`, which re-runs `load_config` and `resolve_runtime_provider`, joins background MCP discovery for up to `mcp_discovery_timeout` (~1.5s), and constructs a fresh agent whose system prompt is then re-derived from disk. It also re-runs `_resolve_model_selection`, i.e. the provider auto-detection this gate exists to avoid — so the `custom:deepseek-v4-pro` mis-route from #5690 is still live for precisely the agents that hit it. Running Hermes' own resolver over the ids this daemon sends reproduces it: `custom` + `deepseek-v4-pro` resolves to `openrouter`, and `anthropic` + a bare Claude id resolves away from `anthropic`, both of which make `_make_agent` drop base_url and api_mode. The fix replaces the raw comparison with `acpModelIDsEquivalent`, which normalises the optional prefix before comparing: - both sides carry an explicit provider → compare the whole id, so a genuine provider switch (openrouter:X → custom:X) still sends set_model; - exactly one side carries a provider → compare model names only; a bare configured id expresses no provider preference, so the session's own provider is authoritative; - either side empty → not equivalent, preserving the existing fall-through for runtimes that report no current model. Only the first colon separates provider from model, so slash-bearing OpenRouter-style names (`nous:moonshotai/kimi-k2.6`) split correctly, and the comparison is case-insensitive because Hermes lowercases the provider when it encodes the id. No migration and no operator action. Rows already written in the prefixed form keep working — both spellings now resolve to the same selection. Deliberately not argued on cost. A cache-hit-rate difference first pointed here, but that measurement was grouped by "does the agent have a configured model", which on the measured deployment was confounded with the transport wire, so this change does not claim it. It stands on the comparison being unsatisfiable and on the guard it restores. Tests: 12 `acpModelIDsEquivalent` cases, including the provider-switch and different-model negatives, the case-folding and whitespace cases, and the slash-bearing model id; plus 6 `splitACPModelID` cases covering the leading-colon and multi-colon shapes. * fix(agent): only skip set_model when an explicit provider is confirmed Review caught a regression introduced by the previous commit: the third equivalence rule was symmetric, but the risk is not. acpModelIDsEquivalent("openrouter:hermes-4", "hermes-4") // => true Comparing model names whenever *either* side lacked a provider meant a caller who named a provider explicitly got the switch swallowed, silently, and the session kept whatever provider it had defaulted to. `origin/main` sends set_model for that input, so the previous commit changed behaviour for it. The premise that justified the symmetry — "Hermes always reports a provider-encoded id" — is false, and this repository already contained the counterexamples: `acp_effort_test.go` captures `currentModelId: "hermes-4"` from `hermes acp` v0.20.0 and `currentModelId: "gpt-5.6-sol"` from jcode, both unprefixed, and jcode routes through this same backend (hermes.go states it covers both binaries). `_encode_model_choice` returning a prefix is what happens when a provider resolves; it is not a property of every input. The rule is now asymmetric: - bare configured id -> equivalent on the model name alone. It expresses no provider preference, so the session's provider is authoritative. This is the case the gate exists for, and it still skips. - explicit configured provider -> equivalent only when the session reports that same provider. A bare current id cannot confirm the match, so we fall through and send set_model, as before this change. Tests: - three `acpModelIDsEquivalent` table entries, including the missing `provider:model` vs bare-id row that let the regression through; - `TestHermesSkipsRedundantSetModelForBareConfiguredModel`, an end-to-end frame-recording test that asserts the gate actually fires for the bare spelling — the unit tests never checked that set_model stopped going out; - `TestHermesSendsSetModelWhenExplicitProviderAndBareCurrent`, which fails without the asymmetric rule (verified: the recorded frame is absent). Also addressed from the same review: - dropped the stale cache-hit-rate paragraph from the `acpModelIDsEquivalent` comment. The PR description claimed it had been removed; it had not. - dropped the paragraph arguing the benefit via MCP refresh and tool-cache prefix invalidation. It described Hermes' Python internals this repository cannot verify or track, and it read as arguing against the sentence right after it. - softened "the comparison can never be true": it holds when the runtime reports a prefixed id, which is not always. - noted that `splitACPModelID` cannot distinguish a provider prefix from a colon inside a bare model name (`llama3:8b`), and that the degradation is safe in the only direction that matters — it falls back to sending set_model, never to a false skip. * docs(agent): correct the bare-current-id evidence in the set_model gate The justification for the asymmetric equivalence rule cited acp_effort_test.go as capturing unprefixed model ids from both `hermes acp` and jcode. Only the jcode half holds. Verified against a live `hermes acp` v0.20.0 — the same version the fixture names — driving `initialize` + `session/new`, the two calls this backend makes: `models.currentModelId` came back provider-encoded in all three cases (no model param, a bare model param, and a provider-encoded one), and all nine availableModels ids were prefixed too. Sending a bare `gpt-5.6-terra` returned `openai-codex:gpt-5.6-terra`, which is the defect this gate fixes and is now first-hand rather than inferred. Hermes Agent's encoder does have a bare branch — acp_adapter/server.py `_encode_model_choice` returns the model alone when the provider is empty, confirmed by calling it directly — but `_build_model_state` defaults the provider (`state.agent.provider or detect_provider() or "openrouter"`) and `normalize_provider("")` falls back to `openrouter`, so a normal install never reaches it. The `hermes-4` in that fixture is better read as a simplification made while writing a test about configOptions, not a verbatim capture. The rule itself is unchanged and still correct: this backend serves Hermes Agent and jcode, jcode really does report unprefixed ids, and refusing to confirm an explicit provider against a bare current id costs nothing. Co-authored-by: multica-agent <[email protected]> --------- Co-authored-by: Garfield <[email protected]> Co-authored-by: Bohan-J <[email protected]> Co-authored-by: multica-agent <[email protected]>