feat(copilot): add agent configuration and model settings support#2180
feat(copilot): add agent configuration and model settings support#2180gastoner wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 9 minutes and 14 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an exported ChangesCopilot agent settings.json configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gastoner
force-pushed
the
copilot_agent_configuration
branch
from
7c14b29 to
08e7f75
Compare
gastoner
changed the title
gastoner
requested review from
benoitf and
jeffmaury
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@extensions/copilot/src/extension.spec.ts`:
- Around line 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.
In `@extensions/copilot/src/extension.ts`:
- Around line 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).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8bd948f-7497-43f2-9918-265a9e49abfb
📒 Files selected for processing (2)
extensions/copilot/src/extension.spec.tsextensions/copilot/src/extension.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
extensions/*/src/extension.ts
📄 CodeRabbit inference engine (AGENTS.md)
Extensions should export a standard activation API from their entry point
Files:
extensions/copilot/src/extension.ts
extensions/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Register inference, container, and Kubernetes providers through the
ProviderRegistryvia extension APIs
Files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
**/*.spec.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx,js,jsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in unit tests; avoid manual mock factories when possible
Usevi.resetAllMocks()inbeforeEachhooks instead ofvi.clearAllMocks()for resetting mocks between tests
When an auto-mocked function or class method needs a real implementation, usevi.mocked(...)with the prototype pattern for class methods:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
extensions/copilot/src/extension.spec.ts
🧠 Learnings (5)
📚 Learning: 2026-05-05T17:30:20.418Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:272-302
Timestamp: 2026-05-05T17:30:20.418Z
Learning: In the openkaiden/kaiden repo, for cloud inference provider extension code under `extensions/*/src/*.ts`, treat `ProviderConnectionStatus = 'unknown'` as a valid/expected value when registering provider connections (e.g., Gemini/Claude/Mistral/OpenAI-compatible/Vertex AI). `'unknown'` indicates the connection was set up but is not continuously monitored—so do not flag it as incorrect. Only Ollama is expected to use `'started'` because it actively polls a local server.
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
📚 Learning: 2026-05-05T17:44:50.991Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:363-387
Timestamp: 2026-05-05T17:44:50.991Z
Learning: In this repo (openkaiden/kaiden), do not raise a code review issue when an extension’s `InferenceProviderConnectionFactory.create` factory method implementation omits (or does not use) the optional `logger` and/or `CancellationToken` parameters in its method signature/implementation. Current extensions (e.g., Vertex AI, Gemini, Claude, Mistral, OpenAI-compatible) follow this pattern, so reviewers should treat it as acceptable for `extensions/*` TypeScript source files.
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
📚 Learning: 2026-05-06T11:15:56.238Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/extension.spec.ts:43-51
Timestamp: 2026-05-06T11:15:56.238Z
Learning: In all extensions under extensions/*/src/extension.ts, deactivate() should only clear the module-level instance reference (e.g., set the instance to undefined) and must not call dispose() directly. The dispose() method is invoked by the extension host when processing extensionContext.subscriptions. Do not suggest asserting dispose() in tests for deactivate(); such assertions are unnecessary because disposal is handled by the host and CI checks should validate subscriptions handling instead.
Applied to files:
extensions/copilot/src/extension.ts
📚 Learning: 2026-05-12T10:01:14.248Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1810
File: extensions/kdn/src/kdn-extension.ts:43-46
Timestamp: 2026-05-12T10:01:14.248Z
Learning: In this repo’s extension code, when logging from binary discovery/resolution logic (e.g., choosing/validating custom paths, extension storage locations, or bundled resource paths), it’s intentional to include full filesystem paths in `console.log`/`console.warn` (such as in `extensions/**/src/*-extension.ts`). During review, do not flag these specific full-path messages as a privacy/security issue as long as they are clearly part of the binary resolution steps. If full-path logging appears outside binary discovery/resolution, review/flag it as usual.
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
🔇 Additional comments (2)
extensions/copilot/src/extension.ts (1)
19-73: LGTM!Also applies to: 77-79
extensions/copilot/src/extension.spec.ts (1)
19-133: LGTM!Also applies to: 153-180
| 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'); | ||
| }); |
There was a problem hiding this comment.
🛠️ 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.
| const chat = (config.chat as Record<string, unknown> | undefined) ?? {}; | ||
| chat.model = context.model.model.label; | ||
| config.chat = chat; |
There was a problem hiding this comment.
🧩 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 -50Repository: openkaiden/kaiden
Length of output: 1445
🏁 Script executed:
sed -n '60,90p' extensions/copilot/src/extension.tsRepository: 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).
08e7f75 to
5a09812
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
extensions/copilot/src/extension.spec.ts (1)
135-151: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a malformed
chatshape regression case.Current parameterized cases validate non-object top-level payloads, but not valid objects with invalid
chatvalues (string/array/null). Add one case to lock behavior for that path.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, Expand the parameterized test cases in the test.each array to include regression cases for valid objects with invalid chat property shapes. In addition to the existing non-object payloads (null, string number, boolean, array), add test cases for objects containing a chat property with invalid values such as a string, array, or null value (e.g., '{"chat": "string"}', '{"chat": null}', '{"chat": [1,2]}'). These cases should verify that the agent still falls back to the empty config correctly when the chat shape is malformed, ensuring the same assertions about updateMock being called once and written.chat.model being set to 'gpt-4o' continue to pass.extensions/copilot/src/extension.ts (1)
74-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
config.chatruntime shape before assigningmodel(Line 74).The cast at Line 74 does not enforce object shape. If
chatis a primitive, Line 75 can throw during assignment and breakpreWorkspaceStart.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 at line 74 in the config.chat assignment does not validate the actual runtime shape of config.chat. If config.chat is a primitive value (string, number, boolean, etc.), attempting to assign the model property at line 75 can fail. Validate that config.chat is actually an object before the assignment by checking its type at runtime, and only use it if it's a valid object; otherwise, use the default empty object. This ensures preWorkspaceStart does not break due to invalid property assignment on a primitive value.
🤖 Prompt for all review comments with 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.
Duplicate comments:
In `@extensions/copilot/src/extension.spec.ts`:
- Around line 135-151: Expand the parameterized test cases in the test.each
array to include regression cases for valid objects with invalid chat property
shapes. In addition to the existing non-object payloads (null, string number,
boolean, array), add test cases for objects containing a chat property with
invalid values such as a string, array, or null value (e.g., '{"chat":
"string"}', '{"chat": null}', '{"chat": [1,2]}'). These cases should verify that
the agent still falls back to the empty config correctly when the chat shape is
malformed, ensuring the same assertions about updateMock being called once and
written.chat.model being set to 'gpt-4o' continue to pass.
In `@extensions/copilot/src/extension.ts`:
- Around line 74-76: The type cast at line 74 in the config.chat assignment does
not validate the actual runtime shape of config.chat. If config.chat is a
primitive value (string, number, boolean, etc.), attempting to assign the model
property at line 75 can fail. Validate that config.chat is actually an object
before the assignment by checking its type at runtime, and only use it if it's a
valid object; otherwise, use the default empty object. This ensures
preWorkspaceStart does not break due to invalid property assignment on a
primitive value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a767238-0754-4241-8a8e-3872df8e6302
📒 Files selected for processing (2)
extensions/copilot/src/extension.spec.tsextensions/copilot/src/extension.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: macOS
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: typecheck
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: Windows
- GitHub Check: unit tests / windows-2022
- GitHub Check: linter, formatters
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: Linux
- GitHub Check: unit tests / macos-15
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
extensions/*/src/extension.ts
📄 CodeRabbit inference engine (AGENTS.md)
Extensions should export a standard activation API from their entry point
Files:
extensions/copilot/src/extension.ts
extensions/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Register inference, container, and Kubernetes providers through the
ProviderRegistryvia extension APIs
Files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
**/*.spec.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx,js,jsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in unit tests; avoid manual mock factories when possible
Usevi.resetAllMocks()inbeforeEachhooks instead ofvi.clearAllMocks()for resetting mocks between tests
When an auto-mocked function or class method needs a real implementation, usevi.mocked(...)with the prototype pattern for class methods:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
extensions/copilot/src/extension.spec.ts
🧠 Learnings (5)
📚 Learning: 2026-05-05T17:30:20.418Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:272-302
Timestamp: 2026-05-05T17:30:20.418Z
Learning: In the openkaiden/kaiden repo, for cloud inference provider extension code under `extensions/*/src/*.ts`, treat `ProviderConnectionStatus = 'unknown'` as a valid/expected value when registering provider connections (e.g., Gemini/Claude/Mistral/OpenAI-compatible/Vertex AI). `'unknown'` indicates the connection was set up but is not continuously monitored—so do not flag it as incorrect. Only Ollama is expected to use `'started'` because it actively polls a local server.
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
📚 Learning: 2026-05-05T17:44:50.991Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:363-387
Timestamp: 2026-05-05T17:44:50.991Z
Learning: In this repo (openkaiden/kaiden), do not raise a code review issue when an extension’s `InferenceProviderConnectionFactory.create` factory method implementation omits (or does not use) the optional `logger` and/or `CancellationToken` parameters in its method signature/implementation. Current extensions (e.g., Vertex AI, Gemini, Claude, Mistral, OpenAI-compatible) follow this pattern, so reviewers should treat it as acceptable for `extensions/*` TypeScript source files.
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
📚 Learning: 2026-05-06T11:15:56.238Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/extension.spec.ts:43-51
Timestamp: 2026-05-06T11:15:56.238Z
Learning: In all extensions under extensions/*/src/extension.ts, deactivate() should only clear the module-level instance reference (e.g., set the instance to undefined) and must not call dispose() directly. The dispose() method is invoked by the extension host when processing extensionContext.subscriptions. Do not suggest asserting dispose() in tests for deactivate(); such assertions are unnecessary because disposal is handled by the host and CI checks should validate subscriptions handling instead.
Applied to files:
extensions/copilot/src/extension.ts
📚 Learning: 2026-05-12T10:01:14.248Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1810
File: extensions/kdn/src/kdn-extension.ts:43-46
Timestamp: 2026-05-12T10:01:14.248Z
Learning: In this repo’s extension code, when logging from binary discovery/resolution logic (e.g., choosing/validating custom paths, extension storage locations, or bundled resource paths), it’s intentional to include full filesystem paths in `console.log`/`console.warn` (such as in `extensions/**/src/*-extension.ts`). During review, do not flag these specific full-path messages as a privacy/security issue as long as they are clearly part of the binary resolution steps. If full-path logging appears outside binary discovery/resolution, review/flag it as usual.
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
🔇 Additional comments (2)
extensions/copilot/src/extension.ts (1)
19-23: LGTM!Also applies to: 44-53, 56-73, 78-79
extensions/copilot/src/extension.spec.ts (1)
19-30: LGTM!Also applies to: 45-133, 153-181
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
gastoner
force-pushed
the
copilot_agent_configuration
branch
from
5a09812 to
eec7b44
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
extensions/copilot/src/extension.spec.ts (1)
135-151:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a regression case for malformed
chatfield shape.This table only exercises non-object top-level payloads. Add a case where top-level JSON is an object but
chatis malformed (string/array/null) to lock in expected normalization behavior.Suggested test addition
+ test.each([ + JSON.stringify({ chat: 'bad-shape', other: true }), + JSON.stringify({ chat: null, other: true }), + JSON.stringify({ chat: [1, 2], other: true }), + ])('normalizes malformed chat field: %s', async payload => { + await activate(extensionContextMock); + const agent = getRegisteredAgent(); + + const configFile = createConfigFile(payload); + 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, Add regression test cases to the test.each() array in the test labeled 'falls back to empty config when parsed JSON is non-object' that cover scenarios where the top-level JSON is a valid object but the chat field inside is malformed. Include test cases such as JSON objects with chat being a string, array, or null value (e.g., '{"chat":"invalid"}', '{"chat":[]}', '{"chat":null}') to verify that the normalization correctly handles these malformed chat field shapes and still falls back to the expected empty config structure with model set to 'gpt-4o'.extensions/copilot/src/extension.ts (1)
73-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
config.chatruntime shape before settingmodel.On Line 73,
config.chatis only type-cast. If the JSON contains a primitivechatvalue (for example,"chat": "bad"), Line 74 can throw during assignment and failpreWorkspaceStart.Suggested 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 73 - 75, The type assertion on line 73 does not validate the actual runtime shape of config.chat. If the JSON configuration contains a primitive value (such as a string) instead of an object for the chat field, line 74 will throw an error when trying to assign the model property. Add runtime validation to check that config.chat is either undefined, null, or is actually an object before treating it as a Record. Use a type guard or typeof check to ensure config.chat is an object shape before proceeding with the model assignment.
🤖 Prompt for all review comments with 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.
Duplicate comments:
In `@extensions/copilot/src/extension.spec.ts`:
- Around line 135-151: Add regression test cases to the test.each() array in the
test labeled 'falls back to empty config when parsed JSON is non-object' that
cover scenarios where the top-level JSON is a valid object but the chat field
inside is malformed. Include test cases such as JSON objects with chat being a
string, array, or null value (e.g., '{"chat":"invalid"}', '{"chat":[]}',
'{"chat":null}') to verify that the normalization correctly handles these
malformed chat field shapes and still falls back to the expected empty config
structure with model set to 'gpt-4o'.
In `@extensions/copilot/src/extension.ts`:
- Around line 73-75: The type assertion on line 73 does not validate the actual
runtime shape of config.chat. If the JSON configuration contains a primitive
value (such as a string) instead of an object for the chat field, line 74 will
throw an error when trying to assign the model property. Add runtime validation
to check that config.chat is either undefined, null, or is actually an object
before treating it as a Record. Use a type guard or typeof check to ensure
config.chat is an object shape before proceeding with the model assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 29a18813-5346-44fd-ae30-31cc5b4ee87e
📒 Files selected for processing (2)
extensions/copilot/src/extension.spec.tsextensions/copilot/src/extension.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: Windows
- GitHub Check: macOS
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: linter, formatters
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / windows-2022
- GitHub Check: Linux
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
extensions/*/src/extension.ts
📄 CodeRabbit inference engine (AGENTS.md)
Extensions should export a standard activation API from their entry point
Files:
extensions/copilot/src/extension.ts
extensions/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Register inference, container, and Kubernetes providers through the
ProviderRegistryvia extension APIs
Files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
**/*.spec.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx,js,jsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in unit tests; avoid manual mock factories when possible
Usevi.resetAllMocks()inbeforeEachhooks instead ofvi.clearAllMocks()for resetting mocks between tests
When an auto-mocked function or class method needs a real implementation, usevi.mocked(...)with the prototype pattern for class methods:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
extensions/copilot/src/extension.spec.ts
🧠 Learnings (5)
📚 Learning: 2026-05-05T17:30:20.418Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:272-302
Timestamp: 2026-05-05T17:30:20.418Z
Learning: In the openkaiden/kaiden repo, for cloud inference provider extension code under `extensions/*/src/*.ts`, treat `ProviderConnectionStatus = 'unknown'` as a valid/expected value when registering provider connections (e.g., Gemini/Claude/Mistral/OpenAI-compatible/Vertex AI). `'unknown'` indicates the connection was set up but is not continuously monitored—so do not flag it as incorrect. Only Ollama is expected to use `'started'` because it actively polls a local server.
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
📚 Learning: 2026-05-05T17:44:50.991Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/vertex-ai.ts:363-387
Timestamp: 2026-05-05T17:44:50.991Z
Learning: In this repo (openkaiden/kaiden), do not raise a code review issue when an extension’s `InferenceProviderConnectionFactory.create` factory method implementation omits (or does not use) the optional `logger` and/or `CancellationToken` parameters in its method signature/implementation. Current extensions (e.g., Vertex AI, Gemini, Claude, Mistral, OpenAI-compatible) follow this pattern, so reviewers should treat it as acceptable for `extensions/*` TypeScript source files.
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
📚 Learning: 2026-05-06T11:15:56.238Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1671
File: extensions/vertex-ai/src/extension.spec.ts:43-51
Timestamp: 2026-05-06T11:15:56.238Z
Learning: In all extensions under extensions/*/src/extension.ts, deactivate() should only clear the module-level instance reference (e.g., set the instance to undefined) and must not call dispose() directly. The dispose() method is invoked by the extension host when processing extensionContext.subscriptions. Do not suggest asserting dispose() in tests for deactivate(); such assertions are unnecessary because disposal is handled by the host and CI checks should validate subscriptions handling instead.
Applied to files:
extensions/copilot/src/extension.ts
📚 Learning: 2026-05-12T10:01:14.248Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1810
File: extensions/kdn/src/kdn-extension.ts:43-46
Timestamp: 2026-05-12T10:01:14.248Z
Learning: In this repo’s extension code, when logging from binary discovery/resolution logic (e.g., choosing/validating custom paths, extension storage locations, or bundled resource paths), it’s intentional to include full filesystem paths in `console.log`/`console.warn` (such as in `extensions/**/src/*-extension.ts`). During review, do not flag these specific full-path messages as a privacy/security issue as long as they are clearly part of the binary resolution steps. If full-path logging appears outside binary discovery/resolution, review/flag it as usual.
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
extensions/copilot/src/extension.tsextensions/copilot/src/extension.spec.ts
There was a problem hiding this comment.
Overall LGTM!
Small nonblocking nit that I got help from ai. Supposedly the chat field cast is fragile. At extension.ts:67 const chat = (config.chat as Record<string, unknown> | undefined) ?? {};
- If config.chat is a non-object value (e.g., "chat": 123 or "chat": [1,2])" the cast would succeed
but we dont use chat at all rn so could be ignored hah
Just needs a rebase and gtg
gastoner
force-pushed
the
copilot_agent_configuration
branch
from
eec7b44 to
cd67f0a
Compare
|
rebased |
Implement settings.json configuration file management for the copilot agent. The preWorkspaceStart hook now writes the selected model into the copilot settings file so the agent launches with the correct model. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
gastoner
force-pushed
the
copilot_agent_configuration
branch
from
cd67f0a to
00be7a2
Compare
2 participants
Implement settings.json configuration file management for the copilot agent. The preWorkspaceStart hook now writes the selected model into the copilot settings file so the agent launches with the correct model.
Closes #2171