Support OpenAPI multipart form requests - #72
Conversation
Agents reach for curl syntax (--body @/tmp/body.json) or pass a bare file
path. Both were sent verbatim as the request body, and the API answered
{"detail": "Bad Request: Invalid JSON"} — a message that reads like a
body-SHAPE problem and sends the caller back to re-read the schema for a
mistake that was purely about transport.
--body/--json-body now resolve "@path" (and curl's "@-") to file contents
under the same 10 MB cap as stdin, and every JSON-media-type body is run
through json.Valid before any network call. A value that looks like a path
(/, ./, ../, ~/ prefix, or an existing file) gets an error naming both
working forms instead of a parse error.
Constraint: bytes must reach the server unchanged — validation uses json.Valid and never re-serializes, so field order and formatting survive
Constraint: body shorthand sets --body internally to marshaled JSON; that path stays valid and untouched
Rejected: schema-aware validation of the body | needs the full JSON Schema evaluator and would reject bodies the API actually accepts
Rejected: silently treating a bare existing path as a file | hides the typo class this is meant to surface, and changes what an existing script sends
Confidence: high
Scope-risk: narrow
Directive: multipart/form-data operations (uploads) skip validation via operationInfo.BodyNonJSON — keep that carve-out if more media types appear
Not-tested: reading from a FIFO or /dev/stdin via @path
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014TwwKSAsAGPBToNb4iUe5s
|
I think maybe we should support |
|
Awesome, I can wait until #75 merges! |
4d7aff1 to
dfb2e96
Compare
0682e36 to
0827738
Compare
There was a problem hiding this comment.
Pull request overview
Adds schema-driven OpenAPI multipart upload support while preserving JSON body compatibility and media types.
Changes:
- Generates multipart flags, file parts, arrays, and content types.
- Adds
--body @filehandling and JSON validation. - Normalizes camelCase flags with deprecated aliases.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents CSV uploads and body input. |
| cmd/omni/agent_help.go | Adds multipart usage guidance. |
| cmd/omni/main.go | Forwards request content types. |
| internal/auth/auth.go | Supports caller-provided content types. |
| internal/auth/auth_test.go | Tests multipart boundaries. |
| internal/openapi/body_input.go | Resolves and validates body input. |
| internal/openapi/body_input_test.go | Tests body sources and validation. |
| internal/openapi/generate.go | Generates multipart commands and normalized flags. |
| internal/openapi/generate_test.go | Tests normalized query flags. |
| internal/openapi/multipart.go | Builds schema-driven multipart requests. |
| internal/openapi/multipart_test.go | Tests multipart generation and uploads. |
Suppressed comments (1)
internal/openapi/multipart.go:219
- Decoding into
interface{}does not verify the declared flag type and does not check for trailing input. For example, an array field accepts{"x":1}and serializes it as one object part, while["a"] trailingsilently ignores the trailing text. Decode into the schema-appropriate array/object type and require EOF after the first JSON value.
case "array", "object":
var parsed interface{}
decoder := json.NewDecoder(strings.NewReader(value))
decoder.UseNumber()
if err := decoder.Decode(&parsed); err != nil {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if bodyFlag != "" && jsonBodyFlag != "" { | ||
| return fmt.Errorf("cannot use both --body and --json-body; use one or the other") | ||
| } | ||
|
|
||
| effectiveBody := bodyFlag | ||
| effectiveBody, flagName := bodyFlag, "body" |
| if bodyProvided { | ||
| decoder := json.NewDecoder(bytes.NewReader(rawBody)) | ||
| decoder.UseNumber() | ||
| if err := decoder.Decode(&values); err != nil { | ||
| return nil, "", fmt.Errorf("invalid multipart --body JSON: %w", err) | ||
| } | ||
| } |
| if err != nil { | ||
| return fmt.Errorf("creating multipart file field %q: %w", field.Name, err) | ||
| } | ||
| if _, err := io.Copy(part, file); err != nil { |
| // a JSON document — an absolute/relative path prefix, or the name of a file | ||
| // that actually exists. | ||
| func looksLikePath(raw string) bool { | ||
| if raw == "" || strings.ContainsAny(raw, " \t\r\n") { |
| // mistyped file path. Operations whose request body isn't JSON (e.g. the | ||
| // multipart upload endpoints) pass validateJSON=false and get the bytes back | ||
| // untouched. |
| } | ||
| } | ||
|
|
||
| // Non-JSON media types (the multipart upload endpoints) skip validation. |
n8agrin
left a comment
There was a problem hiding this comment.
Claude's review pass. The plausible section doesn't seem critical but would be nice to get the other's closed:
Confirmed (live-reproduced):
--body "" on a multipart command (e.g. uploads create) fails with a confusing invalid multipart --body JSON: EOF even when --file/other flags fully supply the request.
Multipart binary-field file paths (--file, or "file" in --body JSON) never get ~ expanded, unlike --body @path, so ~/people.csv fails with "no such file or directory".
Plausible (real code-level regressions, not yet triggered by the current spec):
registerMultipartFlags's collision handling can panic the whole CLI at startup if a future spec has colliding field names — flagged independently by 5 of the 8 finder angles.
requestBodyMediaType dropped the old nil-schema guard, so it could silently prefer a schema-less application/json entry over a real multipart schema.
multipartFields's AllOf walk shares one cycle-guard map instead of cloning per branch (like schema.go's gatherObject does), so a diamond AllOf composition could silently drop properties.
readBodyFile duplicates readStdin's size-cap pattern with already-diverging error text.
|
Superseded — closing. The multipart work already landed on @n8agrin's findings and the Copilot comments are handled in #82, which targets that branch:
Two I didn't take. The |
…82) Follow-up to the multipart work now carried on this branch (ffebcee). `3d9b0e1` and `0827738` already covered the body-flag Changed state and the file-path hint; these are the findings from #72's review that no commit here has picked up yet. - Binary multipart field paths never expanded `~`, unlike `--body @path`, so `--file ~/people.csv` failed with "no such file or directory". - `--body null` decoded into a nil map and panicked ("assignment to entry in nil map") as soon as any generated flag was merged into it. - Array and object flag values decoded into interface{}, so an object was accepted where the schema says array, and anything after the first JSON value was silently dropped: `--labels '["a"] oops'` sent `["a"]`. The declared type is pinned now and the input must end there. - registerMultipartFlags checked its "form-" replacement against nothing, so two fields colliding on one flag name would register the same pflag twice and panic at startup, taking down every command, not just the upload. - requestBodyMediaType could prefer a schema-less application/json entry over a real multipart definition. Each fix has a regression test; without the source changes the tilde and nil-map tests fail, the latter by panicking. Not addressed: Copilot's note that the upload file is buffered in memory before the request is sent. Streaming means threading an io.Reader through APIRequest and internal/auth, which is wider than a review-fix pass. Claude-Session: https://claude.ai/code/session_01DYuiGGkmQifkCbF2qL8Lt6 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Important
Stacked on #75 — this PR targets
feat/body-file-input, notmain. Merge #75 first; this base retargets tomainautomatically once it lands. The diff shown here is multipart-only.Summary
--bodyJSON compatibility and honor required fields, arrays, and per-part content typesHow it composes with #75
#75 added
--body @filereading and client-side JSON validation, gated on aBodyNonJSONflag it computed per operation. Rebased on top of it:BodyNonJSONis replaced byoperationInfo.bodyFlagIsJSON(), derived from themedia type this PR already resolves — one source of truth instead of two scans
of the same request body.
--bodyon an upload command is aJSON object of field values, so it gets feat(body): accept --body @file and validate JSON client-side #75's
@filereading, validity check,and file-path hint before
buildMultipartBodyparses it.@path/to/file.jsonalongside the file-pathsemantics for binary fields.
Two tests cover the seam:
--body @fileon an upload command, and a bare path--bodyproducing #75's hint without calling the executor.Verification
go test ./...go vet ./...uploads create,uploads list, anduploads replace-dataomni.csvend to end: 559 rows ingested and the upload was read back successfullyExample
Fixes #70