Skip to content

windows: '~' expansion uses os.Getenv("HOME") in 9 places, and CI never runs tests on Windows #139

Description

@JordanCoin

A code bug and the process gap that hides it. Filing together because neither is worth much alone.

The code

mcp/main.go expands ~ using os.Getenv("HOME") in 9 places (rg -c 'Getenv("HOME")' mcp/main.go → 9), e.g.:

path = filepath.Join(os.Getenv("HOME"), path[2:])

HOME is normally unset on Windows — Go's os.UserHomeDir() reads USERPROFILE there. So ~/proj becomes filepath.Join("", "proj") = "proj", which filepath.Abs then resolves against the server's current working directory.

Two outcomes, and the second is worse: either a spurious "not an accessible directory" error, or — if a directory of that name happens to sit next to the cwd — codemap silently analyses the wrong project and reports confidently on it.

The repo already has the correct idiom elsewhere: skills/loader.go:40, cmd/setup.go, and plugins/install.go all use os.UserHomeDir(). Only the MCP server does it by hand.

Also unhandled at those sites: bare ~ with no trailing separator, and the Windows-native ~\ form.

The process gap that keeps it invisible

.github/workflows/ci.yml:

os: [ubuntu-latest, macos-latest]      # line 16  — the Test job
os: [ubuntu-latest, macos-latest, windows-latest]   # line 93 — the Build job

Windows gets build + --help + smoke only. go test never runs there. So no test can catch this class, and any test that tries is written by someone who cannot observe the failure.

Compounding it: the tests that do cover ~ expansion call t.Setenv("HOME", ...), which makes them pass on every OS including Windows — they assert the buggy behaviour is fine by supplying the variable Windows would not have.

Suggested fix

  • Replace the 9 sites with os.UserHomeDir(); handle bare ~ and ~\. Mechanical.
  • Add windows-latest to the Test matrix. This is the load-bearing half — without it the next instance of this class lands the same way.

If a full Windows test run is too slow to add to every PR, even a nightly or merge-queue-only Windows test job would close the observability gap. The current state is that a whole platform's correctness is asserted by a build that never executes a test.

Provenance

Surfaced during review of #121, where the same pattern was being added to a 10th site. That one was propagation rather than regression, so it was not worth blocking the PR — but the class is worth fixing, and the CI gap is why it keeps propagating.

Related: #109 (also a doctor/config-resolution correctness issue), #131.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions