Skip to content

refactor(responses): remove dormant stateful runtime dependencies - #944

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

refactor(responses): remove dormant stateful runtime dependencies#944
hanakannzashi wants to merge 2 commits into
codex/issue-934-stateless-surfaces-mainfrom
codex/issue-940-cleanup-main

Conversation

@hanakannzashi

@hanakannzashi hanakannzashi commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • remove only the stateless Responses service's obsolete persistent-history dependencies and helpers
  • keep the complete temporary Stage I Conversation/File surface introduced by feat(api): restrict stateful APIs for migration #943: six authenticated views plus the two resource DELETE compatibility exceptions
  • retain request-scoped transient response/item storage, client-managed function replay, attestation, and the standalone POST /mcp web-search endpoint

API contract preserved by this cleanup

This PR makes no new Stage I HTTP contract change; #943 remains the authoritative endpoint-level API matrix.

API surface Stage I result after #944 #944 effect
POST /v1/responses Retained; stateless, one internal Chat Completions inference, client-managed functions only Removes unreachable persistent-history/agent dependencies while retaining request-scoped transient items.
Named Responses history routes Authenticated 410 Gone Removes their unreachable Responses runtime code.
POST /v1/conversations/batch; GET /v1/conversations/{id}; GET /v1/conversations/{id}/items Retained, API-key/workspace-scoped, no-store No route change; retains required service wiring.
DELETE /v1/conversations/{id} Retained normal-delete exception Preserves Chat account-deletion compatibility.
GET /v1/files; GET /v1/files/{id}; GET /v1/files/{id}/content Retained, API-key/workspace-scoped, no-store No route change; retains FileService/S3 wiring.
DELETE /v1/files/{id} Retained normal-delete exception Preserves Chat account-deletion compatibility.
Other Conversation/File paths Authenticated 410 Gone, no-store No route change.
POST /mcp, direct Chat Completions, and Images Retained independently / unchanged Outside this Responses-only cleanup.

Dependency and merge procedure

Blocked by #943. This PR is ready for review, but it is not ready to merge while it is stacked on #943.

After #943 has merged to main:

  1. rebase this branch onto the resulting main;
  2. retarget this PR to main;
  3. rerun the complete required CI, including the normal test suite, lint, integration/E2E coverage, and security checks; and
  4. update the validation/head reference, then merge this PR.

Scope boundary

This is not the final removal of Conversations, Files, or S3. The cleanup removes stale Responses-only construction of persistent response/history repositories, ConversationService, FileService/S3, and provider-pool dependencies, plus unreachable history, file-input, and citation code. It does not remove the actual DomainServices/AppState Conversation/File/S3 wiring used by the temporary views or retained DELETE routes.

No schema, migration, retained row, S3 object, or S3 configuration cleanup is included. #947 is deferred until Files views and any archive/recovery need have been retired.

Validation on this stacked review head

  • cargo fmt --all -- --check
  • git diff --check
  • cargo check -p api -p services
  • cargo test -p services responses::service::tests --lib (38 passed)
  • cargo clippy -p api -p services --all-targets -- -D warnings
  • fresh isolated Docker PostgreSQL: cargo test -p api --test e2e_all -- --test-threads=16 — 609 passed, 0 failed, 9 ignored

The post-#943-main restack still requires its own complete current-head CI before merge.

Closes #940
Part of #934 and #953

@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 26s

IronLoop completed the review and posted it to GitHub.

🔗 Result

Open submitted review →

Run details

Run: 2ac220c4-8b78-48cd-836b-d2bcf8c39dba
Base: codex/issue-934-stateless-surfaces-main at 0b318ec
Head: codex/issue-940-cleanup-main at a20994c
Created: 2026-08-19 05:15 UTC
Updated: 2026-08-19 05:17 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. The refactor consistently retires stateful Conversations, Files, and response-history paths while preserving authenticated retirement responses and request-scoped stateless response execution.

Validation

  • Static review — Inspected all changed files, surrounding route wiring, stateless validation, response processing, and transient repository behavior.
  • Diff integrity — The complete change contains no whitespace or patch-format errors.
Review details
  • Run: 2ac220c4-8b78-48cd-836b-d2bcf8c39dba
  • Workflow: Review
  • Attempts: 1

@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Reviewed the removal for completeness and for behavior parity on the retained stateless path.

Mechanically clean. No dangling references remain to conversation_service, files_service, file_search_provider, response_repository, response_items_repository, or inference_provider_pool on ResponseServiceImpl; all three init_domain_services_* constructors were updated; build_files_routes no longer needs AppState; aws-* stays in crates/services where S3Storage still lives, so the crates/api drop is safe. build_stateless_messages faithfully reproduces the stateless branch of the old load_conversation_context (org prompt, then instructions + language + time system message, then input items), and the removed filter_to_ancestor_branch / title-generation tests only covered deleted code.


1. Client-supplied assistant history is echoed back in response.output

crates/services/src/responses/service.rs:1105

store_input_as_response_items writes every input Message item into the transient store using the client-supplied role (service.rs:1816), and the final output filter keeps every message whose role is assistant:

.filter(|item| match item {
    models::ResponseOutputItem::Message { role, .. } => role == "assistant",
    models::ResponseOutputItem::FunctionCallOutput { .. } => false,
    _ => true,
})

role is a free-form String and validate_stateless does not constrain it. Because the stateless API requires clients to resend prior turns in input, every multi-turn request now emits the caller's own assistant turns in output, ahead of the newly generated message.

Repro: POST /v1/responses with

"input": [
  {"role": "user", "content": "a"},
  {"role": "assistant", "content": "prior answer"},
  {"role": "user", "content": "b"}
]

produces a response.completed whose output contains two assistant messages ("prior answer" plus the real one).

This predates the PR (it came in with #943), but this PR removes the last non-stateless path, so resending history is now the only multi-turn mechanism and the defect sits squarely on the main path.

The cheapest fix is also in the spirit of this PR: list_by_response (service.rs:1096) is the only live reader of the item store. ResponseItemRepositoryTrait::get_by_id is reached only from tools/mcp.rs:900 (process_approval_responses, unreachable now that validate_stateless rejects mcp_approval_response), and list_by_api_key / list_by_conversation have no callers at all. So store_input_as_response_items no longer serves any purpose and can be dropped entirely, which removes the echo as a side effect. If you prefer to keep it, restrict the filter to items created during this turn rather than by role.


2. config.s3 is now unused but still hard-fails startup

crates/config/src/types.rs:959 - S3Config::from_env() still returns Err (so, boot failure) unless AWS_S3_BUCKET, AWS_S3_REGION, and S3_ENCRYPTION_KEY_FILE/S3_ENCRYPTION_KEY are set, yet after this PR nothing in crates/api reads config.s3. Not a break by itself, but it is a trap for the obvious follow-up: anyone who removes the now-dead S3 env vars from a deployment manifest gets a crash-loop on the next rollout. Either make S3Config optional here, or add an explicit note to the follow-up issue.


3. Misleading comment on MAX_FILE_SIZE

crates/api/src/routes/files.rs:4-7 says the 512 MB constant is "retained for the audio-transcription body limit". It is not: audio transcription is covered by AUDIO_TRANSCRIPTION_MAX_BODY_SIZE (25 MB) at crates/api/src/lib.rs:1519, while MAX_FILE_SIZE is the body limit for POST /v1/images/edits (lib.rs:1555). Worth correcting, since the comment understates a 512 MB cap by 20x - and consider moving the constant out of the now-retired routes::files module.


4. Error detail dropped from image-op logs

crates/services/src/responses/service.rs:2183 and :2206 changed tracing::error!(error = %e, ...) to tracing::error!(model = %..., ...), discarding the provider error entirely. These are upstream inference-provider errors, not customer content, so the privacy rules in CLAUDE.md do not require dropping them - and without them a provider-side image failure is undiagnosable from logs. Same applies to service.rs:719, which lost the {e} from the org-system-prompt warning. Also unrelated to the stated scope of the PR.


Items 2-4 are non-blocking. Item 1 is the one I would fix before merge.

⚠️

@hanakannzashi

Copy link
Copy Markdown
Contributor Author

Rebased this cleanup branch onto the updated #943 head.

Addressed the two scoped review items here:

  • corrected MAX_FILE_SIZE documentation: it is the active Image Edits 512 MB upload limit, not audio transcription;
  • restored the organization system-prompt fetch warning with its error detail.

No schema, migration, data, or retention changes are included.

@hanakannzashi
hanakannzashi force-pushed the codex/issue-934-stateless-surfaces-main branch from b5ab148 to f20cbbe Compare August 21, 2026 05:12
@hanakannzashi
hanakannzashi force-pushed the codex/issue-940-cleanup-main branch from 1be1e87 to 76b6c1b Compare August 21, 2026 09:03
@hanakannzashi hanakannzashi changed the title refactor(api): remove dormant stateful API wiring refactor(responses): remove dormant stateful runtime dependencies Aug 21, 2026
@hanakannzashi
hanakannzashi force-pushed the codex/issue-940-cleanup-main branch 2 times, most recently from 4c662a9 to 55d899f Compare August 21, 2026 09:23
@hanakannzashi

hanakannzashi commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Stage I scope clarification

This stacked cleanup deliberately does not remove Conversation/File routes, services, S3 wiring, schemas, or retained data. Those remain required for the temporary read-only migration views introduced by #943.

Its scope is limited to dormant stateful dependencies inside the Responses runtime. Final Conversation/File and S3 removal belongs to Stage III, after the migration/archive path is complete.

@hanakannzashi
hanakannzashi marked this pull request as draft August 21, 2026 13:55
@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: ff709a0a-ea89-4cb4-9ffe-d6587254012f
  • Base: codex/issue-934-stateless-surfaces-main at 59a96a8
  • Head: codex/issue-940-cleanup-main at 55d899f
  • Created: 2026-08-21 14:23 UTC
  • Updated: 2026-08-21 14:49 UTC

Manual command by think-in-universe · attempt 1 of 3 · completed in 26m 25s

@hanakannzashi
hanakannzashi marked this pull request as ready for review August 21, 2026 14:26
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

Re-reviewed at head 55d899f (previous pass was against a20994c). This is a clean mechanical removal — no blocking issues.

Previously raised, now resolved

  • Assistant-history echo in response.output — fixed on the base branch: store_input_as_response_items now returns input_item_ids and select_output_items (service.rs:1229) excludes them by ID rather than relying on role. The historical_assistant_input_is_not_returned_as_response_output test covers it.
  • MAX_FILE_SIZE comment — the misleading audio-transcription note is gone (routes/files.rs:16).
  • Dropped error detail in the org-system-prompt warning — restored (service.rs:1329).
  • config.s3 unused — no longer applies; lib.rs:449-478 still constructs s3_storage/FileService for the Stage I views.

Verified for this diff

  • No dangling references to citation_tracker, CitationTracker, SourceRegistry, emit_citation_annotation, filter_to_ancestor_branch, process_input_file, or extract_multimodal_content anywhere in crates/.
  • ResponseService::new has exactly one call site (lib.rs:477), updated.
  • build_stateless_messages is a faithful extraction of the old stateless branch: org prompt, then instructions + language + time system message, then input items, with append_replayed_function_call_item / flush_replayed_function_calls ordering unchanged. The only substitution is extract_content_parts -> extract_stateless_content_parts.
  • Dropping the citation tracker actually removes a latent inconsistency: emit_message_completed previously used tracker.finalize(), which flushed a trailing partial-tag buffer that was never emitted as a delta. It now uses current_text, so output_text.done and the sum of the deltas always agree.
  • Every removed path is already rejected upstream by validate_stateless (models.rs:1303): conversation, previous_response_id, input_file, mcp input items, and all server-executed tools.

Non-blocking

1. store_input_as_response_items is now provably dead weightservice.rs:1057

list_by_response (service.rs:732) is the only reader of the transient item store, and select_output_items filters out exactly the IDs this function inserts. Every input item is written to an in-memory map and then unconditionally discarded: one clone plus an async write per input item, per request, for no observable effect. Deleting it (and the input_item_ids plumbing) fits this PR's stated scope and would shrink the diff further. Worth confirming attestation does not depend on the write before removing.

2. Redundant cloneslib.rs:614-621

database, organization_service, and inference_provider_pool are owned params cloned at their last use, now that the second ResponseService::new is gone:

let mut domain_services = init_domain_services_with_pool(
    database.clone(),                // -> database
    config,
    organization_service.clone(),    // -> organization_service
    inference_provider_pool.clone(), // -> inference_provider_pool
    metrics_service,
).await;

3. Injected test doubles are now silently ignoredlib.rs:565, lib.rs:610

_mcp_client_factory and _web_context_search_provider are accepted and dropped. Correct today (both surfaces are rejected), but any e2e test that injects a mock and asserts on it now passes vacuously. The doc comments say "temporarily" — worth an explicit follow-up to delete the helpers along with their call sites.

4. Two-pass extract_stateless_content_partsservice.rs:1419

The !has_images branch has an InputImage => {} arm that is unreachable by construction, and the input_file rejection is duplicated in both branches. A single pass that collects text/image parts and errors on InputFile once would be less fragile if the has_images predicate ever changes.

⚠️ (all findings non-blocking; nothing here should hold the merge once the branch is rebased onto main and full CI is green)

@github-actions github-actions 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.

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ 1 posted as inline comment(s)
  • 📝 0 posted as summary

Comment thread crates/api/src/lib.rs

@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

🟢 No actionable findings

No actionable issues found in the reviewed change.

Validation
  • Compilation — The API and services crates compile successfully.
  • Responses service tests — The focused Responses service test module passed all 38 tests.
Review details
  • Run: ff709a0a-ea89-4cb4-9ffe-d6587254012f
  • Attempts: 1

@hanakannzashi
hanakannzashi force-pushed the codex/issue-940-cleanup-main branch from 55d899f to 5ea72e8 Compare August 24, 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