fix: infer provider from model for long-horizon agent sessions - #2264
fix: infer provider from model for long-horizon agent sessions#2264lsm wants to merge 2 commits into
Conversation
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (GLM)
Model: glm-5.1[1m] | Client: NeoKai | Provider: GLM
Recommendation: APPROVE — correct, minimal, and well-tested. Zero P0-P3 findings.
Note: posted as
COMMENTbecause the PR author and reviewer share the same GitHub account (GitHub rejectsAPPROVE/REQUEST_CHANGESfrom the author). The substantive recommendation is APPROVE.
Scope reviewed
model-switch-handler.ts:175-182— guard now inferspreviousProviderfrom the stored model instead of hard-throwing.space-runtime-service.ts:509-554(buildLongHorizonAgentSessionConfig) — infers provider from model whenagent.provideris unset.space-runtime-service.ts:657-661(regular worker-agent branch) — sameagent.provider ?? inferProviderForModel(model)fallback.- Regression tests in both
model-switch-handler.test.tsandspace-runtime-service.test.ts.
Correctness
- The primary fix mirrors the already-correct worker pattern in
custom-agent.ts:642-644.inferProviderForModel('kimi-for-coding')→'kimi'(the registry strips the[1m]suffix and routeskimi-*/moonshot-*/k3to kimi), so the report's exact case resolves correctly. modelis computed first, thenproviderderives from it, so inference always sees the resolved model — including theDEFAULT_LONG_HORIZON_AGENT_MODELfallthrough, which now correctly yieldsprovider='anthropic'instead ofundefined.- In
model-switch-handler, the "already using this model" check at line 222 compares the rawsession.config.provideragainstnewProvider, so switching an unprovisioned kimi session to the same model proceeds (and repairs the provider) rather than no-op'ing — matching the report's requirement that kimi→kimi succeed. The downstreamresolveModelAlias(..., previousProvider)and acp checks only become more correct with the inferred value.
Backwards compatibility / regression
- Explicit
agent.provideralways wins via??short-circuit; for sessions that already had a provider stored,inferProviderForModelis never even computed. Covered by explicit-wins tests for both builders and for the switch guard. - Self-heal is one-shot:
refreshLongHorizonAgentSessionConfigonly callsupdateConfig+resetQuerywhen the config actually changed, and a no-op wake is explicitly tested. No data migration needed.
Coverage
- Config-builder inference: kimi, non-kimi (anthropic), explicit-wins, default-fallback.
- Refresh:
undefined → kimiself-heal, and no-op when already matching. - Regular worker branch: inference + explicit-wins.
- Switch guard: empty-provider infers from model (glm + kimi), the kimi→kimi same-model regression from the report, and the "no provider AND no model" case still throws.
- Both changed test files pass locally (32/0 and 69/0).
Completeness
A sweep of every provider: assignment sourced from a .provider field across packages/daemon/src confirms no other Session['config'] producer reachable by switchModel is missing the inference — session-lifecycle.ts resolves via getValidatedModelId, custom-agent.ts already infers, and the remaining sites are RPC/DTO/repository plumbing, not session configs.
No changes requested.
|
@codex review Retriggering a fresh Codex review for the current head ( |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
A LH/worker agent whose settings set `model` but left `provider` null ran fine (provider inferred at query time) but was hard-blocked from switching models with "Session has no provider configured" — even to the same model. buildLongHorizonAgentSessionConfig and the regular long-term-agent branch now infer the provider from the model (mirroring custom-agent.ts) when agent.provider is unset. model-switch-handler additionally infers the previous provider from the stored model instead of throwing, so stranded sessions can switch immediately instead of waiting for a wake/refresh.
…ching Covers buildLongHorizonAgentSessionConfig inference (kimi + non-kimi + explicit-provider-wins + default fallback), the regular worker-agent branch, refreshLongHorizonAgentSessionConfig self-heal (undefined → kimi), and the model-switch-handler guard inferring the provider from the stored model — including the kimi → kimi same-model case from the bug report.
bf5c497 to
121e1c5
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Switching the model on a long-horizon (LH) agent threw
Session has no provider configuredbecause the LH/regular session config builders setmodelbut leftproviderundefined, whilemodel-switch-handlerhard-threw before checking the stored model.This infers the provider from the model (mirroring
custom-agent.ts) in both config builders whenagent.provideris unset, and hardens the switch guard to infer the previous provider from the stored model instead of throwing — so existing stranded sessions self-heal on the next wake.buildLongHorizonAgentSessionConfig+ regular worker-agent branch:agent.provider ?? inferProviderForModel(model)model-switch-handler:previousProvider = session.config.provider ?? inferProviderForModel(session.config.model)undefined → kimi), and the switch guard (incl. the kimi → kimi same-model case from the report)