Conversation
…ose files - Widen template create/edit dialog and add scroll overflow to prevent exceeding viewport height - Fix public templates not showing by replacing z.coerce.boolean() with explicit string enum transform (string "false" was coerced to true) - Split compose.yml into production (pulls ghcr image) and compose.dev.yml (builds from local Dockerfile) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Widen dialog to sm:max-w-4xl with scrollable content area - Add resize-y to system prompt textarea - Prompt "Discard Changes" confirmation when closing with unsaved edits - Track initial form state via ref, compare on close Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Volume Mounts: - Add volumeMounts to templates schema + migration - Template volumes namespaced as <owner-username>-<name> at session bind time - Volume name regex validation (shared via server/lib/validation.ts) - Template dialog: Select existing volumes or create new, info tooltip - Agent config: HTML pattern validation on volume name input - Settings/Docker: show template-referenced volumes, admin delete with confirm Session Actions: - Add loading state to stop/destroy confirm dialog (Stopping.../Destroying...) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Docker volume mounts within templates and provides an administrative interface for managing Docker volumes. Key changes include database schema updates to store volume configurations, backend logic to mount volumes during session creation, and frontend enhancements for template management and volume administration. Feedback highlights a security concern regarding unrestricted volume deletion, a redundant database query in session creation, and the need for stricter input validation for mount paths to ensure they are absolute.
| .delete("/volumes/:name", adminMiddleware, async (c) => { | ||
| const volumeName = c.req.param("name"); | ||
| try { | ||
| const docker = await getDockerClient(); | ||
| await docker.getVolume(volumeName).remove(); | ||
| return c.json({ success: true }); | ||
| } catch (err) { | ||
| return c.json( | ||
| { error: `Failed to delete volume: ${err instanceof Error ? err.message : String(err)}` }, | ||
| 500, | ||
| ); | ||
| } | ||
| }) |
There was a problem hiding this comment.
This endpoint allows an administrator to delete any Docker volume by name without verifying if it is a 'managed' volume (one created by an agent or template). This could lead to accidental deletion of critical system volumes like postgres_data. You should validate that the volumeName belongs to the set of managed volumes before proceeding with the deletion.
| const [templateOwner] = await db | ||
| .select({ username: schema.user.username, id: schema.user.id }) | ||
| .from(schema.user) | ||
| .where(eq(schema.user.id, template.userId)) | ||
| .limit(1); |
There was a problem hiding this comment.
This database query is redundant. The template object is already available in the scope. You should join the user table when fetching the template earlier in the handler to avoid an extra round-trip to the database for every session creation.
| export const volumeMountSchema = z.array( | ||
| z.object({ | ||
| name: z.string().regex(VOLUME_NAME_RE, "Invalid volume name"), | ||
| mountPath: z.string().min(1), |
There was a problem hiding this comment.
The mountPath should be validated to ensure it is an absolute path (starting with /). Docker bind mounts typically require absolute paths for the container-side destination. Using a relative path can lead to unexpected behavior or container startup failures.
References
- Ensure input validation is robust and follows expected formats for external system calls (Docker). (link)
| items={[ | ||
| { label: "New volume...", value: "__new__" }, | ||
| ...existingVolumes.map((v) => ({ label: v.name, value: v.name })), | ||
| ]} |
There was a problem hiding this comment.
The items prop is not a standard property for the shadcn/ui Select component (which is a wrapper around Radix UI). The options are already being rendered manually inside SelectContent using SelectItem. This prop is redundant and should be removed to avoid confusion.
References
- Adhere to component library APIs to maintain code clarity and avoid redundant props. (link)
1 participant
Summary
<username>-<name>), shared across all sessions using the templateserver/lib/validation.tscompose.yml(prod, pulls image) andcompose.dev.yml(dev, builds locally)mineparam as enum)Test plan
npx tsc --noEmit— zero errorsnpm test— 81 unit tests pass<username>-<name>:<mountPath>🤖 Generated with Claude Code