Fix Try New: bypass setup form and use scripted runtime for tutorial sessions#439
Draft
charneykaye wants to merge 7 commits into
Draft
Fix Try New: bypass setup form and use scripted runtime for tutorial sessions#439charneykaye wants to merge 7 commits into
charneykaye wants to merge 7 commits into
Conversation
…parse error
JSON.parse('') throws on empty bodies, causing POST /api/setup/outcome (which
returns 204) to report a runtime-unreachable error to the caller. The server
commits before responding, so the data was always written, but the client-side
error was misleading. Return { ok: true, data: undefined } for 204 responses.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e for turns
Two bugs caused the First Words tutorial to fail on first open:
1. handleStartTutorial() navigated to /setup/first_words_tutorial (a form that
required LLM readiness), instead of starting the session immediately. The fix
creates the tutorial session directly via api.createSession() and navigates to
/conversation/:sessionId, bypassing the setup form entirely. The fallback to
the setup form is kept for cases where session creation fails unexpectedly.
2. submit_turn and generate_debrief always used app.state.runtime (the fake
runtime started at process startup), ignoring the active config written by
use_model. This caused tutorial sessions to produce generic fake responses
("Hello there. I am a simulated NPC.") instead of the authored scripted
tutorial content. The fix resolves the runtime from the active DB config so
use_model({ runtime_id: 'scripted' }) actually takes effect for play.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y-new-fails-on-first-open
…l config _resolve_runtime() read the active runtime from the DB on every turn and debrief. That setting is global and mutable: routers/setup_install.py calls set_active_config(runtime_id="llama_cpp") the moment a background model download finishes. The "Start now" tutorial CTA is explicitly designed to be played while that download runs, so an install completing mid-tutorial would silently reroute the remaining scripted turns — and, more likely, the tutorial's authored debrief, which is generated after the conversation ends — to the freshly installed model, abandoning the tutorial script. Snapshot the active runtime id into setup_json at session creation when it is scripted or fake, and resolve from that snapshot instead. The session keeps the runtime it started with for its whole lifetime (forks inherit it, since branch_service copies setup_json verbatim). Sidecar runtimes and sessions without a snapshot still resolve to app.state.runtime, so connection-pool state and test-injected runtimes behave as before. Adds a regression test that flips the active config to llama_cpp mid-tutorial and asserts the following turns and the debrief still come from the script.
Contributor
Author
|
Reviewed code and pushed fixes. |
The session pin added in 60b8ecc was read from the global active runtime at create time, so it only held if api.useModel({runtime_id:'scripted'}) had already landed. Two paths defeat that: - useModel returns an ApiResult and never throws, so its `{ok:false}` result is swallowed by the best-effort try/catch. The tutorial then runs unpinned on app.state.runtime — the fake NPC — which is the exact symptom of issue #427, with no error surfaced. - On the "Start now" path a model is downloading. If the install pipeline finishes in the window between useModel and createSession it calls set_active_config(llama_cpp), and the tutorial session is created unpinned. Let the session declare its own runtime instead: SessionCreateRequest takes an optional runtime_id, restricted by a Literal to the model-free runtimes ("scripted", "fake") so a client can never point a session at a sidecar-backed one. An explicit value wins; otherwise the active-config sniff still pins demo and scripted sessions created through the ordinary setup form. When neither applies, no runtime_id key is written to setup_json, so old and new sessions resolve identically. Also add the missing regression test for the 204 fix in ce5c496: no test covered handleResponse's empty-body path, which is what regressed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Reviewed code and pushed fixes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root causes
Two bugs caused 'Try it right now' to fail on first open:
Bug 1 — Wrong destination after clicking Try it right now
handleStartTutorial()navigated to/setup/first_words_tutorial, a form that validates LLM readiness before enabling the Start button. On v0.2.3 the health endpoint omittedruntime, sollm_readydefaulted tofalseand the Start button was permanently disabled. Even in the current codebase the form was unnecessary friction: the tutorial has fixed settings and should start in one click.Bug 2 — Tutorial used the wrong runtime
submit_turnandgenerate_debriefalways readapp.state.runtime, which is initialised at startup fromconfig.runtime_id("fake") and never updated. Callinguse_model({ runtime_id: 'scripted' })only wrote the selection to the database; it never reached the session pipeline. Tutorial NPC responses came fromFakeChatRuntime("Hello there. I am a simulated NPC.") instead of the authoredScriptedChatRuntimetutorial script.Bug 3 — 204 No Content parsed as an error
handleResponse<T>()calledJSON.parse('')on empty bodies, which throws, returning{ ok: false, error: { kind: 'runtime-unreachable' } }for thePOST /api/setup/outcomeendpoint that returns 204. The server always committed before responding so data was written, but the client error was misleading and obscured real failures.Fixes
apps/web/src/setup/useSetupFlow.ts—handleStartTutorial()now callsapi.createSession()with the tutorial's fixed parameters and navigates directly to/conversation/:sessionId, bypassing the setup form. A fallback to/setup/first_words_tutorialis kept if session creation fails unexpectedly.services/convsim-core/convsim_core/routers/sessions.py— Added_resolve_runtime()helper that reads the active runtime from the database when the selection is"scripted"or"fake"(both stateless, cheap to instantiate per-request). For sidecar-based runtimes (llama.cpp, Ollama) or when no explicit selection exists,app.state.runtimeis returned unchanged, preserving existing behaviour and test-injected runtimes.apps/web/src/api/client.ts—handleResponse<T>()now short-circuits onres.status === 204and returns{ ok: true, data: undefined }without attempting to parse the empty body.Tests
useSetupFlow.test.ts: added navigation spy and three new tests covering the direct session-creation path, the/conversation/:sessionIdnavigation, and the setup-form fallback on session-creation failure.FirstRunWizard.test.tsx: updated six tests that expected navigation to the setup form to expect navigation to the conversation route; addedcreateSessionto the mock andbeforeEachreset.test_scripted_runtime.py: two new integration tests — one verifying that tutorial turns produce the authored scripted text (not the fake placeholder), one verifying that the tutorial debrief uses the scripted runtime's authored summary.Closes #427