fix(web-shell): keep skill slash commands after starting a new session (#6319)

* fix(web-shell): keep skill slash commands after starting a new session

Starting a new session (the sidebar button, quick action, and the /new,
/reset and /clear commands all route through clearSession) ran
getConnectionAfterSessionClear, which deleted connection.commands and
connection.skills. Nothing repopulated them: the SSE loop returns early on
manualSessionClear and the deferred skill-fetch path never re-runs, so before
the new session's first prompt the composer fell back to the hardcoded local
command list. That list omits skills, so typing "/rev" and pressing Tab would
not complete "/review".

Preserve the workspace-scoped commands and skills across a clear (skills,
custom, MCP-prompt and workflow slash commands all live at the workspace/config
level, not the session), and only drop the session-scoped supportedCommands and
context snapshots. This keeps skill-backed slash commands autocompleting in the
new deferred session before its first prompt — the same guarantee #6153 added
for the initial deferred connect — while still forcing the next session to
refetch fresh metadata. The next session's available_commands_update refreshes
the list once it lands.

* fix(web-shell): treat a fulfilled empty command snapshot as authoritative

Address review feedback on the new-session command fix. Preserving commands
across a clear means a later refresh must be able to clear them again when the
workspace command list genuinely shrinks to empty, otherwise the preserved
entries would keep autocompleting forever.

Both refresh paths previously kept the previous list on an empty result
(`commands.length > 0 ? commands : current.commands`):

- The streamed available_commands_update handler now assigns the mapped
  commands directly, matching how skills were already handled — the daemon
  snapshot is authoritative.
- The post-attach supported-commands assignment now falls back to the
  preserved list only when the fetch was skipped or failed
  (supportedCommands === undefined), not when it returned an empty list.

Add tests: an available_commands_update that empties the list clears stale
commands; a fulfilled-empty supported-commands fetch after a clear drops the
preserved commands; and getConnectionAfterSessionClear is exercised with the
commands/skills fields already absent.

* test(web-shell): cover supported-commands fetch failure after a clear

Add error-path coverage for the post-attach command assignment: when the
new session's supportedCommands() rejects, supportedCommands stays undefined
and the commands preserved across the clear must survive rather than being
wiped. Complements the fulfilled-empty test, which locks that a successful
empty snapshot is instead treated as authoritative.
This commit is contained in:
Shaojin Wen 2026-07-05 10:47:05 +08:00 committed by GitHub
parent e3906392b5
commit 54ba259106
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 325 additions and 5 deletions

View file

@ -4355,6 +4355,204 @@ describe('DaemonSessionProvider', () => {
).not.toHaveBeenCalled();
});
it('keeps workspace skill slash commands after clearing so /review still autocompletes', async () => {
const firstSession = createMockSession({
sessionId: 'session-a',
supportedCommands: vi.fn(async () => ({
v: 1 as const,
sessionId: 'session-a',
availableCommands: [],
availableSkills: ['review'],
})),
});
sdkMocks.sessions.push(firstSession);
let actions: DaemonSessionActions | undefined;
let connection: DaemonConnectionState | undefined;
function Harness() {
actions = useDaemonActions();
connection = useDaemonConnection();
return null;
}
await renderWithProvider(<Harness />, {
autoConnect: true,
sessionId: 'session-a',
});
await act(async () => {
await flushPromises();
});
expect(connection?.commands?.map((command) => command.name)).toContain(
'review',
);
expect(connection?.skills).toContain('review');
await act(async () => {
await actions?.clearSession();
});
for (let i = 0; i < 10 && connection?.sessionId !== undefined; i++) {
await act(async () => {
await wait(5);
await flushPromises();
});
}
// Clearing returns to the deferred pre-first-prompt state (no sessionId),
// but the workspace-scoped skill command must survive so '/rev' + Tab keeps
// completing '/review' in the new session before its first prompt creates a
// session (matches the initial deferred-connect guarantee from #6153).
expect(connection).not.toHaveProperty('sessionId');
expect(connection?.commands?.map((command) => command.name)).toContain(
'review',
);
expect(connection?.skills).toContain('review');
// The session-scoped raw snapshots are still dropped so the next session
// refetches instead of reusing stale metadata.
expect(connection).not.toHaveProperty('supportedCommands');
expect(connection).not.toHaveProperty('context');
});
it('drops preserved commands when the next session reports an empty list', async () => {
const firstSession = createMockSession({
sessionId: 'session-a',
supportedCommands: vi.fn(async () => ({
v: 1 as const,
sessionId: 'session-a',
availableCommands: [
{ name: 'cmd-a', description: 'Command A', input: null },
],
availableSkills: ['review'],
})),
});
const emptySession = createMockSession({
sessionId: 'session-b',
supportedCommands: vi.fn(async () => ({
v: 1 as const,
sessionId: 'session-b',
availableCommands: [],
availableSkills: [],
})),
});
// session-a loads on mount; the detached session created after the clear
// pops session-b next.
sdkMocks.sessions.push(firstSession, emptySession);
let actions: DaemonSessionActions | undefined;
let connection: DaemonConnectionState | undefined;
function Harness() {
actions = useDaemonActions();
connection = useDaemonConnection();
return null;
}
await renderWithProvider(<Harness />, {
autoConnect: true,
sessionId: 'session-a',
});
await act(async () => {
await flushPromises();
});
expect(connection?.commands?.map((command) => command.name)).toContain(
'cmd-a',
);
await act(async () => {
await actions?.clearSession();
});
// The preserved list keeps autocompleting through the clear (commands is
// still present here)...
expect(connection?.commands?.map((command) => command.name)).toContain(
'cmd-a',
);
// ...but once the next session's supported-commands fetch comes back empty,
// that fulfilled snapshot is authoritative — the stale commands must not
// survive it.
const providerActions = requireActions(actions);
await act(async () => {
await providerActions.createSession();
});
let attach: Promise<void> | undefined;
act(() => {
attach = providerActions.attachSession();
});
await act(async () => {
await flushPromises();
});
await attach;
expect(connection).toMatchObject({ sessionId: 'session-b' });
expect(connection?.commands).toEqual([]);
expect(connection?.skills).toEqual([]);
});
it('keeps preserved commands when the next supported-commands fetch fails', async () => {
const firstSession = createMockSession({
sessionId: 'session-a',
supportedCommands: vi.fn(async () => ({
v: 1 as const,
sessionId: 'session-a',
availableCommands: [
{ name: 'cmd-a', description: 'Command A', input: null },
],
availableSkills: ['review'],
})),
});
const failingSession = createMockSession({
sessionId: 'session-b',
supportedCommands: vi.fn(async () => {
throw new Error('supported-commands unavailable');
}),
});
sdkMocks.sessions.push(firstSession, failingSession);
let actions: DaemonSessionActions | undefined;
let connection: DaemonConnectionState | undefined;
function Harness() {
actions = useDaemonActions();
connection = useDaemonConnection();
return null;
}
await renderWithProvider(<Harness />, {
autoConnect: true,
sessionId: 'session-a',
});
await act(async () => {
await flushPromises();
});
expect(connection?.commands?.map((command) => command.name)).toContain(
'cmd-a',
);
await act(async () => {
await actions?.clearSession();
});
// A skipped or failed fetch is not authoritative — unlike a fulfilled empty
// snapshot it must not clobber the preserved list. supportedCommands()
// rejecting here leaves supportedCommands === undefined, so the commands
// survive rather than being wiped.
const providerActions = requireActions(actions);
await act(async () => {
await providerActions.createSession();
});
let attach: Promise<void> | undefined;
act(() => {
attach = providerActions.attachSession();
});
await act(async () => {
await flushPromises();
});
await attach;
expect(connection).toMatchObject({ sessionId: 'session-b' });
expect(connection?.commands?.map((command) => command.name)).toContain(
'cmd-a',
);
expect(connection?.skills).toContain('review');
});
it('ignores streamed events from a session after it is cleared', async () => {
const streamStarted = createDeferred<void>();
const releaseOldEvent = createDeferred<void>();

View file

@ -840,8 +840,16 @@ export function DaemonSessionProvider(props: DaemonSessionProviderProps) {
? { clientId: activeSession.clientId }
: {}),
workspaceCwd: activeSession.workspaceCwd,
commands: commands.length > 0 ? commands : current.commands,
skills: skills.length > 0 ? skills : current.skills,
// A fulfilled supported-commands fetch is authoritative even when
// it returns an empty list: fall back to the preserved
// `current.commands` only when the fetch was skipped or failed
// (supportedCommands === undefined). Keying on length instead
// would let a genuinely-empty snapshot leave a stale command list
// in place (see getConnectionAfterSessionClear, which now
// preserves commands across a clear).
commands:
supportedCommands !== undefined ? commands : current.commands,
skills: supportedCommands !== undefined ? skills : current.skills,
models: sessionModels.length > 0 ? sessionModels : current.models,
currentModel: sessionCurrentModel ?? current.currentModel,
currentMode: currentMode ?? current.currentMode,

View file

@ -43,6 +43,36 @@ describe('getConnectionAfterSessionClear', () => {
expect(next).not.toHaveProperty('clientId');
expect(next).not.toHaveProperty('displayName');
expect(next).not.toHaveProperty('tokenCount');
expect(next).not.toHaveProperty('supportedCommands');
expect(next).not.toHaveProperty('context');
// Workspace-scoped slash commands and skills survive a clear so skill-backed
// commands (e.g. /review) keep autocompleting in the fresh deferred session
// before its first prompt creates a session (mirrors #6153 / #6066).
expect(next.commands).toEqual([commandInfo('old-command')]);
expect(next.skills).toEqual(['old-skill']);
});
it('handles commands and skills being undefined before clear', () => {
// Optional fields: clearing before the first available_commands_update
// (open the app, immediately start a new chat) leaves them absent. The
// delete calls are harmless no-ops and nothing is fabricated.
const next = getConnectionAfterSessionClear(
{
status: 'disconnected',
workspaceCwd: '/workspace',
sessionId: 'session-a',
clientId: 'client-a',
supportedCommands: supportedCommandsStatus('session-a'),
context: contextStatus('session-a'),
} as DaemonConnectionState,
'session-a',
);
expect(next).toMatchObject({
status: 'connected',
workspaceCwd: '/workspace',
});
expect(next).not.toHaveProperty('sessionId');
expect(next).not.toHaveProperty('commands');
expect(next).not.toHaveProperty('skills');
expect(next).not.toHaveProperty('supportedCommands');

View file

@ -81,10 +81,19 @@ export function getConnectionAfterSessionClear(
delete next.displayName;
delete next.tokenUsage;
delete next.tokenCount;
delete next.commands;
delete next.skills;
// Drop the session-scoped raw snapshots (both carry the cleared
// sessionId), which also makes the effect's canReuseSessionMetadata
// check refetch fresh data for the next session.
delete next.supportedCommands;
delete next.context;
// Keep `commands`/`skills`: they are workspace-scoped (skills, custom,
// MCP-prompt and workflow slash commands all live at the workspace/config
// level, not the session), so they stay valid after the session is
// cleared. Clearing starts a fresh deferred session that is not created
// until the first prompt (#6066); preserving these keeps skill-backed
// slash commands like /review autocompleting in that window — the same
// guarantee #6153 added for the initial deferred connect. The next
// session's available_commands_update refreshes them once it lands.
}
return {
...next,

View file

@ -13,7 +13,38 @@ import {
getReplayTokenCount,
getReplayTokenUsage,
mapWorkspaceSkills,
updateConnectionFromDaemonEvent,
} from './mappers.js';
import type { DaemonConnectionState } from './types.js';
function availableCommandsEvent(
availableCommands: Array<Record<string, unknown>>,
availableSkills: string[],
): DaemonEvent {
return {
id: 1,
v: 1,
type: 'session_update',
data: {
update: {
sessionUpdate: 'available_commands_update',
availableCommands,
availableSkills,
},
},
} as DaemonEvent;
}
function applyEvent(
current: DaemonConnectionState,
event: DaemonEvent,
): DaemonConnectionState {
let next = current;
updateConnectionFromDaemonEvent(event, (update) => {
next = typeof update === 'function' ? update(next) : update;
});
return next;
}
function usageEvent(
id: number,
@ -217,3 +248,42 @@ describe('mapWorkspaceSkills', () => {
]);
});
});
describe('updateConnectionFromDaemonEvent', () => {
it('replaces commands and skills from an available_commands_update', () => {
const next = applyEvent(
{ status: 'connected', workspaceCwd: '/workspace' },
availableCommandsEvent(
[{ name: 'review', description: 'Review a PR', input: null }],
['review'],
),
);
expect(next.commands?.map((command) => command.name)).toEqual(['review']);
expect(next.skills).toEqual(['review']);
});
it('clears stale commands when the update reports an empty list', () => {
// The daemon snapshot is authoritative: a list that shrank to empty must
// not leave the previous commands autocompleting. Keying on length would
// preserve the stale entries.
const next = applyEvent(
{
status: 'connected',
workspaceCwd: '/workspace',
commands: [
{
name: 'review',
description: '',
raw: { name: 'review', description: '', input: null },
},
],
skills: ['review'],
},
availableCommandsEvent([], []),
);
expect(next.commands).toEqual([]);
expect(next.skills).toEqual([]);
});
});

View file

@ -242,9 +242,14 @@ export function updateConnectionFromDaemonEvent(
}
if (getString(update, 'sessionUpdate') === 'available_commands_update') {
const { commands, skills } = mapAvailableCommandsUpdate(update);
// An available_commands_update is the daemon's authoritative snapshot of
// the current slash commands, so assign it directly (matching `skills`)
// rather than keeping the previous list when it is empty — otherwise a
// command list that shrank to empty would leave stale entries
// autocompleting.
setConnection((current) => ({
...current,
commands: commands.length > 0 ? commands : current.commands,
commands,
skills,
}));
}