Skip to content

Add dashboard scoped-model settings editor - #433

Merged
m-aebrer merged 9 commits into
masterfrom
feature/issue-404-dashboard-scoped-models
Aug 6, 2026
Merged

Add dashboard scoped-model settings editor#433
m-aebrer merged 9 commits into
masterfrom
feature/issue-404-dashboard-scoped-models

Conversation

@m-aebrer

@m-aebrer m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Closes #404

Adds a staged dashboard editor and RPC contract for persistent scoped-model configuration, including canonical ordered persistence, explicit unfiltered clearing, project-shadow warnings, responsive controls, and /scoped-models routing.

Implementation plan posted as a comment below.

@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Implementation Plan — Dashboard scoped-model settings

Problem analysis and settled behavior

The existing feature is split across two surfaces: enabledModels is durable settings data resolved at startup, while dashboard scopedModels is a read-only snapshot of one already-running session. This change must expose the durable setting without confusing it with live state.

The implementation will use these explicit semantics:

  • No persisted field means all current and future available models. The update wire format uses enabledModels: null to request that clear operation; it never depends on JSON serializing undefined.
  • A non-empty array means an ordered partial scope. RPC validates and canonicalizes every entry to an exact available provider/model, rejects duplicates/fuzzy patterns/unavailable models, and preserves supplied order.
  • An empty array is invalid for writes. The editor may stage zero selected models, but Save remains blocked with a visible validation error. Legacy persisted empty or non-matching pattern lists remain inspectable with resolver diagnostics rather than silently becoming a saved “all” state.
  • Existing pattern settings resolve only in coding-agent core. The settings snapshot carries the raw effective patterns, their ordered resolved scoped models, and structured resolver warnings. The browser does no glob, fuzzy, suffix, or duplicate-resolution logic.
  • Saving an edited legacy scope normalizes it. Globs, fuzzy entries, and per-pattern thinking suffixes are replaced by exact canonical model references. The UI/docs will state this before save; running sessions remain unchanged.
  • Implicit all-model order is registry order and is not reorderable. Reorder controls apply to explicit partial scopes. To preserve the future-inclusive unfiltered state, attempting to reorder “all” will not materialize a full inventory list. This matches the upstream fix recorded at /scoped-models: reorder in "all enabled" state flips every model to selected earendil-works/pi#3331.
  • Project context is explicit for this editor. The ordinary Settings page remains global/home-backed. The Scoped models section gets a project-context selector from known fleet/disk roots; its settings/model requests use a utility runtime rooted at that cwd. Reads therefore show the effective global-plus-project scope, while writes still target global settings and return the existing loud project-shadow warning.
  • The slash command deep-links to context. /scoped-models from a session navigates to the Scoped models editor with that session's cwd selected. Direct Settings navigation starts in global/home context.

External prior art also supports keeping provider presentation separate from an explicit enabled list: ChatJS documents provider ordering and canonical curated model defaults as distinct configuration concepts (https://www.chatjs.dev/docs/customization/models). The dashboard will likewise preserve model-registry/provider grouping while maintaining a separate explicit cycling order.

Deliverables

1. Structured core scope resolution and settings source metadata

  • Add a structured model-scope resolution helper that returns ordered ScopedModel results plus the same warning text currently emitted for invalid suffixes and unmatched patterns.
  • Keep the existing resolveModelScope() behavior for CLI/TUI callers by wrapping the structured helper and emitting its warnings as today.
  • Add a SettingsManager project-override query for enabledModels, matching the established hasProjectAgentModelOverride() pattern.
  • Preserve existing global write/clear behavior and merged project-over-global reads.

2. Persistent settings RPC contract

  • Extend RpcSettingsSnapshot with:
    • the raw effective persisted enabledModels patterns;
    • the ordered resolved scoped-model DTOs;
    • structured scope warnings;
    • project-override/source metadata needed by the dashboard.
  • Extend RpcSettingsUpdate with enabledModels?: string[] | null; null explicitly clears the filter.
  • Make settings snapshot construction model-registry-aware after the existing flush/reload boundary, while retaining the current settings-operation lock and loud read/write failure behavior.
  • Validate the complete update before applying anything. For enabledModels, reject empty arrays, duplicates, malformed/fuzzy references, and unavailable models; canonicalize accepted case-insensitive exact references through the core exact-reference matcher.
  • Normalize a supplied array equal to the authoritative available inventory to the clear/unfiltered state as defense in depth.
  • Persist accepted partial order through SettingsManager.setEnabledModels(), clear through setEnabledModels(undefined), and append a verbatim warning when a project enabledModels value shadows the global write.
  • Return the same enriched post-write snapshot so the UI receives the actual effective value after project merging.

3. Cwd-aware dashboard settings transport

  • Add a distinct dashboard settings-update DTO rather than overloading the snapshot DTO; this is where enabledModels: null is legal.
  • Allow the existing settings, settings-models, and save-settings client calls to carry an optional project cwd.
  • Validate an optional cwd on the server and pass it to the already cwd-keyed ensureUtilityRuntime(cwd) path. Keep default requests home-backed and preserve current error propagation.
  • Keep this context scoped to the Scoped models editor so unrelated Settings controls do not unexpectedly change to project-merged values.

4. Staged, responsive Scoped models editor

  • Add a focused Solid component under dashboard client components and mount it as a new Settings section.
  • Load the enriched persistent snapshot and available model inventory for the selected context.
  • Show:
    • enabled models in cycling order;
    • available models grouped by provider;
    • provider/model/name search;
    • per-model and whole-provider enable/disable controls;
    • an explicit Enable all action;
    • move-up/move-down controls for ordered partial scopes;
    • dirty/saving/saved state, resolver/normalization notices, and explicit Save/Reset actions.
  • Use native buttons/checkboxes and move controls rather than drag-only reordering so keyboard, touch, and assistive-technology users have the same behavior.
  • From implicit all, disabling one model/provider materializes the remaining models in registry order; enabling the complete inventory collapses staged state back to implicit all.
  • Block zero-model save in the component before RPC, while keeping the server validation authoritative.
  • On failed save, show the server error verbatim and retain the staged edit; do not present optimistic data as durable. On a shadowed successful write, show the returned warning verbatim and reset the baseline to the returned effective project value.
  • Explain that the setting seeds new sessions and never mutates running sessions.

5. /scoped-models routing and focused Settings entry

  • Extend the dashboard settings route with an optional scoped-model target and encoded cwd while preserving #/settings compatibility.
  • Pass route context into SettingsScreen, select/focus the Scoped models section on entry, and keep direct links reload-safe.
  • Add a scoped-models built-in handler in the session screen that rejects unsupported arguments and navigates with the current runtime cwd instead of showing the generic unavailable notice.

6. Documentation synchronization

Update every issue-mandated user-facing surface:

  • root README.md;
  • packages/coding-agent/README.md;
  • packages/coding-agent/docs/settings.md;
  • packages/coding-agent/docs/rpc.md;
  • packages/coding-agent/docs/dashboard.md;
  • packages/dashboard/README.md.

Document the all/partial/zero contract, canonicalization of legacy patterns, explicit RPC null clear, project-context/global-write behavior and warning, new-session-only effect, responsive controls, and /scoped-models dashboard action. Remove the current not-yet-implemented dashboard statement.

Files to create

  • packages/dashboard/src/client/components/scoped-models-editor.tsx — isolated staged selection/order UI and save boundary.
  • packages/dashboard/test/client/scoped-models-editor.test.tsx — focused component behavior and error-state coverage.

Files to modify

Core and RPC

  • packages/coding-agent/src/core/model-resolver.ts — structured resolution diagnostics while retaining existing callers.
  • packages/coding-agent/src/core/settings-manager.ts — project enabledModels shadow metadata.
  • packages/coding-agent/src/modes/rpc/rpc-types.ts — enriched snapshot and explicit update clear type.
  • packages/coding-agent/src/modes/rpc/rpc-mode.ts — model-aware snapshots, validation, canonical persistence, warnings, and post-write mapping.
  • packages/coding-agent/src/modes/rpc/rpc-client.ts — typed snapshot/update documentation or signatures if required by the enriched DTO.

Dashboard production code

  • packages/dashboard/src/shared/protocol.ts — snapshot/update/resolved-scope DTOs.
  • packages/dashboard/src/client/api.ts — optional cwd settings/model/save calls.
  • packages/dashboard/src/server/server.ts — optional cwd validation and utility-runtime routing.
  • packages/dashboard/src/client/state/store.ts — settings deep-link parse/serialization.
  • packages/dashboard/src/client/app.tsx — pass scoped-model route context into Settings.
  • packages/dashboard/src/client/screens/settings.tsx — context selection, resource wiring, and editor section.
  • packages/dashboard/src/client/screens/session.tsx/scoped-models handler.
  • packages/dashboard/src/client/styles/app.css — editor/order/provider controls and mobile layout.

Tests and fixtures

  • packages/coding-agent/test/model-resolver.test.ts.
  • packages/coding-agent/test/settings-manager.test.ts.
  • packages/coding-agent/test/rpc-settings-commands.test.ts.
  • packages/dashboard/test/runtime-pool.test.ts where shared fake-client snapshots require the new fields.
  • packages/dashboard/test/server.test.ts.
  • packages/dashboard/test/client/store.test.ts.
  • packages/dashboard/test/client/screens.test.tsx.
  • packages/dashboard/test/client/settings-layout.browser.test.ts.

Documentation

  • The six files listed in deliverable 6.

Testing approach

Coding-agent tests

  • Resolver diagnostics: exact, glob, unmatched, invalid-thinking suffix, duplicate, slash/colon-containing model IDs, and stable pattern/match order; verify the legacy wrapper still emits warning text.
  • Settings source: merged project arrays replace global arrays; project override detection is true even for an explicit empty array; clearing removes the global JSON key without changing a project override.
  • RPC snapshot: no filter vs raw empty legacy filter vs partial legacy patterns; ordered canonical resolution; warning transport; project-source metadata.
  • RPC updates: null clear, ordered partial persistence, full-inventory normalization, canonical case normalization, and rejection of zero, duplicates, fuzzy/pattern input, malformed references, and unavailable models.
  • Atomic/error paths: mixed-payload validation changes nothing; failed writes leave the prior durable enabledModels; project-shadowed writes land globally but return the effective project scope plus the exact warning.
  • Client typing: enriched getSettings()/setSettings() snapshots and warnings pass through unchanged.

Dashboard component and integration tests

  • Initial states: implicit all, explicit partial order, pattern-resolved order, resolver warning, project override, and no available models.
  • Editing: model toggle, provider toggle, search filtering, enable all, zero selection, partial reorder boundaries, Reset, and dirty state.
  • All semantics: reorder is disabled/no-op while all is implicit; disabling from all preserves registry order; restoring all sends null, never a full list.
  • Save behavior: partial sends ordered canonical refs; zero never calls the API; success adopts the authoritative response; warnings render verbatim; failure renders verbatim and retains staged edits.
  • Context/routing: project selector sends cwd on settings/models/save requests; #/settings remains compatible; scoped-model deep links round-trip cwd; /scoped-models routes with the active session cwd and never calls api.prompt().
  • Server: default home utility behavior remains unchanged; valid cwd reaches the cwd-keyed utility runtime for GET models/settings and PUT settings; missing cwd is rejected loudly; RPC errors/warnings pass through.
  • Real-browser layout: at 360, 700, 701, and 1024 px, model IDs, provider headings, controls, ordered rows, and validation/warning text stay within the viewport with usable tap controls and no horizontal overflow.

Validation commands

Implementation must finish with:

npm run build
npx vitest --run packages/coding-agent/test/model-resolver.test.ts packages/coding-agent/test/settings-manager.test.ts packages/coding-agent/test/rpc-settings-commands.test.ts
npx vitest --run packages/dashboard/test/client/scoped-models-editor.test.tsx packages/dashboard/test/client/screens.test.tsx packages/dashboard/test/client/store.test.ts packages/dashboard/test/server.test.ts packages/dashboard/test/client/settings-layout.browser.test.ts
npm test
npx tsgo --noEmit
npx biome check --write <changed-files>
npm run verify-workspace-links
git diff --check

The build must precede any manual test against the real dreb binary. Manual dashboard QA should cover desktop and mobile widths, a project override, a failed save, and /scoped-models from a live session; it must not restart the long-running dashboard service without explicit permission.

Acceptance criteria

  • Dashboard Settings shows the effective persistent scoped-model state for global or selected project context, including legacy-resolution warnings.
  • Model search/grouping, individual/provider/all toggles, staged reset/save, and accessible ordered partial reordering work on desktop and mobile.
  • Partial saves persist exact ordered canonical references; all saves remove the filter through explicit null; zero saves fail visibly.
  • Legacy patterns are resolved only by coding-agent core and normalize transparently when edited.
  • Invalid input and persistence failures do not change the prior durable scope or claim success.
  • A selected project override produces the exact visible shadow warning while the global write still lands.
  • New settings affect only new sessions; running scopedModels are untouched.
  • /scoped-models opens the context-aware editor instead of the unavailable notice.
  • Core/RPC, server, component, integration, routing, and real-browser responsive tests cover happy paths and failures.
  • All six documentation surfaces describe the shipped behavior consistently.

Risks and mitigations

  • Async snapshot ripple: resolved scope needs the model registry while the current mapper is synchronous and reused by trust mutations. Keep the existing pure field mapping separable and compose one async enriched snapshot helper at durable response boundaries; update all snapshot-returning tests together.
  • Project ambiguity: never infer a project from whichever runtime opened first. Carry cwd explicitly and use the existing cwd-keyed hidden utility-runtime mechanism.
  • Inventory races: null records the user's future-inclusive “all” intent independent of a stale browser list; strict server validation rejects partial references that disappear before save.
  • Identifier parsing: reuse findExactModelReferenceMatch() so provider IDs containing additional slashes/colons are not broken by ad hoc splitting.
  • Legacy normalization: show raw patterns and diagnostics before save and document that exact-reference persistence drops pattern thinking suffixes.
  • Implicit-all materialization: disable/no-op reordering in all state, following the upstream regression fix, so no full inventory is accidentally frozen.
  • Write durability: retain the current settings lock/flush/drain discipline, test with a real file-backed manager, and treat returned snapshots—not optimistic browser state—as authoritative.
  • Scope control: keep project context limited to this editor and avoid refactoring unrelated Settings controls or runtime model selection.

There are no remaining planning blockers; the behavior above resolves the assessment's project-context, snapshot, diagnostics, normalization, slash-routing, and authoritative-all questions.


Plan created by mach6

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Vitest coverage

Metric Covered Total Coverage
Statements 28340 45421 62.39%
Branches 15526 28053 55.34%
Functions 5538 8438 65.63%
Lines 24174 39007 61.97%

View full coverage run

@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Progress Update

Implemented the dashboard scoped-model settings feature end to end:

  • added structured core scope resolution diagnostics and project override metadata;
  • extended settings RPC snapshots and updates with canonical ordered scopes, explicit null clearing, atomic validation, full-inventory normalization, and loud project-shadow warnings;
  • added cwd-aware dashboard settings/model/save transport and reload-safe scoped-model deep links;
  • built the staged responsive editor with grouped search, model/provider/all toggles, accessible ordering, validation, save/reset, and authoritative success/error handling;
  • routed /scoped-models from live dashboard sessions using the active runtime cwd;
  • synchronized all six user-facing documentation surfaces;
  • added core, RPC, server, store, screen, component, and real-browser responsive coverage.

Verification completed successfully: build, focused suites, full test suite, type-check, Biome, workspace-link verification, diff checks, and isolated desktop/mobile Playwright QA including project shadowing, failed-save retention, successful save warnings, and live-session slash routing.

Commit: b56bddc


Progress tracked by mach6

@m-aebrer
m-aebrer marked this pull request as ready for review August 5, 2026 15:55
@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Code Review

Critical

None.

Important

  1. Project-shadow save test does not verify authoritative response adoptionpackages/dashboard/test/client/scoped-models-editor.test.tsx mocks a project-sourced save response with the same resolved list the user submitted. The test therefore passes even if ScopedModelsEditor.save() ignores the returned effective project scope and retains optimistic global-write state. Add a response whose project-resolved scope differs from the submitted list, then assert cycling rows, checkboxes, warning text, and saved baseline adopt that response. (high, confidence 98)

Suggestions

  1. RPC accepts non-canonical scoped-model references despite its canonical contractpackages/coding-agent/src/modes/rpc/rpc-mode.ts validates writes with findExactModelReferenceMatch(), but that helper intentionally accepts unique bare model IDs and trims provider/model halves. Inputs such as a unique gpt-5 can silently canonicalize instead of being rejected as malformed. Use a strict case-insensitive ${provider}/${id} matcher at this RPC boundary while retaining the permissive helper for legacy/CLI resolution. (medium, confidence 90)

  2. The editor remains actionable with stale state during a project-context refreshpackages/dashboard/src/client/components/scoped-models-editor.tsx keys its resource by cwd, but Solid retains the prior value while loading. The old controls remain rendered and Save sends the current cwd with the prior context's staged order; prior save warnings/status can also remain visible. Render/edit only data tagged with the current cwd, disable controls while changing context, and clear context-specific status when applying a fresh snapshot. (medium, confidence 85)

  3. Scoped-model deep-link and context-selection wiring lacks an integration test — current tests separately cover route serialization, slash-command navigation, an initial cwd prop, and a mocked onCwdChange, but none mounts the real route-to-Settings path and proves that changing context reloads and saves with the selected cwd. Add an integration test spanning app.tsx, settings.tsx, and the editor. (medium, confidence 94)

  4. Responsive browser coverage tests a hand-written fixture, not the shipped editorpackages/dashboard/test/client/settings-layout.browser.test.ts injects static HARNESS_HTML, so production JSX/layout regressions can pass. Render the actual editor or Settings screen with long IDs, warnings, validation text, and ordered controls at 360/700/701/1024 widths. (medium, confidence 99)

  5. The model-aware durable get_settings boundary is not tested end to end — direct snapshot tests manually pass an inventory, while getFreshSettingsForRpc tests omit the registry. A regression dropping session.modelRegistry from the production path could return empty resolution without failing tests. Add a durable legacy-pattern test with a stub registry and assert ordered resolved models and diagnostics. (medium, confidence 89)

  6. Dashboard update typing still admits read-only snapshot fieldspackages/dashboard/src/shared/protocol.ts derives SettingsUpdateDto by omitting only scoped-model computed fields, leaving fields such as effectiveTrustedContextRoots; packages/dashboard/src/client/screens/settings.tsx also types its helper as Partial<SettingsDto>. These payloads compile but fail the RPC allowlist. Define the writable keys explicitly or omit every read-only field, and use SettingsUpdateDto in the screen helper. (low, confidence 88)

  7. Unused exported resolver wrapper expands the public APIpackages/coding-agent/src/core/model-resolver.ts exports resolveModelScopeWithDiagnostics(), but its only caller is the legacy wrapper immediately below it. Inline resolveModelScopePatterns(patterns, await modelRegistry.getAvailable()) into that caller and remove the unused export. (low, confidence 95)

  8. Guaranteed-loaded editor scope uses redundant nullable access — inside <Show when={data()}>, the provider checkbox reads (data()?.models ?? []) rather than the supplied non-null loaded().models accessor. Use the accessor to make the guarantee explicit and remove unreachable fallback code. (low, confidence 85)

Strengths

  • Settings writes preserve loud failures, whole-payload validation, atomicity, and explicit null versus invalid empty-array semantics.
  • Cwd-aware utility-runtime routing is validated and does not silently fall back to home context.
  • The implementation covers all authoritative deliverables and synchronizes all six required documentation surfaces.
  • The editor retains staged edits on failed saves, uses authoritative post-write snapshots, and provides accessible non-drag ordering controls.
  • Core, RPC, server, component, routing, and responsive coverage is broad; focused changed suites passed 608 tests during review.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Review Assessment

Review comment

Classifications

Finding Classification Reasoning
1. Project-shadow save test does not verify authoritative response adoption genuine Factual: The test submits and returns the same resolved scope, so it passes even if the response is ignored. Scope: Authoritative project-effective response adoption and its tests are explicit plan requirements.
2. RPC accepts non-canonical scoped-model references genuine Factual: findExactModelReferenceMatch() accepts unique bare IDs and whitespace around provider/model segments, then canonicalizes them. Scope: Exact canonical provider/model partial writes and malformed-input rejection are explicit contract and acceptance requirements.
3. Editor remains actionable with stale state during context refresh genuine Factual: Solid retains prior resource data while refreshing; old controls stay active, while Save combines old staged order with the new cwd and old status remains visible. Scope: This can write one project's scope in another project context, violating core cwd-specific correctness and data integrity.
4. Deep-link and context-selection wiring lacks an integration test genuine Factual: Existing tests cover routing, slash navigation, and editor cwd separately, but not the real App → SettingsScreen → ScopedModelsEditor path or context-change reload/save behavior. Scope: Context-aware route integration is an explicit testing and acceptance requirement.
5. Responsive browser coverage uses a hand-written fixture genuine Factual: The browser suite uses static HARNESS_HTML plus production CSS, not production JSX, and omits required states. Scope: The plan explicitly requires real-browser coverage of the shipped editor at all listed widths with identifiers, warnings, validation, and controls.
6. Model-aware durable get_settings boundary is not tested end to end genuine Factual: Direct mapper tests supply inventory, but every durable refresh test omits a registry, so dropping the production registry handoff would not fail tests. Scope: Registry-aware snapshots after flush/reload and legacy diagnostics are explicit deliverables requiring boundary coverage.
7. Dashboard update typing admits read-only fields genuine Factual: The update type includes effectiveTrustedContextRoots, and the screen helper accepts snapshot-only fields that RPC rejects. Scope: This PR explicitly introduces a distinct dashboard settings-update DTO rather than overloading the snapshot DTO. Human review confirmed that leaving known read-only fields writable undermines that scoped contract and should be corrected in this PR.
8. Unused exported resolver wrapper expands the public API nitpick Factual: The exported helper has one local caller, but it is used and is not exposed through package exports. Scope: Inlining changes clarity only and is not required for correctness or authorized behavior.
9. Redundant nullable access inside loaded editor scope nitpick Factual: The nullable fallback is redundant inside the truthy <Show> branch. Scope: It is behaviorally equivalent clarity-only cleanup.

Action Plan

  1. Isolate context refreshes: tag data with cwd, hide or disable stale controls, prevent cross-context saves, and clear context-specific status and warnings.
  2. Enforce strict canonical RPC references and add rejection coverage for bare IDs and whitespace-normalized forms while preserving IDs containing additional slashes or colons.
  3. Add durable model-aware get_settings coverage with a stub registry and persisted legacy patterns, asserting resolved order and diagnostics.
  4. Strengthen project-shadow save tests with a returned effective scope different from the submitted global scope, asserting authoritative rows, checkboxes, warnings, dirty state, and Reset baseline.
  5. Add real route-to-editor integration coverage that changes project context and verifies settings, inventory, and save calls all use the selected cwd.
  6. Render the shipped editor or Settings screen in responsive browser tests at every required breakpoint, covering long identifiers, warnings, validation, ordering, and save/reset controls.
  7. Define the dashboard settings-update DTO from writable keys only, omit every computed/read-only snapshot field, and type the Settings screen save helper with that DTO.

Assessment by mach6

@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Progress Update

Fixed all seven genuine review findings:

  • enforced strict canonical provider/model RPC writes;
  • isolated scoped-model editor state by project cwd and prevented stale cross-context saves;
  • strengthened authoritative project-shadow response coverage;
  • added route-to-editor context reload/save integration coverage;
  • replaced the static scoped-model layout fixture with the shipped editor rendered through Vite in Chromium;
  • added durable model-aware get_settings boundary coverage;
  • restricted the dashboard settings update DTO to writable keys.

Verification passed: build, focused core/dashboard suites, type-check, Biome, workspace-link verification, diff checks, and the pre-commit deterministic full suite with 5,516 tests passed and 0 failed.

Commit: a118fb8


Progress tracked by mach6

@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Code Review

Critical

None.

Important

None.

Suggestions

  1. Post-write snapshot failure can misreport a successful save as a persistence failure — After setters and flush() have durably completed, setSettingsForRpc() calls await modelRegistry.getAvailable() to construct the response snapshot inside the same broad catch that reports Failed to persist settings. If that inventory read throws, the caller receives a persistence failure even though the write landed, and the scoped-model editor retains staged state as though Save failed. Separate post-write snapshot construction from persistence error handling and report the durable-write outcome truthfully. (packages/coding-agent/src/modes/rpc/rpc-mode.ts, medium, confidence 85)

  2. Model-inventory failure during durable settings reload is untestedgetFreshSettingsForRpc() explicitly converts a rejected inventory load into Failed to load available models: ..., but no test exercises that path. Add a rejecting registry test and assert a loud error with no partial snapshot. (packages/coding-agent/test/rpc-settings-commands.test.ts, medium, confidence 98)

  3. Atomicity is not tested when a valid scoped-model update precedes another invalid field — Coverage proves an invalid scope prevents another valid field from applying, but not the converse: a valid canonical enabledModels value plus a later-invalid field. Add a mixed-payload test and assert the old scope and unrelated settings remain unchanged in memory and on disk. (packages/coding-agent/test/rpc-settings-commands.test.ts, medium, confidence 95)

  4. The editor's initial legacy-empty scope state is untested — The component intentionally distinguishes absent enabledModels from persisted enabledModels: [], but tests do not mount the latter shape. Verify it renders zero enabled rather than implicit all, shows validation, disables Save, makes no API call, and does not silently convert on Reset/render. (packages/dashboard/test/client/scoped-models-editor.test.tsx, medium, confidence 96)

  5. Settings and inventory load failures lack component-level coverage — Save rejection is tested, but initial/context-refresh rejection of api.settings() or api.settingsModels() is not. Add both cases, including after a loaded project changes context, and assert verbatim errors, hidden stale controls, and no possible save. (packages/dashboard/test/client/scoped-models-editor.test.tsx, medium, confidence 94)

  6. Responsive checks omit the ordered-row text element — The real-browser suite measures the shipped editor at all required widths, but its per-element viewport selector excludes .scoped-model-order-row > span. Add the row and its text to the measured elements so clipping hidden by containment/overflow is detected. (packages/dashboard/test/client/settings-layout.browser.test.ts, low, confidence 87)

  7. Unused exported resolver wrapper adds an unnecessary API surfaceresolveModelScopeWithDiagnostics() is a one-line exported wrapper used only by the immediately following legacy wrapper. Inline resolveModelScopePatterns(patterns, await modelRegistry.getAvailable()) into resolveModelScope() and remove the unused export. (packages/coding-agent/src/core/model-resolver.ts, low, confidence 95)

Strengths

  • Every linked-issue acceptance criterion and every approved-plan deliverable is implemented, including all six required documentation surfaces.
  • The RPC contract cleanly distinguishes implicit all, ordered partial, and invalid zero-model scopes; strict canonical validation and project-shadow warnings are explicit and fail loudly.
  • Core resolver diagnostics remain side-effect free while the legacy wrapper preserves terminal warnings.
  • Editor staging, authoritative response adoption, cwd isolation, failed-save retention, accessible ordering, and real production-JSX browser coverage are thoughtfully implemented.
  • The prior seven genuine findings were verified fixed at current HEAD; focused suites passed during review.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Review Assessment

Review comment

Classifications

Finding Classification Reasoning
1. Post-write snapshot failure can misreport a successful save false-positive Factual: The snapshot is inside the persistence catch, but production ModelRegistry.getAvailable() synchronously filters in-memory models and its custom-key resolution catches command failures; the proposed exception path is not supported by current production behavior. Scope: No scoped fix is needed for an artificial throwing stub. If inventory loading later becomes fallible, the response should explicitly state that settings were saved but snapshot construction failed, because success requires an authoritative snapshot.
2. Model-inventory failure during durable reload is untested false-positive Factual: No rejecting-registry test exists, but production getAvailable() cannot reject and has no ordinary throw path; such a mock would not model current behavior. Scope: Required durable model-aware snapshot coverage already exists for legacy patterns and diagnostics. No tracking issue is warranted unless the API becomes fallible.
3. Valid scoped-model update followed by another invalid field lacks atomicity coverage genuine Factual: Tests cover a valid unrelated field plus invalid enabledModels, but none supplies valid canonical enabledModels with a later-invalid field and verifies the prior scope remains unchanged in memory and storage. Scope: The plan explicitly requires mixed-payload validation to change nothing, and the acceptance criteria require invalid input to preserve prior durable settings.
4. Initial legacy-empty editor state is untested genuine Factual: Current code correctly treats enabledModels: [] as explicit zero rather than implicit all, but component tests never mount that persisted snapshot shape. Scope: Inspecting legacy empty scopes without silently broadening them is explicit settled behavior and part of the zero-scope safety contract.
5. Settings and inventory load failures lack component coverage genuine Factual: The component has resource-error and stale-control gating paths, but tests cover save rejection and a pending refresh—not rejected settings or inventory loads initially or after a context switch. Scope: Loud error propagation and cwd isolation are scoped safety requirements; a failed refresh must not expose controls that can save old staged data under a new cwd.
6. Responsive checks omit ordered-row text genuine Factual: The browser measurement selector omits .scoped-model-order-row and its text span, so containment or overflow can hide row clipping from document-width checks. Scope: The approved real-browser plan explicitly requires ordered rows to remain within the viewport at 360, 700, 701, and 1024 px.
7. Unused exported resolver wrapper adds API surface nitpick Factual: resolveModelScopeWithDiagnostics() is exported but only called by adjacent resolveModelScope(); it is still used and is not exposed through the package export map. Scope: The plan requests a structured diagnostic helper and legacy wrapper; inlining is behaviorally equivalent internal organization, not required for correctness.

Action Plan

  1. Add inverse mixed-payload atomicity coverage: submit valid canonical enabledModels with a later-invalid field and verify the prior scoped-model value and unrelated settings remain unchanged in memory and on disk.
  2. Cover persisted legacy enabledModels: [] in the editor: assert zero enabled, visible validation, disabled Save, no API write, and unchanged state after render and Reset.
  3. Cover settings and inventory load failures, including after a loaded-project context switch; assert verbatim errors, no stale controls, and no possible save.
  4. Include .scoped-model-order-row and its text span in viewport checks at every required width.

Assessment by mach6

@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Progress Update

Fixed review findings 3–6:

  • added inverse mixed-payload atomicity coverage proving valid scoped-model changes and unrelated settings remain unchanged in memory and durable storage when a later field is invalid;
  • covered persisted legacy enabledModels: [] through initial render and Reset without broadening it to implicit all;
  • covered settings and inventory load failures both initially and during project-context changes, including stale-control suppression and verbatim errors;
  • fixed the Solid resource-error path exposed by those tests so rejected loads render loudly instead of throwing from the resource accessor;
  • included ordered rows and their text spans in responsive viewport checks at every required width.

Verification passed: build, 104 focused tests, full test suite, type-check, Biome, workspace-link verification, diff checks, and the pre-commit suite with 5,522 tests passed and 0 failed.

Commit: d406a2e


Progress tracked by mach6

@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Code Review

Critical

None.

Important

  1. Settings route cwd is only applied when Settings mountsSettingsScreen initializes scopedModelsCwd from props.initialScopedModelsCwd, but does not synchronize it when the still-mounted Settings route changes. Browser back/forward, a direct hash change, or navigating from a scoped deep link to plain Settings can leave the editor reading and saving through the old project cwd while the URL indicates another project or global context. Synchronize route props into the signal or key/remount the screen by route context. (packages/dashboard/src/client/screens/settings.tsx, medium, confidence 90)

  2. Editing can silently remove persisted scope entries that are currently unresolvedapplySnapshot() stages only resolvedScopedModels, not unresolved raw enabledModels patterns. If credentials or registry changes make a persisted entry unavailable, any later unrelated toggle/reorder and Save rewrites the setting to the reduced resolved subset, permanently dropping the raw entry; the resolver warning does not explain that consequence. Preserve unresolved entries where possible or require explicit confirmation before dropping warned entries. (packages/dashboard/src/client/components/scoped-models-editor.tsx, medium, confidence 80)

Suggestions

  1. Browser API cwd serialization has no direct test — Component and route tests mock api.settings, api.settingsModels, and api.saveSettings, while server tests construct query strings manually. A regression in withCwd() or one of its call sites could route selected-project reads or writes to home while both sides' tests pass. Add fetch-level adapter tests for all three methods with encoded cwd values and no-cwd defaults. (packages/dashboard/src/client/api.ts, medium, confidence 95)

  2. Late save completion after a project switch is untested — The editor guards success and failure results when the cwd changes during Save, but current context-isolation coverage waits for Save to finish before switching. Add deferred resolve/reject tests proving a Project A result cannot overwrite Project B rows, baseline, warning, error, or saved state. (packages/dashboard/src/client/components/scoped-models-editor.tsx, medium, confidence 93)

  3. Per-cwd utility runtimes have no idle eviction — The new cwd-aware settings routes call ensureUtilityRuntime(cwd), whose utility map is retained until dashboard shutdown. Browsing many project roots can accumulate one child runtime per cwd, including a separate utility runtime beside an existing live session. Consider idle eviction and/or same-cwd runtime reuse. (packages/dashboard/src/server/server.ts, packages/dashboard/src/server/runtime-pool.ts, low, confidence 80)

  4. Route integration test relies on fixed sleeps — The new route-to-editor test waits 25 ms and 10 ms for resource loading and Save, which can be flaky under a loaded CI worker. Replace timers with condition-based waits on rendered state and expected calls. (packages/dashboard/test/client/screens.test.tsx, low, confidence 91)

  5. Unused exported resolver wrapper adds an unnecessary API layerresolveModelScopeWithDiagnostics() is exported but used only by the adjacent legacy wrapper. Inline resolveModelScopePatterns(patterns, await modelRegistry.getAvailable()) into resolveModelScope() and remove the extra export. (packages/coding-agent/src/core/model-resolver.ts, low, confidence 95)

  6. Effective-scope materialization is duplicated in editor togglestoggleModel() and toggleProvider() repeat the same implicit-all ternary. A small currentScope() helper would name the concept and remove duplication without changing reactive behavior. (packages/dashboard/src/client/components/scoped-models-editor.tsx, low, confidence 85)

Strengths

  • All linked-issue acceptance criteria, approved plan deliverables, prior genuine action items, and all six required documentation surfaces are present at current HEAD.
  • The RPC contract is strict and fail-closed: explicit null clearing, invalid zero rejection, canonical ordered partial scopes, full-inventory normalization, whole-payload validation, and durable shadow warnings.
  • Legacy scope resolution remains in coding-agent core with structured diagnostics while preserving terminal behavior for existing callers.
  • The editor has authoritative response adoption, loud load/save errors, cwd-tagged stale-state suppression, accessible ordering, legacy-empty handling, and shipped-JSX browser coverage at all required widths.
  • Core/RPC persistence, project shadowing, route integration, error states, atomicity, and responsive behavior have broad focused coverage.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Review Assessment

Review comment

Classifications

Finding Classification Reasoning
1. Settings route cwd is only applied when Settings mounts genuine Factual: SettingsScreen initializes scopedModelsCwd once and does not synchronize later route-prop changes while the Settings branch remains mounted. Scope: The plan requires reload-safe context routes and ordinary Settings navigation to use global/home context; stale cwd can target the wrong project.
2. Editing can remove unresolved persisted scope entries false-positive Factual: The editor visibly renders resolver warnings and explicitly states that saving an edited legacy scope replaces patterns with exact canonical references; unavailable entries cannot satisfy the RPC write contract. Scope: The approved plan intentionally defines edited legacy scopes as normalized resolved references, so preservation or extra confirmation would change settled behavior.
3. Browser API cwd serialization has no direct test genuine Factual: All three new cwd-aware methods depend on withCwd(), but component tests mock those methods and server tests construct query strings independently, leaving encoding and call-site wiring uncovered. Scope: Optional-cwd client transport is an explicit deliverable and directly testable safety boundary for project-context reads and writes.
4. Late save completion after a project switch is untested genuine Factual: save() contains explicit stale-result guards for both resolve and reject paths, but no test changes cwd while Save remains pending. Scope: Preventing a prior context's result from replacing the new context's state is required cwd-isolation and authoritative-response behavior introduced by this PR.
5. Per-cwd utility runtimes have no idle eviction deferred Factual: The utility map retains cwd-keyed child runtimes until shutdown and does not reuse same-cwd session runtimes. Scope: The approved plan explicitly chose the existing cwd-keyed utility-runtime mechanism; lifecycle redesign predates and exceeds this PR's authorized scope. Optional follow-up may be tracked if accumulation is observed.
6. Route integration test relies on fixed sleeps nitpick Factual: The test uses fixed timers rather than condition-based waits, but its mocks resolve immediately and no concrete failure path is demonstrated. Scope: Condition waits would improve clarity, not fill an authorized correctness gap.
7. Unused exported resolver wrapper adds an API layer nitpick Factual: The wrapper is a one-line export with one adjacent caller, but it is used and not re-exported publicly. Scope: Inlining is an internal organization preference without correctness impact.
8. Effective-scope materialization is duplicated nitpick Factual: Two editor handlers repeat the same implicit-all expression. Scope: Extracting a helper is behaviorally equivalent minor cleanup.

Action Plan

  1. Synchronize SettingsScreen scoped-model cwd with changing route props, and cover scoped-cwd changes plus navigation back to plain global Settings without remounting.
  2. Add pending-save resolve and reject tests that switch project context and prove stale completion cannot alter the new context's rows, baseline, warnings, errors, dirty state, or saved state.
  3. Add fetch-level adapter tests for api.settings, api.settingsModels, and api.saveSettings, covering encoded cwd values and no-cwd default URLs.

Assessment by mach6

@m-aebrer

m-aebrer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Progress Update

Fixed the final three genuine scoped-model review findings:

  • synchronized the scoped-model project cwd with live Settings route changes, including direct project deep links and navigation back to global Settings without remounting;
  • added fetch-level coverage for encoded cwd transport and global defaults across settings reads, model inventory reads, and settings writes;
  • added resolve and reject race coverage proving a late save from one project cannot replace another project's rows, baseline, warnings, errors, dirty state, or saved state.

The route integration test now uses condition-based waits and covers project-to-project and project-to-global route transitions. Required verification also exposed two stale Anthropic model references in tests after the generated registry dropped those IDs; the provider abort test now uses a current model, while the threshold test retains explicit legacy-ID coverage through a synthetic model fixture.

Verification passed: build, 248 focused dashboard tests, focused AI tests, full test suite, type-check, Biome, workspace-link verification, diff checks, and the pre-commit suite with 5,525 tests passed and 0 failed.

Commit: d0504e3


Progress tracked by mach6

@m-aebrer
m-aebrer merged commit 6181f0f into master Aug 6, 2026
3 checks passed
@m-aebrer
m-aebrer deleted the feature/issue-404-dashboard-scoped-models branch August 6, 2026 14:02
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.

Add scoped-model configuration to dashboard settings

1 participant