fix(config): project contextWindow/maxTokens onto flat AppConfig - #326
fix(config): project contextWindow/maxTokens onto flat AppConfig#326AmirF194 wants to merge 1 commit into
Conversation
normalizeProfile already preserves a profile's contextWindow/maxTokens (since 364b0e3), but normalizeConfig() and composeProjectedConfig() both build the flat AppConfig from projectFromConfigSet()'s projected object and drop these two fields, so getAll().contextWindow/maxTokens is always undefined for a custom/Ollama profile. agent-runner.ts reads exactly those two flat fields to size the synthetic-model fallback, so a user-configured context window or max-output-tokens override never reaches the running agent. buildAgentRuntimeSignature() also omitted both fields, so changing only contextWindow/maxTokens left a running session on the old runtime config until an unrelated field changed.
There was a problem hiding this comment.
Findings
-
[Minor] The positive-number guard is repeated in both projection paths and the two paths already behave differently (
normalizeConfigleaves the key absent, whilecomposeProjectedConfigexplicitly deletes it), so the logic can drift if another optional numeric field is added later. The check also acceptsInfinity, despite thevalid positive numberintent. Centralize it, insrc/main/config/config-store.ts:1021-1026and:1055-1064.Suggested fix:
const setOptionalPositiveNumber = <K extends 'contextWindow' | 'maxTokens'>( result: Partial<AppConfig>, key: K, value: number | undefined, ) => { if (typeof value === 'number' && Number.isFinite(value) && value > 0) { result[key] = value; } else { delete result[key]; } };
Then use
setOptionalPositiveNumber(result, 'contextWindow', projected.contextWindow)in both methods (theelsebranch is a safe no-op innormalizeConfigbecause the fresh object has no stale key).
Questions
None.
Summary
Review mode: initial
Review policy: advisory — the check reflects automation health/completion only; it does not approve the PR or resolve findings.
The change correctly propagates optional contextWindow and maxTokens from the active profile into both normalizeConfig() and composeProjectedConfig(), and adds them to the agent runtime signature. No blockers or major correctness issues found. The one maintainability concern above is minor. Residual risk: the new signature test scans source text rather than exercising the function, and the mocked electron-store does not emulate electron-store's rejection of explicit undefined, so a regression that unconditionally writes contextWindow: undefined would not be caught by the current mock.
Testing
Not run (automation). Suggested additions: make the MockStore.set in src/tests/config-store-context-window-projection.test.ts throw when a value is undefined, and consider a behavioral test for buildAgentRuntimeSignature (for example by exporting it) instead of a source-text scan.
Open Cowork Bot
|
No rush, just checking in after a week. CI is green and the branch is clean against main whenever you get a chance to look. |
Summary
normalizeConfig()andcomposeProjectedConfig()build the flatAppConfigobject theagent runner reads from
configStore.getAll(). Both functions read every other projectedfield from
projectFromConfigSet()'s return value exceptcontextWindowandmaxTokens,so a user's saved context-window/max-tokens override for a custom or Ollama provider profile
is silently dropped: the agent runner falls back to a default or auto-probed value instead of
what the user configured.
buildAgentRuntimeSignature()(src/main/index.ts) has the samegap, so even a user who edits only these two fields will not get the running session reloaded
with the new config, since the signature it hashes never changes.
Fixes #262.
Fix
Project both fields through in
normalizeConfig(),composeProjectedConfig(), andbuildAgentRuntimeSignature().normalizeProfile()(a few lines abovenormalizeConfig()in the same file) alreadyestablishes the right pattern for this exact situation, with the comment "preserve optional
numeric fields so callers don't silently lose user-set values": assign the field only when it
is a valid positive number, never write it unconditionally. That matters here specifically
because
contextWindow/maxTokensare genuinely absent on most profiles (no default profilesets them), and
electron-store's underlyingconfpackage throws on.set()if a key ispresent with an explicit
undefinedvalue, unlike a plain object orJSON.stringify, whichboth drop it silently. A first pass at this fix that assigned the fields unconditionally
(
contextWindow: projected.contextWindow) crashedConfigStore's constructor for any profilethat never set them, which is the common case;
composeProjectedConfig()additionally spreads...base, so switching from a profile with an override to one without needs an explicitdelete, or the old value leaks into the new config set's projection.Testing
tests/config-store-context-window-projection.test.ts(new): fails onmain(both thedrop itself and the constructor crash on a profile with no override), passes on this
branch. Covers projection, persistence across a config-set switch, and clearing the value
when switching to a set whose profile has no override.
tests/index-agent-runtime-signature.test.ts(new): fails onmain, passes on this branch.npx vitest run, 154 files / 1108 tests pass.npx tsc --noEmitandnpx eslint src --ext .ts,.tsxboth pass with no new errors or warnings.buildAgentRuntimeSignature()gap as observed through a real runningsession reload (the fix is a one-line addition mirroring the six adjacent fields already
handled the same way, but I did not drive the actual reload path end to end).