Skip to content

feat(sdk): add extensions option to both SDKs - #6470

Closed
juhuan wants to merge 1 commit into
QwenLM:mainfrom
juhuan:feat/sdk-extensions
Closed

feat(sdk): add extensions option to both SDKs#6470
juhuan wants to merge 1 commit into
QwenLM:mainfrom
juhuan:feat/sdk-extensions

Conversation

@juhuan

@juhuan juhuan commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Add extensions option that maps to CLI's --extensions (-e) flag, allowing users to specify which extensions to load for a session. If not provided, all extensions are used.

  • Python SDK: extensions: list[str] | None in QueryOptions, mapped to --extensions (comma-separated)
  • TypeScript SDK: extensions?: string[] in TransportOptions and QueryOptions, with Zod schema validation

Test plan

  • Python SDK tests pass (pytest — 58 passed)
  • TypeScript SDK typecheck passes (tsc --noEmit)
  • TypeScript SDK tests pass (vitest run — 1163 passed)

Add `extensions` option that maps to CLI's `--extensions` (`-e`) flag,
allowing users to specify which extensions to load for a session.

Python SDK:
- Add `extensions: list[str] | None` to QueryOptions
- Map to `--extensions` (comma-separated) in build_cli_arguments

TypeScript SDK:
- Add `extensions?: string[]` to TransportOptions and QueryOptions
- Add to Zod schema
- Pass through to ProcessTransport.buildCliArguments
if options.allowed_tools:
args.extend(["--allowed-tools", ",".join(options.allowed_tools)])

if options.extensions:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The new extensions option is not covered by any test in packages/sdk-python/tests/unit/test_transport.py. The existing test_build_cli_arguments_maps_supported_options test covers all analogous options (core_tools, exclude_tools, allowed_tools) but extensions is absent from both the QueryOptions(...) constructor call and the expected args list.

Consider adding extensions=["ext-a", "ext-b"] to the test's QueryOptions and asserting "--extensions", "ext-a,ext-b" in the expected args, mirroring the pattern used for the other list options.

— qwen3.7-max via Qwen Code /review

args.push('--allowed-tools', this.options.allowedTools.join(','));
}

if (this.options.extensions && this.options.extensions.length > 0) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The new extensions option is not covered by any test in packages/sdk-typescript/test/unit/ProcessTransport.test.ts. The existing test that exercises coreTools and excludeTools (around line 194) does not include extensions in the TransportOptions or the expected args.

Consider adding extensions: ['ext-a', 'ext-b'] to the test's TransportOptions and asserting '--extensions', 'ext-a,ext-b' in expect.arrayContaining([...]), mirroring the pattern used for the other list options.

— qwen3.7-max via Qwen Code /review

@doudouOUC

Copy link
Copy Markdown
Collaborator

qwen-code review summary

PR #6470 adds an extensions option to both the Python and TypeScript SDKs, mapping to the CLI's --extensions flag. The implementation is correct, minimal, and follows the same patterns used by existing list options (allowedTools, coreTools, etc.). No bugs or security issues in the diff itself.

Findings

[Suggestion] Missing test coverage for extensions in both SDKspackages/sdk-python/tests/unit/test_transport.py, packages/sdk-typescript/test/unit/ProcessTransport.test.ts

  • The existing transport tests cover every other list option (core_tools, exclude_tools, allowed_tools, auth_type) but extensions was added without any corresponding test assertion.
  • A future refactor could silently drop --extensions from CLI invocation with no test catching it.
  • Fix: Add extensions=["my-ext"] to existing test inputs and assert "--extensions", "my-ext" appears in output.

Needs Human Review

Possibly: Argument injection via flag-like extension names — If an SDK consumer passes untrusted extension names (e.g., ["--yolo"]), yargs may parse the flag-like string as a separate CLI option rather than as an extension name value. However, this is the exact same pattern used by allowedTools/coreTools/excludeTools across all SDKs — not introduced by this PR. Low practical risk since extension names are validated against loaded extensions on the CLI side.

Verdict

Comment — Clean, correct implementation. Tests would be nice but aren't blocking.

@wenshao

wenshao commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

💡 Suggestion: Consolidate SDK PRs

Hi @juhuan, thanks for the comprehensive SDK work! We noticed you have 15 open PRs that all modify the same core files (transport.py, types.py, queryOptionsSchema.ts, types.ts, ProcessTransport.ts, createQuery.ts) and were created on the same day.

The problem

  • Merge conflicts: Since all 15 PRs touch the same files, whichever merges first will cause conflicts in the remaining 14.
  • Review overhead: Reviewing 15 near-identical PRs separately is inefficient and risks fatigue.
  • CI cost: 15 separate CI runs for the same lint/typecheck passes.

Suggestion: regroup into 2 PRs

We recommend closing the current 15 PRs and reopening them as 2 consolidated PRs:

PR 1 — feat(sdk): expose transport and query options in both SDKs

Covers pure SDK-side option additions (~9 current PRs):

PR 2 — feat(sdk): add control request methods to both SDKs

Covers features that also involve CLI-side ControlDispatcher changes (~4 current PRs):

This keeps a reasonable separation of concerns while eliminating the merge-conflict chain and making review much more manageable.

/cc @juhuan

@wenshao

wenshao commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Closing in favor of consolidated PRs (see suggestion comment above). Please reopen as 2 grouped PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants