Skip to content

feat(api): introduce RequestConfigBuilder for SDK-agnostic abort signal support#701

Draft
easonLiangWorldedtech wants to merge 5 commits into
Zoo-Code-Org:mainfrom
easonLiangWorldedtech:feat/abort-signal-core/config-builder
Draft

feat(api): introduce RequestConfigBuilder for SDK-agnostic abort signal support#701
easonLiangWorldedtech wants to merge 5 commits into
Zoo-Code-Org:mainfrom
easonLiangWorldedtech:feat/abort-signal-core/config-builder

Conversation

@easonLiangWorldedtech

@easonLiangWorldedtech easonLiangWorldedtech commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR introduces RequestConfigBuilder — a generic, SDK-agnostic request configuration builder that provides a fluent API for building type-safe request configurations. It serves as the second foundation for abort signal support in Zoo-Code, complementing #674 which wired the AbortSignal through the core metadata layer.

Why this builder?

#674 successfully added abortSignal?: AbortSignal to ApiHandlerCreateMessageMetadata and wired it from Task.ts. However, each provider still manually constructs its request config by spreading { signal: metadata?.abortSignal, headers: ... } — a repetitive pattern that:

  1. Duplicates logic across every provider implementation
  2. Is error-prone — easy to forget or mistype the spread
  3. Doesn't handle SDK differences well — OpenAI uses queryParams, Bedrock uses body, Anthropic uses apiVersion
  4. Lacks immutability guarantees — direct spreading can accidentally mutate original config objects

This builder solves all of that with a unified, chainable API:

const config = new RequestConfigBuilder()
    .addAbortSignal(metadata)
    .addHeaders({ "X-Custom-Header": "value" })
    .setOption("modelId", "claude-3-opus")
    .build()

Changes

File Description
src/api/providers/config-builder/request-config-builder.ts Core builder class (136 lines) with generic type support, chainable methods, and static utilities
src/api/providers/__tests__/request-config-builder.spec.ts Comprehensive test suite (461 lines, 50+ tests) covering all methods and edge cases
src/api/providers/config-builder/README.md Full documentation with architecture diagram, usage examples for OpenAI/Bedrock/Anthropic, and extension guide
src/api/providers/index.ts Export RequestConfigBuilder from the providers barrel

API Overview

Instance Methods (all chainable)

  • addAbortSignal(metadata?) — Extracts abort signal from metadata and adds it to config
  • addHeaders(headers) — Merges custom headers (empty objects skipped)
  • setOption(key, value) — Type-safe single option setter
  • getOption(key) — Get an option by key
  • build() — Returns shallow copy for immutability; undefined if no options set

Static Methods

  • fromMetadata(metadata?, extraOptions?) — Quick factory for simple signal + options scenarios
  • mergeAbortSignals(primary, secondary?) — Combines two signals (abort when either fires)

Design Principles

  1. Immutabilitybuild() returns a shallow copy; constructor copies defaultOptions
  2. Defensive programming — undefined/empty values are silently skipped, never thrown
  3. Generic type safetyTOptions extends Record<string, any> ensures compile-time correctness per SDK
  4. Zero runtime overhead — Generics erased at compile time
  5. Extensibility — New SDKs extend the base class and add only their specific methods

Pros & Cons

✅ Pros

  • Unified interface across all SDKs (OpenAI, Bedrock, Anthropic, Vertex AI)
  • Type-safe via TypeScript generics (TOptions)
  • Fluent chainable API — concise, readable code
  • Immutability guaranteedbuild() returns shallow copy
  • Defensive by default — no errors on undefined/empty inputs
  • Easy to extend — new SDK support = one extended class with 2-3 methods

⚠️ Cons

  • Extra abstraction layer — may feel like over-engineering for simple single-provider setups
  • Learning curve — team needs to understand generics + builder pattern
  • Migration period — existing providers still use manual config spreading; gradual migration needed
  • SDK-specific builders not yet implemented — only the base class is included (OpenAI/Bedrock/Anthropic extensions are documented as future work in README)

Relationship to other PRs

PR Status Role
#434 Closed Original attempt: manually added signal to 59 files across 16+ providers. Too scattered, hard to maintain.
#674 Merged ✅ Core plumbing: added abortSignal to metadata interface and wired it through Task.ts. This PR builds on top of that foundation.
This PR Open Builder pattern: unified, type-safe config construction for all providers going forward.

Next Steps (after merge)

  1. Create SDK-specific builder classes (OpenAiRequestConfigBuilder, BedrockRequestConfigBuilder, etc.) with provider-specific methods like addPath(), addQueryParams(), setApiVersion()
  2. Gradually migrate existing providers to use the builder pattern
  3. Add integration tests verifying builders work correctly with actual SDK calls

Related

Closes #434 (supersedes the manual approach with a reusable pattern)

Summary by CodeRabbit

  • New Features
    • Added a type-safe, fluent request configuration builder with typed set/get options, header merging, and abort-signal handling (including primary/secondary merging).
    • Supports creating configurations from handler metadata and returning an undefined result when no options are set.
  • Documentation
    • Added a comprehensive README with examples, API reference, and detailed abort-signal semantics.
  • Tests
    • Expanded coverage for chaining, shallow-copy/immutability behavior, and abort-signal propagation edge cases.
  • Chores
    • Re-exported the builder from the providers index for easier access.

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 61ef2775-f2e4-4823-bb15-ae58bd62e7a5

📥 Commits

Reviewing files that changed from the base of the PR and between ba54c38 and af9e14f.

📒 Files selected for processing (2)
  • src/api/providers/__tests__/request-config-builder.spec.ts
  • src/api/providers/config-builder/request-config-builder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/api/providers/config-builder/request-config-builder.ts

📝 Walkthrough

Walkthrough

Adds a generic RequestConfigBuilder<TOptions> under src/api/providers/config-builder/, plus tests, documentation, and a providers barrel re-export. The builder supports chainable option updates, metadata-based abort-signal handling, shallow-copy builds, and static helpers for metadata and abort-signal merging.

Changes

RequestConfigBuilder implementation, tests, and docs

Layer / File(s) Summary
Builder class and export
src/api/providers/config-builder/request-config-builder.ts, src/api/providers/index.ts
Defines the generic RequestConfigBuilder<TOptions> with constructor defaults, addAbortSignal, addHeaders, setOption, getOption, build, fromMetadata, and mergeAbortSignals, then re-exports it from the providers index.
Vitest coverage
src/api/providers/__tests__/request-config-builder.spec.ts
Covers constructor behavior, chainability, shallow-copy semantics, abort-signal and header handling, typed option access, static helpers, abort propagation timing, and integration-style chained usage.
README documentation
src/api/providers/config-builder/README.md
Documents the builder API, usage patterns, abort-signal behavior, generic TOptions design, SDK extension guidance, test strategy, API reference, and design principles.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Zoo-Code-Org/Zoo-Code#674: This PR also wires abortSignal through request-building paths and tests signal propagation behavior.

Suggested reviewers

  • hannesrudolph
  • taltas
  • JamesRobert20

Poem

🐇 I hop through configs, light and neat,
With signals, headers, and options sweet.
One builder to chain, one build to see,
Then off it goes, in rabbitty glee.
🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning This PR adds a reusable builder, but it does not implement the linked issue's required abortSignal propagation through providers and Task.ts. Add the provider and Task abort-signal wiring required by #434, or split the builder into a supporting PR linked to the issue.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly names the new RequestConfigBuilder and its abort-signal-focused purpose.
Description check ✅ Passed The description is detailed and mostly matches the template, but it omits the explicit Closes issue line and a test procedure.
Out of Scope Changes check ✅ Passed The added tests, docs, and barrel export are all directly related to the new builder and its API surface.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/api/providers/config-builder/request-config-builder.ts (1)

119-129: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider simplifying the early-abort case.

When primarySignal is already aborted, the code creates a new AbortController, aborts it, and returns its signal. Since the caller can use primarySignal directly in this case (it's already aborted), you could return it instead:

 	const controller = new AbortController()

 	if (primarySignal.aborted) {
-		controller.abort()
-		return controller.signal
+		return primarySignal
 	}

This avoids allocating an unnecessary controller. The current implementation is correct and defensive, so this is purely an optimization.

🤖 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 119
- 129, In the mergeAbortSignals method, the code currently creates a new
AbortController and aborts it when primarySignal is already aborted, then
returns the new controller's signal. Since an already-aborted signal serves the
same purpose, simplify this early-abort case by returning primarySignal directly
instead of allocating and aborting a new controller. This optimization
eliminates unnecessary object creation while maintaining the same behavior.
🤖 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/README.md`:
- Line 18: The link fragment in the table of contents entry
`#how-mergesignals-works` does not match the actual heading "How
`mergeAbortSignals` Works" on line 196. Update the link fragment from
`#how-mergesignals-works` to `#how-mergeabortsignals-works` so that it correctly
points to the corresponding heading and the anchor link functions properly.

---

Nitpick comments:
In `@src/api/providers/config-builder/request-config-builder.ts`:
- Around line 119-129: In the mergeAbortSignals method, the code currently
creates a new AbortController and aborts it when primarySignal is already
aborted, then returns the new controller's signal. Since an already-aborted
signal serves the same purpose, simplify this early-abort case by returning
primarySignal directly instead of allocating and aborting a new controller. This
optimization eliminates unnecessary object creation while maintaining the same
behavior.
🪄 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: 513b97a4-e024-4847-beb2-9648492cd853

📥 Commits

Reviewing files that changed from the base of the PR and between e8acc6a and e48579f.

📒 Files selected for processing (4)
  • src/api/providers/__tests__/request-config-builder.spec.ts
  • src/api/providers/config-builder/README.md
  • src/api/providers/config-builder/request-config-builder.ts
  • src/api/providers/index.ts

Comment thread src/api/providers/config-builder/README.md Outdated

@edelauna edelauna left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this — the intent to reduce duplication across providers is appreciated. However, after reviewing this against the tracking issue (#404) and its sub-issues (#615#618), I think this PR should either be closed or substantially reworked.

The sub-issues explicitly chose direct wiring over an abstraction

The issue decomposition already prescribes specific patterns for each provider group:

  • #616 (simple providers, ~18 of them): "Use the simple direct object pattern ({ signal: metadata?.abortSignal }) rather than the variadic spread-of-array trick." — a one-liner per provider, no builder needed.
  • #617 (bridging providers): inline addEventListener("abort", ..., { once: true }) + pre-aborted guard — provider-specific wiring.
  • #618 (SDK quirks): each provider has unique config placement (CancellationToken bridge, config object, etc.).

None of these reference or require a RequestConfigBuilder. The builder solves a problem the decomposition deliberately decided not to have.

The only reusable piece is mergeAbortSignals — and the platform already provides it

AbortSignal.any() has been available since Node 20.3.0 and this project pins 20.20.2. The hand-rolled mergeAbortSignals can be replaced with:

AbortSignal.any([primarySignal, secondarySignal])

This also fixes a semantic issue in the current implementation: when secondarySignal is already aborted, the method returns the (non-aborted) primarySignal — contradicting the JSDoc ("If any signal is aborted, the returned signal will be aborted"). AbortSignal.any() handles this correctly.

No current consumer

No existing provider imports RequestConfigBuilder, and per the sub-issues above, none of the planned provider work will need it either. The abstraction is unvalidated.


Recommendation: Close this PR. The provider migrations in #616/#617/#618 should use the direct patterns prescribed in those issues, with AbortSignal.any() where signal merging is needed (#617). If a builder abstraction proves necessary after those migrations land, it can be introduced then with real consumers to validate the API.

@edelauna

Copy link
Copy Markdown
Contributor

I think this work might be premature issue #615 is still open, I'd focus on that first.

@github-actions github-actions Bot added the awaiting-author PR is waiting for the author to address requested changes label Jun 25, 2026
@easonliang28

Copy link
Copy Markdown
Contributor

Thanks for the review. I will work on issue #615 first.

@easonLiangWorldedtech easonLiangWorldedtech marked this pull request as draft June 25, 2026 11:34
@easonLiangWorldedtech easonLiangWorldedtech force-pushed the feat/abort-signal-core/config-builder branch from cac1a1c to c2dd146 Compare June 26, 2026 16:10
@easonLiangWorldedtech easonLiangWorldedtech marked this pull request as ready for review June 26, 2026 16:27
…nfiguration

Body: Implement generic request configuration builder with chainable methods (addAbortSignal, addHeaders, setOption), static factory methods (fromMetadata, mergeAbortSignals), and 40 unit tests.
…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
@easonLiangWorldedtech easonLiangWorldedtech force-pushed the feat/abort-signal-core/config-builder branch from c2dd146 to ba54c38 Compare June 26, 2026 19:41
@github-actions github-actions Bot added awaiting-review PR changes are ready and waiting for maintainer re-review and removed awaiting-author PR is waiting for the author to address requested changes labels Jun 26, 2026
@easonLiangWorldedtech

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

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.

@edelauna

Copy link
Copy Markdown
Contributor

I'm going to move this PR to draft. Let's reopen it once issue #615 has been addressed.

Thank you.

@edelauna edelauna marked this pull request as draft June 29, 2026 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-review PR changes are ready and waiting for maintainer re-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants