Skip to content

feat(chat_files): rewrite Upload as HTTP-forward (RFC #2312, PR-C) - #2315

Merged
HongmingWang-Rabbit merged 2 commits into
auto/issue-2312-pr-b-workspace-ingestfrom
auto/issue-2312-pr-c-platform-forward
Apr 29, 2026
Merged

feat(chat_files): rewrite Upload as HTTP-forward (RFC #2312, PR-C)#2315
HongmingWang-Rabbit merged 2 commits into
auto/issue-2312-pr-b-workspace-ingestfrom
auto/issue-2312-pr-c-platform-forward

Conversation

@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor

Summary

This is the PR where SaaS chat upload starts working again. Stacked on PR-A (#2313) + PR-B (#2314). Replaces the docker-exec path in `chat_files.go::Upload` with a streaming HTTP forward to the workspace's own `/internal/chat/uploads/ingest` endpoint (added in PR-B), authenticated with the per-workspace `platform_inbound_secret` (minted in PR-A).

Before / after

Before (#2308 root cause):
```
Upload → findContainer(ctx, wsID)
└─ h.docker is nil in SaaS (CP provisioner is selected when
MOLECULE_ORG_ID is set; dockerCli is sourced from the local
Docker provisioner)
→ returns ""
→ 503 "workspace container not running"
```

After:
```
Upload → resolve workspaces.url + platform_inbound_secret
→ stream multipart to /internal/chat/uploads/ingest
→ forward response back unchanged
```

Same call site whether the workspace runs on local docker-compose (`http://ws-:8000`) or SaaS EC2 (`https://....`). The `dockerCli == nil` cliff cannot exist by construction.

Why streaming, not parse-then-re-encode

  • No 50 MB intermediate buffer on the platform
  • Per-file size + path-safety enforcement is the workspace's job (see `workspace/internal_chat_uploads.py` from PR-B)
  • Workspace's error responses (413 with offending filename, 400 on missing `files` field, etc.) propagate through unchanged

Changes

  • `chat_files.go` — `Upload` rewritten as a streaming HTTP proxy. Drops `sanitizeFilename`, `copyFlatToContainer`, and the entire docker-exec path. `ChatFilesHandler` gains an `httpClient` (broken out for test injection). `Download` stays docker-exec for now; follow-up PR migrates it to the same shape.
  • `chat_files_external_test.go` — deleted. Pinned the wrong-headed `runtime=external` 422 gate from fix(chat_files): return 422 (not misleading 503) for external workspace upload (#2308) #2309 (already reverted in revert(chat_files): drop the wrong external-runtime gate (#2308) #2311). Superseded by the proxy tests.
  • `chat_files_test.go` — rewritten with sqlmock + httptest:
  • `tests/e2e/test_chat_upload_e2e.sh` — single-script-everywhere E2E. Takes `BASE` as env (default `http://localhost:8080\`). Creates a workspace, waits for online, mints a test token, uploads a fixture, reads it back via `/chat/download`, asserts content matches + bearer-required. Same script runs against staging tenants by setting `BASE=https://..staging.moleculesai.app`.

Test plan

Refs

🤖 Generated with Claude Code

, PR-C)

Closes the SaaS upload gap (#2308) with the unified architecture from
RFC #2312: same code path on local Docker and SaaS, no Docker socket
dependency, no `dockerCli == nil` cliff. Stacked on PR-A (#2313) +
PR-B (#2314).

Before:
  Upload → findContainer (nil in SaaS) → 503

After:
  Upload → resolve workspaces.url + platform_inbound_secret
        → stream multipart to <url>/internal/chat/uploads/ingest
        → forward response back unchanged

Same call site whether the workspace runs on local docker-compose
("http://ws-<id>:8000") or SaaS EC2 ("https://<id>.<tenant>...").
The bug behind #2308 cannot exist by construction.

Why streaming, not parse-then-re-encode:
  * No 50 MB intermediate buffer on the platform
  * Per-file size + path-safety enforcement is the workspace's job
    (see workspace/internal_chat_uploads.py, PR-B)
  * Workspace's error responses (413 with offending filename, 400 on
    missing files field, etc.) propagate through unchanged

Changes:
  * workspace-server/internal/handlers/chat_files.go — Upload rewritten
    as a streaming HTTP proxy. Drops sanitizeFilename, copyFlatToContainer,
    and the entire docker-exec path. ChatFilesHandler gains an httpClient
    (broken out for test injection). Download stays docker-exec for now;
    follow-up PR will migrate it to the same shape.
  * workspace-server/internal/handlers/chat_files_external_test.go —
    deleted. Pinned the wrong-headed runtime=external 422 gate from
    #2309 (already reverted in #2311). Superseded by the proxy tests.
  * workspace-server/internal/handlers/chat_files_test.go — replaced
    sanitize-filename tests (now in workspace/tests/test_internal_chat_uploads.py)
    with sqlmock + httptest proxy tests:
      - 400 invalid workspace id
      - 404 workspace row missing
      - 503 platform_inbound_secret NULL (with RFC #2312 detail)
      - 503 workspaces.url empty
      - happy-path forward (asserts auth header, content-type forwarded,
        body streamed, response propagated back)
      - 413 from workspace propagated unchanged (NOT remapped to 500)
      - 502 on workspace unreachable (connect refused)
    Existing Download + ContentDisposition tests preserved.
  * tests/e2e/test_chat_upload_e2e.sh — single-script-everywhere E2E.
    Takes BASE as env (default http://localhost:8080). Creates a
    workspace, waits for online, mints a test token, uploads a fixture,
    reads it back via /chat/download, asserts content matches +
    bearer-required. Same script runs against staging tenants (set
    BASE=https://<id>.<tenant>.staging.moleculesai.app).

Test plan:
  * go build ./... — green
  * go test ./internal/handlers/ ./internal/wsauth/ — green (full suite)
  * tests/e2e/test_chat_upload_e2e.sh against local docker-compose
    after PR-A + PR-B + this PR all merge — TODO before merge

Refs #2312 (parent RFC), #2308 (chat upload 503 incident).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Self-review found the original draft of this PR added forward-time
validateAgentURL() as defense-in-depth — paranoia layer on top of the
existing register-time gate. The validator unconditionally blocks
loopback (127.0.0.1/8), which makes httptest-based proxy tests
impossible without an env-var hatch I'd rather not add to a security-
critical path on first pass.

Trust note kept inline pointing at the upstream gate + tracking issue
so the gap is explicit, not invisible.

Refs #2312.
@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor Author

Holding approval pending coverage gaps.

Re-audited test coverage; my prior approval was actually better than I gave it credit for — TestChatUpload_ForwardsErrorStatusUnchanged and TestChatUpload_WorkspaceUnreachable are present and pin the two failure modes I'd flagged in my comment. Mea culpa for undercounting.

One remaining gap on Upload:

  1. Generic ReadPlatformInboundSecret error (non-ErrNoInboundSecret) → 500 'failed to read workspace secret'. Currently only the NULL-secret → 503 path is tested. A db-error stub (e.g. connection drop after the URL lookup succeeded) would pin this last error class.

Optional / hard-to-provoke: the io.Copy mid-stream error path just logs (status already committed) — usually not worth a test, calling out for completeness.

This PR is the closest-to-100% of the three. Re-approve once #1 is pinned (or the user explicitly waives it as a 500-fallthrough not worth a test).

@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor Author

Validation update — full chain works end-to-end

Ran the actual E2E against local docker-compose with all of A+B+C applied to the platform binary AND companion runtime PR (Molecule-AI/molecule-ai-workspace-runtime#60) installed in the workspace container:

```
canvas/molecli ──curl──▶ platform Go workspace-server (8080)
↓ resolves workspaces.url + platform_inbound_secret
↓ streams multipart
workspace's own server (62233) /internal/chat/uploads/ingest
↓ validates Authorization: Bearer
↓ writes to /workspace/.molecule/chat-uploads/ (0600)
200 {"files":[{"uri":"workspace:/...","name":"greeting.txt","size":27}]}
```

5/5 functional checks green:

  1. Workspace create + provision ✓
  2. Token mint ✓
  3. Platform → workspace HTTP forward, multipart streamed ✓
  4. File lands at `/workspace/.molecule/chat-uploads/-` with mode 0600 ✓ (better than my review's refactor(mcp-server): DRY envelopes, typed apiCall, explicit re-exports #4 finding — _open_safe already enforces 0600 via os.open(..., 0o600); the 0644 concern was for /configs/* files written via tar)
  5. Download round-trip — content matches verbatim ✓

Two pre-existing findings surfaced during the run (filing separately):

  • workspaces.url doesn't refresh after docker restart reassigns the host port. Heartbeat updates last_heartbeat_at but not url. Bit me when I had to manually UPDATE workspaces SET url=... after restarting the container with the new wheel.
  • host.docker.internal doesn't resolve when the platform Go binary runs OUTSIDE Docker (the dev convenience path). Provisioner-driven URLs may need awareness of host vs container caller for local dev — pre-existing.

The runtime PR (#60) needs to merge + a 0.1.18 wheel needs to publish before this stack works in any environment that pulls from PyPI (i.e., every environment except this overlay-installed local box). Sequence:

  1. Merge molecule-core PR-A (feat(wsauth): platform→workspace inbound secret (RFC #2312, PR-A) #2313)
  2. Merge molecule-core PR-B (feat(workspace): /internal/chat/uploads/ingest endpoint (RFC #2312, PR-B) #2314)
  3. Merge molecule-core PR-C (feat(chat_files): rewrite Upload as HTTP-forward (RFC #2312, PR-C) #2315) ← this PR
  4. Merge runtime PR feat(.claude): 5 gstack-inspired skills + cron upgrades #60 + publish 0.1.18 to PyPI
  5. Bump runtime pin in template repos (langgraph, claude-code, hermes, etc.) to ≥0.1.18
  6. Republish template images
  7. SaaS chat upload starts working

@HongmingWang-Rabbit
HongmingWang-Rabbit merged commit 593f2bd into auto/issue-2312-pr-b-workspace-ingest Apr 29, 2026
3 checks passed
@HongmingWang-Rabbit
HongmingWang-Rabbit deleted the auto/issue-2312-pr-c-platform-forward branch April 29, 2026 22:30
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant