diff --git a/tests/harness/compose.yml b/tests/harness/compose.yml index debbb675..e209287d 100644 --- a/tests/harness/compose.yml +++ b/tests/harness/compose.yml @@ -94,6 +94,13 @@ services: CP_UPSTREAM_URL: "http://cp-stub:9090" RATE_LIMIT: "1000" CANVAS_PROXY_URL: "http://localhost:3000" + # Memory v2 sidecar (PR #2906) bundles the plugin into the + # tenant image and starts it before the main server. The plugin + # runs `CREATE EXTENSION vector` on first boot, which fails on + # the harness's plain postgres:15-alpine (no pgvector). The + # harness doesn't exercise memory features, so disable the + # sidecar via the entrypoint's documented escape hatch. + MEMORY_PLUGIN_DISABLE: "1" networks: [harness-net] healthcheck: test: ["CMD-SHELL", "wget -q -O- http://localhost:8080/health || exit 1"] @@ -142,6 +149,13 @@ services: CP_UPSTREAM_URL: "http://cp-stub:9090" RATE_LIMIT: "1000" CANVAS_PROXY_URL: "http://localhost:3000" + # Memory v2 sidecar (PR #2906) bundles the plugin into the + # tenant image and starts it before the main server. The plugin + # runs `CREATE EXTENSION vector` on first boot, which fails on + # the harness's plain postgres:15-alpine (no pgvector). The + # harness doesn't exercise memory features, so disable the + # sidecar via the entrypoint's documented escape hatch. + MEMORY_PLUGIN_DISABLE: "1" networks: [harness-net] healthcheck: test: ["CMD-SHELL", "wget -q -O- http://localhost:8080/health || exit 1"] diff --git a/workspace-server/Dockerfile b/workspace-server/Dockerfile index 7065e405..ecf43fab 100644 --- a/workspace-server/Dockerfile +++ b/workspace-server/Dockerfile @@ -21,6 +21,14 @@ ARG GIT_SHA=dev RUN CGO_ENABLED=0 GOOS=linux go build \ -ldflags "-X github.com/Molecule-AI/molecule-monorepo/platform/internal/buildinfo.GitSHA=${GIT_SHA}" \ -o /platform ./cmd/server +# Bundle the built-in memory-plugin-postgres binary so an operator can +# activate Memory v2 by setting MEMORY_V2_CUTOVER=true + (default) +# MEMORY_PLUGIN_URL=http://localhost:9100. The entrypoint starts this +# binary in the background; main /platform talks to it over loopback. +# Stays inert until the operator flips the cutover env var. +RUN CGO_ENABLED=0 GOOS=linux go build \ + -ldflags "-X github.com/Molecule-AI/molecule-monorepo/platform/internal/buildinfo.GitSHA=${GIT_SHA}" \ + -o /memory-plugin ./cmd/memory-plugin-postgres # Clone templates + plugins at build time from manifest.json FROM alpine:3.20 AS templates @@ -30,8 +38,9 @@ COPY scripts/clone-manifest.sh /scripts/clone-manifest.sh RUN chmod +x /scripts/clone-manifest.sh && /scripts/clone-manifest.sh /manifest.json /workspace-configs-templates /org-templates /plugins FROM alpine:3.20 -RUN apk add --no-cache ca-certificates git tzdata +RUN apk add --no-cache ca-certificates git tzdata wget COPY --from=builder /platform /platform +COPY --from=builder /memory-plugin /memory-plugin COPY workspace-server/migrations /migrations COPY --from=templates /workspace-configs-templates /workspace-configs-templates COPY --from=templates /org-templates /org-templates @@ -41,6 +50,7 @@ RUN addgroup -g 1000 platform && adduser -u 1000 -G platform -s /bin/sh -D platf EXPOSE 8080 COPY <<'ENTRY' /entrypoint.sh #!/bin/sh +# Set up docker-socket group (unchanged from pre-sidecar entrypoint). if [ -S /var/run/docker.sock ]; then SOCK_GID=$(stat -c '%g' /var/run/docker.sock 2>/dev/null || stat -f '%g' /var/run/docker.sock 2>/dev/null) if [ -n "$SOCK_GID" ] && [ "$SOCK_GID" != "0" ]; then @@ -50,6 +60,52 @@ if [ -S /var/run/docker.sock ]; then addgroup platform root 2>/dev/null || true fi fi + +# Memory v2 sidecar (built-in postgres plugin). Co-located with the +# main server so operators flipping MEMORY_V2_CUTOVER=true don't need +# to provision a separate service. Stays inert at the protocol layer +# until that env var is set — the workspace-server's wiring.go skips +# building the client without MEMORY_PLUGIN_URL, so the running plugin +# is a no-op for traffic. +# +# Env defaults: +# MEMORY_PLUGIN_DATABASE_URL = $DATABASE_URL (share existing Postgres; +# plugin's `memory_namespaces` / `memory_records` tables coexist +# with `agent_memories` and the rest of the platform schema — +# no conflicts. Operator can override with a separate URL.) +# MEMORY_PLUGIN_LISTEN_ADDR = :9100 +# +# Set MEMORY_PLUGIN_DISABLE=1 to skip launching the sidecar entirely +# (e.g. an operator running the plugin externally on a separate host). +if [ -z "$MEMORY_PLUGIN_DISABLE" ] && [ -n "$DATABASE_URL" ]; then + : "${MEMORY_PLUGIN_DATABASE_URL:=$DATABASE_URL}" + : "${MEMORY_PLUGIN_LISTEN_ADDR:=:9100}" + export MEMORY_PLUGIN_DATABASE_URL MEMORY_PLUGIN_LISTEN_ADDR + echo "memory-plugin: starting sidecar on $MEMORY_PLUGIN_LISTEN_ADDR" >&2 + # Drop privs to the platform user — the plugin doesn't need root and + # runs unprivileged elsewhere (tenant image already starts as canvas). + su-exec platform /memory-plugin & + MEMORY_PLUGIN_PID=$! + # Wait up to 30s for the plugin's /v1/health to return 200. Boot + # failure here is fatal — better to crash-loop than to silently + # serve cutover traffic against a dead plugin. + health_port=${MEMORY_PLUGIN_LISTEN_ADDR#:} + ready=0 + for _ in $(seq 1 30); do + if wget -qO- --timeout=2 "http://localhost:${health_port}/v1/health" >/dev/null 2>&1; then + ready=1 + break + fi + sleep 1 + done + if [ "$ready" != "1" ]; then + echo "memory-plugin: ❌ /v1/health never returned 200 after 30s — aborting boot. Check that DATABASE_URL is reachable, has the pgvector extension, and the plugin's migrations applied." >&2 + kill "$MEMORY_PLUGIN_PID" 2>/dev/null || true + exit 1 + fi + echo "memory-plugin: ✅ sidecar healthy on :$health_port" >&2 +fi + exec su-exec platform /platform "$@" ENTRY RUN chmod +x /entrypoint.sh && apk add --no-cache su-exec diff --git a/workspace-server/Dockerfile.tenant b/workspace-server/Dockerfile.tenant index 23140a67..6ccc737e 100644 --- a/workspace-server/Dockerfile.tenant +++ b/workspace-server/Dockerfile.tenant @@ -34,6 +34,13 @@ ARG GIT_SHA=dev RUN CGO_ENABLED=0 GOOS=linux go build \ -ldflags "-X github.com/Molecule-AI/molecule-monorepo/platform/internal/buildinfo.GitSHA=${GIT_SHA}" \ -o /platform ./cmd/server +# Memory v2 sidecar binary (Memory v2 #2728). Bundled so an operator +# can activate cutover by flipping MEMORY_V2_CUTOVER=true without +# provisioning a separate service. See entrypoint-tenant.sh for the +# launch logic. +RUN CGO_ENABLED=0 GOOS=linux go build \ + -ldflags "-X github.com/Molecule-AI/molecule-monorepo/platform/internal/buildinfo.GitSHA=${GIT_SHA}" \ + -o /memory-plugin ./cmd/memory-plugin-postgres # ── Stage 2: Canvas Next.js standalone ──────────────────────────────── FROM node:20-alpine AS canvas-builder @@ -74,8 +81,9 @@ RUN deluser --remove-home node 2>/dev/null || true; \ delgroup node 2>/dev/null || true; \ addgroup -g 1000 canvas && adduser -u 1000 -G canvas -s /bin/sh -D canvas -# Go platform binary +# Go platform binary + Memory v2 sidecar COPY --from=go-builder /platform /platform +COPY --from=go-builder /memory-plugin /memory-plugin COPY workspace-server/migrations /migrations # Templates + plugins (cloned from GitHub in stage 3) @@ -91,7 +99,7 @@ COPY --from=canvas-builder /canvas/public ./public COPY workspace-server/entrypoint-tenant.sh /entrypoint.sh RUN chmod +x /entrypoint.sh && \ - chown -R canvas:canvas /canvas /platform /migrations + chown -R canvas:canvas /canvas /platform /memory-plugin /migrations EXPOSE 8080 # entrypoint.sh starts as root to fix volume perms, then drops to diff --git a/workspace-server/cmd/memory-plugin-postgres/config_test.go b/workspace-server/cmd/memory-plugin-postgres/config_test.go new file mode 100644 index 00000000..252f0d1b --- /dev/null +++ b/workspace-server/cmd/memory-plugin-postgres/config_test.go @@ -0,0 +1,50 @@ +package main + +import ( + "strings" + "testing" +) + +// TestLoadConfig_DefaultListenAddrIsLoopback pins the default-bind contract. +// +// Why this matters: with the prior `:9100` default, the plugin listened on +// every interface. Inside the container it didn't matter (no host port +// mapping today), but a future change that publishes 9100 OR a cross-host +// sidecar deploy would have exposed an unauth'd memory store. Loopback by +// default is the least-privilege baseline; operators with a multi-host +// topology override via MEMORY_PLUGIN_LISTEN_ADDR. +func TestLoadConfig_DefaultListenAddrIsLoopback(t *testing.T) { + t.Setenv("MEMORY_PLUGIN_DATABASE_URL", "postgres://stub") + t.Setenv("MEMORY_PLUGIN_LISTEN_ADDR", "") + + cfg, err := loadConfig() + if err != nil { + t.Fatalf("loadConfig: %v", err) + } + if !strings.HasPrefix(cfg.ListenAddr, "127.0.0.1:") { + t.Errorf("default ListenAddr must bind loopback-only, got %q "+ + "(security regression — would expose plugin on every interface)", + cfg.ListenAddr) + } +} + +func TestLoadConfig_ListenAddrEnvOverride(t *testing.T) { + t.Setenv("MEMORY_PLUGIN_DATABASE_URL", "postgres://stub") + t.Setenv("MEMORY_PLUGIN_LISTEN_ADDR", ":9100") + + cfg, err := loadConfig() + if err != nil { + t.Fatalf("loadConfig: %v", err) + } + if cfg.ListenAddr != ":9100" { + t.Errorf("env override ignored: want :9100, got %q", cfg.ListenAddr) + } +} + +func TestLoadConfig_MissingDatabaseURL(t *testing.T) { + t.Setenv("MEMORY_PLUGIN_DATABASE_URL", "") + + if _, err := loadConfig(); err == nil { + t.Fatal("loadConfig must error when MEMORY_PLUGIN_DATABASE_URL is empty") + } +} diff --git a/workspace-server/cmd/memory-plugin-postgres/main.go b/workspace-server/cmd/memory-plugin-postgres/main.go index 84e01351..148c1dd4 100644 --- a/workspace-server/cmd/memory-plugin-postgres/main.go +++ b/workspace-server/cmd/memory-plugin-postgres/main.go @@ -31,7 +31,13 @@ const ( envListenAddr = "MEMORY_PLUGIN_LISTEN_ADDR" envSkipMigrate = "MEMORY_PLUGIN_SKIP_MIGRATE" - defaultListenAddr = ":9100" + // Loopback-only by default (defense in depth). The platform talks to + // the plugin over `http://localhost:9100` from the same container, so + // binding to all interfaces would only widen the reachable surface + // without enabling any in-design caller. Operators running the plugin + // on a separate host override via MEMORY_PLUGIN_LISTEN_ADDR=:9100 (or + // some other interface). + defaultListenAddr = "127.0.0.1:9100" ) func main() { diff --git a/workspace-server/entrypoint-tenant.sh b/workspace-server/entrypoint-tenant.sh index 9cfc1437..8059cc1c 100644 --- a/workspace-server/entrypoint-tenant.sh +++ b/workspace-server/entrypoint-tenant.sh @@ -20,6 +20,42 @@ cd /canvas PORT=3000 HOSTNAME=0.0.0.0 node server.js & CANVAS_PID=$! +# Memory v2 sidecar (built-in postgres plugin). See Dockerfile entrypoint +# comment for rationale. Stays inert at the protocol layer until the +# operator sets MEMORY_V2_CUTOVER=true; running it is cheap. +# +# Defaults the plugin's DATABASE_URL to the tenant's DATABASE_URL so +# operators don't need to configure two of them. Plugin tables coexist +# with the platform schema. +MEMORY_PLUGIN_PID="" +if [ -z "$MEMORY_PLUGIN_DISABLE" ] && [ -n "$DATABASE_URL" ]; then + : "${MEMORY_PLUGIN_DATABASE_URL:=$DATABASE_URL}" + : "${MEMORY_PLUGIN_LISTEN_ADDR:=:9100}" + export MEMORY_PLUGIN_DATABASE_URL MEMORY_PLUGIN_LISTEN_ADDR + echo "memory-plugin: starting sidecar on $MEMORY_PLUGIN_LISTEN_ADDR" >&2 + /memory-plugin & + MEMORY_PLUGIN_PID=$! + # Wait up to 30s for /v1/health. Boot failure is fatal so a misconfigured + # tenant crash-loops instead of silently serving cutover traffic against + # a dead plugin. + health_port=${MEMORY_PLUGIN_LISTEN_ADDR#:} + ready=0 + for _ in $(seq 1 30); do + if wget -qO- --timeout=2 "http://localhost:${health_port}/v1/health" >/dev/null 2>&1; then + ready=1 + break + fi + sleep 1 + done + if [ "$ready" != "1" ]; then + echo "memory-plugin: ❌ /v1/health never returned 200 after 30s — aborting boot. Check DATABASE_URL reachability + pgvector extension + migrations." >&2 + kill "$MEMORY_PLUGIN_PID" 2>/dev/null || true + kill "$CANVAS_PID" 2>/dev/null || true + exit 1 + fi + echo "memory-plugin: ✅ sidecar healthy on :$health_port" >&2 +fi + # Start Go platform in foreground-ish (we trap signals) # CANVAS_PROXY_URL tells the platform to proxy unmatched routes to Canvas. # CONTAINER_BACKEND: empty = Docker (default for self-hosted/local). @@ -29,15 +65,20 @@ cd / /platform & PLATFORM_PID=$! -# If either process exits, kill the other +# If any process exits, kill the others cleanup() { kill $CANVAS_PID 2>/dev/null || true kill $PLATFORM_PID 2>/dev/null || true + [ -n "$MEMORY_PLUGIN_PID" ] && kill $MEMORY_PLUGIN_PID 2>/dev/null || true } trap cleanup EXIT SIGTERM SIGINT -# Wait for either to exit — whichever exits first triggers cleanup -wait -n $CANVAS_PID $PLATFORM_PID +# Wait for any to exit — whichever exits first triggers cleanup +if [ -n "$MEMORY_PLUGIN_PID" ]; then + wait -n $CANVAS_PID $PLATFORM_PID $MEMORY_PLUGIN_PID +else + wait -n $CANVAS_PID $PLATFORM_PID +fi EXIT_CODE=$? cleanup exit $EXIT_CODE diff --git a/workspace-server/internal/handlers/chat_files_poll_test.go b/workspace-server/internal/handlers/chat_files_poll_test.go index aa5bab34..eb23acf1 100644 --- a/workspace-server/internal/handlers/chat_files_poll_test.go +++ b/workspace-server/internal/handlers/chat_files_poll_test.go @@ -201,7 +201,7 @@ func TestPollUpload_HappyPath_OneFile_StagesAndLogs(t *testing.T) { expectActivityInsert(mock) store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) body, ct := pollUploadFixture(t, map[string][]byte{"report.pdf": []byte("PDF-bytes")}) @@ -259,7 +259,7 @@ func TestPollUpload_MultipleFiles_AllStagedAndLogged(t *testing.T) { expectActivityInsert(mock) store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) body, ct := pollUploadFixture(t, map[string][]byte{ @@ -297,7 +297,7 @@ func TestPollUpload_PushModeFallsThroughToForward(t *testing.T) { // URL empty + mode=push → 503 (no inbound secret check needed). store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) body, ct := pollUploadFixture(t, map[string][]byte{"x": []byte("data")}) @@ -321,7 +321,7 @@ func TestPollUpload_NotConfigured_FallsThrough(t *testing.T) { wsID := "33333333-2222-3333-4444-555555555555" expectURLAndMode(mock, wsID, "", "poll") // resolveWorkspaceForwardCreds emits 422 - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) // No WithPendingUploads — pendingUploads is nil. body, ct := pollUploadFixture(t, map[string][]byte{"x": []byte("data")}) @@ -342,7 +342,7 @@ func TestPollUpload_WorkspaceMissing_404(t *testing.T) { wsID := "44444444-2222-3333-4444-555555555555" expectPollDeliveryModeMissing(mock, wsID) - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(newInMemStorage(), nil) body, ct := pollUploadFixture(t, map[string][]byte{"x": []byte("d")}) @@ -362,7 +362,7 @@ func TestPollUpload_DeliveryModeLookupDBError_500(t *testing.T) { mock.ExpectQuery(`SELECT delivery_mode FROM workspaces WHERE id = \$1`). WithArgs(wsID).WillReturnError(errors.New("connection lost")) - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(newInMemStorage(), nil) body, ct := pollUploadFixture(t, map[string][]byte{"x": []byte("d")}) @@ -382,7 +382,7 @@ func TestPollUpload_NoFilesField_400(t *testing.T) { expectPollDeliveryMode(mock, wsID, "poll") store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) // Multipart with a non-files field — no actual files. @@ -407,7 +407,7 @@ func TestPollUpload_MalformedMultipart_400(t *testing.T) { expectPollDeliveryMode(mock, wsID, "poll") store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) // Body that doesn't match the boundary in Content-Type. @@ -428,7 +428,7 @@ func TestPollUpload_StorageError_500(t *testing.T) { store := newInMemStorage() store.putErr = errors.New("disk full") - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) body, ct := pollUploadFixture(t, map[string][]byte{"x.bin": []byte("data")}) @@ -449,7 +449,7 @@ func TestPollUpload_StorageTooLarge_413(t *testing.T) { store := newInMemStorage() store.putErr = pendinguploads.ErrTooLarge - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) body, ct := pollUploadFixture(t, map[string][]byte{"x.bin": []byte("data")}) @@ -469,7 +469,7 @@ func TestPollUpload_TooManyFiles_400(t *testing.T) { expectPollDeliveryMode(mock, wsID, "poll") store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) // 65 files — over the per-batch cap. @@ -504,7 +504,7 @@ func TestPollUpload_NullDeliveryMode_TreatedAsPush(t *testing.T) { expectURLAndMode(mock, wsID, "", "") store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) body, ct := pollUploadFixture(t, map[string][]byte{"x.bin": []byte("data")}) @@ -537,7 +537,7 @@ func TestPollUpload_PerFileCapPreStorage_413(t *testing.T) { expectPollDeliveryMode(mock, wsID, "poll") store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) // 25 MB + 1 byte. Single file, large enough to trip the early @@ -572,7 +572,7 @@ func TestPollUpload_SanitizesFilenameInResponse(t *testing.T) { expectActivityInsert(mock) store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) body, ct := pollUploadFixture(t, map[string][]byte{"hello world!.pdf": []byte("data")}) @@ -616,7 +616,7 @@ func TestPollUpload_AtomicRollbackOnSecondFileTooLarge(t *testing.T) { expectPollDeliveryMode(mock, wsID, "poll") store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) // Two files: first OK, second over the per-file cap. Pre-validation @@ -653,7 +653,7 @@ func TestPollUpload_AtomicRollbackOnPutBatchError(t *testing.T) { store := newInMemStorage() store.putErr = errors.New("db down mid-batch") - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) body, ct := pollUploadFixture(t, map[string][]byte{ @@ -734,7 +734,7 @@ func TestPollUpload_ActivityRowDiscriminator(t *testing.T) { expectActivityInsertWithTypeAndMethod(mock, wsID, "a2a_receive", "chat_upload_receive") store := newInMemStorage() - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)). + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)). WithPendingUploads(store, nil) body, ct := pollUploadFixture(t, map[string][]byte{"x.pdf": []byte("xx")}) diff --git a/workspace-server/internal/handlers/chat_files_test.go b/workspace-server/internal/handlers/chat_files_test.go index e7829f45..6012d3a7 100644 --- a/workspace-server/internal/handlers/chat_files_test.go +++ b/workspace-server/internal/handlers/chat_files_test.go @@ -105,7 +105,7 @@ func TestChatUpload_InvalidWorkspaceID(t *testing.T) { setupTestDB(t) setupTestRedis(t) - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) c, w := makeUploadRequest(t, "not-a-uuid", &bytes.Buffer{}, "") h.Upload(c) @@ -122,7 +122,7 @@ func TestChatUpload_WorkspaceNotInDB(t *testing.T) { wsID := "00000000-0000-0000-0000-000000000099" expectURLMissing(mock, wsID) - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) body, ct := uploadFixture(t) c, w := makeUploadRequest(t, wsID, body, ct) h.Upload(c) @@ -166,7 +166,7 @@ func TestChatUpload_NoInboundSecret_LazyHeal(t *testing.T) { WithArgs(sqlmock.AnyArg(), wsID). WillReturnResult(sqlmock.NewResult(0, 1)) - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) body, ct := uploadFixture(t) c, w := makeUploadRequest(t, wsID, body, ct) h.Upload(c) @@ -203,7 +203,7 @@ func TestChatUpload_NoInboundSecret_LazyHealFailure(t *testing.T) { WithArgs(sqlmock.AnyArg(), wsID). WillReturnError(sql.ErrConnDone) // mint fails - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) body, ct := uploadFixture(t) c, w := makeUploadRequest(t, wsID, body, ct) h.Upload(c) @@ -231,7 +231,7 @@ func TestChatUpload_NoURL(t *testing.T) { wsID := "00000000-0000-0000-0000-000000000042" expectURLAndMode(mock, wsID, "", "push") - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) body, ct := uploadFixture(t) c, w := makeUploadRequest(t, wsID, body, ct) h.Upload(c) @@ -256,7 +256,7 @@ func TestChatUpload_PollModeEmptyURL(t *testing.T) { wsID := "00000000-0000-0000-0000-000000000099" expectURLAndMode(mock, wsID, "", "poll") - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) body, ct := uploadFixture(t) c, w := makeUploadRequest(t, wsID, body, ct) h.Upload(c) @@ -286,7 +286,7 @@ func TestChatUpload_NullModeEmptyURL(t *testing.T) { wsID := "30ba7f0b-b303-4a20-aefe-3a4a675b8aa4" // user's "mac laptop" expectURLNullMode(mock, wsID, "") - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) body, ct := uploadFixture(t) c, w := makeUploadRequest(t, wsID, body, ct) h.Upload(c) @@ -338,7 +338,7 @@ func TestChatUpload_ForwardsToWorkspace_HappyPath(t *testing.T) { expectURL(mock, wsID, srv.URL) expectInboundSecret(mock, wsID, "super-secret-123") - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) body, ct := uploadFixture(t) c, w := makeUploadRequest(t, wsID, body, ct) h.Upload(c) @@ -380,7 +380,7 @@ func TestChatUpload_ForwardsErrorStatusUnchanged(t *testing.T) { expectURL(mock, wsID, srv.URL) expectInboundSecret(mock, wsID, "tok") - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) body, ct := uploadFixture(t) c, w := makeUploadRequest(t, wsID, body, ct) h.Upload(c) @@ -402,7 +402,7 @@ func TestChatUpload_WorkspaceUnreachable(t *testing.T) { expectURL(mock, wsID, "http://127.0.0.1:1") expectInboundSecret(mock, wsID, "tok") - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) body, ct := uploadFixture(t) c, w := makeUploadRequest(t, wsID, body, ct) h.Upload(c) @@ -418,7 +418,7 @@ func TestChatDownload_InvalidPath(t *testing.T) { setupTestDB(t) setupTestRedis(t) - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) cases := []struct { name, path, wantSubstr string @@ -507,7 +507,7 @@ func TestChatDownload_WorkspaceNotInDB(t *testing.T) { WithArgs(wsID). WillReturnError(sql.ErrNoRows) - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) c, w := makeDownloadRequest(t, wsID, "/workspace/foo.txt") h.Download(c) @@ -533,7 +533,7 @@ func TestChatDownload_NoInboundSecret_LazyHeal(t *testing.T) { WithArgs(sqlmock.AnyArg(), wsID). WillReturnResult(sqlmock.NewResult(0, 1)) - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) c, w := makeDownloadRequest(t, wsID, "/workspace/foo.txt") h.Download(c) @@ -559,7 +559,7 @@ func TestChatDownload_NoInboundSecret_LazyHealFailure(t *testing.T) { WithArgs(sqlmock.AnyArg(), wsID). WillReturnError(sql.ErrConnDone) - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) c, w := makeDownloadRequest(t, wsID, "/workspace/foo.txt") h.Download(c) @@ -592,7 +592,7 @@ func TestChatDownload_ForwardsToWorkspace_HappyPath(t *testing.T) { expectURL(mock, wsID, srv.URL) expectInboundSecret(mock, wsID, "the-secret") - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) c, w := makeDownloadRequest(t, wsID, "/workspace/report.txt") h.Download(c) @@ -634,7 +634,7 @@ func TestChatDownload_404FromWorkspacePropagated(t *testing.T) { expectURL(mock, wsID, srv.URL) expectInboundSecret(mock, wsID, "tok") - h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil)) + h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil)) c, w := makeDownloadRequest(t, wsID, "/workspace/missing.txt") h.Download(c) diff --git a/workspace-server/internal/handlers/org_import.go b/workspace-server/internal/handlers/org_import.go index 94ca0b34..8f4d9a07 100644 --- a/workspace-server/internal/handlers/org_import.go +++ b/workspace-server/internal/handlers/org_import.go @@ -61,16 +61,20 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, absX tier = defaults.Tier } if tier == 0 { - // SaaS-aware fallback. SaaS → T4 (one container per sibling - // EC2, no neighbour to protect from). Self-hosted → T2 - // (safe shared-Docker-daemon default — many workspaces in - // one kernel). Templates that want a different floor - // declare `tier:` in their config.yaml or the org-template's - // `defaults.tier`. - if h.workspace != nil && h.workspace.IsSaaS() { - tier = 4 + // Resolved via the same DefaultTier helper Create + Templates + // use (#2910 PR-E). SaaS → T4 (one container per sibling EC2, + // no neighbour to protect from), self-hosted → T3. Pre-#2910 + // this path returned T2 on self-hosted, asymmetric with + // workspace.go's T3 — undocumented drift. Lifting to + // DefaultTier collapses both call sites onto one source of + // truth so a future tier-default change sweeps every entry + // point at once. Templates that want a different floor still + // declare `tier:` in config.yaml or `defaults.tier` in + // org.yaml. + if h.workspace != nil { + tier = h.workspace.DefaultTier() } else { - tier = 2 + tier = 3 } } diff --git a/workspace-server/internal/handlers/saas_default_tier_test.go b/workspace-server/internal/handlers/saas_default_tier_test.go new file mode 100644 index 00000000..c4d32a94 --- /dev/null +++ b/workspace-server/internal/handlers/saas_default_tier_test.go @@ -0,0 +1,99 @@ +package handlers + +import ( + "strings" + "testing" + + "github.com/Molecule-AI/molecule-monorepo/platform/internal/provisioner" +) + +// Tests for the SaaS-aware default-tier resolution introduced in #2901 +// and hardened in #2910 (multi-model review of #2901 found the original +// claim of "all green" was passing because no SaaS-mode test existed). +// +// These tests pin three invariants: +// +// 1. WorkspaceHandler.IsSaaS() returns true when cpProv is wired, +// false otherwise. +// 2. WorkspaceHandler.DefaultTier() returns 4 on SaaS, 3 self-hosted. +// 3. generateDefaultConfig (TemplatesHandler.Import path) writes the +// passed-in tier into the generated config.yaml — pre-#2910 it +// was hardcoded to 3 and silently disagreed with the create- +// handler default on SaaS. + +// stubCPProv is a minimal stand-in for the CP provisioner — only +// exercises the IsSaaS / HasProvisioner contract, never invoked in +// these tests. +type stubCPProv struct{} + +func (stubCPProv) Start(_ interface{}, _ provisioner.WorkspaceConfig) (string, error) { + return "", nil +} +func (stubCPProv) Stop(_ interface{}, _ string) error { return nil } +func (stubCPProv) Restart(_ interface{}, _ provisioner.WorkspaceConfig) (string, error) { + return "", nil +} + +func TestIsSaaS_TrueWhenCPProvWired(t *testing.T) { + h := &WorkspaceHandler{cpProv: &trackingCPProv{}} + if !h.IsSaaS() { + t.Errorf("IsSaaS()=false with cpProv wired; expected true") + } +} + +func TestIsSaaS_FalseWhenOnlyDocker(t *testing.T) { + // provisioner field set, cpProv nil — the self-hosted path. + // Use a non-nil sentinel so the check actually has something to + // disagree with. trackingCPProv lives in workspace_provision_auto_test.go + // and is the established stub for these handler-level tests. + h := &WorkspaceHandler{provisioner: nil, cpProv: nil} + if h.IsSaaS() { + t.Errorf("IsSaaS()=true with both backends nil; expected false") + } +} + +func TestDefaultTier_SaaS_IsT4(t *testing.T) { + h := &WorkspaceHandler{cpProv: &trackingCPProv{}} + if got := h.DefaultTier(); got != 4 { + t.Errorf("SaaS DefaultTier()=%d; expected 4", got) + } +} + +func TestDefaultTier_SelfHosted_IsT3(t *testing.T) { + h := &WorkspaceHandler{} + if got := h.DefaultTier(); got != 3 { + t.Errorf("self-hosted DefaultTier()=%d; expected 3", got) + } +} + +// generateDefaultConfig — pin that the tier param flows into the +// emitted config.yaml verbatim. Pre-#2910 this was hardcoded "tier: 3" +// regardless of caller intent. +func TestGenerateDefaultConfig_RespectsTierParam(t *testing.T) { + cfg := generateDefaultConfig("Test Agent", map[string]string{"system-prompt.md": ""}, 4) + if !strings.Contains(cfg, "tier: 4\n") { + t.Errorf("expected `tier: 4` in generated config, got:\n%s", cfg) + } + // The pre-#2910 hardcoded `tier: 3` line must NOT appear. + if strings.Contains(cfg, "tier: 3\n") { + t.Errorf("config should not contain `tier: 3` when caller passed 4, got:\n%s", cfg) + } +} + +func TestGenerateDefaultConfig_SelfHostedTierT3(t *testing.T) { + cfg := generateDefaultConfig("Test Agent", map[string]string{"system-prompt.md": ""}, 3) + if !strings.Contains(cfg, "tier: 3\n") { + t.Errorf("expected `tier: 3` in generated config, got:\n%s", cfg) + } +} + +// Bounds check — caller passes 0 or out-of-range, helper falls back +// to T3 (the safer-of-the-two when deployment mode can't be resolved). +func TestGenerateDefaultConfig_OutOfRangeFallsBackToT3(t *testing.T) { + for _, tier := range []int{0, -1, 99} { + cfg := generateDefaultConfig("X", map[string]string{}, tier) + if !strings.Contains(cfg, "tier: 3\n") { + t.Errorf("invalid tier %d should fall back to T3, got:\n%s", tier, cfg) + } + } +} diff --git a/workspace-server/internal/handlers/security_regression_685_686_687_688_test.go b/workspace-server/internal/handlers/security_regression_685_686_687_688_test.go index f8d4fcb9..aa35a517 100644 --- a/workspace-server/internal/handlers/security_regression_685_686_687_688_test.go +++ b/workspace-server/internal/handlers/security_regression_685_686_687_688_test.go @@ -71,7 +71,7 @@ func TestSecurity_GetTemplates_NoAuth_Returns401(t *testing.T) { authDB, authMock := newEnrolledAuthDB(t) tmpDir := t.TempDir() - tmplh := NewTemplatesHandler(tmpDir, nil) + tmplh := NewTemplatesHandler(tmpDir, nil, nil) r := gin.New() r.GET("/templates", middleware.AdminAuth(authDB), tmplh.List) @@ -98,7 +98,7 @@ func TestSecurity_GetTemplates_FreshInstall_FailsOpen(t *testing.T) { authDB, authMock := newFreshInstallAuthDB(t) tmpDir := t.TempDir() - tmplh := NewTemplatesHandler(tmpDir, nil) + tmplh := NewTemplatesHandler(tmpDir, nil, nil) r := gin.New() r.GET("/templates", middleware.AdminAuth(authDB), tmplh.List) diff --git a/workspace-server/internal/handlers/template_import.go b/workspace-server/internal/handlers/template_import.go index 7d4ab4d1..95b5854f 100644 --- a/workspace-server/internal/handlers/template_import.go +++ b/workspace-server/internal/handlers/template_import.go @@ -36,8 +36,14 @@ func normalizeName(name string) string { return result } -// generateDefaultConfig creates a config.yaml from detected prompt files and skills. -func generateDefaultConfig(name string, files map[string]string) string { +// generateDefaultConfig creates a config.yaml from detected prompt files +// and skills. tier is the deployment-aware default (caller passes +// h.wh.DefaultTier() — T4 on SaaS, T3 on self-hosted) so the generated +// file matches what POST /workspaces would default to. Pre-#2910 this +// was hardcoded to 3, which split-brained with the create-handler +// default on SaaS (T4) and pinned newly-imported templates at T3 even +// when downstream Create paths picked T4. +func generateDefaultConfig(name string, files map[string]string, tier int) string { promptFiles := []string{} skillSet := map[string]bool{} @@ -74,9 +80,15 @@ func generateDefaultConfig(name string, files map[string]string) string { var cfg strings.Builder cfg.WriteString(`name: "` + escaped + `"` + "\n") cfg.WriteString("description: Imported agent\n") - // Default to tier 3 ("Privileged") — matches the workspace.go - // create handler default. See its comment for rationale. - cfg.WriteString("version: 1.0.0\ntier: 3\n") + // Tier is SaaS-aware via the caller's DefaultTier (#2910 PR-B). + // Bounds-checked: invalid input falls back to T3 (the historical + // default + the safer-of-the-two when the deployment mode can't + // be resolved). + if tier < 1 || tier > 4 { + tier = 3 + } + cfg.WriteString("version: 1.0.0\n") + cfg.WriteString(fmt.Sprintf("tier: %d\n", tier)) cfg.WriteString("model: anthropic:claude-haiku-4-5-20251001\n") cfg.WriteString("\nprompt_files:\n") if len(promptFiles) > 0 { @@ -148,7 +160,11 @@ func (h *TemplatesHandler) Import(c *gin.Context) { // Auto-generate config.yaml if not provided if _, exists := body.Files["config.yaml"]; !exists { - cfg := generateDefaultConfig(body.Name, body.Files) + tier := 3 + if h.wh != nil { + tier = h.wh.DefaultTier() + } + cfg := generateDefaultConfig(body.Name, body.Files, tier) if err := os.WriteFile(filepath.Join(destDir, "config.yaml"), []byte(cfg), 0600); err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to write config.yaml"}) return @@ -227,7 +243,11 @@ func (h *TemplatesHandler) ReplaceFiles(c *gin.Context) { if _, exists := body.Files["config.yaml"]; !exists { // Check if config.yaml exists in container if _, err := h.execInContainer(ctx, containerName, []string{"test", "-f", "/configs/config.yaml"}); err != nil { - cfg := generateDefaultConfig(wsName, body.Files) + tier := 3 + if h.wh != nil { + tier = h.wh.DefaultTier() + } + cfg := generateDefaultConfig(wsName, body.Files, tier) singleFile := map[string]string{"config.yaml": cfg} h.copyFilesToContainer(ctx, containerName, "/configs", singleFile) } diff --git a/workspace-server/internal/handlers/template_import_test.go b/workspace-server/internal/handlers/template_import_test.go index 42336844..c496f9c5 100644 --- a/workspace-server/internal/handlers/template_import_test.go +++ b/workspace-server/internal/handlers/template_import_test.go @@ -55,7 +55,7 @@ func TestGenerateDefaultConfig_WithFiles(t *testing.T) { "skills/review/templates.md": "Templates", } - cfg := generateDefaultConfig("Test Agent", files) + cfg := generateDefaultConfig("Test Agent", files, 3) // Name is emitted as a double-quoted scalar (#221 sanitizer). if !strings.Contains(cfg, `name: "Test Agent"`) { @@ -85,7 +85,7 @@ func TestGenerateDefaultConfig_Empty(t *testing.T) { "data/something.json": `{"key": "value"}`, } - cfg := generateDefaultConfig("Empty Agent", files) + cfg := generateDefaultConfig("Empty Agent", files, 3) if !strings.Contains(cfg, `name: "Empty Agent"`) { t.Errorf("config should contain quoted agent name, got:\n%s", cfg) @@ -134,7 +134,7 @@ func TestGenerateDefaultConfig_YAMLInjection(t *testing.T) { for _, tc := range adversarialCases { t.Run(tc.desc, func(t *testing.T) { - cfg := generateDefaultConfig(tc.name, map[string]string{}) + cfg := generateDefaultConfig(tc.name, map[string]string{}, 3) var parsed map[string]interface{} if err := yaml.Unmarshal([]byte(cfg), &parsed); err != nil { t.Fatalf("sanitized config does not parse as YAML: %v\n--- config ---\n%s", err, cfg) @@ -205,7 +205,7 @@ func TestImport_Success(t *testing.T) { setupTestRedis(t) tmpDir := t.TempDir() - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) body := `{ "name": "New Agent", @@ -245,7 +245,7 @@ func TestImport_MissingName(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) body := `{"files": {"test.md": "content"}}` @@ -265,7 +265,7 @@ func TestImport_TooManyFiles(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) files := make(map[string]string) for i := 0; i <= maxUploadFiles; i++ { @@ -296,7 +296,7 @@ func TestImport_AlreadyExists(t *testing.T) { tmpDir := t.TempDir() os.MkdirAll(filepath.Join(tmpDir, "existing-agent"), 0755) - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) body := `{"name": "Existing Agent", "files": {"test.md": "content"}}` @@ -317,7 +317,7 @@ func TestImport_WithConfigYaml(t *testing.T) { setupTestRedis(t) tmpDir := t.TempDir() - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) body := `{ "name": "Custom Agent", @@ -354,7 +354,7 @@ func TestReplaceFiles_MissingBody(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -373,7 +373,7 @@ func TestReplaceFiles_TooManyFiles(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) files := make(map[string]string) for i := 0; i <= maxUploadFiles; i++ { @@ -398,7 +398,7 @@ func TestReplaceFiles_WorkspaceNotFound(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) // ReplaceFiles now selects (name, instance_id, runtime) for the // restart-cascade. Match the full column list rather than just the @@ -429,7 +429,7 @@ func TestReplaceFiles_PathTraversal(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) mock.ExpectQuery(`SELECT name, COALESCE\(instance_id, ''\), COALESCE\(runtime, ''\) FROM workspaces WHERE id =`). WithArgs("ws-rf-pt"). diff --git a/workspace-server/internal/handlers/templates.go b/workspace-server/internal/handlers/templates.go index d51dabcd..03776a5d 100644 --- a/workspace-server/internal/handlers/templates.go +++ b/workspace-server/internal/handlers/templates.go @@ -31,10 +31,20 @@ const maxUploadFiles = 200 type TemplatesHandler struct { configsDir string docker *client.Client + // wh is used by Import and ReplaceFiles to call DefaultTier() so a + // generated config.yaml's tier matches the SaaS-vs-self-hosted + // boundary (#2910 PR-B). nil-tolerant — the field is unused when + // the caller doesn't import templates that need a fresh config + // generated. + wh *WorkspaceHandler } -func NewTemplatesHandler(configsDir string, dockerCli *client.Client) *TemplatesHandler { - return &TemplatesHandler{configsDir: configsDir, docker: dockerCli} +// NewTemplatesHandler constructs a TemplatesHandler. wh may be nil for +// callers that only use the read-only template surfaces (List, +// ReadFile, ListFiles). Import + ReplaceFiles need wh non-nil so the +// generated config.yaml picks the SaaS-aware default tier. +func NewTemplatesHandler(configsDir string, dockerCli *client.Client, wh *WorkspaceHandler) *TemplatesHandler { + return &TemplatesHandler{configsDir: configsDir, docker: dockerCli, wh: wh} } // modelSpec describes a single supported model on a template: its id (sent diff --git a/workspace-server/internal/handlers/templates_test.go b/workspace-server/internal/handlers/templates_test.go index cbae8069..3d75bfd5 100644 --- a/workspace-server/internal/handlers/templates_test.go +++ b/workspace-server/internal/handlers/templates_test.go @@ -53,7 +53,7 @@ func TestTemplatesList_EmptyDir(t *testing.T) { setupTestRedis(t) tmpDir := t.TempDir() - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -99,7 +99,7 @@ skills: // Create a directory without config.yaml (should be skipped) os.MkdirAll(filepath.Join(tmpDir, "no-config"), 0755) - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -160,7 +160,7 @@ skills: [] t.Fatalf("write: %v", err) } - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("GET", "/templates", nil) @@ -237,7 +237,7 @@ skills: [] t.Fatalf("write: %v", err) } - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("GET", "/templates", nil) @@ -315,7 +315,7 @@ skills: [] t.Fatalf("write: %v", err) } - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("GET", "/templates", nil) @@ -434,7 +434,7 @@ skills: [] t.Fatalf("write: %v", err) } - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("GET", "/templates", nil) @@ -512,7 +512,7 @@ skills: [] t.Fatalf("write: %v", err) } - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("GET", "/templates", nil) @@ -555,7 +555,7 @@ skills: [] t.Fatalf("write: %v", err) } - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("GET", "/templates", nil) @@ -589,7 +589,7 @@ skills: [] t.Fatalf("write: %v", err) } - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("GET", "/templates", nil) @@ -661,7 +661,7 @@ skills: [] log.SetOutput(&logBuf) defer log.SetOutput(prevOutput) - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest("GET", "/templates", nil) @@ -698,7 +698,7 @@ func TestTemplatesList_NonexistentDir(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler("/nonexistent/path/to/templates", nil) + handler := NewTemplatesHandler("/nonexistent/path/to/templates", nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -723,7 +723,7 @@ func TestListFiles_InvalidRoot(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -748,7 +748,7 @@ func TestListFiles_WorkspaceNotFound(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) mock.ExpectQuery("SELECT name FROM workspaces WHERE id ="). WithArgs("ws-nonexist"). @@ -775,7 +775,7 @@ func TestListFiles_FallbackToHost_NoTemplate(t *testing.T) { setupTestRedis(t) tmpDir := t.TempDir() - handler := NewTemplatesHandler(tmpDir, nil) // nil docker = no container + handler := NewTemplatesHandler(tmpDir, nil, nil) // nil docker = no container mock.ExpectQuery("SELECT name FROM workspaces WHERE id ="). WithArgs("ws-fallback"). @@ -815,7 +815,7 @@ func TestListFiles_FallbackToHost_WithTemplate(t *testing.T) { os.WriteFile(filepath.Join(tmplDir, "config.yaml"), []byte("name: Test Agent\n"), 0644) os.WriteFile(filepath.Join(tmplDir, "system-prompt.md"), []byte("# prompt"), 0644) - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) mock.ExpectQuery("SELECT name FROM workspaces WHERE id ="). WithArgs("ws-tmpl"). @@ -849,7 +849,7 @@ func TestReadFile_PathTraversal(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -870,7 +870,7 @@ func TestReadFile_InvalidRoot(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -892,7 +892,7 @@ func TestReadFile_WorkspaceNotFound(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) mock.ExpectQuery(`SELECT name, COALESCE\(instance_id, ''\), COALESCE\(runtime, ''\) FROM workspaces WHERE id =`). WithArgs("ws-nf"). @@ -926,7 +926,7 @@ func TestReadFile_FallbackToHost_Success(t *testing.T) { os.MkdirAll(tmplDir, 0755) os.WriteFile(filepath.Join(tmplDir, "config.yaml"), []byte("name: Reader Agent\ntier: 1\n"), 0644) - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) // instance_id="" → SaaS branch skipped → falls through to local // Docker / template-dir host fallback (the only path the test @@ -967,7 +967,7 @@ func TestReadFile_FallbackToHost_NotFound(t *testing.T) { setupTestRedis(t) tmpDir := t.TempDir() - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) mock.ExpectQuery(`SELECT name, COALESCE\(instance_id, ''\), COALESCE\(runtime, ''\) FROM workspaces WHERE id =`). WithArgs("ws-nofile"). @@ -999,7 +999,7 @@ func TestWriteFile_PathTraversal(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -1023,7 +1023,7 @@ func TestWriteFile_InvalidBody(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -1046,7 +1046,7 @@ func TestWriteFile_WorkspaceNotFound(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) mock.ExpectQuery(`SELECT name, COALESCE\(instance_id, ''\), COALESCE\(runtime, ''\) FROM workspaces WHERE id =`). WithArgs("ws-wf-nf"). @@ -1080,7 +1080,7 @@ func TestDeleteFile_PathTraversal(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -1101,7 +1101,7 @@ func TestDeleteFile_WorkspaceNotFound(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) mock.ExpectQuery("SELECT name FROM workspaces WHERE id ="). WithArgs("ws-del-nf"). @@ -1133,7 +1133,7 @@ func TestResolveTemplateDir_ByNormalizedName(t *testing.T) { tmplDir := filepath.Join(tmpDir, "my-agent") os.MkdirAll(tmplDir, 0755) - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) result := handler.resolveTemplateDir("My Agent") if result != tmplDir { @@ -1143,7 +1143,7 @@ func TestResolveTemplateDir_ByNormalizedName(t *testing.T) { func TestResolveTemplateDir_NotFound(t *testing.T) { tmpDir := t.TempDir() - handler := NewTemplatesHandler(tmpDir, nil) + handler := NewTemplatesHandler(tmpDir, nil, nil) result := handler.resolveTemplateDir("Nonexistent Agent") if result != "" { @@ -1177,7 +1177,7 @@ func TestCWE78_DeleteFile_TraversalVariants(t *testing.T) { setupTestDB(t) setupTestRedis(t) - handler := NewTemplatesHandler(t.TempDir(), nil) + handler := NewTemplatesHandler(t.TempDir(), nil, nil) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) diff --git a/workspace-server/internal/handlers/workspaces_insert_allowlist_test.go b/workspace-server/internal/handlers/workspaces_insert_allowlist_test.go new file mode 100644 index 00000000..066c6576 --- /dev/null +++ b/workspace-server/internal/handlers/workspaces_insert_allowlist_test.go @@ -0,0 +1,159 @@ +package handlers + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + "testing" +) + +// TestINSERTworkspacesAllowlist enumerates every function in this +// package that emits an `INSERT INTO workspaces (` SQL literal, and +// pins the result against an explicit allowlist. New entries fail the +// build until a reviewer adds them — forcing the question "what +// makes this INSERT idempotent?" at PR-review time, not after the +// next bulk-create leak. +// +// Pairs with TestCreateWorkspaceTree_CallsLookupBeforeInsert (the +// behavior pin for the one bulk path). Together they close the +// regression class: this test catches "did a new function start +// inserting workspaces?", that test catches "did the existing bulk +// path drop its idempotency check?". Either fires immediately when +// drift happens. +// +// Why allowlist rather than pure behavior gate (per memory +// feedback_behavior_based_ast_gates.md): the bulk-create leak class +// is small + stable (1 path today), and a behavior gate would have +// to disambiguate "iterating a YAML array of workspaces" from the +// many other `for ... range` patterns in a Create handler (config +// lines, secrets map, channels). Type-info-aware AST analysis would +// catch the YAML-iteration shape but is heavy. Allowlisting is the +// minimum-viable pin: any PR that adds a new INSERT site is forced +// to pause, add an entry here, and document the safety mechanism in +// the comment alongside. +// +// RFC #2867 class 1. +func TestINSERTworkspacesAllowlist(t *testing.T) { + // expected[key] = safety mechanism. Keep the comment pinned to + // what makes that function safe — if the safety changes, the + // allowlist must be re-reviewed. + expected := map[string]string{ + // org_import.createWorkspaceTree: lookupExistingChild + // before INSERT (#2868 phase 3). Also pinned by + // TestCreateWorkspaceTree_CallsLookupBeforeInsert. + "org_import.go:createWorkspaceTree": "lookup-then-insert via lookupExistingChild", + // registry.Register: external workspace registers itself with + // its known UUID; INSERT is idempotent via ON CONFLICT (id) + // DO UPDATE — re-registration upserts, never duplicates. + "registry.go:Register": "ON CONFLICT (id) DO UPDATE", + // workspace.Create: single-workspace POST /workspaces from a + // human or automation. No iteration; payload describes one + // workspace; UUID is server-generated. Caller intent IS to + // create, so no idempotency check is needed. + "workspace.go:Create": "single-workspace POST, server-generated UUID", + } + + actual := map[string]string{} + + wd, err := os.Getwd() + if err != nil { + t.Fatalf("getwd: %v", err) + } + + entries, err := os.ReadDir(wd) + if err != nil { + t.Fatalf("readdir %s: %v", wd, err) + } + for _, ent := range entries { + name := ent.Name() + if ent.IsDir() { + continue + } + if !strings.HasSuffix(name, ".go") { + continue + } + if strings.HasSuffix(name, "_test.go") { + continue + } + path := filepath.Join(wd, name) + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, nil, parser.ParseComments) + if err != nil { + t.Fatalf("parse %s: %v", path, err) + } + // For each top-level FuncDecl, walk its body and check for an + // `INSERT INTO workspaces (` SQL literal in any CallExpr arg. + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + var foundInsert bool + ast.Inspect(fn.Body, func(n ast.Node) bool { + lit, ok := n.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return true + } + raw := lit.Value + if unq, err := strconv.Unquote(raw); err == nil { + raw = unq + } + if workspacesInsertRE.MatchString(raw) { + foundInsert = true + return false + } + return true + }) + if foundInsert { + key := name + ":" + fn.Name.Name + actual[key] = "(observed via AST walk)" + } + } + } + + // Compute set diffs so failures point at the specific drift. + missing := []string{} + unexpected := []string{} + for k := range expected { + if _, ok := actual[k]; !ok { + missing = append(missing, k) + } + } + for k := range actual { + if _, ok := expected[k]; !ok { + unexpected = append(unexpected, k) + } + } + sort.Strings(missing) + sort.Strings(unexpected) + + if len(unexpected) > 0 { + t.Errorf(`new function(s) emit `+"`INSERT INTO workspaces (`"+` and aren't in the allowlist: + %s + +If this is a legitimate addition, add an entry to expected[] in this test +with the safety mechanism pinned in the comment alongside (lookup-then- +insert / ON CONFLICT / single-workspace path / etc.). The bulk-create +regression class needs explicit per-handler review, not silent drift. + +Reference: RFC #2867 class 1, sibling test +TestCreateWorkspaceTree_CallsLookupBeforeInsert.`, + strings.Join(unexpected, "\n ")) + } + if len(missing) > 0 { + t.Errorf(`expected function(s) no longer emit `+"`INSERT INTO workspaces (`"+`: + %s + +Either the function was renamed/deleted (update the allowlist) or the +INSERT was moved out (verify the new home is also covered). Don't just +delete the entry — confirm the safety mechanism is still in place +elsewhere or that the workspace-create path was intentionally +restructured.`, + strings.Join(missing, "\n ")) + } +} diff --git a/workspace-server/internal/router/router.go b/workspace-server/internal/router/router.go index 86007d00..d6d7b2d7 100644 --- a/workspace-server/internal/router/router.go +++ b/workspace-server/internal/router/router.go @@ -519,8 +519,9 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi r.GET("/canvas/viewport", vh.Get) r.PUT("/canvas/viewport", middleware.CanvasOrBearer(db.DB), vh.Save) - // Templates - tmplh := handlers.NewTemplatesHandler(configsDir, dockerCli) + // Templates — wh threaded so generateDefaultConfig picks the + // SaaS-aware default tier in Import + ReplaceFiles (#2910 PR-B). + tmplh := handlers.NewTemplatesHandler(configsDir, dockerCli, wh) // #686: GET /templates lists all template names+metadata from configsDir. // Open access lets unauthenticated callers enumerate org configurations and // installed plugins. AdminAuth-gate it alongside POST /templates/import.