Skip to content

fix(serve): allow preinstall skill batch toggles #9139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
callmeYe merged 1 commit into from
Aug 14, 2026
Merged

fix(serve): allow preinstall skill batch toggles #9139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions docs/design/daemon-skill-batch-toggle.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,16 @@ The request body is:

`skillNames` is a non-empty string array with at most 100 entries. Names are
trimmed and deduplicated case-insensitively while preserving first-seen order.
The response is best-effort for expected target errors: valid targets are
validated against one status snapshot, persisted in one locked write, and
applied with one live-session refresh. Unknown, hidden, inactive-extension,
and locked targets are returned without blocking the valid targets. Unexpected
persistence and runtime-generation failures fail the whole request.
The response is best-effort for expected target errors: installed targets are
validated against one status snapshot, all valid names are persisted in one
locked write, and changes are applied with one live-session refresh. Names
that are not installed remain valid so callers can declare their state before
installation. Enabling one removes a matching workspace `skills.disabled`
Comment on lines +30 to +31

@ytahdn ytahdn Aug 14, 2026

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 batch contract changed but the canonical protocol reference and the SDK batch error union were not updated — Failure scenario: a client/SDK implementer working from docs/developers/qwen-serve-protocol.md § POST /workspace/skills/enable writes a handler for errors[].code === 'skill_not_found' in batch responses to detect typos; it never fires after this PR, while a misspelled name sent with enabled: false is silently persisted into workspace skills.disabled with changed: true and HTTP 200. The protocol doc's example response (missing → skill_not_found in errors) describes a shape the endpoint no longer produces. Suggested fix: update the protocol doc's batch section to the new semantics (unknown names are valid targets; disable writes skills.disabled, enable is a no-op that removes a matching entry), including the example response, drop skill_not_found from the batch target-error sentence, and trim 'skill_not_found' from DaemonSkillBatchToggleErrorCode in packages/sdk-typescript/src/daemon/types.ts — it is unreachable for the batch endpoint (the single-skill 404 is a separate HTTP surface).

中文说明

批量端点契约已变更，但规范文档与 SDK 类型未同步：docs/developers/qwen-serve-protocol.md 的 POST /workspace/skills/enable 章节仍记录未知名称返回 skill_not_found（示例响应仍展示 missing → errors），DaemonSkillBatchToggleErrorCode 也仍包含该错误码。按文档实现的客户端将永远收不到该错误，而拼错的名称会在 enabled: false 时被静默写入 skills.disabled（changed: true, HTTP 200）。建议同步更新协议文档（未知名称是合法目标、禁用写 skills.disabled、启用是移除匹配项的 no-op），并从 SDK 批量错误联合类型中移除 'skill_not_found'。

— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.11)

entry and is otherwise a no-op, except for the existing `defaultDisabled`
override behavior; disabling one writes `skills.disabled`. Hidden,
inactive-extension, and locked targets are returned without blocking valid
targets. Unexpected persistence and runtime-generation failures fail the whole
request.

```json
{
Expand All @@ -46,15 +51,14 @@ persistence and runtime-generation failures fail the whole request.
"skillName": "deploy",
"enabled": false,
"changed": true
}
],
"errors": [
},
{
"skillName": "missing",
"code": "skill_not_found",
"error": "Skill not found: missing"
"enabled": false,
"changed": true
}
]
],
"errors": []
}
```

Expand Down
20 changes: 9 additions & 11 deletions packages/cli/src/serve/routes/workspace-skills.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,20 +173,18 @@ describe('workspace Skill management routes', () => {
expect(harness.deleteWorkspaceSkill).not.toHaveBeenCalled();
});

