feat: add kortex-cli extension to embed CLI for workspace management#1213
Open
bmahabirbu wants to merge 1 commit intokortex-hub:mainfrom
Open
feat: add kortex-cli extension to embed CLI for workspace management#1213bmahabirbu wants to merge 1 commit intokortex-hub:mainfrom
bmahabirbu wants to merge 1 commit intokortex-hub:mainfrom
Conversation
bmahabirbu
requested review from
benoitf and
fbricon
and removed request for
a team
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds a new kortex-cli extension: package manifest and build tooling, Vite/TypeScript config, an extension entrypoint, a GitHub-release-based downloader, CLI discovery/installer wiring, and a platform helper to resolve binaries. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Host as Extension Host
participant KCLI as KortexCLI
participant Downloader as KortexCliDownloader
participant GitHub as GitHub API
participant FS as File System
participant Sys as System Shell
User->>Host: Activate extension
Host->>KCLI: init()
KCLI->>KCLI: findKortexVersion()
alt extension-managed executable exists
KCLI->>FS: existsSync(installDir)
FS-->>KCLI: exists
KCLI->>Sys: exec("--version") on installed binary
Sys-->>KCLI: version
else not found / user chooses install
KCLI->>Downloader: selectVersion()
Downloader->>GitHub: listReleases()
GitHub-->>Downloader: releases
Downloader-->>User: show QuickPick
User->>Downloader: select release
Downloader->>GitHub: getReleaseAsset(assetId)
GitHub-->>Downloader: asset stream
Downloader->>FS: write asset file
Downloader->>FS: extract (.zip/.tar.gz)
FS-->>Downloader: extraction complete
Downloader->>FS: chmod +x on Unix
Downloader-->>KCLI: return installed path
KCLI->>KCLI: update tool metadata (path, version)
end
KCLI-->>Host: CLI initialized
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
extensions/kortex-cli/src/extension.ts (1)
38-40: Inconsistent ordering:subscriptions.pushbefore vs afterinit().The
downloaderis registered in subscriptions afterinit()completes (lines 35-36), butkortexCLIis registered beforeinit()is called (lines 39-40). This inconsistency could cause issues ifkortexCLI.init()throws—the object would remain in subscriptions in a partially initialized state.Consider making the ordering consistent:
♻️ Suggested fix
const kortexCLI = new KortexCLI(cli, processAPI, downloader, env); - extensionContext.subscriptions.push(kortexCLI); await kortexCLI.init(); + extensionContext.subscriptions.push(kortexCLI);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/src/extension.ts` around lines 38 - 40, The subscription registration ordering is inconsistent and can leave a partially-initialized KortexCLI in subscriptions if init() throws; change the code so KortexCLI is initialized before being registered: call await kortexCLI.init() first, then call extensionContext.subscriptions.push(kortexCLI). Ensure this mirrors the downloader registration pattern (register downloader in extensionContext.subscriptions only after its initialization completes) and update any related cleanup logic that expects the object to be present only after successful init.extensions/kortex-cli/scripts/build.js (1)
42-46: Extraction runs asynchronously after script appears to complete.The
mkdirp().then()pattern means the Node process may exit before extraction finishes, potentially leaving an incompletebuiltin/directory. While this matches the pattern in sibling extensions (e.g.,extensions/ollama/scripts/build.js), usingawaitwould be more robust.♻️ Proposed fix using async/await
-// create unzipped built-in -mkdirp(unzippedDirectory).then(() => { - const unzip = new AdmZip(destFile); - unzip.extractAllTo(unzippedDirectory); -}); +// create unzipped built-in +await mkdirp(unzippedDirectory); +const unzip = new AdmZip(destFile); +unzip.extractAllTo(unzippedDirectory);This would require wrapping the script body in an async IIFE or making it a module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/scripts/build.js` around lines 42 - 46, The extraction currently runs asynchronously because mkdirp(...).then(...) may let the Node process exit before AdmZip(destFile).extractAllTo finishes; change this to use async/await so the process waits: wrap the script body in an async IIFE (or make the file an ES module) and await mkdirp(unzippedDirectory) before creating new AdmZip(destFile) and await the extraction step (ensure extractAllTo is awaited or use a synchronous alternative) so that unzippedDirectory is fully written before the script exits; reference mkdirp, AdmZip, destFile, unzippedDirectory, and extractAllTo when locating the code to change.extensions/kortex-cli/src/kortex-cli-downloader.ts (2)
89-94: Fail fast if extraction didn’t produce the CLI binary.
install()currently returns the expected path even when that file was not extracted. Throwing here keeps the failure in the install flow instead of surfacing later when the tool is launched.Suggested change
const executablePath = this.getKortexExecutablePath(); - if (!this.envAPI.isWindows && existsSync(executablePath)) { - await chmod(executablePath, 0o755); + if (!existsSync(executablePath)) { + throw new Error('Kortex CLI executable was not found after extraction'); + } + if (!this.envAPI.isWindows) { + await chmod(executablePath, 0o755); } return executablePath;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` around lines 89 - 94, The install() flow currently returns getKortexExecutablePath() even if extraction failed; update the end of install() to verify the binary exists and is executable: after calling getKortexExecutablePath() and performing the chmod for non-Windows (using envAPI.isWindows and existsSync), if the file does not exist (existsSync returns false) throw a descriptive Error (e.g. "Kortex CLI binary not found after extraction: <path>") so the install step fails fast rather than returning a non-existent path.
19-19: PreferexecFile()over shell interpolation fortar.
filePathis derived from release metadata, so passing it throughexec("tar ...")makes extraction sensitive to shell metacharacters and quoting edge cases.execFile()keeps the arguments opaque.Suggested change
-import { exec } from 'node:child_process'; +import { execFile } from 'node:child_process'; @@ extractTarGz(filePath: string, outDir: string): Promise<void> { return new Promise((resolve, reject) => { - // eslint-disable-next-line sonarjs/os-command - exec(`tar -xzf "${filePath}" -C "${outDir}"`, err => { + execFile('tar', ['-xzf', filePath, '-C', outDir], err => { if (err) reject(err); else resolve(); }); }); }Also applies to: 65-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` at line 19, The code uses exec(...) to run tar with a shell-interpolated filePath (see the import exec from 'node:child_process' and the locations where filePath is passed into a tar command around lines 19 and 65-69), which is unsafe; replace those exec calls with execFile (or spawn) so you pass tar and its arguments as an array (e.g., program 'tar' and args like '-xzf', filePath, '-C', dest) to keep filePath opaque to the shell, and update the callback/error handling to match execFile's signature; ensure you remove any shell interpolation and still handle errors and stdout/stderr the same way.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/kortex-cli/src/kortex-cli-downloader.ts`:
- Around line 57-60: The install path fails if `#installDirectory` was removed by
uninstall() because init() is only called once; update the install() method in
the KortexCliDownloader (and similarly ensure any other extraction callers) to
recreate the target directory before running tar -C by calling
mkdir(this.#installDirectory, { recursive: true }) or equivalent; ensure
install() checks/creates this.#installDirectory immediately before extraction so
repeated uninstall/install in the same session succeeds.
- Around line 108-123: selectVersion may filter out all releases
(releasesMetadata) and still call showQuickPick, and getLatestVersionAsset can
return undefined despite its non-optional signature; after you filter
releasesMetadata (in selectVersion) check for releasesMetadata.length === 0 and
throw a clear Error like "no other Kortex CLI versions available" before calling
windowAPI.showQuickPick, and similarly update getLatestVersionAsset to validate
its candidate list (or the asset result) and throw a descriptive Error instead
of returning undefined so both methods always either return the expected
non-optional type or raise a clear exception (refer to selectVersion,
releasesMetadata, windowAPI.showQuickPick and getLatestVersionAsset to locate
the code to change).
In `@extensions/kortex-cli/src/utils/system.ts`:
- Around line 26-32: The code in the envAPI.isLinux/isMac branch returns the raw
stdout from processAPI.exec('which', [executable]) (captured as fullPath) which
includes a trailing newline; fix by trimming whitespace (e.g., call .trim() on
the stdout before returning) so callers get a clean path string from
processAPI.exec / fullPath; ensure you still handle the catch path unchanged.
- Around line 34-39: The code currently strips all newlines from the output of
processAPI.exec('where.exe', [executable]) resulting in concatenated paths;
change the post-processing of the returned fullPath to split on newline (e.g.,
/\r?\n/) and return only the first non-empty trimmed line (use the result of
processAPI.exec and the fullPath variable), leaving the existing catch behavior
intact. This ensures only the first path from where.exe is returned.
---
Nitpick comments:
In `@extensions/kortex-cli/scripts/build.js`:
- Around line 42-46: The extraction currently runs asynchronously because
mkdirp(...).then(...) may let the Node process exit before
AdmZip(destFile).extractAllTo finishes; change this to use async/await so the
process waits: wrap the script body in an async IIFE (or make the file an ES
module) and await mkdirp(unzippedDirectory) before creating new AdmZip(destFile)
and await the extraction step (ensure extractAllTo is awaited or use a
synchronous alternative) so that unzippedDirectory is fully written before the
script exits; reference mkdirp, AdmZip, destFile, unzippedDirectory, and
extractAllTo when locating the code to change.
In `@extensions/kortex-cli/src/extension.ts`:
- Around line 38-40: The subscription registration ordering is inconsistent and
can leave a partially-initialized KortexCLI in subscriptions if init() throws;
change the code so KortexCLI is initialized before being registered: call await
kortexCLI.init() first, then call
extensionContext.subscriptions.push(kortexCLI). Ensure this mirrors the
downloader registration pattern (register downloader in
extensionContext.subscriptions only after its initialization completes) and
update any related cleanup logic that expects the object to be present only
after successful init.
In `@extensions/kortex-cli/src/kortex-cli-downloader.ts`:
- Around line 89-94: The install() flow currently returns
getKortexExecutablePath() even if extraction failed; update the end of install()
to verify the binary exists and is executable: after calling
getKortexExecutablePath() and performing the chmod for non-Windows (using
envAPI.isWindows and existsSync), if the file does not exist (existsSync returns
false) throw a descriptive Error (e.g. "Kortex CLI binary not found after
extraction: <path>") so the install step fails fast rather than returning a
non-existent path.
- Line 19: The code uses exec(...) to run tar with a shell-interpolated filePath
(see the import exec from 'node:child_process' and the locations where filePath
is passed into a tar command around lines 19 and 65-69), which is unsafe;
replace those exec calls with execFile (or spawn) so you pass tar and its
arguments as an array (e.g., program 'tar' and args like '-xzf', filePath, '-C',
dest) to keep filePath opaque to the shell, and update the callback/error
handling to match execFile's signature; ensure you remove any shell
interpolation and still handle errors and stdout/stderr the same way.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9679b3cb-01aa-47fb-8495-f987ee8f822f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
extensions/kortex-cli/package.jsonextensions/kortex-cli/scripts/build.jsextensions/kortex-cli/src/extension.tsextensions/kortex-cli/src/kortex-cli-downloader.tsextensions/kortex-cli/src/kortex-cli.tsextensions/kortex-cli/src/utils/system.tsextensions/kortex-cli/tsconfig.jsonextensions/kortex-cli/vite.config.jspackage.json
Comment on lines
+57
to
+60
| async init(): Promise<void> { | ||
| if (!existsSync(this.#installDirectory)) { | ||
| await mkdir(this.#installDirectory, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
Recreate #installDirectory inside install().
init() is the only place that creates this directory, but uninstall() removes it. A reinstall in the same session will fail on macOS/Linux because tar -C expects the extraction target to already exist.
Proposed fix
async install(release: ReleaseArtifactMetadata): Promise<string> {
const destFile = await this.download(release);
+ await mkdir(this.#installDirectory, { recursive: true });
if (destFile.endsWith('.zip')) {
const directory = await Open.file(destFile);
await directory.extract({ path: this.#installDirectory });Also applies to: 75-82, 97-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` around lines 57 - 60, The
install path fails if `#installDirectory` was removed by uninstall() because
init() is only called once; update the install() method in the
KortexCliDownloader (and similarly ensure any other extraction callers) to
recreate the target directory before running tar -C by calling
mkdir(this.#installDirectory, { recursive: true }) or equivalent; ensure
install() checks/creates this.#installDirectory immediately before extraction so
repeated uninstall/install in the same session succeeds.
Comment on lines
+108
to
+123
| async selectVersion(cliInfo?: CliTool): Promise<ReleaseArtifactMetadata> { | ||
| let releasesMetadata = await this.grabLatestReleasesMetadata(); | ||
|
|
||
| if (releasesMetadata.length === 0) throw new Error('cannot grab kortex-cli releases'); | ||
|
|
||
| if (cliInfo) { | ||
| releasesMetadata = releasesMetadata.filter(release => release.tag.slice(1) !== cliInfo.version); | ||
| } | ||
|
|
||
| const selectedRelease = await this.windowAPI.showQuickPick(releasesMetadata, { | ||
| placeHolder: 'Select Kortex CLI version to download', | ||
| }); | ||
|
|
||
| if (!selectedRelease) { | ||
| throw new Error('No version selected'); | ||
| } |
There was a problem hiding this comment.
Handle the empty release list after post-processing.
After filtering out the currently installed version, selectVersion() can end up with no candidates and still fall through to No version selected. getLatestVersionAsset() has the same gap and currently returns undefined despite its non-optional return type.
Proposed fix
async selectVersion(cliInfo?: CliTool): Promise<ReleaseArtifactMetadata> {
let releasesMetadata = await this.grabLatestReleasesMetadata();
if (releasesMetadata.length === 0) throw new Error('cannot grab kortex-cli releases');
if (cliInfo) {
releasesMetadata = releasesMetadata.filter(release => release.tag.slice(1) !== cliInfo.version);
}
+ if (releasesMetadata.length === 0) {
+ throw new Error('No other Kortex CLI versions are available');
+ }
const selectedRelease = await this.windowAPI.showQuickPick(releasesMetadata, {
placeHolder: 'Select Kortex CLI version to download',
});
@@
async getLatestVersionAsset(): Promise<ReleaseArtifactMetadata> {
const latestReleases = await this.grabLatestReleasesMetadata();
+ if (latestReleases.length === 0) {
+ throw new Error('cannot grab kortex-cli releases');
+ }
return latestReleases[0];
}Also applies to: 144-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` around lines 108 - 123,
selectVersion may filter out all releases (releasesMetadata) and still call
showQuickPick, and getLatestVersionAsset can return undefined despite its
non-optional signature; after you filter releasesMetadata (in selectVersion)
check for releasesMetadata.length === 0 and throw a clear Error like "no other
Kortex CLI versions available" before calling windowAPI.showQuickPick, and
similarly update getLatestVersionAsset to validate its candidate list (or the
asset result) and throw a descriptive Error instead of returning undefined so
both methods always either return the expected non-optional type or raise a
clear exception (refer to selectVersion, releasesMetadata,
windowAPI.showQuickPick and getLatestVersionAsset to locate the code to change).
Comment on lines
+26
to
+32
| if (envAPI.isLinux || envAPI.isMac) { | ||
| try { | ||
| const { stdout: fullPath } = await processAPI.exec('which', [executable]); | ||
| return fullPath; | ||
| } catch (err) { | ||
| console.warn('Error getting full path', err); | ||
| } |
There was a problem hiding this comment.
Linux/macOS: trim trailing newline from which output.
The which command output includes a trailing newline. Returning it untrimmed could cause path comparison or file access issues.
🐛 Proposed fix
if (envAPI.isLinux || envAPI.isMac) {
try {
const { stdout: fullPath } = await processAPI.exec('which', [executable]);
- return fullPath;
+ return fullPath.trim();
} catch (err) {
console.warn('Error getting full path', err);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (envAPI.isLinux || envAPI.isMac) { | |
| try { | |
| const { stdout: fullPath } = await processAPI.exec('which', [executable]); | |
| return fullPath; | |
| } catch (err) { | |
| console.warn('Error getting full path', err); | |
| } | |
| if (envAPI.isLinux || envAPI.isMac) { | |
| try { | |
| const { stdout: fullPath } = await processAPI.exec('which', [executable]); | |
| return fullPath.trim(); | |
| } catch (err) { | |
| console.warn('Error getting full path', err); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/kortex-cli/src/utils/system.ts` around lines 26 - 32, The code in
the envAPI.isLinux/isMac branch returns the raw stdout from
processAPI.exec('which', [executable]) (captured as fullPath) which includes a
trailing newline; fix by trimming whitespace (e.g., call .trim() on the stdout
before returning) so callers get a clean path string from processAPI.exec /
fullPath; ensure you still handle the catch path unchanged.
Comment on lines
+34
to
+39
| try { | ||
| const { stdout: fullPath } = await processAPI.exec('where.exe', [executable]); | ||
| return fullPath.replace(/(\r\n|\n|\r)/gm, ''); | ||
| } catch (err) { | ||
| console.warn('Error getting full path', err); | ||
| } |
There was a problem hiding this comment.
Windows where.exe may return multiple paths; only the first should be used.
where.exe outputs one path per line if the executable exists in multiple PATH locations. The current regex removes all line breaks, concatenating the paths into an invalid string (e.g., C:\path1\kortex.exeC:\path2\kortex.exe).
🐛 Proposed fix: return only the first line
try {
const { stdout: fullPath } = await processAPI.exec('where.exe', [executable]);
- return fullPath.replace(/(\r\n|\n|\r)/gm, '');
+ return fullPath.split(/\r?\n/)[0].trim();
} catch (err) {
console.warn('Error getting full path', err);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| const { stdout: fullPath } = await processAPI.exec('where.exe', [executable]); | |
| return fullPath.replace(/(\r\n|\n|\r)/gm, ''); | |
| } catch (err) { | |
| console.warn('Error getting full path', err); | |
| } | |
| try { | |
| const { stdout: fullPath } = await processAPI.exec('where.exe', [executable]); | |
| return fullPath.split(/\r?\n/)[0].trim(); | |
| } catch (err) { | |
| console.warn('Error getting full path', err); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/kortex-cli/src/utils/system.ts` around lines 34 - 39, The code
currently strips all newlines from the output of processAPI.exec('where.exe',
[executable]) resulting in concatenated paths; change the post-processing of the
returned fullPath to split on newline (e.g., /\r?\n/) and return only the first
non-empty trimmed line (use the result of processAPI.exec and the fullPath
variable), leaving the existing catch behavior intact. This ensures only the
first path from where.exe is returned.
7391cfc to
868314f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
extensions/kortex-cli/src/kortex-cli-downloader.ts (2)
75-83:⚠️ Potential issue | 🔴 CriticalRecreate install directory inside install flow before extraction.
After
uninstall()removes the directory, a same-session reinstall can fail because extraction targets a missing path.Suggested fix
async install(release: ReleaseArtifactMetadata): Promise<string> { const destFile = await this.download(release); + await mkdir(this.#installDirectory, { recursive: true }); if (destFile.endsWith('.zip')) { const directory = await Open.file(destFile); await directory.extract({ path: this.#installDirectory });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` around lines 75 - 83, The install flow can fail if uninstall() removed the install directory—ensure the directory exists before extracting: in install(release: ReleaseArtifactMetadata) after const destFile = await this.download(release) and before any extraction (both the '.zip' branch that calls Open.file(...).extract(...) and the '.tar.gz' branch that calls extractTarGz), recreate the install directory (this.#installDirectory) atomically (mkdir -p semantics) with appropriate permissions so extraction has a valid target; update extractTarGz and zip extraction call sites to assume the directory is present.
113-117:⚠️ Potential issue | 🟡 MinorGuard empty release candidates after filtering and before returning latest.
Both flows can end up with no candidates; one shows a misleading selection error and the other can return
undefinedfrom a non-optional method.Suggested fix
async selectVersion(cliInfo?: CliTool): Promise<ReleaseArtifactMetadata> { let releasesMetadata = await this.grabLatestReleasesMetadata(); @@ if (cliInfo) { releasesMetadata = releasesMetadata.filter(release => release.tag.slice(1) !== cliInfo.version); } + if (releasesMetadata.length === 0) { + throw new Error('No other Kortex CLI versions are available'); + } @@ async getLatestVersionAsset(): Promise<ReleaseArtifactMetadata> { const latestReleases = await this.grabLatestReleasesMetadata(); + if (latestReleases.length === 0) { + throw new Error('cannot grab kortex-cli releases'); + } return latestReleases[0]; }Also applies to: 144-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` around lines 113 - 117, After filtering out the current CLI version (in the block using cliInfo and releasesMetadata) add a guard that checks if releasesMetadata is empty before calling this.windowAPI.showQuickPick and before the code path that returns the latest release; if empty, surface a clear error/prompt to the user (or throw/return a defined value) instead of proceeding and producing a misleading selection error or returning undefined from the non-optional method. Apply the same empty-check guard to the other flow referenced around the code that returns the latest release (the alternate branch at lines ~144-146) so both selectedRelease and the “latest” return path never operate on an empty releasesMetadata array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/kortex-cli/src/kortex-cli-downloader.ts`:
- Around line 65-69: The extractTarGz function currently uses exec with an
interpolated shell command causing command-injection risk; change it to call
execFile (imported from child_process) or the promisified variant and pass "tar"
as the executable with an argv array like ["-xzf", filePath, "-C", outDir] so
paths are passed as arguments (no shell parsing), and update the Promise
callback to resolve/reject based on execFile's error/result signature in
extractTarGz.
---
Duplicate comments:
In `@extensions/kortex-cli/src/kortex-cli-downloader.ts`:
- Around line 75-83: The install flow can fail if uninstall() removed the
install directory—ensure the directory exists before extracting: in
install(release: ReleaseArtifactMetadata) after const destFile = await
this.download(release) and before any extraction (both the '.zip' branch that
calls Open.file(...).extract(...) and the '.tar.gz' branch that calls
extractTarGz), recreate the install directory (this.#installDirectory)
atomically (mkdir -p semantics) with appropriate permissions so extraction has a
valid target; update extractTarGz and zip extraction call sites to assume the
directory is present.
- Around line 113-117: After filtering out the current CLI version (in the block
using cliInfo and releasesMetadata) add a guard that checks if releasesMetadata
is empty before calling this.windowAPI.showQuickPick and before the code path
that returns the latest release; if empty, surface a clear error/prompt to the
user (or throw/return a defined value) instead of proceeding and producing a
misleading selection error or returning undefined from the non-optional method.
Apply the same empty-check guard to the other flow referenced around the code
that returns the latest release (the alternate branch at lines ~144-146) so both
selectedRelease and the “latest” return path never operate on an empty
releasesMetadata array.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 651667c0-47e4-47ae-8843-e020d4aa3292
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
extensions/kortex-cli/package.jsonextensions/kortex-cli/scripts/build.jsextensions/kortex-cli/src/extension.tsextensions/kortex-cli/src/kortex-cli-downloader.tsextensions/kortex-cli/src/kortex-cli.tsextensions/kortex-cli/src/utils/system.tsextensions/kortex-cli/tsconfig.jsonextensions/kortex-cli/vite.config.jspackage.json
✅ Files skipped from review due to trivial changes (4)
- extensions/kortex-cli/tsconfig.json
- extensions/kortex-cli/package.json
- extensions/kortex-cli/scripts/build.js
- extensions/kortex-cli/src/utils/system.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- extensions/kortex-cli/vite.config.js
- extensions/kortex-cli/src/extension.ts
- extensions/kortex-cli/src/kortex-cli.ts
Comment on lines
+65
to
+69
| extractTarGz(filePath: string, outDir: string): Promise<void> { | ||
| return new Promise((resolve, reject) => { | ||
| // eslint-disable-next-line sonarjs/os-command | ||
| exec(`tar -xzf "${filePath}" -C "${outDir}"`, err => { | ||
| if (err) reject(err); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify shell-based command execution and data flow to command arguments.
rg -n -C2 'exec\(|execFile\(' extensions/kortex-cli/src/kortex-cli-downloader.ts
rg -n -C2 'asset\.name|destination|extractTarGz\(' extensions/kortex-cli/src/kortex-cli-downloader.tsRepository: kortex-hub/kortex
Length of output: 1308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check imports at top of file
head -50 extensions/kortex-cli/src/kortex-cli-downloader.ts | grep -n "import.*child_process"
# Get full extractTarGz method implementation
sed -n '65,75p' extensions/kortex-cli/src/kortex-cli-downloader.ts
# Check how filePath/outDir are used - verify no escaping
rg -n -B5 -A10 'extractTarGz\(destFile' extensions/kortex-cli/src/kortex-cli-downloader.ts
# Check asset.name and storageData sources
rg -n -B3 'const destination = join' extensions/kortex-cli/src/kortex-cli-downloader.tsRepository: kortex-hub/kortex
Length of output: 1255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify GitHub release asset names can contain special characters
# Check GitHub API documentation constraints on asset.name
# Also verify if there's any sanitization of asset.name before it's used
rg -n 'asset\.name' extensions/kortex-cli/src/kortex-cli-downloader.ts
# Check if there's any string escaping utility applied
rg -n 'escape|quote|shell' extensions/kortex-cli/src/kortex-cli-downloader.tsRepository: kortex-hub/kortex
Length of output: 119
Use execFile() with argv array instead of exec() to prevent command injection via interpolated paths.
The exec() call on line 68 runs the command through a shell; filePath and outDir are interpolated as template literals. Since filePath derives from asset.name (which can contain arbitrary characters from GitHub releases), this allows shell metacharacters (backticks, $(...), semicolons, etc.) to be executed as commands rather than passed as literal arguments. Use execFile() with an arguments array to pass data directly to tar without shell parsing.
Suggested fix
-import { exec } from 'node:child_process';
+import { execFile } from 'node:child_process';
@@
extractTarGz(filePath: string, outDir: string): Promise<void> {
return new Promise((resolve, reject) => {
- // eslint-disable-next-line sonarjs/os-command
- exec(`tar -xzf "${filePath}" -C "${outDir}"`, err => {
+ execFile('tar', ['-xzf', filePath, '-C', outDir], err => {
if (err) reject(err);
else resolve();
});
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| extractTarGz(filePath: string, outDir: string): Promise<void> { | |
| return new Promise((resolve, reject) => { | |
| // eslint-disable-next-line sonarjs/os-command | |
| exec(`tar -xzf "${filePath}" -C "${outDir}"`, err => { | |
| if (err) reject(err); | |
| extractTarGz(filePath: string, outDir: string): Promise<void> { | |
| return new Promise((resolve, reject) => { | |
| execFile('tar', ['-xzf', filePath, '-C', outDir], err => { | |
| if (err) reject(err); | |
| else resolve(); | |
| }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` around lines 65 - 69, The
extractTarGz function currently uses exec with an interpolated shell command
causing command-injection risk; change it to call execFile (imported from
child_process) or the promisified variant and pass "tar" as the executable with
an argv array like ["-xzf", filePath, "-C", outDir] so paths are passed as
arguments (no shell parsing), and update the Promise callback to resolve/reject
based on execFile's error/result signature in extractTarGz.
Contributor
Author
There was a problem hiding this comment.
The exec → execFile suggestion was not applied because the goose extension uses the same
exec + // eslint-disable-next-line sonarjs/os-command pattern, and switching would require a
different lint suppression that doesn't match the codebase conventions
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the goose extension for the exec + sonarjs/os-command pattern
rg -n -C3 'sonarjs/os-command\|exec\b' extensions/goose/ 2>/dev/null || \
rg -n -C3 'os-command' extensions/goose/ 2>/dev/null || \
rg -rn -C3 'sonarjs/os-command' extensions/ 2>/dev/null
Length of output: 589
@bmahabirbu, confirmed — extensions/goose/src/goose-downloader.ts lines 68–69 use the exact same exec + // eslint-disable-next-line sonarjs/os-command pattern. Keeping consistency with the established convention is reasonable.
That said, the underlying shell-injection risk remains for both extensions if filePath or outDir ever contain shell metacharacters. It may be worth tracking this as a follow-up to harden both the goose and kortex-cli extractors together at some point. Would you like me to open a GitHub issue to track that?
✏️ Learnings added
Learnt from: bmahabirbu
Repo: kortex-hub/kortex PR: 1213
File: extensions/kortex-cli/src/kortex-cli-downloader.ts:65-69
Timestamp: 2026-04-02T03:06:18.286Z
Learning: In the kortex-hub/kortex repository, the codebase convention for suppressing the `sonarjs/os-command` lint rule when using `exec()` for shell commands (e.g., tar extraction) is `// eslint-disable-next-line sonarjs/os-command` inline before the `exec()` call. This pattern is established in `extensions/goose/src/goose-downloader.ts` (line 68) and mirrored in `extensions/kortex-cli/src/kortex-cli-downloader.ts`. Switching to `execFile()` is intentionally avoided to maintain consistency with this pattern, even though `execFile()` would eliminate the shell-injection risk.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Adds a new built-in extension that downloads and registers the kortex CLI binary from kortex-hub/kortex-cli GitHub releases, enabling users to work with workspaces out of the box. Closes kortex-hub#1127 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Brian <bmahabir@bu.edu>
bmahabirbu
force-pushed
the
embed-kortex-cli-extension
branch
from
868314f to
835b0e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
extensions/kortex-cli/src/kortex-cli-downloader.ts (4)
75-82:⚠️ Potential issue | 🔴 CriticalRecreate the extraction directory inside
install().If
uninstall()(Line 100-103) ran earlier,install()can fail because extraction assumesthis.#installDirectoryalready exists. Create it right before extraction.Proposed fix
async install(release: ReleaseArtifactMetadata): Promise<string> { const destFile = await this.download(release); + await mkdir(this.#installDirectory, { recursive: true }); if (destFile.endsWith('.zip')) { const directory = await Open.file(destFile); await directory.extract({ path: this.#installDirectory });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` around lines 75 - 82, The install() method assumes this.#installDirectory exists before extracting; ensure the directory is recreated just before extraction to avoid failures after uninstall() removed it. Modify install() so after downloading (destFile) and before calling Open.file(...).extract(...) or extractTarGz(destFile,...), create or ensure existence of this.#installDirectory (e.g., mkdir -p behavior) so both .zip extraction via Open.file(...) and .tar.gz extraction in extractTarGz(...) have a valid target directory.
147-149:⚠️ Potential issue | 🟠 MajorDo not return
undefinedfrom a non-optionalPromise<ReleaseArtifactMetadata>.Line 149 can return
undefinedwhen no releases are available, violating the declared return contract and causing downstream runtime failures.#!/bin/bash set -euo pipefail # Verify missing empty-list guard in getLatestVersionAsset() rg -n -C3 'async getLatestVersionAsset|return latestReleases\[0\]' extensions/kortex-cli/src/kortex-cli-downloader.tsProposed fix
async getLatestVersionAsset(): Promise<ReleaseArtifactMetadata> { const latestReleases = await this.grabLatestReleasesMetadata(); + if (latestReleases.length === 0) { + throw new Error('cannot grab kortex-cli releases'); + } return latestReleases[0]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` around lines 147 - 149, The getLatestVersionAsset function currently returns latestReleases[0] which can be undefined when grabLatestReleasesMetadata() returns an empty array; update getLatestVersionAsset to guard against an empty latestReleases (call to grabLatestReleasesMetadata) and if no releases exist, throw a clear Error (or return a rejected Promise) describing "no releases found" so the function never resolves to undefined; reference the getLatestVersionAsset and grabLatestReleasesMetadata symbols and ensure the thrown error type matches the surrounding error-handling conventions.
65-69:⚠️ Potential issue | 🟠 MajorAvoid shell interpolation in archive extraction command.
Line 68 interpolates
filePath/outDirinto a shell command. BecausefilePathis derived from release asset names, shell metacharacters can be interpreted.#!/bin/bash set -euo pipefail # Verify interpolation path from asset.name into exec() command rg -n -C3 'exec\(`tar -xzf' extensions/kortex-cli/src/kortex-cli-downloader.ts rg -n -C3 'const destination = join\(storageData, asset\.name\)' extensions/kortex-cli/src/kortex-cli-downloader.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` around lines 65 - 69, The extractTarGz function currently passes filePath and outDir into a shell string via exec, which risks shell injection from asset-derived names; change it to invoke the tar binary without a shell or use a safe library: replace the exec(`tar -xzf "${filePath}" -C "${outDir}"`, ...) call in extractTarGz with a shellless invocation (e.g. child_process.spawn or execFile with args ['-xzf', filePath, '-C', outDir']) or use a Node tar library (e.g. tar.x) to extract into outDir, ensuring filePath and outDir are provided as arguments rather than interpolated into a shell string.
116-126:⚠️ Potential issue | 🟡 MinorHandle the “no versions left” case after filtering current version.
After Line 117 filtering,
releasesMetadatacan be empty andshowQuickPickfalls through to a misleading “No version selected” error.Proposed fix
if (cliInfo) { releasesMetadata = releasesMetadata.filter(release => release.tag.slice(1) !== cliInfo.version); } + if (releasesMetadata.length === 0) { + throw new Error('No other Kortex CLI versions are available'); + } const selectedRelease = await this.windowAPI.showQuickPick(releasesMetadata, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/src/kortex-cli-downloader.ts` around lines 116 - 126, After filtering out the current version from releasesMetadata (using cliInfo), handle the case where releasesMetadata becomes empty before calling windowAPI.showQuickPick: check if releasesMetadata.length === 0 and either throw a clear error (e.g. 'No other Kortex CLI versions available') or call windowAPI.showInformationMessage to inform the user, then return/exit; otherwise proceed to call windowAPI.showQuickPick and keep the existing selectedRelease null check. Reference symbols: releasesMetadata, cliInfo, windowAPI.showQuickPick, selectedRelease.
🧹 Nitpick comments (1)
extensions/kortex-cli/src/kortex-cli.ts (1)
31-31: Use the/@/alias for non-sibling import paths.
./utils/systemis not a same-directory sibling import; please switch this to the repository alias style.As per coding guidelines: “Use
/@/path aliases for imports outside the current directory's module group … Use relative imports only for sibling modules within the same directory.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/kortex-cli/src/kortex-cli.ts` at line 31, Replace the non-sibling relative import of whereBinary in kortex-cli.ts with the repository alias form; specifically change the import that currently references './utils/system' to use the /@/ path alias pointing to the utils/system module so the symbol whereBinary is imported via the alias rather than a relative path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/kortex-cli/src/kortex-cli.ts`:
- Around line 67-70: The value returned from whereBinary (assigned to the local
variable location) may contain trailing whitespace/newline; trim that value
before persisting it as the CLI path. Update the return in the function that
calls whereBinary (the block assigning const location = await
whereBinary(this.envAPI, this.processAPI, executable)) to use location.trim()
(or otherwise sanitize) so the returned object’s path property is stored without
trailing newlines or spaces.
---
Duplicate comments:
In `@extensions/kortex-cli/src/kortex-cli-downloader.ts`:
- Around line 75-82: The install() method assumes this.#installDirectory exists
before extracting; ensure the directory is recreated just before extraction to
avoid failures after uninstall() removed it. Modify install() so after
downloading (destFile) and before calling Open.file(...).extract(...) or
extractTarGz(destFile,...), create or ensure existence of this.#installDirectory
(e.g., mkdir -p behavior) so both .zip extraction via Open.file(...) and .tar.gz
extraction in extractTarGz(...) have a valid target directory.
- Around line 147-149: The getLatestVersionAsset function currently returns
latestReleases[0] which can be undefined when grabLatestReleasesMetadata()
returns an empty array; update getLatestVersionAsset to guard against an empty
latestReleases (call to grabLatestReleasesMetadata) and if no releases exist,
throw a clear Error (or return a rejected Promise) describing "no releases
found" so the function never resolves to undefined; reference the
getLatestVersionAsset and grabLatestReleasesMetadata symbols and ensure the
thrown error type matches the surrounding error-handling conventions.
- Around line 65-69: The extractTarGz function currently passes filePath and
outDir into a shell string via exec, which risks shell injection from
asset-derived names; change it to invoke the tar binary without a shell or use a
safe library: replace the exec(`tar -xzf "${filePath}" -C "${outDir}"`, ...)
call in extractTarGz with a shellless invocation (e.g. child_process.spawn or
execFile with args ['-xzf', filePath, '-C', outDir']) or use a Node tar library
(e.g. tar.x) to extract into outDir, ensuring filePath and outDir are provided
as arguments rather than interpolated into a shell string.
- Around line 116-126: After filtering out the current version from
releasesMetadata (using cliInfo), handle the case where releasesMetadata becomes
empty before calling windowAPI.showQuickPick: check if releasesMetadata.length
=== 0 and either throw a clear error (e.g. 'No other Kortex CLI versions
available') or call windowAPI.showInformationMessage to inform the user, then
return/exit; otherwise proceed to call windowAPI.showQuickPick and keep the
existing selectedRelease null check. Reference symbols: releasesMetadata,
cliInfo, windowAPI.showQuickPick, selectedRelease.
---
Nitpick comments:
In `@extensions/kortex-cli/src/kortex-cli.ts`:
- Line 31: Replace the non-sibling relative import of whereBinary in
kortex-cli.ts with the repository alias form; specifically change the import
that currently references './utils/system' to use the /@/ path alias pointing to
the utils/system module so the symbol whereBinary is imported via the alias
rather than a relative path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49d5b3d8-65fc-4baf-90db-5305ba219852
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
extensions/kortex-cli/package.jsonextensions/kortex-cli/scripts/build.jsextensions/kortex-cli/src/extension.tsextensions/kortex-cli/src/kortex-cli-downloader.tsextensions/kortex-cli/src/kortex-cli.tsextensions/kortex-cli/src/utils/system.tsextensions/kortex-cli/tsconfig.jsonextensions/kortex-cli/vite.config.jspackage.json
✅ Files skipped from review due to trivial changes (4)
- extensions/kortex-cli/tsconfig.json
- extensions/kortex-cli/src/utils/system.ts
- extensions/kortex-cli/scripts/build.js
- extensions/kortex-cli/vite.config.js
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- extensions/kortex-cli/src/extension.ts
- extensions/kortex-cli/package.json
Comment on lines
+67
to
+70
| const location = await whereBinary(this.envAPI, this.processAPI, executable); | ||
| return { | ||
| path: location, | ||
| version: stdout.trim(), |
There was a problem hiding this comment.
Trim whereBinary() output before persisting it as the CLI path.
On Linux/macOS, which output commonly includes a trailing newline; storing it untrimmed can produce an invalid/ugly path.
Proposed fix
- const location = await whereBinary(this.envAPI, this.processAPI, executable);
+ const location = (await whereBinary(this.envAPI, this.processAPI, executable)).trim();
return {
path: location,
version: stdout.trim(),
installationSource: 'external',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const location = await whereBinary(this.envAPI, this.processAPI, executable); | |
| return { | |
| path: location, | |
| version: stdout.trim(), | |
| const location = (await whereBinary(this.envAPI, this.processAPI, executable)).trim(); | |
| return { | |
| path: location, | |
| version: stdout.trim(), | |
| installationSource: 'external', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/kortex-cli/src/kortex-cli.ts` around lines 67 - 70, The value
returned from whereBinary (assigned to the local variable location) may contain
trailing whitespace/newline; trim that value before persisting it as the CLI
path. Update the return in the function that calls whereBinary (the block
assigning const location = await whereBinary(this.envAPI, this.processAPI,
executable)) to use location.trim() (or otherwise sanitize) so the returned
object’s path property is stored without trailing newlines or spaces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
kortex-clibuilt-in extension that downloads and registers the kortex CLI binary fromkortex-hub/kortex-cliGitHub releasesCloses #1127
Test plan
pnpm installresolves dependencies for the new extensionpnpm run build:extensions:kortex-clibuilds successfullypnpm run test:extensionspasses🤖 Generated with Claude Code