Skip to content

fix: end multipart responses with the closing boundary - #592

Merged
CyMule merged 3 commits into
mainfrom
fix/multipart-close-boundary
Oct 8, 2026
Merged

CyMule merged 3 commits into
mainfrom
fix/multipart-close-boundary

Conversation

@CyMule

@CyMule CyMule commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

What

multipart/mixed responses now end with the closing boundary line that the MIME format requires.

Before (end of response):   After (end of response):
<last part data>            <last part data>
                            --3f9c...--

Why

Without the closing line, strict multipart parsers report the response as incomplete. If processing fails partway through, the closing line is still left out, so clients can tell a cut-off response from a complete one.

Validation

Tests parse the response with Python's email parser (no defects) for one part, several parts, and no parts, and cover an interrupted stream. They fail on main. Adds a CHANGELOG entry and bumps the version to 0.1.15.

Refreshed against main after #590 and #591 merged. Targeted API tests: 43 passed; response compatibility tests: 4 passed. Changed-source lint passed.

@CyMule
CyMule marked this pull request as ready for review October 7, 2026 15:43
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-07T15:45:13.522668Z 98456c1 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Shadow auto-approve: would not auto-approve. This PR does not meet the repository auto-approval settings.

View guided diff | Re-trigger cubic

@CyMule CyMule changed the title Close multipart responses with the terminal boundary fix: end multipart responses with the closing boundary Oct 7, 2026
@paulkarayan

Copy link
Copy Markdown

Automated review, forwarded by @paulkarayan. Findings are code-verified where they name a file and line; the strong-review gpt-pro leg and cubic did not run.

GO -- 0 blocker(s), 0 concern(s), 2 note(s), 0 nit(s).

Correct, spec-conforming fix: multipart responses now end with the closing boundary; needs a rebase and a new version number since main took 0.1.12.

Notes (2)

  1. Needs a rebase: main already claims 0.1.12 (unstructured-api 589), the PR is CONFLICTING in CHANGELOG.md, and open unstructured-api 591/590/581 already claim 0.1.13; pick the next free version.
  2. Wire bytes change for every multipart/mixed caller: a client that hand-splits on --boundary now sees a trailing '--\r\n' segment; no such consumer found locally, CHANGELOG discloses it.
Full review
# Unstructured-IO/unstructured-api#592 -- fix: end multipart responses with the closing boundary

Asked by pk, DM, https://unstructuredai.slack.com/archives/D0BFPFGDNP7/p1791404835140279
Author: CyMule (Nick Franck). Reviewed at head 98456c18e013e4f4e7947cf847d966d794700d8c, base origin/main 8195367.

**Verdict:** GO
**Blast radius:** 3/5

`MultipartMixedResponse.stream_response` now sends `--<boundary>--\r\n` as its final body message instead of an empty one, so a completed `multipart/mixed` response is a well-formed MIME body; a stream that raises mid-way still ends without it, so a client can tell truncated from complete. The code is correct: a stdlib `email` parse of base bytes reports `CloseBoundaryNotFoundDefect` and the head bytes parse clean with identical part payloads (observed). Codex and the fable leg found no defect, CI is green. Before it merges it needs a rebase: main took version 0.1.12 an hour before this review, so the PR now conflicts and needs a new version number.

## Notes

- **Needs a rebase and a new version (observed).** `origin/main` already carries `__version__ = "0.1.12"` and a `## 0.1.12` CHANGELOG entry from 589 (merged 2026-10-07 22:03 UTC). GitHub reports this PR `mergeable: CONFLICTING` (CHANGELOG.md). Three other open PRs (unstructured-api 591, 590, 581) already claim 0.1.13, and each conflicts with this one only in `CHANGELOG.md` and `__version__.py` (`git merge-tree`; `general.py` auto-merges with all three). The author picks the next free number when rebasing; whichever of these lands later rebumps.
- **Wire bytes change for every multipart caller (observed).** A client that hand-splits the body on `--<boundary>` and treats every non-empty segment as a part now gets a third segment `--\r\n` after the last part. The stdlib `email` parser handles it (observed); other parsers were not tested. No such hand-rolled consumer was found in this repo or in the local Unstructured checkouts searched (unknown for external clients). The CHANGELOG entry discloses the change.

## Findings

None. No BLOCKER or CONCERN survived verification.

## Dismissed

- ruff: `I001`, `SIM211`, `E721`, `E501` and one `format --check` diff in `prepline_general/api/general.py` (lines 1-44, 55, 653). All five reproduce identically on the `origin/main` copy of the file; none is on a line this PR touches. The PR's own lines and the new test file are clean. CI `lint` passed.
- Empty-iterator case emits a close delimiter with zero parts, which RFC 2046 does not allow (fable, as a non-defect). Unreachable: `files: List[UploadFile]` is a required form field, so FastAPI returns 422 before `response_generator` runs.
- Possible Content-Length conflict from a non-empty final body message (fable). `StreamingResponse` sets no content-length; the response is chunked. Inferred from Starlette source semantics, not run.
- blast-radius `wire_contract` 3: the risk it names (lenient clients splitting on the boundary) is real but has no identified caller; recorded as a Note, not a finding.

## What each leg said

- **blast-radius:** 3/5. One-line code change, but it alters the final bytes of every `multipart/mixed` response, so external clients are the risk surface; `needs_live_test: true`.
- **cheshire:** 0 flags, complexity SIMPLE, no slop, no doc updates implied. Ran on the packaged starter config ("no config found"), without AGENTS.md (ungrounded).
- **collisions (review-graph):** complete, 0 pairs (5 of 25 gh calls; first attempt hit another run's lock, the retry succeeded). Below bar: unstructured-api 581 and 591 share only `general.py`; manual `git merge-tree` confirms `general.py` auto-merges with both and the only conflicts are CHANGELOG/version bookkeeping (see Notes). 5 open PRs excluded by the exclude hook.
- **fable (strong-review, report-only, no PR marker):** SAFE TO MERGE, no actionable findings. Confirmed RFC 2046 framing from `build_part`'s trailing CRLF, failure path unchanged, zero-part body unreachable, no in-repo consumer splits on the boundary.
- **codex (`codex exec review --base origin/main`):** no actionable regression; the closing delimiter matches the boundary and the tests cover completed, empty and interrupted streams.
- **ruff 0.15.12 (uv.lock version):** changed lines clean; 4 check errors + 1 format diff are pre-existing on base (see Dismissed).
- **differential probe (stdlib, base vs head bytes):** base => `CloseBoundaryNotFoundDefect`, head => no defects, payloads identical.
- **CI (`gh pr checks`, 2026-10-07 22:23 UTC):** all pass (test 8m43s, test_dockerfile, lint, changelog, CodeQL, Socket, cubic bot); `claude` skipping.
- **NOT RUN, gpt-pro / oracle strong-review leg:** needs a browser on a shared CDP port; excluded from this unattended run. strong-review is therefore fable-only.
- **NOT RUN, cubic (local):** disabled for this run. The cubic GitHub check on the PR passed; that is CI, not a local pass.
- **NOT RUN, test-critic and marmalade:** outside this run's scope.

## Not covered

- The new tests were not run locally (the env needs `unstructured[all-docs]`); CI `test` passed at this head.
- No real HTTP path was exercised: no uvicorn server, no real SDK or platform client parsing a live multipart response.
- External consumers of `Accept: multipart/mixed` (unstructured-client SDKs, customer code) were not enumerated beyond a local grep.
- The Starlette 1.1.0 call path (`__call__` -> `stream_response`) was checked against a cached Starlette source of unconfirmed version, not the locked one.
- gpt-pro strong-review leg, local cubic, test-critic, marmalade (see above).

@CyMule
CyMule merged commit 4483521 into main Oct 8, 2026
14 checks passed
@CyMule
CyMule deleted the fix/multipart-close-boundary branch October 8, 2026 02:49
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.

2 participants