Fix: harness and cli point to the same binary#55
Conversation
Kimchi Code Review
Summary📊 Review Score: 88/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: no — No tests were added for the new 📝 Found 2 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: 88/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 for the new guardAgainstSelfExec function, which contains non-trivial logic for executable path resolution, file identity comparison via os.SameFile, and multiple error handling branches that would benefit from unit testing (especially for edge cases involving symlinks, hard links, and permission errors).
📝 Found 2 issue(s). See inline comments for details.
| } | ||
|
|
||
| // guardAgainstSelfExec refuses to launch the harness if its resolved path | ||
| // points at the running CLI binary. Without this check a misconfigured path |
There was a problem hiding this comment.
The guardAgainstSelfExec function implements file system logic (resolving executable paths, os.Stat, os.SameFile) that is critical for preventing infinite loops, but lacks unit test coverage. Without tests, regressions in path resolution or file comparison logic could allow self-exec scenarios to slip through or incorrectly block valid executions.
💡 Suggestion: Add unit tests for guardAgainstSelfExec covering: 1) identical paths, 2) different paths pointing to same inode (hard links), 3) symlinks resolving to self, 4) error cases where ResolveExecutablePath or os.Stat fail, verifying the function returns nil (soft failure) as expected.
| // ResolveHarnessPath derives the harness binary path from the kimchi executable's | ||
| // resolved directory. For example, if kimchi is at /usr/local/bin/kimchi, this | ||
| // returns /usr/local/bin/kimchi. | ||
| // ResolveHarnessPath returns the path where the harness binary lives. |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The change in ResolveHarnessPath from deriving the path from the executable's directory to using harnessDataDir() fundamentally alters where the harness binary is stored. This may leave orphaned binaries in old locations (/usr/local/bin/ or wherever the CLI resides) after upgrades, potentially confusing users or wasting disk space.
💡 Suggestion: Consider adding migration logic or documentation to clean up the harness binary from the old location (the CLI directory) on first run after upgrade, or log a warning if a harness is detected at the legacy path to guide users to remove it.
2 participants
Kimchi Summary
What changed
Adds protection against infinite launch loops when the resolved harness binary path points at the kimchi CLI itself. The harness is now installed to a dedicated data directory instead of alongside the CLI binary to avoid filename collisions.
Why
Since both the CLI and the harness are now named
kimchi, the previous resolution logic could place the harness binary in the same directory as the CLI, causingsyscall.Execto replace the CLI with itself. This produced an infinite restart loop rather than launching the actual coding harness.Key changes
cmd/harness.go: AddedguardAgainstSelfExechelper that compares the harness path with the current executable usingos.SameFile. If they match, it returns a descriptive error instead of looping.cmd/harness.go: Inserted calls toguardAgainstSelfExecbeforelaunchHarnessin both the "update checks disabled" and post-installation code paths.internal/update/harness.go: ChangedResolveHarnessPathto returnHarnessPathInDir(filepath.Join(harnessDataDir(), "bin"))instead of deriving the path from the CLI's installation directory.Impact
Users with existing installations that have the harness collocated with the CLI binary will need to remove the stale file and reinstall. The CLI now emits a clear error message with remediation steps:
rm <path> && kimchi update.