Skip to content

fix(gsd): prevent auto-mode setModel calls from persisting to settings.json#3486

Open
deseltrus wants to merge 3 commits intogsd-build:mainfrom
deseltrus:fix/auto-setmodel-persist
Open

fix(gsd): prevent auto-mode setModel calls from persisting to settings.json#3486
deseltrus wants to merge 3 commits intogsd-build:mainfrom
deseltrus:fix/auto-setmodel-persist

Conversation

@deseltrus
Copy link
Copy Markdown
Contributor

@deseltrus deseltrus commented Apr 4, 2026

TL;DR

What: Fix three layers of model/thinking-level persistence leaking through auto-mode.
Why: Transient model switches silently overwrite the user's saved defaults — every subsequent session starts with the wrong model and thinking level.
How: (1) Pass options through the Extension API setModel wrapper, (2) add persist: false to auto-mode setModel calls, (3) thread persist through thinking level updates.

What

Three bugs combine to silently corrupt settings.json during auto-mode:

1. Extension API wrapper drops persist option (loader.ts) — ROOT CAUSE

The ExtensionAPI.setModel wrapper in loader.ts accepts only model, silently dropping the options argument:

// Before — options silently dropped
setModel(model) { return runtime.setModel(model); }

// After — options passed through
setModel(model, options) { return runtime.setModel(model, options); }

This means every pi.setModel(model, { persist: false }) call from any extension becomes runtime.setModel(model) — persist is always true. This single bug renders all downstream persist guards ineffective.

Note: The ExtensionCommandContext wrapper (agent-session.ts) already passes options correctly — only the loader.ts wrapper was affected.

2. Auto-mode setModel calls missing persist flag (auto.ts)

Two pi.setModel() calls omit { persist: false }:

  • stopAuto() model restore (cleanup path)
  • Hook model dispatch (transient per-unit override)

All 7 other setModel() calls in the GSD extension already use persist: false.

3. Thinking level not guarded by persist option (agent-session.ts)

_applyModelChange() calls setThinkingLevel() unconditionally. Even when the model switch uses persist: false, the thinking level change is always written to settings.json. If a transient switch triggers thinking level clamping (e.g. xhighhigh because an intermediate model lacks the capability), the clamped level is persisted.

File Change
packages/pi-coding-agent/src/core/extensions/loader.ts Pass options through setModel wrapper
packages/pi-coding-agent/src/core/agent-session.ts Extract _applyThinkingLevel() that respects persist option
src/resources/extensions/gsd/auto.ts Add { persist: false } to both setModel() calls
src/resources/extensions/gsd/tests/model-isolation.test.ts Structural regression test

Why

The combination produces this failure mode:

  1. User sets Model A with thinking level xhigh as their default
  2. Auto-mode dispatches a unit, triggering a transient model switch via selectAndApplyModel
  3. The extension calls pi.setModel(modelB, { persist: false })
  4. loader.ts drops { persist: false } → model B is persisted to settings.json
  5. The model switch triggers setThinkingLevel → if model B lacks xhigh support, thinking level is clamped to high and persisted
  6. Next session reads corrupted defaults from settings.json

Related: #650 (original model config bleed report — the auto.ts fix addresses remaining call sites, but the loader.ts bug was the actual reason previous persist guards were ineffective)

How

Commit 1: auto.ts — add { persist: false } to remaining calls

Mechanical, consistent with the 7 existing calls that already use it.

Commit 2: agent-session.ts — thread persist through thinking level

Extract _applyThinkingLevel(level, options?) as a private method. Public setThinkingLevel(level) API unchanged.

Commit 3: loader.ts — pass options through wrapper (ROOT CAUSE)

One-line fix that makes all downstream persist guards actually work.

Test Evidence

  • Root cause verified via runtime instrumentation: File-based debug logging in _applyThinkingLevel confirmed that persist=undefined reached the function even when callers passed { persist: false } — proving the loader wrapper was dropping the argument.
  • Stack trace confirmed call path: selectAndApplyModelloader.js:472 setModel(model)runtime.setModel(model) — options lost at the wrapper boundary.
  • Regression test: model-isolation.test.ts structural test verifies all setModel calls in auto.ts use persist: false. 8/8 pass.
  • User-initiated changes still persist: Model selector and Ctrl+P cycling paths do not use persist: false and correctly write to settings.json.

AI-assisted contribution — reviewed and tested by contributor.

  • fix

…s.json

Two setModel() calls in auto.ts omit { persist: false }, causing
transient model switches during auto-mode to silently overwrite the
user's saved default model in settings.json. Every subsequent session
then starts with the wrong model.

The affected calls are:
- stopAuto() model restore (cleanup path)
- Hook model dispatch (transient per-unit override)

All other setModel calls in the GSD extension already use persist: false.
This aligns the two remaining calls with the established pattern.

Adds a structural regression test that fails if any setModel call in
auto.ts is missing persist: false.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 4, 2026

🔴 PR Risk Report — CRITICAL

Files changed 4
Systems affected 4
Overall risk 🔴 CRITICAL

Affected Systems

Risk System
🔴 critical Agent Core
🔴 critical State Machine
🔴 critical Auto Engine
🟠 high Extensions
File Breakdown
Risk File Systems
🔴 packages/pi-coding-agent/src/core/agent-session.ts Agent Core, State Machine
🟠 packages/pi-coding-agent/src/core/extensions/loader.ts Extensions
🔴 src/resources/extensions/gsd/auto.ts Auto Engine
src/resources/extensions/gsd/tests/model-isolation.test.ts (unclassified)

⚠️ Critical risk — please verify: state persistence, auth token lifecycle, agent loop race conditions, RPC protocol compatibility.

_applyModelChange() calls setThinkingLevel() unconditionally, which
always writes the effective thinking level to settings.json. When
auto-mode dispatches use setModel({ persist: false }), the model
default is correctly protected, but the thinking level default is not.

If a transient model switch triggers thinking level clamping (e.g.
xhigh → high), the clamped level is persisted and every subsequent
session starts with the wrong thinking level.

Extract _applyThinkingLevel(level, options?) as a private method that
respects the persist option from _applyModelChange. The public
setThinkingLevel(level) API is unchanged — it delegates to
_applyThinkingLevel without options, preserving the existing persist
behavior for user-initiated changes.
The ExtensionAPI.setModel wrapper in loader.ts accepts only the model
parameter, silently dropping the options argument. This means every
pi.setModel(model, { persist: false }) call from any extension becomes
runtime.setModel(model) — the persist guard is lost and transient model
switches always overwrite the user's saved default in settings.json.

This is the actual root cause behind model and thinking level bleed
during auto-mode. The auto.ts and agent-session.ts fixes (persist:false
on setModel calls, _applyThinkingLevel persist guard) were correct but
ineffective because the options never reached the runtime.

The ExtensionCommandContext wrapper (agent-session.ts L2069) already
passes options correctly — only the loader.ts wrapper was affected.
@jeremymcs
Copy link
Copy Markdown
Collaborator

Adversarial Review — persist: false thinking-level leak

Verdict: needs-attention

Finding (high severity)

The persist: false path added for transient auto-mode model switches is not fully isolated for thinking level.

_applyThinkingLevel (agent-session.ts:1748-1751) still appends thinking_level_change entries to durable session history even when options.persist === false. On session resume, replay logic calls setThinkingLevel(...) which writes back to settings by default — meaning a transient auto-mode model/thinking switch can survive in session state and silently overwrite the user's saved default in settings.json.

This defeats the intended isolation fix.

Recommendation

  1. Don't record thinking-level changes as durable thinking_level_change entries when persist: false, or mark them as transient and skip them during session replay.
  2. Ensure resume applies replayed thinking with a non-persisting path (e.g., internal setter with { persist: false }) so replay cannot mutate settings.json.

Suggested test

Add an integration test that:

  1. Triggers setModel(..., { persist: false })
  2. Reloads/resumes the session
  3. Asserts settings.json default thinking level is unchanged

Review generated via Codex adversarial review

@jeremymcs
Copy link
Copy Markdown
Collaborator

This PR has merge conflicts with the base branch. Please rebase or merge main to resolve before review can proceed.

🤖 Automated PR audit — 2026-04-04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants