Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 117 additions & 3 deletions extensions/copilot/src/extension.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,17 @@
* SPDX-License-Identifier: Apache-2.0
***********************************************************************/

import type { Disposable, ExtensionContext } from '@openkaiden/api';
import type {
Agent,
AgentConfigurationFile,
AgentWorkspaceContext,
Disposable,
ExtensionContext,
} from '@openkaiden/api';
import { agents } from '@openkaiden/api';
import { beforeEach, describe, expect, test, vi } from 'vitest';

import { activate } from './extension';
import { activate, COPILOT_SETTINGS_PATH } from './extension';

const AGENT_DISPOSABLE_MOCK: Disposable = { dispose: vi.fn() };

Expand All @@ -36,6 +42,30 @@ beforeEach(() => {
vi.mocked(agents.registerAgent).mockReturnValue(AGENT_DISPOSABLE_MOCK);
});

function getRegisteredAgent(): Agent {
return vi.mocked(agents.registerAgent).mock.calls[0]![0];
}

function createContext(configFiles: AgentConfigurationFile[], modelLabel = 'gpt-4o'): AgentWorkspaceContext {
return {
model: {
model: { label: modelLabel },
},
configurationFiles: configFiles,
workspace: {},
};
}

function createConfigFile(content = '{}'): AgentConfigurationFile & { updateMock: ReturnType<typeof vi.fn> } {
const updateMock = vi.fn();
const file: AgentConfigurationFile = {
path: COPILOT_SETTINGS_PATH,
read: vi.fn().mockResolvedValue(content),
update: updateMock,
};
return Object.assign(file, { updateMock });
}

describe('activate', () => {
test('registers copilot agent', async () => {
await activate(extensionContextMock);
Expand All @@ -61,9 +91,93 @@ describe('activate', () => {
test('registered agent supports all model types except vertexai', async () => {
await activate(extensionContextMock);

const agent = vi.mocked(agents.registerAgent).mock.calls[0]![0];
const agent = getRegisteredAgent();
expect(agent.isSupportedModelType!({ name: 'openai' })).toBe(true);
expect(agent.isSupportedModelType!({ name: 'gemini' })).toBe(true);
expect(agent.isSupportedModelType!({ name: 'vertexai' })).toBe(false);
});

test('registers agent with settings.json configuration file', async () => {
await activate(extensionContextMock);

const agent = getRegisteredAgent();
expect(agent.configurationFiles).toHaveLength(1);
expect(agent.configurationFiles[0]!.path).toBe(COPILOT_SETTINGS_PATH);
});

describe('preWorkspaceStart', () => {
test('writes model configuration into settings.json', async () => {
await activate(extensionContextMock);
const agent = getRegisteredAgent();

const configFile = createConfigFile();
await agent.preWorkspaceStart(createContext([configFile]));

expect(configFile.updateMock).toHaveBeenCalledOnce();
const written = JSON.parse(configFile.updateMock.mock.calls[0]![0] as string);
expect(written).toEqual({
chat: { model: 'gpt-4o' },
});
});

test('preserves existing configuration fields', async () => {
await activate(extensionContextMock);
const agent = getRegisteredAgent();

const existingConfig = JSON.stringify({ chat: { model: 'old-model', temperature: 0.7 }, other: true });
const configFile = createConfigFile(existingConfig);
await agent.preWorkspaceStart(createContext([configFile], 'claude-sonnet'));

const written = JSON.parse(configFile.updateMock.mock.calls[0]![0] as string);
expect(written.chat.model).toBe('claude-sonnet');
expect(written.chat.temperature).toBe(0.7);
expect(written.other).toBe(true);
});

test.each([
'null',
'"a string"',
'123',
'true',
'[1, 2]',
])('falls back to empty config when parsed JSON is non-object: %s', async (payload: string) => {
await activate(extensionContextMock);
const agent = getRegisteredAgent();

const configFile = createConfigFile(payload);
await agent.preWorkspaceStart(createContext([configFile]));

expect(configFile.updateMock).toHaveBeenCalledOnce();
const written = JSON.parse(configFile.updateMock.mock.calls[0]![0] as string);
expect(written.chat.model).toBe('gpt-4o');
});
Comment on lines +137 to +153

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add a regression case for malformed chat field shape.

Current cases cover non-object top-level JSON, but not object payloads where chat itself is invalid (string/array/null). Add one case to ensure preWorkspaceStart replaces malformed chat with an object and still preserves sibling fields.

Suggested test addition
+    test('replaces malformed chat field while preserving sibling fields', async () => {
+      await activate(extensionContextMock);
+      const agent = getRegisteredAgent();
+
+      const existingConfig = JSON.stringify({ chat: 'bad-shape', other: true });
+      const configFile = createConfigFile(existingConfig);
+      await agent.preWorkspaceStart(createContext([configFile], 'claude-sonnet'));
+
+      const written = JSON.parse(configFile.updateMock.mock.calls[0]![0] as string);
+      expect(written.chat.model).toBe('claude-sonnet');
+      expect(written.other).toBe(true);
+    });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@extensions/copilot/src/extension.spec.ts` around lines 135 - 151, The
test.each test case labeled 'falls back to empty config when parsed JSON is
non-object' currently only tests payloads where the top-level JSON itself is
non-object (null, string, number, boolean, array), but does not cover regression
cases where the JSON is a valid object but the chat field inside it is malformed
(like when chat is a string, array, or null). Add one or more test payloads to
the test.each array that represent valid JSON objects with invalid chat field
shapes (for example, an object where chat is a string, array, or null) to verify
that preWorkspaceStart correctly replaces the malformed chat with a proper
object while preserving other sibling fields.


test('handles invalid JSON by starting with empty config', async () => {
await activate(extensionContextMock);
const agent = getRegisteredAgent();

const configFile = createConfigFile('not valid json');
await agent.preWorkspaceStart(createContext([configFile]));

expect(configFile.updateMock).toHaveBeenCalledOnce();
const written = JSON.parse(configFile.updateMock.mock.calls[0]![0] as string);
expect(written.chat.model).toBe('gpt-4o');
});

test('does nothing when config file is not in context', async () => {
await activate(extensionContextMock);
const agent = getRegisteredAgent();

const updateMock = vi.fn();
const otherFile: AgentConfigurationFile = {
path: 'some/other/path.json',
read: vi.fn(),
update: updateMock,
};

await agent.preWorkspaceStart(createContext([otherFile]));

expect(updateMock).not.toHaveBeenCalled();
});
});
});
38 changes: 34 additions & 4 deletions extensions/copilot/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
* SPDX-License-Identifier: Apache-2.0
***********************************************************************/

import type { ExtensionContext } from '@openkaiden/api';
import type { AgentWorkspaceContext, ExtensionContext } from '@openkaiden/api';
import { agents } from '@openkaiden/api';

export const COPILOT_SETTINGS_PATH = '.copilot/settings.json';

export async function activate(extensionContext: ExtensionContext): Promise<void> {
const disposable = agents.registerAgent({
id: 'copilot',
Expand All @@ -39,13 +41,41 @@ export async function activate(extensionContext: ExtensionContext): Promise<void
command: 'copilot',
acp: { args: ['--acp'] },
tags: [],
configurationFiles: [],
destinationSkillsFolder: '${HOME}/.copilot/skills',
configurationFiles: [
{
path: COPILOT_SETTINGS_PATH,
async read(): Promise<string> {
return '{}';
},
},
],
isSupportedModelType(type): boolean {
return type.name !== 'vertexai';
},
async preWorkspaceStart(): Promise<void> {
throw new Error('not implemented');
async preWorkspaceStart(context: AgentWorkspaceContext): Promise<void> {
const configFile = context.configurationFiles.find(f => f.path === COPILOT_SETTINGS_PATH);
if (!configFile) {
return;
}

const content = await configFile.read();
let config: Record<string, unknown>;
try {
const parsed: unknown = JSON.parse(content);
config =
typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed)
? (parsed as Record<string, unknown>)
: {};
} catch {
config = {};
}

const chat = (config.chat as Record<string, unknown> | undefined) ?? {};
chat.model = context.model.model.label;
config.chat = chat;
Comment on lines +74 to +76

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

git ls-files | grep -E "extensions/copilot/src/extension.ts"

Repository: openkaiden/kaiden

Length of output: 97


🏁 Script executed:

head -100 extensions/copilot/src/extension.ts | tail -50

Repository: openkaiden/kaiden

Length of output: 1445


🏁 Script executed:

sed -n '60,90p' extensions/copilot/src/extension.ts

Repository: openkaiden/kaiden

Length of output: 764


Guard config.chat runtime shape before assigning model.

At line 74, the type-cast does not enforce runtime shape. If config.chat is a primitive or array in the JSON configuration, line 75 will throw when attempting property assignment, causing preWorkspaceStart to fail. The code already validates the root parsed object with runtime type checks (lines 67-70); apply the same pattern here to ensure config.chat is a plain object before property assignment.

Proposed fix
-      const chat = (config.chat as Record<string, unknown> | undefined) ?? {};
+      const existingChat = config.chat;
+      const chat =
+        typeof existingChat === 'object' && existingChat !== null && !Array.isArray(existingChat)
+          ? (existingChat as Record<string, unknown>)
+          : {};
       chat.model = context.model.model.label;
       config.chat = chat;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@extensions/copilot/src/extension.ts` around lines 74 - 76, The type-cast on
line 74 does not validate the runtime shape of config.chat, so if it is a
primitive value or array in the JSON configuration, the property assignment on
line 75 will fail and crash preWorkspaceStart. Add a runtime type check before
assigning the model property, following the same validation pattern already used
for the parsed object at lines 67-70. Ensure that config.chat is validated as a
plain object (and not a primitive or array) before attempting to assign
config.chat.model, and handle invalid cases appropriately (either by skipping
the assignment or using a default empty object as a fallback).


await configFile.update(JSON.stringify(config, undefined, 2));
},
});
extensionContext.subscriptions.push(disposable);
Expand Down