test: #104 set USERPROFILE alongside HOME in the doctor and agentic fixtures

[li-jin-quan]

Issue

Closes #104

What this changes

Both halves are test-only.

Home redirection. The doctor and agentic suites isolate themselves from a real wallet by pointing HOME at a temp dir. The code under test resolves that path with os.UserHomeDir, which reads $HOME on Unix but %USERPROFILE% on Windows - so on Windows the isolation never took effect. Both packages now carry a setHome helper that sets the two variables, used at the fixture sites. Same shape as your comment on the issue: the helper is duplicated per package rather than shared, runDoctor keeps the call inside the existing KH_TEST_KEEP_HOME conditional, and agenticWallet keeps setting that flag.

KH_HOST. ResolveHost reads KH_HOST ahead of the factory's DefaultHost, so an exported KH_HOST sends the checks to the live host instead of the httptest server and the assertions fail with nothing pointing at the cause. That clear does not belong in setHome: it has nothing to do with the temporary profile, and because runDoctor gates setHome on KH_TEST_KEEP_HOME, putting it there handed the host guard a conditional that only exists for the home directory. setHome now sets the two home variables and nothing else, and runDoctor clears KH_HOST unconditionally at the top.

Taking it out of setHome removed the clear that seven fixtures were getting for free - the seven in doctor_test.go that call tc.Execute themselves rather than runDoctor. Each of them clears KH_HOST on its own line now. The three probes in doctor_auth_test.go and the four TestDoctorCmd_CLIVersion* tests already carried their own clear from the two earlier commits; setHome is the half they were missing.

Scope

Both changes: the home-directory redirection and the KH_HOST clear, in cmd/doctor and internal/agentic. Test-only. No production code, no other packages, no CI.

internal/cache and internal/config also call os.UserHomeDir, but their tests set no HOME at all, so they are outside this and I left them alone.

How it was verified

On Windows, go 1.26.3, same machine, with a real wallet file in the profile the whole time:

	before	after
go test ./cmd/doctor/ ./internal/agentic/	13 failures	ok
KH_HOST=https://live.invalid, same two packages	ok on 365aa5c	ok
go test ./...	-	ok, 35 packages green, 30 of them with tests
go vet ./...	-	clean
go test -race ./cmd/doctor/ ./internal/agentic/	-	ok
go test -race ./... (what make test runs)	-	ok, 30 packages

Since this fixes the fixtures rather than the code under test, the failing run is the reproduction. Two details from it are worth naming:

• TestLoad_ReadsTheWallet asserted sub_abc123 and got the developer's own subOrgId - the real profile being read, not a missing file.
• TestLoad_MissingFileIsNotConfigured failed because that real wallet existed. It passes now, which is the case it was written for.

A third, from the KH_HOST half: with the clear taken back off the seven fixtures in doctor_test.go, KH_HOST=https://live.invalid go test ./cmd/doctor/ fails TestDoctorCmd_AllPass, TestDoctorCmd_WarnOnly and TestDoctorCmd_JSON. That is the run the last commit is built on.

Correcting a note from the first version of this body: -race did not run then because I read this machine as having no C compiler. It has one on PATH, so it ran - make test is go test -race ./... and that is green across the 30 packages. Same for make lint - it only needed golangci-lint, so I installed it, and it reports 0 issues.

---

