Conversation
This reverts commit 213a872.
Co-authored-by: Adam <2363879+adamdotdevin@users.noreply.github.com>
|
Found 1 test failure on Blacksmith runners: Failure
|
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| use tauri_plugin_store::StoreExt; | ||
| use tokio::sync::oneshot; | ||
|
|
||
| use crate::constants::{SETTINGS_STORE, WSL_ENABLED_KEY}; |
There was a problem hiding this comment.
CRITICAL: WSL_ENABLED_KEY is imported but not defined in constants.rs
create_command() now references WSL_ENABLED_KEY, but packages/desktop/src-tauri/src/constants.rs currently only defines SETTINGS_STORE, DEFAULT_SERVER_URL_KEY, and UPDATER_ENABLED. This will fail to compile unless the constant is added (or the import is corrected to the module that defines it).
| resolve_windows_app_path(app_name).is_some() | ||
| fn check_windows_app(_app_name: &str) -> bool { | ||
| // Check if command exists in PATH, including .exe | ||
| return true; |
There was a problem hiding this comment.
WARNING: Windows app existence check now always returns true
check_windows_app() returning true unconditionally means callers will think any app exists, even when it doesn’t. That can break UX flows that depend on accurate detection (and may mask configuration problems). Consider restoring a real check (e.g., resolve_windows_app_path(app_name).is_some()).
| } | ||
|
|
||
| let resolved = PathBuf::from(expand_env(&exe)); | ||
| let resolved = PathBuf::from(token); |
There was a problem hiding this comment.
WARNING: .cmd/.bat resolution no longer expands environment variables
token extracted from a script can commonly contain %ProgramFiles%, %LOCALAPPDATA%, etc. Converting directly via PathBuf::from(token) will treat those literally, so resolution will fail even when the target exists. Consider reintroducing environment-variable expansion for Windows paths (or using a more robust parser for command wrappers).
packages/opencode/script/publish.ts
Outdated
| kilo: `./bin/kilo`, | ||
| kilocode: `./bin/kilo`, | ||
| // kilocode_change end | ||
| [pkg.name]: `./bin/${pkg.name}`, |
There was a problem hiding this comment.
CRITICAL: bin entry uses a scoped package name as command + path
Here pkg.name is @kilocode/cli (contains /). Using it as the bin key creates an invalid command name, and ./bin/${pkg.name} becomes ./bin/@kilocode/cli (invalid path and doesn’t match the built binary name like opencode). This likely makes the published package unusable. Suggest deriving the binary name from package.json’s bin map (e.g. opencode) rather than pkg.name.
Code Review SummaryStatus: 10 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (32 files)
|
kevinvandijk
force-pushed
the
kevinvandijk/kilo-opencode-v1.1.59
branch
from
1538865 to
81c490f
Compare
kevinvandijk
force-pushed
the
kevinvandijk/kilo-opencode-v1.1.59
branch
from
81c490f to
7b5478a
Compare
Co-authored-by: Catriel Müller <catrielmuller@gmail.com>
| use crate::constants::{UPDATER_ENABLED, window_state_flags}; | ||
| use crate::{ | ||
| constants::{UPDATER_ENABLED, window_state_flags}, | ||
| server::get_wsl_config, |
There was a problem hiding this comment.
CRITICAL: get_wsl_config is imported from server but this function does not exist in server.rs. This will cause a compilation error.
A get_wsl_config function needs to be added to packages/desktop/src-tauri/src/server.rs that returns a struct with an enabled: bool field, or this import and usage (line 28) should be removed/replaced.
|
|
||
| const CLI_INSTALL_DIR: &str = ".kilo/bin"; | ||
| const CLI_BINARY_NAME: &str = "kilo"; // kilocode_change | ||
| const CLI_BINARY_NAME: &str = "opencode"; |
There was a problem hiding this comment.
WARNING: This reverts a kilocode_change — the binary name was previously changed from "opencode" to "kilo" for Kilo branding. This upstream merge has overwritten that change.
| const CLI_BINARY_NAME: &str = "opencode"; | |
| const CLI_BINARY_NAME: &str = "kilo"; // kilocode_change |
| } | ||
|
|
||
| let temp_script = std::env::temp_dir().join("kilo-install.sh"); // kilocode_change | ||
| let temp_script = std::env::temp_dir().join("opencode-install.sh"); |
There was a problem hiding this comment.
WARNING: This reverts a kilocode_change — the temp script name was previously "kilo-install.sh". This upstream merge has overwritten that change.
| let temp_script = std::env::temp_dir().join("opencode-install.sh"); | |
| let temp_script = std::env::temp_dir().join("kilo-install.sh"); // kilocode_change |
| tracing::info!(port, "Spawning sidecar"); | ||
|
|
||
| let envs = [ | ||
| ("KILO_SERVER_USERNAME", "opencode".to_string()), |
There was a problem hiding this comment.
WARNING: This reverts a kilocode_change — the server username was previously "kilo". This upstream merge has overwritten that change.
| ("KILO_SERVER_USERNAME", "opencode".to_string()), | |
| ("KILO_SERVER_USERNAME", "kilo".to_string()), |
| .env("KILO_SERVER_PASSWORD", password) // kilocode_change | ||
| .spawn() | ||
| .expect("Failed to spawn kilo"); // kilocode_change | ||
| .expect("Failed to spawn opencode"); |
There was a problem hiding this comment.
WARNING: This reverts a kilocode_change — the error message previously referenced "kilo" instead of "opencode".
| .expect("Failed to spawn opencode"); | |
| .expect("Failed to spawn kilo"); // kilocode_change |
kevinvandijk
force-pushed
the
kevinvandijk/kilo-opencode-v1.1.59
branch
from
b8fc355 to
c845e6a
Compare
| let version = app.package_info().version.to_string(); | ||
| let mut script = vec
Desktop