feat(host-service): host agent configs (v2 PR 1, argv-array shape)#3914
Merged
feat(host-service): host agent configs (v2 PR 1, argv-array shape)#3914
Conversation
PR 1 of the canonical workspace.create() refactor — see
plans/20260425-host-agent-configs-pr1.md.
Introduces the V2 host-runtime agent config model. Configs live in
host.db, are edited through a new V2 Agents settings page, and are
listed via a new settings.agentConfigs.* tRPC router. The V2 modal +
launch dispatch are intentionally NOT migrated in this PR; that lands
later (PR 5).
Backend
- New host_agent_configs table (argv-array shape: command, args[],
promptTransport, promptArgs[], env) with migration 0004. Storing
argv directly avoids shell-quoting bugs and makes prompt injection
a list push instead of string concatenation.
- Hardcoded AgentPreset catalog (claude, amp, codex, gemini, opencode,
pi, copilot, cursor-agent). Default seed = claude/amp/codex/gemini/
copilot. Superset Chat intentionally excluded.
- settings.agentConfigs.{list, listPresets, add, update, remove,
reorder, resetToDefaults} with seeding on first list, duplicate-
presetId support, and reorder integrity validation.
- 17 router tests against an in-memory bun:sqlite db.
Renderer (desktop)
- Under FEATURE_FLAGS.V2_CLOUD, the Agents settings page renders a new
V2AgentsSettings component that talks to the active host's
settings.agentConfigs.*. Non-V2 keeps the legacy preset UI unchanged.
- Single shell-style command input parsed via shell-quote into
command + args; separate prompt-only args input; argv/stdin transport
toggle. Add (preset picker), remove, up/down reorder, reset-to-
defaults wired through tRPC mutations + react-query invalidation.
- 8 argv-parsing/joining tests.
Launch resolution (consumed in a later PR):
argv = prompt
? [command, ...args, ...promptArgs, ...(transport === "argv" ? [prompt] : [])]
: [command, ...args]
Empty launches drop promptArgs, so codex/opencode/copilot don't carry
their prompt-mode flags into promptless sessions.
…tions Replaces the hand-rolled CREATE TABLE with the actual drizzle migrate() run. Now every test boots from an empty in-memory db that migrates 0000…0004 in sequence, so any drift between schema.ts and the generated migration SQL would fail every test instead of silently passing.
…to-reorder) Restructure V2AgentsSettings to mirror v1's AgentCard look-and-feel: - Each row now shows preset icon + label + description + chevron, matching v1's AgentCardHeader layout. Icons resolve via getPresetIcon by presetId. - Drag-to-reorder via @dnd-kit/sortable replaces the up/down arrow buttons. Reorder mutation has optimistic updates so the row settles immediately on drop and rolls back on error. - Splits the card into its own folder (components/V2AgentCard) per AGENTS.md project structure (one folder per component). Server side: AgentPreset gains a `description` field (read-only, returned by listPresets). Renderer maps presetId → description from listPresets to feed the card. No on-disk schema change — descriptions remain catalog-only, never stored on the user's instance row.
Renders the same getPresetIcon-resolved SVG next to each label in the dropdown. Bundled-asset lookup only, no extra IPC or host-service call.
Adds Mastracode as an Add-menu template only (not in default seed, matching v1's includeInDefaultTerminalPresets behavior). Uses `mastracode --prompt <prompt>` for prompt launches and `mastracode` alone for promptless launches. v1's `; mastracode` post-prompt REPL suffix is intentionally dropped — V2 doesn't model shell chaining.
1 task
Contributor
📝 WalkthroughWalkthroughAdds a V2 agent settings UI (feature-flagged) and new host-side agent config management: frontend components for listing, editing, reordering, adding, and resetting agents; argv utilities and tests; a new DB table and Drizzle schema; and tRPC settings router with agent presets and agentConfigs endpoints. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as V2AgentsSettings
participant Cache as React Query Cache
participant Client as tRPC Client
participant Server as Settings Router
participant DB as Database
rect rgba(100,150,200,0.5)
Note over User,DB: Add Agent Flow
User->>UI: Select "Add agent" preset
UI->>Client: agentConfigs.add(presetId)
Client->>Server: add mutation
Server->>DB: INSERT config row
DB-->>Server: inserted row
Server-->>Client: HostAgentConfigDto
Client->>Cache: invalidate agentConfigs.list
Cache->>UI: refetch list
UI-->>User: show new agent (or toast on error)
end
rect rgba(150,200,100,0.5)
Note over User,DB: Reorder Flow
User->>UI: Drag agent to new position
UI->>Cache: optimistic reorder (update order fields)
UI->>Client: agentConfigs.reorder(ids)
Client->>Server: reorder mutation
Server->>DB: UPDATE display_order for ids (transaction)
DB-->>Server: confirmation
alt success
Server-->>Client: reordered list
Client->>Cache: replace/validate cache
UI-->>User: persist new order
else failure
Server-->>Client: error
Client->>Cache: restore previous cache
UI-->>User: show error toast & revert UI
end
end
rect rgba(200,150,100,0.5)
Note over User,DB: Edit Agent Flow
User->>UI: Edit card input (label/command/promptArgs)
UI->>UI: parse/validate on blur
alt valid
UI->>Client: agentConfigs.update(id, patch)
Client->>Server: update mutation
Server->>DB: UPDATE mutable fields
DB-->>Server: updated row
Server-->>Client: updated DTO
Client->>Cache: invalidate agentConfigs.list
UI-->>User: reflect changes
else invalid
UI-->>User: restore original value & toast
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 3/8 reviews remaining, refill in 32 minutes and 24 seconds.Comment |
Greptile SummaryThis PR introduces the V2 host-runtime agent config model: a new Three issues in
Confidence Score: 3/5Not safe to merge as-is — three P1 defects in the tRPC router can cause data loss or a bricked settings page. Two missing transactions (resetToDefaults, reorder) and unguarded JSON.parse in the hot list() path are present defects, not speculative risks. Any of the three can corrupt or destroy user-configured agent data in production. packages/host-service/src/trpc/router/settings/agent-configs.ts — all three P1 issues are in this file.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/settings/agent-configs.ts | Core tRPC router for CRUD on agent configs; has three P1 issues: non-atomic resetToDefaults (data-loss risk), non-atomic reorder loop (inconsistent display_order on partial failure), and unguarded JSON.parse in toOutput (bricking list on any malformed row). |
| packages/host-service/src/trpc/router/settings/agent-presets.ts | Hardcoded preset catalog with helpers; clean, well-structured, defensive copies on every return. |
| packages/host-service/src/db/schema.ts | Adds hostAgentConfigs table with correct column definitions and display_order index; aligns with migration SQL. |
| apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsx | Main V2 settings component; clean DnD wiring with optimistic updates and proper rollback on reorder error. |
| apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsx | Per-agent card with inline editing; blur-to-save pattern works correctly for command/promptArgs, but empty label is silently discarded without restoring the previous value. |
| packages/host-service/src/trpc/router/settings/agent-configs.test.ts | 17 router tests run against a real migration chain on an in-memory SQLite DB; good coverage of seeding, CRUD, reorder, and reset. |
Sequence Diagram
sequenceDiagram
participant UI as V2AgentsSettings (Renderer)
participant Card as V2AgentCard
participant HC as host-service tRPC
participant DB as host.db (SQLite)
UI->>HC: settings.agentConfigs.list()
HC->>DB: SELECT * (seedDefaultsIfEmpty if 0 rows)
DB-->>HC: rows[]
HC-->>UI: HostAgentConfigDto[]
UI->>HC: settings.agentConfigs.listPresets()
HC-->>UI: AgentPreset[] (hardcoded catalog)
Note over UI: User drags to reorder
UI->>UI: optimistic update (setQueryData)
UI->>HC: settings.agentConfigs.reorder({ids})
HC->>DB: UPDATE displayOrder per id (N individual writes)
DB-->>HC: ok
HC-->>UI: HostAgentConfigDto[] (new order)
Note over Card: User edits command and blurs
Card->>HC: settings.agentConfigs.update({id, patch})
HC->>DB: UPDATE hostAgentConfigs SET ...
DB-->>HC: updated row
HC-->>Card: HostAgentConfigDto
Note over UI: User clicks Reset to Defaults
UI->>HC: settings.agentConfigs.resetToDefaults()
HC->>DB: DELETE existing rows
HC->>DB: INSERT default seed rows
DB-->>HC: new rows
HC-->>UI: HostAgentConfigDto[]
Comments Outside Diff (5)
-
packages/host-service/src/trpc/router/settings/agent-configs.ts, line 1992-2009 (link)resetToDefaultsis not atomic — data loss on insert failureThe delete runs before the insert outside any transaction. If
db.insert(…).values(seeds).run()throws (e.g., schema constraint, disk error), all rows are permanently deleted with nothing replacing them, leaving the table empty.// Wrap the delete + insert in a transaction: ctx.db.transaction((tx) => { if (existing.length > 0) { tx.delete(hostAgentConfigs) .where(inArray(hostAgentConfigs.id, existing.map((r) => r.id))) .run(); } if (seeds.length > 0) { tx.insert(hostAgentConfigs).values(seeds).run(); } });
Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/src/trpc/router/settings/agent-configs.ts Line: 1992-2009 Comment: **`resetToDefaults` is not atomic — data loss on insert failure** The delete runs before the insert outside any transaction. If `db.insert(…).values(seeds).run()` throws (e.g., schema constraint, disk error), all rows are permanently deleted with nothing replacing them, leaving the table empty. ```ts // Wrap the delete + insert in a transaction: ctx.db.transaction((tx) => { if (existing.length > 0) { tx.delete(hostAgentConfigs) .where(inArray(hostAgentConfigs.id, existing.map((r) => r.id))) .run(); } if (seeds.length > 0) { tx.insert(hostAgentConfigs).values(seeds).run(); } }); ``` How can I resolve this? If you propose a fix, please make it concise.
-
packages/host-service/src/trpc/router/settings/agent-configs.ts, line 1981-1987 (link)reorderloop is not wrapped in a transactionEach
UPDATEruns as a separate statement. If the process is killed or an update throws mid-loop, some rows will have their newdisplayOrderand others will keep the old one, leaving the persisted order permanently inconsistent. Wrapping theforEachin actx.db.transaction(…)call makes this all-or-nothing.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/src/trpc/router/settings/agent-configs.ts Line: 1981-1987 Comment: **`reorder` loop is not wrapped in a transaction** Each `UPDATE` runs as a separate statement. If the process is killed or an update throws mid-loop, some rows will have their new `displayOrder` and others will keep the old one, leaving the persisted order permanently inconsistent. Wrapping the `forEach` in a `ctx.db.transaction(…)` call makes this all-or-nothing. How can I resolve this? If you propose a fix, please make it concise.
-
packages/host-service/src/trpc/router/settings/agent-configs.ts, line 1736-1758 (link)parseArgv/parseEnvcallJSON.parsewithout error handlingIf a row in the DB contains malformed JSON (e.g., manual edit, partial write during a crash, or a future migration bug),
JSON.parse(value)throws synchronously. BecausetoOutputis called on every row inlist(), a single malformed row makes the entire list query fail with an unhandled exception, effectively bricking the settings page until the DB is fixed manually.Wrap each call in a try/catch and fall back to the safe default:
function parseArgv(value: string): string[] { try { const parsed: unknown = JSON.parse(value); if (!Array.isArray(parsed) || parsed.some((i) => typeof i !== "string")) return []; return parsed as string[]; } catch { return []; } }
Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/src/trpc/router/settings/agent-configs.ts Line: 1736-1758 Comment: **`parseArgv` / `parseEnv` call `JSON.parse` without error handling** If a row in the DB contains malformed JSON (e.g., manual edit, partial write during a crash, or a future migration bug), `JSON.parse(value)` throws synchronously. Because `toOutput` is called on every row in `list()`, a single malformed row makes the entire list query fail with an unhandled exception, effectively bricking the settings page until the DB is fixed manually. Wrap each call in a try/catch and fall back to the safe default: ```ts function parseArgv(value: string): string[] { try { const parsed: unknown = JSON.parse(value); if (!Array.isArray(parsed) || parsed.some((i) => typeof i !== "string")) return []; return parsed as string[]; } catch { return []; } } ``` How can I resolve this? If you propose a fix, please make it concise.
-
packages/host-service/src/trpc/router/settings/agent-configs.ts, line 1945-1953 (link)removesilently succeeds for non-existent IDsdb.delete(…).where(eq(…, input.id)).run()is a no-op when the ID doesn't exist, and the procedure still returns{ success: true }. This makes it impossible for callers to distinguish a successful deletion from a stale request targeting an already-removed row. Adding arowsAffectedcheck (or a pre-flightSELECT) and throwingNOT_FOUNDwhen nothing was deleted would make the API surface consistent withupdate.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/src/trpc/router/settings/agent-configs.ts Line: 1945-1953 Comment: **`remove` silently succeeds for non-existent IDs** `db.delete(…).where(eq(…, input.id)).run()` is a no-op when the ID doesn't exist, and the procedure still returns `{ success: true }`. This makes it impossible for callers to distinguish a successful deletion from a stale request targeting an already-removed row. Adding a `rowsAffected` check (or a pre-flight `SELECT`) and throwing `NOT_FOUND` when nothing was deleted would make the API surface consistent with `update`. How can I resolve this? If you propose a fix, please make it concise.
-
apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsx, line 406-410 (link)Empty label blurs silently without restoring prior value
handleLabelBlurskips the mutation whenlabel.trim().length === 0, but it does not reset the locallabelstate back toconfig.label. The card will display a blank label field until the next query invalidation (triggered by a different mutation). A user who accidentally empties the label and tabs away sees an empty field with no error, potentially confusing the state.const handleLabelBlur = () => { if (label.trim().length === 0) { setLabel(config.label); // restore on invalid input return; } if (label !== config.label) { updateMutation.mutate({ label }); } };
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsx Line: 406-410 Comment: **Empty label blurs silently without restoring prior value** `handleLabelBlur` skips the mutation when `label.trim().length === 0`, but it does not reset the local `label` state back to `config.label`. The card will display a blank label field until the next query invalidation (triggered by a different mutation). A user who accidentally empties the label and tabs away sees an empty field with no error, potentially confusing the state. ```ts const handleLabelBlur = () => { if (label.trim().length === 0) { setLabel(config.label); // restore on invalid input return; } if (label !== config.label) { updateMutation.mutate({ label }); } }; ``` How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
packages/host-service/src/trpc/router/settings/agent-configs.ts:1992-2009
**`resetToDefaults` is not atomic — data loss on insert failure**
The delete runs before the insert outside any transaction. If `db.insert(…).values(seeds).run()` throws (e.g., schema constraint, disk error), all rows are permanently deleted with nothing replacing them, leaving the table empty.
```ts
// Wrap the delete + insert in a transaction:
ctx.db.transaction((tx) => {
if (existing.length > 0) {
tx.delete(hostAgentConfigs)
.where(inArray(hostAgentConfigs.id, existing.map((r) => r.id)))
.run();
}
if (seeds.length > 0) {
tx.insert(hostAgentConfigs).values(seeds).run();
}
});
```
### Issue 2 of 5
packages/host-service/src/trpc/router/settings/agent-configs.ts:1981-1987
**`reorder` loop is not wrapped in a transaction**
Each `UPDATE` runs as a separate statement. If the process is killed or an update throws mid-loop, some rows will have their new `displayOrder` and others will keep the old one, leaving the persisted order permanently inconsistent. Wrapping the `forEach` in a `ctx.db.transaction(…)` call makes this all-or-nothing.
### Issue 3 of 5
packages/host-service/src/trpc/router/settings/agent-configs.ts:1736-1758
**`parseArgv` / `parseEnv` call `JSON.parse` without error handling**
If a row in the DB contains malformed JSON (e.g., manual edit, partial write during a crash, or a future migration bug), `JSON.parse(value)` throws synchronously. Because `toOutput` is called on every row in `list()`, a single malformed row makes the entire list query fail with an unhandled exception, effectively bricking the settings page until the DB is fixed manually.
Wrap each call in a try/catch and fall back to the safe default:
```ts
function parseArgv(value: string): string[] {
try {
const parsed: unknown = JSON.parse(value);
if (!Array.isArray(parsed) || parsed.some((i) => typeof i !== "string")) return [];
return parsed as string[];
} catch {
return [];
}
}
```
### Issue 4 of 5
packages/host-service/src/trpc/router/settings/agent-configs.ts:1945-1953
**`remove` silently succeeds for non-existent IDs**
`db.delete(…).where(eq(…, input.id)).run()` is a no-op when the ID doesn't exist, and the procedure still returns `{ success: true }`. This makes it impossible for callers to distinguish a successful deletion from a stale request targeting an already-removed row. Adding a `rowsAffected` check (or a pre-flight `SELECT`) and throwing `NOT_FOUND` when nothing was deleted would make the API surface consistent with `update`.
### Issue 5 of 5
apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsx:406-410
**Empty label blurs silently without restoring prior value**
`handleLabelBlur` skips the mutation when `label.trim().length === 0`, but it does not reset the local `label` state back to `config.label`. The card will display a blank label field until the next query invalidation (triggered by a different mutation). A user who accidentally empties the label and tabs away sees an empty field with no error, potentially confusing the state.
```ts
const handleLabelBlur = () => {
if (label.trim().length === 0) {
setLabel(config.label); // restore on invalid input
return;
}
if (label !== config.label) {
updateMutation.mutate({ label });
}
};
```
Reviews (1): Last reviewed commit: "feat(host-service): add Mastracode prese..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/AgentsSettings/AgentsSettings.tsx (1)
23-69: ⚡ Quick winSplit
V1AgentsSettingsinto its own file.This file now declares two components (
AgentsSettingsandV1AgentsSettings), which breaks the project’s component-file convention.Suggested direction
- function V1AgentsSettings({ visibleItems }: AgentsSettingsProps) { + // Move to: components/V1AgentsSettings/V1AgentsSettings.tsx + // and import it here.As per coding guidelines "
**/*.{tsx,ts}: Do not create multi-component files; use one component per file."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/AgentsSettings/AgentsSettings.tsx` around lines 23 - 69, The file declares a second top-level component V1AgentsSettings which violates the one-component-per-file rule; extract V1AgentsSettings into its own new file exporting the component (preserving its props type AgentsSettingsProps and the same imports it uses), update the original file to import V1AgentsSettings where it was used, and ensure references to AgentCard, electronTrpc.settings.getAgentPresets.useQuery, SETTING_ITEM_ID, and isItemVisible remain available (move or re-export any shared types/helpers if needed).packages/host-service/src/trpc/router/settings/agent-configs.test.ts (1)
157-159: ⚡ Quick winAvoid
anyin the invalid-input test case.Line 158 uses
as any; use anunknowncast path instead so the test stays intentionally invalid without bypassing the no-anyrule.Example adjustment
- // biome-ignore lint/suspicious/noExplicitAny: testing invalid input - patch: { promptTransport: "file" as any }, + patch: { promptTransport: "file" as unknown as "argv" | "stdin" },As per coding guidelines "
**/*.{ts,tsx}: Avoid usinganytype; prefer explicit type safety in TypeScript."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/settings/agent-configs.test.ts` around lines 157 - 159, The test's invalid-input case uses "as any" for patch.promptTransport which violates the no-any rule; change the cast to go through unknown so the value remains intentionally invalid without using any (e.g., replace the "as any" cast on the patch.promptTransport value with an "as unknown" (or "as unknown as <target type>") cast). Update the test object (the patch property in agent-configs.test.ts) to use the unknown-cast path instead of any so linting passes while preserving the invalid-input behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsx`:
- Around line 144-148: handleTransportChange currently sets local state
immediately and fires updateMutation.mutate but doesn't revert the UI when the
mutation fails; change handleTransportChange to capture the previous
promptTransport value, perform the optimistic setPromptTransport(next), then
call updateMutation.mutate({ promptTransport: next }, { onError: () =>
setPromptTransport(previous) }) so the toggle is rolled back on error (reference
handleTransportChange, promptTransport, setPromptTransport, and
updateMutation.mutate).
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/utils/argv.test.ts`:
- Around line 9-63: Add a regression unit test in argv.test.ts that round-trips
the lossy argv case: a command that contains spaces and an empty quoted argument
(e.g. command with a space like "my cmd" plus "" as one arg). Use
joinCommandArgs to build the string from a command and args array (including the
empty quoted arg) and then parse it back with parseCommandString (or use
parseArgs/joinArgs if appropriate) and assert the reparsed command and args
exactly match the originals; target the functions parseCommandString and
joinCommandArgs (and parseArgs/joinArgs if you prefer) so this case is covered
before merging.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/utils/argv.ts`:
- Around line 7-27: joinCommandArgs and parseCommandString are not lossless:
joinCommandArgs emits the command raw and parseCommandString trims/drops empty
tokens, so inputs with spaces or explicit empty quoted args won't round-trip.
Update joinCommandArgs to quote the command as well (use the existing quote
helper) and join command+args with spaces; update parseCommandString to stop
trimming tokens and to preserve empty string tokens produced by parse (i.e.,
remove .map(token => token.trim()) and the length>0 filter, only filter to keep
typeof token === "string"), so quoted empty args and commands with spaces are
preserved.
In `@packages/host-service/src/trpc/router/settings/agent-configs.ts`:
- Around line 51-73: Wrap the JSON.parse calls in parseArgv and parseEnv with
try/catch so malformed JSON doesn't throw and instead returns the existing safe
fallback (parseArgv should return [] and parseEnv should return {}); keep the
current type checks/validations (Array.isArray + string item checks for
parseArgv, and null/object/Array and string value checks for parseEnv) and only
return the typed result when validation passes, otherwise return the fallback
inside the catch or after failed validation; update functions parseArgv and
parseEnv accordingly.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/AgentsSettings/AgentsSettings.tsx`:
- Around line 23-69: The file declares a second top-level component
V1AgentsSettings which violates the one-component-per-file rule; extract
V1AgentsSettings into its own new file exporting the component (preserving its
props type AgentsSettingsProps and the same imports it uses), update the
original file to import V1AgentsSettings where it was used, and ensure
references to AgentCard, electronTrpc.settings.getAgentPresets.useQuery,
SETTING_ITEM_ID, and isItemVisible remain available (move or re-export any
shared types/helpers if needed).
In `@packages/host-service/src/trpc/router/settings/agent-configs.test.ts`:
- Around line 157-159: The test's invalid-input case uses "as any" for
patch.promptTransport which violates the no-any rule; change the cast to go
through unknown so the value remains intentionally invalid without using any
(e.g., replace the "as any" cast on the patch.promptTransport value with an "as
unknown" (or "as unknown as <target type>") cast). Update the test object (the
patch property in agent-configs.test.ts) to use the unknown-cast path instead of
any so linting passes while preserving the invalid-input behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b215815c-54f9-43fb-9bbb-fa367c26a359
📒 Files selected for processing (17)
apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/AgentsSettings/AgentsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/utils/argv.test.tsapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/utils/argv.tspackages/host-service/drizzle/0004_mean_blacklash.sqlpackages/host-service/drizzle/meta/0004_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/package.jsonpackages/host-service/src/db/schema.tspackages/host-service/src/trpc/router/router.tspackages/host-service/src/trpc/router/settings/agent-configs.test.tspackages/host-service/src/trpc/router/settings/agent-configs.tspackages/host-service/src/trpc/router/settings/agent-presets.tspackages/host-service/src/trpc/router/settings/index.ts
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Backend (agent-configs.ts): - parseArgv/parseEnv wrap JSON.parse in try/catch — a single bad row no longer fails the whole list() response. - Trim label and command via z.string().trim().min(1) so whitespace-only values can't slip through to the UI as blank rows. - Wrap reorder updates in a transaction so a crash mid-loop can't leave display_order half-updated. - Wrap resetToDefaults delete+insert in a transaction so a crash between the two can't leave the table empty. - remove() now reads the row first and throws NOT_FOUND for unknown ids, matching update()'s behavior. Renderer (V2AgentCard.tsx): - Roll back optimistic promptTransport on update error so the toggle doesn't lie about persistence. Renderer (V2AgentsSettings.tsx): - Render an explicit error state (with Retry) when configsQuery fails, so a host-service outage isn't masked as "no agents configured". argv helpers (argv.ts): - Stop trimming/dropping empty tokens; quote command alongside args in joinCommandArgs. Round-trip is now lossless for paths with spaces and empty quoted args. Tests: - 3 new router tests (NOT_FOUND remove, whitespace rejection, trim). - 2 new argv tests (path with spaces, empty quoted arg).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsx`:
- Around line 73-84: The useEffect in V2AgentCard is copying every config field
back into local editable state (setLabel, setCommandText, setPromptArgsText,
setPromptTransport) on every refetch which overwrites in-progress edits; change
the effect to only apply resets when the agent identity changes (e.g.,
config.id) or when the specific field is not dirty/being edited: add per-field
dirty or focus state for label, commandText, and promptArgsText (and focus
handlers on the inputs), and in the useEffect (or separate effects) only call
setLabel(join...) / setCommandText(joinCommandArgs...) /
setPromptArgsText(joinArgs...) if that field is not dirty OR when config.id
changes; keep existing helpers joinCommandArgs and joinArgs and preserve
updating promptTransport only when appropriate.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsx`:
- Around line 151-157: The drag handler (handleDragEnd) currently fires
reorderMutation immediately and can submit overlapping writes; modify
handleDragEnd to first check reorderMutation.isPending and return early (ignore
the drag-end) while a reorder is in flight, so only one
reorderMutation.mutate(arrayMove(sortableIds, oldIndex, newIndex)) runs at a
time; reference handleDragEnd, sortableIds, and reorderMutation (use
reorderMutation.isPending) to implement this guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e2cc240-2993-418a-9000-d8b173fdd3c4
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/utils/argv.test.tsapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/utils/argv.tspackages/host-service/src/trpc/router/settings/agent-configs.test.tspackages/host-service/src/trpc/router/settings/agent-configs.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/utils/argv.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/host-service/src/trpc/router/settings/agent-configs.test.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/utils/argv.ts
- packages/host-service/src/trpc/router/settings/agent-configs.ts
Comment on lines
+73
to
+84
| useEffect(() => { | ||
| setLabel(config.label); | ||
| setCommandText(joinCommandArgs(config.command, config.args)); | ||
| setPromptArgsText(joinArgs(config.promptArgs)); | ||
| setPromptTransport(config.promptTransport); | ||
| }, [ | ||
| config.label, | ||
| config.command, | ||
| config.args, | ||
| config.promptArgs, | ||
| config.promptTransport, | ||
| ]); |
Contributor
There was a problem hiding this comment.
Avoid resyncing every editable field on each refetch.
Lines 73-84 unconditionally copy config back into label, commandText, and promptArgsText. Since every successful patch triggers onChanged() and a refetch, saving one field can wipe unsaved edits in another field the user is still typing into. Please gate these resets behind per-field dirty/focus state, or limit the effect to identity changes such as config.id.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsx`
around lines 73 - 84, The useEffect in V2AgentCard is copying every config field
back into local editable state (setLabel, setCommandText, setPromptArgsText,
setPromptTransport) on every refetch which overwrites in-progress edits; change
the effect to only apply resets when the agent identity changes (e.g.,
config.id) or when the specific field is not dirty/being edited: add per-field
dirty or focus state for label, commandText, and promptArgsText (and focus
handlers on the inputs), and in the useEffect (or separate effects) only call
setLabel(join...) / setCommandText(joinCommandArgs...) /
setPromptArgsText(joinArgs...) if that field is not dirty OR when config.id
changes; keep existing helpers joinCommandArgs and joinArgs and preserve
updating promptTransport only when appropriate.
Comment on lines
+151
to
+157
| const handleDragEnd = (event: DragEndEvent) => { | ||
| const { active, over } = event; | ||
| if (!over || active.id === over.id) return; | ||
| const oldIndex = sortableIds.indexOf(String(active.id)); | ||
| const newIndex = sortableIds.indexOf(String(over.id)); | ||
| if (oldIndex < 0 || newIndex < 0) return; | ||
| reorderMutation.mutate(arrayMove(sortableIds, oldIndex, newIndex)); |
Contributor
There was a problem hiding this comment.
Prevent overlapping reorder writes.
reorder persists the entire ordered id list, so firing a second mutation before the first settles can let an older request win and save a stale order. Please serialize these mutations or ignore new drag-end events while reorderMutation.isPending.
Suggested minimal guard
const handleDragEnd = (event: DragEndEvent) => {
+ if (reorderMutation.isPending) return;
const { active, over } = event;
if (!over || active.id === over.id) return;
const oldIndex = sortableIds.indexOf(String(active.id));
const newIndex = sortableIds.indexOf(String(over.id));
if (oldIndex < 0 || newIndex < 0) return;
reorderMutation.mutate(arrayMove(sortableIds, oldIndex, newIndex));
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleDragEnd = (event: DragEndEvent) => { | |
| const { active, over } = event; | |
| if (!over || active.id === over.id) return; | |
| const oldIndex = sortableIds.indexOf(String(active.id)); | |
| const newIndex = sortableIds.indexOf(String(over.id)); | |
| if (oldIndex < 0 || newIndex < 0) return; | |
| reorderMutation.mutate(arrayMove(sortableIds, oldIndex, newIndex)); | |
| const handleDragEnd = (event: DragEndEvent) => { | |
| if (reorderMutation.isPending) return; | |
| const { active, over } = event; | |
| if (!over || active.id === over.id) return; | |
| const oldIndex = sortableIds.indexOf(String(active.id)); | |
| const newIndex = sortableIds.indexOf(String(over.id)); | |
| if (oldIndex < 0 || newIndex < 0) return; | |
| reorderMutation.mutate(arrayMove(sortableIds, oldIndex, newIndex)); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsx`
around lines 151 - 157, The drag handler (handleDragEnd) currently fires
reorderMutation immediately and can submit overlapping writes; modify
handleDragEnd to first check reorderMutation.isPending and return early (ignore
the drag-end) while a reorder is in flight, so only one
reorderMutation.mutate(arrayMove(sortableIds, oldIndex, newIndex)) runs at a
time; reference handleDragEnd, sortableIds, and reorderMutation (use
reorderMutation.isPending) to implement this guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 1 of the canonical
workspace.create()refactor. Introduces the V2 host-runtime agent config model. Configs live inhost.db, are edited through a new V2 Agents settings page, and are listed via a newsettings.agentConfigs.*tRPC router.V2 modal + launch dispatch are intentionally not migrated here; that lands later (PR 5).
Design doc: #3893.
Backend
host_agent_configstable (argv-array shape:command,args[],promptTransport,promptArgs[],env) with migration0004. Storing argv directly avoids shell-quoting bugs and makes prompt injection a list push instead of string concatenation.AgentPresetcatalog: claude, amp, codex, gemini, mastracode, opencode, pi, copilot, cursor-agent. Default seed = claude/amp/codex/gemini/copilot. Mastracode is available as an Add-template only (matches v1includeInDefaultTerminalPresets: false).settings.agentConfigs.{list, listPresets, add, update, remove, reorder, resetToDefaults}with seeding on first list, duplicate-presetIdsupport, reorder integrity validation, and zod-validated patches.0000…0004), so any drift between schema and generated SQL fails every test.Renderer (desktop)
FEATURE_FLAGS.V2_CLOUD, the Agents settings page renders a newV2AgentsSettingscomponent that talks to the active host'ssettings.agentConfigs.*. Non-V2 keeps the legacy preset UI unchanged (clean if/else branch inAgentsSettings.tsx).getPresetIcon) + label + description + chevron per row.@dnd-kit/sortable(mouse 4px threshold, touch 150ms hold + 5px tolerance, keyboard sortable). Optimistic updates so the row settles immediately on drop and rolls back on error.shell-quoteintocommand+argson save; separate prompt-only args input; argv/stdin transport toggle. Add (preset picker with icons), remove, reset-to-defaults wired through tRPC mutations + react-query invalidation.AgentPresetgains adescriptionfield returned bylistPresets()— used by the row header and Add menu. Descriptions stay catalog-only; never stored on the user's instance row.Launch resolution (consumed by a later PR)
Empty launches drop
promptArgs, so codex (--), opencode (--prompt), and copilot (-i) don't carry their prompt-mode flags into promptless sessions.Out of scope (deliberately deferred)
agent-prompt-template.source— V2 is host-scoped user data only.Test plan
bun test packages/host-service/src/trpc/router/settings/— 17 router tests pass against the real migration chainbun test apps/desktop/.../V2AgentsSettings/utils/— 8 argv parsing tests passbun run typecheck— 26/26 packages cleanbun run lint— cleanbun run build:host— host-service prod bundle exits 0AgentCardUI unchangedSummary by CodeRabbit
New Features
Tests & Data