From 51e48a267ab5a920f1dc050e6b090c6c706989cf Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 29 Apr 2026 13:52:23 -0700 Subject: [PATCH] revert(chat_files): drop the wrong external-runtime gate (#2308) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #2309 added an early-return that 422'd uploads to external workspaces with "file upload not supported." Both halves of that diagnosis were wrong: 1. External workspaces SHOULD support uploads — gating with 422 locks off intended functionality and labels it as design. 2. The 503 the user actually hit was on an INTERNAL workspace, not an external one. The runtime check never even ran. Real root cause (separate fix incoming): - findContainer(...) requires a non-nil h.docker. - In SaaS (MOLECULE_ORG_ID set), main.go selects the CP provisioner instead of the local Docker provisioner — dockerCli is nil. - findContainer short-circuits to "" → 503 "container not running" on every workspace, internal or external, on Railway-hosted SaaS where workspaces actually live on EC2. This PR strips the misleading gate so #2308 can be re-investigated against the real symptom. The proper fix routes the multipart upload over HTTP to the workspace's URL when dockerCli is nil — tracked as a follow-up. Refs #2308. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/chat_files.go | 24 ----- .../handlers/chat_files_external_test.go | 99 ------------------- 2 files changed, 123 deletions(-) delete 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 96709848..490828b6 100644 --- a/workspace-server/internal/handlers/chat_files.go +++ b/workspace-server/internal/handlers/chat_files.go @@ -13,7 +13,6 @@ import ( "bytes" "context" "crypto/rand" - "database/sql" "encoding/hex" "fmt" "io" @@ -25,7 +24,6 @@ import ( "regexp" "strings" - "github.com/Molecule-AI/molecule-monorepo/platform/internal/db" "github.com/docker/docker/api/types/container" "github.com/gin-gonic/gin" ) @@ -180,28 +178,6 @@ 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 deleted file mode 100644 index ce37b940..00000000 --- a/workspace-server/internal/handlers/chat_files_external_test.go +++ /dev/null @@ -1,99 +0,0 @@ -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() -}