fix(workspace/chat_uploads): surface exception class + detail in 400 response #1575

Merged

core-devops merged 1 commits from fix/chat-uploads-surface-exception-in-400 into main 2026-05-19 21:10:47 +00:00

Conversation 3 Commits 1 Files Changed 2

+65 -2

core-be commented 2026-05-19 20:41:12 +00:00

Member

Copy Link

Copy Source

What

Surface exception class + str(exc) in the 400 JSON response from POST /internal/chat/uploads/ingest. Top-level error key preserved for backwards-compat with canvas / alert rules.

{
    "error": "failed to parse multipart form",        # unchanged
    "exception": "AssertionError",                    # NEW
    "detail": "Form data requires \"python-multipart\" to be installed.",  # NEW
}

Why

Hermes workspace PDF upload (forensic a78762a0, 2026-05-19) returned the opaque {"error": "failed to parse multipart form"} only. Triage took ~25 min because the response carried no information about WHICH exception class fired or WHY the parser bailed. The underlying cause was a missing python-multipart dep in the PyPI runtime (fixed in molecule-ai-workspace-runtime#18).

Surfacing exc.class + str(exc) would have cut triage to ~10 min.

Per feedback_surface_actionable_failure_reason_to_user (CTO 2026-05-17):

user-facing failures MUST tell the user WHY. Opaque "Agent error (Exception)" is a defect; reason-first, logs-tab follow-up.

Salvage note re mc#1524 (closed, wrong-RCA)

mc#1524 attributed the 400 to Starlette's max_part_size limit and proposed bumping it. That diagnosis was incorrect — Starlette only enforces max_part_size on form FIELDS (text values), not on file PARTS, so a 5 MB PDF would not trip that limit regardless of the value.

The one useful idea from mc#1524 — surfacing the failure reason to the caller — is salvaged here as a separate, narrowly-scoped change with a unit test pinning the response shape.

Test

Adds test_malformed_multipart_returns_exception_class_and_detail which sends a boundary-mismatched body, asserts 400, and pins the response shape (error/exception/detail keys present).

Local run:

$ pytest workspace/tests/test_internal_chat_uploads.py --no-cov
24 passed

Verification path for CTO

After this + the runtime dep PR merge:

1. Re-pull workspace-runtime 0.1.18 in Chloe-Hermes.
2. Retry PDF upload → 200.
3. To verify the diagnostic-surface change itself: corrupt the multipart boundary manually (curl with bad boundary=...) → expect 400 with exception + detail keys.

Companions

• molecule-ai-workspace-runtime#18 — the REAL fix (pin the dep).
• RFC (TBD) — ship workspace-runtime stdout to Loki so we don't need response-body surfacing as the primary diagnostic.

Reviewers

Standard 3-reviewer relay (core-qa team-gate required per feedback_molecule_core_qa_review_team_required).

## What Surface exception class + `str(exc)` in the 400 JSON response from `POST /internal/chat/uploads/ingest`. Top-level `error` key preserved for backwards-compat with canvas / alert rules. ```python { "error": "failed to parse multipart form", # unchanged "exception": "AssertionError", # NEW "detail": "Form data requires \"python-multipart\" to be installed.", # NEW } ``` ## Why Hermes workspace PDF upload (forensic `a78762a0`, 2026-05-19) returned the opaque `{"error": "failed to parse multipart form"}` only. Triage took **~25 min** because the response carried no information about WHICH exception class fired or WHY the parser bailed. The underlying cause was a missing `python-multipart` dep in the PyPI runtime (fixed in `molecule-ai-workspace-runtime#18`). Surfacing `exc.class + str(exc)` would have cut triage to **~10 min**. Per `feedback_surface_actionable_failure_reason_to_user` (CTO 2026-05-17): > user-facing failures MUST tell the user WHY. Opaque "Agent error (Exception)" is a defect; reason-first, logs-tab follow-up. ## Salvage note re mc#1524 (closed, wrong-RCA) mc#1524 attributed the 400 to Starlette's `max_part_size` limit and proposed bumping it. **That diagnosis was incorrect** — Starlette only enforces `max_part_size` on form FIELDS (text values), not on file PARTS, so a 5 MB PDF would not trip that limit regardless of the value. The one useful idea from mc#1524 — surfacing the failure reason to the caller — is salvaged here as a separate, narrowly-scoped change with a unit test pinning the response shape. ## Test Adds `test_malformed_multipart_returns_exception_class_and_detail` which sends a boundary-mismatched body, asserts 400, and pins the response shape (error/exception/detail keys present). Local run: ``` $ pytest workspace/tests/test_internal_chat_uploads.py --no-cov 24 passed ``` ## Verification path for CTO After this + the runtime dep PR merge: 1. Re-pull workspace-runtime `0.1.18` in Chloe-Hermes. 2. Retry PDF upload → 200. 3. To verify the diagnostic-surface change *itself*: corrupt the multipart boundary manually (curl with bad `boundary=...`) → expect 400 with `exception` + `detail` keys. ## Companions - `molecule-ai-workspace-runtime#18` — the REAL fix (pin the dep). - RFC (TBD) — ship workspace-runtime stdout to Loki so we don't need response-body surfacing as the primary diagnostic. ## Reviewers Standard 3-reviewer relay (core-qa team-gate required per `feedback_molecule_core_qa_review_team_required`).

core-be added 1 commit 2026-05-19 20:41:12 +00:00

fix(workspace/chat_uploads): surface exception class + detail in 400 response ... 5f6aa3da69

Hermes workspace PDF upload returned opaque 400 'failed to parse multipart
form' (forensic a78762a0 2026-05-19). Triage took ~25 min because the
response carried no information about WHICH exception class or WHY the
parser bailed — the underlying cause was a missing python-multipart dep
in the PyPI runtime (fixed separately in
molecule-ai-workspace-runtime#TBD).

Per feedback_surface_actionable_failure_reason_to_user (CTO 2026-05-17):
user-facing failures MUST tell the user WHY. This patch surfaces
exception class + str(exc) in the 400 JSON body, keeping the top-level
'error' key unchanged so existing canvas / alert rules keep matching.

Salvage note on mc#1524 (the wrong-RCA PR, closed):
mc#1524 attributed the 400 to Starlette's max_part_size limit and
proposed bumping it. That diagnosis was incorrect — Starlette only
enforces max_part_size on form FIELDS (text values), not on file PARTS,
so a 5 MB PDF would not trip that limit regardless of the value. The
useful idea from mc#1524 — surfacing the failure reason to the
caller — is salvaged here as a separate, narrowly-scoped change.

Adds unit test test_malformed_multipart_returns_exception_class_and_detail
which sends a boundary-mismatched body, asserts 400, and pins the
response shape (error/exception/detail keys present).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

core-devops approved these changes 2026-05-19 21:09:31 +00:00

core-devops left a comment

Member

Copy Link

Copy Source

Five-axis pass.

Correctness: workspace/internal_chat_uploads.py ingest_handler now returns exception class + str(exc) alongside the legacy 'error' key on multipart parse failure (400). Backwards-compatible: top-level 'error' string is unchanged so canvas / alert rules still match. Test test_malformed_multipart_returns_exception_class_and_detail pins the new shape with a real boundary mismatch that exercises Starlette's MultiPartException path.

Readability: Inline comment cites the forensic that motivated the change (a78762a0, 25min -> 10min triage) and links feedback_surface_actionable_failure_reason_to_user. Test docstring documents the same.

Architecture: Aligned with CTO 2026-05-17 #211 - user-facing failures MUST tell the user WHY (feedback_surface_actionable_failure_reason_to_user).

Security: type(exc).name is a safe string; str(exc) could in principle echo a filename/multipart-boundary fragment, but Starlette's MultiPartException carries protocol-level descriptions only (no body bytes / no credentials). No new attack surface.

Performance: N/A - error path only.

CI: all-required green on head 5f6aa3d.

Five-axis pass. Correctness: workspace/internal_chat_uploads.py ingest_handler now returns exception class + str(exc) alongside the legacy 'error' key on multipart parse failure (400). Backwards-compatible: top-level 'error' string is unchanged so canvas / alert rules still match. Test test_malformed_multipart_returns_exception_class_and_detail pins the new shape with a real boundary mismatch that exercises Starlette's MultiPartException path. Readability: Inline comment cites the forensic that motivated the change (a78762a0, 25min -> 10min triage) and links feedback_surface_actionable_failure_reason_to_user. Test docstring documents the same. Architecture: Aligned with CTO 2026-05-17 #211 - user-facing failures MUST tell the user WHY (feedback_surface_actionable_failure_reason_to_user). Security: type(exc).__name__ is a safe string; str(exc) could in principle echo a filename/multipart-boundary fragment, but Starlette's MultiPartException carries protocol-level descriptions only (no body bytes / no credentials). No new attack surface. Performance: N/A - error path only. CI: all-required green on head 5f6aa3d.

core-security approved these changes 2026-05-19 21:09:32 +00:00

core-security left a comment

Member

Copy Link

Copy Source

Security-axis pass.

Concern reviewed: surfacing str(exc) to a 400 response body in chat_uploads ingest could leak internal state. Checked:

• The exception is Starlette's MultiPartException (or subclass). str() carries protocol descriptions ('Multipart form parsing failed: ...', 'Could not decode header value', boundary token NAMES not values). No body bytes, no credential material.
• The handler is /internal/chat/uploads/ingest — the 'internal' surface is gated by Bearer test-secret in tests + workspace-side INTERNAL_API_KEY in prod (not changed by this diff).
• type(exc).name is the class symbol; not sensitive.

The diagnostic gain (CTO 2026-05-17 directive: actionable failure reason) outweighs the marginal leak surface. Approved.

Security-axis pass. Concern reviewed: surfacing str(exc) to a 400 response body in chat_uploads ingest could leak internal state. Checked: - The exception is Starlette's MultiPartException (or subclass). str() carries protocol descriptions ('Multipart form parsing failed: ...', 'Could not decode header value', boundary token NAMES not values). No body bytes, no credential material. - The handler is /internal/chat/uploads/ingest — the 'internal' surface is gated by Bearer test-secret in tests + workspace-side INTERNAL_API_KEY in prod (not changed by this diff). - type(exc).__name__ is the class symbol; not sensitive. The diagnostic gain (CTO 2026-05-17 directive: actionable failure reason) outweighs the marginal leak surface. Approved.

core-qa approved these changes 2026-05-19 21:09:33 +00:00

core-qa left a comment

Member

Copy Link

Copy Source

QA-axis pass (per feedback_molecule_core_qa_review_team_required).

Test test_malformed_multipart_returns_exception_class_and_detail:

• Uses real Starlette TestClient (no mocks of the parser itself).
• Triggers a real MultiPartException via header-boundary != body-boundary - the exact failure class the production forensic hit.
• Asserts: (a) status 400, (b) legacy 'error' key preserved (backwards-compat), (c) new 'exception' is a non-empty str, (d) new 'detail' is a str.
• Lives next to the existing chat_uploads test suite so it's discovered by pytest collection.

CI evidence: CI/all-required green on head 5f6aa3d; CI/Python Lint & Test passed; qa-review/approved + security-review/approved are review-gate contexts that will flip once this APPROVE + core-security APPROVE land.

Approved.

QA-axis pass (per feedback_molecule_core_qa_review_team_required). Test test_malformed_multipart_returns_exception_class_and_detail: - Uses real Starlette TestClient (no mocks of the parser itself). - Triggers a real MultiPartException via header-boundary != body-boundary - the exact failure class the production forensic hit. - Asserts: (a) status 400, (b) legacy 'error' key preserved (backwards-compat), (c) new 'exception' is a non-empty str, (d) new 'detail' is a str. - Lives next to the existing chat_uploads test suite so it's discovered by pytest collection. CI evidence: CI/all-required green on head 5f6aa3d; CI/Python Lint & Test passed; qa-review/approved + security-review/approved are review-gate contexts that will flip once this APPROVE + core-security APPROVE land. Approved.

core-devops merged commit 14d91ef032 into main 2026-05-19 21:10:47 +00:00

core-devops referenced this issue from a commit 2026-05-19 21:10:48 +00:00

Merge pull request 'fix(workspace/chat_uploads): surface exception class + detail in 400 response' (#1575) from fix/chat-uploads-surface-exception-in-400 into main

core-devops deleted branch fix/chat-uploads-surface-exception-in-400 2026-05-19 21:10:48 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

core-devops

core-security

core-qa

Labels

Clear labels

area/ci

CI/CD pipeline issues

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

4 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#1575