Settings → Agent: Advanced Settings section for agent step budget and auto-continues

[ianu82]

What

Adds a collapsed Advanced Settings group at the bottom of Settings → Agent exposing the two new cowork-server agent budgets (mindsdb/cowork-server#245):

• Max steps per task (5–500, default 50) — "How many actions (running code, reading files, searching) Anton may take on one request before pausing to check in with you. Raise it so big tasks finish in one go; lower it for a tighter leash on time and cost."
• Max auto-continues (0–25, default 5) — "When Anton stops but the work looks unfinished, Cowork sends it back to complete the job — this caps how many times. Raise it for hands-off thoroughness; set 0 to always stop at the first draft."

Each input shows its range and default inline (5–500 · default 50) and participates in the page's normal Save flow.

Design decisions (each one maps to a bug found in review)

The inputs follow a deliberate commit policy, extracted into pure, directly-tested functions (clampBudgetValue / clampBudgets in settingsTransform.js):

• Values are strings end-to-end. Server rows arrive as strings and the page-wide dirty compare is a JSON diff, so types must survive the save → re-fetch round trip unchanged.
• An untouched field never commits. Blur alone must not materialize a key the server never sent — that flipped Save to dirty with zero edits, and would PUT an unknown key to an older server (the renderer ships OTA and can lead the server).
• An emptied field reverts to the last committed value, never the factory default. Clearing a saved 500 to retype must not save 50.
• save() clamps as a backstop. Escape-dismiss skips React's blur, so a raw draft (e.g. 9999) can survive in app state; clampBudgets() guarantees no PUT ever carries an out-of-range value.
• Empty/unparseable drafts are dropped from the write entirely. "No instruction" ≠ "reset to default": dropping the key means the server keeps its stored value and the post-save re-fetch heals the display.
• Number() not parseInt (number inputs accept 5e2, which means 500, not 5), and the range hint is aria-describedby-linked for screen readers.

Version skew

• New UI + old server: the section renders with defaults but never writes unknown keys — harmless.
• Old UI + new server: settings aren't shown; the new 50/5 defaults still apply (that part ships entirely in the server PR).

Testing

10 new cases in settingsTransform.test.js covering the transform mapping, diff behavior, clamping (including scientific notation, whitespace, revert-to-previous, drop-don't-default, no-materialization, reference stability). On this branch rebased onto staging: 590/590 tests, typecheck clean.

The diff was adversarially reviewed by independent review agents in two rounds; the commit policy above is the direct result (three MAJOR findings in round one, and round two caught that the first version of the save-time clamp turned a loud server 422 into silent data loss — hence drop-don't-default).

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a82c75b400

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep draft edits out of the committed-value ref

When a saved value such as 120 is edited to 300 and then cleared before blur, this effect has already replaced lastValidRef with the unsaved 300; the empty-field blur therefore restores and commits 300 instead of the last server-committed 120. Out-of-range drafts similarly restore their clamped draft rather than the stored value, so the ref should only be refreshed from committed/server state rather than every parseable change.

Useful? React with 👍 / 👎.

[ianu82]

Good catch — fixed in cfa5a78. The ref was tracking the latest parseable draft, which contradicted the documented revert-to-committed policy (clearing an unsaved 300 restored the 300 over the saved 120, and an abandoned out-of-range draft came back as its clamped form). The component now takes the committed value from the page's saved-state snapshot (lastSavedJson) via a savedValue prop, and the draft-tracking ref is gone. Empty-with-no-saved-value still commits nothing (the save-time clampBudgets() drops empty drafts from the write).

[alecantu7]

Reviewed at cfa5a78f (post-Codex-fix) in a clean worktree: settingsTransform.test.js 40 passed, full suite 567 passed, typecheck clean. (Two renderer suites can't load in my environment — remark-math missing from my node_modules — which accounts for the gap to your 590. Not a PR issue.)

The commit-policy work is genuinely careful and I checked the parts most likely to be subtly wrong — they're right:

• clampBudgetValue handles the whole nasty input space correctly: ''/whitespace guarded before Number() (so no Number('') === 0 trap), 'abc' → prev, Math.round so a typed 50.7 can't 422 as a float, '5e2' → 500, and Infinity/'1e400' fall through the NaN guard into Math.min(max, …) → clamps to max rather than exploding.
• clampBudgets preserves reference identity when nothing changes, and its drop-don't-default rule is the right call — turning a loud 422 into silent data loss was a real trap and you avoided it.
• The Codex fix is correct: sourcing savedValue from the lastSavedJson snapshot rather than a draft-tracking ref genuinely implements the documented revert-to-committed policy.
• Cross-repo contract matches exactly. cowork-server#245 declares max_tool_rounds (ge=5, le=500, default 50) and max_continuations (ge=0, le=25, default 5) — same names, same bounds, same defaults as BUDGET_FIELDS and the inline hints.

The one thing I'd change before this merges is the version-skew story, which is the weakest part of an otherwise tight PR. The claim is:

New UI + old server: the section renders with defaults but never writes unknown keys — harmless.

That holds only while the user doesn't touch the fields — which is the feature's entire purpose. diffSettingsForWrite has no "was this key in the fetched snapshot" guard; it skips only when prev === value, and on an old server prev is undefined. Probe against this branch:

writes -> {"max_tool_rounds":"200"}     // lastFetched has no budget keys; user typed 200
untouched writes -> {}                  // untouched really is safe

On the server side that's not ignored: SettingsService._validate_key raises ValueError(f"Unknown setting: '{key}'") and PUT /settings/{key} maps it to 400.

Also worth correcting the second half of that section — "a failed key fails the whole multi-key save" isn't what updateSettings does. It loops over every key, collects failures, and throws an aggregate at the end, so the actual outcome is subtler and worse than a clean abort: every other setting in that save does commit, the user gets Failed to save settings: max_tool_rounds: … Unknown setting, setLastSavedJson is never reached so the page stays dirty over already-persisted changes, and the post-write _lastFetchedSettings re-fetch is skipped, leaving a stale snapshot for the next diff.

Why this is live and not hypothetical: cowork-server#245 is still open. And as this PR itself notes, the renderer ships OTA and can lead the server — so "merge #245 first" isn't a sufficient control, because what matters is the version of cowork-server actually installed on the user's machine, not what's on staging. That's exactly why the fix should be a capability gate rather than a merge-order note. Detail inline; it's a few lines and it collapses the 400, the partial-save-reported-as-failure, and the stale-snapshot symptom into nothing.

[alecantu7]

This renders unconditionally, which is what makes the old-server path reachable. Suggest gating it on the server actually serving the keys.

There's an exact capability probe available: cowork-server's list_settings returns a row for every UserSettings.model_fields entry, so a server that has #245 always sends both keys and one that doesn't never does. That makes presence in the fetched snapshot a reliable signal:

const hasBudgets = lastSavedSettings != null
  && 'maxToolRounds' in lastSavedSettings
  && 'maxContinuations' in lastSavedSettings;
…
{hasBudgets && <CollapsibleGroup title="Advanced Settings" …>}

(lastSavedSettings is the first snapshot of the fetched settings, so it's a clean stand-in for "what the server sent" without threading a new prop.)

That's strictly better than a merge-order note on #245, because the renderer ships OTA and can lead the installed server by days — the gate is correct for every skew combination, including a user who takes a UI update but whose cowork-server auto-update hasn't run yet.

It also makes the "never writes unknown keys" invariant true by construction instead of true-only-if-untouched, so the clampBudgets docstring at settingsTransform.js:535 stops overselling what the layer below actually enforces.

[ianu82]

Implemented in 3899567 exactly as suggested — hasBudgetSettings checks both keys in lastSavedSettings and gates the whole CollapsibleGroup. Agreed it's strictly better than the merge-order note: correct for every skew combination including OTA-leads-installed-server, and it makes the never-write-unknown-keys invariant structural. Thanks for pointing out that list_settings serves every model field — that's what makes presence a clean capability probe.

[alecantu7]

Keys the server never sent stay absent: materializing them here would create a phantom write — and a failing one on an older server.

The reasoning is exactly right, and clampBudgets does honour it (if (v == null) continue). But it's worth knowing the invariant isn't enforced anywhere downstream: diffSettingsForWrite writes any key whose value differs from lastFetched[clientKey], and on an older server that's undefined — so the moment the user edits the field, the key is materialized by setSetting and this function passes it straight through to a PUT that 400s.

So the guarantee currently rests entirely on the user not touching the input. The UI gate I suggested on SettingsView.jsx:1956 is the cheapest way to make it structural.

A presence guard here (if (!(clientKey in lastFetched)) continue;) would also do it, but I'd avoid that one: it's a global change to every setting's write path, and _lastFetchedSettings falls back to MOCK_DATA.settings when the backend is offline, so "absent from the snapshot" isn't always "absent on the server." Gating the UI is the narrower fix.

[ianu82]

Right — the invariant only held while the field was untouched. In 3899567 the UI gate closes the main path, and I also took the budget-scoped presence guard in diffSettingsForWrite (not the global one, for exactly the mock-fallback reason you flagged — the comment in the code says so) as defense in depth: the gate keeps users from editing the field, the guard keeps a materialized key out of the PUTs even if some future path materializes one. clampBudgets' docstring now describes both layers instead of overselling its own.

[alecantu7]

This is the one place the old-server case could have been caught, and the fixture rules it out: lastFetched is passed with both keys ({ maxToolRounds: '50', maxContinuations: '3' }), so it only exercises the new-server path.

Worth one more assertion pinning the behaviour you actually want once the gate is in — that a budget key absent from lastFetched produces no write:

expect(diffSettingsForWrite({ maxToolRounds: '200' }, { theme: 'dark' })).toEqual({});

As of this commit that assertion fails ({ max_tool_rounds: '200' }), which is precisely the finding — so it doubles as the regression test for whichever fix you pick.

[ianu82]

Added your assertion verbatim in 3899567 (plus a companion asserting non-budget keys keep the old writable-when-absent behavior). It failed before the diff-layer guard and passes after, so it pins the fix exactly as you intended.

[alecantu7]

Minor, and it disappears with the gate: on a server without these settings this renders 50 / 5 as though they were the effective values, when the server has no such setting at all. The numbers do become true once #245 ships (they're the server-side defaults), so it's a forward-looking rather than wrong display — but until then it's the UI asserting state that doesn't exist yet.

Also a small a11y nit while you're here: aria-label={label} duplicates the visible <Section title> text the input already sits under, so screen readers announce the name twice. The aria-describedby range hint is a nice touch and worth keeping.

[ianu82]

The fake-50/5 display is gone with the gate — on a server without the settings the section no longer renders at all. On the aria-label: I looked at dropping it, but the Section title is a bare h3 with no programmatic association to the input (no label element, no aria-labelledby), so removing aria-label would leave the input nameless in forms-mode/focus navigation — the double announcement only happens in browse mode passing over the heading first. Kept the label; if we later give Section an id-wired title, switching to aria-labelledby would fix the duplication properly. Also fixed the max-auto-continues subtitle copy here to match the corrected #245 wording (0 = stop after first attempt, with a summary of what's missing).

[alecantu7]

Re-reviewed at 38995679. settingsTransform.test.js → 41 passed, full renderer+main suite → 568 passed, typecheck clean. (Two renderer suites don't load in my env — remark-math missing from my node_modules — same as last time, not this PR.)

The version-skew hole is closed properly, and in both layers:

• hasBudgetSettings gates the whole group, so the section can't render against a server that would 400 the write.
• The budget-scoped guard in diffSettingsForWrite sits before the prev read and can't affect any other key — and the new test proves both directions ({maxToolRounds:'200'} vs absent snapshot → {}, and {greeting:'hi'} vs {} → still writes). That's the assertion that failed before the guard and passes now, so the regression is pinned where it should be.
• Copy fix landed, and it matches the corrected #245 wording you shipped in b3537b8 — nice that the two PRs are consistent now rather than drifting.

Two corrections I owe you, one of which is now baked into a code comment because of my review.

1. My mock-fallback claim was wrong. I said _lastFetchedSettings "falls back to MOCK_DATA.settings when the backend is offline." It doesn't — api.js:998 initialises it to {}, it's only ever assigned inside the successful branch (:1044) and after a save's re-fetch (:1085), and the offline catch returns the mock object without assigning it. So the mock never becomes lastFetched.

Your conclusion (scope the guard to budgets) is still right, just for a different reason: _lastFetchedSettings starts as {}, so before the first successful fetch every key looks absent, and a global rule would silently turn the first save into a no-op instead of a write. Worth rewording the comment at settingsTransform.js:487 — it currently states a premise that isn't true of this codebase, and the next person to touch that line will reason from it.

2. You're right about aria-label and I was wrong. I hadn't checked that Section renders a bare h3 with no id/aria-labelledby wiring, so dropping the label really would leave the input nameless in forms-mode navigation — a worse outcome than a duplicate announcement in browse mode. Keeping it is correct, and the aria-labelledby idea is the right follow-up if Section ever gets an id-wired title.

One small residual thing on the gate inline — not blocking, and it only bites in a narrow case.

[alecantu7]

The probe is right; the source it reads can latch. lastSavedSettings is derived from lastSavedJson, which is snapshotted exactly once (if (lastSavedJson === null && settings && Object.keys(settings).length > 0)). So whichever settings object this view first sees decides hasBudgetSettings for the lifetime of the mount.

That matters in one case: if Settings is opened while the backend is down, App is serving MOCK_DATA.settings, which has no budget keys (I checked — they're absent from the mock). The snapshot latches without them, and the section stays hidden for the rest of that mount even after a successful fetch brings the real rows in. Recovering needs a remount (navigate away and back).

Gating on the live settings prop instead avoids the latch and is equally safe:

const hasBudgetSettings = settings != null
  && 'maxToolRounds' in settings
  && 'maxContinuations' in settings;

The safety argument still holds because the only writer that could materialize these keys is setSetting from the budget field itself — which is inside the gate. So on an older server the keys can never appear in settings, and on a newer one they're present from the first fetch. savedValue={lastSavedSettings?.…} should stay as-is; that one genuinely wants the committed snapshot.

Entirely fine to leave if you'd rather not touch it — the failure mode is a hidden section in an offline-open, which is arguably reasonable behaviour anyway.

[alecantu7]

This is the comment carrying my incorrect premise — worth fixing while it's cheap, since it reads as the justification for the scoping decision.

lastFetched here is _lastFetchedSettings, which is {} at api.js:998 and is assigned only on a successful fetch (:1044) or a post-save re-fetch (:1085). The offline path return { ...MOCK_DATA.settings, … } returns the mock to the caller without ever assigning it, so lastFetched is never mock data.

The accurate reason to keep this budget-scoped is the empty-initial-state one: {} before the first successful fetch means every key looks absent, so as a global rule this would turn the first save of a session into a silent no-op. Same decision, true premise.

Suggested rewording:

// Deliberately budget-scoped: `lastFetched` is {} until the first
// successful fetch, so as a global rule this would silently drop the
// first save of a session. For budget keys the trade is worth it —
// the server returns a row for every settings field, so absence really
// does mean an older server that would 400 the write.