Skip to content

fix(ui): honor config.json's default_model when creating a session - #3850

Merged
kovtcharov-amd merged 3 commits into
amd:mainfrom
cameronmichaelharper-ai:fix/ui-session-default-model-ignores-config
Sep 18, 2026
Merged

kovtcharov-amd merged 3 commits into
amd:mainfrom
cameronmichaelharper-ai:fix/ui-session-default-model-ignores-config

Conversation

@cameronmichaelharper-ai

@cameronmichaelharper-ai cameronmichaelharper-ai commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Before this fix, a brand-new chat, a new scheduled task, or any session created with no explicitly picked model silently landed on the hard-coded Gemma-4-E4B-it-GGUF default — even for a user who had already run gaia config set default_model <id>, since that setting only ever reached gaia CLI commands, never the Agent UI. A session could land on the exact model #3596 already documents as unreliable at native tool-calling, with nothing in the UI pointing back to why. After this fix, a new session resolves its model the same way CLI commands already do (explicit pick > configured default > built-in floor), and the device-switch auto-rewrite guard was updated in lockstep so a session sitting on a configured default keeps following device switches instead of looking "pinned to a custom model."

Fixes #3843.

Test plan

  • tests/unit/chat/ui/test_database.py: session creation uses the configured default when set, still falls back to the hard-coded floor when unset, an explicit model= still wins over a configured default.
  • tests/unit/test_multi_device_wiring.py: a session on a configured default still follows a device switch; a corrupt config.json returns an actionable 500 (not a raw one) from both the create and device-switch endpoints.
  • tests/unit/chat/ui/test_chat_helpers.py: _build_create_kwargs forwards a configured default as model_id rather than omitting it.
  • An autouse fixture in tests/unit/conftest.py isolates ~/.gaia/config.json for the whole unit suite, so these tests (and the existing device-switch tests) don't depend on the contributor's own machine.
  • black clean on all touched files.
  • Verified against clean upstream/main: the one test class here that uses FastAPI's TestClient fails identically pre- and post-patch on this Windows dev machine (a pre-existing socket.socketpair()/asyncio-proactor-loop issue unrelated to this change); everything else passes, including a standalone script directly exercising the router's exact default-model boolean for all input cases.

@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Request changes

This makes a new chat in the Agent UI use the model you set with gaia config set default_model, instead of always jumping to the hard-coded Gemma default. The core fix is right, the precedence (explicit pick > your config > built-in) matches what the CLI already does, and the author correctly spotted that the device-switch guard had to move with it. Three small things to clear up before merge.

  • The docs still say this setting only affects the command line. The config reference and the setting's own description both scope default_model to gaia chat / gaia llm / gaia prompt. As of this PR it also governs every new chat created in the UI, so both need a line updated — the project rule is that a behaviour change updates every doc that describes it.
  • The test suite now reads whatever config the developer happens to have. The three new tests isolate it, but an existing device-switch test still asserts the hard-coded model name while creating a session with no model. On a clean CI machine that passes; on any developer machine where someone has actually set a default model, it fails for no obvious reason. Worth an isolation fixture for the whole unit suite rather than per-test patches.
  • One knock-on effect isn't covered. The same "is this session on a default model?" question is asked in a second place, when the agent itself is built — and that spot wasn't updated. The practical result looks correct (your configured model really does reach the agent), but nothing tests it, and a comment in this PR's own file promises the two checks stay in sync.

Also: please drop the "Generated with Claude Code" footer from the PR description — the repo prohibits AI attribution on any artifact.

Real-world evidence

N/A — no evidence bundle was produced for this run, so the verdict rests on static review plus the PR's unit tests. This does change a user-visible Agent UI surface (which model a brand-new chat lands on), and the PR's own test plan notes the router tests couldn't be executed locally because of a pre-existing Windows socket issue, so the new device-switch regression test will run for the first time in CI. A screenshot of a new chat showing the configured model in the dropdown would close the loop — a nudge, not a blocker.

🔍 Technical details

🟡 Important

1. Docs not updated for the widened scope of default_model (docs/reference/cli.mdx:1423, src/gaia/config.py:48-49)

CLAUDE.md: "A functional change must update EVERY doc that describes it." default_model is documented in two places as CLI-only; this PR makes it govern ChatDatabase.create_session(), i.e. every "New Chat"/"New Task" and every scheduler.create_session().

| `default_model` | _(unset)_ | Default model ID for `gaia chat` / `gaia llm` / `gaia prompt`, and for new Agent UI sessions |

