Skip to content

fix(agents): sync stale this.model snapshot after /model switch#93056

Open
samson910022 wants to merge 6 commits into
openclaw:mainfrom
samson910022:fix/stale-model-after-switch-92415
Open

fix(agents): sync stale this.model snapshot after /model switch#93056
samson910022 wants to merge 6 commits into
openclaw:mainfrom
samson910022:fix/stale-model-after-switch-92415

Conversation

@samson910022

@samson910022 samson910022 commented Jun 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

  • Fixes the stale AgentSession.this.model snapshot after a session-level /model switch.
  • Keeps long-lived prompt/status reads aligned with the persisted session-store selection.
  • Marks Telegram /model writes as pending so the session can resync on the next read.
  • Threads the canonical session store path into embedded session creation so the sync helper reads the right file.

Linked Context

Closes #92415

Related #92388
Related #92424
Related #95353

Evidence

  • Behavior or issue addressed: After a cross-provider model switch, the next user prompt and status reads could still use the stale in-memory model snapshot.
  • Real environment tested: Windows 11 24H2 (x64), Node.js v24.14.1
  • Exact steps or command run after this patch:
    • node scripts/run-vitest.mjs src/agents/sessions/agent-session.model-sync.test.ts
    • node scripts/run-vitest.mjs src/agents/embedded-agent-runner/run/attempt.spawn-workspace.resource-loader.test.ts
    • node scripts/run-vitest.mjs src/agents/embedded-agent-runner/run/attempt.spawn-workspace.context-engine.test.ts
  • Evidence after fix: All three targeted Vitest shards passed, including the embedded-runner bridge test and the model-sync regression tests.
  • Observed result after fix: The session model syncs from the canonical session store before prompt/status reads.
  • What was not tested: Live daemon/provider switching with external credentials.
  • Proof limitations or environment constraints: The live OpenClaw daemon path was not exercised here; the proof is focused on the changed code paths plus targeted regression coverage.

Tests and Validation

  • Added regression coverage for prompt-time model resync and getSessionStats() / context usage reads.
  • Added bridge coverage for embedded session construction forwarding storePath and sessionKey.
  • Added a real ModelRegistry auth-backed path without mocking hasConfiguredAuth().
  • The fix failed before when the session store path and bridge coverage were incomplete.

Risk Checklist

  • Did user-visible behavior change? Yes
  • Did config, environment, or migration behavior change? No
  • Did security, auth, secrets, network, or tool execution behavior change? Yes
  • What is the highest-risk area? Session-store sync timing and auth-gated model resolution.
  • How is that risk mitigated? Focused regression tests cover the canonical store lookup, embedded runner bridge, and auth-backed model selection.

Current Review State

  • What is the next action? Re-run review after the PR body fix.
  • What is still waiting on author, maintainer, CI, or external proof? ClawSweeper re-review.
  • Which bot or reviewer comments were addressed? The earlier review findings about store-path wiring, pending flags, and prompt/status freshness.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: L triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 14, 2026
@clawsweeper

clawsweeper Bot commented Jun 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 2:44 PM ET / 18:44 UTC.

Summary
The PR adds AgentSession session-store model resync around prompt/status reads, forwards storePath/sessionKey into embedded sessions, marks Telegram model-picker writes pending, and adds regression tests.

PR surface: Source +116, Tests +718. Total +834 across 10 files.

Reproducibility: yes. from source inspection, but not from a live run in this read-only review; current main can persist model switches while AgentSession prompt/status reads keep using the in-memory this.model snapshot.

Review metrics: 3 noteworthy metrics.

  • Session construction options: 2 optional fields added. storePath and sessionKey now control whether AgentSession can read persisted model switch state.
  • Runtime sync read sites: 2 read sites added. The helper can mutate active model state during prompt handling and status/context reads, which is central to the runtime risk.
  • Telegram model-picker write: 1 write path changed. The visible Telegram picker now marks selections pending, making that channel path a primary proof target.

Stored data model
Persistent data-model change detected: serialized state: src/agents/sessions/agent-session-services.ts, serialized state: src/agents/sessions/agent-session.model-sync.test.ts, serialized state: src/agents/sessions/agent-session.ts, serialized state: src/agents/sessions/sdk.ts, unknown-data-model-change: src/agents/sessions/agent-session.model-sync.test.ts, unknown-data-model-change: src/agents/sessions/agent-session.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #92415
Summary: This PR is the active candidate fix for the canonical stale AgentSession model snapshot issue; older related PRs were closed unmerged partial attempts, and #97517 overlaps without replacing this fix candidate.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Remove the duplicate CURRENT_SESSION_VERSION import so type checks can run cleanly.
  • [P1] Add redacted CLI, daemon, or Telegram proof for explicit and default /model switches followed by prompt/status reads.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body supplies targeted Vitest validation only and explicitly says live daemon/provider switching was not exercised, so contributor real behavior proof is still needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A native Telegram proof would materially verify that the picker sets pending state and that the next prompt/status reflects explicit and default selections. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis telegram desktop proof: switch /model to another provider and back to default, then verify a follow-up prompt and /status use the selected model.

Risk before merge

  • [P1] Current head does not type-check because CURRENT_SESSION_VERSION is imported twice in AgentSession.
  • [P1] The contributor proof is still targeted-test only; no redacted CLI, daemon, or Telegram run shows a real /model switch followed by prompt/status reads using the selected model.
  • [P1] The diff changes how a running AgentSession mutates in-memory model state from persisted session-store entries, so a wrong store/key or provider/model sync would affect session state and auth/provider routing.

Maintainer options:

  1. Fix type check and require live model-switch proof (recommended)
    Have the contributor remove the duplicate import and add redacted real CLI, daemon, or Telegram proof for explicit and default model switches before merge.
  2. Maintainer supplies live validation
    A maintainer can provide the missing live proof themselves after the type error is fixed, then accept the remaining session/auth routing risk.
  3. Pause or replace the branch
    If live proof remains unavailable or the branch keeps accumulating CI-only churn, keep Session-level AgentSession.this.model snapshot is never refreshed after /model switch (affects contextWindow, reasoning, thinkingLevelMap, branch summary) #92415 open and replace this with a maintainer-proven fix path.

Next step before merge

  • [P1] Contributor action is needed for both the type-check failure and real behavior proof; automation should not take over because it cannot supply the contributor's real setup proof.

Security
Cleared: No concrete supply-chain, workflow, dependency, downloaded-code, or secret-handling concern was found; provider/auth effects remain functional merge risk.

Review findings

  • [P2] Remove the duplicate session version import — src/agents/sessions/agent-session.ts:21
Review details

Best possible solution:

Remove the duplicate import, then land this or a narrow replacement only after redacted CLI/daemon or Telegram proof shows explicit and default /model switches updating prompt/status/session state on current head.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection, but not from a live run in this read-only review; current main can persist model switches while AgentSession prompt/status reads keep using the in-memory this.model snapshot.

Is this the best way to solve the issue?

No, not mergeable yet; the implementation direction is plausible and bounded, but the duplicate import and missing real behavior proof must be cleared first.

Full review comments:

  • [P2] Remove the duplicate session version import — src/agents/sessions/agent-session.ts:21
    Current head imports CURRENT_SESSION_VERSION at both lines 21 and 31, and the current check-prod-types/check-test-types jobs fail with TS2300 on those identifiers. Remove one import so AgentSession compiles before merge.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.95

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 3f380635f59b.

Label changes

Label justifications:

  • P1: The PR targets an active model-switch regression that can affect session state, context decisions, and provider/model routing for real users.
  • merge-risk: 🚨 session-state: The diff changes how a running AgentSession updates its in-memory model from persisted session-store state.
  • merge-risk: 🚨 auth-provider: The synchronized model controls provider/model selection before auth-gated runtime behavior.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body supplies targeted Vitest validation only and explicitly says live daemon/provider switching was not exercised, so contributor real behavior proof is still needed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The PR changes Telegram /model picker persistence and the visible follow-up status/prompt behavior can be shown in a short Telegram Desktop proof.
Evidence reviewed

PR surface:

