From 4a6095ee1a524f85f0d2153c061d043f0552357c Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 29 Apr 2026 12:37:49 -0700 Subject: [PATCH] fix(chat_files): return 422 with structured detail for external workspaces (closes #2308) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Symptom: pasting a screenshot into the canvas chat for a runtime="external" workspace returned `503 {"error":"workspace container not running"}` — accurate from the upload handler's POV (no container exists for external workspaces) but misleading because it implies the container has crashed. Fix: detect runtime="external" via DB lookup BEFORE the container-find step and return 422 with: - error: "file upload not supported for external workspaces" - detail: explains why + points at admin/secrets workaround + references issue #2308 for the v0.2 native-support roadmap - runtime: "external" (machine-readable for clients) Why 422 not 200/501: - 422 = Unprocessable Entity — the request is well-formed but the workspace's runtime can't accept it. Standard REST semantics. - 200 with empty result would lie; 501 implies the API itself is unimplemented (it's not — works for non-external workspaces); 503 was the misleading status this PR fixes. Verified via live E2E against localhost: - Created `runtime=external,external=true` workspace - Posted multipart to /workspaces/:id/chat/uploads - Got 422 with the expected structured body Unit test (`chat_files_external_test.go`) pins the contract via sqlmock + httptest. Notable: the handler is constructed with `templates: nil` to prove the runtime check happens BEFORE any docker plumbing — if a future change moves the check below findContainer, the test crashes on nil-deref instead of silently regressing. Out of scope (for v0.2 follow-up): - Native external-workspace file ingest via artifacts table or the channel-plugin's inbox/ pattern. Requires separate design pass. Closes #2308 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/chat_files.go | 25 +++++ .../handlers/chat_files_external_test.go | 99 +++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 workspace-server/internal/handlers/chat_files_external_test.go diff --git a/workspace-server/internal/handlers/chat_files.go b/workspace-server/internal/handlers/chat_files.go index f8533e28..96709848 100644 --- a/workspace-server/internal/handlers/chat_files.go +++ b/workspace-server/internal/handlers/chat_files.go @@ -13,6 +13,7 @@ import ( "bytes" "context" "crypto/rand" + "database/sql" "encoding/hex" "fmt" "io" @@ -24,6 +25,7 @@ import ( "regexp" "strings" + "github.com/Molecule-AI/molecule-monorepo/platform/internal/db" "github.com/docker/docker/api/types/container" "github.com/gin-gonic/gin" ) @@ -177,6 +179,29 @@ func (h *ChatFilesHandler) Upload(c *gin.Context) { } ctx := c.Request.Context() + + // External workspaces have no container — the container-find-then-tar-exec + // flow below doesn't apply. Surface a clear 4xx instead of the misleading + // "container not running" 503 (#2308). File ingest for external workspaces + // is on the v0.2 roadmap (likely via the artifacts table or the channel- + // plugin's inbox/ pattern); flagging unsupported is the v0.1 contract. + var wsRuntime string + if err := db.DB.QueryRowContext(ctx, + `SELECT COALESCE(runtime, '') FROM workspaces WHERE id = $1`, workspaceID, + ).Scan(&wsRuntime); err != nil && err != sql.ErrNoRows { + log.Printf("chat_files Upload: runtime lookup failed for %s: %v", workspaceID, err) + } + if wsRuntime == "external" { + c.JSON(http.StatusUnprocessableEntity, gin.H{ + "error": "file upload not supported for external workspaces", + "detail": "External workspaces have no container to receive uploads. " + + "Use POST /admin/secrets to share data, or attach via your external agent's own input channel. " + + "Native external-workspace upload tracked at https://github.com/Molecule-AI/molecule-core/issues/2308.", + "runtime": "external", + }) + return + } + containerName := h.templates.findContainer(ctx, workspaceID) if containerName == "" { c.JSON(http.StatusServiceUnavailable, gin.H{"error": "workspace container not running"}) diff --git a/workspace-server/internal/handlers/chat_files_external_test.go b/workspace-server/internal/handlers/chat_files_external_test.go new file mode 100644 index 00000000..ce37b940 --- /dev/null +++ b/workspace-server/internal/handlers/chat_files_external_test.go @@ -0,0 +1,99 @@ +package handlers + +import ( + "bytes" + "encoding/json" + "mime/multipart" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/gin-gonic/gin" +) + +// TestChatFilesUpload_ExternalRuntime_Returns422 pins the contract that +// upload to a runtime="external" workspace returns a clear 422 with the +// "external workspaces don't support file upload" message instead of the +// misleading 503 "container not running" the v0.1 surface returned (#2308). +// +// Without this branch, an operator pasting a screenshot into the canvas chat +// for an external CEO workspace got a `503 {"error":"workspace container +// not running"}` — accurate from the upload handler's POV (no container +// exists for external workspaces) but misleading because it implies the +// container has crashed. The 422 with structured detail tells the operator +// what's actually happening + points at the v0.2 follow-up issue. +func TestChatFilesUpload_ExternalRuntime_Returns422(t *testing.T) { + mock := setupTestDB(t) + + const wsID = "00000000-0000-0000-0000-000000000001" + + // Runtime lookup returns "external" — should trigger the early return. + mock.ExpectQuery("SELECT COALESCE\\(runtime, ''\\) FROM workspaces"). + WithArgs(wsID). + WillReturnRows(sqlmock.NewRows([]string{"runtime"}).AddRow("external")) + + // Construct a multipart upload body — handler must reject BEFORE + // touching docker, so a nil templates field is intentional. If the + // runtime check is removed in a future change, this test crashes on + // nil deref of h.templates instead of silently passing. + body, contentType := buildMultipartUpload(t, "screenshot.png", []byte("fake-png-bytes")) + + // Use a nil-templates handler — proves runtime check happens BEFORE + // any docker plumbing. The handler is constructed in production via + // NewChatFilesHandler(templates) but the runtime branch should never + // reach into templates for external workspaces. + h := &ChatFilesHandler{templates: nil} + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: wsID}} + c.Request = httptest.NewRequest("POST", "/workspaces/"+wsID+"/chat/files", bytes.NewReader(body)) + c.Request.Header.Set("Content-Type", contentType) + + h.Upload(c) + + if w.Code != http.StatusUnprocessableEntity { + t.Fatalf("expected 422, got %d: %s", w.Code, w.Body.String()) + } + + var resp map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("failed to parse response: %v", err) + } + if !strings.Contains(resp["error"], "external workspaces") { + t.Errorf("expected error to mention external workspaces, got: %q", resp["error"]) + } + if resp["runtime"] != "external" { + t.Errorf("expected runtime=external in response, got %q", resp["runtime"]) + } + // Spot-check that the error points at issue #2308 so operators reading + // it know where to track v0.2 file-ingest. + if !strings.Contains(resp["detail"], "2308") { + t.Errorf("expected detail to reference issue #2308, got: %q", resp["detail"]) + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// buildMultipartUpload returns a body + content-type pair suitable for +// posting a single file through the chat upload handler. +func buildMultipartUpload(t *testing.T, filename string, contents []byte) ([]byte, string) { + t.Helper() + var buf bytes.Buffer + w := multipart.NewWriter(&buf) + part, err := w.CreateFormFile("files", filename) + if err != nil { + t.Fatalf("CreateFormFile: %v", err) + } + if _, err := part.Write(contents); err != nil { + t.Fatalf("part.Write: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("multipart Close: %v", err) + } + return buf.Bytes(), w.FormDataContentType() +}