* fix(#589): show in-session model picker when user sends /model Typing a bare '/model' in the chat composer now opens the model picker so the user can hot-swap the CURRENT session's model via the existing slash-command path, instead of hand-typing the provider/model string. - ChatViewModel.preloadModelOptions() caches GET /api/model/options at GatewayReady so the picker opens instantly; falls back to on-demand refresh if opened before preload finishes. - sendMessage() intercepts a bare '/model' (no arg) -> showModelPicker=true instead of dispatching; a typed '/model provider/model' still dispatches via handleSlashCommand. - sendSlashModel() routes through command.dispatch with session_id, so the swap is session-scoped (not the global POST /api/model/set). - Extracted ModelPickerDialog into ui/model/components and reused by both ModelScreen and ChatScreen (adds loading + empty states). - Added a top-bar model chip (current session model label) that opens the same picker. - Added unit tests: bare /model opens picker, picker selection hot-swaps via slash dispatch with session_id, typed /model with arg dispatches directly. Note: CI shows 2 pre-existing test failures (ChatViewModelTest.testToggleSearch, HermesWsClientTest.testAutoReconnect) that also fail on a clean main full suite (MockK static-mock leakage between classes). Unrelated to this change; compile + ktlint + the 3 new tests pass. * fix(#589): use backend-valid session-scoped /model command syntax The model picker now dispatches the canonical backend command form '/model <model> --provider <slug> --session' instead of the unsupported 'provider/model' slash form. The '--session' flag keeps the switch scoped to the current chat only (per-session override) and never writes the global model config, which matches the backend model-switch contract. - sendSlashModel builds '/model $model --provider $provider --session'. - Updated unit tests to assert the corrected arg payload ('gpt-4o --provider openai --session') and the fully-typed dispatch path. * fix(#589): add pinned-models section to in-session model picker - ModelPickerDialog now renders a Pinned section (mirrors global model screen) - search filter matches model names too, not just provider name/slug - ChatUiState gains modelPickerPinned, fed from AuthManager.getPinnedModels() - top-bar model chip removed; picker opens on /model only - test mock for getPinnedModels() added * fix(#589): route /model via prompt.submit instead of command.dispatch (resolves 4018) /model is a real backend slash command handled in the backend's message pipeline (_handle_message -> _handle_model_command), so it must arrive as a normal prompt.submit message, not the command.dispatch RPC (which only knows quick/plugin/bundle/skill commands and 4018s on /model). - SlashCommandDispatcher: add SlashResult.ModelSwitch, map /model -> ModelSwitch - ChatViewModel: handle ModelSwitch -> handleModelSwitch() sends via WsMethods.PROMPT_SUBMIT with {session_id, text} - Tests: rewrite the two /model tests to assert prompt.submit (previously asserted the buggy command.dispatch path); add /model -> ModelSwitch test Independent agy review: APPROVE, no blocking issues. * fix(#589): route /model via slash.exec (correct transport; prompt.submit made LLM treat it as text) Earlier attempt routed /model through prompt.submit, but the TUI gateway's prompt.submit handler does NOT parse slash commands — it runs the text as a normal agent turn, so the LLM received '/model ...' as a user message (the exact reported bug). The correct RPC is slash.exec, which runs the command through the slash worker and _mirror_slash_side_effects -> _apply_model_switch to hot-swap the live session model. command.dispatch is also wrong (only knows quick/plugin/bundle/skill commands -> 4018). - WsMethods: add SLASH_EXEC = "slash.exec" - ChatViewModel.handleModelSwitch: send slash.exec with {command, session_id} (previously prompt.submit with {text}); corrected doc comment - SlashCommandDispatcher: corrected comments (slash.exec, not prompt.submit) - ChatViewModelTest: rewrite both /model tests to assert slash.exec with the /model <model> --provider <slug> --session command string - SlashCommandDispatcherTest: /model -> ModelSwitch already covered Verified against hermes-agent tui_gateway/server.py: slash.exec (L13190) -> worker.run + _mirror_slash_side_effects (L13299) -> _apply_model_switch (L13118); parse_model_flags accepts --provider/--session (hermes_cli/model_switch.py:355). Independent agy review of the prior attempt: APPROVE (the wrong-transport assumption was the author's, not agy's). * fix(#589): route /model via config.set key=model (slash.exec 4001'd; prompt.submit made LLM treat it as text) Third transport attempt, now verified against the gateway source: - prompt.submit: gateway runs text as a normal agent turn -> LLM sees '/model' as a message (the original reported bug). WRONG. - slash.exec: gateway returns 4001 'session not found'. The slash worker path resolves the session differently / requires agent-built state the mobile session isn't in at switch time. WRONG. - config.set key=model: gateway (server.py config.set L10253) routes key=='model' straight to _apply_model_switch using the SAME _sessions.get(session_id) lookup that the working command.dispatch uses. CORRECT. sendSlashModel still builds '/model <model> --provider <slug> --session'; parse_model_flags (hermes_cli/model_switch.py:355) extracts model/provider/ session from it. handleModelSwitch now sends config.set {key:model, value: command, session_id}. - WsMethods: add CONFIG_SET = 'config.set' - ChatViewModel.handleModelSwitch: config.set instead of slash.exec - SlashCommandDispatcher: comments updated (config.set, not slash.exec) - ChatViewModelTest: both /model tests assert config.set key=model + command - also fixes the MockK testToggleSearch flake by preloading byte-buddy agent via JAVA_TOOL_OPTIONS locally (container blocks JVM self-attach) * fix(#589): strip /model prefix before config.set key=model (5001 'model names can't have spaces') config.set key=model passes the value DIRECTLY to parse_model_flags — it does NOT strip a leading '/model' the way slash.exec / prompt.submit do. Sending '/model <model> ...' made parse_model_flags treat '/model <model>' as the model name (contains a space) -> 5001. handleModelSwitch now strips the '/model' prefix and sends the bare spec '<model> --provider <slug> --session', matching the TUI client's modelValueForConfigSet (ui-tui session.ts:23) used by useMainApp.ts:123. - ChatViewModel.handleModelSwitch: spec = command.removePrefix('/model').trim() - ChatViewModelTest: assert bare spec 'gpt-4o --provider openai --session' - comment updated * fix(#589): strip /model prefix case-insensitively before config.set SlashCommandDispatcher and isModelPickerCommand match /model case-insensitively, but handleModelSwitch used removePrefix("/model") (case-sensitive). A typed /MODEL ... forwarded the literal slash prefix to config.set and the backend's parse_model_flags wouldn't recognize it — silent hot-swap failure. Use startsWith("/model", ignoreCase=true) + substring(6) so any casing is stripped. Added a regression test asserting /MODEL does not forward a leading slash. * fix(test): inject searchDispatcher to fix MockKException in testToggleSearch --------- Co-authored-by: M57 <m57@hermes-control.app>
This commit is contained in:
M57andM57 authored and GitHub committed
1 parent
c543eb6470
commit
48f5ffbe52
8 files changed
+565
-66
No files matched your search
@@ -13,6 +13,7 @@ import com.m57.hermescontrol.data.ws.JsonRpcError
|
||||
import com.m57.hermescontrol.data.ws.WsEvent
|
||||
import com.m57.hermescontrol.data.ws.WsMethods
|
||||
import com.m57.hermescontrol.ui.chat.fakes.FakeChatPersistenceRepository
|
||||
import io.mockk.coEvery
|
||||
import io.mockk.every
|
||||
import io.mockk.mockk
|
||||
import io.mockk.mockkObject
|
||||
@@ -68,6 +69,7 @@ class ChatViewModelTest {
|
||||
every { Dispatchers.Main } returns testMainDispatcher
|
||||
|
||||
mockkObject(AuthManager)
|
||||
every { AuthManager.getPinnedModels() } returns emptyList()
|
||||
mockkObject(HermesWsClient)
|
||||
mockkObject(ApiClient)
|
||||
mockkObject(HermesDatabase)
|
||||
@@ -101,6 +103,30 @@ class ChatViewModelTest {
|
||||
arg<((String) -> Unit)?>(2)?.invoke(id)
|
||||
id
|
||||
}
|
||||
|
||||
// Stub model-options so preloadModelOptions() (fired at GatewayReady) is safe.
|
||||
val mockApi = mockk<com.m57.hermescontrol.data.remote.HermesApiService>(relaxed = true)
|
||||
every { ApiClient.hermesApi } returns mockApi
|
||||
coEvery {
|
||||
mockApi.getModelOptions(any(), any())
|
||||
} returns
|
||||
retrofit2.Response.success(
|
||||
com.m57.hermescontrol.data.model.ModelOptionsResponse(
|
||||
providers =
|
||||
listOf(
|
||||
com.m57.hermescontrol.data.model.ModelProvider(
|
||||
slug = "openai",
|
||||
name = "OpenAI",
|
||||
models = listOf("gpt-4o", "gpt-4o-mini"),
|
||||
),
|
||||
com.m57.hermescontrol.data.model.ModelProvider(
|
||||
slug = "anthropic",
|
||||
name = "Anthropic",
|
||||
models = listOf("claude-3-5-sonnet"),
|
||||
),
|
||||
),
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
@After
|
||||
@@ -113,7 +139,7 @@ class ChatViewModelTest {
|
||||
|
||||
/** Create a ViewModel with the fake repo injected directly. */
|
||||
private fun createViewModel(startCleanup: Boolean = false): ChatViewModel =
|
||||
ChatViewModel(app, startCleanup, fakeRepo)
|
||||
ChatViewModel(app, startCleanup, fakeRepo, testDispatcher)
|
||||
|
||||
/**
|
||||
* Create ViewModel, simulate GatewayReady, feed SESSION_CREATE result,
|
||||
@@ -320,6 +346,129 @@ class ChatViewModelTest {
|
||||
verify { HermesWsClient.send(WsMethods.COMMAND_DISPATCH, any(), any()) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testBareModelCommand_opensPickerInsteadOfDispatch() =
|
||||
runTest {
|
||||
val (viewModel, _) = createViewModelWithSession()
|
||||
|
||||
// A bare "/model" must NOT dispatch a slash command; it opens the picker.
|
||||
viewModel.sendMessage("/model")
|
||||
advanceUntilIdle()
|
||||
|
||||
assertTrue(
|
||||
"picker should be shown when bare /model is typed",
|
||||
viewModel.uiState.value.showModelPicker,
|
||||
)
|
||||
assertTrue(
|
||||
"picker should have preloaded providers (cached at GatewayReady)",
|
||||
viewModel.uiState.value.modelPickerProviders.isNotEmpty(),
|
||||
)
|
||||
verify(exactly = 0) { HermesWsClient.send(WsMethods.COMMAND_DISPATCH, any(), any()) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testModelPickerSelection_hotSwapsCurrentSessionViaSlash() =
|
||||
runTest {
|
||||
val (viewModel, sessionId) = createViewModelWithSession()
|
||||
|
||||
viewModel.sendMessage("/model")
|
||||
advanceUntilIdle()
|
||||
assertTrue(viewModel.uiState.value.showModelPicker)
|
||||
|
||||
// Selecting a model must send the bare spec "gpt-4o --provider openai
|
||||
// --session" via the `config.set` RPC with key="model" (the gateway
|
||||
// routes key=="model" to _apply_model_switch; the /model prefix is
|
||||
// stripped before send because config.set does not parse slash
|
||||
// commands). NOT command.dispatch (4018s on /model), NOT prompt.submit
|
||||
// (LLM would treat it as text). Capture the config.set params.
|
||||
val modelCalls = mutableListOf<Triple<String, String, String>>()
|
||||
every { HermesWsClient.send(WsMethods.CONFIG_SET, any(), any()) } answers {
|
||||
val params = arg<Map<String, Any>>(1)
|
||||
modelCalls.add(
|
||||
Triple(
|
||||
params["key"] as String,
|
||||
params["value"] as String,
|
||||
params["session_id"] as String,
|
||||
),
|
||||
)
|
||||
"req-cfg-${modelCalls.size}"
|
||||
}
|
||||
|
||||
viewModel.sendSlashModel("openai", "gpt-4o")
|
||||
advanceUntilIdle()
|
||||
|
||||
assertFalse("picker closes after selection", viewModel.uiState.value.showModelPicker)
|
||||
assertEquals(
|
||||
"openai/gpt-4o",
|
||||
viewModel.uiState.value.currentSessionModel,
|
||||
)
|
||||
verify { HermesWsClient.send(WsMethods.CONFIG_SET, any(), any()) }
|
||||
val call = modelCalls.firstOrNull { it.first == "model" }
|
||||
assertNotNull("selection must route through config.set key=model", call)
|
||||
assertEquals("gpt-4o --provider openai --session", call!!.second)
|
||||
assertEquals(sessionId, call.third)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testTypedModelCommandWithArg_dispatchesDirectly() =
|
||||
runTest {
|
||||
val (viewModel, sessionId) = createViewModelWithSession()
|
||||
|
||||
// A fully-typed "/model <model> --provider <slug> --session" bypasses the
|
||||
// picker and dispatches straight to the backend as a normal prompt.
|
||||
viewModel.sendMessage("/model gpt-4o --provider openai --session")
|
||||
advanceUntilIdle()
|
||||
|
||||
assertFalse(
|
||||
"typed /model with arg should not open the picker",
|
||||
viewModel.uiState.value.showModelPicker,
|
||||
)
|
||||
// A fully-typed /model goes to the backend via the `config.set` RPC
|
||||
// (key="model"), which the gateway routes to _apply_model_switch. NOT
|
||||
// command.dispatch (4018s on /model) and NOT prompt.submit (LLM would
|
||||
// treat it as text).
|
||||
verify { HermesWsClient.send(WsMethods.CONFIG_SET, any(), any()) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testTypedModelCommand_caseInsensitive_doesNotForwardSlashPrefix() =
|
||||
runTest {
|
||||
val (viewModel, sessionId) = createViewModelWithSession()
|
||||
|
||||
// A fully-typed /MODEL (uppercase) must still route through
|
||||
// config.set key=model with the BARE spec — the leading "/MODEL"
|
||||
// slash prefix must be stripped, or parse_model_flags on the
|
||||
// backend won't recognize it and the hot-swap silently fails.
|
||||
val modelCalls = mutableListOf<Triple<String, String, String>>()
|
||||
every { HermesWsClient.send(WsMethods.CONFIG_SET, any(), any()) } answers {
|
||||
val params = arg<Map<String, Any>>(1)
|
||||
modelCalls.add(
|
||||
Triple(
|
||||
params["key"] as String,
|
||||
params["value"] as String,
|
||||
params["session_id"] as String,
|
||||
),
|
||||
)
|
||||
"req-cfg-ci-${modelCalls.size}"
|
||||
}
|
||||
|
||||
viewModel.sendMessage("/MODEL gpt-4o --provider openai --session")
|
||||
advanceUntilIdle()
|
||||
|
||||
val call = modelCalls.firstOrNull { it.first == "model" }
|
||||
assertNotNull("uppercase /MODEL must route through config.set key=model", call)
|
||||
assertEquals(
|
||||
"slash prefix must be stripped before send",
|
||||
"gpt-4o --provider openai --session",
|
||||
call!!.second,
|
||||
)
|
||||
assertFalse(
|
||||
"value must not carry the literal /MODEL prefix",
|
||||
call.second.startsWith("/"),
|
||||
)
|
||||
assertEquals(sessionId, call.third)
|
||||
}
|
||||
|
||||
// ── Connection / init tests ──────────────────────────────────────────────
|
||||
|
||||
@Test
|
||||
|
||||
@@ -43,6 +43,16 @@ class SlashCommandDispatcherTest {
|
||||
assertEquals(SlashResult.SessionBranch, dispatcher.dispatch("/FORK hello"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `model routes to ModelSwitch`() {
|
||||
// /model is a real backend slash command (issue #589) that must travel as
|
||||
// a normal prompt message — NOT via command.dispatch (which 4018s on it,
|
||||
// because that RPC only knows quick/plugin/bundle/skill commands).
|
||||
assertEquals(SlashResult.ModelSwitch, dispatcher.dispatch("/model"))
|
||||
assertEquals(SlashResult.ModelSwitch, dispatcher.dispatch("/MODEL openai/gpt-4o --session"))
|
||||
assertEquals(SlashResult.ModelSwitch, dispatcher.dispatch("/Model"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `NEW uppercase still routes to NewSession`() {
|
||||
// Dispatcher lower-cases before matching, so case must not matter.
|
||||
|
||||
Reference in new issue
Block a user