Prioritize the spec ones over the meta ones

Author: benbrandt

src/CodexAcpClient.ts

@@ -205,8 +205,8 @@ export class CodexAcpClient {
     }
 
     async resumeSession(request: acp.ResumeSessionRequest, onSubscribed?: () => void): Promise<SessionMetadata> {
-        const additionalDirectories = readAdditionalDirectories(request.cwd, request.additionalDirectories);
-        await this.refreshSkills(request.cwd, additionalDirectories, request._meta);
+        const additionalDirectories = readAdditionalDirectories(request.cwd, request.additionalDirectories, request._meta);
+        await this.refreshSkills(request.cwd, additionalDirectories);
 
         const response = await this.codexClient.threadResume({
             config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []),
@@ -227,8 +227,8 @@ export class CodexAcpClient {
     }
 
     async loadSession(request: acp.LoadSessionRequest, onSubscribed?: () => void): Promise<SessionMetadataWithThread> {
-        const additionalDirectories = readAdditionalDirectories(request.cwd, request.additionalDirectories);
-        await this.refreshSkills(request.cwd, additionalDirectories, request._meta);
+        const additionalDirectories = readAdditionalDirectories(request.cwd, request.additionalDirectories, request._meta);
+        await this.refreshSkills(request.cwd, additionalDirectories);
 
         const response = await this.codexClient.threadResume({
             config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []),
@@ -250,8 +250,8 @@ export class CodexAcpClient {
     }
 
     async newSession(request: acp.NewSessionRequest): Promise<SessionMetadata> {
-        const additionalDirectories = readAdditionalDirectories(request.cwd, request.additionalDirectories);
-        await this.refreshSkills(request.cwd, additionalDirectories, request._meta);
+        const additionalDirectories = readAdditionalDirectories(request.cwd, request.additionalDirectories, request._meta);
+        await this.refreshSkills(request.cwd, additionalDirectories);
 
         const response = await this.codexClient.threadStart({
             config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers),
@@ -365,23 +365,19 @@ export class CodexAcpClient {
 
     private async refreshSkills(
         cwd: string,
-        additionalDirectories: string[],
-        meta?: Record<string, unknown> | null
+        additionalRoots: string[]
     ): Promise<void> {
         if (!cwd) {
             return;
         }
 
-        const additionalRoots = uniqueStrings([
-            ...readAdditionalRoots(meta),
-            ...additionalDirectories,
-        ]).map(root => path.join(root, ".agents", "skills"));
-        if (!arraysEqual(this.skillExtraRoots, additionalRoots)) {
-            await this.codexClient.skillsExtraRootsSet({ extraRoots: additionalRoots });
-            this.skillExtraRoots = additionalRoots;
+        const skillExtraRoots = additionalRoots.map(root => path.join(root, ".agents", "skills"));
+        if (!arraysEqual(this.skillExtraRoots, skillExtraRoots)) {
+            await this.codexClient.skillsExtraRootsSet({ extraRoots: skillExtraRoots });
+            this.skillExtraRoots = skillExtraRoots;
         }
         await this.codexClient.listSkills({
-            cwds: [cwd, ...additionalDirectories],
+            cwds: [cwd, ...additionalRoots],
             forceReload: true,
         });
     }
@@ -493,8 +489,7 @@ export class CodexAcpClient {
     ): Promise<TurnCompletedNotification | null> {
         const input = buildPromptItems(request.prompt);
         const effort = modelId.effort as ReasoningEffort | null; //TODO remove unsafe conversion
-
-        await this.refreshSkills(cwd, additionalDirectories, request._meta);
+        await this.refreshSkills(cwd, additionalDirectories);
         if (shouldCancel?.()) {
             return null;
         }
@@ -740,10 +735,10 @@ interface GatewayConfig {
     }
 }
 
-function readAdditionalRoots(meta: Record<string, unknown> | null | undefined): string[] {
+function readMetaAdditionalRoots(meta?: Record<string, unknown> | null): string[] | undefined {
     const rawRoots = meta?.["additionalRoots"];
     if (!Array.isArray(rawRoots)) {
-        return [];
+        return undefined;
     }
 
     return uniqueStrings(rawRoots
@@ -752,16 +747,11 @@ function readAdditionalRoots(meta: Record<string, unknown> | null | undefined):
         .filter(value => value.length > 0));
 }
 
-function readAdditionalDirectories(cwd: string, rawDirectories: unknown): string[] {
-    if (rawDirectories === undefined) {
+function readAdditionalDirectories(cwd: string, additionalDirectories?: string[],  meta?: Record<string, unknown> | null): string[] {
+    const rawDirectories = additionalDirectories ?? readMetaAdditionalRoots(meta);
+    if (!rawDirectories) {
         return [];
     }
-    if (rawDirectories === null) {
-        throw RequestError.invalidParams(undefined, "additionalDirectories must be an array");
-    }
-    if (!Array.isArray(rawDirectories)) {
-        throw RequestError.invalidParams(undefined, "additionalDirectories must be an array");
-    }
 
     const directories: string[] = [];
     const seen = new Set<string>([cwd]);

src/__tests__/CodexACPAgent/CodexAcpClient.test.ts

@@ -262,13 +262,13 @@ describe('ACP server test', { timeout: 40_000 }, () => {
         });
 
         expect(listSkillsSpy).toHaveBeenCalledWith({
-            cwds: ["/workspace"],
+            cwds: ["/workspace", "/skills/one", "/skills/two"],
             forceReload: true,
         });
         expect(listSkillsSpy.mock.invocationCallOrder[0]!).toBeLessThan(threadStartSpy.mock.invocationCallOrder[0]!);
     });
 
-    it('applies ACP additional directories to new session config and skill discovery', async () => {
+    it('prefers ACP additional directories over legacy meta roots for new session skill discovery', async () => {
         const mockFixture = createCodexMockTestFixture();
         const codexAcpClient = mockFixture.getCodexAcpClient();
         const codexAppServerClient = mockFixture.getCodexAppServerClient();
@@ -290,6 +290,9 @@ describe('ACP server test', { timeout: 40_000 }, () => {
             cwd: "/workspace",
             additionalDirectories: ["/workspace/extra", "/workspace", "/workspace/extra"],
             mcpServers: [],
+            _meta: {
+                additionalRoots: ["/skills/one", "/workspace/extra", "/workspace"],
+            },
         });
 
         expect(session.additionalDirectories).toEqual(["/workspace/extra"]);
@@ -559,7 +562,7 @@ describe('ACP server test', { timeout: 40_000 }, () => {
         await codexAcpAgent.prompt(promptRequest);
 
         expect(listSkillsSpy).toHaveBeenCalledWith({
-            cwds: ["/workspace"],
+            cwds: ["/workspace", "/skills/one", "/skills/two"],
             forceReload: true,
         });
         expect(listSkillsSpy.mock.invocationCallOrder[0]!).toBeLessThan(turnStartSpy.mock.invocationCallOrder[0]!);