From cafac25178b277043bdd5b2f54d956541c830f68 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 23 Sep 2025 06:08:44 +0000 Subject: [PATCH 1/5] Initial plan From 1585f4854278b9ac42ed58837976a2d17a03dbe0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 23 Sep 2025 06:30:26 +0000 Subject: [PATCH 2/5] Fix null dereference in server.ts trace handling Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- packages/cli/src/server.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 9cebd0cbef..94b684c6d6 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -272,6 +272,9 @@ export async function startServer( const { partialCb, inner } = options; if (!wss.clients?.size) throw new Error("GitHub Copilot Chat Models not connected"); + // Ensure trace is available, create a dummy one if needed to prevent null dereference + const safeTrace = trace || new MarkdownTrace(); + return new Promise((resolve, reject) => { let responseSoFar: string = ""; let tokensSoFar: number = 0; @@ -282,10 +285,10 @@ export async function startServer( chats[chatId] = async (chunk) => { if (!responseSoFar && chunk.model) { logVerbose(`chat model ${chunk.model}`); - trace.itemValue("chat model", chunk.model); - trace.appendContent("\n\n"); + safeTrace.itemValue("chat model", chunk.model); + safeTrace.appendContent("\n\n"); } - trace.appendToken(chunk.chunk); + safeTrace.appendToken(chunk.chunk); responseSoFar += chunk.chunk ?? ""; tokensSoFar += chunk.tokens ?? 0; partialCb?.({ @@ -296,11 +299,11 @@ export async function startServer( }); finishReason = chunk.finishReason as any; if (finishReason) { - trace.appendContent("\n\n"); - trace.itemValue(`finish reason`, finishReason); + safeTrace.appendContent("\n\n"); + safeTrace.itemValue(`finish reason`, finishReason); delete chats[chatId]; if (chunk.error) { - trace.error(undefined, chunk.error); + safeTrace.error(undefined, chunk.error); reject(chunk.error); } else resolve({ From c84458f9fda4c51c39bfeb1771ef594260548ce4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 23 Sep 2025 06:38:21 +0000 Subject: [PATCH 3/5] Add test for trace null dereference fix Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- packages/cli/test/server.trace.test.ts | 116 +++++++++++++++++++++++++ 1 file changed, 116 insertions(+) create mode 100644 packages/cli/test/server.trace.test.ts diff --git a/packages/cli/test/server.trace.test.ts b/packages/cli/test/server.trace.test.ts new file mode 100644 index 0000000000..ca3ecdf201 --- /dev/null +++ b/packages/cli/test/server.trace.test.ts @@ -0,0 +1,116 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' + +// Test for null dereference fix in server.ts +describe('server trace null dereference', () => { + let mockTrace: any + + beforeEach(() => { + mockTrace = { + itemValue: vi.fn(), + appendContent: vi.fn(), + appendToken: vi.fn(), + error: vi.fn(), + } + }) + + afterEach(() => { + vi.clearAllMocks() + }) + + it('should handle undefined trace safely in chat completion handler', () => { + // This test simulates the scenario that would cause null dereference + // before the fix was applied + + // Mock the chat completion handler behavior + const mockChunk = { + model: 'test-model', + chunk: 'test-chunk', + finishReason: 'stop', + error: null + } + + // Simulate the old unsafe behavior that would cause null dereference + const unsafeHandler = (trace: any) => { + if (mockChunk.model) { + trace.itemValue("chat model", mockChunk.model) + trace.appendContent("\n\n") + } + trace.appendToken(mockChunk.chunk) + + if (mockChunk.finishReason) { + trace.appendContent("\n\n") + trace.itemValue("finish reason", mockChunk.finishReason) + } + } + + // Test with null/undefined trace (this would fail before the fix) + expect(() => { + unsafeHandler(null) + }).toThrow() + + expect(() => { + unsafeHandler(undefined) + }).toThrow() + + // Simulate the safe behavior after the fix + const safeHandler = (trace: any) => { + const safeTrace = trace || mockTrace + if (mockChunk.model) { + safeTrace.itemValue("chat model", mockChunk.model) + safeTrace.appendContent("\n\n") + } + safeTrace.appendToken(mockChunk.chunk) + + if (mockChunk.finishReason) { + safeTrace.appendContent("\n\n") + safeTrace.itemValue("finish reason", mockChunk.finishReason) + } + } + + // Test with null/undefined trace (this should work after the fix) + expect(() => { + safeHandler(null) + }).not.toThrow() + + expect(() => { + safeHandler(undefined) + }).not.toThrow() + + // Verify the safe fallback trace methods were called + expect(mockTrace.itemValue).toHaveBeenCalledWith("chat model", "test-model") + expect(mockTrace.appendContent).toHaveBeenCalled() + expect(mockTrace.appendToken).toHaveBeenCalledWith("test-chunk") + expect(mockTrace.itemValue).toHaveBeenCalledWith("finish reason", "stop") + }) + + it('should use provided trace when available', () => { + const providedTrace = { + itemValue: vi.fn(), + appendContent: vi.fn(), + appendToken: vi.fn(), + error: vi.fn(), + } + + const mockChunk = { + model: 'test-model', + chunk: 'test-chunk' + } + + // Simulate the safe handler behavior + const safeHandler = (trace: any) => { + const safeTrace = trace || mockTrace + if (mockChunk.model) { + safeTrace.itemValue("chat model", mockChunk.model) + } + safeTrace.appendToken(mockChunk.chunk) + } + + safeHandler(providedTrace) + + // Verify the provided trace was used, not the fallback + expect(providedTrace.itemValue).toHaveBeenCalledWith("chat model", "test-model") + expect(providedTrace.appendToken).toHaveBeenCalledWith("test-chunk") + expect(mockTrace.itemValue).not.toHaveBeenCalled() + expect(mockTrace.appendToken).not.toHaveBeenCalled() + }) +}) \ No newline at end of file From 4e813439f1280bc1625f21aa901c7fd2fb9dc2f8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 23 Sep 2025 07:50:01 +0000 Subject: [PATCH 4/5] Replace safeTrace pattern with optional chaining as requested Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- packages/cli/src/server.ts | 15 ++++------ packages/cli/test/server.trace.test.ts | 40 ++++++++++++++++---------- 2 files changed, 31 insertions(+), 24 deletions(-) diff --git a/packages/cli/src/server.ts b/packages/cli/src/server.ts index 94b684c6d6..72cd4940ca 100644 --- a/packages/cli/src/server.ts +++ b/packages/cli/src/server.ts @@ -272,9 +272,6 @@ export async function startServer( const { partialCb, inner } = options; if (!wss.clients?.size) throw new Error("GitHub Copilot Chat Models not connected"); - // Ensure trace is available, create a dummy one if needed to prevent null dereference - const safeTrace = trace || new MarkdownTrace(); - return new Promise((resolve, reject) => { let responseSoFar: string = ""; let tokensSoFar: number = 0; @@ -285,10 +282,10 @@ export async function startServer( chats[chatId] = async (chunk) => { if (!responseSoFar && chunk.model) { logVerbose(`chat model ${chunk.model}`); - safeTrace.itemValue("chat model", chunk.model); - safeTrace.appendContent("\n\n"); + trace?.itemValue("chat model", chunk.model); + trace?.appendContent("\n\n"); } - safeTrace.appendToken(chunk.chunk); + trace?.appendToken(chunk.chunk); responseSoFar += chunk.chunk ?? ""; tokensSoFar += chunk.tokens ?? 0; partialCb?.({ @@ -299,11 +296,11 @@ export async function startServer( }); finishReason = chunk.finishReason as any; if (finishReason) { - safeTrace.appendContent("\n\n"); - safeTrace.itemValue(`finish reason`, finishReason); + trace?.appendContent("\n\n"); + trace?.itemValue(`finish reason`, finishReason); delete chats[chatId]; if (chunk.error) { - safeTrace.error(undefined, chunk.error); + trace?.error(undefined, chunk.error); reject(chunk.error); } else resolve({ diff --git a/packages/cli/test/server.trace.test.ts b/packages/cli/test/server.trace.test.ts index ca3ecdf201..248bed7d99 100644 --- a/packages/cli/test/server.trace.test.ts +++ b/packages/cli/test/server.trace.test.ts @@ -52,18 +52,17 @@ describe('server trace null dereference', () => { unsafeHandler(undefined) }).toThrow() - // Simulate the safe behavior after the fix + // Simulate the safe behavior after the fix using optional chaining const safeHandler = (trace: any) => { - const safeTrace = trace || mockTrace if (mockChunk.model) { - safeTrace.itemValue("chat model", mockChunk.model) - safeTrace.appendContent("\n\n") + trace?.itemValue("chat model", mockChunk.model) + trace?.appendContent("\n\n") } - safeTrace.appendToken(mockChunk.chunk) + trace?.appendToken(mockChunk.chunk) if (mockChunk.finishReason) { - safeTrace.appendContent("\n\n") - safeTrace.itemValue("finish reason", mockChunk.finishReason) + trace?.appendContent("\n\n") + trace?.itemValue("finish reason", mockChunk.finishReason) } } @@ -76,7 +75,12 @@ describe('server trace null dereference', () => { safeHandler(undefined) }).not.toThrow() - // Verify the safe fallback trace methods were called + // Test with real trace to ensure it still works when provided + expect(() => { + safeHandler(mockTrace) + }).not.toThrow() + + // Verify the trace methods were called when trace was provided expect(mockTrace.itemValue).toHaveBeenCalledWith("chat model", "test-model") expect(mockTrace.appendContent).toHaveBeenCalled() expect(mockTrace.appendToken).toHaveBeenCalledWith("test-chunk") @@ -96,21 +100,27 @@ describe('server trace null dereference', () => { chunk: 'test-chunk' } - // Simulate the safe handler behavior + // Simulate the safe handler behavior using optional chaining const safeHandler = (trace: any) => { - const safeTrace = trace || mockTrace if (mockChunk.model) { - safeTrace.itemValue("chat model", mockChunk.model) + trace?.itemValue("chat model", mockChunk.model) } - safeTrace.appendToken(mockChunk.chunk) + trace?.appendToken(mockChunk.chunk) } safeHandler(providedTrace) - // Verify the provided trace was used, not the fallback + // Verify the provided trace was used expect(providedTrace.itemValue).toHaveBeenCalledWith("chat model", "test-model") expect(providedTrace.appendToken).toHaveBeenCalledWith("test-chunk") - expect(mockTrace.itemValue).not.toHaveBeenCalled() - expect(mockTrace.appendToken).not.toHaveBeenCalled() + + // Test with null/undefined trace - should not throw + expect(() => { + safeHandler(null) + }).not.toThrow() + + expect(() => { + safeHandler(undefined) + }).not.toThrow() }) }) \ No newline at end of file From be23c4eb34992975c345ca6ead6a1bc30e00d584 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 23 Sep 2025 07:58:31 +0000 Subject: [PATCH 5/5] genai: /docs [skip ci] --- packages/cli/test/server.trace.test.ts | 124 ++++++++++++------------- 1 file changed, 62 insertions(+), 62 deletions(-) diff --git a/packages/cli/test/server.trace.test.ts b/packages/cli/test/server.trace.test.ts index 248bed7d99..4611228342 100644 --- a/packages/cli/test/server.trace.test.ts +++ b/packages/cli/test/server.trace.test.ts @@ -1,126 +1,126 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; // Test for null dereference fix in server.ts -describe('server trace null dereference', () => { - let mockTrace: any - +describe("server trace null dereference", () => { + let mockTrace: any; + beforeEach(() => { mockTrace = { itemValue: vi.fn(), appendContent: vi.fn(), appendToken: vi.fn(), error: vi.fn(), - } - }) + }; + }); afterEach(() => { - vi.clearAllMocks() - }) + vi.clearAllMocks(); + }); - it('should handle undefined trace safely in chat completion handler', () => { + it("should handle undefined trace safely in chat completion handler", () => { // This test simulates the scenario that would cause null dereference // before the fix was applied - + // Mock the chat completion handler behavior const mockChunk = { - model: 'test-model', - chunk: 'test-chunk', - finishReason: 'stop', - error: null - } + model: "test-model", + chunk: "test-chunk", + finishReason: "stop", + error: null, + }; // Simulate the old unsafe behavior that would cause null dereference const unsafeHandler = (trace: any) => { if (mockChunk.model) { - trace.itemValue("chat model", mockChunk.model) - trace.appendContent("\n\n") + trace.itemValue("chat model", mockChunk.model); + trace.appendContent("\n\n"); } - trace.appendToken(mockChunk.chunk) - + trace.appendToken(mockChunk.chunk); + if (mockChunk.finishReason) { - trace.appendContent("\n\n") - trace.itemValue("finish reason", mockChunk.finishReason) + trace.appendContent("\n\n"); + trace.itemValue("finish reason", mockChunk.finishReason); } - } + }; // Test with null/undefined trace (this would fail before the fix) expect(() => { - unsafeHandler(null) - }).toThrow() + unsafeHandler(null); + }).toThrow(); expect(() => { - unsafeHandler(undefined) - }).toThrow() + unsafeHandler(undefined); + }).toThrow(); // Simulate the safe behavior after the fix using optional chaining const safeHandler = (trace: any) => { if (mockChunk.model) { - trace?.itemValue("chat model", mockChunk.model) - trace?.appendContent("\n\n") + trace?.itemValue("chat model", mockChunk.model); + trace?.appendContent("\n\n"); } - trace?.appendToken(mockChunk.chunk) - + trace?.appendToken(mockChunk.chunk); + if (mockChunk.finishReason) { - trace?.appendContent("\n\n") - trace?.itemValue("finish reason", mockChunk.finishReason) + trace?.appendContent("\n\n"); + trace?.itemValue("finish reason", mockChunk.finishReason); } - } + }; // Test with null/undefined trace (this should work after the fix) expect(() => { - safeHandler(null) - }).not.toThrow() + safeHandler(null); + }).not.toThrow(); expect(() => { - safeHandler(undefined) - }).not.toThrow() + safeHandler(undefined); + }).not.toThrow(); // Test with real trace to ensure it still works when provided expect(() => { - safeHandler(mockTrace) - }).not.toThrow() + safeHandler(mockTrace); + }).not.toThrow(); // Verify the trace methods were called when trace was provided - expect(mockTrace.itemValue).toHaveBeenCalledWith("chat model", "test-model") - expect(mockTrace.appendContent).toHaveBeenCalled() - expect(mockTrace.appendToken).toHaveBeenCalledWith("test-chunk") - expect(mockTrace.itemValue).toHaveBeenCalledWith("finish reason", "stop") - }) + expect(mockTrace.itemValue).toHaveBeenCalledWith("chat model", "test-model"); + expect(mockTrace.appendContent).toHaveBeenCalled(); + expect(mockTrace.appendToken).toHaveBeenCalledWith("test-chunk"); + expect(mockTrace.itemValue).toHaveBeenCalledWith("finish reason", "stop"); + }); - it('should use provided trace when available', () => { + it("should use provided trace when available", () => { const providedTrace = { itemValue: vi.fn(), appendContent: vi.fn(), appendToken: vi.fn(), error: vi.fn(), - } + }; const mockChunk = { - model: 'test-model', - chunk: 'test-chunk' - } + model: "test-model", + chunk: "test-chunk", + }; // Simulate the safe handler behavior using optional chaining const safeHandler = (trace: any) => { if (mockChunk.model) { - trace?.itemValue("chat model", mockChunk.model) + trace?.itemValue("chat model", mockChunk.model); } - trace?.appendToken(mockChunk.chunk) - } + trace?.appendToken(mockChunk.chunk); + }; - safeHandler(providedTrace) + safeHandler(providedTrace); // Verify the provided trace was used - expect(providedTrace.itemValue).toHaveBeenCalledWith("chat model", "test-model") - expect(providedTrace.appendToken).toHaveBeenCalledWith("test-chunk") - + expect(providedTrace.itemValue).toHaveBeenCalledWith("chat model", "test-model"); + expect(providedTrace.appendToken).toHaveBeenCalledWith("test-chunk"); + // Test with null/undefined trace - should not throw expect(() => { - safeHandler(null) - }).not.toThrow() + safeHandler(null); + }).not.toThrow(); expect(() => { - safeHandler(undefined) - }).not.toThrow() - }) -}) \ No newline at end of file + safeHandler(undefined); + }).not.toThrow(); + }); +});