• Targets main
• Title carries the issue number
• make lint and make test pass - all three CI jobs replicated locally: golangci-lint run ./... reports 0 issues (2.13.2, against this repo's v2 config), go test -race -coverprofile=coverage.txt ./... is green across the 30 packages, CGO_ENABLED=0 go build -o bin/kh ./cmd/kh succeeds, and go generate ./docs/ leaves no diff
• go generate ./docs/ not needed - no command or flag changed

[suisuss]

What this changes

cmd/doctor/doctor_test.go and internal/agentic/wallet_test.go each gain a setHome helper that sets HOME and USERPROFILE together, and all 13 fixture sites move onto it. I checked the base tree: 13 is the complete count of Setenv("HOME" in the repo, all 13 are converted, and the only remaining occurrence in each file is the one inside the helper.

The mechanism holds. Go's os.UserHomeDir reads HOME everywhere except Windows, where it reads USERPROFILE and has no HOMEDRIVE/HOMEPATH fallback, so setting the one variable left the Windows fixtures pointed at the developer's real profile. Of the three call sites that resolve through it, config.ConfigDir already honours XDG_CONFIG_HOME on every platform, so the genuinely home-derived read these suites exercise is the agentic wallet.

Does it match the description

Matches. The scope statement is accurate, including the part that scopes internal/cache and internal/config out because their tests set no HOME at all - I checked and that is right. Flagging -race and make lint as unrun rather than claiming them is the correct call, and the two failure modes quoted from the pre-fix run are the reproduction.

Blocking

Nothing.

Mechanical - actionable as-is

Nothing in the diff.

Two adjacent gaps, neither introduced here and neither required for this to land. Taking them or leaving them is your call.

• pkg/cmdutil/host.go:24 returns KH_HOST before falling back to cfg.DefaultHost, so setHome isolates the home directory while an ambient KH_HOST still redirects every doctor check away from the injected httptest server. A developer with KH_HOST exported hits TestDoctorCmd_AgenticWalletSignatureIsAccepted signing a request to the live host, which answers 401, and the assertion fails with nothing pointing at the cause. cmd/wallet/agentic_wrapper_test.go:78 already uses the t.Setenv("KH_SESSION_COOKIE", "") pattern for this. One line beside setHome closes it.
• All four jobs in .github/workflows/ci.yml are ubuntu-latest, so nothing in the pipeline can observe the class of bug this fixes, and the next test written the old way will merge green. That one is ours, not yours - a windows-latest entry on the test job is the fix and I will raise it separately.

Verdict

Approved - the conversion is complete, correct on every platform the CLI ships to, and the description matches the diff.

[li-jin-quan]

Did the first one.

setHome clears KH_HOST now. The three probes in doctor_auth_test.go got the same line too - they build their own server directly and never went through setHome, so they had the same hole.

Reproduced before touching anything. With KH_HOST=https://app.keeperhub.com exported:

--- FAIL: TestDoctorCmd_AgenticWalletSignatureIsAccepted (0.95s)
    [pass] API: reachable (897ms)
    [warn] Agentic Wallet: unknown to this host (check --host, or run kh wallet add)

897ms is the live host answering. cmd/doctor passes now with KH_HOST exported, and still passes without it.

Two side notes. Still no -race here: no cgo toolchain on this machine, and make test is go test -race ./.... go vet ./cmd/doctor ./internal/agentic is clean. golangci-lint isn't installed either. And running the whole suite with KH_HOST exported gives 18 packages passing and 12 failing, all under cmd/ - cmd/doctor is off that list now, so the other 12 look like the same gap sitting in their own fixtures. Happy to take those in a follow-up PR, or leave them for the CI change you're raising?

Left the windows-latest job alone.

[suisuss]

What this changes since the last review

One commit, +9. setHome now also clears KH_HOST (cmd/doctor/doctor_test.go:348), and the three tests in doctor_auth_test.go that isolate through XDG_CONFIG_HOME rather than setHome clear it individually at :36, :59 and :76.

Previously raised

• The KH_HOST gap, which I raised as optional and adjacent - taken, and handled better than I described it. I only mentioned the setHome call sites; you also covered the three auth tests that never go through setHome, which is the half I would have missed.

Blocking

Nothing.

Mechanical - actionable as-is

• cmd/doctor/doctor_version_test.go - the four TestDoctorCmd_CLIVersion* tests have the same gap and were not covered. They isolate with XDG_CONFIG_HOME only, and versionDoctorFactory:45-50 pins Config.DefaultHost to the test server, which ResolveHost still reads after KH_HOST. With KH_HOST exported, TestDoctorCmd_CLIVersionWarnsWhenBelowServerMinimum asks the live host for its KH-Minimum-CLI-Version header and fails on the warn assertion. Same one-line fix as the auth tests.
• Not taken, and fine either way: the three CF_* variables are still ambient, so ApplyHostHeaders will attach a developer's real Cloudflare service token to the outbound request. That only matters once KH_HOST points somewhere real, which these changes now prevent - so it is defence in depth rather than a live path.

Verdict

Approved - the conversion is complete and correct, and the isolation gap is closed everywhere except the version tests noted above.

[li-jin-quan]

Fixed — thanks for the pointer. cmd/doctor/doctor_version_test.go now clears KH_HOST in all four TestDoctorCmd_CLIVersion* tests, same one-line fix as the auth tests.

One correction on the repro, because it changes what you'd actually see:

Against app.keeperhub.com those four pass today. Its floor is 0.11.1, which is exactly the value the fixtures mock, so the live header and the fixture header agree and every assertion holds. I tried pointing KH_HOST at the live host and got four green.

They only break once the floor moves past a mocked version. I stood up a local server advertising 0.12.0:

KH_HOST=http://127.0.0.1:18099 go test ./cmd/doctor/ -run TestDoctorCmd_CLIVersion

--- FAIL: TestDoctorCmd_CLIVersionWarnsWhenBelowServerMinimum
    "[warn] CLI Version: v0.3.0 is outdated; minimum required is 0.12.0. Run: kh update" does not contain "0.11.1"
--- FAIL: TestDoctorCmd_CLIVersionPassesWhenAtServerMinimum
    "[warn] CLI Version: v0.11.1 is outdated; minimum required is 0.12.0. Run: kh update" does not contain "pass"

So it's the PassesWhenAtServerMinimum one that dies on the warn assertion; the other dies on the version string after the status assertion passes. Either way the verdict was being decided by a remote service, which is the part that matters.

After the fix, same command, four green. The per-test cost also drops from 0.28–0.93s to 0.02–0.03s — that's the outbound call disappearing.

Not taken: the three CF_* variables. Agreed it's defence in depth rather than a live path — once KH_HOST is cleared the request goes to the httptest server, and HeadersForHost only attaches a token for hosts that are actually behind Access. Happy to do a separate PR if you want the doctor fixtures fully hermetic, but it felt out of scope for the home-isolation issue.

[suisuss]

What this changes

cmd/doctor/doctor_test.go and internal/agentic/wallet_test.go each gain a setHome helper that sets HOME and USERPROFILE together, and all 13 fixture sites move onto it. The doctor copy also clears KH_HOST (:348), and the three probes in doctor_auth_test.go (:36, :59, :76) plus the four TestDoctorCmd_CLIVersion* tests in doctor_version_test.go (:62, :74, :88, :100) clear it inline.

Both halves land on variables the code under test actually reads. agentic.ConfigPath resolves through os.UserHomeDir (internal/agentic/wallet.go:38), which reads %USERPROFILE% on Windows with no HOMEDRIVE/HOMEPATH fallback. ResolveHost returns os.Getenv("KH_HOST") ahead of cfg.DefaultHost (pkg/cmdutil/host.go:24-30), and the non-empty guard there means t.Setenv("KH_HOST", "") does neutralise it rather than resolving to an empty host. Every site is t.Setenv, so nothing leaks past the test that set it; the repo has no bare os.Setenv and no t.Parallel, so the process-global writes cannot race.

Does it match the description

Undersells. The body describes only the home redirection - "One change", "two test files" - and the diff is four files and two changes: the KH_HOST clearing is not in it. It is fully described in the thread, at my request, so nothing here is hidden; the body just has not caught up with the last two commits.

Mechanical - actionable as-is

• cmd/doctor/doctor_auth_test.go:36, :59, :76 and cmd/doctor/doctor_version_test.go:62, :74, :88, :100 - these seven got the KH_HOST third of setHome but not the HOME/USERPROFILE half, which is the half #104 is about. They call tc.Execute directly rather than runDoctor, so all six checks run, and checkAgenticWallet (cmd/doctor/doctor.go:492-493) loads the developer's real ~/.keeperhub/wallet.json and signs it into the outbound request. No assertion breaks - checkAgenticWallet returns only pass or warn, never fail, so require.NoError(t, tc.Execute(...)) holds either way - but on Windows these seven are still pointed at the real profile. Replace the bare t.Setenv("KH_HOST", "") with setHome(t, t.TempDir()), which already clears KH_HOST.

• cmd/doctor/doctor_test.go:348 with :357 - the KH_HOST clear sits inside setHome, and runDoctor calls setHome only when KH_TEST_KEEP_HOME is empty, so the host guard inherits a conditional that exists for the home directory. I asked for that conditional shape on the issue when the helper carried HOME alone; carrying KH_HOST as well changes what it gates. Every in-repo path is still covered, because the only writer of KH_TEST_KEEP_HOME is agenticWallet (:408), which calls setHome itself - the gap is an ambient export only. Move t.Setenv("KH_HOST", "") out of setHome and call it unconditionally at the top of runDoctor.

• The body's Scope section: fold the KH_HOST change into it so a squash merge does not record this as a one-variable change.

Answers to the two open questions

The Windows CI job is mine, not yours - I said on the issue I would raise it separately and I still will. Nothing for you to file.

The three CF_* variables stay out, as agreed. Nothing to revisit.

With the team

• The ~15 other cmd/ packages with the same ambient-KH_HOST exposure - 33 test files pin Config.DefaultHost to an httptest server, and your run found 12 failing under an exported KH_HOST. I'm weighing a per-test t.Setenv("KH_HOST", "") line against a package-level TestMain (there is already one at internal/auth/keyring_test.go:85) - the tradeoff is a few hundred repeated lines and the next fixture silently missing one, against a hook that hides the isolation from the test that depends on it. I'm discussing it with the core team now and will come back with a verdict shortly. Nothing here is blocked on you, and I'd rather settle the shape before you spend a PR on it.

Not yours to clear

CI has not run on any of the three commits - lint, test and docs-check are all action_required at 365aa5c, so go test -race ./... still has not executed against this change on any machine. That is a fork-run approval on my side. For what it is worth, I read the concurrency shape rather than relying on the run: Execute waits on its WaitGroup before returning, no test calls t.Parallel, and Go's env accessors are mutex-guarded, so I do not expect -race to have anything to say here.

Verdict

Changes requested - the seven direct-Execute tests in doctor_auth_test.go and doctor_version_test.go still read the real home, which is the isolation #104 asks for, and it is one call each.

[li-jin-quan]

Both landed in 303e75c.

Your two asks interact, so flagging it rather than letting the diff decide quietly: taking KH_HOST out of setHome also takes it off the seven fixtures in doctor_test.go that call tc.Execute themselves, because setHome was the only thing clearing it for them. With KH_HOST=https://live.invalid, TestDoctorCmd_AllPass, TestDoctorCmd_WarnOnly and TestDoctorCmd_JSON go red - 365aa5c is green only because setHome supplied the clear as a side effect. Each of the seven carries its own clear now, the same two lines the other seven already read as.

runDoctor clears KH_HOST unconditionally at the top, ahead of the KH_TEST_KEEP_HOME gate, and setHome is the two home variables only.

The Scope section covers both changes now.

-race turned out to be runnable here after all - there is a C toolchain on PATH - so it and go vet ./... are in the verified table. make lint is still the gap.