Source +116, Tests +718. Total +834 across 10 files.

View PR surface stats
Area Files Added Removed Net
Source 7 116 0 +116
Tests 3 719 1 +718
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 10 835 1 +834

Acceptance criteria:

  • [P1] check-prod-types and check-test-types pass after the duplicate import is removed.
  • [P1] PR body includes redacted real CLI, daemon, or Telegram proof for explicit and default /model switches followed by prompt/status reads.

What I checked:

Likely related people:

  • steipete: Current blame for AgentSession prompt handling, model override pending-state code, and the Telegram picker path points to 2bc50d0656bd3c5905f0d972ff429c8388c43b9e. (role: recent area contributor; confidence: medium; commits: 2bc50d0656bd, 3f380635f59b; files: src/agents/sessions/agent-session.ts, src/sessions/model-overrides.ts, extensions/telegram/src/bot-handlers.runtime.ts)
  • vincentkoc: Recent history on the Telegram model-selection path includes aa017bf9ddfa0d2e583b1addcfaf5e8c2db4eea8 and a large adjacent session/model stabilization commit e085fa1a3ffd32d0ea6917e1e6fb4ecbffbb77d2. (role: adjacent area contributor; confidence: medium; commits: aa017bf9ddfa, e085fa1a3ffd; files: extensions/telegram/src/bot-handlers.runtime.ts, src/agents/sessions/agent-session.ts, src/sessions/model-overrides.ts)
  • Frank Yang: History search shows 5d46e4dc4f458d3ef2097c881eab3a78f2abee2b touched session reset/model override behavior in the same session-state area. (role: adjacent session-state contributor; confidence: low; commits: 5d46e4dc4f45; files: src/agents/sessions/agent-session.ts, src/sessions/model-overrides.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (9 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-04T18:01:59.532Z sha b366df6 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-04T18:08:29.085Z sha b366df6 :: needs real behavior proof before merge. :: [P1] Handle default model resets in session-store sync
  • reviewed 2026-07-04T18:13:23.546Z sha b366df6 :: needs real behavior proof before merge. :: [P1] Handle default model resets in session-store sync
  • reviewed 2026-07-04T18:28:52.903Z sha b366df6 :: needs real behavior proof before merge. :: [P1] Handle default model resets in session-store sync
  • reviewed 2026-07-04T18:40:24.388Z sha b366df6 :: needs real behavior proof before merge. :: [P1] Handle default model resets in session-store sync
  • reviewed 2026-07-04T19:05:52.312Z sha 0060de4 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-04T19:12:59.350Z sha 0060de4 :: needs real behavior proof before merge. :: [P2] Widen the mocked options type
  • reviewed 2026-07-04T19:56:39.910Z sha 0060de4 :: needs real behavior proof before merge. :: [P2] Widen the mocked options type

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. labels Jun 14, 2026
@clawsweeper clawsweeper Bot added P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. labels Jun 14, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 15, 2026
Closed
4 tasks
@clawsweeper clawsweeper Bot removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. labels Jun 17, 2026
@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 17, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 19, 2026
Open
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. labels Jun 19, 2026
This was referenced Jun 20, 2026
Closed
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jun 28, 2026
Closed
@openclaw-barnacle openclaw-barnacle Bot added triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. channel: telegram Channel integration: telegram labels Jul 4, 2026
@samson910022

samson910022 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated the stale-model fix to use the canonical session store path, sync before prompt/status reads, mark Telegram /model writes pending, and add bridge coverage for the embedded runner plus real registry/auth behavior.

Verification:

  • node scripts/run-vitest.mjs src/agents/sessions/agent-session.model-sync.test.ts
  • node scripts/run-vitest.mjs src/agents/embedded-agent-runner/run/attempt.spawn-workspace.resource-loader.test.ts
  • node scripts/run-vitest.mjs src/agents/embedded-agent-runner/run/attempt.spawn-workspace.context-engine.test.ts

@clawsweeper

clawsweeper Bot commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 4, 2026
@samson910022
samson910022 force-pushed the fix/stale-model-after-switch-92415 branch from b366df6 to 0060de4 Compare July 4, 2026 18:59
@samson910022

samson910022 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Rebased onto current upstream/main, resolved the Telegram and context-engine conflicts, and added the default-reset regression so liveModelSwitchPending with cleared overrides falls back to the session default model.

Verification:

  • node scripts/run-vitest.mjs src/agents/sessions/agent-session.model-sync.test.ts
  • node scripts/run-vitest.mjs src/agents/embedded-agent-runner/run/attempt.spawn-workspace.resource-loader.test.ts src/agents/embedded-agent-runner/run/attempt.spawn-workspace.context-engine.test.ts

@clawsweeper

clawsweeper Bot commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@samson1357924
fix(agents): sync stale this.model snapshot after /model switch
f7664ac 
AgentSession.this.model can become stale when /model command writes to
the session store but the in-memory AgentSession never re-reads it.
Only the prompt() entry point — used by TUI interactive and plugin SDK
long-lived sessions — needs the fix; the embedded runner creates a
fresh AgentSession per run so it was already safe.

Add syncModelFromStoreEntry() called at the start of prompt() before
model validation. It guard-chains: store access → liveModelSwitchPending
flag → valid provider+id → registry lookup → auth configured → direct
assignment + appendModelChange() + setThinkingLevel(). A
model-already-matches guard prevents repeated transcript entries.

Closes openclaw#92415
PR openclaw#92424 (replaced)
@samson1357924
fix(agents): repair test file lint/type errors for CI
1824efb 
Remove dead code (4 unused helper functions), unused imports, fix
ResourceLoader mock return types, use correct AuthCredential shape,
and add curly braces to satisfy oxlint.

check-lint: 16 errors -> 0
check-test-types: 15 errors -> 0
@samson1357924
fix(agents): wire storePath and sessionKey into AgentSession construc…
278d83a 
…tion

The syncModelFromStoreEntry() fix added in this PR was dead code in
production because no caller of createAgentSession() passed storePath
or sessionKey. Both are already available in the embedded runner params
(sessionFile = store path, sessionKey = session key) but were never
forwarded to the AgentSession constructor.

Add storePath/sessionKey to EmbeddedAgentSessionOptions bridge type
and pass them from attempt.ts and compact.ts call sites. This makes
the model-override sync actually activate on production TUI/plugin SDK
paths where the session store contains pending liveModelSwitchPending
state.
@samson1357924
fix(agents): address review findings on test file
a6d4f06 
- Remove empty test (covered by real test)
- Add afterEach with vi.restoreAllMocks() for centralized spy cleanup
- Remove all individual mockRestore() calls (now handled by afterEach)
- Remove unused findSpy/authSpy variable assignments
- Add model-already-matches guard test (critical missing coverage)
- Add modelOverride missing guard test (guard 4b)
@samson1357924
fix(agents): sync stale model from session store
b0a599e
@samson1357924
fix(test): widen resource-loader mock type for storePath/sessionKey
211df63
@samson910022
samson910022 force-pushed the fix/stale-model-after-switch-92415 branch from 0060de4 to 211df63 Compare July 5, 2026 18:29
@samson910022

samson910022 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Rebased onto latest upstream/main and fixed the check-test-types failure by widening the resource-loader mock-call type from { resourceLoader?: unknown } to { resourceLoader?: unknown; storePath?: unknown; sessionKey?: unknown }.

Verification:

  • pnpm check:test-types — passed
  • node scripts/run-vitest.mjs src/agents/sessions/agent-session.model-sync.test.ts — 11/11 passed
  • node scripts/run-vitest.mjs src/agents/embedded-agent-runner/run/attempt.spawn-workspace.resource-loader.test.ts src/agents/embedded-agent-runner/run/attempt.spawn-workspace.context-engine.test.ts — 71/71 passed
    New head: 211df634a80

@clawsweeper

clawsweeper Bot commented Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 7, 2026
Open
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 20, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling channel: telegram Channel integration: telegram mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P1 High-priority user-facing bug, regression, or broken workflow. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: L stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

3 participants

@samson910022 @Patrick-Erichsen @samson1357924