LLM-1387: only update harness from now on#57
Conversation
Kimchi Code Review
Summary📊 Review Score: 82/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: no — No tests were added or modified alongside these changes. The PR modifies core update and launch logic significantly, increasing risk without test coverage to verify the new harness-only flow behaves correctly on network failures or edge cases. 📝 Found 3 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 82/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 2/5 (1 = trivial, 5 = very complex)
🧪 Tests: no — No tests were added or modified alongside these changes. The PR modifies core update and launch logic significantly, increasing risk without test coverage to verify the new harness-only flow behaves correctly on network failures or edge cases.
📝 Found 3 issue(s). See inline comments for details.
| // Reuse the update command logic for CLI + harness updates. | ||
| // Uses cache (skipCache=false) since harness launches happen frequently. | ||
| if _, err := runUpdate(cmd, false, false, false); err != nil { | ||
| result, err := runUpdate(cmd, false, false, false) |
There was a problem hiding this comment.
runUpdate now propagates errors from runHarnessUpdate as fatal, whereas previously runHarnessUpdate logged warnings and continued. If the update check fails (e.g., network error, GitHub API unavailable), the entire kimchi harness command fails and prevents launching the existing harness, degrading user experience.
💡 Suggestion: Change the error handling to log a warning and continue instead of returning: if err != nil { fmt.Fprintf(cmd.ErrOrStderr(), "Warning: harness update check failed: %v\n", err) } else if result != nil && result.Updated { ... }
| @@ -153,10 +107,11 @@ func runHarnessUpdate(cmd *cobra.Command, ctx context.Context, force, dryRun, sk | |||
| result, err := wf.Run(ctx) | |||
There was a problem hiding this comment.
The comment Failures are reported as warnings and never block the CLI update is now stale and misleading. The function currently returns errors to the caller, which blocks execution (e.g., in cmd/harness.go). This contradicts the documented behavior.
💡 Suggestion: Update the comment to reflect the new behavior: Failures are returned as errors to the caller, which should handle them appropriately
| @@ -78,58 +73,17 @@ func NewUpdateCommand() *cobra.Command { | |||
| return cmd | |||
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The printUpdateResult helper is called with result without checking for nil. While wf.Run() likely never returns nil for success, a defensive nil check would prevent potential panics if the contract changes.
💡 Suggestion: Add a nil check before calling printUpdateResult: if result != nil { printUpdateResult(...) }
Zilv1nas
force-pushed
the
update-only-harness
branch
from
0a7b398 to
3851799
Compare
Zilv1nas
force-pushed
the
update-only-harness
branch
from
3851799 to
edd7628
Compare
2 participants
Kimchi Summary
What changed
Removes CLI self-update functionality; the
updateandharnesscommands now only manage the coding harness (kimchi-dev). The install script now redirects users to install the harness directly from the kimchi-dev repository.Why
The Kimchi CLI is being deprecated in favor of the coding harness. This change simplifies the toolchain by unifying on a single binary (
kimchi) and directing users to install the harness directly.Key changes
cmd/update.go: RemovedrunCLIUpdateandupdateResult;runUpdatenow only orchestrates harness updates and returns*update.WorkflowResultcmd/harness.go: Added check forresult.Updatedto prompt users to re-run the command after a harness updateinternal/tui/steps/update.go: Removed CLI-related update states (updateStateChoice,updateStateApplying, etc.) and simplified to harness-only flow; removedcliandcliResultfields fromUpdateStepinternal/tui/steps/welcome.go: Removed concurrent CLI update check (reduced WaitGroup count)internal/update/github_client.go: Changed harness binary name from"kimchi-code"to"kimchi"scripts/install.sh: Replaced CLI installation logic with redirection tohttps://github.com/castai/kimchi-dev/releases/latest/download/install.shImpact
kimchi updateno longer updates the CLI binary; it only updates the harnesskimchiinstead ofkimchi-codekimchi-devrepository instead of the CLI from thekimchirepository