refactor(executors): extract mergeAbortSignals leaf from base.ts (PR-013) - #145
Conversation
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Note
|
codeant-ai
Bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors open-sse/executors/base.ts by extracting mergeAbortSignals and User-Agent header helper functions into dedicated modules (mergeAbortSignals.ts and userAgentHeader.ts) along with new unit tests. The reviewer identified three critical issues: a missing import of mergeAbortSignals in base.ts causing a runtime error, an incorrect parameter structure passed to applyConfiguredUserAgent that ignores the custom user agent, and a mismatch where the mergeAbortSignals implementation fails to handle undefined or null signals as expected by its unit tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import { | ||
| applyConfiguredUserAgent, | ||
| getCustomUserAgent, | ||
| setUserAgentHeader, | ||
| } from "./userAgentHeader.ts"; |
There was a problem hiding this comment.
The mergeAbortSignals function was extracted to its own file but is not imported in base.ts. This will cause a runtime ReferenceError when mergeAbortSignals is called during execution or token counting.
| import { | |
| applyConfiguredUserAgent, | |
| getCustomUserAgent, | |
| setUserAgentHeader, | |
| } from "./userAgentHeader.ts"; | |
| import { | |
| applyConfiguredUserAgent, | |
| getCustomUserAgent, | |
| setUserAgentHeader, | |
| } from "./userAgentHeader.ts"; | |
| import { mergeAbortSignals } from "./mergeAbortSignals.ts"; |
| if (k.toLowerCase() === "user-agent") { | ||
| setUserAgentHeader(headers, v); | ||
| applyConfiguredUserAgent(headers, { userAgent: v }); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
There is a bug here where applyConfiguredUserAgent is called with { userAgent: v }. However, applyConfiguredUserAgent expects providerSpecificData which contains customUserAgent, not userAgent. This will result in the custom user agent from extra headers being completely ignored. We should call setUserAgentHeader(headers, v) directly as was done originally.
| if (k.toLowerCase() === "user-agent") { | |
| setUserAgentHeader(headers, v); | |
| applyConfiguredUserAgent(headers, { userAgent: v }); | |
| continue; | |
| } | |
| if (k.toLowerCase() === "user-agent") { | |
| setUserAgentHeader(headers, v); | |
| continue; | |
| } |
| export function mergeAbortSignals( | ||
| primary: AbortSignal, | ||
| secondary: AbortSignal | ||
| ): AbortSignal { | ||
| const controller = new AbortController(); | ||
|
|
||
| const abortFrom = (source: AbortSignal) => { | ||
| if (!controller.signal.aborted) { | ||
| controller.abort(source.reason); | ||
| } | ||
| }; | ||
|
|
||
| if (primary.aborted) { | ||
| abortFrom(primary); | ||
| return controller.signal; | ||
| } | ||
| if (secondary.aborted) { | ||
| abortFrom(secondary); | ||
| return controller.signal; | ||
| } | ||
|
|
||
| primary.addEventListener("abort", () => abortFrom(primary), { once: true }); | ||
| secondary.addEventListener("abort", () => abortFrom(secondary), { once: true }); | ||
| return controller.signal; | ||
| } No newline at end of file |
There was a problem hiding this comment.
The newly added unit tests in mergeAbortSignals.test.ts expect mergeAbortSignals to handle undefined arguments and return undefined or the single defined signal. However, the current implementation does not handle undefined or null values and will throw a TypeError at runtime. We should update the signature and implementation to support optional/nullable signals as expected by the tests.
export function mergeAbortSignals(
primary?: AbortSignal | null,
secondary?: AbortSignal | null
): AbortSignal | undefined {
if (!primary && !secondary) return undefined;
if (!primary) return secondary ?? undefined;
if (!secondary) return primary;
const controller = new AbortController();
const abortFrom = (source: AbortSignal) => {
if (!controller.signal.aborted) {
controller.abort(source.reason);
}
};
if (primary.aborted) {
abortFrom(primary);
return controller.signal;
}
if (secondary.aborted) {
abortFrom(secondary);
return controller.signal;
}
primary.addEventListener("abort", () => abortFrom(primary), { once: true });
secondary.addEventListener("abort", () => abortFrom(secondary), { once: true });
return controller.signal;
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5427ea0f3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return controller.signal; | ||
| } | ||
|
|
||
| // extracted to ./mergeAbortSignals.ts (Wave 6 resilience leaf) |
There was a problem hiding this comment.
After moving these helpers out of base.ts, the module no longer exports setUserAgentHeader or mergeAbortSignals, while production executors still import them from ./base.ts (default.ts:1, gemini-cli.ts:1, claude-web.ts:22, gemini-business.ts:31). Because executors/index.ts imports those files during chat routing startup, the app can fail to load with missing named exports before serving requests; keep compatibility re-exports or update all existing imports in the same change.
Useful? React with 👍 / 👎.
| if (typeof k === "string" && k.length > 0 && typeof v === "string") { | ||
| if (k.toLowerCase() === "user-agent") { | ||
| setUserAgentHeader(headers, v); | ||
| applyConfiguredUserAgent(headers, { userAgent: v }); |
There was a problem hiding this comment.
When upstreamExtraHeaders includes User-Agent/user-agent, this branch now passes { userAgent: v } to applyConfiguredUserAgent, but that helper only reads customUserAgent, so the override is silently discarded. In the inspected execute() flow, connection-level custom UA is applied first and mergeUpstreamExtraHeaders is meant to override it later; route/model-level UA overrides therefore stop working for CLI-compatible upstreams that depend on the exact User-Agent.
Useful? React with 👍 / 👎.
🤖 Augment PR SummarySummary: Refactors executor abort/user-agent helpers out of Changes:
Technical Notes: The refactor aims to make these small utilities independently testable; however, a couple call-site/test expectations appear to have drifted from the extracted implementations and may need alignment. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| if (typeof k === "string" && k.length > 0 && typeof v === "string") { | ||
| if (k.toLowerCase() === "user-agent") { | ||
| setUserAgentHeader(headers, v); | ||
| applyConfiguredUserAgent(headers, { userAgent: v }); |
There was a problem hiding this comment.
open-sse/executors/base.ts:167 — applyConfiguredUserAgent reads providerSpecificData.customUserAgent, so passing { userAgent: v } means a User-Agent coming from extra headers is silently ignored (previously it was always applied via setUserAgentHeader). This looks like a behavior regression for per-model extra headers.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| headers: Record<string, string>, | ||
| userAgent: string, | ||
| ): void { | ||
| headers["User-Agent"] = userAgent; |
There was a problem hiding this comment.
open-sse/executors/userAgentHeader.ts:35-44 — The docstring says setUserAgentHeader is a no-op for empty userAgent, but the implementation always writes headers["User-Agent"] = userAgent (so an empty string would overwrite an existing UA). This also conflicts with the new unit test expectation.
Severity: medium
Other Locations
open-sse/executors/__tests__/userAgentHeader.test.ts:53
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| * The returned signal is always a NEW `AbortSignal` instance — the caller | ||
| * can safely forward it through any pipeline without aliasing the sources. | ||
| */ | ||
| export function mergeAbortSignals( |
There was a problem hiding this comment.
open-sse/executors/mergeAbortSignals.ts:18 — The extracted mergeAbortSignals currently requires two AbortSignals and always returns a new merged signal, but the newly added tests expect it to accept undefined, sometimes return undefined, and sometimes return one of the original signals. The contract between implementation and tests seems inconsistent and will likely cause failing tests / confusion about intended semantics.
Severity: medium
Other Locations
open-sse/executors/__tests__/mergeAbortSignals.test.ts:6open-sse/executors/__tests__/mergeAbortSignals.test.ts:12open-sse/executors/__tests__/mergeAbortSignals.test.ts:18open-sse/executors/__tests__/mergeAbortSignals.test.ts:24open-sse/executors/__tests__/mergeAbortSignals.test.ts:31open-sse/executors/__tests__/mergeAbortSignals.test.ts:49open-sse/executors/__tests__/mergeAbortSignals.test.ts:96
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| // a second abort on the upstream is a no-op for the listener we registered | ||
| upstream.abort(new Error("second")); | ||
| expect(merged.aborted).toBe(true); | ||
| expect((merged.reason as Error).message).toBe("second"); |
There was a problem hiding this comment.
open-sse/executors/tests/mergeAbortSignals.test.ts:86-88 — AbortController.abort() is specified as a no-op once the signal is already aborted (including not updating signal.reason), so asserting the merged signal’s reason becomes the second abort reason looks incorrect.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| upstream.abort(); | ||
| // a second abort on the upstream is a no-op for the listener we registered | ||
| upstream.abort(new Error("second")); | ||
| expect(merged.aborted).toBe(true); | ||
| expect((merged.reason as Error).message).toBe("second"); |
There was a problem hiding this comment.
Suggestion: This assertion is incorrect because AbortController.abort() is idempotent after the first abort; the abort reason does not change on subsequent calls. The test currently expects impossible behavior and will fail even when implementation is correct. [logic error]
Severity Level: Critical 🚨
- ❌ mergeAbortSignals test suite fails on every run.
- ❌ CI pipelines red, blocking merges and releases.
- ⚠️ Future regressions in abort logic harder to detect.Steps of Reproduction ✅
1. Open `open-sse/executors/__tests__/mergeAbortSignals.test.ts`, and locate the test case
`it("does not throw when one upstream signal fires twice", () => { ... })` at lines 80–88,
where `upstream.abort();` (line 84) is followed by `upstream.abort(new Error("second"));`
(line 86) and the assertion `expect((merged.reason as Error).message).toBe("second");`
(line 88).
2. Open `open-sse/executors/mergeAbortSignals.ts` and inspect `mergeAbortSignals` at lines
18–41: it creates a new `AbortController`, defines `abortFrom(source)` (lines 24–27) which
calls `controller.abort(source.reason)` only when `!controller.signal.aborted`, and
registers `primary.addEventListener("abort", () => abortFrom(primary), { once: true });`
and `secondary.addEventListener("abort", () => abortFrom(secondary), { once: true });`
(lines 39–40).
3. At runtime, in this test, the first `upstream.abort()` call (with no explicit reason)
sets `upstream.signal.aborted` to true and `upstream.signal.reason` to `undefined`; the
listener invokes `abortFrom(upstream.signal)` once, which aborts the merged controller
with `undefined` as its reason, and then the listener is removed due to `{ once: true }`.
4. The second call `upstream.abort(new Error("second"))` changes `upstream.signal.reason`
but does not re-trigger the removed listener; `controller.signal.aborted` is already true,
so its `reason` remains the initial value and can never be updated to `"second"`, causing
the assertion `expect((merged.reason as Error).message).toBe("second");` to fail every
time the test suite is run, even though the implementation is consistent with the
AbortController API contract (abort is effectively idempotent after the first call).
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** open-sse/executors/__tests__/mergeAbortSignals.test.ts
**Line:** 84:88
**Comment:**
*Logic Error: This assertion is incorrect because `AbortController.abort()` is idempotent after the first abort; the abort reason does not change on subsequent calls. The test currently expects impossible behavior and will fail even when implementation is correct.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if (k.toLowerCase() === "user-agent") { | ||
| setUserAgentHeader(headers, v); | ||
| applyConfiguredUserAgent(headers, { userAgent: v }); | ||
| continue; |
There was a problem hiding this comment.
Suggestion: The User-Agent passthrough branch is wiring the wrong property name into applyConfiguredUserAgent. getCustomUserAgent only reads customUserAgent, so this path silently drops upstream user-agent overrides instead of applying them. Pass customUserAgent (or call setUserAgentHeader directly) so the header is actually merged. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Upstream User-Agent overrides silently ignored across executors.
- ⚠️ Debugging client-specific UA issues becomes difficult.
- ⚠️ Some providers may mis-handle requests without expected UA.Steps of Reproduction ✅
1. In `open-sse/executors/base.ts`, inspect `mergeUpstreamExtraHeaders` (lines 159–173 in
the diff): when iterating `extra`, the branch `if (k.toLowerCase() === "user-agent") {`
(line 166) calls `applyConfiguredUserAgent(headers, { userAgent: v });` (line 167) and
then `continue;` (line 168) instead of writing `headers[k] = v`.
2. Open `open-sse/executors/userAgentHeader.ts` and examine `getCustomUserAgent` and
`applyConfiguredUserAgent`: `getCustomUserAgent` (lines 22–29) only returns a trimmed
string when `providerSpecificData.customUserAgent` is a non-empty string, otherwise it
returns `null`; `applyConfiguredUserAgent` (lines 51–58) calls
`getCustomUserAgent(providerSpecificData)` and only invokes `setUserAgentHeader(headers,
customUserAgent)` if that return value is truthy.
3. Because the `mergeUpstreamExtraHeaders` branch passes `{ userAgent: v }` into
`applyConfiguredUserAgent`, `getCustomUserAgent` sees no `customUserAgent` property and
returns `null`, so `applyConfiguredUserAgent` becomes a no-op; due to the `continue;`
statement, the `"User-Agent"` entry from `extra` is not written into `headers` at all,
while other non-User-Agent extra headers are merged correctly.
4. Use the Grep results to see real callers of `mergeUpstreamExtraHeaders`: executors such
as `gitlab.ts` (line 560, `mergeUpstreamExtraHeaders(headers,
input.upstreamExtraHeaders);`), `muse-spark-web.ts` (line 1242), `ninerouter.ts` (line
169), `cursor.ts` (line 1099), `qoder.ts` (line 117), `nlpcloud.ts` (line 462), `trae.ts`
(line 218), `blackbox-web.ts` (line 436), `cliproxyapi.ts` (line 400), and `codex.ts`
(line 848) all call this helper before issuing upstream HTTP requests; if any of these
pass `ExecuteInput.upstreamExtraHeaders` containing `"User-Agent": "custom-ua/1.0"`, the
custom UA override is silently dropped due to the mismatched property name, so upstream
services never see the intended User-Agent header.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** open-sse/executors/base.ts
**Line:** 166:168
**Comment:**
*Api Mismatch: The User-Agent passthrough branch is wiring the wrong property name into `applyConfiguredUserAgent`. `getCustomUserAgent` only reads `customUserAgent`, so this path silently drops upstream `user-agent` overrides instead of applying them. Pass `customUserAgent` (or call `setUserAgentHeader` directly) so the header is actually merged.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| export function mergeAbortSignals( | ||
| primary: AbortSignal, | ||
| secondary: AbortSignal | ||
| ): AbortSignal { |
There was a problem hiding this comment.
Suggestion: The implementation dereferences both inputs as required signals, so passing an undefined signal will throw at runtime when reading .aborted. This regresses the previous optional-signal behavior exercised by the new tests; guard undefined inputs and handle single-signal/no-signal cases before accessing signal properties. [null pointer]
Severity Level: Critical 🚨
- ❌ mergeAbortSignals tests crash on undefined signal inputs.
- ⚠️ Future optional signal callers risk runtime TypeErrors.
- ⚠️ Abort-handling resilience utilities become harder to rely on.Steps of Reproduction ✅
1. Open `open-sse/executors/mergeAbortSignals.ts` and inspect `mergeAbortSignals` (lines
18–41): the function signature `mergeAbortSignals(primary: AbortSignal, secondary:
AbortSignal): AbortSignal` treats both inputs as required and immediately dereferences
them, checking `primary.aborted` (lines 30–33) and `secondary.aborted` (lines 34–36)
without any null/undefined guards.
2. Open `open-sse/executors/__tests__/mergeAbortSignals.test.ts` and examine the first
four tests (lines 5–25): they explicitly call `mergeAbortSignals(undefined, undefined)`
(lines 5–7), pass two typed `AbortSignal | undefined` values that are both `undefined`
(lines 10–13), and call `mergeAbortSignals(controller.signal, undefined)` (lines 15–18)
and `mergeAbortSignals(undefined, controller.signal)` (lines 21–24), exercising the
optional-signal behavior.
3. Running the test suite (e.g., `vitest`) with this implementation causes
`mergeAbortSignals` to execute with `primary` and/or `secondary` equal to `undefined` in
these tests; when the function hits `if (primary.aborted) {` (line 30), accessing
`.aborted` on `undefined` throws a runtime TypeError instead of returning `undefined` or
the non-null signal as expected by the tests.
4. This mismatch between implementation and usage regresses the behavior previously
inlined in `BaseExecutor` (where optional signals were handled with patterns like `signal
? mergeAbortSignals(signal, timeoutSignal) : timeoutSignal` shown in executors such as
`glm.ts` and `base.ts` at lines 377 and 616/825): tests that treat `mergeAbortSignals` as
a safe helper for possibly-null signals now crash, and any future production code that
forwards optional signals without such guards would hit the same undefined-dereference
issue.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** open-sse/executors/mergeAbortSignals.ts
**Line:** 18:21
**Comment:**
*Null Pointer: The implementation dereferences both inputs as required signals, so passing an undefined signal will throw at runtime when reading `.aborted`. This regresses the previous optional-signal behavior exercised by the new tests; guard undefined inputs and handle single-signal/no-signal cases before accessing signal properties.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
d5427ea to
7f874dc
Compare
L17 Latency Budget ReportChecked against: budgets/rest-endpoints.yaml. |
|
1 participant
User description
Summary
CodeAnt-AI Description
Extract header and abort-signal handling into testable helpers
What Changed
User-Agenthandling into its own helper so provider-specific values are trimmed, applied to the request header, and mirrored to lowercase when needed.Impact
✅ Cleaner request headers✅ Fewer missed aborts during request handling✅ Safer header and cancellation behavior under edge cases💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.