Skip to content

Commit a62b568

Browse files
fix: publish the commands when the skills change (#643)
## Problem A skill that appears while a session is open — a new `SKILL.md`, a new symlink into `.agents/skills`, or a link whose target changed — did not show up in the slash-command completion until the next completed turn. The app-server sends `skills/changed` for exactly this, and documents it as "Treat this as an invalidation signal and re-run `skills/list`". The adapter listed it among the ignored notifications, so nothing re-ran. The only re-list was the post-turn publish, which is why the skill eventually appeared — one turn late. ## Change - `CodexAcpServer` handles `skills/changed` on both the baseline turn tracker and the prompt subscription, and asks the command registry to publish for that session. - `CodexCommands.publishOnSkillsChanged` debounces the publish by 300 ms, so one burst of file changes lists the skills once. The timer is `unref`'d, and a session that closed in the meantime publishes nothing. - `CodexEventHandler` gives `skills/changed` its own case instead of leaving it in the ignored block — the change itself is no session update. - The stale comment at the post-turn publish ("Codex sends no notification for a new skill file.") is corrected; that publish stays as the catch-all for a notification missed during a turn. ## Tests Two cases in `CodexAcpClient.test.ts`: a burst of skill changes publishes once after it settles and publishes again when the list actually differs; a session that closed while its change settled never lists. `npx tsc --noEmit` is clean. The three failures in a full `vitest run` (`CodexHomeWatcher`) reproduce on untouched `main` on this machine.
1 parent 59974c7 commit a62b568

5 files changed

Lines changed: 243 additions & 4 deletions

File tree

‎src/CodexAcpServer.ts‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1416,6 +1416,7 @@ export class CodexAcpServer {
14161416
}
14171417
await this.trackCodexTurnStart(sessionState, event);
14181418
await this.trackSteerLanding(sessionState, event);
1419+
this.handleSkillsChanged(sessionState, event);
14191420
// Codex-started turns carry no `userMessage` item; `handleSessionScopedNotification`
14201421
// already drops that item type, so nothing is synthesized here.
14211422
await baselineEventHandler.handleSessionScopedNotification(event);
@@ -1782,6 +1783,9 @@ export class CodexAcpServer {
17821783
this.turnStartQueueTail.delete(params.sessionId);
17831784
this.queuedV2PromptCancellers.delete(params.sessionId);
17841785
this.steeringQueues.delete(params.sessionId);
1786+
if (sessionState) {
1787+
this.availableCommands.cancelPendingSkillsPublish(sessionState);
1788+
}
17851789
}
17861790
this.endSessionCloseFence(params.sessionId);
17871791
}
@@ -2960,6 +2964,24 @@ export class CodexAcpServer {
29602964
await this.publishGoalSnapshot(sessionState, snapshot, force, false);
29612965
}
29622966

