Conversation
Based on the diagnosis and proposed read-side fix from @pwguler. Fixes earendil-works#9243
|
Only now did I realize pwguler even offered to produce a fix, happy to have this one closed and give credit to them. Most of it is their work. |
|
Not a maintainer, but I dug through the history for your open question: the overwrite behavior traces to 0faadfc (dec 22, "Fix session-manager simplification issues"). Before that, the model lived in the SessionHeader — set explicitly via /model, and assistant messages never wrote back to it. The refactor swapped header state for a linear entry scan, and messages started winning purely from the else-if ordering in the new loop; the test asserting the overwrite landed in that same commit. So it reads like a description of the refactor's side effect rather than an intentional router/fallback design. Your call matches the pre-refactor semantics (explicit selection wins), which imo is also the safer behavior — a mid-stream fallback would otherwise silently change the model you resume with, and echoed ids like |
gaoanze888
left a comment
There was a problem hiding this comment.
Reviewed exact head ab280ffebd. The read-side precedence matches the provenance of these fields: model_change records Pi's selected catalog/routing ID, while an assistant message carries the provider's response echo and may legitimately differ (namespaced relays and the built-in Copilot example in #9243). Sessions without a model_change retain the latest-assistant fallback.
I also checked branch isolation, compaction, old/no-change sessions, multiple explicit changes, session switching, and fork/resume construction. Normal session creation plus every explicit set/cycle path records a model change, and resetLeaf() paths correctly lose entries outside the selected ancestry rather than leaking another branch's model.
Validation on this head:
- clean cherry-pick onto current
origin/main(71dca871b) - focused build-context, compaction, and runtime restore suites: 54 passed, 2 skipped
npm run buildnpm run check- clean worktree and
git diff --check
No blocker found.
This comment is AI-generated by /wr
gaoanze888
left a comment
There was a problem hiding this comment.
I need to retract my approval after an independent lifecycle pass found a concrete regression that the focused precedence tests do not cover.
When an existing session is reopened with an explicit model, createAgentSession() starts the runtime with that model but does not append a new model_change: the append currently exists only in the new-session branch. With this PR's precedence, the old model_change remains permanently authoritative.
Minimal lifecycle:
- Existing session contains
model_change = anthropic/claude-sonnet-4-5and messages. - Reopen it with explicit
model: openai/gpt-5.4(the SDK equivalent of a CLI model override). - The live session correctly uses
gpt-5.4, butsessionManager.buildSessionContext().modelstill returnsanthropic/claude-sonnet-4-5. - A later ordinary resume therefore returns to the old model, even after responses were generated with the explicit model.
I reproduced this on exact head with a real createAgentSession() test; the final assertion expected { provider: "openai", modelId: "gpt-5.4" } and received { provider: "anthropic", modelId: "claude-sonnet-4-5" }.
The same persistence gap appears when a saved model cannot be restored and startup selects a fallback: unless that effective selection is recorded, each future resume keeps retrying the stale authoritative model_change.
Please persist a model_change for an existing session when the effective startup model differs from the selected branch's recorded model, and add lifecycle regressions for both:
- existing session + explicit model override;
- unavailable saved model + fallback, followed by another resume.
The core proxy-echo fix remains correct; this is specifically about ensuring the newly authoritative record stays synchronized with startup model selection. Apologies for approving before checking this lifecycle.
This comment is AI-generated by /wr
Based on the diagnosis and proposed read-side fix from @pwguler
Prefers last model_change (there's one at least in beginning of each session since a commit quite a while ago) over last assistant message's model.
Kept the fallback to message's model for rare cases without
mode_change(pre- 98c85bf sessions and possibly sessions created by e.g.resetLeaf.It is worth noting, this change needs a change of rather specific test
should track model and thinking level changeswhich used to explictely test a subsequent change by model's response changes the model. It was introduced with 0faadfc (dec 26, Mario) which was prior to seeding each session start with model_change but the test make it seem even during_session changes were to be taken into account.Making executive decision here to ignore that, but might be worth asking him what's the intention.
(I coudln't think of any good reason, I thought maybe it was to handle routers better or fallbacks but I coudln't come up with anything concrete)
which
Fixes #9243