feat(agent): configure subagent models and role presets - #365
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review mode: initial
Findings
-
[Major] Config load can throw and brick startup on invalid persisted
subagentdata —src/main/config/config-store.ts: the newsubagent: normalizeSubagentConfig(raw.subagent)runs inside the config load/getAllpath, andnormalizeSubagentConfigthrows on any validation failure (src/shared/subagent-config.ts). For new installsraw.subagentisundefinedso the default applies, but a hand-edited/corrupted config, or a stored"subagent": null(which bypasses the= DEFAULTdefault because the value isnull, notundefined) will make everyconfigStore.getAll()call throw. BecausegetAll()is used broadly (e.g.src/main/agent/subagent-extension.ts), a throw here can break session creation and app usability, and the UI has no way to recover since it also depends on config loading.
Suggested fix:// config-store.ts (load path) let subagent: SubagentConfig; try { subagent = normalizeSubagentConfig(raw.subagent); } catch (error) { log(`[ConfigStore] Invalid subagent config, falling back to default: ${String(error)}`); subagent = normalizeSubagentConfig(undefined); } // ...return { ...rest, subagent }
Alternatively make
normalizeSubagentConfigcoerce/repair during load and keep strict validation only on the save path. -
[Minor] Invalid
subagentpayload makes the entire config save fail —src/main/config/config-store.ts:subagent: normalizeSubagentConfig(updates.subagent ?? current.subagent). When the renderer sends an invalid draft, normalization throws and the wholesave/updatecall fails, discarding unrelated field updates in the same call. The UI surfaces an error, but the blast radius is larger than the subagent section. Consider validating/isolating thesubagentfield so unrelated updates still persist, or reject only that field. -
[Minor] Restrict-tools toggle with an empty list silently disables all tools —
src/renderer/components/.../SettingsSubagents.tsxsendsallowedTools: entry.allowedTools?.map(t => t.trim()).filter(Boolean); checking "restrict tools" and leaving the field blank results inallowedTools: [], andisAllowedinsrc/main/agent/subagent-extension.tstreats an empty array as "match nothing" (!allowed_toolsis false,includes(name)is false), so the role gets zero tools with no warning. This matches the documented empty-list semantics but is an easy footgun.
Suggested fix:// renderer: only emit a restricted list when non-empty, otherwise undefined allowedTools: entry.allowedTools?.length ? entry.allowedTools.map((t) => t.trim()).filter(Boolean) : undefined,
or add inline UI validation when restriction is enabled but no tools are listed.
-
[Minor] Normalized
allowedToolscan retain empty strings —src/shared/subagent-config.ts:allowedTools: preset.allowedTools?.map((tool) => tool.trim())trims but does not filter. A value like[' ']passes theminLength: 1typebox check (original string length > 1) and becomes['']after trim. Harmless for matching but inconsistent with the renderer (.filter(Boolean)) and with the empty-list semantics.
Suggested fix:allowedTools: preset.allowedTools ?.map((tool) => tool.trim()) .filter((tool) => tool.length > 0),
-
[Minor] Large preset descriptions are embedded into the tool schema —
src/main/agent/subagent-extension.ts: theagentparameter description interpolates every preset's name/description, and preset instructions allow up to 1000 chars with up to 30 presets. This can inflate the tool schema and per-request token cost. Consider listing names only and documenting descriptions elsewhere. -
[Nit] New test file bypasses the documented
src/tests/convention —tests/subagent-config.test.tsmirrorssrc/shared/subagent-config.tsbut lives at thetests/root, whereas the repo convention issrc/tests/mirroring source (e.g.src/tests/agent/subagent-extension.test.ts).
Questions
- Should an invalid persisted
subagentconfig be treated as recoverable (fall back to defaults) rather than fatal? The current load-path behavior is strict.
Summary
Review mode: initial
Review policy: advisory — the check reflects automation health/completion only; it does not approve the PR or resolve findings.
The configurable child-model/role-preset/tool-restriction work is coherent, and the precedence (call override > role preset > default child model > parent model) matches the description. The main risk is robustness: normalizeSubagentConfig throws and is now called on the config load path (src/main/config/config-store.ts), so malformed persisted data can break config loading and, transitively, subagent creation. Secondary items are the empty-restrict-list footgun, empty-string retention in allowedTools, schema bloat from embedding preset descriptions, and test placement.
Testing
Not run (automation). Suggested coverage: a config-store test asserting getAll() does not throw and falls back to DEFAULT_SUBAGENT_CONFIG when subagent is null or otherwise invalid; a normalizeSubagentConfig unit test for [' '] trimming/filtering; and a renderer/share test asserting a restricted-but-empty tool list round-trips to undefined rather than [].
Open Cowork Bot
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Child execution can bypass sandbox isolation and model overrides can escape the active provider.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds configurable subagent models, role presets, tool restrictions, per-session concurrency, and a bilingual settings interface.
Changes:
- Adds validated subagent configuration and settings UI.
- Extends child runtime model, role, tool, permission, and concurrency handling.
- Refreshes cached runtimes and expands automated coverage.
| File | Description |
|---|---|
tests/subagent-config.test.ts |
Tests configuration validation. |
tests/pi-session-runtime.test.ts |
Tests cache invalidation. |
src/tests/agent/subagent-extension.test.ts |
Covers child execution behavior. |
src/shared/subagent-config.ts |
Defines defaults and validation. |
src/renderer/types/index.ts |
Exposes subagent configuration type. |
src/renderer/i18n/locales/zh.json |
Adds Chinese translations. |
src/renderer/i18n/locales/en.json |
Adds English translations. |
src/renderer/components/SettingsPanel.tsx |
Adds navigation and scrolling. |
src/renderer/components/settings/SettingsSubagents.tsx |
Implements configuration UI. |
src/main/config/config-store.ts |
Persists validated settings. |
src/main/agent/subagent-extension.ts |
Implements configurable child execution. |
src/main/agent/pi-session-runtime.ts |
Includes settings in runtime signatures. |
src/main/agent/agent-runner.ts |
Supplies settings to cache signatures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| this.getParentAbortSignal, | ||
| this.concurrencyState | ||
| this.activeCounts, | ||
| context.session.cwd || configStore.getAll()?.defaultWorkdir || process.cwd() |
| const selectedModel = model?.trim() || preset?.model || childConfig.model || config.model; | ||
| const modelString = resolvePiModelString({ | ||
| provider: config.provider, | ||
| customProtocol: config.customProtocol, | ||
| model: selectedModel, |
There was a problem hiding this comment.
Findings
-
[Minor] Unrelated config writes are blocked while a persisted subagent config is invalid —
src/main/config/config-store.ts(update()):subagentConfigError: updates.subagent === undefined ? current.subagentConfigError : undefinedkeeps the error on the merged config, andsaveConfigthrows whenever that field is set. As a resultupdate({ theme: 'dark' }), an API-key change, or a model change all fail with "Repair subagent settings before saving configuration" until the user happens to open the Subagents tab. Repair is possible (the draft falls back toDEFAULT_SUBAGENT_CONFIG, which validates), but the failure surfaces on an unrelated screen and blocks ordinary settings saves.
Suggested fix:// block only written payloads that are themselves broken if (config.subagentConfigError && updates.subagent !== undefined) { throw new Error('Repair subagent settings before saving configuration'); }
(This changes the behavior currently asserted by the config-store test, so the test would need to be updated alongside it — or, if blocking is intended, surface the repair affordance directly on the failing settings form.)
-
[Minor]
ensureNormalized()drops the entire normalization write when the subagent payload is invalid —src/main/config/config-store.ts:if (!normalized.subagentConfigError) this.store.set(normalized). Any other normalization/migration computed in the same pass is not persisted while the error stands, so unrelated drift is silently re-derived on every read.
Suggested fix (persist everything except the volatile error and the invalid raw payload):const { subagent, subagentConfigError, ...rest } = normalized; // electron-store set() merges keys, so the raw invalid `subagent` value stays intact this.store.set(subagentConfigError ? rest : normalized);
-
[Minor] "Add" preset creates a draft that can only fail at save time —
src/renderer/components/SettingsSubagents.tsx: the appended preset usesprompt: '', which violates the non-emptypromptrule enforced bynormalizeSubagentConfig. Clicking Add then Save produces a generic validation error instead of an inline indication on the offending field.
Suggested fix:setDraft({ ...draft, presets: [...draft.presets, { name: `agent-${number}`, description: '', prompt: 'You are a helpful subagent.', model: '' }] });
and/or render the per-field error next to
promptfor the selected preset so the user is pointed at it before saving. -
[Minor]
maxConcurrentinput can produce an out-of-range draft —src/renderer/components/SettingsSubagents.tsx:Number(event.target.value)turns a cleared field into0, which is belowmin, and the value is only rejected at save time.
Suggested fix:onChange={(event) => { const next = Number(event.target.value); updatePreset; // n/a – apply to draft setDraft({ ...draft, maxConcurrent: Number.isFinite(next) && next > 0 ? next : draft.maxConcurrent }); }}
(Clamp to the documented 1–8 range and keep the previous value when the field is emptied.)
-
[Minor] Non-null assertion on the concurrency map can silently disable the throttle —
src/main/subagent/subagent-extension.ts(finallyblock):const remaining = activeCounts.get(parentSessionId)! - 1;. If the key is ever absent (e.g. a future early return moved between the check and thetry),undefined - 1isNaN,activeCounts.set(key, NaN)persists, andactiveCount >= childConfig.maxConcurrentbecomes permanently false for that session — the limit stops working with no error.
Suggested fix:const remaining = (activeCounts.get(parentSessionId) ?? 1) - 1; if (remaining <= 0) activeCounts.delete(parentSessionId); else activeCounts.set(parentSessionId, remaining);
-
[Nit] Runtime signature and runtime behavior read the subagent config from different sources —
buildPiSessionRuntimeSignatureusesconfigStore.get('subagent')(raw persisted value, which can still be the invalid payload) while tool creation uses the normalizedgetAll().subagent. The signature can therefore differ from what the child actually runs, causing unnecessary invalidation/re-resolution.
Suggested fix: feed the same normalized value into both, e.g. computeconst subagent = config.subagentConfigError ? undefined : normalizeSubagentConfig(config.subagent)once and pass it to the signature builder. -
[Nit] Raw config error text is echoed into model-visible tool output —
Subagent configuration error: ${config.subagentConfigError}. Repair it in settings.interpolates a string derived from stored, user-editable values (e.g. an unknown preset name) into the agent's context.
Suggested fix: return a fixed message to the agent and log the specific cause locally; keep the detailed text for the settings UI only.
Questions
src/main/subagent/subagent-extension.ts:new Map([...createCodingTools(cwd), ...createReadOnlyTools(cwd)].map(...))means later entries win, so read-only variants override same-named coding tools (e.g.read) for every child session, including the unrestricted default/implementer role. Is that ordering intentional, and are the two implementations identical for shared names? Not verifiable from the diff alone — if the read-onlyreadis more restrictive, this silently changes behavior for all subagents, not just restricted presets.
Summary
- Review mode: follow-up after new commits
- Review policy: advisory — the check reflects automation health/completion only; it does not approve the PR or resolve findings.
- No blockers found. The main risk introduced in this diff is the config-write coupling: once an invalid
subagentpayload is persisted, every settings save fails until the user repairs that one tab, andensureNormalized()also stops persisting unrelated normalization while the error stands. - Residual risks / testing gaps: the
allowedTools: []=> "no tools" semantics and empty-string handling are guarded in the renderer UI only, so any non-UI writer of the config can still produce a child with zero tools; the concurrency-slot bookkeeping relies on the increment/decrement pairing staying intact across future edits; the new synthetic-model fallback path replaces the removed!piModelerror, so a genuinely unresolvable model now degrades to a synthesized provider/id instead of failing fast — worth confirming that is the desired failure mode.
Testing
- Not run (automation).
- Suggested: a config-store test asserting that unrelated updates succeed (or fail with a clearly-scoped message) while
subagentConfigErroris set, plus a round-trip test that a validsubagentupdate clears the error and restores writes; a unit test for thefinallyrelease path asserting the key is deleted at zero and never left atNaN; and a test covering the synthetic-model fallback (id/provider/api/baseUrl) for bothcustomand non-official OpenAI base URLs.
Open Cowork Bot
There was a problem hiding this comment.
Findings
-
[Nit] Config error text is echoed into model-visible tool output —
config.subagentConfigErrorcan contain user-supplied preset names (e.g.Unknown default subagent: <name>), so the message surfaces user input to the model as tool output. Evidence:src/main/agent/subagent-extension.ts(spawn_subagent error branch:`Subagent configuration error: ${config.subagentConfigError}. Repair it in settings.`).
Suggested fix:text: sandboxIsolated ? 'Subagents are unavailable in sandbox sessions...' : 'Subagent configuration error. Repair it in Settings > Subagents.',
(log the detailed
config.subagentConfigErrorinternally instead of returning it to the model). -
[Nit] Runtime signature is built from the raw persisted subagent rather than the normalized value —
buildPiSessionRuntimeSignaturereceivesconfigStore.get('subagent'), which can be an invalid/nullraw value while the runtime actually executes with the normalizedconfigStore.getAll().subagent. This can make the signature diverge from the executed runtime (and from whatgetAll()reports). Evidence:src/main/agent/agent-runner.ts(subagent: configStore.get('subagent')).
Suggested fix:// use the same normalized view that the runtime actually uses subagent: configStore.getAll().subagent,
Summary
Review mode: follow-up after new commits
This pass re-verified the previously flagged items against the current changed lines:
- The concurrency accounting was changed from a
Map<string, number>with a non-null assertion toMap<string, Set<string>>; slot add/remove happens synchronously around theexecuteawait boundaries and the empty-set cleanup (if (active.size === 0) activeSubagents.delete(parentSessionId)) reads back the sameSetreference stored in the map, so the prior concern is resolved. saveConfignow preserves an invalid persisted subagent for repair while persisting unrelated fields, andupdate({ subagent })still throws on invalid input before anystore.set, so the earlier "blocking unrelated writes" concern is resolved.
The two remaining items above are low-severity (Nit): one is an information-echo into model-visible output, the other a signature/raw-vs-normalized mismatch. No Blocker/Major issues were confirmed in the current diff.
Review policy: advisory — the check reflects automation health/completion only; it does not approve the PR or resolve findings.
Testing
- Existing Vitest coverage in
src/tests/exercises concurrency slot reuse, invalid-subagent recovery, and the permission-hook registration; no additional tests are strictly required for the Nits above. - Not run (automation).
Open Cowork Bot
Summary
Add configurable child models, named role presets, tool restrictions, and per-parent-session concurrency to the existing
spawn_subagentruntime. This lets users choose a model for focused child work and keep reviewer/researcher roles read-only without duplicating agent execution.Based on
main(4c2cdfc). No new dependencies. Sandbox sessions explicitly reject host-tool subagents until child tools support sandbox isolation. Invalid persisted subagent settings remain intact and can be repaired in settings; unrelated config saves are rejected until repaired.Type of change
feat)Checklist
Testing
--noEmit, ESLint, changed-file Prettier, andgit diff --checkpassed.Tests cover model resolution, role selection and validation, tool restrictions, permission decisions, working directory, concurrency release, SDK progress/errors, and runtime invalidation.