Isolate CLI user state for Kai Dev - #95
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks 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 |
![kaicontext[bot]](https://avatars.githubusercontent.com/in/4190771?s=60&v=4){kind=small}
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.
Kai reviewNote Reviewing this change… The verification result will appear here shortly. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
jschatz1 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
main pins engine v0.6.67, which predates the merge of kai-engine #81 and has no kaipath.UserPath; this branch needs it. The tagged v0.6.68 also predates that merge, so the pin moves to main (68f0588) until the next engine tag.
58a6320 to
d181186
Compare
There was a problem hiding this comment.
jschatz1 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Kai review
Kai Summary
Nothing new since my last review — the point already on this PR still stands. 👇
Where I'd land: 5/5 — ready to merge.
Scope: I read kaicontext/kai-cli at the merged revision (d925fb7, "Address review with isolated-state regression coverage"). The one thing I could not read is github.com/kaicontext/kai-engine/kaipath — the engine module this change pins to v0.6.69-0.20260910160213-68f0588df199. Three of the four call sites route path construction through kaipath.UserPath, and that function's behavior is the load-bearing contract for the whole change. The repo holds only a local api/kaipath/kaipath.go that re-exports engine.Resolve, not UserPath; the engine source is not vendored here. I treat UserPath's semantics as an unread contract — a limitation, not a defect — and I state it once here rather than opening a concern per call site.
What the change does: It replaces hardcoded ~/.kai path construction at four sites with kaipath.UserPath, so that a KAI_DATA_DIR override redirects the bin directory (kitlauncher's Default()), the agent-hook binary path (stableKaiPath), the update-check cache (updateCheckFile), and the TUI panic-log fallback (logTUIPanic). KAI_INSTALL_DIR still wins for the bin dir. The engine dependency moves from a released tag (v0.6.67) to a pseudo-version pinned at commit 68f0588df199 — which is what makes the new UserPath signature available. Import grouping is reshuffled in cmd/kai/main.go and internal/tui/app.go to accommodate the new import; the alignment whitespace and the restoreTerminalForSafety comment-block reformatting are cosmetic.
My take: The change is small, internally consistent, and the regression test is a genuine one — TestDefaultDataDirectory (internal/kitlauncher/kitlauncher_test.go:801) asserts the actual behavior (the expected path strings), not the mechanism. With KAI_DATA_DIR="" it expects filepath.Join(home, ".kai", "bin"); with KAI_DATA_DIR=dev it expects filepath.Join(dev, "bin"); with KAI_INSTALL_DIR=bundle the launcher's own override still wins. That test would fail if UserPath ignored KAI_DATA_DIR or if Default() dropped the KAI_INSTALL_DIR branch. I found no defect that would break anything that works today. One concern is worth stating; it does not block merge.
The one concern — os.Getenv("HOME") on the updateCheckFile line (cmd/kai/main.go:169): On Windows, HOME is typically unset, so os.Getenv("HOME") returns "" and kaipath.UserPath("", "update-check.json") receives an empty home. The fallback behavior then depends entirely on the unread engine contract — whether UserPath re-derives a home (e.g. via USERPROFILE) or produces a relative or junk path. This is not introduced by this change: the old code was filepath.Join(os.Getenv("HOME"), ".kai", "update-check.json"), which had the identical empty-string problem on Windows and would have written to a relative .kai/update-check.json from the cwd. So the behavior is preserved, not regressed. I raise it only because the two sibling call sites (stableKaiPath at log_ingest.go:659 and logTUIPanic's fallback at app.go:988) use os.UserHomeDir(), which is the cross-platform-correct API and falls back across USERPROFILE/HOMEDRIVE+HOMEPATH; main.go is the lone site that still uses the raw os.Getenv("HOME"). If UserPath does not internally re-derive a home from an empty input, the update-check cache silently lands in the cwd on Windows — the same place it landed before. Nothing the author needs to fix to honor the intent of this change; worth a follow-up if UserPath doesn't paper over the empty-home case itself. I could not confirm which way UserPath handles an empty first argument — that's the unread contract, and I won't dress a guess up as a finding.
On the test: TestDefaultDataDirectory is the right shape. It would fail on the old hardcoded code if the old code were somehow expected to honor KAI_DATA_DIR (it wouldn't, which is the bug being fixed), and it asserts the three branches (default, KAI_DATA_DIR, KAI_INSTALL_DIR) with real path comparisons. The one gap is that it does not exercise the stableKaiPath or updateCheckFile paths directly — those rely on the same UserPath contract by transitivity, but no test in the diff asserts their output. That's acceptable coverage for a merge; I name the gap so it isn't mistaken for coverage it doesn't have.
Decision worth a human's eyes (not a defect): Pinning kai-engine to a pseudo-version (v0.6.69-0.20260910160213-68f0588df199) rather than a released tag means every downstream build of this CLI now depends on an unreleased engine commit. That's the whole point of the change — UserPath lives in that commit — and the repo's own review_commit_deps_test.go documents that this is deliberate. But until the engine cuts a real release and this line moves back to a tag, any consumer building kai-cli is pulling an untagged engine snapshot. That's a release-coordination tradeoff the author already made consciously; flagging it so it isn't lost in the import shuffle.
This is ready to merge. The change does what it intends, the regression test is real, and the one concern I raised preserves pre-existing behavior rather than regressing it.
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 · +2 −2 |
internal/kitlauncher/kitlauncher.go |
modified · +3 −1 |
internal/kitlauncher/kitlauncher_test.go |
modified · +21 −0 |
internal/tui/app.go |
modified · +30 −31 |
What I opened — 10 files, 28 turns, 1m18s
2 of the 7 changed files don't appear below: go.mod, go.sum.
api/kaipath/kaipath.gocmd/kai/log_ingest.gocmd/kai/main.gocmd/kai/review_commit_deps_test.gocmd/kai/review_commit_depsrc.gofast.jsoninstall.shinternal/kitlauncher/kitlauncher.gointernal/kitlauncher/kitlauncher_test.gointernal/tui/app.go
+67 −46 · 7 files · reaches 32 · 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".
1 participant
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.