[gjkim42]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Session attachments larger than 10 MiB are rejected. Raise the per-file limit to 100 MiB and total attachment storage per Session from 100 MiB to 1 GiB, with matching browser validation and documentation. The CLI and server use the shared runtime limit.

Give authenticated web attachment uploads a 15-minute read deadline. Stream downloads from the Session Pod to the HTTP response with bounded buffers, closing and canceling the exec stream when the consumer closes. Validate metadata before sending the response, set Content-Length, and abort failed or incorrectly sized transfers.

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

Validation passed:

• make update
• env -u CODEX_HOME -u CODEX_AUTH_JSON make test
• make verify

The test command removes inherited Codex environment settings that otherwise cause two unrelated entrypoint tests to fail. Unit coverage includes maximum-size storage, upload deadlines, streaming before exec completion, cancellation, invalid metadata, and failed or incorrectly sized transfers. The e2e Session test adds a 16 MiB upload/download round-trip; it runs in PR CI.

Does this PR introduce a user-facing change?

Session attachments support files up to 100 MiB each and up to 1 GiB of total attachment storage per Session. Web uploads allow up to 15 minutes to receive each file, and downloads stream without buffering the whole attachment in console memory.

[gjkim42]

/kelos claude-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-claude-reviewer-issue-comment-58e8f2256e09 has succeeded. ✅

[cubic-dev-ai]

No issues found across 5 files

_{Re-trigger cubic}

[kelos-bot]

🤖 Kelos Claude Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Raises the per-file Session attachment limit to 100 MiB and the per-Session quota to 1 GiB, gives authenticated web uploads a 15-minute read deadline, and streams downloads from the Pod exec to the HTTP response with Content-Length and abort-on-mismatch (reviewed at head 28298ff).

Findings Overview

Priority	Count	File:Line	Summary
P0	0	—	none
P1	0	—	none
P2	0	—	none
P3	1	test/e2e/session_test.go:1329	16 MiB e2e round-trip runs under the shared 30 s web-client timeout

Findings

No blocking findings. Both P1 items from the previous review are resolved at this head:

• Upload deadline — internal/consoleserver/server.go:1502 calls http.NewResponseController(writer).SetReadDeadline(now+15m) before any body read. In net/http this maps directly to conn.rwc.SetReadDeadline, which replaces the wholeReqDeadline derived from the server-wide ReadTimeout: 30s, and the deadline is reset per request after the handler returns. The handler runs only behind requireAuth, and there is no ResponseWriter wrapper in the middleware chain, so the call cannot hit ErrNotSupported in production. TestSessionAttachmentUploadAndDownload pins the deadline window.
• Download memory — internal/sessionattachment/client.go:63 now runs the exec against an io.Pipe, returns after the metadata line, and holds only a 4 KiB bufio buffer. Close() cancels the exec context before closing the pipe, so a blocked Stdout.Write unblocks with io.ErrClosedPipe and StreamWithContext observes context.Canceled (both orderings are covered by TestDownloadCloseCancelsExec). The metadata line is bounded by ReadSlice's 4 KiB buffer, which is safe because normalizeAttachmentName caps names at 255 bytes.
• Transfer integrity — internal/consoleserver/server.go:1567 sets Content-Length from validated metadata, copies through LimitReader(SizeBytes+1), and panics with http.ErrAbortHandler on error or size mismatch. Because the copy only completes once the exec reaches EOF, a non-zero exit or stderr output after the last byte still aborts the response. "attachment not found" errors still surface before headers are written and keep mapping to 404.

Suggestions (optional)

• [P3] test/e2e/session_test.go:1329 — loginSessionWeb builds the web client with Timeout: 30 * time.Second, and the new round-trip at lines 222–224 pushes 16 MiB (16 B × 1 Mi, not the 15 MiB stated in the PR description) each way through kubectl port-forward plus the API server exec tunnel under that same per-request budget. It should comfortably fit on a healthy kind cluster, but a slow CI runner would surface as a client-side timeout rather than a product failure. Consider a longer timeout for the two large-transfer calls so a flake there is not mistaken for a regression.

Key takeaways

• The transport gaps flagged last round are closed: uploads get a per-handler 15-minute read deadline and downloads stream with bounded memory, so the Helm chart's default 256Mi console-server limit is no longer at risk from a couple of large image previews.
• Unit coverage is thorough for the new seams (streaming before exec completion, cancellation, invalid metadata, exec errors before and after metadata, and short/long/failed transfers in the handler), and the e2e test now exercises a multi-MiB transfer end to end.
• The store-test I/O suggestion from the previous review was explicitly deferred by the author; it is not re-raised here.

[gjkim42]

Verified the review against head c88a58d. Both required findings applied:

• Upload deadline: attachment uploads now set a 15-minute read deadline. The existing deadline for other request bodies stays in place. The upload test verifies the deadline and successful transfer.
• Download memory: downloads now stream through an io.Pipe into the HTTP response. Metadata is checked before response headers are sent, Content-Length comes from the validated size, and failed or incorrectly sized transfers abort. Closing the response stream cancels exec and closes the pipe. Tests cover streaming, cancellation, metadata errors, and transfer failures.

The optional transport-coverage suggestion is also addressed with a 15 MiB e2e upload/download round-trip. The optional store-test I/O optimization is deferred: there is no measured CI problem, and making store limits configurable solely to reduce test I/O would expand this change unnecessarily.

Local validation passed: make update, env -u CODEX_HOME -u CODEX_AUTH_JSON make test, and make verify. E2E validation runs in PR CI.

[gjkim42]

/kelos claude-review

[gjkim42]

Reviewed the latest Kelos review (updated 2026-09-07 13:57 UTC) against head 28298ff. It approves the changes and reports no blocking findings; both earlier P1 findings are resolved. There are no unresolved inline review threads.

The remaining P3 suggestion concerns the e2e client's 30-second timeout. This is optional, and the current e2e run passed without a timeout, so no harness or product change is required on the available evidence. The fixture is 16 MiB; the PR description has been corrected.

Squashed the two commits into one, preserving the exact file tree from 28298ff. Build, verification, unit, integration, and e2e checks passed for that tree. CI will run again for the squashed commit.