And the dataclass docstring at src/gaia/config.py:48-49 ("Persistent default model ID for model-bearing commands (gaia chat / gaia llm / gaia prompt)") needs the same widening. The "Default model precedence" section at cli.mdx:1427 is worth a sentence too.

2. Existing tests now read the real ~/.gaia/config.json (tests/unit/test_multi_device_wiring.py:513)

test_switch_to_gpu_keeps_default_model does client.post("/api/sessions", json={}) then asserts model == "Gemma-4-E4B-it-GGUF". After this change that assertion depends on the host machine's config file. tests/unit/conftest.py has no autouse config isolation, and mock_home wouldn't help anyway — GAIA_CONFIG_FILE is resolved at import time, which is exactly why the new tests patch the module globals directly.

Green in CI (clean HOME), red for any contributor who has run gaia config set default_model …. Rather than patching per test, an autouse fixture in tests/unit/conftest.py fixes it once for the suite:

@pytest.fixture(autouse=True)
def _isolate_gaia_config(tmp_path, monkeypatch):
    """Keep unit tests off the developer's real ~/.gaia/config.json.

    GAIA_CONFIG_FILE is resolved at import time, so patching HOME is not enough.
    """
    from gaia import config as config_mod

    monkeypatch.setattr(config_mod, "GAIA_CONFIG_DIR", tmp_path)
    monkeypatch.setattr(config_mod, "GAIA_CONFIG_FILE", tmp_path / "config.json")

The three new tests could then drop their local patching and keep only the GaiaConfig(...).save() line.

3. The mirrored guard in _chat_helpers wasn't updated, and the knock-on isn't tested (src/gaia/ui/_chat_helpers.py:516, comment at src/gaia/ui/routers/sessions.py:181)

sessions.py:181 says the rewrite guard "mirrors the runtime guard in _chat_helpers". That guard is elif model_id and model_id != _DB_DEFAULT_MODEL: — branch 2 of _build_create_kwargs's precedence. A session on a configured default now has model != SESSION_DEFAULT_MODEL, so it takes branch 2 ("session-explicit model") instead of branch 3, which omits model_id so the agent's own kwargs.setdefault governs (the #841 fix).

For the chat/gaia path that is almost certainly the behaviour you want — the configured model actually reaches the agent rather than being persisted and then ignored. But it is a real change to agent construction for registry agents that declare no models preference (the ones registry.resolve_model() returns None for at _chat_helpers.py:1486), and it's neither described in the PR nor covered by a test. Two things to do:

  • Add a test asserting _build_create_kwargs forwards the configured default as model_id — locking in the intended behaviour rather than leaving it incidental.
  • Refresh the sessions.py:181 comment, which now over-promises: the two guards deliberately treat the configured default differently.

🟢 Minor

4. A corrupt config now breaks "New Chat" with an opaque message (src/gaia/ui/database.py:46)

GaiaConfig.load() raises GaiaConfigError on a present-but-invalid config.json. create_session is wrapped at routers/sessions.py:107 in a catch-all that returns "Failed to create session. Check server logs for details." — so a typo in config.json now makes every new chat fail with a message that doesn't name the cause, where previously the UI was unaffected. Failing loudly is correct per CLAUDE.md; the message isn't actionable. Consider surfacing GaiaConfigError's own text (it already names the file and the fix) in the HTTP detail. PUT /api/sessions/{id} with a device has no handler at all and will 500 raw.

5. Comment blocks are much longer than the repo standard (src/gaia/ui/database.py:17-32, :400-404, src/gaia/ui/routers/sessions.py:190-194)

CLAUDE.md "Code Comments — Short or Skip": one short line for the why, and "Don't reference the current task, fix, or callers inline" — the (#3843) / "previously only gaia CLI commands honored it" framing rots as soon as the code moves. The docstring on resolved_default_model() already carries the meaning; the 16-line block above SESSION_DEFAULT_MODEL can lose most of its weight:

# Hard-coded floor for new sessions — kept in sync with the SQL schema DEFAULT
# and any code that reads session["model"] and falls back when NULL. Not what a
# new session necessarily gets: resolved_default_model() checks the user's
# configured default_model first.
SESSION_DEFAULT_MODEL = "Gemma-4-E4B-it-GGUF"

6. Drop the AI-attribution footer from the PR description

CLAUDE.md "No Claude Attribution of Any Kind" covers PR descriptions explicitly. Please remove the 🤖 Generated with Claude Code line.

Strengths

  • Catching the device-switch guard as part of the same change is the good instinct here — fixing create_session() alone would have quietly stopped config-default sessions from following a device switch, a regression that would have been much harder to trace later.
  • The three-way test split (configured default wins over the floor, floor survives when unset, explicit model= beats both) covers the precedence properly rather than just the happy path.
  • Reusing GaiaConfig.resolve_model() instead of re-implementing the precedence keeps one definition of "highest wins" across CLI and UI.

create_session() previously fell straight to the hard-coded
SESSION_DEFAULT_MODEL ("Gemma-4-E4B-it-GGUF") whenever a caller didn't
supply a model — which is every "New Chat"/"New Task" click and every
scheduler.create_session() call. ~/.gaia/config.json's default_model only
ever reached `gaia` CLI commands (GaiaConfig.resolve_model), so a user's
configured default gave their UI sessions no protection at all, and they
could silently land on the exact model amd#3596 already documents as
unreliable at native tool-calling.

Adds resolved_default_model() as the single place that resolves "no model
given" against config.json's default_model before falling back to the
literal, used by both create_session() and the device-switch auto-rewrite
guard in routers/sessions.py (that guard's is_default_model check needed
the same update, or a session sitting on a configured default would look
"pinned" and stop following device switches).

Fixes amd#3843
…model_id guard

default_model was only documented as CLI-scoped; cli.mdx and GaiaConfig's
docstring now say it also governs new Agent UI sessions.

Unit tests previously isolated ~/.gaia/config.json per test; an autouse
fixture in tests/unit/conftest.py does it once for the whole suite, so an
existing device-switch test asserting the hard-coded model name can't
silently start reading a contributor's real config.

_build_create_kwargs's own "is this the default model" check compares
against the same literal the device-switch guard does, but the two guards
answer different questions on purpose (session rewrite vs. reaching the
agent as model_id) — a comment now says so explicitly instead of claiming
they mirror each other, and a test locks in that a configured default
still reaches the agent rather than being silently dropped.

A corrupt config.json now surfaces its own actionable message through both
endpoints that read it, instead of an unhelpful generic 500.
@cameronmichaelharper-ai
cameronmichaelharper-ai force-pushed the fix/ui-session-default-model-ignores-config branch from 3996dcf to 05a790c Compare September 14, 2026 23:29
@github-actions github-actions Bot added the documentation Documentation changes label Sep 14, 2026
@cameronmichaelharper-ai

Copy link
Copy Markdown
Contributor Author

All three addressed, plus the two minor items and the attribution footer.

  • Docs: cli.mdx's config table/precedence section and GaiaConfig.default_model's docstring now say it governs new Agent UI sessions too, not just gaia chat/llm/prompt.
  • Test isolation: added an autouse _isolate_gaia_config fixture in tests/unit/conftest.py (your suggested shape) and dropped the per-test patching from the three new tests now that it's global.
  • _chat_helpers guard: added TestBuildCreateKwargsConfiguredDefault in test_chat_helpers.py locking in that a configured default reaches the agent as model_id, and reworded the routers/sessions.py comment to state the two guards deliberately diverge instead of claiming they mirror.
  • Corrupt config (minor): both POST /api/sessions and the device-switch path in PUT /api/sessions/{id} now catch GaiaConfigError and surface its own message instead of a generic/raw 500 — covered by two new tests.
  • Comment length (minor): trimmed the flagged blocks in database.py to roughly your suggested wording.
  • Dropped the Claude Code footer from the PR description.

Pushed as a second commit on the branch; happy to squash before merge if you'd rather.

…tion-identity mismatch

The macOS smoke job's new test (added in the last commit) failed:
except GaiaConfigError as e: never matched, falling through to the
generic except Exception clause, even though the traceback shows the
exact GaiaConfigError raised at the expected call site. Reproduced
locally (direct coroutine call, matching code) and it matches correctly
there every time, so this isn't a logic bug in the ordinary sense --
something about that one CI job's import graph makes the except clause's
isinstance check fail without changing the exception's own traceback or
message. Not fully root-caused.

Merges the two except clauses into one and adds a qualified-name string
match as a fallback alongside isinstance, so detecting a GaiaConfigError
no longer depends on the two modules agreeing on class identity. Verified
against the corrupt-config case directly (TestClient itself can't run on
this Windows dev machine, per the existing pre-existing socketpair note).
@itomek itomek self-assigned this Sep 17, 2026
@kovtcharov-amd
kovtcharov-amd added this pull request to the merge queue Sep 18, 2026
Merged via the queue into amd:main with commit dbe1ed5 Sep 18, 2026
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Documentation changes tests Test changes

Projects

None yet

3 participants