feat(api): introduce RequestConfigBuilder for SDK-agnostic abort signal support - #1008
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesRequest configuration and abort-signal support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant RequestConfigBuilder
participant AbortSignalUtilities
participant RequestConfig
Provider->>RequestConfigBuilder: add options, headers, and metadata
RequestConfigBuilder->>AbortSignalUtilities: merge internal and external signals
AbortSignalUtilities-->>RequestConfigBuilder: return signal and cleanup
RequestConfigBuilder-->>RequestConfig: build shallow-copied configuration
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/api/providers/config-builder/README.md (2)
196-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocumented
mergeAbortSignalsimplementation doesn't match the actual code.This section shows a hand-written implementation (manual
AbortController+addEventListener, with a special case returningprimarySignalunchanged whensecondarySignalis already aborted). The real implementation inabort-signal.tsdelegates to nativeAbortSignal.any(), which behaves differently — notably, it will immediately reflect an already-aborted secondary signal, the opposite of what this doc claims. This will mislead anyone extending the builder based on this doc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/config-builder/README.md` around lines 196 - 221, Update the “How mergeAbortSignals Works” section to accurately document the actual implementation in mergeAbortSignals and its use of native AbortSignal.any(). Remove the hand-written AbortController example and state that any already-aborted input, including secondarySignal, causes the returned signal to be aborted immediately.
479-495: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAPI reference is missing
addMergedSignal/mergeAbortSignalAndTimeout.The Instance/Static Methods tables don't document
addMergedSignal, even though it's the method intended to address issue#404(merging a provider's internal controller with an external abort signal and optional timeout). Worth adding given it's a key part of this PR's purpose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/config-builder/README.md` around lines 479 - 495, Update the README API reference tables for the configuration builder to document the missing addMergedSignal instance method and mergeAbortSignalAndTimeout static method, including their parameters, return types, and behavior for combining provider/external abort signals with an optional timeout. Keep the descriptions aligned with the actual method signatures and implementation.src/api/providers/config-builder/request-config-builder.ts (1)
42-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHeader merge is case-sensitive; HTTP header names are not.
addHeadersmerges via plain object spread on raw key strings. Differently-cased keys (e.g."x-custom"vs"X-Custom") won't be recognized as the same header, so a lateraddHeaderscall intended to override an earlier one could instead add a duplicate header with different casing sent to the wire.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/config-builder/request-config-builder.ts` around lines 42 - 50, Update addHeaders to merge HTTP header names case-insensitively, so differently-cased keys are treated as the same header and later values override earlier ones. Preserve the existing headers and options behavior while ensuring the resulting headers do not contain duplicate names differing only by casing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/providers/config-builder/request-config-builder.ts`:
- Around line 63-73: Update RequestConfigBuilder.addMergedSignal and build so
the internal cleanup handle is not stored in or returned as part of the SDK
options object. Expose cleanup separately through the builder’s public result or
accessor, while preserving the merged signal in the built options and existing
behavior when no merged signal was added.
---
Nitpick comments:
In `@src/api/providers/config-builder/README.md`:
- Around line 196-221: Update the “How mergeAbortSignals Works” section to
accurately document the actual implementation in mergeAbortSignals and its use
of native AbortSignal.any(). Remove the hand-written AbortController example and
state that any already-aborted input, including secondarySignal, causes the
returned signal to be aborted immediately.
- Around line 479-495: Update the README API reference tables for the
configuration builder to document the missing addMergedSignal instance method
and mergeAbortSignalAndTimeout static method, including their parameters, return
types, and behavior for combining provider/external abort signals with an
optional timeout. Keep the descriptions aligned with the actual method
signatures and implementation.
In `@src/api/providers/config-builder/request-config-builder.ts`:
- Around line 42-50: Update addHeaders to merge HTTP header names
case-insensitively, so differently-cased keys are treated as the same header and
later values override earlier ones. Preserve the existing headers and options
behavior while ensuring the resulting headers do not contain duplicate names
differing only by casing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b693b2e0-7220-4c8d-8f69-b87c93c8adb4
📒 Files selected for processing (6)
src/api/providers/__tests__/request-config-builder.spec.tssrc/api/providers/config-builder/README.mdsrc/api/providers/config-builder/request-config-builder.tssrc/api/providers/index.tssrc/api/providers/utils/__tests__/abort-signal.spec.tssrc/api/providers/utils/abort-signal.ts
7e9d8d4 to
c0a7e12
Compare
github-actions
Bot
added
the
awaiting-review
| await vi.advanceTimersByTimeAsync(100) | ||
|
|
||
| expect(config.signal?.aborted).toBe(true) | ||
| builder.getCleanup()() |
There was a problem hiding this comment.
Here getCleanup()() runs only after the timer has already fired (line 360 advances time, 362 asserts aborted). Does this actually verify that cleanup prevents the abort? abort-signal.spec.ts calls cleanup() before advancing time and asserts the signal stays un-aborted plus getTimerCount() === 0 — mirroring that here would cover the builder's cleanup wiring.
| const signal = mergeAbortSignals(internalController.signal, merged.signal) | ||
|
|
||
| this.options = { ...this.options, signal } as TOptions | ||
| this.cleanupFn = merged.cleanup |
There was a problem hiding this comment.
What happens if addMergedSignal is called twice? This overwrites cleanupFn without invoking the previous one, so the first setTimeout would never be cleared. Since the API is chainable, maybe chain them (const prev = this.cleanupFn; this.cleanupFn = () => { prev(); merged.cleanup() }) or document single-call semantics?
| primaryController.abort() | ||
|
|
||
| // Wait for event to propagate | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) |
There was a problem hiding this comment.
This (and lines 442 and 464) use a real setTimeout(resolve, 10) while the addMergedSignal tests above use vi.useFakeTimers(). Could this race under CI load? AbortSignal.any propagation is synchronous, so asserting aborted directly after abort() (or using fake timers) would be deterministic.
| }) | ||
| }) | ||
|
|
||
| describe("addMergedSignal", () => { |
There was a problem hiding this comment.
Is the already-aborted external signal case covered? I don't see a test where metadata.abortSignal is already aborted before addMergedSignal — worth adding one asserting the config signal is immediately aborted and getCleanup()() is still safe to call?
| return this | ||
| } | ||
|
|
||
| const existingHeaders = (this.options as any).headers ?? {} |
There was a problem hiding this comment.
The as any here (plus the six as TOptions casts) suggests TOptions extends Record<string, any> isn't really guaranteeing anything — addHeaders assumes a headers field the type never declares. Would constraining TOptions to include headers? / signal? (or dropping the class-level generic and keeping the per-method ones) give real type safety without the casts?
There was a problem hiding this comment.
this is not required for this feature
github-actions
Bot
added
awaiting-author
…nfiguration Body: Implement generic request configuration builder with chainable methods (addAbortSignal, addHeaders, setOption), static factory methods (fromMetadata, mergeAbortSignals), and 40 unit tests.
… and multi-SDK examples
…ls early-abort - Fix README TOC: change #how-mergesignals-works to #how-mergeabortsignals-works to match the actual heading anchor - Simplify mergeAbortSignals: return primarySignal directly when it's already aborted instead of creating a new AbortController
…onfigBuilder (Zoo-Code-Org#615) - Add default empty object parameter to addHeaders() so calling with undefined no longer throws TypeError from Object.keys(undefined) - Reorder mergeAbortSignals to check primarySignal.aborted before allocating AbortController, preventing unnecessary controller creation
c0a7e12 to
7335967
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/providers/__tests__/request-config-builder.spec.ts (1)
348-365: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFake timers aren't restored on assertion failure.
vi.useRealTimers()(Line 364) only runs if every prior assertion passes. If e.g. the assertion on Line 358 or 362 throws, fake timers leak into subsequent tests in the file.♻️ Guarantee timer cleanup
- test("should merge internal controller signal with timeout", async () => { - vi.useFakeTimers() + test("should merge internal controller signal with timeout", async () => { + vi.useFakeTimers() + try { const internalController = new AbortController() const builder = new RequestConfigBuilder() builder.addMergedSignal(internalController, undefined, 100) const config = builder.build() as { signal?: AbortSignal; _cleanup?: () => void } expect(config.signal).not.toBe(internalController.signal) expect(config).not.toHaveProperty("_cleanup") expect(config.signal?.aborted).toBe(false) await vi.advanceTimersByTimeAsync(100) expect(config.signal?.aborted).toBe(true) builder.getCleanup()() - vi.useRealTimers() - }) + } finally { + vi.useRealTimers() + } + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/request-config-builder.spec.ts` around lines 348 - 365, Ensure the “should merge internal controller signal with timeout” test always restores real timers and performs signal cleanup, including when an assertion or timer advancement fails. Wrap the test body using the existing setup/cleanup pattern or a try/finally block, keeping vi.useFakeTimers() setup and vi.useRealTimers() restoration paired.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/api/providers/__tests__/request-config-builder.spec.ts`:
- Around line 348-365: Ensure the “should merge internal controller signal with
timeout” test always restores real timers and performs signal cleanup, including
when an assertion or timer advancement fails. Wrap the test body using the
existing setup/cleanup pattern or a try/finally block, keeping
vi.useFakeTimers() setup and vi.useRealTimers() restoration paired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6c91bd5-e42c-4c4a-844e-e6532ee56f96
📒 Files selected for processing (6)
src/api/providers/__tests__/request-config-builder.spec.tssrc/api/providers/config-builder/README.mdsrc/api/providers/config-builder/request-config-builder.tssrc/api/providers/index.tssrc/api/providers/utils/__tests__/abort-signal.spec.tssrc/api/providers/utils/abort-signal.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/api/providers/index.ts
- src/api/providers/utils/tests/abort-signal.spec.ts
- src/api/providers/utils/abort-signal.ts
- src/api/providers/config-builder/README.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/providers/config-builder/request-config-builder.ts`:
- Around line 23-24: Update the constructor of the request config builder to
remove entries whose default option values are undefined when initializing
this.options. Preserve defined defaults and ensure build() treats entirely
undefined defaults as absent, returning undefined rather than an object
containing undefined-valued keys.
- Around line 14-18: Update RequestConfigOptions and the generic constraints
used by RequestConfigBuilder and RequestConfigBuilder.fromMetadata so the public
constraint requires only optional headers and signal fields, removing the
mandatory Record<string, unknown> index signature. Preserve arbitrary-key
support through the default generic type rather than the shared constraint.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fde9c2f2-617e-422a-acb8-3cabcc1e3522
📒 Files selected for processing (2)
src/api/providers/__tests__/request-config-builder.spec.tssrc/api/providers/config-builder/request-config-builder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/providers/tests/request-config-builder.spec.ts
github-actions
Bot
added
awaiting-review
3 participants
feat(api): introduce RequestConfigBuilder for SDK-agnostic abort signal support
Problem
Currently each provider manually constructs its request config by spreading
{ signal: metadata?.abortSignal, headers: ... }— a repetitive pattern that:queryParams, Bedrock usesbody, Anthropic usesapiVersionProposed Solution
Introduce
RequestConfigBuilder— a generic, SDK-agnostic request configuration builder that provides a fluent API for building type-safe request configurations.Usage Example
Key Features
TOptions extends Record<string, any>ensures compile-time correctness per SDKmergeAbortSignals()returns primary directly when no secondary providedbuild()returns shallow copyAPI Methods
Instance Methods (all chainable)
addAbortSignal(metadata?)— Extracts abort signal from metadata and adds it to configaddHeaders(headers?)— Merges custom headers (empty objects skipped, no default param overhead)setOption(key, value)— Type-safe single option settergetOption(key)— Get an option by keybuild()— Returns shallow copy for immutability;undefinedif no options setaddMergedSignal(internalController, metadata?, timeoutMs?)— Merge internal controller + external signal with timeout support (for providers that maintain their own AbortController)Static Methods
fromMetadata(metadata?, extraOptions?)— Quick factory for simple signal + options scenariosmergeAbortSignals(primary, secondary?)— Combines two signals (returns primary directly when no secondary — zero overhead)Files to Add/Modify
src/api/providers/config-builder/request-config-builder.tssrc/api/providers/utils/abort-signal.tsmergeAbortSignalAndTimeout()+mergeAbortSignals()(~49 lines)src/api/providers/__tests__/request-config-builder.spec.tssrc/api/providers/utils/__tests__/abort-signal.spec.tssrc/api/providers/config-builder/README.mdsrc/api/providers/index.tsRequestConfigBuilderfrom the providers barrelDesign Principles
build()returns a shallow copy; constructor copies defaultOptionsTOptions extends Record<string, any>ensures compile-time correctness per SDKmergeAbortSignals()returns primary directly when no secondary providedKey Improvements vs Original PR #701 Draft
addHeaders(headers: Record<string, string> = {})default param overheadheaders?: Record<string, string>mergeAbortSignals([single])creates unnecessary AbortControlleraddMergedSignal()for providers with internal AbortControllerRelationship to Other PRs/Issues
abortSignalto metadata interface and wired it through Task.ts. This builder builds on top of that foundation.Next Steps (after merge)
addMergedSignal()instead of manual signal mergingOpenAiRequestConfigBuilder,BedrockRequestConfigBuilder) with provider-specific methods likeaddPath(),addQueryParams(),setApiVersion()Related
Related to #404 — provides the infrastructure foundation for consistent abort signal handling across all providers. Actual fix will be implemented when refactoring individual providers in a follow-up PR.
Closes #1007
Summary by CodeRabbit