Skip to content

feat(api): restrict stateful APIs for migration - #943

Open
hanakannzashi wants to merge 31 commits into
mainfrom
codex/issue-934-stateless-surfaces-main
Open

feat(api): restrict stateful APIs for migration#943
hanakannzashi wants to merge 31 commits into
mainfrom
codex/issue-934-stateless-surfaces-main

Conversation

@hanakannzashi

@hanakannzashi hanakannzashi commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR restricts the legacy confidential-data APIs to an explicit Stage I bounded surface. It preserves six existing retrieval views and two resource DELETE operations needed by the established Chat account-deletion workflow; every other legacy Conversation/File mutation is retired. It also makes Responses stateless and single-inference.

Exact Stage I API matrix

Unless noted otherwise, /v1/responses, Conversation, and File routes use the existing Bearer API-key/workspace context. Their route groups apply Cache-Control: no-store, including authenticated 410 responses.

Responses

Endpoint or request feature Stage I behavior Boundary
POST /v1/responses Retained, stateless Omitted store and store: false are accepted; store: true is 400. Each successful request performs exactly one internal Chat Completions completion-stream call. No responses or response_items database rows are created, read, or updated.
conversation, previous_response_id, background: true 400 Server-retained response/conversation state is unavailable. Omitted background and background: false normalize to false.
Message input Retained Clients provide their own prior context. Plain text, input_text, assistant-message output_text, and input_image are accepted; input_file is 400.
Custom type: "function" Retained, client-managed Cloud may return function_call, but never executes or retries it. The caller replays a non-empty function_call and its matching function_call_output in a new request; thought_signature is preserved. A function merely named web_search remains a client function.
MCP continuation input 400 mcp_approval_response and mcp_list_tools input items are unsupported.
Builtin/remote tool types 400 web_search, web_context_search, file_search, code_interpreter, computer, and remote mcp are rejected before provider work.
Image-output model through Responses 400 Use direct image APIs instead. This includes aliases resolving to image-output models.
Any method on /v1/responses/{id}, /v1/responses/{id}/cancel, or /v1/responses/{id}/input_items Authenticated 410 Gone These named response-history paths are retired. Other unregistered /v1/responses/** paths retain normal router 404/405 behavior.
GET /v1/signature/{resp_id} Retained, unchanged Best-effort gateway attestation stores response ID plus signed request/response digest material, never raw response/history content. Interrupted streams create no partial resp_* record.

Conversations

Method + path Stage I behavior Notes
POST /v1/conversations/batch Retained semantic read Caller supplies 1–1000 IDs. Workspace-owned matches are returned in request order; missing or foreign IDs are returned in missing_ids. This is not a Conversation-list API.
GET /v1/conversations/{id} Retained read Existing API-key and workspace boundary.
GET /v1/conversations/{id}/items Retained read Existing API-key and workspace boundary.
DELETE /v1/conversations/{id} Retained normal-delete exception Existing public API-key/workspace-scoped resource delete, retained for Chat account deletion; not a Chat-only credential or route.
Every other Conversation route/method Authenticated 410 Gone Includes list/create/update/item creation/pin/archive/clone, wrong methods, trailing-slash variants, and unlisted descendants.

Files

Method + path Stage I behavior Notes
GET /v1/files Retained read Existing API-key and workspace boundary.
GET /v1/files/{id} Retained read Existing API-key and workspace boundary.
GET /v1/files/{id}/content Retained read Existing API-key and workspace boundary.
DELETE /v1/files/{id} Retained normal-delete exception Existing public API-key/workspace-scoped resource delete, retained for Chat account deletion.
Every other File route/method Authenticated 410 Gone Includes upload, non-delete mutations, wrong methods, trailing-slash variants, and unlisted descendants; disabled paths do not reach storage/repository mutation code.

The six temporary retrieval views and two resource DELETE operations are the complete retained Conversation/File surface. No new Cloud export endpoint or GET /v1/conversations list endpoint is introduced. The DELETE exception is a product/workflow exception, not an authorization distinction: any caller already holding an authorized workspace API key can invoke it.

Separate surfaces

Method + path Stage I behavior Notes
POST /mcp Retained, independent Root route, not /v1/mcp; its MCP server exposes only web_search and is never invoked by Responses.
POST /v1/images/generations, POST /v1/images/edits Retained, unchanged Direct image inference; not Responses builtin tools or File API writes.
POST /v1/chat/completions Retained, unchanged Public direct inference route. Stateless Responses uses the same internal completion service once; it does not issue an HTTP request to this route.

Rollout boundary

The retained DELETE routes allow Chat's existing asynchronous DELETE /v1/users/me worker to clean up Cloud resources before it performs local finalization. This PR deliberately does not introduce a trusted Chat-only credential or internal deletion route; Cloud cannot distinguish a worker request from another authorized workspace caller.

The retrieval views are retired in Stage III after the migration/export window. Retiring the two DELETE exceptions remains tied to the account-deletion lifecycle and is outside this PR.

Validation

  • cargo fmt --all -- --check
  • git diff --check
  • service, API/OpenAPI, retained-view/delete, mutation-410, API-key, and Cache-Control: no-store coverage
  • strict Clippy for API, services, and inference providers
  • complete GitHub CI passed on the current feat(api): restrict stateful APIs for migration #943 head
  • fresh isolated Docker PostgreSQL: cargo test -p api --test e2e_all -- --test-threads=16 — 609 passed, 0 failed, 9 ignored

Part of #934 and #953.

Closes #936
Closes #937
Closes #939
Closes #941

@ironloopai

ironloopai Bot commented Aug 19, 2026

Copy link
Copy Markdown

🧭 IronLoop Run · Review

This comment updates in place as the Run moves through its stages.

🟩 Final result · Completed

🟨 Queued🟦 Working🟦 Posting results🟩 Completed

Automatic trigger · attempt 1 of 3 · completed in 1m 2s

IronLoop completed the review and posted it to GitHub.

🔗 Result

Open submitted review →

Run details

Run: 2cbb6feb-1180-4639-a6e8-416118ff4dec
Base: main at 95b3c8c
Head: codex/issue-934-stateless-surfaces-main at 0b318ec
Created: 2026-08-19 05:04 UTC
Updated: 2026-08-19 05:05 UTC

@ironloopai ironloopai 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.

🔍 IronLoop review

🟢 No actionable findings

No actionable defects found in the complete change. The retired APIs remain authenticated, stateless response validation is enforced at both HTTP and service boundaries, and request content is confined to request-scoped repositories.

Validation

  • Changed-surface inspection — Reviewed all changed production code, affected call paths, replacement tests, and captured review feedback; no prior feedback or threads required deduplication.
  • Captured CI — The captured Test Suite, Build release, Lint, Integration tests, E2E tests, Unit tests, cargo audit, and cargo deny checks all passed.
Review details
  • Run: 2cbb6feb-1180-4639-a6e8-416118ff4dec
  • Workflow: Review
  • Attempts: 1

@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review — feat(api): retire stateful API surfaces

Building on the earlier IronLoop pass (no actionable findings). The mechanics of the retirement are solid: the transient repositories faithfully mirror the DB repo semantics (including the odd default web_search tool), dropping the SSE accumulation Mutex<Vec<u8>> is a real memory/latency win on long streams, and setting usage.response_id = None correctly avoids a dangling FK now that no responses row exists. Findings below are mostly undocumented consequences rather than defects in the new code.

1. Response attestation/verifiability is silently removed — not mentioned anywhere user-facing

create_response no longer calls store_response_signature, which leaves zero production callers of that trait method, and service.rs:1474 sets response_id: None on the usage record. Net effect for every /v1/responses call:

  • GET /v1/signature/resp_* returns SignatureNotFound (404) forever. get_chat_signature_impl only finds gateway signatures written by store_response_signature for resp_ ids.
  • The disconnect fallback in check_fallback_conditions (get_stop_reason_by_response_id, chat_signatures.rs:227) can never fire either, since usage rows no longer carry the response id — so clients do not even get the STREAM_DISCONNECTED signal.
  • skip_provider_chat_signature: false is still set (service.rs:1475), so a provider signature round-trip is still performed and stored under the provider chat id — which is never surfaced to the caller. Verification data is produced, paid for, and unreachable.

Meanwhile /v1/signature/{chat_id} is still advertised in OpenAPI (and asserted by test_openapi_signature_requires_api_key), and docs/local-development.md still lists it. For a TEE product where verifiability is a headline feature, this deserves an explicit decision rather than being a side effect:

  • Signature rows are (request_hash, response_hash) — hashes, not customer content — so keeping them does not conflict with store: false / no-store. Simplest fix is to keep storing them.
  • If the decision is to drop them, set skip_provider_chat_signature: true for responses (stop paying for the fetch) and return a distinguishable SIGNATURE_UNSUPPORTED-style result for resp_ chat ids instead of a bare 404, plus update the docs and PR summary.

2. Function calling now has no working path at all

validate_stateless rejects both ResponseTool::Function and ResponseInputItem::FunctionCallOutput. Because ResponseInputItem has no FunctionCall variant, a client cannot replay the assistant tool call the way the upstream store: false flow does — so here statelessness means function/tool calling is gone entirely, not just server-side continuation. Defensible as a scope call, but it should be stated as such in the PR body/docs with a tracked follow-up (add a FunctionCall input item so the stateless loop is expressible), rather than reading as "continuation removed".

3. require_approval granular form is over-rejected

ResponseTool::Mcp { require_approval, .. } if !matches!(
    require_approval, McpApprovalRequirement::Simple(McpApprovalMode::Never)) => Err(...)

McpApprovalRequirement also has Granular { never: McpToolNameFilter }, and requires_approval() returns false for every tool listed there — such a config never emits an approval request and is perfectly stateless, yet it 400s. That is the form the upstream MCP docs use. Consider accepting Granular when allowed_tools is a subset of never.tool_names (or at least say in the error message that only "require_approval": "never" is accepted). Separately: the serde default is Always, so any MCP tool that omits require_approval now hard-fails — worth calling out in migration notes.

4. store: true is a hard 400, and it used to be the default

The route previously did request.store = request.store.or(Some(true)). Clients that explicitly send store: true (common with OpenAI-compatible SDKs and agent frameworks) now break with no gradual path, and during a rolling deploy they will see 200/400 flapping across pods. If intended, fine — but give the error a machine-readable code so callers can detect it, and mention it in the changelog.

5. Inconsistent retirement envelopes (minor)

  • conversations::conversation_api_gone sets code: "conversation_api_retired"; files_api_deprecated and response_history_gone set no code. Clients doing programmatic migration detection get three different shapes for the same event. Suggest files_api_retired / response_history_retired.
  • Only the responses 410 carries Cache-Control: no-store; 410 is heuristically cacheable per RFC 9111, so the conversations/files 410s may be cached by intermediaries. Harmless given they are permanent, but worth being deliberate.

6. with_no_store_cache_header is now dead weight (minor)

build_response_routes applies map_response(no_store_response) to the whole router, and create_response is mounted nowhere else — so the eight per-branch with_no_store_cache_header(...) wrappings in responses.rs are redundant and add noise to every error path. Keep the layer, drop the wrappers (or vice versa), not both.

7. CLAUDE.md not updated (doc drift)

CLAUDE.md still documents the Conversations and Files APIs, the conversations/files service modules, and the function-call resumption flow (previous_response_id + FunctionCallOutput) as live behavior. Since it is the agent-facing spec for this repo, leaving it stale will actively steer future changes toward re-adding retired behavior. Please update it alongside README.md / docs/local-development.md.

Nothing here blocks on correctness of the new code — item 1 is the one I would want resolved (or explicitly signed off) before merge, since it changes a security/trust guarantee that is still advertised in the API surface.

⚠️

@hanakannzashi

Copy link
Copy Markdown
Contributor Author

@ironloopai review

@ironloopai

ironloopai Bot commented Aug 20, 2026

Copy link
Copy Markdown

🧭 IronLoop Run · Review

This comment updates in place as the Run moves through its stages.

🟩 Final result · Completed

🟨 Queued🟦 Working🟦 Posting results🟩 Completed

Manual command by hanakannzashi · attempt 1 of 3 · completed in 52s

IronLoop completed the review and posted it to GitHub.

🔗 Result

Open submitted review →

Run details

Run: 2d574e9e-b969-4a7e-a801-7e067bdee0d9
Base: main at d340152
Head: codex/issue-934-stateless-surfaces-main at 3e8e606
Created: 2026-08-20 02:41 UTC
Updated: 2026-08-20 02:42 UTC

@ironloopai ironloopai 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.

🔍 IronLoop review

🟢 No actionable findings

The change consistently retires stateful API surfaces, enforces request-scoped Responses behavior, and preserves completed-response attestation without retaining raw content. No new actionable findings remain after accounting for the captured review discussion.

Validation

  • Captured CI — Build, unit, integration, E2E, lint, cargo audit, and cargo deny checks all passed.
  • Static review — Inspected the complete production-code change, affected routing, stateless validation, transient repositories, attestation handling, tests, and existing review feedback.
Review details
  • Run: 2d574e9e-b969-4a7e-a801-7e067bdee0d9
  • Workflow: Review
  • Attempts: 1

@hanakannzashi hanakannzashi changed the title feat(api): retire stateful API surfaces feat(api): make stateful APIs read-only for migration Aug 21, 2026
@hanakannzashi

hanakannzashi commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Stage I scope clarification

The current PR implements the Stage I boundary from #953, not a full Conversation/File API removal.

It retains the six pre-existing, authenticated workspace-scoped Conversation/File view endpoints listed in the PR body, while mutations and unlisted descendants return 410 Gone with Cache-Control: no-store. No export/list API was added. Responses remains stateless and history endpoints remain retired.

Earlier review comments in this thread describe an all-410 version of the branch; they are historical context and are superseded by the current implementation and PR description. Final API removal is tracked as Stage III work.

@think-in-universe think-in-universe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the latest patch and found one issue in the stateless function replay path.

Comment thread crates/api/src/routes/attestation/signature.rs
Comment thread crates/api/src/routes/conversations.rs Outdated
Comment thread crates/api/src/models.rs
Comment thread crates/services/src/responses/models.rs
@hanakannzashi hanakannzashi changed the title feat(api): make stateful APIs read-only for migration feat(api): restrict stateful APIs for migration Aug 21, 2026
@hanakannzashi

Copy link
Copy Markdown
Contributor Author

Superseding Stage I scope clarification: retained resource DELETE

The chosen Stage I boundary retains DELETE /v1/conversations/{conversation_id} and DELETE /v1/files/{file_id} for Chat's existing asynchronous account-deletion worker. They are the only Conversation/File mutation exception; all other mutations and unlisted descendants remain authenticated 410 Gone with Cache-Control: no-store.

These remain the existing API-key/workspace-scoped public endpoints, not a new trusted Chat-only route or credential. Earlier comments that characterize every Conversation/File mutation as retired are historical and superseded by the current PR description and this clarification.

Comment thread crates/services/src/responses/service_helpers.rs
@think-in-universe

Copy link
Copy Markdown
Contributor

@ironloopai review

@ironloopai

ironloopai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review · Status

🟩 Completed

IronLoop completed the review and posted it to GitHub.

Result

Open submitted review →

Run details
  • Run: 841ce0c7-585d-42f6-be53-e3f3ede9f597
  • Base: main at 54b4a7c
  • Head: codex/issue-934-stateless-surfaces-main at 59a96a8
  • Created: 2026-08-21 14:20 UTC
  • Updated: 2026-08-21 14:47 UTC

Manual command by think-in-universe · attempt 1 of 3 · completed in 27m 17s

@think-in-universe

Copy link
Copy Markdown
Contributor

@claude review

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review — Stage I bounded surface (feat(api): restrict stateful APIs for migration)

Re-reviewed against the current head, not the earlier all-410 branch. Acknowledging prior threads: the two scope clarifications from @hanakannzashi supersede my 2026-08-19 comment, and most of that comment is now resolved on this head — attestation is back (store_response_signature on both streaming and non-streaming paths, with a bounded timeout), function calling has a real client-managed replay path (ResponseInputItem::FunctionCall), the duplicated with_no_store_cache_header wrappers are gone in favour of one map_response layer, and CLAUDE.md is updated. The MCP require_approval point is moot now that mcp tools are rejected outright. @think-in-universe also flagged something in the stateless function replay path — if that inline thread is the item below, treat this as a +1 with a concrete repro; CI is green, so it is not covered by tests.

The route-layout work is solid: build_stage_one_*_route_layout is exercised by a real Axum test, the 410s sit inside auth, and no_store_response is applied outside the auth layer so rejects are covered too. transient.rs faithfully mirrors the repository semantics without touching the DB.


⚠️ 1. Replayed function-call transcripts can be rebuilt into a message order providers reject

crates/services/src/responses/service.rsappend_replayed_function_call_item / flush_replayed_function_calls

Pending function_call items are flushed into an assistant message at the first following function_call_output or Message. validate_stateless accepts orderings that this rebuild turns into a transcript violating the rule that tool messages must immediately follow the assistant message carrying their tool_calls — and two of the new unit tests in models.rs assert exactly those orderings are valid:

Case A — interleaved_calls_and_outputs (asserted is_ok()):

input:  fc(1), fc(2), out(1), fc(3), out(2), out(3)
built:  assistant[tc1,tc2] , tool(1) , assistant[tc3] , tool(2) , tool(3)

assistant[tc1,tc2] is followed by only one tool message before a new assistant message, and tool(2) answers a call declared two messages earlier. OpenAI-compatible backends return 400 (an assistant message with tool_calls must be followed by tool messages responding to each tool_call_id).

Case B — message_between_call_and_output (asserted is_ok()):

input:  fc(1), message(user,"continue"), out(1)
built:  assistant[tc1] , user("continue") , tool(1)

Same failure — the Message arm flushes pending calls before pushing the user turn.

Net effect: a request Cloud validates as OK fails at the provider and surfaces as response.failed / ResponseError::Completion, rather than a clear 400 invalid_request_error from our own validator. MockProvider does not validate transcript ordering, so E2E stays green.

Two ways out — either is fine, but pick one:

  • Tighten validation: require each function_call run to be immediately followed by its matching function_call_output items (no intervening Message, no reordering). Cheap, and it matches what the docs already tell clients to send.
  • Group on rebuild: buffer function_call_output items keyed by call_id, then emit assistant[all pending tool_calls] followed by all of its outputs in declaration order, instead of flushing on the first output.

⚠️ 2. Canonical single-call replay splits the assistant turn in two

Same code path, the shape your own stateless_function_call_replay_accepts_returned_output_without_server_state test builds:

input:  user , assistant("I will look that up.") , fc(1) , out(1)
built:  user , assistant("I will look that up.") , assistant("", tool_calls=[1]) , tool(1)

The model originally produced one assistant turn with both text and tool_calls; the replay reconstructs two consecutive assistant messages, one with empty content. OpenAI tolerates this, but strict-alternation adapters (Anthropic/Gemini-shaped backends) do not, and it changes the prompt the model sees on the second turn. Worth merging an assistant Message with the function_call items that immediately follow it.


Notes (non-blocking)

  • Final SSE frame is now gated on a DB write. signed_response_sse_stream uses stream.then(..) and awaits persist_response_attestation before yielding the response.completed frame (previously tokio::spawn, fire-and-forget). Correct for making GET /v1/signature/resp_* immediately readable, but a stalled chat_signatures write now adds up to RESPONSE_ATTESTATION_STORE_TIMEOUT (5s) to the tail of every streaming response and holds the connection open. Reasonable trade — just confirm 5s is the number you want under DB pressure.
  • STREAM_DISCONNECTED for resp_* is now permanently unreachable. CompletionRequest.response_id is None (unavoidable — organization_usage_log.response_id has an FK to responses(id) and no row is created), so the resp_ branch of check_fallback_conditions at crates/services/src/attestation/chat_signatures.rs:215-235 can never fire; disconnected Responses streams get a bare 404 instead of the STREAM_DISCONNECTED signal. You documented this in the client_disconnect.rs module docstring — consider also deleting the now-dead branch so it does not read as live behaviour.
  • Retirement envelopes are still inconsistent. conversation_write_disabled sets code: "conversation_write_disabled", while files_write_disabled and response_history_gone set no code. Clients doing programmatic migration detection get two different shapes for the same event; suggest adding matching codes for symmetry.
  • routes/files.rs::upload_file and MAX_FILE_SIZE are now unrouted and unreferenced (still pub, so no dead-code warning). Fine if Stage III removes them, but the handler keeps a utoipa::path attribute for POST /v1/files, a path that no longer appears in the spec — a trap for the next reader.

Items 1 and 2 are the ones I would want resolved before merge: both sit in the one genuinely new behaviour this PR ships (client-managed function replay), and both are currently locked in by passing unit tests.

⚠️

@ironloopai ironloopai 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.

Review · Summary

Found one issue in the new image-model restriction.

Findings: 🟠 Medium 1

Code-specific findings are attached to the diff.

Validation
  • Static review — Reviewed the stateless request boundary, route restrictions, function replay, attestation flow, and cache controls; the change has no whitespace errors.
Review details
  • Run: 841ce0c7-585d-42f6-be53-e3f3ede9f597
  • Attempts: 1

Comment thread crates/services/src/responses/service.rs
@hanakannzashi

Copy link
Copy Markdown
Contributor Author

Follow-up on this review: item 2 is fixed in a3cb82f. A replayed assistant output_text immediately followed by its function_call items is now reconstructed as one provider assistant turn, with the tool result following it; the regression E2E now asserts that shape. We intentionally did not take item 1 in this migration: the agreed contract is to preserve the existing acceptance constraints rather than add a new contiguity rule or normalize/reorder caller-supplied interleavings. Cloud maps the supplied items into one provider request, without an agent-loop fallback; a future cross-provider transcript-normalization contract should be scoped separately. The image-alias review finding is also fixed in the same commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants