add previous_response_id continuation and responses contract typing

[ndycode]

Summary

• type previous_response_id, text.format, and prompt_cache_retention in the Responses request contract
• persist last response ids for opt-in continuation mode without changing the default stateless path
• add coverage for continuation and request-contract behavior across the transformer and response handling

Stack

• base: #309

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

📝 Walkthrough

Walkthrough

added response continuation feature enabling requests to resume from prior response ids. when enabled via config, the plugin derives a session key, retrieves the last stored response id from session affinity store, injects it into request body, and captures new response ids from sse streams via callbacks. extended session store to track response ids and updated request/response handling across fetch and sse layers.

Changes

Cohort / File(s)	Summary
Config & Schema lib/config.ts, lib/schemas.ts, lib/types.ts	Added responseContinuation: false config option with getResponseContinuation resolver. Extended PluginConfigSchema and RequestBody with new responseContinuation, previous_response_id, prompt_cache_retention fields, and TextFormatConfig union type for text output formats.
Session Affinity lib/session-affinity.ts	Extended session state to track lastResponseId. Added getLastResponseId() and rememberLastResponseId() methods for retrieving and persisting response ids with ttl-aware expiration.
Request & Response Handlers lib/request/fetch-helpers.ts, lib/request/response-handler.ts	Updated handleSuccessResponse to accept onResponseId callback. Extracted response id from sse payloads via maybeCaptureResponseEvent() and added attachResponseIdCapture() to wrap streams with TransformStream for id capture during sse parsing.
Plugin Entry Point index.ts	Added response continuation flow: clones request body, conditionally retrieves stored previous_response_id from session affinity, injects it when absent, captures new response id via callback, and stores it back to session store on success.
Plugin Config Tests test/plugin-config.test.ts	Added getResponseContinuation test suite covering env var override (CODEX_AUTH_RESPONSE_CONTINUATION), config fallback, and boolean parsing of '1' and '0'.
Integration Tests test/index.test.ts	Added two cases validating response continuation behavior: lookup and injection of stored previous_response_id when enabled, and explicit request-provided id overriding stored state.
Response Handler Tests test/response-handler.test.ts, test/request-transformer.test.ts, test/session-affinity.test.ts	Added coverage for attachResponseIdCapture stream wrapping, convertSseToJson callback invocation, field preservation in request transformation, and session store response id tracking across ttl boundaries.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Plugin as Plugin Handler
    participant SessionStore as Session Affinity<br/>Store
    participant API as Codex API
    participant Parser as SSE Parser

    Client->>Plugin: Request (no previous_response_id)
    Plugin->>SessionStore: getLastResponseId(sessionKey)
    SessionStore-->>Plugin: lastResponseId (if stored & valid)
    Plugin->>Plugin: Clone body, inject previous_response_id
    Plugin->>API: Fetch with continuation
    API-->>Plugin: Response stream
    Plugin->>Parser: parseSseStream(stream, onResponseId)
    Parser->>Parser: Read SSE events
    Parser->>Parser: Extract id from response.done event
    Parser->>Plugin: Invoke onResponseId(responseId)
    Plugin->>SessionStore: rememberLastResponseId(sessionKey, id)
    SessionStore-->>Plugin: Stored
    Parser-->>Plugin: Return final response
    Plugin-->>Client: Final response

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~90 minutes

concerns flagged:

• stream handling risk: createResponseIdCapturingStream wraps response body with TransformStream (lib/request/response-handler.ts:+90). verify backpressure handling and error propagation through transform chain. no explicit test for stream errors mid-parse.

• concurrency gap: rememberLastResponseId() and getLastResponseId() are not atomic. if two responses for same session key arrive near-simultaneously, the second call may overwrite the first's id or read stale data. session store uses plain object map—no locking. flag for shared session scenarios.

• sse parsing regression: parseSseStream now invokes onResponseId callback which can throw (lib/request/response-handler.ts:+25). callback errors are caught and logged, but the main parse flow continues. verify this doesn't mask callback failures silently in production.

• missing stream edge case tests: no coverage for attachResponseIdCapture when response body is already consumed, or when sse data line appears mid-chunk (split across multiple chunks). TransformStream chunk-level behavior untested.

• windows compat: stream handling should work cross-platform, but TransformStream usage is relatively new. no explicit windows validation mentioned.

• test isolation: test/index.test.ts:+119 mocks getResponseContinuation but doesn't verify its env var precedence against all config combinations. add regression test combining CODEX_AUTH_RESPONSE_CONTINUATION=1 with responseContinuation: false in config to confirm env wins.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	PR description is concise but incomplete: missing 'What Changed' section, validation checklist unchecked, and Docs/Governance/Risk sections omitted.	expand description to include full template: populate What Changed section with detailed file changes, complete all validation checkboxes, fill Docs/Governance/Risk sections, and confirm test status (npm test and documentation.test.ts).

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	title follows conventional commits format with type 'add', scope omitted, and summary under 72 chars in lowercase imperative mood.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/openai-parity-pr2

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch feat/openai-parity-pr2

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@index.ts`:
- Around line 2471-2479: The response-id write is racing with the
account-affinity write because onResponseId (capturedResponseId) can arrive
after handleSuccessResponse returns; change to perform an atomic per-request
update in sessionAffinityStore instead of two separate writes: add a new method
like rememberLastSession(sessionAffinityKey, accountId, responseId) (or
rememberLastAccountAndResponse) and call it only once when the final responseId
is available (for non-streaming flow use the immediate responseId, for streaming
wait for onResponseId before calling the atomic method); update usages around
handleSuccessResponse, capturedResponseId,
sessionAffinityStore.rememberLastResponseId and the synchronous account-affinity
write to use the new atomic call, and add a vitest that issues two concurrent
requests using the same sessionAffinityKey to assert the store ends up with
matching accountId+responseId (cover both code paths referenced near the other
occurrence around the second block).
- Around line 1350-1359: The response-continuation feature is erroneously gated
on sessionAffinityStore in the shouldUseResponseContinuation logic (see
parsedBody, shouldUseResponseContinuation,
getResponseContinuation(pluginConfig), sessionAffinityStore, getLastResponseId,
continuationSessionKey, parsedBody.previous_response_id); update the logic to
not silently no-op when response continuation is enabled without session
affinity by either (A) ensuring the session affinity store is instantiated/kept
alive whenever getResponseContinuation(pluginConfig) is true so
getLastResponseId/capture callbacks work, or (B) throw a clear configuration
error/fail-fast when responseContinuation=true while session affinity is
disabled; also add a vitest that enables responseContinuation=true with session
affinity disabled to assert the new behavior.

In `@test/index.test.ts`:
- Around line 1324-1333: The tests currently use buildRoutingManager() via
AccountManager.loadFromDisk.mockResolvedValueOnce(...) which advances selection
on each lookup and makes the two-call happy-path non-idempotent; change the stub
so the primary-account lookup is deterministic across independent sdk.fetch()
calls by returning an idempotent routing manager (or mock that returns the same
account every time) instead of a selection-advancing instance—update the mocks
at AccountManager.loadFromDisk (and the second occurrence) to mockResolvedValue
(or explicitly mock the primary-selection method) so consecutive requests see
the same account, and keep buildRoutingManager only for the failover cases that
need traversal behavior.
- Around line 1319-1432: The tests only inspect the argument passed into
fetchHelpers.transformRequestForCodex and miss whether previous_response_id
survives until the actual outbound request; update both tests to parse and
assert the JSON body passed to the mocked globalThis.fetch (inspect
vi.mocked(globalThis.fetch).mock.calls[...] [0/1][0/1].body) to ensure
previous_response_id is present/absent as expected, and add a new regression
test that fires two sdk.fetch calls concurrently with the same prompt_cache_key
to verify the continuation store doesn’t race (use Promise.all to run both
requests and then assert the two fetch calls’ bodies reflect correct
preservation/override of previous_response_id). Reference
transformRequestForCodex, handleSuccessResponse, sdk.fetch, prompt_cache_key and
previous_response_id when locating where to add/modify assertions.

In `@test/plugin-config.test.ts`:
- Line 193: Add a deterministic vitest regression that verifies loading
disk-backed plugin config preserves responseContinuation when true: call the
existing loadPluginConfig helper (or the module-level loadPluginConfig function)
with a temporary file containing {"responseContinuation": true} and assert the
returned config.responseContinuation === true; place this test alongside the
other plugin-config.test cases so it fails if the file-path parsing drops the
flag, and ensure it uses vitest's temporary file/cleanup utilities to remain
deterministic on Windows and in concurrent runs.

In `@test/session-affinity.test.ts`:
- Around line 126-135: Add a regression test to verify that
SessionAffinityStore.remember(...) does not clear an existing lastResponseId:
create a store, call remember("session-a", 1, t1), call
rememberLastResponseId("session-a", "resp_123", t2), then call
remember("session-a", 2, t3) to update the account index and assert that
getLastResponseId("session-a", t4) still returns "resp_123" and
getPreferredAccountIndex("session-a", t4) returns 2; place this alongside the
existing session-affinity tests to cover the behavior implemented in remember()
/ rememberLastResponseId().

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 87860fb3-bfde-451d-844b-4f6a5aa63dbe

📥 Commits

Reviewing files that changed from the base of the PR and between 9e13a24 and 173d64f.

📒 Files selected for processing (12)

• index.ts
• lib/config.ts
• lib/request/fetch-helpers.ts
• lib/request/response-handler.ts
• lib/schemas.ts
• lib/session-affinity.ts
• lib/types.ts
• test/index.test.ts
• test/plugin-config.test.ts
• test/request-transformer.test.ts
• test/response-handler.test.ts
• test/session-affinity.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (2)

lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

• lib/schemas.ts
• lib/config.ts
• lib/types.ts
• lib/request/fetch-helpers.ts
• lib/session-affinity.ts
• lib/request/response-handler.ts

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

• test/request-transformer.test.ts
• test/session-affinity.test.ts
• test/plugin-config.test.ts
• test/response-handler.test.ts
• test/index.test.ts

🔇 Additional comments (17)

lib/config.ts (2)

151-151: lgtm - default config addition follows existing pattern.

the new responseContinuation: false default is positioned correctly within the config object.

---

921-937: lgtm - config accessor follows established conventions.

getResponseContinuation correctly uses resolveBooleanSetting with env override CODEX_AUTH_RESPONSE_CONTINUATION, matching the pattern of other boolean config getters in this file.

lib/types.ts (2)

35-55: lgtm - type definitions cover known format variants with extensibility.

the TextFormatConfig union handles text, json_object, json_schema, plus a generic fallback. index signatures allow forward compatibility with new api fields.

---

124-124: lgtm - request body extended with responses api contract fields.

prompt_cache_retention and previous_response_id align with the response continuation feature. text.format typing matches upstream api.

Also applies to: 133-136

test/request-transformer.test.ts (1)

615-659: lgtm - regression tests cover field preservation through request transformation.

these tests verify previous_response_id, prompt_cache_retention, and text.format survive the transform pipeline. deterministic and use vitest correctly.

lib/session-affinity.ts (3)

69-81: lgtm - preserves lastResponseId when updating account index.

correctly retrieves existing entry before overwrite to carry forward the stored response id. this prevents losing continuation state during account rotation.

---

84-98: lgtm - getLastResponseId correctly handles expiry.

deletes expired entries lazily and returns trimmed response id or null. mirrors the pattern used by getPreferredAccountIndex.

---

100-122: ttl refresh on every rememberLastResponseId call is intentional; verify test coverage.

line 118 resets expiresAt = now + this.ttlMs on each response id capture. this is a deliberate sliding-window design: active sessions stay live as long as they're actively used. the pattern matches remember() at line 78—reads don't refresh, only writes do. integration tests at test/index.test.ts:1334-1392 confirm responseContinuation feature works correctly across multiple turns.

the prune() method (lib/session-affinity.ts:160-169) provides explicit cleanup for truly inactive entries. unit test at test/session-affinity.test.ts:117-124 covers basic response id storage; consider adding an explicit test case for "ttl refresh prevents expiry on update" to document the sliding-window behavior.

test/session-affinity.test.ts (1)

117-124: lgtm - covers happy path for response id storage/retrieval.

deterministic test with explicit timestamps. verifies both getLastResponseId and that getPreferredAccountIndex still works after response id update.

lib/request/response-handler.ts (4)

11-33: lgtm - safe callback invocation with error handling.

notifyResponseId wraps the callback in try/catch to prevent upstream errors from crashing the response flow. logging the responseId on failure is acceptable - these are opaque upstream identifiers, not auth tokens.

---

35-50: lgtm - unified terminal event handling.

maybeCaptureResponseEvent correctly handles both response.done and response.completed event types, extracting and notifying the response id from data.response.

---

162-204: lgtm - streaming capture implementation is correct.

createResponseIdCapturingStream:

• buffers partial lines until newline (line 172 keeps tail)
• forwards chunks unchanged via controller.enqueue(chunk) (line 196)
• handles final incomplete line in flush() (lines 199-200)
• silently ignores malformed json to preserve stream integrity

---

274-292: streaming path is correct - body not consumed before capture.

verified lib/request/fetch-helpers.ts:868: attachResponseIdCapture receives unconsumed response body directly from handleSuccessResponse. header operations (response.headers.get, ensureContentType) don't consume the body. test coverage at test/response-handler.test.ts:186-204 confirms sse stream preservation works as expected. no changes needed.

test/response-handler.test.ts (2)

119-130: lgtm - tests onResponseId callback in json conversion path.

uses vi.fn() to verify callback invocation with correct response id. also confirms the json body still contains the expected payload.

---

186-205: lgtm - tests streaming capture preserves content and invokes callback.

verifies:

• original sse text is unchanged after transformation
• onResponseId callback receives correct id
• content-type header is preserved

lib/schemas.ts (1)

50-50: lgtm - schema matches config and defaults.

responseContinuation: z.boolean().optional() aligns with lib/config.ts:151 default and lib/config.ts:931-937 accessor.

lib/request/fetch-helpers.ts (1)

848-868: nice success-path plumbing here.

both branches now forward onResponseId through the existing capture points in lib/request/response-handler.ts:20-33 and lib/request/response-handler.ts:162-204, so json-converted and pass-through streams stay consistent.

[ndycode]

@coderabbitai review
@greptile @cubic review

[coderabbitai]

@ndycode sure, kicking off a full review of PR #310 now. i'll look at the previous_response_id continuation logic, session affinity store changes, sse capture path, and the new config/type additions.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

All review threads are resolved and later commits addressed this stale automated change request.

[ndycode]

Closing because this work is now included in main via #318 and #319.