feat(config): honor CLAUDE_CONFIG_DIR for all .claude path lookups - #142
Conversation
snip hardcoded ~/.claude in discover, learn, and the Claude Code hook installer, so snip learn/discover/init silently found nothing for users running Claude Code with a non-default CLAUDE_CONFIG_DIR. Route every .claude path through a shared config.ClaudeBaseDir() helper that checks CLAUDE_CONFIG_DIR (matching Claude Code's own behavior) before falling back to ~/.claude. Leaves SNIP_CONFIG and snip's own config resolution untouched.
ClaudeBaseDir() previously swallowed the underlying os.UserHomeDir error and returned a bare empty string, forcing initcmd's two callers to hand-roll a generic error message with no information about why resolution failed (a regression versus the prior direct os.UserHomeDir() + %w wrapping). Change it to return (string, error) so callers can wrap the real cause; discover/learn keep their existing silent-on-failure behavior by discarding the error. Also consolidate the two byte-identical claudeProjectsDir() copies in discover.go and learn.go into config.ClaudeProjectsDir(), removing the duplicated implementation and duplicated test pairs. Found by pre-pr-review's silent-failure-hunter and code-simplifier passes.
edouard-claude
left a comment
edouard-claude
left a comment
There was a problem hiding this comment.
Thank you — genuinely. This is one of the best-prepared contributions this project has had.
You found a real bug that nobody reported: three separate code paths silently returning nothing for anyone running Claude Code with a relocated config directory, and you noticed it was the same hardcoded assumption in all three rather than fixing one and moving on. The pre-PR review pass is good practice on its own, but the "Reviewed and dismissed" section is the part I want to single out: you checked a flagged item against git show master:<file>, established it predated your branch, and said so instead of quietly widening the diff. That is the check most reviews skip, and it is exactly how I would want a finding handled. Consolidating the duplicated helper rather than editing it twice is the same instinct.
So none of what follows is a complaint about the work. Two fixes, one question, and some notes.
Verdict: good change, two things to fix before merge, one question I would like answered.
Verified
go build ./..., go vet ./..., go test -count=1 ./..., -race, -tags lite, golangci-lint run (0 issues), snip verify 45/45. Mutation check: replacing the os.Getenv("CLAUDE_CONFIG_DIR") read with "" fails TestClaudeBaseDirRespectsEnvVar, TestClaudeProjectsDirRespectsEnvVar and TestUninstallClaudeCodeRespectsClaudeConfigDir, so the change is genuinely pinned.
Consolidating the duplicated claudeProjectsDir() was the right call, and I want to flag something you got right that is easy to get wrong: learn.go:592 builds .claude/rules/ relative to the project, and you left it alone. That is correct, CLAUDE_CONFIG_DIR is about user config rather than project rules, and nothing in the repo says so. Worth a one-line comment there so the next person does not "fix" it.
Please fix: the tests sandbox themselves with the mechanism under test
TestInitClaudeCodeRespectsClaudeConfigDir, TestInitClaudeCodeMigratesLegacyHookUnderClaudeConfigDir and TestUninstallClaudeCodeRespectsClaudeConfigDir set only CLAUDE_CONFIG_DIR. If ClaudeBaseDir() ever regresses, the fallback is ~/.claude and the test suite starts operating on the developer's real Claude Code config.
Not theoretical. Running the mutation above:
snip uninstalled (claude-code)
--- FAIL: TestUninstallClaudeCodeRespectsClaudeConfigDir (0.00s)
init_test.go:679: expected snip hook removed from CLAUDE_CONFIG_DIR settings
The test correctly fails, but uninstallClaudeCode() ran against my real ~/.claude/settings.json first. It only came out unchanged because my snip hook does not happen to live in that file. On a machine where it does, go test ./... would silently uninstall it.
One line each:
t.Setenv("HOME", t.TempDir())so the fallback path is sandboxed too. Same for the two config tests that call os.UserHomeDir().
Please fix: the gofmt hunks are out of scope
The struct realignments in discover.go (sessionLine) and learn.go (commandEntry, ErrorPattern) are unrelated to CLAUDE_CONFIG_DIR. They are correct gofmt output on code that was already unformatted before your branch, so nothing is wrong with them, but house rule here is one PR per change.
They also surface something worth its own issue rather than a drive-by: master is not gofmt-clean (gofmt -l internal/ lists six files) and make lint does not check it, which is why it drifted. I will file that separately. Please drop the two hunks here.
Question: is the premise verified?
The description says CLAUDE_CONFIG_DIR is "the same env var Claude Code itself respects". I could not confirm that from a primary source:
- it is absent from the documented environment variables (https://code.claude.com/docs/en/env-vars) and from the settings page (https://code.claude.com/docs/en/settings);
- anthropics/claude-code#28808 is a feature request asking for it to control the project-level
.claude/directory, which is a different thing from the~/.claudethis PR relocates; - anthropics/claude-code#3833 reports its behaviour as unclear and undocumented.
Both issues are closed, neither with an authoritative answer.
This does not block the PR, because the change fails safe: with the variable unset, which is the common case, behaviour is byte-identical to master. But the failure mode if snip's reading diverges from Claude Code's is bad and quiet: snip init writes the hook into a directory Claude Code never reads, and the user gets a successful-looking install that does nothing.
Could you confirm empirically? Set CLAUDE_CONFIG_DIR to a fresh directory, run snip init, start Claude Code, and check that the hook actually fires. If it does, please put that in the commit message, since the docs will not back it up. If Claude Code turns out to read ~/.claude regardless, the honest shape is probably to check CLAUDE_CONFIG_DIR and fall back to ~/.claude when the former holds no settings.json.
Minor
TestClaudeBaseDirDoesNotAffectSnipConfigpasses with or without this PR:configPath()never readCLAUDE_CONFIG_DIR. It is a reasonable guard against a future mistake, but its name suggests it covers this change and it does not. Worth a comment saying which it is.ClaudeProjectsDir()swallowing the error and returning""matches the pre-existing silent behaviour ofdiscover/learn, and you called that out, which I appreciate. Now that it is a shared helper, a// Returns "" when the home directory cannot be resolved; callers treat that as "no sessions found".on the declaration would stop the next caller assuming it cannot fail.
Logistics
CI has not run on this PR at all: it comes from a fork and has zero check runs. I will approve the workflow so we get a real signal on ubuntu.
The branch is also behind master, which has moved a fair amount today (#132, #135, #137, #139, #140, #141). Rebasing will show snip verify at 55 tests rather than 45, and picks up a new CI step that actually runs those filter tests.
To be clear about the shape of this review: the two fixes are small and mechanical, and the question about CLAUDE_CONFIG_DIR is one I could not answer myself from the documentation, so I am asking rather than objecting. Happy to take this as soon as the HOME sandboxing is in and the gofmt hunks are out. Thanks again for the care you put into it.
Without HOME sandboxed, a regression in ClaudeBaseDir() falls back to the developer's real ~/.claude and go test operates on it directly (confirmed: mutating the CLAUDE_CONFIG_DIR read makes uninstallClaudeCode() touch the real settings.json before the assertion catches it).
sessionLine, commandEntry and ErrorPattern struct tags were reformatted incidentally while editing nearby code. Revert to master's current (unformatted) alignment -- master isn't gofmt-clean on these files and that's a separate issue, not part of the CLAUDE_CONFIG_DIR change.
Makes the "no sessions found" meaning of a "" return explicit on the declaration so the next caller doesn't assume the function can't fail.
|
Addressing this review body (id 4793247528). Fixed: tests sandboxed themselves with the mechanism under testAdded Re-ran your mutation ( Fixed: gofmt hunks droppedReverted the Answering: is the premise verified?Yes, on both counts:
Given both, no fallback-and-check-for- Minor notes
LogisticsLeft the rebase onto current |
Both packages routed through config.ClaudeProjectsDir(), but nothing covered it: reverting either findProjectDirs to a hardcoded ~/.claude/projects kept the suite green, so the two commands this branch set out to fix were unguarded. Each test sandboxes HOME and cwd, points CLAUDE_CONFIG_DIR at a temp dir, and asserts the project directory is resolved under it.
|
Pushed one commit to your branch: Everything else was already settled: the doc entry you quoted is in env-vars.md verbatim, and the HOME sandboxing and gofmt scoping are exactly what I asked for. Thanks for the care on this one. |
Six files were unformatted, which is how edouard-claude#142 picked up three struct realignments that had nothing to do with its change. Isolated here so the reformatting is reviewable on its own; the check that keeps it from drifting again is the next commit. Refs edouard-claude#144
2 participants
Summary
snip learn,snip discover, and the Claude Code hook installer (snip init/snip uninstall) hardcoded~/.claude, so they silently found nothing for users running Claude Code with a non-defaultCLAUDE_CONFIG_DIR.config.ClaudeBaseDir()as the single source of truth for.claudepath resolution: checksCLAUDE_CONFIG_DIR(the same env var Claude Code itself respects) before falling back to~/.claude.config.ClaudeProjectsDir()(base dir +projects) used by bothdiscoverandlearn— previously each package had its own byte-identical copy of this helper.discover.go,learn.go, andinitcmd/init.gonow route through these helpers instead of constructing.claudepaths directly.SNIP_CONFIGand snip's own config resolution (configPath()) are untouched —CLAUDE_CONFIG_DIRonly affects.claudelookups, not snip's own config directory.Fixed during review
A pre-PR review pass (code-reviewer, semgrep, silent-failure-hunter, variant-bug-hunter, code-simplifier) surfaced two issues, both fixed:
ClaudeBaseDir()originally swallowed the underlyingos.UserHomeDir()error and returned"", soinitClaudeCode/uninstallClaudeCodehad to hand-roll a generic error message instead of wrapping the real cause (a regression vs. the pre-PR code, which didfmt.Errorf("get home dir: %w", err)). ChangedClaudeBaseDir()to return(string, error)so callers can wrap the actual failure;discover/learnkeep their existing silent-on-failure behavior by discarding the error, matching their pre-existing UX.discover.goandlearn.goeach had a byte-identicalclaudeProjectsDir()wrapper. Consolidated intoconfig.ClaudeProjectsDir(), removing the duplicate implementation and duplicate test pairs.Reviewed and dismissed
discover/learnreturn "no session directories found" instead of surfacing a HOME-resolution error — confirmed viagit show master:<file>that this exact behavior predates this PR (not a regression), so left as-is.discover/learn(silent) andinitcmd(loud error) on a resolution failure also predates this branch.Test plan
go build ./...go vet ./...gofmt -lcleango test ./...(all 18 packages pass)ClaudeBaseDir/ClaudeProjectsDirenv-var/fallback/isolation-from-SNIP_CONFIG(config),initClaudeCode/uninstallClaudeCodeenv-var behavior (initcmd)