Skip to content

feat(sdk): add include_directories option to both SDKs - #6468

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

feat(sdk): add include_directories option to both SDKs#6468
juhuan wants to merge 1 commit into
QwenLM:mainfrom
juhuan:feat/sdk-include-dirs

Conversation

@juhuan

@juhuan juhuan commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Add include_directories (Python) / includeDirectories (TypeScript) option that maps to CLI's --include-directories flag, allowing users to add extra directories to the workspace context.

  • Python SDK: include_directories: list[str] | None in QueryOptions, mapped to --include-directories (comma-separated)
  • TypeScript SDK: includeDirectories?: 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 `include_directories` (Python) / `includeDirectories` (TypeScript)
option that maps to CLI's `--include-directories` flag, allowing users
to add extra directories to the workspace context.

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

TypeScript SDK:
- Add `includeDirectories?: string[]` to TransportOptions and QueryOptions
- Add to Zod schema
- Pass through to ProcessTransport.buildCliArguments

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Thanks for the PR, @juhuan!

The PR body doesn't follow the PR template. The template requires these sections:

  • What this PR does — your "Summary" covers this, but the heading name differs
  • Why it's needed — missing. Why do SDK users need includeDirectories? What workflow does it enable?
  • Reviewer Test Plan (How to verify / Evidence / Tested on) — missing. The "Test plan" lists passing unit tests, but the template asks for reviewer-facing verification steps and OS coverage
  • Risk & Scope — missing
  • Linked Issues — missing
  • Chinese translation (<details> block) — missing

Could you update the PR body to match the template? This helps reviewers evaluate the change efficiently.

The code change itself looks clean at a glance — follows the same pattern as allowedTools / coreTools in both SDKs. Happy to do a full review once the template is in place.

中文说明

感谢 PR,@juhuan

PR 正文没有按照 PR 模板 填写。模板要求以下部分:

  • What this PR does — 你的 "Summary" 覆盖了内容,但标题名称不同
  • Why it's needed — 缺失。为什么 SDK 用户需要 includeDirectories?它支持什么工作流?
  • Reviewer Test Plan(验证方法 / 证据 / 测试平台)— 缺失。"Test plan" 列出了通过的单元测试,但模板要求面向审查者的验证步骤和操作系统覆盖情况
  • Risk & Scope — 缺失
  • Linked Issues — 缺失
  • 中文翻译<details> 块)— 缺失

能否更新 PR 正文以匹配模板?这有助于审查者高效评估变更。

代码变更本身看起来很干净——在两个 SDK 中都遵循了 allowedTools / coreTools 的相同模式。模板就位后,我很乐意进行完整的代码审查。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

No review findings. Downgraded from Approve to Comment: CI still running.

The PR cleanly adds includeDirectories / include_directories to both TypeScript and Python SDKs, faithfully following the existing patterns for allowedTools / coreTools / excludeTools. All 58 Python tests and 1163 TypeScript tests pass.

One minor note for the author: consider adding include_directories to the existing CLI argument serialization tests in both SDKs (test_build_cli_arguments_maps_supported_options in Python, should build CLI arguments correctly with all options in TypeScript) to match the coverage of sibling options.

— qwen3.7-max via Qwen Code /review

@doudouOUC doudouOUC left a comment

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] Both SDK READMEs omit the new option from their API reference

packages/sdk-typescript/README.md documents all 21 QueryOptions in a table but omits includeDirectories. packages/sdk-python/README.md lists peer options (core_tools, exclude_tools, allowed_tools) but not include_directories. Consider adding a row/entry for the new option so SDK users can discover it.

— qwen3.7-max via Qwen Code /review

if options.allowed_tools:
args.extend(["--allowed-tools", ",".join(options.allowed_tools)])

if options.include_directories:

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.

[Critical] Missing tests for include_directories in both SDKs

The existing test_build_cli_arguments_maps_supported_options test (Python) and equivalent ProcessTransport.test.ts test (TypeScript) cover all peer list options (core_tools, exclude_tools, allowed_tools) but include_directories is absent from both. The new code path (if options.include_directories: args.extend(...)) and the TypeScript .length > 0 guard are entirely untested.

Impact: A regression in the argument name, join separator, or guard condition would go undetected.

Suggested fix: Add include_directories=["/workspace/shared"] to the Python QueryOptions(...) call and assert "--include-directories", "/workspace/shared" in the expected args. Mirror in ProcessTransport.test.ts with includeDirectories: ['/workspace/shared']. Also add a test verifying that include_directories=[] / includeDirectories: [] does not emit the flag.

— qwen3.7-max via Qwen Code /review

@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