Skip to content
This repository was archived by the owner on May 15, 2026. It is now read-only.

Fix: harness and cli point to the same binary #55

Merged
igor-susic1 merged 1 commit into from
May 6, 2026
Merged

Fix: harness and cli point to the same binary #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions cmd/harness.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ func runHarness(cmd *cobra.Command, args []string) error {
return fmt.Errorf("coding harness is not installed and update checks are disabled — " +
"unset KIMCHI_NO_UPDATE_CHECK or install the harness manually")
}
if err := guardAgainstSelfExec(harnessPath); err != nil {
return err
}
return launchHarness(cmd, harnessPath, args)
}

Expand All @@ -46,6 +49,10 @@ func runHarness(cmd *cobra.Command, args []string) error {
return nil
}

if err := guardAgainstSelfExec(harnessPath); err != nil {
return err
}

var props map[string]any
if v, err := update.HarnessCurrentVersion(ctx); err == nil && v != nil {
props = map[string]any{"version": v.String()}
Expand All @@ -55,6 +62,33 @@ func runHarness(cmd *cobra.Command, args []string) error {
return launchHarness(cmd, harnessPath, args)
}

// guardAgainstSelfExec refuses to launch the harness if its resolved path
// points at the running CLI binary. Without this check a misconfigured path
Copy link
Copy Markdown

@kimchi-review kimchi-review Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️🧪 Testing

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.

// would cause syscall.Exec to replace the CLI with itself, producing an
// infinite launch loop instead of a clear failure.
func guardAgainstSelfExec(harnessPath string) error {
selfPath, err := update.ResolveExecutablePath()
if err != nil {
klog.V(1).ErrorS(err, "self-exec guard: could not resolve current executable; skipping check")
return nil
}
selfInfo, err := os.Stat(selfPath)
if err != nil {
klog.V(1).ErrorS(err, "self-exec guard: stat self failed; skipping check", "path", selfPath)
return nil
}
harnessInfo, err := os.Stat(harnessPath)
if err != nil {
klog.V(1).ErrorS(err, "self-exec guard: stat harness failed; skipping check", "path", harnessPath)
return nil
}
if os.SameFile(selfInfo, harnessInfo) {
return fmt.Errorf("refusing to launch harness: resolved harness path %q is the kimchi CLI itself "+
"(this would loop forever). Remove the stray file and reinstall: rm %q && kimchi update", harnessPath, harnessPath)
}
return nil
}

// launchHarness resolves the API key (prompting if missing), ensures it is
// saved in the config file, and execs into the harness binary.
func launchHarness(cmd *cobra.Command, harnessPath string, args []string) error {
Expand Down
12 changes: 4 additions & 8 deletions internal/update/harness.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,11 @@ func HarnessPathInDir(dir string) string {
return filepath.Join(dir, kimchiDevRepo.Binary)
}

// 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.
Copy link
Copy Markdown

@kimchi-review kimchi-review Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️🔧 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.

// It is placed inside the harness data directory so it does not collide
// with the Go CLI binary (both are now named "kimchi" after the merge).
func ResolveHarnessPath() (string, error) {
execPath, err := ResolveExecutablePath()
if err != nil {
return "", fmt.Errorf("resolve harness path: %w", err)
}
return HarnessPathInDir(filepath.Dir(execPath)), nil
return HarnessPathInDir(filepath.Join(harnessDataDir(), "bin")), nil
}

// HarnessInstalled reports whether the harness binary exists at the given path.
Expand Down
Loading