fix(console): stop wizard saves from deleting custom tiers - #938
Merged
Conversation
Review follow-up to #935 (#931). Two bugs the config narrowing made reachable, plus the DESIGN.md contradictions it left behind. - buildConfigPayload merged llm_providers with what is on disk but rebuilt llm_tiers from heavy/standard/light alone and returned it wholesale. The alias-keyed PATCH replaces the on-disk set with whatever the patch sends — the same reason the provider merge exists — so a user with a custom tier (schema.go documents "heavy/standard/light or custom tiers") lost it by changing one model in the wizard. #935 routed Config.svelte's llm_tiers editing to that wizard deep link, which opened the path to normal users. - Inspect rendered uncommitted edits. shouldShowFieldActions hides Save/Discard outside Quick Start, but getDisplayValue preferred dirtyFields regardless, so a pane banner-labelled "Read-only inspection" showed an unsaved value as though the server had loaded it. - DESIGN.md claimed the 13 Quick Start gates "stay interactive" while the consequence table three bullets later made embodiment_providers_json read-only, and promised masked entry for 7 credential fields when only 2 still have an input. Both now describe what the code does, including that token rotation needs host filesystem access. - Recorded configStructured.ts's draft builders and the saveConfig / getProviders wrappers as retained-but-unreferenced, kept for the embodiment-preset follow-up DESIGN.md already names. Left in place rather than deleted because #931 is still open; the note is the delete inventory if that follow-up is declined. Verified: npm run check 529 files 0 errors, npm test 303/303 (the new tier test fails against the old code), go test ./internal/tarsserver/.
|
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
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.



Summary
Review follow-up to #935 (#931). Two bugs the config narrowing made reachable, plus the DESIGN.md contradictions it left behind. The removed editors are not restored — that scope narrowing is the point of #931.
Wizard saves silently delete custom tiers
buildConfigPayloadmergesllm_providerswith what is already on disk, and its own comment explains why: "the backend's alias-keyed PATCH replaces the on-disk alias set with what the patch sends — so we must include every existing alias here". The identical reasoning applies tollm_tiers, which was instead rebuilt fromrequiredTiers(heavy/standard/light) alone and returned wholesale.internal/config/schema.go:70documentsllm_tiersas "heavy/standard/light (or custom tiers)". So a user with avisiontier who clicks Edit in wizard onllm_tiersand changes one model loses that tier at save, and any role bound to it breaks at the next restart.The limitation pre-dated #935, but #935 deleted the safe local tier editor and routed
Config.svelteinto this path (Config.svelte:391), which is what opened it to normal users.Inspect rendered uncommitted edits as server state
shouldShowFieldActionsisviewMode === 'quick', so Save/Discard are hidden outside Quick Start — butgetDisplayValuepreferreddirtyFieldsregardless of mode. A pane whose banner reads "Read-only inspection", and which DESIGN.md describes as showing "what the server actually loaded", displayed an unsaved value with no way to commit it. Toggle something in Quick Start, switch to Inspect to confirm, see the new value, restart — the edit is gone.DESIGN.md contradicted itself and the code
embodiment_providers_json— as staying interactive; line 296, in the same section, made that field read-only. The code implements 296. CLAUDE.md makes DESIGN.md the design source of truth, so the next contributor would have assumed an interactive embodiment card exists.memory_embed_api_keyandtools_web_search_api_keystill have inputs;api_auth_token,api_user_token,api_admin_token,tools_web_search_perplexity_api_keyandwork_scheduler_a2a_bearer_tokenhave zero occurrences insrc/. Now stated plainly, including the consequence that token rotation requires filesystem access to the host — a console-only operator cannot rotate them.Dead code: documented, not deleted
Deleting the modal editors left
configStructured.ts's seven draft builders and their constant tables with no component consumer (Config.svelteimports only four display helpers), kept green by their own unit test;saveConfigandgetProviderslikewise have zero call sites. I did not delete them: #931 is still open and DESIGN.md:296 already names a possible embodiment-preset follow-up that would reuse exactly this code. Instead DESIGN.md now carries the inventory, so it reads as deferred rather than forgotten — and as the delete list if that follow-up is declined.Not changed
The removed capability itself — embodiment provider editing, YAML save, provider-alias deletion, per-provider live model lists. Those are #931's intent, not defects.
Test plan
npm run check— 529 files, 0 errors, 0 warnings (this also establishes that refactor(console): narrow console surface to chat and observability #935 type-checks, which its review could not verify)npm test— 303/303 (note:test:ciis a curated slice that excludesonboarding.test.ts, so the new test only runs under the full suite)go test ./internal/tarsserver/🤖 Generated with Claude Code