Skip to content

fix(gateway): enforce OpenAI-compatible response contracts - #116354

Open
steipete wants to merge 1 commit into
mainfrom
codex/autoqa-gateway-api-20260730
Open

fix(gateway): enforce OpenAI-compatible response contracts#116354
steipete wants to merge 1 commit into
mainfrom
codex/autoqa-gateway-api-20260730

Conversation

@steipete

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes several OpenAI-compatible Gateway contract failures: streamed objects changed their creation time mid-response, Responses usage could omit real runtime usage or produce inconsistent totals, malformed embedding requests were silently coerced, and oversized/stalled JSON uploads reset the socket before the promised 413/408 error could reach the client.

Why This Change Was Made

Each logical response now owns one timestamp and one canonical usage projection, embeddings validate the upstream request contract before provider creation, and the bounded-body reader has an explicit response-owner mode that flushes the existing JSON error before closing the request. Existing direct body-reader and plugin callers retain their prior teardown behavior. This is an AI-assisted maintainer QA fix.

User Impact

OpenAI SDK clients receive stable stream identities, complete and internally consistent usage data, deterministic 400 errors for invalid embeddings input, and actual structured upload-limit/timeout responses instead of ECONNRESET.

Evidence

  • Installed openai-node@6.49.0 source/types were checked directly for Chat chunk timestamps, Responses lifecycle/usage details, and embeddings validation.
  • Blacksmith Testbox tbx_01kys7heeb752cfqwtapy2z6d9: 180/180 focused Gateway + real-socket infra tests passed.
  • After targeted formatting, the affected OpenAI suite passed 33/33 again.
  • Remote check:changed: passed core/coreTests formatting, tsgo, lint, dead-export, import-cycle, storage, webhook, pairing, and boundary guards.
  • Fresh Codex autoreview: no accepted/actionable findings; patch correct with confidence 0.90.
  • Deliberately does not overlap open disconnect-cancellation PRs fix(gateway): cancel requests disconnected during preparation #116257 or fix(gateway): abort embeddings when clients disconnect #116146.

@openclaw-barnacle openclaw-barnacle Bot added the gateway Gateway runtime label Jul 30, 2026
@steipete
steipete marked this pull request as ready for review July 30, 2026 10:22
@openclaw-barnacle openclaw-barnacle Bot added size: M maintainer Maintainer-authored PR labels Jul 30, 2026
@steipete steipete self-assigned this Jul 30, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 30, 2026
@clawsweeper

clawsweeper Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 1, 2026, 3:31 PM ET / 19:31 UTC.

ClawSweeper review

What this changes

This PR makes OpenAI-compatible Gateway endpoints keep stream timestamps stable, reject invalid embedding options before provider creation, preserve structured body-limit errors, and expand Responses usage output.

Merge readiness

Blocked until real behavior proof from a real setup is added - 6 items remain

Keep this protected, maintainer-owned Gateway PR open. Current main still lacks its stable chat-stream timestamp, embeddings option validation, and response-owning bounded-body behavior, but its separate Responses usage implementation was superseded by the merged canonical repair in #117533; the branch must rebase without restoring that duplicate logic and needs inspectable real HTTP proof before merge. Likely related people: steipete is the high-confidence routing candidate from the current merged Gateway usage repair and this branch’s history.

Priority: P2
Reviewed head: 0fd3c3bd83ddc5f2f756a4a71e7e303ce0b0c8d8

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The remaining fixes are source-supported, but the branch is dirty, retains a superseded parallel usage implementation, and lacks inspectable real behavior proof.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The body reports focused and real-socket test runs, but it provides no inspectable after-fix HTTP/SSE transcript, terminal capture, redacted runtime log, recording, or linked artifact showing the client-visible timestamp, validation, or 413/408 behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The body reports focused and real-socket test runs, but it provides no inspectable after-fix HTTP/SSE transcript, terminal capture, redacted runtime log, recording, or linked artifact showing the client-visible timestamp, validation, or 413/408 behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 8 items Current main still has timestamp drift: All chat-completion SSE writer helpers compute created independently with Date.now(), so chunks emitted across a second can carry different timestamps; the PR’s timestamp fix remains materially useful.
Current main still lacks the embeddings-option contract checks: The current embedding input validator covers text count and size, while the handler has no pre-provider validation for encoding_format or positive-integer dimensions; the PR adds those checks and tests.
Current main still destroys limited request bodies before the caller can flush JSON: readRequestBodyWithLimit destroys the request on declared-length and timeout failures, and readJsonBodyOrError currently invokes it without a response owner; this leaves the PR’s real-socket response-flush repair distinct from the merged usage work.
Findings 1 actionable finding [P1] Reuse the canonical Responses usage projection
Security None None.

How this fits together

OpenClaw’s Gateway accepts OpenAI-compatible HTTP requests, validates them, starts agent/provider work, and serializes JSON or SSE responses back to SDK clients. The changed code sits at that client contract boundary, including request-body lifecycle handling and token-usage projection.

flowchart LR
  Client[OpenAI-compatible client] --> Endpoint[Gateway HTTP endpoint]
  Endpoint --> Validate[Request and body validation]
  Validate --> Agent[Agent and provider execution]
  Agent --> Usage[Canonical usage projection]
  Usage --> Stream[JSON or SSE serializer]
  Stream --> Client
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The body reports focused and real-socket test runs, but it provides no inspectable after-fix HTTP/SSE transcript, terminal capture, redacted runtime log, recording, or linked artifact showing the client-visible timestamp, validation, or 413/408 behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Reuse the canonical Responses usage projection (P1) - The branch-local toUsage and usage-selection logic duplicates the current-main canonical projection merged in fix(gateway): preserve Responses usage details #117533. Because this PR is dirty, resolving its conflict could restore a parallel mapper and let the two Gateway paths drift; drop this block during rebase and keep the shared implementation. This is the previously reported P1, still present at the same head.
  • Resolve merge risk (P1) - GitHub reports this branch as dirty; resolving it must retain current main’s canonical Responses usage selector and wire mapper rather than reintroducing the branch-local duplicate.
  • Resolve merge risk (P1) - The remaining body-reader change changes when a client socket is destroyed around 413 and 408 responses, so it needs an after-fix real-socket transcript proving the client receives the promised structured error.
  • Resolve merge risk (P1) - The PR changes OpenAI-compatible wire behavior; SDK clients may rely on stable stream fields and consistent validation/error delivery, so the exact rebased behavior needs endpoint-level regression coverage.
  • Complete next step (P2) - The branch has a concrete P1 rebase correction and needs real behavior proof, but its maintainer label and dirty merge state require human-owned follow-through rather than an automated repair lane.

Findings

  • [P1] Reuse the canonical Responses usage projection — src/gateway/openresponses-http.ts:360-396
Agent review details

Security

None.

PR surface

Source +123, Tests +251. Total +374 across 13 files.

View PR surface stats
Area Files Added Removed Net
Source 7 178 55 +123
Tests 6 256 5 +251
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 13 434 60 +374

Review metrics

None.

Stored data model

Persistent data-model change detected: serialized state: src/gateway/openai-http.test.ts, serialized state: src/infra/http-body.test.ts, unknown-data-model-change: src/gateway/embeddings-http.test.ts, unknown-data-model-change: src/gateway/embeddings-http.ts, vector/embedding metadata: src/gateway/embeddings-http.ts. Confirm migration or upgrade compatibility proof before merge.

Merge-risk options

Maintainer options:

  1. Rebase around the canonical usage repair (recommended)
    Resolve the dirty merge by keeping the current shared Responses usage projection and retaining only the still-unique Gateway contract fixes, then rerun focused endpoint tests.
  2. Pause the combined branch
    If the remaining timestamp, embeddings, and body-flush repairs cannot be cleanly separated and re-proved, close this branch and land them as narrower current-main fixes.

Technical review

Best possible solution:

Rebase onto current main, discard the superseded Responses usage/schema/test changes in favor of the merged canonical implementation, retain only the independent timestamp, embeddings-validation, and response-flush repairs, then add redacted real HTTP/SSE evidence for each client-visible contract.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: a streamed chat response crossing a second can emit different created values, invalid embedding options are not currently rejected before provider creation, and body-limit handling destroys the request before the JSON error path owns the response. The exact live reproduction was not executed in this read-only review.

Is this the best way to solve the issue?

No, not as the branch stands: its Responses usage code duplicates a current-main canonical repair from #117533. A rebase that preserves that repair while retaining the three independent contract fixes is the narrower maintainable solution.

