refactor: remote environments with access tokens#401
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds agent mode across backend, frontend, and build tooling. Introduces an agent Dockerfile, compose, and CI workflow; extends backend config, middleware, bootstrap, models/DTOs, services, API handler, and migrations for agent pairing, image updates, and updater endpoints; updates frontend types, APIs, components, and routes to use environment-scoped image/update operations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed Checks (1 warning, 1 inconclusive)
✅ Passed Checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
backend/internal/database/database.go (1)
206-233: Bug: absolute SQLite paths are mangled; don’t strip leading “/” from u.Path
strings.TrimPrefix(pathPart, "/")turnsfile:/abs/path.dbintoabs/path.db. This can redirect data into the CWD and break expected permissions.Apply:
-func ensureSQLiteDirectory(connString string) error { +func ensureSQLiteDirectory(connString string) error { if !strings.HasPrefix(connString, "file:") { return nil } u, err := url.Parse(connString) if err != nil { return fmt.Errorf("failed to parse SQLite DSN: %w", err) } - // For "file:data/arcane.db?...", path is in Opaque; for "file:/abs/path.db", it's in Path - pathPart := u.Opaque - if pathPart == "" { - pathPart = u.Path - } - // Trim leading slash to handle file:/relative.db - pathPart = strings.TrimPrefix(pathPart, "/") + // For "file:data/arcane.db?...", path is in Opaque (relative); for "file:/abs/path.db", it's in Path (keep absolute) + var pathPart string + if u.Opaque != "" { + pathPart = u.Opaque + } else { + pathPart = u.Path // may be absolute like "/abs/path.db" — keep leading slash + } if pathPart == "" || strings.HasPrefix(pathPart, ":memory:") { return nil } dir := filepath.Dir(pathPart) if dir == "" || dir == "." { return nil } return os.MkdirAll(dir, 0o755) }Also consider recognizing all in-memory forms (e.g.,
file::memory:?cache=shared) as non-files.frontend/src/lib/components/image-update-item.svelte (1)
183-191: Remove stray “+” rendering in tooltip.There’s an extraneous "+" before the
<div>that will render in the UI.- + <div class="mt-2"> + <div class="mt-2">frontend/src/lib/components/sidebar/sidebar-env-switcher.svelte (1)
39-46: Async error handling is ineffective without awaiting setEnvironment.
The try/catch won’t catch promise rejections since setEnvironment isn’t awaited.Apply:
-function handleSelect(env: Environment) { - try { - environmentStore.setEnvironment(env); - } catch (error) { - console.error('Failed to set environment:', error); - toast.error('Failed to Connect to Environment'); - } -} +async function handleSelect(env: Environment) { + try { + await environmentStore.setEnvironment(env); + } catch (error) { + console.error('Failed to set environment:', error); + toast.error('Failed to connect to environment'); + } +}frontend/src/lib/components/sheets/new-environment-sheet.svelte (1)
11-11: Zod import/path and schema bug: use z.string().url(...) instead of z.url(...)
z.urldoes not exist (andzod/v4is a non-standard entrypoint). This will fail at build-time.Apply:
-import { z } from 'zod/v4'; +import { z } from 'zod'; @@ - name: z.string().min(1, 'Name is required').max(25, 'Name too long'), - apiUrl: z.url('Must be a valid URL').min(1, 'Server URL is required'), - bootstrapToken: z.string() + name: z.string().trim().min(1, 'Name is required').max(25, 'Name too long'), + apiUrl: z.string().trim().url('Must be a valid URL'), + // Optional: allow empty; we'll omit it from the DTO if blank + bootstrapToken: z.string().optional()Also applies to: 24-27
frontend/src/lib/services/api/environment-api-service.ts (1)
114-117: Bug: passing query params as POST body in pruneImagesAxios interprets
{ params }here as data, not config.- return this.handleResponse(this.api.post(`/environments/${envId}/images/prune`, { params })); + return this.handleResponse(this.api.post(`/environments/${envId}/images/prune`, undefined, { params }));backend/internal/api/environment_handler.go (2)
63-65: Gate sensitive routes with admin middleware (Create/Update/Delete, Pair, UpdaterRun).These mutate tokens or system state and should be admin-only.
Apply:
- apiGroup.POST("", handler.CreateEnvironment) + apiGroup.POST("", authMiddleware.WithAdminRequired().Add(), handler.CreateEnvironment) ... - apiGroup.PUT("/:id", handler.UpdateEnvironment) + apiGroup.PUT("/:id", authMiddleware.WithAdminRequired().Add(), handler.UpdateEnvironment) - apiGroup.DELETE("/:id", handler.DeleteEnvironment) + apiGroup.DELETE("/:id", authMiddleware.WithAdminRequired().Add(), handler.DeleteEnvironment) ... - apiGroup.POST("/:id/updater/run", handler.UpdaterRun) + apiGroup.POST("/:id/updater/run", authMiddleware.WithAdminRequired().Add(), handler.UpdaterRun) ... - apiGroup.POST("/:id/agent/pair", handler.PairAgent) + apiGroup.POST("/:id/agent/pair", authMiddleware.WithAdminRequired().Add(), handler.PairAgent)Also applies to: 68-70, 70-71, 133-135, 137-138
3-18: Validate ApiUrl to mitigate SSRF and bad inputs (Create).Reject empty/invalid/non-http(s) URLs before persisting or pairing.
Apply:
import ( "bytes" "fmt" "io" "net/http" + "net/url" "strings" "time"- env := &models.Environment{ - ApiUrl: req.ApiUrl, + if strings.TrimSpace(req.ApiUrl) == "" { + c.JSON(http.StatusBadRequest, gin.H{"success": false, "data": gin.H{"error": "apiUrl is required"}}) + return + } + if u, err := url.ParseRequestURI(req.ApiUrl); err != nil || (u.Scheme != "http" && u.Scheme != "https") { + c.JSON(http.StatusBadRequest, gin.H{"success": false, "data": gin.H{"error": "apiUrl must be a valid http(s) URL"}}) + return + } + env := &models.Environment{ + ApiUrl: req.ApiUrl, Enabled: true, }Also applies to: 578-585
🧹 Nitpick comments (57)
backend/resources/migrations/sqlite/010_rework_environments.down.sql (1)
1-4: SQLite DROP COLUMN may fail depending on engine; consider a safer rollback pathSQLite only supports
ALTER TABLE ... DROP COLUMNin newer versions; older deployments or alternate SQLite engines can fail. If prod nodes vary, prefer the “recreate table” pattern or confirm engine version parity before relying on DROP COLUMN. Also confirm no triggers/views depend on removed/added columns.Would you like me to draft a reversible down-migration that recreates
environmentswithoutaccess_tokenand copies data safely?docker/README.md (1)
1-4: Polish README wordingMinor grammar and tone fix.
Apply:
-# Docker folder - -this folder will be for the docker resources +# Docker + +This folder contains Docker resources.frontend/src/lib/components/error.svelte (1)
41-41: Button size now defaults — confirm UX intentDropping
size="sm"switches to the component’s default size. If the surrounding layouts assume compact CTAs, keepsize="sm"..vscode/settings.json (1)
11-15: Keep workspace settings minimal; avoid committing user/extension-specific prefs
kiroAgent.configureMCPis extension-specific. Consider moving it to personal settings to avoid surprising contributors.backend/internal/config/config.go (1)
29-34: Provide aLogValueimplementation forConfigto redact agent tokens
Add afunc (c Config) LogValue() slog.Value { … }inbackend/internal/config/config.gothat omits or masksAgentTokenandAgentBootstrapTokenwhen logging the struct (e.g. viaslog.Any("config", cfg)). No existing logs currently expose these fields.backend/internal/database/database.go (1)
206-233: Optional: robust in-memory DSN detectionCovers
file::memory:variants.Apply:
- if pathPart == "" || strings.HasPrefix(pathPart, ":memory:") { + if pathPart == "" || strings.Contains(pathPart, ":memory:") { return nil }frontend/src/routes/compose/new/+page.ts (1)
4-14: Thread SvelteKit fetch through load for SSR/cookies.Not passing
event.fetchcan drop auth cookies on the initial SSR render. Prefer a typed load and forwardfetchto your API client.+import type { PageLoad } from './$types'; -export const load = async () => { - const [allTemplates, envTemplate] = await Promise.all([ - templateAPI.loadAll().catch((err) => { +export const load: PageLoad = async ({ fetch }) => { + const [allTemplates, envTemplate] = await Promise.all([ + templateAPI.loadAll(fetch).catch((err) => { console.warn('Failed to load templates:', err); return []; }), - templateAPI.getEnvTemplate().catch((err) => { + templateAPI.getEnvTemplate(fetch).catch((err) => { console.warn('Failed to load env template:', err); return ''; }) ]);If
templateAPI.*doesn’t acceptfetch, consider injecting it via your API client factory instead.frontend/src/lib/types/image.type.ts (2)
86-92: Narrow VersionComparison string fields to literal unions.Constrain
updateTypeandchangeLevelto avoid typos and improve exhaustiveness in consumers.export interface VersionComparison { currentVersion: string; targetVersion: string; isNewer: boolean; - updateType: string; - changeLevel: string; + updateType: 'tag' | 'digest' | 'none'; + changeLevel: 'major' | 'minor' | 'patch' | 'prerelease' | 'unknown'; }
69-77: Align ImageUpdateData shape with UI assumptions (optional).UI treats
erroras optional andupdateTypeas one of a small set. Consider mirroring that here for stronger typing.Outside this hunk, update
ImageUpdateInfoDtoaccordingly:// in the same file, above export interface ImageUpdateInfoDto { hasUpdate: boolean; updateType: 'digest' | 'tag' | 'none' | 'error'; currentVersion: string; latestVersion: string; currentDigest: string; latestDigest: string; checkTime: string; responseTimeMs: number; error?: string; // optional; default to '' authMethod?: 'none' | 'anonymous' | 'credential' | 'unknown'; authUsername?: string; authRegistry?: string; usedCredential?: boolean; }frontend/src/lib/components/image-update-item.svelte (3)
460-466: Update tooltip text to reflect ID-based check.Message still references tag requirement.
- {#if canCheckUpdate()} - Click to check for updates from registry. - {:else} - Unable to check updates for images without proper tags. - {/if} + {#if canCheckUpdate()} + Click to check for updates from registry. + {:else} + Missing image ID; cannot check updates. + {/if}
81-83: Make digest display robust across algorithms.Avoid hard-coding indices; support e.g. sha256/sha384.
- if (localUpdateInfo?.updateType === 'digest' && localUpdateInfo?.latestDigest) { - return localUpdateInfo.latestDigest.slice(7, 19) + '...'; - } + if (localUpdateInfo?.updateType === 'digest' && localUpdateInfo?.latestDigest) { + const v = (localUpdateInfo.latestDigest.split(':').pop() ?? ''); + return v.slice(0, 12) + (v.length > 12 ? '...' : ''); + }
92-100: Consider normalizing errors to avoid leaking registry internals.
toast.error(result.error)may surface raw backend messages. Wrap/translate where possible.frontend/src/lib/components/sidebar/sidebar-env-switcher.svelte (1)
48-54: getEnvLabel: add a defensive fallback for empty names.
Not critical, but avoids blank labels if a remote env lacks a name.-function getEnvLabel(env: Environment): string { - if (env.isLocal) { - return 'Local Docker'; - } else { - return env.name; - } -} +function getEnvLabel(env: Environment): string { + if (env.isLocal) return 'Local Docker'; + const name = (env.name ?? '').trim(); + return name || env.apiUrl || 'Unnamed environment'; +}backend/internal/models/environment.go (1)
11-11: AccessToken JSON omission is good; consider at-rest protection.
Storing raw tokens is sometimes unavoidable; prefer DB/volume encryption or a secret store/KMS if feasible.backend/internal/middleware/cors_middleware.go (2)
74-79: Broaden dev defaults to common local ports and HTTPSCatches typical Vite/Next setups and HTTPS localhost to reduce friction.
if cfg == nil || cfg.Environment != "production" { origins = append(origins, - "http://localhost:3000", "http://127.0.0.1:3000", - "http://localhost:3552", "http://127.0.0.1:3552", + "http://localhost:3000", "http://127.0.0.1:3000", + "https://localhost:3000", "https://127.0.0.1:3000", + "http://localhost:3552", "http://127.0.0.1:3552", + "https://localhost:3552", "https://127.0.0.1:3552", + "http://localhost:5173", "http://127.0.0.1:5173", + "https://localhost:5173", "https://127.0.0.1:5173", ) }
61-63: Custom origins fully override derived origins; confirm this is intendedToday, any non-empty custom list discards AppUrl and dev defaults. If callers expect “additive” behavior, merge instead of override.
backend/resources/migrations/sqlite/011_add_environment_name.down.sql (1)
1-2: Drop index before dropping the column (safer order)While IF EXISTS guards help, dropping the index first avoids edge cases across SQLite versions.
-ALTER TABLE environments DROP COLUMN name; -DROP INDEX IF EXISTS idx_environments_name; +DROP INDEX IF EXISTS idx_environments_name; +ALTER TABLE environments DROP COLUMN name;backend/resources/migrations/sqlite/011_add_environment_name.up.sql (1)
2-2: Case-insensitive lookups: index with NOCASE.If names are user-facing, consider CI lookups.
-CREATE INDEX IF NOT EXISTS idx_environments_name ON environments(name); +CREATE INDEX IF NOT EXISTS idx_environments_name ON environments(name COLLATE NOCASE);frontend/src/routes/images/+page.ts (2)
17-21: Parallelize API calls to speed up page load.Replace sequential awaits with a single Promise.all.
const [totalSize, images, settings] = await Promise.all([ environmentAPI.getTotalImageSize(), environmentAPI.getImages(imageRequestOptions), settingsAPI.getSettings() ]);
17-21: Consider defensive error handling in load().If any call fails, SvelteKit may error the page. Optionally wrap with try/catch and return fallbacks.
backend/resources/migrations/postgres/011_add_environment_name.up.sql (1)
2-2: Case-insensitive search index.If UIs search names case-insensitively, add a functional index:
-CREATE INDEX IF NOT EXISTS idx_environments_name ON environments (name); +CREATE INDEX IF NOT EXISTS idx_environments_name ON environments (name); +CREATE INDEX IF NOT EXISTS idx_environments_name_ci ON environments (lower(name));Alternatively, use CITEXT.
frontend/src/lib/services/api/environment-management-api-service.ts (1)
12-15: Return-shape consistency.This returns
res.data(paginated object). Other methods unwrapres.data.data. Ensure consumers consistently expect this (e.g.,environments.data.datain +layout).frontend/src/routes/+layout.ts (2)
21-26: Avoid magic numbers for pagination.Extract
limit: 1000to a shared constant or env/config to keep behavior consistent across pages.
30-33: Double.dataaccess is brittle.
tryCatch(...).datathen.dataagain on the payload couples to transport shape. Consider normalizing API services to always return unwrapped domain objects.frontend/src/routes/containers/+page.svelte (1)
16-16: Type selectedIds.Add an explicit type for consistency with other pages.
let selectedIds = $state<string[]>([]);frontend/src/routes/environments/+page.svelte (1)
78-83: Avoid double-fetch and race in onEnvironmentCreatedYou fetch environments and then call refresh(), which fetches again (and isn’t awaited). Replace both with a single awaited refresh for consistency and fewer requests.
- async function onEnvironmentCreated() { - showEnvironmentSheet = false; - environments = await environmentManagementAPI.getEnvironments(requestOptions); - toast.success('Environment added successfully'); - refresh(); - } + async function onEnvironmentCreated() { + showEnvironmentSheet = false; + await refresh(); + toast.success('Environment added successfully'); + }frontend/src/routes/environments/[id]/+page.ts (1)
5-9: Explicitly map backend 404/401 to SvelteKit errors (optional)
Let errors bubble by default, but if you rely on custom status pages, wrap the API call in a try/catch and throw SvelteKiterrorwith the response status and message for precise handling:+import { error } from '@sveltejs/kit'; export const load: PageLoad = async ({ params }) => { - const environment = await environmentManagementAPI.get(params.id); - return { - environment - }; + try { + const environment = await environmentManagementAPI.get(params.id); + return { environment }; + } catch (e: any) { + throw error(e.status ?? 500, e.message ?? 'Failed to load environment'); + } };backend/resources/migrations/postgres/010_rework_environments.down.sql (1)
1-4: Align down migration with hashed token approach (if adopted)If you switch to access_token_hash in the up migration, drop that column here and (re)add only prior columns/indexes. Also consider whether hostname default '' is desired or should be NULL to reflect unknown.
-ALTER TABLE environments DROP COLUMN IF EXISTS access_token; +ALTER TABLE environments DROP COLUMN IF EXISTS access_token_hash;scripts/development/download-compose.sh (2)
13-27: Broaden arch detection fallback (optional)You already map amd64/arm64. Consider handling ppc64le/s390x explicitly when uname -m is used.
- case "${UNAME_ARCH}" in - x86_64|amd64) ARCH_GH="x86_64" ;; - aarch64|arm64) ARCH_GH="aarch64" ;; - *) ARCH_GH="x86_64" ;; - esac + case "${UNAME_ARCH}" in + x86_64|amd64) ARCH_GH="x86_64" ;; + aarch64|arm64) ARCH_GH="aarch64" ;; + ppc64le) ARCH_GH="ppc64le" ;; + s390x) ARCH_GH="s390x" ;; + *) ARCH_GH="x86_64" ;; + esac
31-35: Add checksum verification and atomic writeSecure the download by verifying the official checksums and writing atomically to avoid partial binaries.
-echo "Downloading docker-compose v${VERSION} (${ARCH_GH}) -> ${DEST}" -mkdir -p "$(dirname "${DEST}")" -curl -fsSL --retry 3 --retry-delay 2 -o "${DEST}" "${URL}" -chmod +x "${DEST}" -echo "docker-compose installed at ${DEST}" +echo "Downloading docker-compose v${VERSION} (${ARCH_GH}) -> ${DEST}" +mkdir -p "$(dirname "${DEST}")" +TMP="$(mktemp)" +curl -fsSL --retry 3 --retry-delay 2 -o "${TMP}" "${URL}" +# Optional checksum verification (requires sha256sum) +if command -v sha256sum >/dev/null 2>&1; then + SUM_URL="https://github.com/docker/compose/releases/download/v${VERSION}/checksums.txt" + curl -fsSL --retry 3 --retry-delay 2 "${SUM_URL}" | grep "docker-compose-linux-${ARCH_GH}\$" | sha256sum -c - >/dev/null +fi +install -m 0755 "${TMP}" "${DEST}" +rm -f "${TMP}" +echo "docker-compose installed at ${DEST}"backend/internal/bootstrap/bootstrap.go (1)
60-66: Safe token loading without leaking contentsGood: you don’t log the token value. Consider masking the log even on debug and validating non-empty after trim.
- if tok := appServices.Settings.GetStringSetting(appCtx, "agentToken", ""); tok != "" { + if tok := strings.TrimSpace(appServices.Settings.GetStringSetting(appCtx, "agentToken", "")); tok != "" {backend/internal/services/environment_service.go (1)
257-269: Persist metadata and verify update succeeded.Update updated_at and check RowsAffected to surface not-found.
Apply this diff:
func (s *EnvironmentService) PairAndPersistAgentToken(ctx context.Context, environmentID, apiUrl, bootstrapToken string) (string, error) { token, err := s.PairAgentWithBootstrap(ctx, apiUrl, bootstrapToken) if err != nil { return "", err } - if err := s.db.WithContext(ctx). - Model(&models.Environment{}). - Where("id = ?", environmentID). - Update("access_token", token).Error; err != nil { - return "", fmt.Errorf("failed to persist agent token: %w", err) - } + res := s.db.WithContext(ctx). + Model(&models.Environment{}). + Where("id = ?", environmentID). + Updates(map[string]interface{}{ + "access_token": token, + "updated_at": time.Now(), + }) + if res.Error != nil { + return "", fmt.Errorf("failed to persist agent token: %w", res.Error) + } + if res.RowsAffected == 0 { + return "", fmt.Errorf("environment not found") + } return token, nil }Dockerfile-agent (2)
29-31: Avoid apk upgrade in final image.
apk upgradebloats images and can introduce nondeterminism.Apply this diff:
-FROM alpine:3 AS agent -RUN apk upgrade && apk --no-cache add ca-certificates tzdata curl su-exec +FROM alpine:3 AS agent +RUN apk --no-cache add ca-certificates tzdata curl su-exec
32-37: Optional hardening: drop root and add healthcheck.Run as non-root and add a lightweight healthcheck.
Apply this diff:
WORKDIR /app RUN mkdir -p /app/data && chmod 755 /app/data COPY --from=agent-builder /out/arcane-agent /usr/local/bin/arcane-agent COPY --from=agent-builder /out/docker-compose /usr/local/bin/docker-compose COPY --chmod=755 scripts/docker/entrypoint.sh /usr/local/bin/entrypoint.sh ENTRYPOINT ["/usr/local/bin/entrypoint.sh"] @@ EXPOSE 3553 VOLUME ["/app/data"] +RUN addgroup -S arcane && adduser -S arcane -G arcane -h /app +USER arcane +HEALTHCHECK --interval=30s --timeout=3s --retries=3 CMD wget -qO- http://127.0.0.1:3553/health || exit 1 + CMD ["/usr/local/bin/arcane-agent"]Also applies to: 38-40, 46-49
docker-compose.agent.yml (3)
11-20: Add a container healthcheck.Helps orchestration and self-healing.
ports: - '3553:3553' @@ restart: unless-stopped + healthcheck: + test: ["CMD-SHELL", "curl -fsS http://localhost:3553/health >/dev/null"] + interval: 10s + timeout: 3s + retries: 3 + start_period: 15s
16-18: Docker socket mount: high-privilege surface. Review threat model.If rootless/remote Docker is viable, prefer it; otherwise consider read-only where possible and least-privileged group mappings.
Would you like a hardened compose variant (read-only rootfs, tmpfs for /tmp, user mapping via PUID/PGID)?
10-10: Avoid hard-coded container_name.It breaks multi-instance workflows and collides in shared hosts.
Remove
container_nameunless required for external references.Dockerfile (2)
43-45: Avoidapk upgradein final image for reproducibility and smaller attack surface.-FROM alpine:3 AS runner -RUN apk upgrade && apk --no-cache add ca-certificates tzdata curl shadow su-exec +FROM alpine:3 AS runner +RUN apk --no-cache add ca-certificates tzdata curl shadow su-exec
55-55: Ensure docker-compose is executable and add a basic healthcheck.COPY --from=backend-builder /out/docker-compose /usr/local/bin/docker-compose +RUN chmod +x /usr/local/bin/docker-compose +HEALTHCHECK --interval=30s --timeout=3s --retries=3 CMD curl -fsS http://localhost:${PORT}/health || exit 1frontend/src/routes/environments/environment-table.svelte (2)
77-100: Rename ‘hostname’ to ‘name’ for clarity and consistency.-async function handleDeleteOne(id: string, hostname: string) { +async function handleDeleteOne(id: string, name: string) { @@ - message: `Are you sure you want to remove "${hostname}"? This action cannot be undone.`, + message: `Are you sure you want to remove "${name}"? This action cannot be undone.`, @@ - message: `Failed to remove environment "${hostname}"`, + message: `Failed to remove environment "${name}"`, @@ - toast.success(`Removed "${hostname}"`); + toast.success(`Removed "${name}"`); @@ - onclick={() => handleDeleteOne(item.id, item.name)} + onclick={() => handleDeleteOne(item.id, item.name)}Also applies to: 200-206
48-59: Option: delete in parallel to reduce latency on bulk removes.- for (const id of ids) { - const result = await tryCatch(environmentManagementAPI.delete(id)); - handleApiResultWithCallbacks({ result, message: `Failed to remove environment`, setLoadingState: () => {}, onSuccess: () => { successCount++; } }); - if (result.error) failureCount++; - } + const results = await Promise.all( + ids.map(async (id) => ({ id, result: await tryCatch(environmentManagementAPI.delete(id)) })) + ); + for (const { result } of results) { + handleApiResultWithCallbacks({ result, message: `Failed to remove environment`, setLoadingState: () => {}, onSuccess: () => { successCount++; } }); + if (result.error) failureCount++; + }Dockerfile-static (3)
7-8: Avoidapk upgrade; keep the runtime minimal.-RUN apk upgrade && apk --no-cache add ca-certificates tzdata curl su-exec \ +RUN apk --no-cache add ca-certificates tzdata curl su-exec \ && (delgroup ping || true) && (apk del iputils || true)
10-13: Ensure docker-compose is executable.RUN set -eux; chmod +x /tmp/download-compose.sh; \ /tmp/download-compose.sh "${COMPOSE_VERSION}" "/usr/local/bin/docker-compose" "${TARGETARCH}" +RUN chmod +x /usr/local/bin/docker-compose
18-21: Add a healthcheck for the static image, too.EXPOSE 3552 ENV ENVIRONMENT=production +HEALTHCHECK --interval=30s --timeout=3s --retries=3 CMD curl -fsS http://localhost:3552/health || exit 1frontend/src/routes/environments/[id]/+page.svelte (2)
237-243: Harden token input to prevent browser persistence and hints.-<input +<input class="bg-background focus:ring-primary mt-1 w/full rounded-md border px-3 py-2 text-sm outline-none focus:ring-2" type="password" placeholder="Enter AGENT_BOOTSTRAP_TOKEN" bind:value={bootstrapToken} + autocomplete="off" + autocapitalize="off" + spellcheck="false" + inputmode="text" />
84-85: Optional fallback for display name.-const environmentDisplayName = $derived(environment.name); +const environmentDisplayName = $derived(environment.name || environment.id);backend/internal/bootstrap/router_bootstrap.go (2)
30-39: Broaden skip patterns to reduce noisy asset logs.loggerSkipPatterns := []string{ - "GET /api/containers/*/stats/stream", - "GET /api/containers/*/logs/stream", - "GET /_app", - "GET /img", - "GET /fonts", + "GET /api/containers/*/stats/stream", + "GET /api/containers/*/logs/stream", + "GET /_app*", + "GET /img/*", + "GET /fonts/*", "GET /api/system/stats", "GET /health", "HEAD /health", }
65-68: Serve HEAD /health explicitly.Some clients probe with HEAD.
router.GET("/health", func(c *gin.Context) { c.JSON(http.StatusOK, gin.H{"status": "UP"}) }) +router.HEAD("/health", func(c *gin.Context) { + c.Status(http.StatusOK) +})frontend/src/lib/components/sheets/new-environment-sheet.svelte (2)
110-116: Harden token input UX to reduce leakage via browser autofill/spellcheckDisable autocomplete/capitalization and spellcheck for secrets.
- <FormInput + <FormInput label="Bootstrap Token" type="password" placeholder="AGENT_BOOTSTRAP_TOKEN from the agent" description="If provided, manager will auto‑pair with the agent and store the generated token" - bind:input={$inputs.bootstrapToken} + autocomplete="off" + autocapitalize="off" + spellcheck={false} + bind:input={$inputs.bootstrapToken} />
66-68: Close the sheet on success (optional UX improvement)Keeps flow tight after creation. If parent doesn’t handle it, set
open = falsehere.form.reset(); onEnvironmentCreated?.(); + open = false;Also applies to: 77-77
backend/internal/middleware/auth_middleware.go (1)
53-56: Constrain bootstrap pairing to POST (defense-in-depth)Avoids granting admin context on non-intended verbs.
- if strings.HasPrefix(c.Request.URL.Path, "/api/environments/0/agent/pair") && + if c.Request.Method == http.MethodPost && + strings.HasPrefix(c.Request.URL.Path, "/api/environments/0/agent/pair") && m.cfg.AgentBootstrapToken != "" && c.GetHeader("X-Arcane-Agent-Bootstrap") == m.cfg.AgentBootstrapToken {frontend/src/lib/services/api/environment-api-service.ts (1)
321-379: Optional: add generics for stronger typing on responsesImproves DX and narrows
any.Example:
- return this.handleResponse(this.api.get(`/environments/${envId}/image-updates/summary`)); + return this.handleResponse<ImageUpdateSummary>(this.api.get(`/environments/${envId}/image-updates/summary`));Apply similarly to other new endpoints.
backend/internal/api/environment_handler.go (5)
685-705: Avoid no-op update after successful pairing.If only pairing occurred, skip UpdateEnvironment and return the refreshed entity.
Apply:
- if (req.AccessToken == nil) && req.BootstrapToken != nil && *req.BootstrapToken != "" { + paired := false + if (req.AccessToken == nil) && req.BootstrapToken != nil && *req.BootstrapToken != "" { ... - if _, err := h.environmentService.PairAndPersistAgentToken(c.Request.Context(), environmentID, apiUrl, *req.BootstrapToken); err != nil { + if _, err := h.environmentService.PairAndPersistAgentToken(c.Request.Context(), environmentID, apiUrl, *req.BootstrapToken); err != nil { ... - } + } else { paired = true } } else if req.AccessToken != nil { updates["access_token"] = *req.AccessToken } - updated, err := h.environmentService.UpdateEnvironment(c.Request.Context(), environmentID, updates) + if paired && len(updates) == 0 { + updated, err := h.environmentService.GetEnvironmentByID(c.Request.Context(), environmentID) + if err != nil || updated == nil { + c.JSON(http.StatusInternalServerError, gin.H{"success": false, "data": gin.H{"error": "Failed to load updated environment"}}) + return + } + out, mapErr := dto.MapOne[*models.Environment, dto.EnvironmentDto](updated) + if mapErr != nil { + c.JSON(http.StatusInternalServerError, gin.H{"success": false, "data": gin.H{"error": "Failed to map environment"}}) + return + } + c.JSON(http.StatusOK, gin.H{"success": true, "data": out}) + return + } + + updated, err := h.environmentService.UpdateEnvironment(c.Request.Context(), environmentID, updates)Also applies to: 705-718
763-766: Standardize error envelope to match other handlers.Use data.error for consistency.
Apply:
- c.JSON(http.StatusInternalServerError, gin.H{ - "success": false, - "error": "Failed to update heartbeat", - }) + c.JSON(http.StatusInternalServerError, gin.H{ + "success": false, + "data": gin.H{"error": "Failed to update heartbeat"}, + })
574-574: Align error messages for bad JSON.Use a consistent phrase across Create/Update (e.g., “Invalid request body: …”).
Also applies to: 670-671
210-214: Avoid per-request handler allocations.Store UpdaterHandler/ImageUpdateHandler on EnvironmentHandler and reuse.
Also applies to: 238-241
479-568: Consider httputil.ReverseProxy to reduce proxy footguns.It handles header normalization, hop-by-hop filtering, streaming, and error propagation out of the box.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
.github/workflows/build-agent-image.yml(1 hunks).vscode/settings.json(1 hunks)Dockerfile(2 hunks)Dockerfile-agent(1 hunks)Dockerfile-static(1 hunks)backend/internal/api/environment_handler.go(8 hunks)backend/internal/bootstrap/bootstrap.go(3 hunks)backend/internal/bootstrap/router_bootstrap.go(1 hunks)backend/internal/config/config.go(2 hunks)backend/internal/database/database.go(3 hunks)backend/internal/dto/environment_dto.go(2 hunks)backend/internal/middleware/auth_middleware.go(4 hunks)backend/internal/middleware/cors_middleware.go(1 hunks)backend/internal/models/environment.go(1 hunks)backend/internal/services/environment_service.go(3 hunks)backend/internal/services/settings_service.go(2 hunks)backend/internal/utils/encryption.go(1 hunks)backend/resources/migrations/postgres/010_rework_environments.down.sql(1 hunks)backend/resources/migrations/postgres/010_rework_environments.up.sql(1 hunks)backend/resources/migrations/postgres/011_add_environment_name.down.sql(1 hunks)backend/resources/migrations/postgres/011_add_environment_name.up.sql(1 hunks)backend/resources/migrations/sqlite/010_rework_environments.down.sql(1 hunks)backend/resources/migrations/sqlite/010_rework_environments.up.sql(1 hunks)backend/resources/migrations/sqlite/011_add_environment_name.down.sql(1 hunks)backend/resources/migrations/sqlite/011_add_environment_name.up.sql(1 hunks)docker-compose.agent.yml(1 hunks)docker/README.md(1 hunks)frontend/src/lib/components/error.svelte(1 hunks)frontend/src/lib/components/image-update-item.svelte(2 hunks)frontend/src/lib/components/sheets/new-environment-sheet.svelte(4 hunks)frontend/src/lib/components/sidebar/sidebar-env-switcher.svelte(5 hunks)frontend/src/lib/services/api/autoupdate-api-service.ts(0 hunks)frontend/src/lib/services/api/environment-api-service.ts(2 hunks)frontend/src/lib/services/api/environment-management-api-service.ts(1 hunks)frontend/src/lib/services/api/image-api-service.ts(0 hunks)frontend/src/lib/services/api/image-update-api-service.ts(0 hunks)frontend/src/lib/services/api/index.ts(0 hunks)frontend/src/lib/stores/environment.store.ts(2 hunks)frontend/src/lib/types/environment.type.ts(1 hunks)frontend/src/lib/types/image.type.ts(1 hunks)frontend/src/lib/utils/redirect.util.ts(0 hunks)frontend/src/routes/+layout.svelte(0 hunks)frontend/src/routes/+layout.ts(2 hunks)frontend/src/routes/compose/+page.svelte(2 hunks)frontend/src/routes/compose/new/+page.ts(1 hunks)frontend/src/routes/containers/+page.svelte(2 hunks)frontend/src/routes/environments/+page.svelte(2 hunks)frontend/src/routes/environments/[id]/+page.svelte(7 hunks)frontend/src/routes/environments/[id]/+page.ts(1 hunks)frontend/src/routes/environments/environment-table.svelte(4 hunks)frontend/src/routes/images/+page.svelte(2 hunks)frontend/src/routes/images/+page.ts(1 hunks)scripts/development/download-compose.sh(1 hunks)
💤 Files with no reviewable changes (6)
- frontend/src/lib/utils/redirect.util.ts
- frontend/src/lib/services/api/image-api-service.ts
- frontend/src/routes/+layout.svelte
- frontend/src/lib/services/api/autoupdate-api-service.ts
- frontend/src/lib/services/api/image-update-api-service.ts
- frontend/src/lib/services/api/index.ts
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-agent-image.yml
32-32: shellcheck reported issue in this script: SC2086:info:3:49: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (50)
backend/resources/migrations/postgres/011_add_environment_name.down.sql (1)
1-2: LGTM — idempotent down migrationIndex and column drops are guarded with IF EXISTS; safe to apply multiple times.
backend/internal/database/database.go (1)
98-101: Good call creating directories for file-based SQLitePre-creating the parent dir removes a common first-run footgun.
frontend/src/lib/components/image-update-item.svelte (2)
12-15: All imageUpdateAPI references replaced Verified thatenvironmentAPI.checkImageUpdateByIDis used and noimageUpdateAPIimports remain.
34-36: The scripts will verify the presence ofimageIdand show the relevant UI markup for tooltip updates.frontend/src/lib/types/environment.type.ts (2)
1-13: Environment model looks good; consider adding 'unknown' status.If initial state is indeterminate, an 'unknown' variant can help avoid overloading 'offline'.
[ suggest_optional_refactor ]-export type EnvironmentStatus = 'online' | 'offline' | 'error'; +export type EnvironmentStatus = 'online' | 'offline' | 'error' | 'unknown';
17-19: Bootstrap token on the client: verify exposure.Ensure this token is only used transiently and never persisted in stores/logs.
frontend/src/lib/stores/environment.store.ts (2)
3-4: Imports look correct and consistent with type centralization.
Using goto and importing Environment from the shared type module aligns with the refactor.
107-110: Correct use of goto with invalidateAll to refresh loads.
Navigating to the current URL with { replaceState: true, invalidateAll: true } is a valid way to re-run load functions after switching environments. (svelte.dev)frontend/src/lib/components/sidebar/sidebar-env-switcher.svelte (4)
10-11: Type/import sourcing LGTM.
Pulling Environment from $lib/types keeps components decoupled from the store.
82-82: Using getEnvLabel for the current selection is correct.
Keeps display logic centralized.
128-128: Using getEnvLabel in the list is consistent.
Good reuse and consistent UX for local vs remote entries.
139-139: Separator inside the admin block makes sense.
Limits the visual split to admin-only actions.backend/internal/models/environment.go (3)
10-10: Confirm whether LastSeen should remain sortable.
The sortable tag was removed; if list endpoints rely on it, consider restoring.Option (if needed):
- LastSeen *time.Time `json:"lastSeen" gorm:"column:last_seen"` + LastSeen *time.Time `json:"lastSeen" gorm:"column:last_seen" sortable:"true"`
16-16: TableName override LGTM.
Explicit table binding is clear and avoids surprises.
13-13: No action needed: BaseModel JSON tags (id,createdAt,updatedAt) match frontend expectations.backend/internal/middleware/cors_middleware.go (2)
48-55: Verify usage of X-Arcane-Agent-Token before exposing
No occurrences of this header were found in the codebase; confirm whether any handler actually setsX-Arcane-Agent-Token. If so, append it toconf.ExposeHeaders; otherwise, omit this change.
3-5: go.mod specifies Go 1.25.0 which meets log/slog’s minimum requirement of Go 1.21+.backend/internal/dto/environment_dto.go (1)
5-8: Pointer fields for partial updates look goodUsing pointers for Name/Enabled/AccessToken/BootstrapToken enables PATCH semantics and avoids accidental zeroing.
Also applies to: 13-16
backend/resources/migrations/sqlite/011_add_environment_name.down.sql (1)
1-2: Confirm minimum SQLite versionALTER TABLE ... DROP COLUMN requires SQLite 3.35+. Ensure runtime/CI uses ≥3.35 or provide a backfill (table-rebuild) down migration.
backend/resources/migrations/sqlite/011_add_environment_name.up.sql (1)
1-2: Do you need uniqueness?If environment names must be unique, make the index UNIQUE (and validate data before migration). Otherwise, ignore.
frontend/src/routes/images/+page.ts (1)
1-1: Import consolidation to environmentAPI/settingsAPI looks good.backend/resources/migrations/sqlite/010_rework_environments.up.sql (2)
4-4: Should api_url be unique?If duplicate environments with same
api_urlaren’t allowed, add a UNIQUE index (or composite with tenant/org). Validate data before enforcing.
1-2: SQLite version dependency.
ALTER TABLE ... DROP COLUMNrequires SQLite ≥ 3.35.0. Confirm minimum supported version or use the copy-table pattern.backend/resources/migrations/postgres/011_add_environment_name.up.sql (2)
1-1: Align with SQLite semantics.Current PG: nullable
name. If you accept my SQLite change (remove DEFAULT ''), you’re aligned. Otherwise, addDEFAULT ''here for parity.
1-2: Uniqueness policy?If names must be unique, enforce with
UNIQUE(orUNIQUE (lower(name))).frontend/src/routes/compose/+page.svelte (1)
32-38: Verify environment scoping for runAutoUpdate().Ensure this call targets the currently selected environment (store initialized before use), or pass an explicit environment ID.
frontend/src/lib/services/api/environment-management-api-service.ts (2)
2-3: Type import source change looks correct.
6-35: List() removal verified – no call sites remain
Search across the frontend codebase found no references to.list(); nothing further to update.frontend/src/routes/containers/+page.svelte (1)
28-35: Verify environment scoping for runAutoUpdate().Same as Compose: confirm it targets the active environment or pass an ID.
frontend/src/routes/environments/+page.svelte (1)
118-118: LGTM on direct table renderRemoving the Card wrapper is fine; bindings for environments, selectedIds, and requestOptions remain correct.
frontend/src/routes/images/+page.svelte (2)
13-13: API consolidation looks goodSwitching to environmentAPI import aligns with backend handler consolidation.
52-52: Confirmed checkMultipleImages contract: it returns aPromise<Record<string, ImageUpdateInfoDto>>and useshandleResponse, which throws on non-2xx responses, so the existingtry/catchand success toast remain correct.backend/internal/bootstrap/bootstrap.go (2)
110-121: Agent-mode bootstrap scope looks correctSkipping default admin and OIDC sync in agent mode matches the intended separation of concerns.
67-76: No changes needed for production key handling InitEncryption already panics whencfg.EncryptionKeyis empty in production (non-agent), satisfying the fail-fast requirement.backend/internal/utils/encryption.go (1)
24-27: Production agents should not bypass key requirements if persistence fails.If AgentMode is true but persistence errors occur, the current logic would still proceed. Ensure failures panic/abort rather than silently running with weak keys.
Do you want me to wire this to your EnsureEncryptionKey bootstrap path instead and keep a hard requirement in production?
backend/internal/services/environment_service.go (1)
66-69: Search scope reduction: confirm intentional UX change.Filtering only by api_url (dropping hostname/description) narrows search results.
If unintentional, I can submit a patch to support multiple fields while keeping the query indexed.
Dockerfile-agent (2)
41-44: Agent runs with ENVIRONMENT=production; ensure strong per-agent key (ties to encryption.go fix).If you keep production here, avoid static keys and persist a unique key per agent.
Want me to wire a one-time key generation in entrypoint to guarantee a persisted 32‑byte key before starting the binary?
2-2: No changes needed: base image matches go.mod’s Go 1.25.0
golang:1.25-alpine exists on Docker Hub and aligns with the Go version in backend/go.mod.Dockerfile (1)
46-50: Privilege drop verified
Entrypoint.sh at line 106 usesexec su-exec "$APP_USER" "$@"to switch to the configured PUID/PGID before starting the application.frontend/src/routes/environments/environment-table.svelte (2)
121-126: LGTM: Friendly Name migration is wired correctly.Also applies to: 160-160
200-201: No action required:DropdownMenu.Itemsupportsvariant="destructive".frontend/src/routes/environments/[id]/+page.svelte (1)
65-82: Bootstrap token isn’t logged in the frontend—confirm server-side redaction
Ensure your backend’s request-logging or telemetry strips/redactsbootstrapTokenfrom any logged payloads.backend/internal/bootstrap/router_bootstrap.go (2)
61-64: LGTM: cfg-aware CORS and Auth wiring.This keeps middleware behavior consistent across modes.
72-75: Auth on agent pairing/rotate endpoints confirmed ExistingNewEnvironmentHandleris mounted underauthMiddleware.Add(), which in AgentMode permits only requests bearing the configuredAgentBootstrapTokenheader (X-Arcane-Agent-Bootstrap) or valid credentials for/api/environments/0/agent/pair(including rotate) and rejects all others.backend/internal/middleware/auth_middleware.go (1)
51-81: AgentMode forbids JWT completely — confirm intended behaviorAs written, when AgentMode=true, requests without a valid agent header get 403 (no JWT fallback). This likely blocks UI/API endpoints that expect normal auth.
Do we want:
- Agent-only endpoints to require agent headers, but allow JWT elsewhere; or
- Global agent-only?
If the former, consider falling back to JWT for non-agent paths:
- if m.cfg != nil && m.cfg.AgentMode { + if m.cfg != nil && m.cfg.AgentMode { // ... existing agent header checks ... - c.JSON(http.StatusForbidden, models.APIError{ ... }); c.Abort(); return + // If not an agent path, continue with normal JWT flow + if !strings.HasPrefix(c.Request.URL.Path, "/api/environments/") { + // fall through to JWT path below + } else { + c.JSON(http.StatusForbidden, models.APIError{ + Code: "FORBIDDEN", + Message: "Invalid or missing agent token", + }) + c.Abort() + return + } }frontend/src/lib/services/api/environment-api-service.ts (2)
365-373: LGTM on updater run/statusConsistent usage of
handleResponsehere.
375-379: LGTM on history endpointReturns
handleResponse(this.api.get(...))with optional params; good.backend/internal/api/environment_handler.go (3)
490-495: LGTM: target URL construction preserves query string.
123-131: Nice addition: image-update endpoints wiring is clean and scoped.
152-154: No changes required —utils.GenerateRandomStringusesrand.Read, which is from Go’scrypto/randpackage, so the token generation is already cryptographically secure.
| - name: Set DOCKER_IMAGE_NAME | ||
| run: | | ||
| REPO_OWNER=${{ github.repository_owner }} | ||
| DOCKER_IMAGE_NAME="ghcr.io/${REPO_OWNER,,}/arcane-agent" | ||
| echo "DOCKER_IMAGE_NAME=${DOCKER_IMAGE_NAME}" >>${GITHUB_ENV} | ||
|
|
There was a problem hiding this comment.
Quote GITHUB_ENV redirection to satisfy shellcheck and avoid word-splitting
Fix SC2086 by quoting $GITHUB_ENV and explicitly using bash.
Apply:
- - name: Set DOCKER_IMAGE_NAME
- run: |
- REPO_OWNER=${{ github.repository_owner }}
- DOCKER_IMAGE_NAME="ghcr.io/${REPO_OWNER,,}/arcane-agent"
- echo "DOCKER_IMAGE_NAME=${DOCKER_IMAGE_NAME}" >>${GITHUB_ENV}
+ - name: Set DOCKER_IMAGE_NAME
+ shell: bash
+ run: |
+ set -euo pipefail
+ REPO_OWNER='${{ github.repository_owner }}'
+ DOCKER_IMAGE_NAME="ghcr.io/${REPO_OWNER,,}/arcane-agent"
+ { echo "DOCKER_IMAGE_NAME=${DOCKER_IMAGE_NAME}"; } >> "$GITHUB_ENV"📝 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.
| - name: Set DOCKER_IMAGE_NAME | |
| run: | | |
| REPO_OWNER=${{ github.repository_owner }} | |
| DOCKER_IMAGE_NAME="ghcr.io/${REPO_OWNER,,}/arcane-agent" | |
| echo "DOCKER_IMAGE_NAME=${DOCKER_IMAGE_NAME}" >>${GITHUB_ENV} | |
| - name: Set DOCKER_IMAGE_NAME | |
| shell: bash | |
| run: | | |
| set -euo pipefail | |
| REPO_OWNER='${{ github.repository_owner }}' | |
| DOCKER_IMAGE_NAME="ghcr.io/${REPO_OWNER,,}/arcane-agent" | |
| { echo "DOCKER_IMAGE_NAME=${DOCKER_IMAGE_NAME}"; } >> "$GITHUB_ENV" |
🧰 Tools
🪛 actionlint (1.7.7)
32-32: shellcheck reported issue in this script: SC2086:info:3:49: Double quote to prevent globbing and word splitting
(shellcheck)
🤖 Prompt for AI Agents
.github/workflows/build-agent-image.yml around lines 31-36: the GITHUB_ENV
redirection is unquoted (SC2086) and the step should explicitly run under bash;
update the step to run with bash and quote the redirection (e.g. use printf/echo
>> "$GITHUB_ENV") and quote any expansions used in the redirection so the
environment file path is not subject to word-splitting.
…ove LastSeen from EnvironmentDto
1 participant
This should not be tested in the current state in a Prod environment, this still is a WIP
Summary by CodeRabbit
New Features
DevOps
Migrations
UI/UX
Documentation