it('toggles a deduplicated Skill batch and returns per-target errors', async () => {
it('toggles a deduplicated Skill batch and returns per-target outcomes', async () => {
const harness = createHarness();
harness.setWorkspaceSkillsEnabled.mockResolvedValueOnce({
enabled: false,
activation: 'applied',
sessionsRefreshed: 1,
sessionsFailed: 0,
results: [{ skillName: 'review', enabled: false, changed: true }],
results: [
{ skillName: 'review', enabled: false, changed: true },
{ skillName: 'missing', enabled: false, changed: true },
],
errors: [
{
skillName: 'missing',
code: 'skill_not_found',
error: 'Skill not found: missing',
},
{
skillName: 'locked',
code: 'skill_not_toggleable',
Expand Down Expand Up @@ -216,13 +214,13 @@ describe('workspace Skill management routes', () => {
enabled: false,
changed: true,
},
],
errors: [
{
skillName: 'missing',
code: 'skill_not_found',
error: 'Skill not found: missing',
enabled: false,
changed: true,
},
],
errors: [
{
skillName: 'locked',
code: 'skill_not_toggleable',
Expand Down
32 changes: 26 additions & 6 deletions packages/cli/src/serve/run-qwen-serve.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,30 @@ describe('workspace skill settings persistence', () => {
expect(savedUser.skills.disabled).toEqual(['locked-skill']);
expect(savedUser.skills.enabled).toBeUndefined();

const preinstallNoop = await persistDisabledSkillsBatch!(
workspace,
['future-skill'],
true,
);
expect(preinstallNoop.outcomes).toEqual([
{ skillName: 'future-skill', changed: false },
]);
expect(preinstallNoop.settingsChanges).toEqual([]);
expect(setValues).toHaveBeenCalledOnce();

const preinstallEnable = await persistDisabledSkillsBatch!(
workspace,
['orphan'],
true,
);
expect(preinstallEnable.outcomes).toEqual([
{ skillName: 'orphan', changed: true },
]);
expect(preinstallEnable.settingsChanges).toEqual([
{ key: 'skills.disabled', value: ['review', 'alpha'] },
]);
expect(setValues).toHaveBeenCalledTimes(2);

const enableResult = await persistDisabledSkillsBatch!(
workspace,
['opt-in'],
Expand All @@ -929,16 +953,12 @@ describe('workspace skill settings persistence', () => {
value: ['opt-in'],
},
]);
expect(setValues).toHaveBeenCalledTimes(2);
expect(setValues).toHaveBeenCalledTimes(3);

const savedAfterEnable = JSON.parse(
fs.readFileSync(path.join(workspace, '.qwen', 'settings.json'), 'utf8'),
) as { skills: { disabled: string[]; enabled: string[] } };
expect(savedAfterEnable.skills.disabled).toEqual([
'orphan',
'review',
'alpha',
]);
expect(savedAfterEnable.skills.disabled).toEqual(['review', 'alpha']);
expect(savedAfterEnable.skills.enabled).toEqual(['opt-in']);

const guard = vi.fn();
Expand Down
62 changes: 43 additions & 19 deletions packages/cli/src/serve/workspace-service/__tests__/facade.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2098,6 +2098,38 @@ describe('createDaemonWorkspaceService', () => {
},
];

it('accepts enabling a Skill before installation as an idempotent result', async () => {
const persistDisabledSkillsBatch = vi.fn().mockResolvedValue({
outcomes: [{ skillName: 'future-skill', changed: false }],
settingsChanges: [],
});
const svc = createDaemonWorkspaceService(
makeDeps({
queryWorkspaceStatus: vi.fn().mockResolvedValue({
v: 1,
workspaceCwd: '/workspace',
initialized: true,
skills,
}),
persistDisabledSkillsBatch,
isChannelLive: () => false,
}),
);

await expect(
svc.setWorkspaceSkillsEnabled(makeCtx(), ['future-skill'], true),
).resolves.toMatchObject({
results: [{ skillName: 'future-skill', enabled: true, changed: false }],
errors: [],
});
expect(persistDisabledSkillsBatch).toHaveBeenCalledWith(
'/workspace',
['future-skill'],
true,
undefined,
);
});

it('persists and refreshes once while preserving ordered target outcomes', async () => {
const queryWorkspaceStatus = vi.fn().mockResolvedValue({
v: 1,
Expand All @@ -2108,6 +2140,7 @@ describe('createDaemonWorkspaceService', () => {
const persistDisabledSkillsBatch = vi.fn().mockResolvedValue({
outcomes: [
{ skillName: 'review', changed: true },
{ skillName: 'missing', changed: true },
{
skillName: 'locked',
error: new WorkspaceSkillNotToggleableError(
Expand All @@ -2119,7 +2152,10 @@ describe('createDaemonWorkspaceService', () => {
{ skillName: 'deploy', changed: true },
],
settingsChanges: [
{ key: 'skills.disabled', value: ['review', 'deploy'] },
{
key: 'skills.disabled',
value: ['review', 'missing', 'deploy'],
},
],
});
const invokeWorkspaceCommand = vi.fn().mockResolvedValue({
Expand Down Expand Up @@ -2147,7 +2183,7 @@ describe('createDaemonWorkspaceService', () => {
expect(persistDisabledSkillsBatch).toHaveBeenCalledOnce();
expect(persistDisabledSkillsBatch).toHaveBeenCalledWith(
'/workspace',
['review', 'locked', 'deploy'],
['review', 'missing', 'locked', 'deploy'],
false,
undefined,
);
Expand All @@ -2163,14 +2199,10 @@ describe('createDaemonWorkspaceService', () => {
sessionsFailed: 0,
results: [
{ skillName: 'review', enabled: false, changed: true },
{ skillName: 'missing', enabled: false, changed: true },
{ skillName: 'deploy', enabled: false, changed: true },
],
errors: [
{
skillName: 'missing',
code: 'skill_not_found',
error: 'Skill not found: missing',
},
{
skillName: 'hidden',
code: 'skill_not_toggleable',
Expand All @@ -2197,7 +2229,7 @@ describe('createDaemonWorkspaceService', () => {
type: 'settings_changed',
data: {
key: 'skills.disabled',
value: ['review', 'deploy'],
value: ['review', 'missing', 'deploy'],
scope: 'workspace',
},
originatorClientId: 'client-1',
Expand Down Expand Up @@ -2225,6 +2257,7 @@ describe('createDaemonWorkspaceService', () => {
),
},
{ skillName: 'review', changed: true },
{ skillName: 'missing', changed: true },
],
settingsChanges: [],
}),
Expand All @@ -2240,6 +2273,7 @@ describe('createDaemonWorkspaceService', () => {

expect(result.results).toEqual([
{ skillName: 'review', enabled: false, changed: true },
{ skillName: 'missing', enabled: false, changed: true },
{ skillName: 'deploy', enabled: false, changed: true },
]);
expect(result.errors).toEqual([
Expand All @@ -2250,11 +2284,6 @@ describe('createDaemonWorkspaceService', () => {
reason: 'locked',
lockedScope: 'user',
},
{
skillName: 'missing',
code: 'skill_not_found',
error: 'Skill not found: missing',
},
]);
});

Expand Down Expand Up @@ -2329,18 +2358,13 @@ describe('createDaemonWorkspaceService', () => {
);

await expect(
svc.setWorkspaceSkillsEnabled(
makeCtx(),
['missing', 'hidden', 'inactive'],
false,
),
svc.setWorkspaceSkillsEnabled(makeCtx(), ['hidden', 'inactive'], false),
).resolves.toMatchObject({
activation: 'applied',
sessionsRefreshed: 0,
sessionsFailed: 0,
results: [],
errors: [
{ skillName: 'missing', code: 'skill_not_found' },
{ skillName: 'hidden', code: 'skill_not_toggleable' },
{ skillName: 'inactive', code: 'skill_inactive_extension' },
],
Expand Down
10 changes: 6 additions & 4 deletions packages/cli/src/serve/workspace-service/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,10 +959,12 @@ export function createDaemonWorkspaceService(
for (const requestedName of requestedSkillNames) {
const normalizedName = requestedName.trim().toLowerCase();
const skill = skillsByName.get(normalizedName);
let domainError: unknown;
if (!skill) {
domainError = new WorkspaceSkillNotFoundError(requestedName);
} else if (skill.userInvocable === false) {
targets.push({ requestedName, skillName: requestedName });
continue;
}
Comment on lines 962 to +965

@ytahdn ytahdn Aug 14, 2026

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] Batch and single-Skill toggle endpoints now diverge for unknown names — Failure scenario: a client toggling an uninstalled skill future-skill gets POST /workspace/skills/future-skill/enable → 404 (not persisted), but POST /workspace/skills/enable with {"skillNames":["future-skill"],"enabled":false} → 200, changed: true, and workspace skills.disabled now contains future-skill. The divergence is deliberate and documented in the design doc, but it is invisible in the client-facing protocol reference and is not pinned by a consistency test. Suggested fix: state the divergence explicitly in the protocol doc (which merges with the batch-section rewrite) and add a consistency test pinning the two endpoints' different handling of the same unknown name — do not align the single endpoint's 404.

中文说明

本 diff 让批量端点接受未安装名称，而单数端点仍对未知名称返回 404 skill_not_found（index.ts:827）。该分裂是设计文档明确支持的刻意设计（单数端点保持不变），但面向客户端的协议文档对此不可见，且没有一致性测试固定两端点对同一未知名称的不同处理。建议：在协议文档中明示该分裂（可与批量章节重写合并），并补充一致性测试；不要改变单数端点的 404 行为。

— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.11)

let domainError: unknown;
if (skill.userInvocable === false) {
domainError = new WorkspaceSkillNotToggleableError(
skill.name,
'not_user_invocable',
Expand Down Expand Up @@ -990,7 +992,7 @@ export function createDaemonWorkspaceService(
if (!error) throw domainError;
targets.push({ requestedName, error });
} else {
targets.push({ requestedName, skillName: skill!.name });
targets.push({ requestedName, skillName: skill.name });
}
}

Expand Down
Loading