Isolate CLI user state for Kai Dev - #95
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Kai review
Kai Summary
Read through this one. 5 things worth your eyes before it merges. 👇
Where I'd land: 4/5 — your call, then merge.
I have enough to write the review. The kai-engine kaipath.UserPath source is external and not readable with my tools — that's the one thing I must flag as unverified rather than assert.
The rest of the read-through
Scope: I read kai-cli at commit 83e4581 ("Isolate CLI user state for Kai Dev"): cmd/kai/main.go, cmd/kai/log_ingest.go, internal/kitlauncher/kitlauncher.go, internal/tui/app.go, and the go.mod/go.sum bump. The one thing I could not read is the kaipath package itself — it lives in the external module github.com/kaicontext/kai-engine and isn't vendored into this workspace, so I could not confirm UserPath's signature or return value from source.
What it does: Four hardcoded filepath.Join(home, ".kai", …) constructions are replaced with kaipath.UserPath(home, …), and kai-engine is bumped from v0.6.56 to the pseudo-version v0.6.59-0.20260908191034-bdf8837ffaf2 to provide that helper. The intent is to funnel all user-state path resolution through one function so "Kai Dev" can point it at an isolated directory. As a mechanical refactor this is clean and the call sites are the right ones.
Concerns:
The behavior change is the whole point, and it's invisible from this repo. The refactor is only a no-op if UserPath(home, parts...) returns exactly filepath.Join(home, ".kai", parts...). But the commit's stated purpose is that it won't always — it can redirect to a "Kai Dev" directory. I cannot read the kai-engine source to see what triggers that redirect (an env var? a build tag? a marker file?). That matters because the four sites redirect together: stableKaiPath (the binary baked into agent hooks), kitlauncher.Default (where the kit binary is looked up / downloaded), updateCheckFile, and the TUI panic log. If the redirect fires unexpectedly, stableKaiPath and kitlauncher will look for binaries under a dev directory that has none, and the launcher will fall through to downloading into it. Please confirm what condition switches UserPath to the dev path, and that under normal production use it still resolves to ~/.kai — that's the load-bearing fact the whole change rests on and it can't be checked here.
kaipath.UserPath(home) with a single argument (internal/tui/app.go:990). Every other call passes trailing path components; this one passes none and assigns the result to dir, expecting the base ~/.kai directory (the code then does filepath.Join(dir, "tui-panic.log")). Whether a zero-variadic call returns the base directory cleanly depends entirely on UserPath's implementation — I couldn't verify it. If UserPath assumes at least one component, this panic-logging fallback silently breaks. It's best-effort code (a broken result just means the panic log isn't written), so it's low-blast-radius, but it's the one call shape that differs from all the others and deserves a direct check against the helper's contract.
os.Getenv("HOME") vs os.UserHomeDir() (cmd/kai/main.go:171). Pre-existing, not introduced here — the old line used the same os.Getenv("HOME"). Worth noting only because the other three sites use os.UserHomeDir(): on Windows HOME is usually unset, so updateCheckFile becomes a relative .kai/update-check.json. Not this PR's bug to fix, and CI does a Windows vet not a Windows run, so leave it — just don't let the refactor be mistaken for having normalized home resolution. It didn't.
Import grouping (all four files). The new "github.com/kaicontext/kai-engine/kaipath" was inserted at the top of the standard-library import group instead of the third-party group, e.g. above "context"/"bufio". gofmt sorts within a group, so these files are not gofmt-clean. I checked CI: it runs go vet, execlint, and a boundary test but does not gate on gofmt/goimports, so this won't fail the build — but it will churn on the next person's editor-on-save and reads as unreviewed. Move it into the third-party block with the other github.com/... imports.
Nothing here touches money, auth, secrets, concurrency, or resource lifecycles, and no signature changed, so there's no caller-update or race surface to worry about. The go.mod/go.sum bump is internally consistent (single require line, old checksum retained as expected).
Merge readiness: Mechanically sound and low-risk, but I'd want one human confirmation before merge — namely what makes UserPath choose the dev directory and that production still lands on ~/.kai — because that's a behavior change this repo can't verify on its own. The single-arg call and the import ordering are quick follow-ups, not blockers.
Important files changed
| File | Change |
|---|---|
cmd/kai/log_ingest.go |
modified · +2 −1 |
cmd/kai/main.go |
modified · +1 −1 |
go.mod |
modified · +1 −1 |
go.sum |
modified · +2 −0 |
internal/kitlauncher/kitlauncher.go |
modified · +2 −1 |
internal/tui/app.go |
modified · +2 −1 |
Additional findings
I couldn't pin these to a line in the diff:
- external:kaipath.UserPath — the refactor is only a no-op if UserPath(home, parts...) equals filepath.Join(home, ".kai", parts...); the redirect condition to a "Kai Dev" dir is in kai-engine and unverifiable from this repo, and it swings stableKaiPath/kitlauncher/update-check/panic-log together.
Decisions
Correct as written, but somebody should say yes to these:
- Routing all four user-state paths (agent-hook binary, kit launcher/download dir, update-check cache, panic log) through UserPath means a future "Kai Dev" redirect relocates all of them at once; confirm the trigger so production installs keep resolving to ~/.kai and the launcher doesn't download binaries into an empty dev dir.
+10 −5 · 6 files · reaches 30 · the full analysis
💬 Reply to any of my comments and I'll answer, or say @kaicontext anywhere on this PR — a question, or "take another look at the retry logic".
| if dir == "" { | ||
| if home, err := os.UserHomeDir(); err == nil { | ||
| dir = filepath.Join(home, ".kai") | ||
| dir = kaipath.UserPath(home) |
There was a problem hiding this comment.
kaipath.UserPath(home) is the only zero-variadic call; whether it returns the base ~/.kai depends on the helper's contract, unverified, and if it assumes ≥1 component the panic-log fallback silently breaks.
|
|
||
| // updateCheckFile is the path to the cached update check result. | ||
| var updateCheckFile = filepath.Join(os.Getenv("HOME"), ".kai", "update-check.json") | ||
| var updateCheckFile = kaipath.UserPath(os.Getenv("HOME"), "update-check.json") |
There was a problem hiding this comment.
pre-existing os.Getenv("HOME") (unchanged by this PR) yields a relative path on Windows where HOME is unset; not this change's bug, just not fixed by it.
| "bufio" | ||
| "encoding/json" | ||
| "fmt" | ||
| "github.com/kaicontext/kai-engine/kaipath" |
There was a problem hiding this comment.
cmd/kai/main.go, internal/kitlauncher/kitlauncher.go, internal/tui/app.go:12 — new github.com/... import placed in the stdlib group; not gofmt-clean (CI doesn't gate gofmt, so non-blocking).
There was a problem hiding this comment.
Kai review
Kai Summary
Read through this one. 4 things worth your eyes before it merges. 👇
Where I'd land: 3/5 — small fixes first.
Scope: kai-cli repo, the merged change at d925fb7. I read every file in the diff and every reachable caller of the changed paths. One thing I could not read: kaipath.UserPath itself. It lives in the external kai-engine module (pseudo-version v0.6.59-0.20260908192613-5ab1b102fc2f, which this diff bumps to), is neither vendored nor present as source in this repo, and I had no shell to walk the module cache. The four call sites in this diff are verifiable; the function body is not. I also did not run the build or the test — review mode gave me no execution tool — so the verdict below is static analysis only.
The rest of the read-through
What the change does: replaces three hardcoded ~/.kai joins with kaipath.UserPath(...) so KAI_DATA_DIR is honored consistently — the kit launcher's BinDir default (internal/kitlauncher/kitlauncher.go:156), the update-check cache path (cmd/kai/main.go:169), and the TUI panic-log directory fallback (internal/tui/app.go:988). It adds TestDefaultDataDirectory covering default / KAI_DATA_DIR / KAI_INSTALL_DIR scenarios, plus import reordering and a gofmt'd comment block. Overall: the intent is right and the mechanical migration of the three sites is clean, but there are two real defects in the test and the user-facing message, and the entire correctness story rests on an external contract I could not confirm.
1. TestDefaultDataDirectory does not provably fail on the unfixed code — the regression is unverified. internal/kitlauncher/kitlauncher_test.go:807
The test's first leg clears KAI_DATA_DIR and KAI_INSTALL_DIR and asserts Default().BinDir == filepath.Join(home, ".kai", "bin"). But Default() computes home from os.UserHomeDir() (which reads $HOME on Unix), and the old code was filepath.Join(home, ".kai", "bin"). With the env cleared, old and new code produce the identical string — so this leg passes either way and pins nothing. The only leg that could distinguish old from new is the KAI_DATA_DIR-override assertion at line 812, and that leg's power depends entirely on whether the external kaipath.UserPath(home, "bin") reads KAI_DATA_DIR and substitutes it for home/.kai — which is the function body I cannot read. If it does, the test fails on old code and is a real regression test; if it doesn't (joins only, never reads the env), the test passes on old code too. I could not resolve which from this repo. A test that calls the thing and checks the happy path is not a regression test unless it would visibly fail when the honoring is removed — and nothing here asserts the failure mode of the original bug.
2. The rewritten user-facing message drops the filename and mis-describes the panic-log location. internal/tui/app.go:695 (and the comment at :684)
The REPL's transient error line changed from "see ~/.kai/tui-panic.log" to "see the user Kai state directory (KAI_DATA_DIR or ~/.kai)" — losing the tui-panic.log filename entirely. More substantively, logTUIPanic (app.go:981) resolves its directory as: the primary project's KaiDir first (line 984), falling back to kaipath.UserPath(home) only when that's empty (line 988). The new message says "KAI_DATA_DIR or ~/.kai" and never mentions the project dir — so a user whose TUI panics inside a project reads the message, looks in $KAI_DATA_DIR (or ~/.kai), and finds nothing, because the trace actually landed in the project's .kai. The original message was at least concrete (~/.kai/tui-panic.log); the rewrite sends users to the wrong place and gives them no file to look for. Restore the filename and either cover the project-dir case or drop the path specifics.
3. Unverified external contract: kaipath.UserPath is load-bearing and I could not read it. kitlauncher.go:156, main.go:169, log_ingest.go:659, app.go:988
The whole change — "KAI_DATA_DIR is honored consistently" — rests on UserPath reading KAI_DATA_DIR and substituting it for home/.kai. I confirmed the four call sites; I could not confirm the function does this. Two specific risks I want named because I can't rule them out from this repo: (a) UserPath(home) with zero path elements (app.go:988) vs. UserPath(home, "bin") with one (kitlauncher.go:156). If the contract is "join the resolved data dir with the remaining elements," the zero-arg form should return the bare data dir (~/.kai), matching the old code. If it instead requires a non-empty tail or returns home unchanged when none are given, the panic log silently lands in $HOME directly — and the new comment claiming "KAI_DATA_DIR or ~/.kai" would be wrong when KAI_DATA_DIR is set. This call site has no test coverage. (b) Whether UserPath treats an empty KAI_DATA_DIR as unset (the test sets it to "" and expects fallback to ~/.kai — correct only if empty-string is ignored). Confirm both against the engine module's kaipath source.
4. updateCheckFile now captures KAI_DATA_DIR at package-init time. cmd/kai/main.go:169
This is a package-level var, evaluated once at init. os.Getenv("HOME") and the KAI_DATA_DIR read inside UserPath are both frozen for the process lifetime. The old code had the same freeze property for HOME, so it's not a regression — but the change extends the frozen capture to KAI_DATA_DIR, the one variable most likely to be set dynamically (test harnesses, t.Setenv in the same binary, a subcommand that re-homes the data dir). The blast radius is real: updateCheckFile is read at :186 and :220 and written at :252-253. A stale value silently reads/writes the wrong cache, and no test here asserts the path follows the env var. Not a blocker for merge, but worth knowing.
DECISIONS
-
Setting
KAI_DATA_DIRnow relocates the managed kit binary and the update-check cache, not just user state. Before this change,~/.kai/bin/(wherekai codedownloads and runskit, and wherekai updatereplaces it) and~/.kai/update-check.jsonwere fixed regardless of any data-dir env var. After, a user who setsKAI_DATA_DIRto relocate state onto another volume will have both migrate. The consequential one isBinDir: a user with an existing~/.kai/bin/kitwho setsKAI_DATA_DIRfor the first time will, on next run, not find their previously-downloaded kit at the new location and will trigger a fresh download. That's almost certainly the intended behavior, but it's a behavior change to a path that executes a binary — the author should confirm it's desired and that no existing install scripts or docs assume~/.kai/binis invariant. -
The TUI panic log now varies with
KAI_DATA_DIRtoo. Anyone with operational tooling keyed on~/.kai/tui-panic.log(log shippers, a support runbook) will stop finding new traces for users who've relocated their data dir. The in-app message already dropped the literal path; make sure external docs aren't still pointing only at~/.kai/tui-panic.log.
Merge readiness: not yet. The test doesn't demonstrably catch the bug it's named for, and the rewritten panic message drops the filename and can mislead users into looking in the wrong directory. Both are small, local fixes. Once the kaipath.UserPath contract is confirmed against the engine source and the message is corrected, this is a straightforward change.
Important files changed
| File | Change |
|---|---|
cmd/kai/log_ingest.go |
modified · +2 −1 |
cmd/kai/main.go |
modified · +8 −10 |
go.mod |
modified · +1 −1 |
go.sum |
modified · +4 −0 |
internal/kitlauncher/kitlauncher.go |
modified · +3 −1 |
internal/kitlauncher/kitlauncher_test.go |
modified · +21 −0 |
internal/tui/app.go |
modified · +30 −31 |
+69 −44 · 7 files · reaches 30 · the full analysis
💬 Reply to any of my comments and I'll answer, or say @kaicontext anywhere on this PR — a question, or "take another look at the retry logic".
| t.Setenv("USERPROFILE", home) | ||
| t.Setenv("KAI_DATA_DIR", "") | ||
| t.Setenv("KAI_INSTALL_DIR", "") | ||
| if got := Default().BinDir; got != filepath.Join(home, ".kai", "bin") { |
There was a problem hiding this comment.
the default-path leg passes identically on the old hardcoded code; only the KAI_DATA_DIR leg could distinguish old from new, and its power depends on the unread external kaipath.UserPath contract, so the regression is not verified.
| // added. | ||
| m.repl = m.repl.AppendSystemError(fmt.Sprintf( | ||
| "internal error suppressed (see ~/.kai/tui-panic.log) — continuing")) | ||
| "internal error suppressed (see the user Kai state directory (KAI_DATA_DIR or ~/.kai)) — continuing")) |
There was a problem hiding this comment.
the user-facing message dropped the tui-panic.log filename and claims "KAI_DATA_DIR or ~/.kai", but logTUIPanic writes to the project's KaiDir first and only falls back to UserPath(home); users are sent to the wrong place with no file to look for.
| if dir == "" { | ||
| if home, err := os.UserHomeDir(); err == nil { | ||
| dir = filepath.Join(home, ".kai") | ||
| dir = kaipath.UserPath(home) |
There was a problem hiding this comment.
kaipath.UserPath(home) is called with zero path elements; if the external contract requires a non-empty tail, the panic log silently lands in the wrong directory, and this call site has no test coverage.
|
|
||
| // updateCheckFile is the path to the cached update check result. | ||
| var updateCheckFile = filepath.Join(os.Getenv("HOME"), ".kai", "update-check.json") | ||
| var updateCheckFile = kaipath.UserPath(os.Getenv("HOME"), "update-check.json") |
There was a problem hiding this comment.
updateCheckFile now captures KAI_DATA_DIR at package-init time, freezing the cache path for the process lifetime; runtime re-homing won't move it, and no test asserts it follows the env var.
Honor
KAI_DATA_DIRfor the CLI's per-user paths so a CLI launched by Kai Dev uses its development state. Managed binary lookup, update-check state, ingestion fallback and TUI fallback use the shared path resolver; normal CLI installs still default to~/.kai.Depends on https://github.com/kaicontext/kai-engine/pull/81. Pin the engine commit so this branch can build without a local Go workspace. Merge the engine PR first.
Validation:
go test ./internal/kitlauncher ./cmd/kai -run 'Test.*(Update|Binary|Path|Config)'passed on the PR branch.The dependency contract is
func UserPath(home string, parts ...string) string: an unsetKAI_DATA_DIRkeeps~/.kai, a set value selects the explicit root, and zero trailing components returns that root. Source: https://github.com/kaicontext/kai-engine/blob/5ab1b102fc2f05938b0b0fdc00463a26511f0d8a/kaipath/user.go . Production users do not change directories merely by upgrading.Regression coverage verifies default production lookup, the data-directory override and precedence of the bundled
KAI_INSTALL_DIR.