feat(api): add configurable --max-request-size flag to serve api - #3938
Conversation
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Summary
Solid, well-scoped implementation. The variadic Option pattern keeps all existing New/NewWithManager call sites compiling unchanged and preserves the 1 MiB default, and the zero/negative fallback is both implemented and tested. Tests cover the three behaviours that matter — under-limit passthrough, over-limit 413, and zero/negative fallback — and correctly target the POST /api/sessions/:id/messages route called out in #3937.
One major item to address before merge (the canonical CLI reference), plus two non-blocking notes — all left as inline comments.
Leaving this as a comment rather than an approval only because the required checks haven't reported yet; happy to approve once they're green.
| | ------------------ | ---------------- | ------------------------------------------------ | | ||
| | `-l, --listen` | `127.0.0.1:8080` | Address to listen on | | ||
| | `--auth-token` | (none) | Bearer token required for all API requests. Leave empty to disable authentication (safe when listening on loopback interfaces only). Recommended when `--listen` binds to a network-reachable interface. | | ||
| | `--max-request-size <bytes>` | `1048576` (1 MiB) | Maximum request body size in bytes. Requests whose body exceeds this limit are rejected with HTTP 413 (Request Entity Too Large). | |
There was a problem hiding this comment.
[major] The canonical CLI reference is missing this flag.
docs/features/cli/index.md carries a per-command flags table, and the serve api table (under ### docker agent serve api, lines 282–292 on main) still lacks --max-request-size. The precedent set by serve chat — the very flag this PR mirrors — documents it in both places:
docs/features/cli/index.md:396docs/features/chat-server/index.md:198
This PR updates only this file, so the CLI reference now under-documents serve api relative to serve chat. Please add a matching row to the serve api flags table in docs/features/cli/index.md.
| maxBytes := o.maxRequestBytes | ||
| if maxBytes <= 0 { | ||
| maxBytes = defaultMaxRequestBytes | ||
| } |
There was a problem hiding this comment.
[minor] Duplicated default/fallback logic across the two servers.
This repeats the defaultMaxRequestBytes constant and the <= 0 fallback that pkg/chatserver/server.go already has (:104, :226-228). Acceptable as-is since these are independent packages — flagging only in case you'd prefer a small shared helper so the two can't drift.
| cmd.PersistentFlags().StringVar(&flags.fakeResponses, "fake", "", "Replay AI responses from cassette file (for testing)") | ||
| cmd.PersistentFlags().StringVar(&flags.recordPath, "record", "", "Record AI API interactions to cassette file") | ||
| cmd.PersistentFlags().StringVar(&flags.authToken, "auth-token", "", "Bearer token required for API requests (empty = no authentication)") | ||
| cmd.PersistentFlags().Int64Var(&flags.maxRequestSize, "max-request-size", 1<<20, "Maximum request body size in bytes (default 1 MiB). Requests exceeding this limit are rejected with HTTP 413.") |
There was a problem hiding this comment.
[nit] Help-string double default.
(default 1 MiB) renders alongside Cobra's auto-appended (default 1048576). You already called this out as intentional for consistency with serve chat (cmd/root/chat.go:51) — agreed, no change needed. Noting it only so the next reader doesn't re-file it.
Addressing the three review threads: [major] Missing [minor] Duplicated default/fallback logic — Keeping it as-is. [nit] Double default in help string — Intentional, matches the existing |
Mirrors the --max-request-size flag that serve chat already has. - Add Option/WithMaxRequestBytes to pkg/server via functional-options (variadic, so all existing NewWithManager/New call sites compile untouched and keep the 1 MiB default). - Register --max-request-size PersistentFlag on the serve api command (int64 bytes, default 1<<20) and thread the value through server.New. - BodyLimit middleware is global on the echo instance, so the cap applies consistently to every /api route including POST /api/sessions/:id/messages. - Oversized requests return HTTP 413 (Request Entity Too Large). - Tests: existing default-cap test kept; new table tests for custom cap on the messages route, plus zero/negative fallback to 1 MiB. - Docs: add --max-request-size row to the api-server CLI Flags table. Closes #3937
The per-command flags reference in docs/features/cli/index.md was missing the new --max-request-size flag for serve api. Add a matching row, mirroring the existing serve chat entry at line 395.
d06f142 to
0724fe0
Compare
2 participants
Fixes #3937.
Adds
--max-request-size <bytes>toserve api, mirroring the flag thatserve chatalready has. The default remains 1 MiB; requests exceeding the limit return HTTP 413.Changes:
pkg/server: newOption/WithMaxRequestBytesfunctional option (variadic — all ~22 existingNewWithManager/Newcall sites compile unchanged, keeping the 1 MiB default)cmd/root/api.go:--max-request-sizePersistentFlag (int64 bytes, default1<<20) threaded throughserver.Newpkg/server/server_test.go: table tests for custom cap onPOST /api/sessions/:id/messages(under-limit→400, over-limit→413) and zero/negative fallback to 1 MiBdocs/features/api-server/index.md: new CLI flags table row documenting default, units, and 413 behaviourNote: The flag help string includes "(default 1 MiB)" alongside Cobra's auto-appended "(default 1048576)" — this matches the existing pattern in
serve chatand is intentionally kept for consistency.