Expose settings read and write over RPC

[m-aebrer]

Closes #314

Adds a typed settings read/write surface to RPC mode: a get_settings command returning a snapshot of dashboard-relevant persistent defaults, and a set_settings command for validated writes to persistent defaults via SettingsManager — without touching live session state.

Implementation plan posted as a comment below.

[m-aebrer]

Implementation Plan

Analysis

RPC mode currently has six per-runtime setters (set_model, set_thinking_level, set_steering_mode, set_follow_up_mode, set_auto_compaction, set_auto_retry) but no way to read settings, and no way to write persistent defaults without also mutating the live session.

Codebase findings that shape the design:

• The existing setters already persist. Every AgentSession setter these commands call also writes through to SettingsManager (e.g. setSteeringMode → settingsManager.setSteeringMode). So the real gaps are: (1) no read surface at all, and (2) no way to change a persistent default without switching the live session (e.g. set the default model for next startup while the current session keeps running on its current model).
• SettingsManager already has every typed getter/setter needed (getDefaultProvider/Model, getDefaultThinkingLevel, getSteeringMode, getFollowUpMode, getCompactionEnabled, getRetryEnabled and their set counterparts). This is plumbing plus DTO design, as the issue notes.
• No runtime validation exists at the RPC boundary today — incoming JSON is cast as RpcCommand with zero schema checking. A settings write command handling client-supplied payloads must validate explicitly and return descriptive errors (no silent clamping — note the existing set_thinking_level path clamps silently via AgentSession; the new persistent write will NOT clamp, it will reject).
• Silent-save hazard: SettingsManager.save() silently no-ops when the settings file failed to parse at load (globalSettingsLoadError). A dashboard writing settings through RPC would get success: true while nothing persists. The write handler must detect this and return an explicit error. Write-queue I/O errors are recorded via drainErrors() — the write handler flushes and drains so persistence failures surface as RPC errors instead of vanishing.
• Prior art (LSP): the Language Server Protocol moved from push-only config to an explicit pull model (workspace/configuration) because servers couldn't know effective settings otherwise — same lesson applies here: the dashboard needs get_settings, not just write commands.

Design

Two new commands, following the established RPC patterns from the session-listing and tree work:

1. get_settings — read-only snapshot of persistent defaults:

{"type": "get_settings"}
→ {"type": "response", "command": "get_settings", "success": true, "data": {
     "defaultProvider": "anthropic",
     "defaultModel": "claude-sonnet-4-5",
     "defaultThinkingLevel": "high",        // may be absent if never set
     "steeringMode": "one-at-a-time",
     "followUpMode": "one-at-a-time",
     "compactionEnabled": true,
     "retryEnabled": true
   }}

Values come from SettingsManager getters (merged global+project view), NOT from live session state. Clients get live state from the existing get_state — the docs will spell out this distinction (persistent defaults vs current runtime), making it legible per the issue's requirement.

2. set_settings — single command with a strictly validated partial payload (all fields optional, at least one required):

{"type": "set_settings", "settings": {"defaultThinkingLevel": "low", "retryEnabled": false}}
→ {"type": "response", "command": "set_settings", "success": true, "data": { ...full snapshot, same shape as get_settings }}

• Persistent-only: writes via SettingsManager setters only; never touches Agent/AgentSession runtime state. Clean boundary: existing per-runtime commands = "change now (+persist)", set_settings = "change defaults only".
• Validate everything, reject loudly:
  • Unknown keys in the payload → error naming the key
  • defaultThinkingLevel must be one of off|minimal|low|medium|high|xhigh (validated against the full union — it's a stored default, not tied to the current model's capabilities)
  • steeringMode/followUpMode must be all|one-at-a-time
  • compactionEnabled/retryEnabled must be boolean
  • defaultProvider/defaultModel must be supplied together and must match a model in modelRegistry.getAvailable() (same validation as the existing set_model handler); written atomically via setDefaultModelAndProvider
  • Empty/missing settings object → error
• Atomic: validate the entire payload first; apply nothing if any field fails.
• Durable + loud on failure: after applying, await settingsManager.flush() and check drainErrors(); if the settings file had a load error (which makes save() a silent no-op) or the write failed, return success: false with a descriptive message. This needs one small SettingsManager addition to expose the load-error state (e.g. a hasLoadError()-style accessor) since globalSettingsLoadError is currently private.

A single set_settings is chosen over six set_default_* commands because the dashboard settings tab saves multiple fields at once; atomic multi-field validation gives it one round-trip and one error surface. The response returning the full snapshot saves the client a follow-up get_settings.

Validation value sets will be defined once (shared constants/type guards next to the DTO in rpc-types.ts or reused from existing helpers like isValidThinkingLevel in cli/args.ts) so the unions can't drift from the handler.

Deliverables

File	Change
packages/coding-agent/src/modes/rpc/rpc-types.ts	RpcSettingsSnapshot DTO interface; get_settings + set_settings command variants; two success-response variants
packages/coding-agent/src/modes/rpc/rpc-mode.ts	Exported, unit-testable handlers getSettingsForRpc(...) and setSettingsForRpc(...) (following the deleteSessionForRpc pattern) + two switch cases
packages/coding-agent/src/modes/rpc/rpc-client.ts	getSettings(): Promise<RpcSettingsSnapshot> and setSettings(settings: Partial<...>): Promise<RpcSettingsSnapshot>
packages/coding-agent/src/modes/rpc/index.ts + packages/coding-agent/src/index.ts	Export new DTO type(s)
packages/coding-agent/src/core/settings-manager.ts	Small accessor exposing the global-settings load-error state so the write handler can fail loudly instead of silently no-op saving
packages/coding-agent/docs/rpc.md	New ### Settings section under ## Commands: both commands, request/response JSON, full validation-error list, explicit "persistent defaults vs runtime state" explanation, cross-link to docs/settings.md (no key-by-key duplication)
packages/coding-agent/CHANGELOG.md	[Unreleased] → ### Added entry
packages/coding-agent/test/rpc-settings-commands.test.ts	New test file (see below)
packages/coding-agent/test/rpc.test.ts	One live e2e test: get → set → get round-trip
Root README.md / packages/coding-agent/README.md	Check RPC command mentions; update if the commands surface there

Acceptance criteria

• get_settings returns the seven-field snapshot reflecting SettingsManager state
• set_settings persists via SettingsManager, visible in subsequent get_settings reads and in a fresh SettingsManager instance reading the same file
• Live session state (current model, thinking level, modes) is untouched by set_settings
• Every invalid input case returns success: false with a descriptive error naming the offending field/value; nothing is applied on any failure
• Settings-file load errors and write failures produce explicit RPC errors, not silent success
• RpcClient methods round-trip both commands
• docs/rpc.md documents the surface including the persistent-vs-runtime distinction

Testing approach

New test/rpc-settings-commands.test.ts (no subprocess, no API key — follows rpc-session-commands.test.ts patterns, using SettingsManager.inMemory() / SettingsManager.create(projectDir, agentDir) with temp dirs):

• Read: snapshot reflects defaults; reflects prior writes; absent defaultThinkingLevel handled
• Validation errors: invalid thinking level, invalid steering/follow-up mode, non-boolean toggle, unknown key, provider without model (and vice versa), unavailable provider/model combo, empty payload
• Atomicity: one valid + one invalid field → error, valid field NOT applied
• Persistence: write via handler → flush() → new SettingsManager over the same agentDir reads the written values (fresh-runtime simulation)
• Loud failure: corrupt settings file on load → set_settings returns explicit error
• RpcClient: mocked-send tests — correct command shape sent, data unwrapped, error message thrown (rejects.toThrow)

Plus one live e2e test in rpc.test.ts (spawns built CLI, auto-skipped without API key): get_settings → set_settings → get_settings verifying the round-trip through the real wire protocol.

Risks and open questions