Full review comments:

  • [P1] Reuse the canonical Responses usage projection — src/gateway/openresponses-http.ts:360-396
    The branch-local toUsage and usage-selection logic duplicates the current-main canonical projection merged in fix(gateway): preserve Responses usage details #117533. Because this PR is dirty, resolving its conflict could restore a parallel mapper and let the two Gateway paths drift; drop this block during rebase and keep the shared implementation. This is the previously reported P1, still present at the same head.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 3b70c3535ff5.

Labels

Label justifications:

  • P2: The PR repairs OpenAI-compatible Gateway contract behavior with limited but real SDK-client blast radius.
  • merge-risk: 🚨 compatibility: The branch changes OpenAI-compatible SSE fields, embedding request validation, and Responses wire output, so a bad rebase could break existing client assumptions.
  • merge-risk: 🚨 availability: The bounded-body change controls request-socket teardown and can turn a structured 413 or 408 into a reset if lifecycle ordering is wrong.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The body reports focused and real-socket test runs, but it provides no inspectable after-fix HTTP/SSE transcript, terminal capture, redacted runtime log, recording, or linked artifact showing the client-visible timestamp, validation, or 413/408 behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Current main still has timestamp drift: All chat-completion SSE writer helpers compute created independently with Date.now(), so chunks emitted across a second can carry different timestamps; the PR’s timestamp fix remains materially useful. (src/gateway/openai-http.ts:270, 3b70c3535ff5)
  • Current main still lacks the embeddings-option contract checks: The current embedding input validator covers text count and size, while the handler has no pre-provider validation for encoding_format or positive-integer dimensions; the PR adds those checks and tests. (src/gateway/embeddings-http.ts:188, 3b70c3535ff5)
  • Current main still destroys limited request bodies before the caller can flush JSON: readRequestBodyWithLimit destroys the request on declared-length and timeout failures, and readJsonBodyOrError currently invokes it without a response owner; this leaves the PR’s real-socket response-flush repair distinct from the merged usage work. (src/infra/http-body.ts:332, 3b70c3535ff5)
  • Responses usage work is already merged on main: Current openresponses-http.ts delegates to resolveAgentRunUsage and toOpenAiResponsesUsage, the canonical shared usage path introduced by the merged repair; this supersedes the branch-local mapper for that portion of the PR. (src/gateway/openresponses-http.ts:346, fb6f60a704fc)
  • The branch retains the superseded parallel mapper: The PR head defines its own toUsage and selection logic rather than using the current shared canonical projection, matching the still-unresolved P1 finding from the prior review cycle. (src/gateway/openresponses-http.ts:360, 0fd3c3bd83dd)
  • Merged usage repair is main-only, not in a tagged release: The merged fix commit is contained by main; git tag --contains produced no release tag. The repository context identifies v2026.7.1 as the latest release, which predates the merged usage repair. (src/gateway/openresponses-http.ts:346, fb6f60a704fc)

Likely related people:

  • steipete: Authored both the PR head and the merged current-main canonical Responses usage repair that this branch must preserve during rebase. (role: recent Gateway contract contributor; confidence: high; commits: 0fd3c3bd83dd, fb6f60a704fc; files: src/gateway/openresponses-http.ts, src/agents/usage.ts, src/gateway/openai-http.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Rebase while retaining current main’s canonical Responses usage projection.
  • Add redacted after-fix HTTP/SSE and partial-upload terminal evidence showing stable timestamps, deterministic 400s, and received 413/408 JSON responses.
  • Refresh focused Gateway tests against the rebased head.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (6 earlier review cycles)
  • reviewed 2026-07-30T10:27:25.441Z sha 0fd3c3b :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-01T06:21:23.398Z sha 0fd3c3b :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-01T06:53:18.345Z sha 0fd3c3b :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-01T11:31:36.934Z sha 0fd3c3b :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-01T15:30:28.868Z sha 0fd3c3b :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-01T19:18:54.808Z sha 0fd3c3b :: needs real behavior proof before merge. :: [P1] Reuse the canonical Responses usage projection

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Aug 1, 2026
@clawsweeper

clawsweeper Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(gateway): enforce OpenAI-compatible response contracts This is item 2/3 in the current shard. Shard 9/82.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

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

Labels

gateway Gateway runtime maintainer Maintainer-authored PR merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant