Skip to content

feat(sdk): add disabled_slash_commands option to Python and TS SDKs - #6480

Closed
juhuan wants to merge 1 commit into
QwenLM:mainfrom
juhuan:feat/sdk-disabled-slash-commands
Closed

feat(sdk): add disabled_slash_commands option to Python and TS SDKs#6480
juhuan wants to merge 1 commit into
QwenLM:mainfrom
juhuan:feat/sdk-disabled-slash-commands

Conversation

@juhuan

@juhuan juhuan commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add disabled_slash_commands (Python) / disabledSlashCommands (TS) option mapping to CLI's --disabled-slash-commands flag
  • Slash command names to hide/disable, matched case-insensitively against the final command name
  • Implemented in both Python SDK (types, transport) and TS SDK (types, schema, transport, createQuery)
  • Unit tests added for both SDKs

Test plan

  • Python SDK unit tests pass (35 tests)
  • TS SDK unit tests pass (75 tests)
  • --disabled-slash-commands flag present in CLI args with comma-separated values

Expose CLI's --disabled-slash-commands flag through both SDKs'
QueryOptions, allowing users to hide/disable specific slash commands.

@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.

Hey @juhuan — thanks for the PR! The code changes look clean and focused — adding disabledSlashCommands / disabled_slash_commands to both SDKs is a straightforward wiring job, and the implementation follows existing patterns well.

However, the PR body doesn't follow the PR template. Most required sections are missing:

  • What this PR does — PR has "Summary" instead
  • Why it's needed — no motivation or problem statement
  • Reviewer Test Plan (How to verify, Evidence Before/After, Tested on table) — PR has a minimal "Test plan" with checkboxes but not the structured format
  • Risk & Scope — missing
  • Linked Issues — missing
  • 中文说明 — missing

Could you restructure the PR body to match the template? This helps maintainers review efficiently and understand the context at a glance. Happy to help if anything is unclear.

中文说明

你好 @juhuan — 感谢提交 PR!代码改动本身很简洁,在两个 SDK 中添加 disabledSlashCommands / disabled_slash_commands 的实现也遵循了现有模式。

不过,PR 正文没有遵循 PR 模板,大部分必填部分缺失:

  • What this PR does — PR 用的是 "Summary"
  • Why it's needed — 缺少动机或问题描述
  • Reviewer Test Plan(验证方法、前后对比、测试平台)— PR 只有一个简单的 "Test plan" 复选框
  • Risk & Scope — 缺失
  • Linked Issues — 缺失
  • 中文说明 — 缺失

能否按照模板重新组织 PR 正文?这样有助于维护者快速理解和审查。如有疑问随时沟通。

Qwen Code · qwen3.7-max

* Equivalent to CLI's `--disabled-slash-commands` flag.
* Matched case-insensitively against the final command name.
* @example ['/init', '/vim']
*/

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] The @example ['/init', '/vim'] uses slash-prefixed names, but the CLI matches against bare command names ('init', 'vim'). The yargs coerce and addDisabled functions never strip a leading /, so '/init' will never match 'init' — commands will silently remain enabled.

Verified at both matching sites (CommandService.create and nonInteractiveCliCommands.ts): they compare against cmd.name.toLowerCase() where cmd.name is the bare name (e.g., 'init' in initCommand.ts, 'vim' in vimCommand.ts). The official CLI docs in docs/users/configuration/settings.md correctly use bare names: "disabled": ["auth", "mcp"].

Suggested change
*/
* @example ['init', 'vim']

Alternatively, strip the leading / in both SDK transports before joining, since users will naturally think in /command terms.

— qwen3.7-max via Qwen Code /review

)

assert "--disabled-slash-commands" in args
idx = args.index("--disabled-slash-commands")

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] Test values ["/init", "/vim"] use slash-prefixed names, but the CLI expects bare names ('init', 'vim'). This propagates the incorrect format as test-as-documentation — developers reading this test will copy the slash-prefixed format.

Suggested change
idx = args.index("--disabled-slash-commands")
QueryOptions(disabled_slash_commands=["init", "vim"])

Also update the assertion:

    assert args[idx + 1] == "init,vim"

— qwen3.7-max via Qwen Code /review

pathToQwenExecutable: 'qwen',
disabledSlashCommands: ['/init', '/vim'],
};

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] Test values ['/init', '/vim'] use slash-prefixed names, but the CLI expects bare names ('init', 'vim'). This propagates the incorrect format as test-as-documentation.

Suggested change
disabledSlashCommands: ['init', 'vim'],

Also update the assertion:

        expect.arrayContaining(['--disabled-slash-commands', 'init,vim']),

— 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.

@wenshao wenshao closed this Jul 7, 2026
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.

3 participants