fix(core): handle error variant in disabled skill command delegation#4804
Merged
Conversation
Contributor
📋 Review SummaryThis PR fixes a TypeScript build error introduced by #4532, where the 🔍 General Feedback
🎯 Specific Feedback🔴 CriticalNo critical issues identified. 🟡 HighNo high priority issues identified. 🟢 MediumNo medium priority issues identified. 🔵 Low
✅ Highlights
|
Collaborator
|
Thanks for the quick fix on the build break — this is clearly urgent since it's blocking all CI on main. Template check: needs update The PR body is missing several required sections from the PR template:
For a type-only build fix, most of these are quick fills: "How to verify" can be On direction: clearly aligned — fixing a build break from #4532/#4533 is exactly what we need right now. No concerns. On approach: the fix correctly mirrors the existing error-object handling pattern at lines 494–506. One note: the diff removes the comment block explaining why Once the template is filled in and the comment restored, this should be ready to go. 中文说明感谢快速修复构建错误——这很紧急，因为它阻塞了 main 上的所有 CI。 模板检查：需要更新 PR 描述缺少 PR 模板 中的以下必填部分：
对于纯类型修复，这些可以快速填写："How to verify" 可以是 方向： 完全对齐——修复 #4532/#4533 引起的构建中断正是当前需要的。没有顾虑。 方案： 修复正确地复用了 494–506 行已有的 error 对象处理模式。注意一点：diff 移除了说明为何在委派路径中不触发 模板补全、注释恢复后应该就可以合入了。 — Qwen Code · qwen3.7-max |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a TypeScript type error in the disabled-skill delegation path of SkillToolInvocation.execute() after ModelInvocableCommandExecutorResult was widened to string | { error: string }. The change ensures the delegation result is correctly handled when the executor returns an error object, aligning behavior with the existing non-disabled command execution handling in the same file.
Changes:
- Add an
{ error: string }handling branch forcommandExecutor()results when a skill is disabled and delegation occurs. - Return the error text directly for both
llmContentandreturnDisplaywhen delegation reports an error.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
qwen-code-ci-bot
requested changes
Jun 5, 2026
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Template sections missing (Reviewer Test Plan, Risk & Scope, 中文说明). Please fill in the required sections from the PR template. Also preserve the removed design-rationale comment. See my review comment above for details. 🙏
…t in disabled skill path
The disabled-skill command delegation (line 457) used the executor
result directly as text content, but ModelInvocableCommandExecutorResult
is `string | { error: string }`. Handle the error object case the same
way the non-disabled path does (lines 494-506).
Fixes build break introduced by #4532 merging after the type was widened.
abb94a7 to
b4c4a17
Compare
Collaborator
Author
|
@qwen-code /triage |
Collaborator
Code ReviewIndependent proposal: given a TS2322 where Diff assessment: the PR matches this approach precisely. It inserts a No correctness bugs, no security concerns, no over-abstraction. One file, +6 lines, zero deletions. Clean. VerificationBefore (main, without PR): After (with PR applied): Tmux testing is N/A here — this is a type-only fix with no runtime behavior change. The typecheck output above is the evidence. CI checks on the PR itself: 4/7 passing (Lint, Classify, CodeQL, CodeQL duplicate), 3 pending (Test on macOS/Windows/Ubuntu). No failures. 中文说明代码审查独立方案： 已知 Diff 评估： PR 精确匹配此方案。在现有的 无正确性 bug、无安全问题、无过度抽象。单文件，+6 行，0 删除。干净。 验证修复前（main，未应用 PR）： tsc 报 TS2322 错误（见上方输出） 修复后（应用 PR）： tsc 通过，exit code 0 Tmux 测试 N/A——这是纯类型修复，无运行时行为变化。上述 typecheck 输出即为证据。 PR 上的 CI 检查：4/7 通过（Lint、Classify、CodeQL x2），3 个待完成（macOS/Windows/Ubuntu 测试）。无失败。 — Qwen Code · qwen3.7-max |
Collaborator
|
Stepping back: this is a textbook blocking-fix PR. The build break is real (reproduced TS2322 on main), the fix is the minimum viable change (+6 lines, one guard clause), and it mirrors the exact pattern already established in the same file. No scope creep, no speculative abstractions, no runtime behavior change. The independent proposal I wrote before reading the diff was identical to what the PR does — which is a good sign. There's no simpler path I can see. Verified: typecheck fails before, passes after. CI on the PR shows no failures (3 test jobs still pending). Verdict: approve and merge. This is blocking every other PR targeting main. 中文说明退后一步看：这是一个教科书式的阻塞性修复 PR。构建中断是真实的（在 main 上复现了 TS2322），修复是最小可行变更（+6 行，一个守卫子句），并且完全复用了同文件中已有的模式。没有范围蔓延、没有投机性抽象、没有运行时行为变化。 在阅读 diff 之前我写的独立方案与 PR 的做法完全一致——这是个好信号。没有更简单的路径了。 验证：修复前 typecheck 失败，修复后通过。PR 上的 CI 无失败（3 个测试任务仍在运行中）。 结论： 批准并合入。这正在阻塞所有指向 main 的其他 PR。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 5, 2026
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
approved these changes
Jun 5, 2026
wenshao
reviewed
Jun 5, 2026
| // track via `onSkillLoaded` — no skill body was loaded, and | ||
| // conflating the two would inflate skill telemetry / | ||
| // `/context` skill-token attribution with command runs. | ||
| if (typeof content === 'object' && 'error' in content) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The new { error: string } branch (lines 464-469) has no test coverage. The disabled-skill execute guard describe block covers string-return, null-executor, null-return, and throw scenarios, but none exercise commandExecutor returning { error: '...' }.
The non-disabled path has an analogous test (logs prompt attribution when executor returns an error in skill.test.ts), proving the pattern is testable. Without a test, a future refactor that breaks the type guard or the return shape would go undetected.
Consider adding a test to the disabled-skill execute guard describe block:
it('returns the error message when commandExecutor returns an error object for a disabled skill', async () => {
vi.mocked(config.getDisabledSkillNames).mockReturnValue(
new Set(['mytool']),
);
const executor = vi.fn().mockResolvedValue({
error: 'Blocked by policy',
});
vi.mocked(config.getModelInvocableCommandsExecutor).mockReturnValue(
executor,
);
const invocation = (
skillTool as SkillToolWithProtectedMethods
).createInvocation({ skill: 'mytool' });
const result = await invocation.execute();
expect(mockSkillManager.loadSkillForRuntime).not.toHaveBeenCalled();
expect(executor).toHaveBeenCalledWith('mytool');
const llmText = partToString(result.llmContent);
expect(llmText).toBe('Blocked by policy');
expect(result.returnDisplay).toBe('Blocked by policy');
});— qwen3.7-max via Qwen Code /review
DragonnZhang
approved these changes
Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Fixes a TypeScript build error in
packages/core/src/tools/skill.tswhere the disabled-skill command delegation path directly usedModelInvocableCommandExecutorResultas text content, but the type includes an error object variant ({ error: string }).Adds proper type narrowing to handle both the
stringand{ error: string }cases, matching the existing pattern at lines 494-506.Why it's needed
All CI checks on PRs targeting main are failing with:
Reviewer Test Plan
How to verify
Evidence (Before & After)
tscfails with TS2322 on skill.ts line 465tscpasses (type narrowing handles both variants)Tested on
Risk & Scope
Linked Issues
Build break from #4532/#4533
中文说明
修复
packages/core/src/tools/skill.ts中的 TypeScript 构建错误。disabled skill 的命令委派路径直接把ModelInvocableCommandExecutorResult当 string 用，但类型已扩展为string | { error: string }。添加了类型收窄，处理两种情况，与 494-506 行的现有模式一致。
此错误阻塞了所有 PR 的 CI 检查。