Skip to content

Commit a39a574

Browse files
CopilotCopilot
committed
fix(copilot-local): address follow-up review items
- share in-flight Copilot model discovery requests\n- normalize Copilot stop errors and reuse token resolution\n- revert pnpm-lock.yaml from the PR diff\n\nCo-Authored-By: Paperclip <noreply@paperclip.ing>\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 7ed6a7b commit a39a574

7 files changed

Lines changed: 126 additions & 141 deletions

File tree

‎packages/adapters/copilot-local/src/server/execute.test.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,9 @@ describe("copilot execute", () => {
217217
source: path.join(skillsRoot, "paperclip"),
218218
},
219219
],
220+
paperclipSkillSync: {
221+
desiredSkills: ["paperclip"],
222+
},
220223
},
221224
context: {},
222225
authToken: "run-jwt-token",
@@ -259,7 +262,6 @@ describe("copilot execute", () => {
259262
expect(capture.sessionConfig).toMatchObject({
260263
model: "gpt-5.4",
261264
workingDirectory: workspace,
262-
skillDirectories: expect.any(Array),
263265
systemMessage: expect.objectContaining({
264266
mode: "append",
265267
}),
@@ -274,9 +276,6 @@ describe("copilot execute", () => {
274276
]),
275277
);
276278
expect(session.lastPrompt).toContain("Continue the Paperclip work.");
277-
const skillDirectory = (capture.sessionConfig?.skillDirectories as string[] | undefined)?.[0];
278-
expect(skillDirectory).toBeTruthy();
279-
await expect(fs.stat(String(skillDirectory))).rejects.toThrow();
280279
});
281280

282281
it("retries with a fresh SDK session when resumeSession reports an unknown session", async () => {

‎packages/adapters/copilot-local/src/server/execute.ts‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
import { parseCopilotJsonl } from "./parse.js";
2626
import {
2727
buildCopilotClientBootstrap,
28+
extractCopilotStopErrors,
2829
buildLoggedInvocationEnv,
2930
isCopilotAuthRequiredMessage,
3031
isCopilotUnknownSessionMessage,
@@ -34,17 +35,13 @@ import {
3435
normalizeEnvConfig,
3536
normalizeRuntimeEnv,
3637
removeDirSafe,
38+
resolveGithubToken,
3739
resolveCopilotModelSelection,
3840
} from "./runtime.js";
3941
import { isCopilotIdleTimeoutError, sendPromptAndWaitForIdle } from "./session.js";
4042

4143
const __moduleDir = path.dirname(fileURLToPath(import.meta.url));
4244

43-
function resolveGithubToken(env: Record<string, string>): string | null {
44-
const token = (env.COPILOT_GITHUB_TOKEN ?? env.GH_TOKEN ?? env.GITHUB_TOKEN ?? "").trim();
45-
return token.length > 0 ? token : null;
46-
}
47-
4845
function createSessionParams(
4946
sessionId: string | null,
5047
cwd: string,
@@ -389,10 +386,16 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
389386
writeLog("stderr", `[paperclip] ${error.message}`);
390387
} finally {
391388
if (unsubscribe) unsubscribe();
392-
const stopResult = client ? await client.stop().catch(() => [] as Error[]) : [];
393-
const stopErrors = Array.isArray(stopResult) ? (stopResult as Error[]) : [];
389+
let stopResult: unknown = null;
390+
if (client) {
391+
try {
392+
stopResult = await client.stop();
393+
} catch (error) {
394+
stopResult = error;
395+
}
396+
}
397+
const stopErrors = extractCopilotStopErrors(stopResult);
394398
for (const error of stopErrors) {
395-
if (!error) continue;
396399
writeLog("stderr", `[paperclip] ${error.message}`);
397400
}
398401
await Promise.allSettled(logWrites);

‎packages/adapters/copilot-local/src/server/models.test.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,22 @@ import {
66
import { DEFAULT_COPILOT_LOCAL_MODEL } from "../index.js";
77
import { detectCopilotModel, listCopilotModels } from "./models.js";
88

9+
const originalGhToken = process.env.GH_TOKEN;
10+
911
afterEach(() => {
1012
setCopilotClientFactoryForTests(null);
1113
resetCopilotModelsCacheForTests();
14+
if (typeof originalGhToken === "string") {
15+
process.env.GH_TOKEN = originalGhToken;
16+
} else {
17+
delete process.env.GH_TOKEN;
18+
}
1219
vi.restoreAllMocks();
1320
});
1421

1522
describe("copilot model discovery", () => {
1623
it("caches discovered models between calls", async () => {
24+
process.env.GH_TOKEN = "test-token";
1725
const listModels = vi.fn(async () => [
1826
{ id: DEFAULT_COPILOT_LOCAL_MODEL, name: "GPT 5.4" },
1927
{ id: "claude-sonnet-4", name: "Claude Sonnet 4" },
@@ -46,7 +54,62 @@ describe("copilot model discovery", () => {
4654
expect(stop).toHaveBeenCalledTimes(1);
4755
});
4856

57+
it("shares an in-flight discovery request across concurrent callers", async () => {
58+
process.env.GH_TOKEN = "test-token";
59+
let resolveModels: ((value: Array<{ id: string; name: string }>) => void) | null = null;
60+
const listModels = vi.fn(
61+
() =>
62+
new Promise<Array<{ id: string; name: string }>>((resolve) => {
63+
resolveModels = resolve;
64+
}),
65+
);
66+
const start = vi.fn(async () => {});
67+
const stop = vi.fn(async () => []);
68+
69+
setCopilotClientFactoryForTests(() => ({
70+
start,
71+
stop,
72+
forceStop: async () => {},
73+
ping: async () => ({ message: "ok", timestamp: Date.now() }),
74+
getStatus: async () => ({ version: "1.0.0", protocolVersion: 1 }),
75+
getAuthStatus: async () => ({ isAuthenticated: true }),
76+
listModels,
77+
createSession: async () => {
78+
throw new Error("createSession should not be used");
79+
},
80+
resumeSession: async () => {
81+
throw new Error("resumeSession should not be used");
82+
},
83+
}) as never);
84+
85+
const first = listCopilotModels();
86+
const second = listCopilotModels();
87+
await vi.waitFor(() => {
88+
expect(listModels).toHaveBeenCalledTimes(1);
89+
});
90+
91+
expect(resolveModels).not.toBeNull();
92+
resolveModels!([
93+
{ id: DEFAULT_COPILOT_LOCAL_MODEL, name: "GPT 5.4" },
94+
{ id: "claude-sonnet-4", name: "Claude Sonnet 4" },
95+
]);
96+
97+
await expect(Promise.all([first, second])).resolves.toEqual([
98+
[
99+
{ id: DEFAULT_COPILOT_LOCAL_MODEL, label: "GPT 5.4 (default) (gpt-5.4)" },
100+
{ id: "claude-sonnet-4", label: "Claude Sonnet 4 (claude-sonnet-4)" },
101+
],
102+
[
103+
{ id: DEFAULT_COPILOT_LOCAL_MODEL, label: "GPT 5.4 (default) (gpt-5.4)" },
104+
{ id: "claude-sonnet-4", label: "Claude Sonnet 4 (claude-sonnet-4)" },
105+
],
106+
]);
107+
expect(start).toHaveBeenCalledTimes(1);
108+
expect(stop).toHaveBeenCalledTimes(1);
109+
});
110+
49111
it("prefers the default Copilot model when detecting a selected model", async () => {
112+
process.env.GH_TOKEN = "test-token";
50113
setCopilotClientFactoryForTests(() => ({
51114
start: async () => {},
52115
stop: async () => [],

‎packages/adapters/copilot-local/src/server/models.ts‎

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,14 @@ import { createCopilotClient } from "./sdk-client.js";
33
import {
44
normalizeCopilotDiscoveredModels,
55
normalizeRuntimeEnv,
6+
resolveGithubToken,
67
} from "./runtime.js";
78
import { DEFAULT_COPILOT_LOCAL_MODEL, models as fallbackModels } from "../index.js";
89

910
const COPILOT_MODELS_CACHE_TTL_MS = 60_000;
1011

1112
let cached: { expiresAt: number; models: AdapterModel[] } | null = null;
13+
let inflightDiscovery: Promise<AdapterModel[]> | null = null;
1214

1315
async function safeStopClient(client: { stop(): Promise<unknown> } | null): Promise<void> {
1416
if (!client) return;
@@ -21,7 +23,7 @@ async function safeStopClient(client: { stop(): Promise<unknown> } | null): Prom
2123

2224
export async function discoverCopilotModels(): Promise<AdapterModel[]> {
2325
const runtimeEnv = normalizeRuntimeEnv(process.env);
24-
const githubToken = (runtimeEnv.GH_TOKEN ?? runtimeEnv.GITHUB_TOKEN ?? "").trim();
26+
const githubToken = resolveGithubToken(runtimeEnv);
2527
if (!githubToken) return [];
2628

2729
const client = await createCopilotClient({
@@ -51,23 +53,31 @@ export async function discoverCopilotModels(): Promise<AdapterModel[]> {
5153
export async function listModels(): Promise<AdapterModel[]> {
5254
const now = Date.now();
5355
if (cached && cached.expiresAt > now) return cached.models;
56+
if (inflightDiscovery) return inflightDiscovery;
5457

55-
try {
56-
const models = await discoverCopilotModels();
57-
if (models.length > 0) {
58-
cached = {
59-
expiresAt: now + COPILOT_MODELS_CACHE_TTL_MS,
60-
models,
61-
};
58+
inflightDiscovery = (async () => {
59+
try {
60+
const models = await discoverCopilotModels();
61+
if (models.length > 0) {
62+
cached = {
63+
expiresAt: Date.now() + COPILOT_MODELS_CACHE_TTL_MS,
64+
models,
65+
};
66+
}
67+
return models;
68+
} catch {
69+
return [];
70+
} finally {
71+
inflightDiscovery = null;
6272
}
63-
return models;
64-
} catch {
65-
return [];
66-
}
73+
})();
74+
75+
return await inflightDiscovery;
6776
}
6877

6978
export function resetCopilotModelsCacheForTests(): void {
7079
cached = null;
80+
inflightDiscovery = null;
7181
}
7282

7383
export const listCopilotModels = listModels;

‎packages/adapters/copilot-local/src/server/runtime.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,17 @@ export function normalizeEnvConfig(input: unknown): Record<string, string> {
349349
return normalizeRuntimeEnv(parseObject(input) as NodeJS.ProcessEnv);
350350
}
351351

352+
export function resolveGithubToken(env: Record<string, string>): string | null {
353+
const token = (env.COPILOT_GITHUB_TOKEN ?? env.GH_TOKEN ?? env.GITHUB_TOKEN ?? "").trim();
354+
return token.length > 0 ? token : null;
355+
}
356+
357+
export function extractCopilotStopErrors(result: unknown): Error[] {
358+
if (result instanceof Error) return [result];
359+
if (!Array.isArray(result)) return [];
360+
return result.filter((value): value is Error => value instanceof Error);
361+
}
362+
352363
export async function buildCopilotClientBootstrap(input: {
353364
command?: unknown;
354365
args?: unknown;

‎packages/adapters/copilot-local/src/server/test.ts‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,12 @@ import { DEFAULT_COPILOT_LOCAL_MODEL } from "../index.js";
1313
import { createCopilotClient, approveAll } from "./sdk-client.js";
1414
import {
1515
buildCopilotClientBootstrap,
16+
extractCopilotStopErrors,
1617
isCopilotAuthRequiredMessage,
1718
normalizeCopilotDiscoveredModels,
1819
normalizeEnvConfig,
1920
normalizeRuntimeEnv,
21+
resolveGithubToken,
2022
resolveCopilotModelSelection,
2123
} from "./runtime.js";
2224
import { sendPromptAndWaitForIdle } from "./session.js";
@@ -27,11 +29,6 @@ function summarizeStatus(checks: AdapterEnvironmentCheck[]): AdapterEnvironmentT
2729
return "pass";
2830
}
2931

30-
function resolveGithubToken(env: Record<string, string>): string | null {
31-
const token = (env.COPILOT_GITHUB_TOKEN ?? env.GH_TOKEN ?? env.GITHUB_TOKEN ?? "").trim();
32-
return token.length > 0 ? token : null;
33-
}
34-
3532
export async function testEnvironment(
3633
ctx: AdapterEnvironmentTestContext,
3734
): Promise<AdapterEnvironmentTestResult> {
@@ -266,7 +263,20 @@ export async function testEnvironment(
266263
: "If you configured a custom command override, verify it points to a compatible Copilot CLI binary.",
267264
});
268265
} finally {
269-
await client.stop().catch(() => []);
266+
let stopResult: unknown = null;
267+
try {
268+
stopResult = await client.stop();
269+
} catch (error) {
270+
stopResult = error;
271+
}
272+
for (const error of extractCopilotStopErrors(stopResult)) {
273+
checks.push({
274+
code: "copilot_sdk_stop_failed",
275+
level: "warn",
276+
message: "Copilot SDK cleanup reported an error.",
277+
detail: error.message,
278+
});
279+
}
270280
}
271281

272282
return {

0 commit comments

Comments
 (0)