fix(gateway): accept rotating Copilot Responses IDs - #1959
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| event === "response.failed" | ||
| ) { | ||
| const response = parsed.response as Record<string, unknown> | undefined; | ||
| if (acc.id && response?.id !== acc.id) { | ||
| const requireCompleteLifecycle = | ||
| opts.validation !== "codex" || opts.pinResponseId === true; | ||
| if ( | ||
| !response || | ||
| typeof response.id !== "string" || |
There was a problem hiding this comment.
Bug: The identity check for terminal response events incorrectly ignores the requireCompleteLifecycle flag, leading to overly strict validation for codex streams.
Severity: MEDIUM
Suggested Fix
Update the identity check for terminal events to be conditional on the requireCompleteLifecycle flag. When the flag is true, enforce the strict response.id === acc.id check. When false, apply a more lenient check, such as only comparing IDs if both acc.id and response.id are defined.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/gateway/src/pipeline.ts#L10561-L10568
Potential issue: The `requireCompleteLifecycle` variable is defined to control
validation strictness but is not used in the identity check for terminal events like
`response.completed` and `response.failed`. This causes an overly strict validation
where the code unconditionally checks if `response.id` matches `acc.id`. For streams
where a complete lifecycle is not required (e.g., `codex` validation without
`pinResponseId`), this can incorrectly throw a "Responses terminal event changed
response identity" error.
Also affects:
packages/gateway/src/pipeline.ts:12525~12543
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Confirmed and fixed in f9704d5. Unpinned Codex terminal events may omit a repeated ID; an explicit mismatch still fails, and public/pinned lifecycles remain strict. The second terminal-output check now follows the same rule. Regressions cover sparse principal and recall continuation streams, mismatched IDs, and missing pinned IDs; the full test suite passes.
Coverage Results 📊✅ Patch coverage is 93.92% (170 of 181 changed executable lines covered; target 80%). Changed files with executable lines (5)
Coverage diff@@ Coverage Diff @@
## main #1959 +/-##
==========================================
+ Coverage 85.01% 85.05% +0.04%
==========================================
Files 318 318 —
Tracked lines 49482 49619 +137
Branches 40478 40709 +231
==========================================
+ Hits 42061 42199 +138
+ Misses 7421 7420 -1
- Partials 4285 4295 +10Generated by Coverage Action |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
🐛 1 issue in files not directly in the diff
🐛 Sparse principal terminal loses response identity
When a Codex principal terminal omits its ID, validateResponseLifecycle accepts it, but the no-recall path forwards it unchanged. Clients receive a terminal without the ID established by response.created.
|
Reviewed Devin’s sparse-terminal finding: the observation is correct, but it is not a gateway regression. Unpinned Codex permits a terminal that omits repeated |
|
1 participant
Summary
Addresses #1788. The latest report shows a Copilot Responses lifecycle failure; it does not establish that the earlier Gemini 400 shares this cause.
Verification