Merge Komodo v2 and align CLI terminal UX#5
Conversation
|
Important Review skippedToo many files! This PR contains 299 files, which is 149 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (299)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Merges upstream Komodo v2 into the fork and updates supporting tooling (automation scripts, devcontainer config, editor tasks, CI) to align with the new CLI/terminal UX and repo structure.
Changes:
- Added Deno-based action scripts for build/deploy workflows and TOML version bumping.
- Updated Rust workspace version + dependencies to Komodo v2-era set.
- Refreshed developer tooling: VS Code tasks, devcontainer configuration, and a new GitHub Actions CI workflow.
Reviewed changes
Copilot reviewed 12 out of 1087 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| action/run.ts | Adds a Deno automation script to bump Cargo workspace version, commit, push, and trigger km run actions. |
| action/deploy.ts | Adds a deploy entrypoint using the shared run() helper. |
| action/deploy-fe.ts | Adds a frontend deploy trigger script via km run. |
| action/deno.json | Adds import mapping for @std/toml. |
| action/build.ts | Adds a build entrypoint using the shared run() helper. |
| Cargo.toml | Updates workspace version to 2.0.0 and refreshes dependency versions/sets for v2 alignment. |
| .vscode/tasks.json | Renames “frontend” tasks/paths to “ui”. |
| .vscode/resolver.code-snippets | Updates Rust resolver-related snippets for the new resolver API shape. |
| .github/workflows/ci.yml | Introduces CI to build, test, and rustfmt-check on PRs/pushes. |
| .devcontainer/postCreate.sh | Sets up key directories/ownership for local devcontainer usage. |
| .devcontainer/devcontainer.json | Updates Node version, forwarded ports, and adds NODE_OPTIONS override. |
| .devcontainer/dev.compose.yaml | Switches base image/ports and adds environment for local UI + CORS settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [version, tag, count] = prev_version.split("-"); | ||
| const next_count = Number(count) + 1; | ||
|
|
||
| const next_version = `${version}-${tag}-${next_count}`; |
There was a problem hiding this comment.
prev_version is now "2.0.0" (per this PR), so split("-") will not produce tag/count. This results in count being undefined and next_count becoming NaN, producing a broken next_version (and a broken commit message). Please update the version-bump logic to handle plain semver (no -), and optionally only apply the -<tag>-<count> scheme when a prerelease/build suffix is present.
| const cmd = ` | ||
| cargo check | ||
| echo "" | ||
|
|
||
| git add --all | ||
| git commit --all --message "deploy ${version}-${tag}-${next_count}" | ||
|
|
||
| echo "" | ||
| git push | ||
| echo "" | ||
|
|
||
| km run -y action ${action} "KOMODO_BRANCH=${branch}&KOMODO_VERSION=${version}&KOMODO_TAG=${tag}-${next_count}" | ||
| ` |
There was a problem hiding this comment.
This builds a shell command string that interpolates action (function input) and branch (derived from git) directly into bash -c. If either contains shell metacharacters (e.g., quotes, $(), ;), this can break quoting or enable command injection. Prefer running commands without bash -c by passing Deno.Command an executable + argument array, or at minimum apply robust shell escaping to interpolated values before embedding them.
| new Deno.Command("bash", { | ||
| args: ["-c", cmd], | ||
| }).spawn(); |
There was a problem hiding this comment.
This builds a shell command string that interpolates action (function input) and branch (derived from git) directly into bash -c. If either contains shell metacharacters (e.g., quotes, $(), ;), this can break quoting or enable command injection. Prefer running commands without bash -c by passing Deno.Command an executable + argument array, or at minimum apply robust shell escaping to interpolated values before embedding them.
| new Deno.Command("bash", { | ||
| args: ["-c", cmd], | ||
| }).spawn(); |
There was a problem hiding this comment.
spawn() is fire-and-forget and the run() function returns immediately; when invoked via await run(...) (e.g., from action/build.ts / action/deploy.ts), the Deno process can exit before the child process finishes (and failures won’t be surfaced). Consider awaiting completion via .output()/.status() and propagating non-zero exit codes so CI/automation can reliably detect failures.
| new Deno.Command("bash", { | |
| args: ["-c", cmd], | |
| }).spawn(); | |
| const result = await new Deno.Command("bash", { | |
| args: ["-c", cmd], | |
| }).output(); | |
| if (!result.success) { | |
| const stderr = new TextDecoder("utf-8").decode(result.stderr); | |
| throw new Error( | |
| `Failed to run action "${action}". Exit code: ${result.code}. Stderr: ${stderr}`, | |
| ); | |
| } |
| new Deno.Command("bash", { | ||
| args: ["-c", cmd], | ||
| }).spawn(); No newline at end of file |
There was a problem hiding this comment.
Similar to action/run.ts, this uses spawn() and does not await completion or handle failures. If this script is used in automation, it can report success even when km run fails. Consider awaiting .status()/.output() and returning a failing exit code when the command fails.
| new Deno.Command("bash", { | |
| args: ["-c", cmd], | |
| }).spawn(); | |
| const child = new Deno.Command("bash", { | |
| args: ["-c", cmd], | |
| }).spawn(); | |
| const status = await child.status; | |
| if (!status.success) { | |
| console.error( | |
| `Command "${cmd}" failed with exit code ${status.code}`, | |
| ); | |
| Deno.exit(status.code ?? 1); | |
| } |
| const prev_version = ( | ||
| TOML.parse(cargo_toml_str) as { | ||
| workspace: { package: { version: string } }; | ||
| } | ||
| ).workspace.package.version; | ||
|
|
||
| const [version, tag, count] = prev_version.split("-"); | ||
| const next_count = Number(count) + 1; | ||
|
|
||
| const next_version = `${version}-${tag}-${next_count}`; | ||
|
|
||
| await Deno.writeTextFile( | ||
| "Cargo.toml", | ||
| cargo_toml_str.replace( | ||
| `version = "${prev_version}"`, | ||
| `version = "${next_version}"` | ||
| ) |
There was a problem hiding this comment.
Updating Cargo.toml via string replacement is brittle: it can silently fail if formatting changes (spaces, comments), and it can accidentally replace the wrong version = "..." occurrence if the same substring appears elsewhere. Since the script already parses TOML, consider updating the parsed TOML structure and re-serializing it (or otherwise targeting [workspace.package].version more precisely) to make the bump robust to formatting changes.
| const prev_version = ( | |
| TOML.parse(cargo_toml_str) as { | |
| workspace: { package: { version: string } }; | |
| } | |
| ).workspace.package.version; | |
| const [version, tag, count] = prev_version.split("-"); | |
| const next_count = Number(count) + 1; | |
| const next_version = `${version}-${tag}-${next_count}`; | |
| await Deno.writeTextFile( | |
| "Cargo.toml", | |
| cargo_toml_str.replace( | |
| `version = "${prev_version}"`, | |
| `version = "${next_version}"` | |
| ) | |
| const cargoConfig = TOML.parse(cargo_toml_str) as { | |
| workspace: { package: { version: string } }; | |
| }; | |
| const prev_version = cargoConfig.workspace.package.version; | |
| const [version, tag, count] = prev_version.split("-"); | |
| const next_count = Number(count) + 1; | |
| const next_version = `${version}-${tag}-${next_count}`; | |
| cargoConfig.workspace.package.version = next_version; | |
| await Deno.writeTextFile( | |
| "Cargo.toml", | |
| TOML.stringify(cargoConfig), |
| key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-cargo- |
There was a problem hiding this comment.
The cache key doesn’t include the Rust toolchain version, so caches can be reused across toolchain updates (which can cause subtle build issues, especially with target/ caching). Consider incorporating the toolchain version into the cache key (or using a Rust-specific caching action that handles this) to reduce flaky CI behavior.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@copilot apply changes based on the comments in this thread |
Implemented in de10960. I applied the review-thread fixes in |
Uh oh!
There was an error while loading. Please reload this page.