revert(chat_files): drop the wrong external-runtime gate (#2308)

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) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang 2026-04-29 13:52:23 -07:00
parent e779a4ae7b
commit 51e48a267a
2 changed files with 0 additions and 123 deletions

View File

@ -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"})

View File

@ -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()
}