From b22c9dca0e7fd9f2ff9cce351c31083bbe22b242 Mon Sep 17 00:00:00 2001 From: chorus-codes <280607145+chorus-codes@users.noreply.github.com> Date: Sun, 31 May 2026 09:58:00 +1000 Subject: [PATCH] fix(kimi): key transport on the resolved binary, not ~/.kimi-code existence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to v0.8.61 (#99). chooseKimiTransport decided "drive kimi directly vs route to opencode-go" on `fs.existsSync(~/.kimi-code)`, but dispatch resolves the binary by name and detection resolves a concrete path — these disagree when BOTH kimi builds are installed. If a ~/.kimi-code dir existed but PATH resolved `kimi` to an unconfigured legacy Python build, v0.8.61 drove it directly → "LLM not set", instead of falling back to opencode-go. - New `isNativeKimiBinary(homeDir, binPath)` realpaths BOTH the binary and the ~/.kimi-code dir before a lexical containment check, so a symlinked binary (/usr/local/bin/kimi → ~/.kimi-code/bin/kimi) OR a symlinked ~/.kimi-code dir both compare in the same namespace. Returns undefined when the path is unknown so the caller falls back to dir existence. - chooseKimiTransport now takes a tri-state `isNativeBuild?: boolean`, staying fs-light/unit-testable. `false` (resolved, non-native) skips the dir shortcut and falls to the config probe — the dual-install fix. - isPathUnder: `..` guard now matches a real parent ref only, not a child segment that merely starts with dots (e.g. `..helpers/bin`). Tests: +15 (isPathUnder lexical, isNativeKimiBinary incl. symlinked-binary and symlinked-dir cases, tri-state precedence). Two chorus self-review rounds: round 1 request_changes (realpath asymmetry) → fixed → round 2 approved by all 7 reviewers. Note: a fallback-detected kimi whose dir isn't on the spawn PATH still ENOENTs (runHeadless spawns bare `kimi`) — pre-existing, tracked separately. --- src/daemon/agents/kimi.ts | 93 +++++++++++++++++++++++++++---- tests/kimi-transport.test.ts | 103 ++++++++++++++++++++++++++++++++++- 2 files changed, 183 insertions(+), 13 deletions(-) diff --git a/src/daemon/agents/kimi.ts b/src/daemon/agents/kimi.ts index 7be7f38..052b1cd 100644 --- a/src/daemon/agents/kimi.ts +++ b/src/daemon/agents/kimi.ts @@ -28,6 +28,7 @@ import { quoteValue, quotePath, validateValue } from './quote.js'; import { spawnHeadless } from '../headless.js'; import { parseOpencode, parseOpencodeExit, parseKimi } from './parsers/index.js'; import { atomicWriteJsonSync } from '../../lib/atomic-write.js'; +import { detectAllClis } from '../../lib/cli-detect.js'; import { wrapWithPty } from './opencode.js'; import { assertSandboxSupported, sandboxFailClosed } from './sandbox-guard.js'; @@ -55,18 +56,71 @@ type KimiTransport = 'kimi-cli' | 'opencode'; let cachedTransport: KimiTransport | null = null; +/** True when `child` resolves to a location strictly inside `parent`. + * Lexical only (no fs) and prefix-false-match safe — `~/.kimi-code-backup` + * is NOT under `~/.kimi-code`. Callers that care about symlinks resolve + * BOTH paths before calling (see `isNativeKimiBinary`). The `..` guard + * matches a real parent ref (`..` or `../…`) but NOT a child segment that + * merely starts with dots (e.g. `..helpers/bin`). */ +export function isPathUnder(child: string, parent: string): boolean { + const rel = path.relative(parent, child); + return ( + rel.length > 0 && + rel !== '..' && + !rel.startsWith(`..${path.sep}`) && + !path.isAbsolute(rel) + ); +} + +/** Best-effort realpath: resolves symlinks when the path exists, else + * degrades to lexical normalisation. Never throws. */ +function realpathOrLexical(p: string): string { + try { + return fs.realpathSync(p); + } catch { + return path.resolve(p); + } +} + /** - * Pure transport decision — no module-level cache, takes the home dir and - * the raw env override as arguments so it's testable without touching the - * real `~`. `detectKimiTransport()` wraps it with the cache + real homedir. + * Is the `kimi` binary at `binPath` the native Kimi Code build (installed + * under `/.kimi-code`)? Returns `undefined` when `binPath` is + * unknown so the caller can fall back to a directory-existence heuristic. + * + * Realpaths BOTH the binary and the `~/.kimi-code` dir before comparing, so + * the check is correct whether the symlink is on the binary (e.g. + * `/usr/local/bin/kimi` → `~/.kimi-code/bin/kimi`) OR on the directory + * itself (`~/.kimi-code` → `/opt/kimi-code`). Resolving only one side would + * put the two paths in different namespaces and false-negative. + */ +export function isNativeKimiBinary( + homeDir: string, + binPath?: string, +): boolean | undefined { + if (!binPath) return undefined; + return isPathUnder( + realpathOrLexical(binPath), + realpathOrLexical(path.join(homeDir, '.kimi-code')), + ); +} + +/** + * Pure transport decision — no module-level cache, no fs beyond the legacy + * config probe, so it's testable without touching the real `~`. + * `detectKimiTransport()` wraps it with the cache, real homedir, and the + * native-build verdict from `isNativeKimiBinary`. * * Precedence: * 1. `CHORUS_KIMI_TRANSPORT` env override always wins. - * 2. Native Kimi Code (code.kimi.com → `~/.kimi-code`) → drive `kimi` - * directly. It's account-authed via `kimi login` and has no - * `~/.kimi/config.toml`, so the legacy config probe below would have - * wrongly shunted it to opencode-go — ignoring the user's install or - * failing outright when they lack an OpenCode Go subscription (#98). + * 2. The resolved `kimi` binary is the native Kimi Code build + * (`isNativeBuild === true`) → drive it directly. It's account-authed + * via `kimi login` and has no `~/.kimi/config.toml`, so the legacy + * config probe below would otherwise shunt it to opencode-go — + * ignoring the install or failing without an OpenCode Go sub (#98). + * Keying on the RESOLVED binary (not just the dir's existence) stops a + * stale `~/.kimi-code` from forcing the direct path when PATH actually + * resolves `kimi` to an unconfigured Python build (→ "LLM not set"). + * When the build is unknown (`undefined`), fall back to dir existence. * 3. Python kimi-cli with a wired model in `~/.kimi/config.toml` (empty * config exits 1 "LLM not set", so a populated one is the gate). * 4. Otherwise fall back to opencode + opencode-go. @@ -74,12 +128,19 @@ let cachedTransport: KimiTransport | null = null; export function chooseKimiTransport( homeDir: string, override?: string, + isNativeBuild?: boolean, ): KimiTransport { if (override === 'kimi-cli' || override === 'opencode') return override; - // Native Kimi Code install — the active `kimi` on PATH (its installer - // renames any prior Python kimi-cli to `kimi-legacy`). Drive it directly. - if (fs.existsSync(path.join(homeDir, '.kimi-code'))) return 'kimi-cli'; + // Native Kimi Code build — drive `kimi` directly. When the build is + // unknown (no resolved binary), fall back to dir existence so behaviour + // is no worse than not knowing. An explicit `false` (resolved to a + // non-native binary) deliberately skips this and falls to the config + // probe — that's the dual-install bug fix. + if (isNativeBuild === true) return 'kimi-cli'; + if (isNativeBuild === undefined && fs.existsSync(path.join(homeDir, '.kimi-code'))) { + return 'kimi-cli'; + } // Standalone Python kimi-cli — usable only when a model is wired: // non-empty `default_model` OR any `[models.]` table. @@ -101,9 +162,17 @@ export function chooseKimiTransport( function detectKimiTransport(): KimiTransport { if (cachedTransport) return cachedTransport; + // chorus's canonical "where is kimi" answer (PATH lookup first, then the + // fallback-dir scan) — the same binary `runHeadless` resolves by bare name. + // Deciding on which build actually runs (rather than on a possibly-stale + // ~/.kimi-code directory) is what fixes the dual-install case. Cached + // (detectAllClis has a 30s TTL; cachedTransport caches the verdict here). + const home = os.homedir(); + const detected = detectAllClis().find((c) => c.id === 'kimi-cli')?.path; cachedTransport = chooseKimiTransport( - os.homedir(), + home, process.env.CHORUS_KIMI_TRANSPORT, + isNativeKimiBinary(home, detected), ); return cachedTransport; } diff --git a/tests/kimi-transport.test.ts b/tests/kimi-transport.test.ts index 1f37154..73bc1b9 100644 --- a/tests/kimi-transport.test.ts +++ b/tests/kimi-transport.test.ts @@ -18,7 +18,11 @@ import os from 'node:os'; import path from 'node:path'; import { randomUUID } from 'node:crypto'; -import { chooseKimiTransport } from '../src/daemon/agents/kimi.js'; +import { + chooseKimiTransport, + isNativeKimiBinary, + isPathUnder, +} from '../src/daemon/agents/kimi.js'; let tmpRoot: string; beforeAll(() => { @@ -84,3 +88,100 @@ describe('chooseKimiTransport', () => { expect(chooseKimiTransport(home, 'kimi-cli')).toBe('kimi-cli'); }); }); + +describe('chooseKimiTransport — native-build verdict (both builds installed)', () => { + // The decision keys on whether the `kimi` that will actually run is the + // native build (a tri-state boolean), not on the mere existence of a + // ~/.kimi-code dir. This prevents detection (which resolves a concrete + // path) and dispatch from disagreeing when both builds coexist on PATH. + + it('drives kimi directly when the resolved binary is native, even with an empty ~/.kimi/config.toml', () => { + writeKimiConfig('# no model set\n'); + expect(chooseKimiTransport(home, undefined, true)).toBe('kimi-cli'); + }); + + it('routes to opencode when a stale ~/.kimi-code dir exists but the resolved kimi is a NON-native unconfigured build', () => { + // The exact "both installed" edge: ~/.kimi-code exists, but PATH resolves + // `kimi` to the legacy Python build with no model wired. An explicit + // `false` must skip the dir-existence shortcut and fall through to the + // config probe → opencode-go (driving it directly would exit "LLM not set"). + fs.mkdirSync(path.join(home, '.kimi-code'), { recursive: true }); + writeKimiConfig('# no model set\n'); + expect(chooseKimiTransport(home, undefined, false)).toBe('opencode'); + }); + + it('drives a non-native build directly when its ~/.kimi/config.toml IS configured', () => { + fs.mkdirSync(path.join(home, '.kimi-code'), { recursive: true }); + writeKimiConfig('default_model = "kimi-k2.6"\n'); + expect(chooseKimiTransport(home, undefined, false)).toBe('kimi-cli'); + }); + + it('falls back to ~/.kimi-code dir existence when the build is unknown (undefined)', () => { + // Preserves v0.8.61 behaviour when detection couldn't resolve a path. + fs.mkdirSync(path.join(home, '.kimi-code'), { recursive: true }); + expect(chooseKimiTransport(home, undefined, undefined)).toBe('kimi-cli'); + }); + + it('override still wins over a native-build verdict', () => { + expect(chooseKimiTransport(home, 'opencode', true)).toBe('opencode'); + }); +}); + +describe('isPathUnder (lexical containment)', () => { + it('accepts a binary directly under the parent', () => { + expect(isPathUnder('/h/.kimi-code/bin/kimi', '/h/.kimi-code')).toBe(true); + }); + it('rejects an identical path (not strictly inside)', () => { + expect(isPathUnder('/h/.kimi-code', '/h/.kimi-code')).toBe(false); + }); + it('rejects a sibling with a shared prefix (~/.kimi-code-backup)', () => { + expect(isPathUnder('/h/.kimi-code-backup/bin/kimi', '/h/.kimi-code')).toBe(false); + }); + it('rejects a parent ref', () => { + expect(isPathUnder('/h/.kimi/bin/kimi', '/h/.kimi-code')).toBe(false); + }); + it('accepts a child segment that merely starts with dots (..helpers)', () => { + // gemini #46-review: `!rel.startsWith("..")` wrongly rejected this. + expect(isPathUnder('/h/.kimi-code/..helpers/bin/kimi', '/h/.kimi-code')).toBe(true); + }); +}); + +describe('isNativeKimiBinary (realpaths both sides)', () => { + it('returns undefined when no binary path is known', () => { + expect(isNativeKimiBinary(home, undefined)).toBeUndefined(); + }); + it('true for a real binary under ~/.kimi-code', () => { + const bin = path.join(home, '.kimi-code', 'bin', 'kimi'); + fs.mkdirSync(path.dirname(bin), { recursive: true }); + fs.writeFileSync(bin, '', { mode: 0o755 }); + expect(isNativeKimiBinary(home, bin)).toBe(true); + }); + it('false for a Python-style path outside ~/.kimi-code', () => { + const bin = path.join(home, '.local', 'bin', 'kimi'); + fs.mkdirSync(path.dirname(bin), { recursive: true }); + fs.writeFileSync(bin, '', { mode: 0o755 }); + expect(isNativeKimiBinary(home, bin)).toBe(false); + }); + it('true when the binary is a SYMLINK on PATH pointing into ~/.kimi-code', () => { + // e.g. /usr/local/bin/kimi → ~/.kimi-code/bin/kimi + const real = path.join(home, '.kimi-code', 'bin', 'kimi'); + fs.mkdirSync(path.dirname(real), { recursive: true }); + fs.writeFileSync(real, '', { mode: 0o755 }); + const linkDir = path.join(home, 'pathdir'); + fs.mkdirSync(linkDir, { recursive: true }); + const link = path.join(linkDir, 'kimi'); + fs.symlinkSync(real, link); + expect(isNativeKimiBinary(home, link)).toBe(true); + }); + it('true when ~/.kimi-code itself is a SYMLINK to another location (asymmetry fix)', () => { + // The exact gap reviewers flagged: realpathing only the binary while the + // dir stays lexical would false-negative. Both sides resolve here. + const target = path.join(home, 'opt-kimi-code'); + fs.mkdirSync(path.join(target, 'bin'), { recursive: true }); + const bin = path.join(target, 'bin', 'kimi'); + fs.writeFileSync(bin, '', { mode: 0o755 }); + fs.symlinkSync(target, path.join(home, '.kimi-code')); + // detection resolves the binary through the dir symlink: + expect(isNativeKimiBinary(home, path.join(home, '.kimi-code', 'bin', 'kimi'))).toBe(true); + }); +});