2967+
/**
2968+
* Lists the skills of a session again when Codex reports that its skill files changed.
2969+
*
2970+
* The notification carries no thread, so app-server sends it to every subscribed session, and each one lists
2971+
* its own cwds. A skill that appears while a session is open - a new file, a new symlink, a changed link
2972+
* target - becomes a command right away instead of only after the next turn.
2973+
*/
2974+
private handleSkillsChanged(sessionState: SessionState, event: ServerNotification): void {
2975+
if (event.method !== "skills/changed") {
2976+
return;
2977+
}
2978+
const sessionGeneration = this.getSessionGeneration(sessionState.sessionId);
2979+
this.availableCommands.publishOnSkillsChanged(
2980+
sessionState,
2981+
() => this.sessionPublishIsCurrent(sessionState, sessionGeneration),
2982+
);
2983+
}
2984+
29632985
private sessionPublishIsCurrent(sessionState: SessionState, sessionGeneration: number): boolean {
29642986
return this.sessions.get(sessionState.sessionId) === sessionState
29652987
&& this.getSessionGeneration(sessionState.sessionId) === sessionGeneration
@@ -4368,6 +4390,7 @@ export class CodexAcpServer {
43684390
// notifications for as long as no later prompt replaces it.
43694391
await this.trackCodexTurnStart(sessionState, event);
43704392
await this.trackSteerLanding(sessionState, event);
4393+
this.handleSkillsChanged(sessionState, event);
43714394
if (pendingInsertion !== undefined
43724395
&& isInsertedUserMessage(event, params.sessionId, pendingInsertion.clientUserMessageId)) {
43734396
await resolvePendingInsertion();
@@ -4722,11 +4745,13 @@ export class CodexAcpServer {
47224745

47234746
await clearRecoveredSessionFailure(eventHandler);
47244747

4725-
// Codex sends no notification for a new skill file. A skill that appeared during the turn becomes a
4726-
// slash command after it. Never await: the prompt response does not wait for the skill list.
4748+
// A skill that appeared during the turn becomes a slash command after it, even when the
4749+
// `skills/changed` notification of its file was missed. Never await: the prompt response does not
4750+
// wait for the skill list.
4751+
const turnSessionGeneration = this.getSessionGeneration(sessionState.sessionId);
47274752
void this.availableCommands.publish(
47284753
sessionState,
4729-
() => this.sessions.get(sessionState.sessionId) === sessionState,
4754+
() => this.sessionPublishIsCurrent(sessionState, turnSessionGeneration),
47304755
true,
47314756
);
47324757

‎src/CodexCommands.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,15 @@ export type CommandHandleOptions = {
4646

4747
export type LogoutHandler = () => void | Promise<void>;
4848

49+
/** How long a `skills/changed` burst settles before the commands are listed again. */
50+
const SKILLS_CHANGED_PUBLISH_DELAY_MS = 300;
51+
52+
/** How long a `skills/changed` burst may hold the publish back before it goes out anyway. */
53+
const SKILLS_CHANGED_PUBLISH_MAX_DELAY_MS = 2_000;
54+
55+
/** A publish that waits out a burst of skill file changes, and the moment it stops waiting for more. */
56+
type PendingSkillsPublish = {timer: ReturnType<typeof setTimeout>; deadline: number};
57+
4958
export class CodexCommands {
5059
private readonly connection: AcpClientConnection;
5160
private readonly codexAcpClient: CodexAcpClient;
@@ -54,6 +63,10 @@ export class CodexCommands {
5463
private readonly mcpCommand: McpCommand;
5564
/** The commands that each session got last, to skip a publish that changes nothing. */
5665
private readonly published = new WeakMap<SessionState, string>();
66+
/** The pending publish of each session whose skills changed, so one burst of file changes lists once. */
67+
private readonly pendingSkillsPublishes = new WeakMap<SessionState, PendingSkillsPublish>();
68+
/** The publish each session started last, so a slower earlier one does not overwrite its list. */
69+
private readonly publishRevisions = new WeakMap<SessionState, number>();
5770

5871
constructor(
5972
connection: AcpClientConnection,
@@ -69,6 +82,39 @@ export class CodexCommands {
6982
this.mcpCommand = new McpCommand(codexAcpClient, runWithProcessCheck, mcpServerSignIn);
7083
}
7184

85+
/**
86+
* Lists the skills again and publishes the commands once the skill files settle.
87+
*
88+
* Codex watches the skill files and reports every change as `skills/changed`, naming nothing that changed, so
89+
* the list is the only way to learn what a session has now. An editor that writes a skill, or a checkout that
90+
* adds one, fires several of those notifications, so the publish waits out the burst. Each session keeps one
91+
* pending publish, and a publish that changes no command sends nothing.
92+
*/
93+
publishOnSkillsChanged(sessionState: SessionState, shouldPublish: () => boolean): void {
94+
const pending = this.pendingSkillsPublishes.get(sessionState);
95+
clearTimeout(pending?.timer);
96+
// A checkout, or a watcher that keeps writing, would otherwise push the publish out forever.
97+
const deadline = pending?.deadline ?? Date.now() + SKILLS_CHANGED_PUBLISH_MAX_DELAY_MS;
98+
const delay = Math.max(0, Math.min(SKILLS_CHANGED_PUBLISH_DELAY_MS, deadline - Date.now()));
99+
const timer = setTimeout(() => {
100+
this.pendingSkillsPublishes.delete(sessionState);
101+
void this.publish(sessionState, shouldPublish, true);
102+
}, delay);
103+
// The adapter must not stay alive for a publish that no one waits for.
104+
timer.unref?.();
105+
this.pendingSkillsPublishes.set(sessionState, {timer, deadline});
106+
}
107+
108+
/** Drops the publish a closing session was still waiting out. */
109+
cancelPendingSkillsPublish(sessionState: SessionState): void {
110+
const pending = this.pendingSkillsPublishes.get(sessionState);
111+
if (pending === undefined) {
112+
return;
113+
}
114+
clearTimeout(pending.timer);
115+
this.pendingSkillsPublishes.delete(sessionState);
116+
}
117+
72118
/** Sends the available commands. With `onlyChanges`, it sends nothing when the commands did not change. */
73119
async publish(
74120
sessionState: SessionState,
@@ -79,11 +125,17 @@ export class CodexCommands {
79125
if (!shouldPublish()) {
80126
return;
81127
}
128+
const revision = (this.publishRevisions.get(sessionState) ?? 0) + 1;
129+
this.publishRevisions.set(sessionState, revision);
82130
const skillsResponse = await this.runWithProcessCheck(() => this.codexAcpClient.listSkills(this.createSkillsListParams(sessionState)));
83131
const availableCommands = this.buildAvailableCommands(
84132
skillsResponse?.data ?? [],
85133
sessionState.clientCapabilities.airClient,
86134
);
135+
// A publish that started later already listed the skills, and its list is the newer one.
136+
if (this.publishRevisions.get(sessionState) !== revision) {
137+
return;
138+
}
87139
if (availableCommands.length === 0 || !shouldPublish()) {
88140
return;
89141
}

‎src/CodexEventHandler.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -697,6 +697,9 @@ export class CodexEventHandler {
697697
case "account/gatewayOAuth/changed":
698698
// Gateway login state is account-scoped and has no ACP session update counterpart.
699699
return null;
700+
case "skills/changed":
701+
// The server lists the skills again and publishes the commands. The change itself is no update.
702+
return null;
700703
// ignored events
701704
case "thread/deleted":
702705
case "thread/reverted":
@@ -733,7 +736,6 @@ export class CodexEventHandler {
733736
case "thread/realtime/closed":
734737
case "windowsSandbox/setupCompleted":
735738
case "account/login/completed":
736-
case "skills/changed":
737739
case "mcpServer/oauthLogin/completed":
738740
case "externalAgentConfig/import/completed":
739741
case "rawResponseItem/completed":

‎src/__tests__/CodexACPAgent/CodexAcpClient.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2013,6 +2013,36 @@ describe('ACP server test', { timeout: 40_000 }, () => {
20132013
expect(published()).toBe(2);
20142014
});
20152015

2016+
// The event-driven side of `skills/changed` lives in `skills-changed.test.ts`; this covers only the cap
2017+
// that keeps a burst from holding the publish back forever.
2018+
it('publishes a never-ending burst of skill file changes anyway', async () => {
2019+
vi.useFakeTimers();
2020+
try {
2021+
const mockFixture = createCodexMockTestFixture();
2022+
const codexAcpAgent = mockFixture.getCodexAcpAgent();
2023+
const listSkills = vi.spyOn(mockFixture.getCodexAcpClient(), "listSkills")
2024+
.mockResolvedValue({data: [{cwd: "/workspace", skills: [{
2025+
name: "build", description: "build", shortDescription: "build",
2026+
path: "/workspace", scope: "user" as const, enabled: true, pluginId: null,
2027+
}], errors: []}]});
2028+
const sessionState = createTestSessionState({sessionId: "session-id", cwd: "/workspace"});
2029+
// @ts-expect-error - exercising private state
2030+
codexAcpAgent.sessions.set(sessionState.sessionId, sessionState);
2031+
// @ts-expect-error - exercising private helper
2032+
const skillsChanged = () => codexAcpAgent.handleSkillsChanged(sessionState, {method: "skills/changed", params: {}});
2033+
2034+
// A writer that never pauses for 300 ms: a trailing debounce alone would never fire.
2035+
for (let elapsed = 0; elapsed < 2_500; elapsed += 200) {
2036+
skillsChanged();
2037+
await vi.advanceTimersByTimeAsync(200);
2038+
}
2039+
2040+
expect(listSkills).toHaveBeenCalledTimes(1);
2041+
} finally {
2042+
vi.useRealTimers();
2043+
}
2044+
});
2045+
20162046
it('handles builtin slash command locally', async () => {
20172047
const mockFixture = createCodexMockTestFixture();
20182048
const codexAcpAgent = mockFixture.getCodexAcpAgent();
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import {afterEach, describe, expect, it} from 'vitest';
2+
import {connectSession, settle, type PromptSession, type TranscriptEntry} from './v2-prompt-harness';
3+
import type * as acpV2 from '@agentclientprotocol/sdk/experimental/v2';
4+
import type {ServerNotification} from '../../app-server';
5+
6+
/**
7+
* Codex reports every change under a skill root as `skills/changed`, naming nothing that changed. The adapter
8+
* lists the skills again and publishes the slash commands, so a skill that appears while a session is open - a
9+
* new file, a new symlink, a changed link target - becomes a command right away instead of only after the next
10+
* completed turn.
11+
*/
12+
describe('skills/changed republishes the available commands', () => {
13+
let closeClient: (() => void) | null = null;
14+
15+
afterEach(() => {
16+
closeClient?.();
17+
closeClient = null;
18+
});
19+
20+
const skillsChanged = (): ServerNotification =>
21+
({method: "skills/changed", params: {}} as unknown as ServerNotification);
22+
23+
const skills = (...names: string[]) => ({
24+
data: [{
25+
cwd: "/workspace",
26+
errors: [],
27+
skills: names.map(name => ({
28+
name,
29+
description: name,
30+
shortDescription: name,
31+
path: `/workspace/.agents/skills/${name}`,
32+
scope: "project" as const,
33+
enabled: true,
34+
pluginId: null,
35+
})),
36+
}],
37+
});
38+
39+
const commandNames = (transcript: TranscriptEntry[]): string[][] =>
40+
transcript.flatMap(entry =>
41+
"sessionUpdate" in entry && entry.sessionUpdate.sessionUpdate === "available_commands_update"
42+
? [(entry.sessionUpdate.availableCommands as acpV2.AvailableCommand[]).map(command => command.name)]
43+
: []);
44+
45+
/** Waits out the publish the notification debounces, plus the wire round-trip behind it. */
46+
const settleSkillsPublish = async (client: PromptSession) => {
47+
await new Promise(resolve => setTimeout(resolve, 400));
48+
await settle();
49+
};
50+
51+
it('publishes a skill that appeared before any turn has run', async () => {
52+
const client = await connectSession();
53+
closeClient = () => client.connection.close();
54+
client.setCodexResponse("skills/list", async () => skills("deploy"));
55+
const start = client.transcript.length;
56+
57+
client.emit(skillsChanged());
58+
await settleSkillsPublish(client);
59+
60+
expect(commandNames(client.transcript.slice(start)).at(-1)).toContain("$deploy");
61+
});
62+
63+
it('publishes once for a burst of changes, and nothing when the skills are the same', async () => {
64+
const client = await connectSession();
65+
closeClient = () => client.connection.close();
66+
let listed = 0;
67+
client.setCodexResponse("skills/list", async () => {
68+
listed += 1;
69+
return skills("deploy");
70+
});
71+
72+
client.emit(skillsChanged());
73+
client.emit(skillsChanged());
74+
client.emit(skillsChanged());
75+
await settleSkillsPublish(client);
76+
77+
expect(listed).toBe(1);
78+
const start = client.transcript.length;
79+
80+
// The same skills: the list is read again, but the client already has those commands.
81+
client.emit(skillsChanged());
82+
await settleSkillsPublish(client);
83+
84+
expect(listed).toBe(2);
85+
expect(commandNames(client.transcript.slice(start))).toEqual([]);
86+
});
87+
88+
it('keeps the newer list when a slower earlier publish answers last', async () => {
89+
const client = await connectSession();
90+
closeClient = () => client.connection.close();
91+
let listed = 0;
92+
client.setCodexResponse("skills/list", async () => {
93+
listed += 1;
94+
if (listed === 1) {
95+
// The first list is answered long after the second one, and it carries the older skills.
96+
await new Promise(resolve => setTimeout(resolve, 1_000));
97+
return skills("build");
98+
}
99+
return skills("build", "deploy");
100+
});
101+
const start = client.transcript.length;
102+
103+
client.emit(skillsChanged());
104+
await new Promise(resolve => setTimeout(resolve, 400));
105+
client.emit(skillsChanged());
106+
await new Promise(resolve => setTimeout(resolve, 1_500));
107+
await settle();
108+
109+
expect(listed).toBe(2);
110+
expect(commandNames(client.transcript.slice(start)).at(-1)).toContain("$deploy");
111+
});
112+
113+
it('publishes nothing for a session that closed before its change settled', async () => {
114+
const client = await connectSession();
115+
closeClient = () => client.connection.close();
116+
let listed = 0;
117+
client.setCodexResponse("skills/list", async () => {
118+
listed += 1;
119+
return skills("deploy");
120+
});
121+
const listedAtClose = () => listed;
122+
123+
client.emit(skillsChanged());
124+
await client.request("session/close", {sessionId: "thread-1"});
125+
const before = listedAtClose();
126+
await settleSkillsPublish(client);
127+
128+
expect(listed).toBe(before);
129+
});
130+
});

0 commit comments

Comments
 (0)