Conversation
| private readonly baseUrl: string | ||
| private readonly authHeader: string | ||
| private readonly authUsername = "opencode" | ||
| private readonly authUsername = "kilo" |
There was a problem hiding this comment.
not sure why this broke, but it seems like this was changed in the backend breaking the vscode extension.
The mockGitContext variable was declared without a type annotation, causing TypeScript to infer a narrow type from the initial assignment (status: "modified"). This made reassignment with status: "added" fail with TS2322. Adding the GitContext type allows all valid status values.
|
Run from extension or commandline: |
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| connectionService: KiloConnectionService, | ||
| ): vscode.Disposable[] { | ||
| const command = vscode.commands.registerCommand("kilo-code.new.generateCommitMessage", async () => { | ||
| const extension = vscode.extensions.getExtension<GitExtensionExports>("vscode.git") |
There was a problem hiding this comment.
SUGGESTION: Ensure the built-in Git extension is activated before accessing exports
vscode.extensions.getExtension('vscode.git') can return an extension object with exports undefined until it’s activated. Consider await extension.activate() before calling extension.exports.getAPI(1) to avoid incorrectly reporting "No Git repository found".
There was a problem hiding this comment.
CRITICAL: connectionService.getHttpClient() appears to throw, so this null-check won’t work
packages/kilo-vscode/src/services/cli-backend/connection-service.ts defines getHttpClient(): HttpClient and throws when not connected. This line will throw before reaching the if (!client) branch, so the command will likely fail without showing "Kilo backend is not connected".
Consider wrapping this call in a try/catch (or adding a non-throwing accessor) so the user gets the intended error message.
There was a problem hiding this comment.
WARNING: --staged-only option is defined but not used
The command builder defines staged-only (default true), but the handler always calls generateCommitMessage({ path: process.cwd() }) without using args['staged-only']. This will surprise users and makes the option misleading.
Either wire this flag through to the underlying git-context selection logic, or remove the option until it’s supported.
| function parseNameStatus(output: string): Array<{ status: string; path: string }> { | ||
| if (!output) return [] | ||
| return output.split("\n").map((line) => { | ||
| const [status, ...rest] = line.split("\t") |
There was a problem hiding this comment.
WARNING: Rename entries from git diff --name-status are not parsed correctly
For rename status lines (e.g. R100 old.ts new.ts), rest.join('\t') produces a combined string like old.ts new.ts. Downstream, this entry.path is treated as a single path (lock-file filtering, selectedFiles filtering, and git diff ... -- entry.path), which will break for renames.
Consider parsing rename lines into a structured shape (old/new), and using the new path for .path (and/or storing both).
| }) | ||
| } | ||
|
|
||
| function mapStatus(code: string): FileChange["status"] { |
There was a problem hiding this comment.
SUGGESTION: FileChange['status'] includes untracked but it’s never produced
mapStatus() maps ?? / ? to added, while FileChange['status'] allows untracked. If untracked is intentionally modeled, consider returning untracked for ?? and adjusting handling; otherwise consider removing untracked from the union to avoid confusion.
| validator( | ||
| "json", | ||
| z.object({ | ||
| path: z.string().meta({ description: "Workspace/repo path" }), |
There was a problem hiding this comment.
WARNING: Route accepts an arbitrary filesystem path from the client
This endpoint uses the caller-provided path as the repo directory for git operations. Even though this server is local + authenticated, it still broadens the API surface (any client that can reach the backend can point it at arbitrary directories).
Consider restricting path to the active instance/workspace directory (e.g. from the instance context / directory header) or validating that path is within an allowed workspace root.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)CRITICAL
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (9 files)
|
| }) | ||
|
|
||
| it("shows error when no git repository is found", async () => { | ||
| vi.mocked(vscode.extensions.getExtension).mockReturnValue({ |
There was a problem hiding this comment.
WARNING: Mocked vscode.git extension is missing isActive/activate, which can cause the command callback to throw
registerCommitMessageService() now checks if (!extension.isActive) await extension.activate(). In this test, the mocked extension object only provides exports, so extension.activate will be undefined and the callback will throw before reaching the "No Git repository found" branch.
Add isActive: true (or activate: vi.fn().mockResolvedValue(undefined) + isActive: false) to the mocked extension return values so the test exercises the intended path.
| }), | ||
| }, | ||
| } as any) | ||
| vi.mocked(mockConnectionService.getHttpClient as any).mockReturnValue(null) |
There was a problem hiding this comment.
WARNING: Test setup implies getHttpClient() returns null, but the production code assumes it throws (and never null-checks)
This test does mockReturnValue(null) and expects an error toast, but registerCommitMessageService() only handles the disconnected case via try/catch and then calls client.generateCommitMessage(...) unconditionally. If getHttpClient() ever returned null without throwing, this would become a runtime error instead of a user-facing message.
Either (a) update the test to mockImplementation(() => { throw new Error('...') }) to match the connection service contract, or (b) add an explicit if (!client) guard in the command implementation.
| return | ||
| } | ||
|
|
||
| const path = folder.uri.fsPath |
There was a problem hiding this comment.
SUGGESTION: Use the git repository root path instead of the first workspace folder
When VS Code has multiple workspace folders (or the repo is not the first folder), using workspaceFolders[0] can generate a commit message for the wrong directory. Since you already have repository.rootUri, using that is more precise.
| const path = folder.uri.fsPath | |
| const path = repository.rootUri.fsPath |
|
Found 12 test failures on Blacksmith runners: Failures
|
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }) | ||
| return result.stdout.toString().trim() |
There was a problem hiding this comment.
CRITICAL: git() trims leading whitespace, which can corrupt git status --porcelain parsing
Using packages/opencode/src/commit-message/git-context.ts:137, .trim() can remove the leading space of the first porcelain line (e.g. " M file"), shifting the fixed-column layout that parsePorcelain() relies on (slice(0,2) / slice(3)). This can drop the first character of the filepath (and then subsequent git diff -- <path> calls target the wrong file).
| return result.stdout.toString().trim() | |
| return result.stdout.toString().trimEnd() |
| .withProgress( | ||
| { location: vscode.ProgressLocation.SourceControl, title: "Generating commit message..." }, | ||
| async () => { | ||
| const message = await client.generateCommitMessage(path, undefined, previousMessage) |
There was a problem hiding this comment.
CRITICAL: Possible runtime error if getHttpClient() returns undefined/null instead of throwing
packages/kilo-vscode/src/services/commit-message/index.ts:65 calls client.generateCommitMessage(...), but client is declared as HttpClient | undefined and the code only handles the throwing case. If connectionService.getHttpClient() returns undefined/null when disconnected, this will throw and show a generic failure message. Consider explicitly checking client after the call and returning early with a targeted error message.
2 participants
What
AI-powered commit message generation using Conventional Commits format. Uses
small_modelfrom the configured provider — no user model selection needed.Three surfaces
kilo commit: generates a commit message from staged changes and commitsPOST /commit-message: backend endpoint for commit message generationArchitecture
Shared core module in
packages/opencode/src/commit-message/handles git context gathering (diff, staged files) and LLM-based message generation. The VS Code extension and CLI both consume this via the HTTP route.New files
packages/opencode/src/commit-message/generate.ts— core generation logicpackages/opencode/src/commit-message/git-context.ts— git diff/staged file gatheringpackages/opencode/src/commit-message/types.ts— shared typespackages/opencode/src/commit-message/index.ts— barrel exportpackages/opencode/src/commit-message/__tests__/— unit testspackages/opencode/src/server/routes/commit-message.ts— HTTP route handlerpackages/opencode/src/cli/cmd/commit.ts— CLI commandpackages/kilo-vscode/src/services/commit-message/index.ts— VS Code SCM integrationpackages/kilo-vscode/src/services/commit-message/__tests__/index.spec.ts— testspackages/kilo-vscode/docs/commit-message-implementation-plan.md— implementation planpackages/kilo-vscode/docs/commit-message-reimplementation.md— design docModified files
packages/opencode/src/server/server.ts— register new routepackages/opencode/src/index.ts— export commit-message modulepackages/kilo-vscode/src/services/cli-backend/http-client.ts— addgenerateCommitMessage()methodpackages/kilo-vscode/src/extension.ts— register SCM commandpackages/kilo-vscode/package.json— add SCM menu contribution