• Enum drift: thinking-level/mode unions exist in several places (agent types, settings-manager signatures, cli/args). Mitigation: validators defined once and reused.
• Scope note: writes target global scope only (matching every existing SettingsManager core setter — project-scoped writes don't exist for these fields and stay out of scope).
• Snapshot growth: issue notes the exact field list may be finalized against the UX spec from the dashboard design issue. The DTO is a plain interface — adding fields later is additive and non-breaking. Starting with the seven known-needed core fields.
• No change events: if settings change out-of-band (another process), a connected dashboard won't be notified; it re-reads on demand. Event emission is out of scope here.
• defaultProvider/defaultModel validation requires configured auth for the target provider (matches set_model behavior via getAvailable()); documented in rpc.md.

---

Plan created by mach6

[github-actions]

Vitest coverage

Metric	Covered	Total	Coverage
Statements	18514	34610	53.49%
Branches	10088	21478	46.96%
Functions	2916	5437	53.63%
Lines	16364	30783	53.15%

View full coverage run

[m-aebrer]

Progress Update

Implemented the full plan — the settings RPC surface is complete.

What landed:

• get_settings — returns the persistent-defaults snapshot (defaultProvider, defaultModel, defaultThinkingLevel, steeringMode, followUpMode, compactionEnabled, retryEnabled) from the SettingsManager merged view. Distinct from get_state (live runtime).
• set_settings — partial payload, validate-everything-first then apply (atomic: unknown keys, bad enums, non-boolean toggles, provider-without-model, unavailable model combos all reject with nothing applied). Writes persistent defaults only; never touches live session state. Returns the post-write snapshot.
• Loud persistence: new SettingsManager.hasGlobalSettingsLoadError() accessor — a corrupt settings.json (which makes save() silently no-op) now returns an explicit RPC error instead of false success. The handler also flushes and drains write errors so I/O failures surface.
• RpcClient.getSettings() / setSettings(), DTO exports (RpcSettingsSnapshot, RpcSettingsUpdate) through modes/rpc/index.ts and modes/index.ts.
• Single source of truth for enums: exported VALID_THINKING_LEVELS from cli/args and reused it in the handler, so the union can't drift.
• Docs: new ### Settings section in docs/rpc.md with the persistent-vs-runtime distinction, validation table, and full error list; CHANGELOG entry.

Tests: new rpc-settings-commands.test.ts (20 tests — read, every validation path, atomicity, disk persistence with fresh-manager read-back, corrupt-file loud failure, write-error surfacing, RpcClient wrappers) plus a live e2e round-trip in rpc.test.ts (get → set → re-get, runtime-state isolation, wire-level validation error).

Verification: build + biome clean, full workspace suite green (pre-commit ran 4010 tests, 0 failed). Smoke-tested against the real binary: snapshot read, exact validation error messages, settings.json written correctly, fresh-process read-back, and corrupt-file loud error (file left untouched) all confirmed.

Commit: 37c20b7

---

Progress tracked by mach6

[m-aebrer]

Code Review

Critical

No critical findings.

Important

1. drainErrors() scoping — stale errors cause false persistence failures
File: packages/coding-agent/src/modes/rpc/rpc-mode.ts (lines 274-281)
Agents: code-reviewer (82), error-auditor (85), test-reviewer (88)

setSettingsForRpc detects write failures by calling settingsManager.drainErrors() after flush() and treating any returned errors as persistence failures. But drainErrors() is a session-wide sink — it returns all accumulated SettingsError entries regardless of source. Other operations can leave errors in the bucket:

• session.setModel() → setDefaultModelAndProvider() → save() → failed enqueueWrite records an error but never drains it
• session.reload() → settingsManager.reload() can record load/parse failures
• A corrupt project settings file (not global) pushes an error at construction time

Result: set_settings returns ok: false, "Failed to persist settings: ..." even when its own write succeeded — because it drains an unrelated stale error. The unit test mocks drainErrors directly, hiding this cross-attribution path.

Suggested fix: Drain (and discard) pre-existing errors immediately before the apply phase, so only errors recorded between apply and flush are attributed to this operation.

---

2. Concurrent set_settings commands can silently swallow a real write failure
File: packages/coding-agent/src/modes/rpc/rpc-mode.ts (lines 272-280)
Agent: error-auditor (80)

RPC input lines are dispatched with void handleInputLine(line) — commands run concurrently. Two overlapping set_settings commands both await flush() then both call drainErrors(). The first to resume drains all errors (including the second's write failure); the second then sees an empty list and returns ok: true despite its own write having failed — a genuine silent failure that contradicts the PR's "loud persistence" design.

Suggested fix: Serialize settings writes (mutex around apply+flush+drain) or use a per-write result channel instead of the shared error list.

---

3. Settings dispatch only tested by API-key-gated e2e test
File: packages/coding-agent/test/rpc.test.ts
Agent: test-reviewer (90)

The only test exercising the actual RPC switch-case wiring (handleCommand → getSettingsForRpc(session.settingsManager) and command.settings forwarding) lives inside describe.skipIf(!process.env.ANTHROPIC_API_KEY ...). In standard CI without keys, the dispatch layer has zero coverage — a wrong field reference or mis-wired error mapping would ship green. The unit tests call the handlers directly, bypassing dispatch.

Suggested fix: Add a dispatch-level test that exercises get_settings/set_settings through the real command switch without requiring a live model (following existing patterns for other commands that don't need API calls).

Suggestions

4. "Must be strings" validation branch untested
File: packages/coding-agent/src/modes/rpc/rpc-mode.ts — typeof update.defaultProvider !== "string" branch
Agent: test-reviewer (90)

The "defaultProvider and defaultModel must be strings" error path (non-string values like { defaultProvider: 123, defaultModel: 456 }) has no test. Since RPC input is arbitrary JSON, this is reachable at runtime.

5. Partial update preservation of unrelated settings untested
File: packages/coding-agent/test/rpc-settings-commands.test.ts
Agent: test-reviewer (80)

No test verifies that a second partial setSettingsForRpc call preserves fields written by a prior call. SettingsManager.save() uses a non-trivial markModified/merge mechanism. A regression there would silently wipe unrelated persisted defaults.

6. Redundant separate export for VALID_THINKING_LEVELS
File: packages/coding-agent/src/cli/args.ts (lines 54, 60-61)
Agent: simplifier (88)

The constant is declared as const VALID_THINKING_LEVELS = ... then re-exported with export { VALID_THINKING_LEVELS }. Could simply be export const VALID_THINKING_LEVELS = ... — eliminates the indirection, behavior identical.

Strengths

• All issue 314 requirements fully delivered — completeness checker found zero gaps
• Validation is careful and correct: atomic whole-payload validation before any mutation, every error names the offending field/value
• SettingsReader/SettingsWriter type slices are precise and idiomatic — minimal surface, testable without full SettingsManager
• DTO and union additions are consistent with established RPC patterns
• hasGlobalSettingsLoadError() correctly guards the silent-no-op write path
• Comprehensive test suite: 20 unit tests covering every validation path, atomicity, disk persistence with fresh-manager read-back, corrupt-file detection, write-error surfacing, plus RpcClient wrappers and a live e2e round-trip

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

---

Reviewed by mach6

[m-aebrer]

Review Assessment

Review comment

Classifications

Finding	Classification	Reasoning
1. drainErrors scoping / stale errors cause false persist failures	genuine	drainErrors() is session-wide; other RPC commands (set_model etc. via setDefaultModelAndProvider → save) record write errors and never drain, so a later set_settings mis-attributes them and returns false ok:false. Construction-time errors are drained at startup (main.ts:669), but in-session writers are not.
2. Concurrent set_settings swallows real failure	genuine	void handleInputLine(line) runs commands concurrently; overlapping set_settings share the error bucket, so one can drain the other's failure and report false success. Same root cause as finding 1.
3. Dispatch only tested by key-gated e2e	nitpick	Matches existing pattern for all RPC commands; untested glue is a trivial 3-line switch case.
4. "must be strings" branch untested	nitpick	Reachable defensive guard, one-line test would close it, non-blocking.
5. Partial-update preservation untested	nitpick	Real coverage gap but the merge logic is correct; worthwhile regression test, not a defect.
6. Redundant separate export	nitpick	Stylistic; no dependency/ordering reason for the split.

Action Plan

1. Scope write-failure detection in setSettingsForRpc (fixes findings 1 and 2). Drain/discard pre-existing errors immediately before the apply phase so only errors recorded during this operation's flush are attributed to it. This eliminates the false-failure path (finding 1) and makes the concurrent-swallow scenario (finding 2) safe for the first-to-drain handler. For full concurrent safety, serialize settings writes with a mutex around apply+flush+drain — or document single-writer semantics.

Deferred: In-session settings writers (set_model, set_steering_mode, set_follow_up_mode, set_auto_compaction, set_auto_retry) never surface their own persistence errors — a broader loud-failure gap in the existing RPC command layer, outside this PR's scope.

---

Assessment by mach6

[m-aebrer]

Progress Update

Fixed both genuine review findings (findings 1 and 2 — same root cause: drainErrors() shared error bucket).

What changed:

• Serialized settings writes — added settingsWriteLock, a promise-chain mutex that ensures only one set_settings apply+flush+drain block runs at a time. Concurrent commands queue instead of racing on the shared error bucket (finding 2).
• Discard stale errors before applying — the handler now drains (and discards) any pre-existing errors from the shared bucket before starting its own writes. This prevents stale errors from prior commands (set_model, set_steering_mode, etc.) being mis-attributed to this operation (finding 1).
• Updated test mocks — the "surfaces write errors" test now correctly accounts for the two-drain pattern (stale discard + post-flush check).
• Added test: stale-error discarding — verifies that a stale error from a prior command does NOT cause set_settings to report false failure.
• Added test: concurrent serialization — verifies that two concurrent set_settings calls are serialized (flush-start/flush-end ordering) so errors are correctly attributed.

Verification: build clean, biome clean, 4013 tests pass (pre-commit), 2555 coding-agent tests pass (22 in settings file).

Commit: 4281d16

---

Progress tracked by mach6