[codex] Refactor agent registry and add custom agent CRUD groundwork#2994
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Amp Code CLI as a supported terminal agent, introduces custom agent creation/update/delete via TRPC mutations with schema validation, refactors builtin agent definitions into a typed manifest catalog, extends MCP configuration to read from Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client / UI
participant API as TRPC Router
participant Validation as Normalization
participant Storage as Settings Store
participant Preset as Preset Resolver
Client->>API: createCustomAgent(label, command, promptCommand, taskPromptTemplate)
API->>Validation: normalizeCreateCustomAgentInput(input)
Validation->>Validation: validate fields, trim strings, validate task prompt template
alt Invalid Input
Validation-->>API: throw TRPCError(BAD_REQUEST)
API-->>Client: error
else Valid Input
Validation-->>API: normalized definition
API->>Storage: upsertCustomAgentDefinition(definition with custom:uuid id)
Storage->>Storage: add/replace in customDefinitions array
API->>Storage: saveAgentCustomDefinitions(definitions)
Storage-->>API: saved
API->>Preset: resolveAgentPreset(custom:uuid)
Preset-->>API: resolved preset with user-provided fields
API-->>Client: { id, label, command, promptCommand, ... }
end
sequenceDiagram
participant Client as Chat Client
participant MCP as MCP Overview
participant FileReader as File System
participant Parser as JSON/Zod Parser
Client->>MCP: getMcpOverview(cwd)
MCP->>FileReader: iterate config files (.amp/settings.json, .mcp.json, ...)
alt .amp/settings.json exists
FileReader-->>MCP: file content
MCP->>Parser: parse JSON → extract amp.mcpServers
alt Valid JSON
Parser-->>MCP: servers array
MCP-->>Client: { sourcePath: .amp/settings.json, servers: [...] }
else Invalid JSON
Parser-->>MCP: error (fallback)
MCP->>FileReader: try next file (.mcp.json)
end
else .mcp.json exists
FileReader-->>MCP: file content
MCP->>Parser: parse JSON → extract mcpServers
Parser-->>MCP: servers array
MCP-->>Client: { sourcePath: .mcp.json, servers: [...] }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: 1
🧹 Nitpick comments (4)
packages/chat/src/server/desktop/router/mcp-overview/mcp-overview.ts (1)
16-19: Consider extracting the duplicatedmcpServersreader.The
readServerscallbacks for.mastracode/mcp.jsonand.mcp.jsonare identical.🔧 Optional DRY improvement
+const readMcpServers = (parsed: unknown) => { + const result = mcpSettingsSchema.safeParse(parsed); + return result.success ? result.data.mcpServers : null; +}; + const MCP_SETTINGS_FILES = [ { relativePath: ".mastracode/mcp.json", - readServers: (parsed: unknown) => { - const result = mcpSettingsSchema.safeParse(parsed); - return result.success ? result.data.mcpServers : null; - }, + readServers: readMcpServers, }, { relativePath: ".amp/settings.json", readServers: (parsed: unknown) => { const result = ampMcpSettingsSchema.safeParse(parsed); return result.success ? result.data["amp.mcpServers"] : null; }, }, { relativePath: ".mcp.json", - readServers: (parsed: unknown) => { - const result = mcpSettingsSchema.safeParse(parsed); - return result.success ? result.data.mcpServers : null; - }, + readServers: readMcpServers, }, ] as const;Also applies to: 30-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat/src/server/desktop/router/mcp-overview/mcp-overview.ts` around lines 16 - 19, Extract the duplicated logic that reads mcpServers into a single helper function (e.g., readMcpServers) and use it for both callbacks; specifically, create a function that accepts parsed: unknown, calls mcpSettingsSchema.safeParse(parsed), and returns result.success ? result.data.mcpServers : null, then replace the inline readServers implementations for the `.mastracode/mcp.json` and `.mcp.json` entries with references to this new helper (keep references to mcpSettingsSchema.safeParse and the mcpServers field so behavior is unchanged).apps/desktop/src/lib/trpc/routers/settings/index.ts (2)
239-266: Potential TOCTOU issue with double database reads.The mutation reads
readRawAgentCustomDefinitions()twice: once to look up the definition (line 243) and again when callingupsertCustomAgentDefinition(line 254). In a concurrent environment, another mutation could modify the definitions between these reads, potentially causing unexpected behavior.Consider reading once and reusing:
🔧 Suggested improvement
updateCustomAgent: publicProcedure .input(updateCustomAgentInputSchema) .mutation(({ input }) => { + const currentDefinitions = readRawAgentCustomDefinitions(); const definition = getCustomAgentDefinitionById({ - customDefinitions: readRawAgentCustomDefinitions(), + customDefinitions: currentDefinitions, id: input.id as `custom:${string}`, }); if (!definition) { throw new TRPCError({ code: "NOT_FOUND", message: `Custom agent ${input.id} not found`, }); } const nextDefinitions = upsertCustomAgentDefinition({ - currentDefinitions: readRawAgentCustomDefinitions(), + currentDefinitions, definition: applyCustomAgentDefinitionPatch({ definition, patch: normalizeCustomAgentPatch(input.patch), }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/settings/index.ts` around lines 239 - 266, The mutation updateCustomAgent currently calls readRawAgentCustomDefinitions() twice which creates a TOCTOU race; fix it by reading the definitions once into a local variable (e.g., const currentDefinitions = readRawAgentCustomDefinitions()) and then use currentDefinitions for getCustomAgentDefinitionById(...) and pass currentDefinitions into upsertCustomAgentDefinition(...); apply the patch via applyCustomAgentDefinitionPatch(...) against the single-read definition and then call saveAgentCustomDefinitions(nextDefinitions) and return the resolved preset as before.
267-295: Same TOCTOU consideration applies to deleteCustomAgent.Similar to
updateCustomAgent, this mutation reads the definitions twice. Additionally, the cleanup of preset overrides (lines 287-292) is a good practice to prevent orphaned data.🔧 Suggested improvement
deleteCustomAgent: publicProcedure .input(z.object({ id: z.string().regex(/^custom:/) })) .mutation(({ input }) => { + const currentDefinitions = readRawAgentCustomDefinitions(); const existingDefinition = getCustomAgentDefinitionById({ - customDefinitions: readRawAgentCustomDefinitions(), + customDefinitions: currentDefinitions, id: input.id as `custom:${string}`, }); if (!existingDefinition) { throw new TRPCError({ code: "NOT_FOUND", message: `Custom agent ${input.id} not found`, }); } saveAgentCustomDefinitions( deleteCustomAgentDefinition({ - currentDefinitions: readRawAgentCustomDefinitions(), + currentDefinitions, id: input.id as `custom:${string}`, }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/settings/index.ts` around lines 267 - 295, The deleteCustomAgent mutation currently performs TOCTOU by calling readRawAgentCustomDefinitions() twice and then operating on separate snapshots; instead, read the current definitions once into a const (use that single value for getCustomAgentDefinitionById and deleteCustomAgentDefinition) and similarly read overrides once into a const for resetAgentPresetOverride, then pass those single snapshots to saveAgentCustomDefinitions and saveAgentPresetOverrides respectively; update the deleteCustomAgent handler to use these locals and keep calls to getCustomAgentDefinitionById, deleteCustomAgentDefinition, resetAgentPresetOverride, saveAgentCustomDefinitions, and saveAgentPresetOverrides but always operate on the same in-memory values to avoid race conditions.packages/shared/src/builtin-terminal-agents.ts (1)
21-31: Add a duplicate-id guard for the built-in manifest.
createAgentRecord/fromEntrieswill silently overwrite on duplicateid. A fail-fast assertion would prevent subtle catalog corruption.Suggested patch
+function assertUniqueAgentIds( + agents: readonly BuiltinTerminalAgentManifest[], +): void { + const seen = new Set<string>(); + for (const { id } of agents) { + if (seen.has(id)) { + throw new Error(`Duplicate builtin terminal agent id: ${id}`); + } + seen.add(id); + } +} + export const BUILTIN_TERMINAL_AGENTS = [ // ... ] as const satisfies readonly BuiltinTerminalAgentManifest[]; + +assertUniqueAgentIds(BUILTIN_TERMINAL_AGENTS);Also applies to: 33-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/builtin-terminal-agents.ts` around lines 21 - 31, The createAgentRecord function silently overwrites entries when two manifests share the same id; update it to perform a fail-fast duplicate-id check before calling Object.fromEntries: iterate agents (or map over them) while tracking seen ids in a Set, and if agent.id is already present throw or assert with a clear message including the duplicate id and preferably the agent manifest identity; only after the check proceed to build and return the Record using getValue(agent). Also apply the same duplicate-id guard to any other helper that builds records from BuiltinTerminalAgentManifest in this file (the analogous function/block around the other record creation).
🤖 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/lib/trpc/routers/settings/agent-preset-router.utils.ts`:
- Around line 38-45: The id regex in updateCustomAgentInputSchema currently
allows the bare "custom:" value; tighten it so ids must include at least one
character after the prefix (rejecting exactly "custom:"). Update the id
validation in updateCustomAgentInputSchema to use a regex that requires one or
more characters after "custom:" (e.g., change from /^custom:/ to a pattern like
/^custom:.+/) so the schema rejects bare "custom:" at input validation time.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/settings/index.ts`:
- Around line 239-266: The mutation updateCustomAgent currently calls
readRawAgentCustomDefinitions() twice which creates a TOCTOU race; fix it by
reading the definitions once into a local variable (e.g., const
currentDefinitions = readRawAgentCustomDefinitions()) and then use
currentDefinitions for getCustomAgentDefinitionById(...) and pass
currentDefinitions into upsertCustomAgentDefinition(...); apply the patch via
applyCustomAgentDefinitionPatch(...) against the single-read definition and then
call saveAgentCustomDefinitions(nextDefinitions) and return the resolved preset
as before.
- Around line 267-295: The deleteCustomAgent mutation currently performs TOCTOU
by calling readRawAgentCustomDefinitions() twice and then operating on separate
snapshots; instead, read the current definitions once into a const (use that
single value for getCustomAgentDefinitionById and deleteCustomAgentDefinition)
and similarly read overrides once into a const for resetAgentPresetOverride,
then pass those single snapshots to saveAgentCustomDefinitions and
saveAgentPresetOverrides respectively; update the deleteCustomAgent handler to
use these locals and keep calls to getCustomAgentDefinitionById,
deleteCustomAgentDefinition, resetAgentPresetOverride,
saveAgentCustomDefinitions, and saveAgentPresetOverrides but always operate on
the same in-memory values to avoid race conditions.
In `@packages/chat/src/server/desktop/router/mcp-overview/mcp-overview.ts`:
- Around line 16-19: Extract the duplicated logic that reads mcpServers into a
single helper function (e.g., readMcpServers) and use it for both callbacks;
specifically, create a function that accepts parsed: unknown, calls
mcpSettingsSchema.safeParse(parsed), and returns result.success ?
result.data.mcpServers : null, then replace the inline readServers
implementations for the `.mastracode/mcp.json` and `.mcp.json` entries with
references to this new helper (keep references to mcpSettingsSchema.safeParse
and the mcpServers field so behavior is unchanged).
In `@packages/shared/src/builtin-terminal-agents.ts`:
- Around line 21-31: The createAgentRecord function silently overwrites entries
when two manifests share the same id; update it to perform a fail-fast
duplicate-id check before calling Object.fromEntries: iterate agents (or map
over them) while tracking seen ids in a Set, and if agent.id is already present
throw or assert with a clear message including the duplicate id and preferably
the agent manifest identity; only after the check proceed to build and return
the Record using getValue(agent). Also apply the same duplicate-id guard to any
other helper that builds records from BuiltinTerminalAgentManifest in this file
(the analogous function/block around the other record creation).
🪄 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: ebca65d3-5c32-49c9-a15a-ebb5021a15cc
⛔ Files ignored due to path filters (1)
packages/ui/src/assets/icons/preset-icons/amp.svgis excluded by!**/*.svg
📒 Files selected for processing (29)
.amp/settings.json.gitignoreAGENTS.mdREADME.mdapps/desktop/docs/EXTERNAL_FILES.mdapps/desktop/src/lib/trpc/routers/settings/agent-preset-router.utils.test.tsapps/desktop/src/lib/trpc/routers/settings/agent-preset-router.utils.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-amp.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-common.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers.test.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers.tsapps/desktop/src/main/lib/agent-setup/desktop-agent-capabilities.tsapps/desktop/src/main/lib/agent-setup/desktop-agent-setup.tsapps/desktop/src/main/lib/agent-setup/index.tsapps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/shared/utils/agent-settings.test.tsapps/desktop/src/shared/utils/agent-settings.tsapps/docs/content/docs/agent-integration.mdxapps/docs/content/docs/mcp.mdxapps/docs/content/docs/terminal-presets.mdxpackages/chat/src/server/desktop/router/mcp-overview/mcp-overview.test.tspackages/chat/src/server/desktop/router/mcp-overview/mcp-overview.tspackages/mcp/src/tools/devices/start-agent-session/shared.tspackages/shared/src/agent-catalog.tspackages/shared/src/agent-command.test.tspackages/shared/src/agent-command.tspackages/shared/src/builtin-terminal-agents.tspackages/ui/src/assets/icons/preset-icons/index.ts
Comment on lines
+38
to
+45
| export const updateCustomAgentInputSchema = z.object({ | ||
| id: z.string().regex(/^custom:/), | ||
| patch: createCustomAgentInputSchema | ||
| .partial() | ||
| .refine((patch) => Object.keys(patch).length > 0, { | ||
| message: "Patch must include at least one field", | ||
| }), | ||
| }); |
Contributor
There was a problem hiding this comment.
Tighten custom-agent id validation.
Line 39 currently accepts a bare custom: id. That should be rejected at input validation time.
Suggested patch
export const updateCustomAgentInputSchema = z.object({
- id: z.string().regex(/^custom:/),
+ id: z.string().regex(/^custom:.+$/, {
+ message: "id must start with 'custom:' and include a non-empty suffix",
+ }),
patch: createCustomAgentInputSchema
.partial()
.refine((patch) => Object.keys(patch).length > 0, {
message: "Patch must include at least one field",
}),
});📝 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
| export const updateCustomAgentInputSchema = z.object({ | |
| id: z.string().regex(/^custom:/), | |
| patch: createCustomAgentInputSchema | |
| .partial() | |
| .refine((patch) => Object.keys(patch).length > 0, { | |
| message: "Patch must include at least one field", | |
| }), | |
| }); | |
| export const updateCustomAgentInputSchema = z.object({ | |
| id: z.string().regex(/^custom:.+$/, { | |
| message: "id must start with 'custom:' and include a non-empty suffix", | |
| }), | |
| patch: createCustomAgentInputSchema | |
| .partial() | |
| .refine((patch) => Object.keys(patch).length > 0, { | |
| message: "Patch must include at least one field", | |
| }), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/settings/agent-preset-router.utils.ts`
around lines 38 - 45, The id regex in updateCustomAgentInputSchema currently
allows the bare "custom:" value; tighten it so ids must include at least one
character after the prefix (rejecting exactly "custom:"). Update the id
validation in updateCustomAgentInputSchema to use a regex that requires one or
more characters after "custom:" (e.g., change from /^custom:/ to a pattern like
/^custom:.+/) so the schema rejects bare "custom:" at input validation time.
…mp-code-support # Conflicts: # AGENTS.md
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
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
amp -x, docs, and the official Amp square mark iconagentCustomDefinitions, including create, update, delete, and normalization helpers.amp/settings.json, while stopping this repo from tracking its own Amp workspace settingsWhy
Adding Amp exposed that first-class agent support was still spread across several hardcoded lists and setup paths. That made each new built-in agent more expensive to add and left the existing custom-agent schema without explicit CRUD support.
This repo also should not commit workspace-specific Amp settings for itself. Amp config support is useful for users in their own repos, but keeping a repo-owned
.amp/settings.jsonhere is unnecessary and misleading.Impact
Validation
bun test packages/shared/src/agent-command.test.ts apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts packages/chat/src/server/desktop/router/mcp-overview/mcp-overview.test.ts apps/desktop/src/shared/utils/agent-settings.test.ts apps/desktop/src/lib/trpc/routers/settings/agent-preset-router.utils.test.tsbunx turbo run typecheck --filter=./packages/shared --filter=./packages/mcp --filter=./packages/chat --filter=./packages/ui --filter=./apps/desktopbun run lintampamp --helpamp -x --helpamp -x "Reply with exactly OK"printf "Reply with exactly OK\n" | amp -xamp -x "$(cat prompt.txt)"The execute-mode checks reached Amp correctly but failed with Amp's paid-credit requirement for
-x(402), which confirms the command shapes are correct even though full non-interactive execution is blocked on account state.Summary by CodeRabbit
New Features
.amp/settings.json)Documentation