From e72e5395fd11f602e47282869556f170c5e99948 Mon Sep 17 00:00:00 2001 From: Manu Lorente Date: Wed, 24 Jun 2026 19:24:07 -0600 Subject: [PATCH] feat(doctor): repair auto-memory junction + OS-aware env-contract checks (HARNESS-040) Wire `dotf doctor` to detect and repair the per-machine setup drift the no-CI model relies on it to catch. - Add `checkAutoMemoryLink`: verify the Claude auto-memory dir is a vault link so /handoff lands in the single sink, FAIL/--fix-repair when missing, SKIP when no vault source, and WARN (never destroy) when it is a real diverged dir. - memlink: add read-only `Status`; share the cross-OS Claude project-key encoding (`ClaudeProjectKey`/`ClaudeMemoryTarget` map / \ : -> -, fixing the wrong target on Windows) and delete the local `encodeProjectPath`; widen `isLink` to detect junctions (`ModeIrregular`, not `ModeSymlink`, on Go 1.26). - Select the env-contract OS dialect from `GOOS` instead of a hardcoded "linux", ending the false-positive PATH drift the session-start banner showed on Windows; `expandHome` now resolves `$env:USERPROFILE` for the windows dialect. Defers the Hive venv repair (#574) and createLink cmd-quoting robustness (#575). Closes #551 --- cli/internal/doctor/checks_automemory.go | 57 ++++++++++ cli/internal/doctor/checks_automemory_test.go | 87 +++++++++++++++ cli/internal/doctor/checks_contract.go | 27 +++-- .../doctor/checks_contract_os_test.go | 101 ++++++++++++++++++ cli/internal/doctor/contract.go | 8 +- cli/internal/doctor/doctor.go | 1 + cli/internal/doctor/fs.go | 8 +- cli/internal/mem/session_start_adapter.go | 5 +- cli/internal/mem/session_start_detect.go | 4 +- cli/internal/mem/session_start_injectors.go | 6 -- .../mem/session_start_injectors_test.go | 6 -- cli/internal/memlink/memlink.go | 69 +++++++++++- cli/internal/memlink/memlink_test.go | 63 +++++++++++ docs/lessons.md | 12 +++ .../features.json | 37 +++++++ .../proposal.md | 48 +++++++++ .../tasks.md | 44 ++++++++ .../verification.md | 44 ++++++++ 18 files changed, 594 insertions(+), 33 deletions(-) create mode 100644 cli/internal/doctor/checks_automemory.go create mode 100644 cli/internal/doctor/checks_automemory_test.go create mode 100644 cli/internal/doctor/checks_contract_os_test.go create mode 100644 specs/HARNESS-040-doctor-fix-drift-repair/features.json create mode 100644 specs/HARNESS-040-doctor-fix-drift-repair/proposal.md create mode 100644 specs/HARNESS-040-doctor-fix-drift-repair/tasks.md create mode 100644 specs/HARNESS-040-doctor-fix-drift-repair/verification.md diff --git a/cli/internal/doctor/checks_automemory.go b/cli/internal/doctor/checks_automemory.go new file mode 100644 index 00000000..1436adfe --- /dev/null +++ b/cli/internal/doctor/checks_automemory.go @@ -0,0 +1,57 @@ +package doctor + +import ( + "path/filepath" + + "github.com/mlorentedev/dotfiles/cli/internal/memlink" +) + +// checkAutoMemoryLink verifies the Claude auto-memory directory is a link +// (junction on Windows, symlink on POSIX) into the knowledge vault, so /handoff +// lands the session continuity block in the single sink — the vault — and not in +// ~/.claude or the code repo. That link is otherwise created only by the silent +// session-start hook; on a machine where it is missing, /handoff orphans the +// block (knowledge#120, observed 2026-06-19). Surfacing + repairing it from the +// one command users actually run for setup drift is the #551 acceptance. +// +// Mirrors checkVaultHooks: verify always, repair only under --fix, idempotent. +// Crucially it shares memlink with the session-start adapter, so doctor and the +// hook compute an identical target on every OS — and, like memlink.Ensure, it +// never overwrites a real data directory (the divergence case is a WARN to +// reconcile by hand, not a destructive fix). +func checkAutoMemoryLink(sys *System, start string, rep *Report, fix bool) { + rep.Section("Auto-memory vault link") + + // Same ADR-025 seam + legacy default as checkVault / checkVaultHooks, so a + // path-default fix is inherited here too. + vault := sys.env("VAULT_PATH", filepath.Join(sys.home(), "Projects", "knowledge")) + target := memlink.ClaudeMemoryTarget(sys.home(), start) + + switch memlink.Status(start, target, "", vault) { + case memlink.StateLinked: + rep.Pass("auto-memory linked to the vault: " + target) + + case memlink.StateNoSource: + // No vault memory source for this project — a valid state (not every repo + // has a vault project), so SKIP, not FAIL. + rep.Skip("no vault memory source for this project — nothing to link") + + case memlink.StateRealDir: + // The agent's own data lives here, diverged from the vault. Repairing would + // mean destroying it, which memlink refuses by contract — so do NOT --fix; + // flag it for a manual reconcile (knowledge#120). + rep.Warn("auto-memory is a real directory, not a vault link: " + target + + " — /handoff writes here, not the vault. Reconcile by hand (merge into the vault source, then relink); --fix will not overwrite real data.") + + case memlink.StateRepairable: + if !fix { + rep.Fail("auto-memory not linked to the vault — run `dotf doctor --fix` so /handoff reaches the vault") + return + } + if msg, _ := memlink.Ensure(start, target, "", vault); msg != "" { + rep.Fix("linked auto-memory to the vault: " + target) + } else { + rep.Fail("could not create the auto-memory link at " + target + " (check permissions)") + } + } +} diff --git a/cli/internal/doctor/checks_automemory_test.go b/cli/internal/doctor/checks_automemory_test.go new file mode 100644 index 00000000..f4c45463 --- /dev/null +++ b/cli/internal/doctor/checks_automemory_test.go @@ -0,0 +1,87 @@ +package doctor + +import ( + "bytes" + "path/filepath" + "strings" + "testing" + + "github.com/mlorentedev/dotfiles/cli/internal/memlink" +) + +// autoMemEnv builds a HOME+VAULT_PATH env and a vault holding a memory source for +// project "myproj", returning the System and the start dir whose base is myproj. +func autoMemEnv(t *testing.T) (sys *System, start, vault string) { + t.Helper() + home := t.TempDir() + vault = t.TempDir() + mkdirAll(t, filepath.Join(vault, "10_projects", "myproj", "memory")) + start = "/work/myproj" // filepath.Base → "myproj"; resolves convention 1 + sys = newSys(map[string]string{"HOME": home, "VAULT_PATH": vault}, nil, nil) + return sys, start, vault +} + +func TestCheckAutoMemoryLink(t *testing.T) { + t.Run("no vault source → SKIP", func(t *testing.T) { + home := t.TempDir() + vault := t.TempDir() + sys := newSys(map[string]string{"HOME": home, "VAULT_PATH": vault}, nil, nil) + var buf bytes.Buffer + rep := capture(&buf) + checkAutoMemoryLink(sys, "/work/orphan", rep, false) + if rep.Failures() != 0 || !strings.Contains(buf.String(), "nothing to link") { + t.Errorf("expected SKIP, got\n%s", buf.String()) + } + }) + + t.Run("source exists, link missing, no --fix → FAIL", func(t *testing.T) { + sys, start, _ := autoMemEnv(t) + var buf bytes.Buffer + rep := capture(&buf) + checkAutoMemoryLink(sys, start, rep, false) + if rep.Failures() != 1 || !strings.Contains(buf.String(), "dotf doctor --fix") { + t.Errorf("expected FAIL with a --fix hint, got\n%s", buf.String()) + } + }) + + // NB: subtest names must avoid commas/parens — they leak into t.TempDir() paths, + // and a bare comma in a path breaks `cmd /c mklink` arg parsing on Windows. + t.Run("fix links it then a re-run is idempotent", func(t *testing.T) { + sys, start, _ := autoMemEnv(t) + + var fixBuf bytes.Buffer + repFix := capture(&fixBuf) + checkAutoMemoryLink(sys, start, repFix, true) + if !strings.Contains(fixBuf.String(), "linked auto-memory to the vault") { + t.Fatalf("--fix should create the link, got\n%s", fixBuf.String()) + } + + var reBuf bytes.Buffer + repRe := capture(&reBuf) + checkAutoMemoryLink(sys, start, repRe, true) + if repRe.Failures() != 0 || !strings.Contains(reBuf.String(), "auto-memory linked to the vault") { + t.Errorf("re-run should be an idempotent PASS, got\n%s", reBuf.String()) + } + }) + + t.Run("real non-empty dir → WARN, never destroyed by --fix", func(t *testing.T) { + sys, start, _ := autoMemEnv(t) + target := memlink.ClaudeMemoryTarget(sys.home(), start) + ownFile := filepath.Join(target, "MEMORY.md") + writeFile(t, ownFile, "local agent data") + + var buf bytes.Buffer + rep := capture(&buf) + checkAutoMemoryLink(sys, start, rep, true) // even with --fix + out := buf.String() + if rep.Failures() != 0 { + t.Errorf("a real data dir must WARN, not FAIL\n%s", out) + } + if !strings.Contains(out, "real directory") || !strings.Contains(out, "Reconcile") { + t.Errorf("expected the manual-reconcile WARN, got\n%s", out) + } + if !pathExists(ownFile) { + t.Error("--fix must NOT destroy the agent's own data dir") + } + }) +} diff --git a/cli/internal/doctor/checks_contract.go b/cli/internal/doctor/checks_contract.go index f404772f..832bb07f 100644 --- a/cli/internal/doctor/checks_contract.go +++ b/cli/internal/doctor/checks_contract.go @@ -13,17 +13,19 @@ import ( // persist it, not mutating an ephemeral child environment. func checkContractEnvVars(sys *System, c *Contract, rep *Report, fix bool) { rep.Section("Environment variables (contract)") + osName := contractOS(sys) for _, e := range c.EnvVars { - if e.RequiredOn == "windows" { - rep.Pass(e.Name + " (windows-scoped, skipped on Linux)") + // A var scoped to a different OS than this one does not apply here. + if e.RequiredOn != "" && e.RequiredOn != osName { + rep.Pass(fmt.Sprintf("%s (%s-scoped, skipped on %s)", e.Name, e.RequiredOn, osName)) continue } current := sys.Getenv(e.Name) if current == "" { - def := expandHome(sys, e.Default["linux"]) + def := expandHome(sys, e.Default[osName]) if def == "" { - if e.requiredOnLinux() { + if e.requiredOn(osName) { rep.Fail(e.Name + " unset and no default available (required)") } else { rep.Pass(e.Name + " unset (optional, no default)") @@ -32,7 +34,7 @@ func checkContractEnvVars(sys *System, c *Contract, rep *Report, fix bool) { } if fix { rep.Fix(fmt.Sprintf("%s unset — add to your shell profile: export %s=%q", e.Name, e.Name, def)) - } else if e.requiredOnLinux() { + } else if e.requiredOn(osName) { rep.Warn(fmt.Sprintf("%s unset (required); default %q — run --fix or set in profile", e.Name, def)) } else { rep.Warn(fmt.Sprintf("%s unset; default %q would be reported with --fix", e.Name, def)) @@ -59,7 +61,7 @@ func checkContractEnvVars(sys *System, c *Contract, rep *Report, fix bool) { // will set it on next login), never a hard FAIL. func checkContractPath(sys *System, c *Contract, rep *Report) { rep.Section("PATH entries (contract)") - for _, entry := range c.RequiredPathEntries["linux"] { + for _, entry := range c.RequiredPathEntries[contractOS(sys)] { expanded := expandHome(sys, entry) if pathContains(sys, expanded) { rep.Pass(expanded + " in PATH") @@ -103,6 +105,19 @@ func checkRequiredBinaries(sys *System, c *Contract, rep *Report) { } } +// contractOS maps the runtime GOOS to the env-contract's OS dialect key. The +// contract declares only two dialects: "linux" (POSIX — $HOME paths, the shell +// profiles) and "windows" ($env:USERPROFILE paths). macOS ("darwin") and the +// "" test default share the POSIX/linux dialect, so only Windows branches. This +// replaces the formerly hardcoded "linux" key, which made the env-contract sweep +// report Linux paths on Windows — a false-positive drift every session (#551). +func contractOS(sys *System) string { + if sys.GOOS == "windows" { + return "windows" + } + return "linux" +} + // contractBinaryNames returns the set of binary names already version-checked by // the contract, so the core-tools section can avoid double-reporting them. func contractBinaryNames(c *Contract) map[string]bool { diff --git a/cli/internal/doctor/checks_contract_os_test.go b/cli/internal/doctor/checks_contract_os_test.go new file mode 100644 index 00000000..a744c5fa --- /dev/null +++ b/cli/internal/doctor/checks_contract_os_test.go @@ -0,0 +1,101 @@ +package doctor + +import ( + "bytes" + "strings" + "testing" +) + +// TestContractOS maps the runtime GOOS to the env-contract's two dialect keys. +// Only Windows branches; macOS ("darwin") and the "" test default share the +// POSIX/linux dialect. +func TestContractOS(t *testing.T) { + for goos, want := range map[string]string{ + "windows": "windows", + "linux": "linux", + "darwin": "linux", + "": "linux", + } { + if got := contractOS(&System{GOOS: goos}); got != want { + t.Errorf("contractOS(GOOS=%q) = %q, want %q", goos, got, want) + } + } +} + +// TestCheckContractPath_Dialects proves the PATH-entries check reads the OS +// dialect for the running platform, not a hardcoded "linux" — the root cause of +// the false-positive drift banner on Windows (Linux entries expanded with a +// Windows home). +func TestCheckContractPath_Dialects(t *testing.T) { + contract := &Contract{RequiredPathEntries: map[string][]string{ + "linux": {"$HOME/.local/bin"}, + "windows": {`$env:USERPROFILE\scripts`}, + }} + + t.Run("linux dialect", func(t *testing.T) { + home := t.TempDir() + // expandHome does literal token substitution, so the expanded entry keeps + // the contract's forward slash ("$HOME/.local/bin") — build the PATH entry + // the same way rather than with filepath.Join, whose separator is host-OS. + linuxEntry := home + "/.local/bin" + env := map[string]string{"HOME": home, "PATH": linuxEntry} + var buf bytes.Buffer + checkContractPath(newSys(env, nil, nil), contract, capture(&buf)) // GOOS "" → linux + out := buf.String() + if !strings.Contains(out, linuxEntry+" in PATH") { + t.Errorf("linux entry should be checked + pass\n%s", out) + } + if strings.Contains(out, "scripts") { + t.Errorf("windows entry must NOT be checked on linux\n%s", out) + } + }) + + t.Run("windows dialect", func(t *testing.T) { + home := t.TempDir() // POSIX temp dir → no drive colon to collide with the ':' list separator on the test host + winEntry := home + `\scripts` + env := map[string]string{"USERPROFILE": home, "PATH": winEntry} + s := newSys(env, nil, nil) + s.GOOS = "windows" + var buf bytes.Buffer + checkContractPath(s, contract, capture(&buf)) + out := buf.String() + if !strings.Contains(out, winEntry+" in PATH") { + t.Errorf("windows entry should be checked + pass (and $env:USERPROFILE expanded)\n%s", out) + } + if strings.Contains(out, ".local/bin") { + t.Errorf("linux entry must NOT be checked on windows\n%s", out) + } + }) +} + +// TestCheckContractEnvVars_WindowsDialect proves env-var defaults and OS scoping +// follow the running platform: on Windows a linux-scoped var skips, the +// windows-scoped var validates, and a defaulted var resolves its WINDOWS default. +func TestCheckContractEnvVars_WindowsDialect(t *testing.T) { + home := t.TempDir() + contract := &Contract{EnvVars: []ContractEnvVar{ + {Name: "HOME", RequiredOn: "linux", Validation: "path_exists"}, + {Name: "USERPROFILE", RequiredOn: "windows", Validation: "path_exists"}, + {Name: "DOTFILES_DIR", Default: map[string]string{ + "linux": "$HOME/linuxonly", + "windows": `$env:USERPROFILE\winonly`, + }, Validation: "path_exists"}, + }} + env := map[string]string{"USERPROFILE": home} // DOTFILES_DIR unset → falls back to the dialect default + s := newSys(env, nil, nil) + s.GOOS = "windows" + + var buf bytes.Buffer + checkContractEnvVars(s, contract, capture(&buf), false) + out := buf.String() + + if !strings.Contains(out, "HOME (linux-scoped, skipped on windows)") { + t.Errorf("a linux-scoped var must SKIP on windows\n%s", out) + } + if !strings.Contains(out, "USERPROFILE="+home) { + t.Errorf("the windows-scoped USERPROFILE must validate on windows\n%s", out) + } + if !strings.Contains(out, "winonly") || strings.Contains(out, "linuxonly") { + t.Errorf("DOTFILES_DIR must resolve the WINDOWS default, not the linux one\n%s", out) + } +} diff --git a/cli/internal/doctor/contract.go b/cli/internal/doctor/contract.go index dea34441..28c3904a 100644 --- a/cli/internal/doctor/contract.go +++ b/cli/internal/doctor/contract.go @@ -27,10 +27,10 @@ type ContractEnvVar struct { Validation string `json:"validation"` } -// requiredOnLinux reports whether this var must be present on Linux: either -// globally required, or scoped required_on: linux. -func (e ContractEnvVar) requiredOnLinux() bool { - return e.Required || e.RequiredOn == "linux" +// requiredOn reports whether this var must be present on the given OS dialect: +// either globally required, or scoped required_on to that same OS. +func (e ContractEnvVar) requiredOn(os string) bool { + return e.Required || e.RequiredOn == os } // ContractBinary is a required binary with an optional pinned minimum version diff --git a/cli/internal/doctor/doctor.go b/cli/internal/doctor/doctor.go index f11e7657..db985c26 100644 --- a/cli/internal/doctor/doctor.go +++ b/cli/internal/doctor/doctor.go @@ -81,6 +81,7 @@ func Run(opts Options) (int, error) { checkOptionalTools(sys, cfg, contract, rep) checkVault(sys, rep) checkVaultHooks(sys, rep, opts.Fix) + checkAutoMemoryLink(sys, start, rep, opts.Fix) checkPathFiles(sys, cfg, rep) checkSecrets(sys, cfg, rep) checkPATExpiry(sys, cfg, rep) diff --git a/cli/internal/doctor/fs.go b/cli/internal/doctor/fs.go index 8bb8cb21..2b2d3ff3 100644 --- a/cli/internal/doctor/fs.go +++ b/cli/internal/doctor/fs.go @@ -75,12 +75,16 @@ func fileContains(p, substr string) bool { return strings.Contains(string(raw), substr) } -// expandHome substitutes $HOME and ${HOME} with the resolved home dir, leaving -// other variables untouched — the exact scope of doctor.sh's expand_path. +// expandHome substitutes the home-dir tokens of both contract dialects with the +// resolved home dir, leaving other variables untouched — the POSIX $HOME/${HOME} +// (doctor.sh's expand_path) plus PowerShell's $env:USERPROFILE, so the windows +// dialect resolves even when the GOOS seam selects it on a POSIX test host. func expandHome(sys *System, s string) string { home := sys.home() s = strings.ReplaceAll(s, "${HOME}", home) s = strings.ReplaceAll(s, "$HOME", home) + s = strings.ReplaceAll(s, "${env:USERPROFILE}", home) + s = strings.ReplaceAll(s, "$env:USERPROFILE", home) return s } diff --git a/cli/internal/mem/session_start_adapter.go b/cli/internal/mem/session_start_adapter.go index 09b630f3..1700bb6e 100644 --- a/cli/internal/mem/session_start_adapter.go +++ b/cli/internal/mem/session_start_adapter.go @@ -6,6 +6,8 @@ import ( "os" "path/filepath" "time" + + "github.com/mlorentedev/dotfiles/cli/internal/memlink" ) // This file is the Claude session-start adapter (CLI-025 PR2b-2b): it composes the @@ -65,8 +67,7 @@ func ClaudeContext(in ClaudeContextInput) string { ctx += vaultHealth(vaultRoot, vaultName, in.ScriptsDir) ctx += memorySymlink(in.Cwd, in.Vault, in.Home) - encoded := encodeProjectPath(in.Cwd) - memoryDir := filepath.Join(in.Home, ".claude", "projects", encoded, "memory") + memoryDir := memlink.ClaudeMemoryTarget(in.Home, in.Cwd) ctx += knowledgeHealth(filepath.Join(memoryDir, "MEMORY.md"), cfg.threshold("memory_md_max_lines", 150), cfg.threshold("crystallize_max_days", 14), in.Now) ctx += vaultBaseline(vaultRoot) diff --git a/cli/internal/mem/session_start_detect.go b/cli/internal/mem/session_start_detect.go index f53aff12..03805086 100644 --- a/cli/internal/mem/session_start_detect.go +++ b/cli/internal/mem/session_start_detect.go @@ -93,9 +93,7 @@ func workSDKFamilyBlock(family string) string { // and surfaces the "[auto-memory] Created …" line when it created one. Computes // Claude's encoded target path and delegates the OS-agnostic link to memlink. func memorySymlink(cwd, vault, home string) string { - encoded := encodeProjectPath(cwd) - target := filepath.Join(home, ".claude", "projects", encoded, "memory") - msg, _ := memlink.Ensure(cwd, target, "", vault) + msg, _ := memlink.Ensure(cwd, memlink.ClaudeMemoryTarget(home, cwd), "", vault) if msg == "" { return "" } diff --git a/cli/internal/mem/session_start_injectors.go b/cli/internal/mem/session_start_injectors.go index 0fed8c18..b4ecfb24 100644 --- a/cli/internal/mem/session_start_injectors.go +++ b/cli/internal/mem/session_start_injectors.go @@ -17,12 +17,6 @@ import ( // byte-equivalence is pinned by its own unit test. An injector with nothing to say // returns "". -// encodeProjectPath maps a CWD to Claude Code's per-project key by replacing '/' -// with '-' (the shell's `tr '/' '-'`). -func encodeProjectPath(cwd string) string { - return strings.ReplaceAll(cwd, "/", "-") -} - // claudeJSONSize warns when ~/.claude/.claude.json has been truncated below // threshold bytes (the upstream `claude plugin install` strip bug, SDD-021). Empty // when the file is absent or a healthy size. diff --git a/cli/internal/mem/session_start_injectors_test.go b/cli/internal/mem/session_start_injectors_test.go index cbfb509c..f097218b 100644 --- a/cli/internal/mem/session_start_injectors_test.go +++ b/cli/internal/mem/session_start_injectors_test.go @@ -8,12 +8,6 @@ import ( "time" ) -func TestEncodeProjectPath(t *testing.T) { - if got := encodeProjectPath("/home/me/Projects/dotfiles"); got != "-home-me-Projects-dotfiles" { - t.Errorf("got %q", got) - } -} - func TestClaudeJSONSize(t *testing.T) { t.Run("absent file is silent", func(t *testing.T) { if got := claudeJSONSize(filepath.Join(t.TempDir(), "nope.json"), 10240); got != "" { diff --git a/cli/internal/memlink/memlink.go b/cli/internal/memlink/memlink.go index 9fefd4b1..c7b654e8 100644 --- a/cli/internal/memlink/memlink.go +++ b/cli/internal/memlink/memlink.go @@ -59,6 +59,61 @@ func Ensure(cwd, target, project, vault string) (string, error) { return fmt.Sprintf("[auto-memory] Created %s for %s", linkNoun(), project), nil } +// LinkState classifies an auto-memory target for reporting — the read-only +// counterpart to Ensure. Ensure mutates; Status only observes, so `dotf doctor` +// can report PASS/WARN/FAIL/SKIP without the side effect of creating a link. +type LinkState int + +const ( + // StateLinked: target is already a symlink/junction. Healthy. + StateLinked LinkState = iota + // StateRealDir: target is a non-empty real directory — the agent's own data. + // Ensure deliberately leaves it untouched; doctor must NOT destroy it either. + // A manual reconcile into the vault is required (knowledge#120 divergence). + StateRealDir + // StateRepairable: a vault source exists and the target is missing or empty, + // so Ensure (or doctor --fix) would create the link. + StateRepairable + // StateNoSource: no vault source resolves for this project — nothing to link. + StateNoSource +) + +// Status classifies target without mutating it, mirroring Ensure's decision +// order exactly so the two never disagree about what Ensure would do. +func Status(cwd, target, project, vault string) LinkState { + if project == "" { + project = filepath.Base(cwd) + } + if isLink(target) { + return StateLinked + } + if isDir(target) && dirNotEmpty(target) { + return StateRealDir + } + if resolveVaultMemory(cwd, project, vault) == "" { + return StateNoSource + } + return StateRepairable +} + +// ClaudeProjectKey encodes a working directory into Claude Code's per-project key +// — the directory name under ~/.claude/projects. Claude maps every path separator +// AND the Windows drive colon to '-', so `/home/me/proj` and `C:\Users\me\proj` +// become `-home-me-proj` and `C--Users-me-proj`. The retired shell twin mapped +// only '/', silently producing the wrong key — and thus the wrong junction target +// — on Windows (the root cause of the unlinked auto-memory dir, #551). +func ClaudeProjectKey(cwd string) string { + return strings.NewReplacer("/", "-", `\`, "-", ":", "-").Replace(cwd) +} + +// ClaudeMemoryTarget is the per-project auto-memory directory Claude surfaces: +// `/.claude/projects//memory`. Shared by the +// session-start adapter (which creates the link) and `dotf doctor` (which +// verifies it), so the two compute an identical path on every OS. +func ClaudeMemoryTarget(home, cwd string) string { + return filepath.Join(home, ".claude", "projects", ClaudeProjectKey(cwd), "memory") +} + // resolveVaultMemory finds the vault memory source for a project via the three // conventions in precedence order, returning "" when none resolves to a real dir. // This is the agent- and OS-agnostic core shared by every caller. @@ -130,8 +185,10 @@ func createLink(src, target string) error { if runtime.GOOS == "windows" { // `mklink /J ` is a cmd builtin, so it runs via // `cmd /c`. Args are passed as argv elements (not concatenated into a shell - // string), and both paths are CLI-resolved vault/.claude locations, not user - // input — no shell-metachar surface. + // string); os/exec quotes any with spaces. Both paths are CLI-resolved + // vault/.claude locations, not user input — no shell-metachar surface. + // (A path component containing a bare cmd delimiter such as a comma is a + // known narrow gap — see HARNESS follow-up; spaces already round-trip.) return exec.Command("cmd", "/c", "mklink", "/J", target, src).Run() } return os.Symlink(src, target) @@ -152,10 +209,14 @@ func isDir(p string) bool { } // isLink reports whether p is a symlink (POSIX) or a junction/reparse point -// (Windows). Go surfaces both as ModeSymlink via Lstat on the supported toolchain. +// (Windows). POSIX symlinks carry ModeSymlink; Windows junctions — which `dotf` +// uses because they need no privilege — surface via Lstat as ModeIrregular (NOT +// ModeSymlink) on the Go 1.26 toolchain (verified empirically), so accept both. +// A plain directory is ModeDir and matches neither, so this never misfires on +// the agent's own real memory dir. func isLink(p string) bool { info, err := os.Lstat(p) - return err == nil && info.Mode()&os.ModeSymlink != 0 + return err == nil && info.Mode()&(os.ModeSymlink|os.ModeIrregular) != 0 } // dirNotEmpty reports whether p is a readable directory holding at least one entry diff --git a/cli/internal/memlink/memlink_test.go b/cli/internal/memlink/memlink_test.go index d63580a9..ae30b459 100644 --- a/cli/internal/memlink/memlink_test.go +++ b/cli/internal/memlink/memlink_test.go @@ -72,6 +72,69 @@ func TestResolveVaultMemory(t *testing.T) { }) } +// --- ClaudeProjectKey / ClaudeMemoryTarget: cross-OS encoding ---------------- + +func TestClaudeProjectKey(t *testing.T) { + for in, want := range map[string]string{ + "/home/me/Projects/dotfiles": "-home-me-Projects-dotfiles", + `C:\Users\mlorente\Projects\Workspace\dotfiles`: "C--Users-mlorente-Projects-Workspace-dotfiles", + } { + if got := ClaudeProjectKey(in); got != want { + t.Errorf("ClaudeProjectKey(%q) = %q, want %q", in, got, want) + } + } +} + +func TestClaudeMemoryTarget(t *testing.T) { + got := ClaudeMemoryTarget("/h", "/home/me/proj") + want := filepath.Join("/h", ".claude", "projects", "-home-me-proj", "memory") + if got != want { + t.Errorf("ClaudeMemoryTarget = %q, want %q", got, want) + } +} + +// --- Status: the read-only classifier doctor reports on ---------------------- + +func TestStatus(t *testing.T) { + t.Run("no source → StateNoSource", func(t *testing.T) { + target := filepath.Join(t.TempDir(), "memory") + if got := Status("/nowhere/x", target, "x", t.TempDir()); got != StateNoSource { + t.Errorf("got %v, want StateNoSource", got) + } + }) + + t.Run("source exists, target missing → StateRepairable", func(t *testing.T) { + vault := t.TempDir() + mkdirAll(t, filepath.Join(vault, "10_projects", "p", "memory")) + target := filepath.Join(t.TempDir(), "memory") + if got := Status("/x/p", target, "p", vault); got != StateRepairable { + t.Errorf("got %v, want StateRepairable", got) + } + }) + + t.Run("real non-empty dir → StateRealDir (Ensure would leave it; doctor must not destroy)", func(t *testing.T) { + vault := t.TempDir() + mkdirAll(t, filepath.Join(vault, "10_projects", "p", "memory")) + target := filepath.Join(t.TempDir(), "memory") + writeFile(t, filepath.Join(target, "own.md"), "agent data") + if got := Status("/x/p", target, "p", vault); got != StateRealDir { + t.Errorf("got %v, want StateRealDir", got) + } + }) + + t.Run("already linked → StateLinked", func(t *testing.T) { + vault := t.TempDir() + writeFile(t, filepath.Join(vault, "10_projects", "p", "memory", "MEMORY.md"), "x") + target := filepath.Join(t.TempDir(), "agent", "p", "memory") + if _, err := Ensure("/w/p", target, "p", vault); err != nil { + t.Fatalf("Ensure setup: %v", err) + } + if got := Status("/w/p", target, "p", vault); got != StateLinked { + t.Errorf("got %v, want StateLinked", got) + } + }) +} + // --- Ensure: idempotency + happy path ---------------------------------------- func TestEnsure(t *testing.T) { diff --git a/docs/lessons.md b/docs/lessons.md index 0a2be76b..301759cf 100644 --- a/docs/lessons.md +++ b/docs/lessons.md @@ -1294,3 +1294,15 @@ Note: the body's `---` is safe because it's inside the `|` scalar. **Problem**: The CI action pins golangci-lint **v2.12.2**, which runs the staticcheck `QF*` (quickfix) category; the older binary on the dev machine did not flag them. The editor's `gopls` analyzer DID surface `QF1002` as a hint — but a hint reads as style noise, so it shipped and only CI caught it. **Solution**: Treat gopls `QF*`/style hints as CI-enforced, not advisory — clear them before pushing. When practical, match CI's golangci-lint version locally; otherwise the cheap habit is: when the editor underlines a staticcheck quickfix, apply it rather than dismiss it. + +--- + +### [2026-06-25] Three Windows path gotchas behind a "broken" auto-memory junction (Go 1.26) + +**Context**: HARNESS-040 (#551) wired `dotf doctor --fix` to the merged `memlink` primitive to detect+repair the Claude auto-memory↔vault junction. Implementing it on Windows surfaced three non-obvious cross-OS facts the POSIX-first shell code had silently papered over. + +**Problem**: (1) **Encoding** — Claude's per-project key under `~/.claude/projects/` maps *every* path separator AND the drive colon to `-` (`C:\Users\me\proj` → `C--Users-me-proj`), but the ported `encodeProjectPath` only replaced `/`. On Windows it computed the wrong key, so the junction was created at (or looked for at) the wrong path — the latent root cause of the "junction never created here". (2) **Link detection** — a `mklink /J` junction surfaces via `os.Lstat` as `ModeIrregular`, **not** `ModeSymlink`, on Go 1.26 (verified empirically). The old `isLink` checked only `ModeSymlink`, so it never recognized a junction; `Ensure`'s "already linked" no-op only worked by accidentally falling through to its `dirNotEmpty` branch. (3) **cmd quoting** — `exec.Command("cmd","/c","mklink","/J",target,src)` relies on Go's `EscapeArg`, which quotes args with **spaces** (so `C:\Users\First Last\...` works) but not a bare **comma**; cmd then splits the path on the comma and mklink fails silently. A comma slipped in via a `t.TempDir()` path derived from a subtest name containing `(PASS, no dup)`. + +**Solution**: Put the encoding in the shared `memlink` primitive (`ClaudeProjectKey` maps `/ \ :` → `-`; `ClaudeMemoryTarget` joins the full path) so the session-start adapter and doctor compute an identical target on every OS, and deleted the local `mem.encodeProjectPath`. Widened `isLink` to `ModeSymlink|ModeIrregular`. Named test subtests without `,`/`()` so they don't poison `t.TempDir()` paths; ticketed the real cmd-quoting robustness fix (#575) rather than rabbit-holing on `cmd /s /c` quoting in this PR. + +**Rule**: When code that creates/inspects filesystem links is "ported from shell", re-derive the Windows facts from scratch — separators, the drive colon, junction-vs-symlink mode bits, and cmd argument quoting are all places POSIX intuition is wrong. Keep one OS-aware encoding as SSOT shared by every caller; never let two callers re-encode a path independently. And keep test names free of shell/cmd metacharacters — `t.TempDir()` embeds the test name, so a comma or paren in a subtest name becomes a real path component. diff --git a/specs/HARNESS-040-doctor-fix-drift-repair/features.json b/specs/HARNESS-040-doctor-fix-drift-repair/features.json new file mode 100644 index 00000000..df81de33 --- /dev/null +++ b/specs/HARNESS-040-doctor-fix-drift-repair/features.json @@ -0,0 +1,37 @@ +[ + { + "id": "HARNESS-040-doctor-fix-drift-repair-f1", + "behavior": "Missing auto-memory junction with a present vault source FAILs; --fix recreates it; re-run is an idempotent PASS", + "verification": "cd cli && go test ./internal/doctor/ -run 'TestCheckAutoMemoryLink/(source_exists|fix_links_it)'", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-040-doctor-fix-drift-repair-f2", + "behavior": "No vault source resolves -> SKIP, not FAIL", + "verification": "cd cli && go test ./internal/doctor/ -run 'TestCheckAutoMemoryLink/no_vault_source'", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-040-doctor-fix-drift-repair-f3", + "behavior": "A real non-empty auto-memory dir is a WARN and is never destroyed by --fix", + "verification": "cd cli && go test ./internal/doctor/ -run 'TestCheckAutoMemoryLink/real_non-empty_dir'", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-040-doctor-fix-drift-repair-f4", + "behavior": "env-contract checks select the OS dialect from GOOS (windows vs linux), ending false-positive drift on Windows while leaving linux output unchanged", + "verification": "cd cli && go test ./internal/doctor/ -run 'TestContractOS|TestCheckContractPath_Dialects|TestCheckContractEnvVars'", + "state": "pending", + "evidence": "" + }, + { + "id": "HARNESS-040-doctor-fix-drift-repair-f5", + "behavior": "Shared cross-OS Claude project-key encoding and read-only link Status in memlink", + "verification": "cd cli && go test ./internal/memlink/ -run 'TestClaudeProjectKey|TestClaudeMemoryTarget|TestStatus'", + "state": "pending", + "evidence": "" + } +] diff --git a/specs/HARNESS-040-doctor-fix-drift-repair/proposal.md b/specs/HARNESS-040-doctor-fix-drift-repair/proposal.md new file mode 100644 index 00000000..adf45b58 --- /dev/null +++ b/specs/HARNESS-040-doctor-fix-drift-repair/proposal.md @@ -0,0 +1,48 @@ +--- +id: "HARNESS-040-doctor-fix-drift-repair" +type: spec +status: implementing # draft | implementing | verifying | archived +created: "2026-06-25" +issue: "mlorentedev/dotfiles#551" # repo#NNN — GitHub issue / Project item that tracks this spec +tags: [spec, proposal] +template_version: "1.0" +--- + +# HARNESS-040-doctor-fix-drift-repair + +## Why + +`/handoff` writes the session continuity block to the knowledge vault, and on Claude that vault dir is surfaced into the agent's per-project memory through a junction at `~/.claude/projects//memory`. Today that junction is created **only** by a silent SessionStart hook (now `memlink` via the Claude adapter); on a machine where it is missing, `/handoff` orphans the continuity block in `.claude` instead of landing it in the vault (observed 2026-06-19, and still broken on this machine). `dotf doctor` is the one place a user runs to find and repair post-setup drift, but it cannot see or fix this junction. Separately, the doctor's env-contract sweep is hardcoded to the `linux` OS key, so on Windows it checks the wrong PATH entries and per-OS defaults and reports a **false-positive drift banner every session** (e.g. `C:\Users\mlorente/.dotfiles/scripts not in PATH`, a Linux entry expanded with a Windows home). Both are per-machine setup gaps that the no-CI model relies on `doctor` to catch. + +## What + +After this PR: + +1. `dotf doctor` reports the auto-memory↔vault junction as a first-class check: **PASS** when the link exists and resolves to the project's vault memory source, **FAIL** (with a `run --fix` hint) when it is missing while a vault source exists, **SKIP** when no vault source exists (a valid state). `dotf doctor --fix` recreates the junction idempotently via the shared `memlink` primitive — the same noun the SessionStart adapter uses — so `/handoff` lands the continuity block in the vault, not in `.claude`. +2. The env-contract sweep (`checkContractEnvVars`, `checkContractPath`) selects its OS key (`linux` / `windows` / `darwin`) from the injected `System.GOOS` instead of the hardcoded `"linux"`. On Linux behavior is unchanged; on Windows the doctor now checks the `windows` PATH entries and defaults, ending the false-positive drift that the `--quick` session-start banner surfaces. + +## Out of scope + +- **Hive MCP venv repair** (issue #551 part 3: `No module named 'rich.traceback'`). Repairing a Python venv from the Go doctor is a cross-language concern; tracked in its own issue (see References) per atomic-PR discipline. +- **`machine.json` scaffolding / reconciliation** for non-standard repo layouts (e.g. `~/Projects/Workspace/`). The OS-aware fix above removes the false drift; actively rewriting the per-machine override file is a separate config concern, deferred unless the OS-aware fix proves insufficient. +- Changing `memlink`'s link-creation semantics (junction vs symlink), the SessionStart hook, or the `--quick` mode's check *set* (only the OS key its existing checks read changes). + +## Risks / open questions + +- **Blast radius into session-start.** `checkContractEnvVars`/`checkContractPath` are also run by `dotf doctor --quick` (the SessionStart hook). Making them OS-aware changes the Windows banner output. This is the intended fix, but tests must cover both GOOS branches so the Linux/POSIX byte-equivalence expectations are preserved (Linux output is unchanged). +- **Junction target resolution must match the adapter exactly.** The check must compute the same `~/.claude/projects//memory` target and the same vault source precedence as `mem/session_start_detect.go`, or doctor and session-start would disagree. Mitigation: reuse `memlink` for both detection and repair; share the encode logic rather than re-deriving it. +- **memlink is best-effort silent by contract** (a failed link must never crash a session). Doctor needs a *non-mutating* status to report PASS/FAIL without side effects, and a loud repair under `--fix`. Resolution: add a thin `memlink.Status` (read-only) beside `Ensure`; do not weaken `Ensure`'s resilience contract. + +## Acceptance criteria + +- [ ] On a machine with a missing junction but a present vault memory source, `dotf doctor` reports the junction check as FAIL; `dotf doctor --fix` recreates it and a re-run reports PASS (idempotent: a second `--fix` neither errors nor duplicates state). +- [ ] When no vault memory source resolves for the project, the junction check is SKIP (not FAIL) under both `--fix` and plain modes. +- [ ] With `System.GOOS = "windows"`, `checkContractPath` checks the `windows` `required_path_entries` and `checkContractEnvVars` resolves `windows` defaults; with `GOOS = "linux"` the output is byte-for-byte unchanged from today. +- [ ] `cli` `go test ./...` passes; no unrelated changes in the diff. + +## References + +- Issue: `mlorentedev/dotfiles#551` (HARNESS-040) +- Deferred follow-ups: #574 (part 3 — Hive venv repair, cross-language), #575 (`memlink.createLink` robustness for path components with bare cmd delimiters) +- Code: `cli/internal/memlink/memlink.go` (the shared primitive), `cli/internal/mem/session_start_detect.go` (the adapter consumer), `cli/internal/doctor/checks_contract.go` (OS-key sites), `cli/internal/doctor/checks_vault_hooks.go` (the verify/repair check pattern this mirrors) +- ADR: `docs/adr/adr-025-cross-machine-paths.md` (the path seam), ADR-021 (the doctor consolidation) diff --git a/specs/HARNESS-040-doctor-fix-drift-repair/tasks.md b/specs/HARNESS-040-doctor-fix-drift-repair/tasks.md new file mode 100644 index 00000000..c9a0a73e --- /dev/null +++ b/specs/HARNESS-040-doctor-fix-drift-repair/tasks.md @@ -0,0 +1,44 @@ +--- +tags: [spec, tasks, templates] +created: "2026-06-25" +--- + +# Tasks - HARNESS-040-doctor-fix-drift-repair + +> TDD order. One task = one focused commit. Tick as you go. + +## Setup + +- [x] Branch created from main: `feat/doctor-fix-drift-repair` (descriptive, no ticket-ID per AGENTS.md branch rule) +- [x] `proposal.md` is complete and acceptance criteria are testable +- [x] No open questions left in `proposal.md` "Risks / open questions" + +## Implementation + +### Part 2 — OS-aware env-contract checks (smaller, isolates the contract churn first) + +- [ ] Add a `contractOSKey(sys)` helper + table test: `GOOS` "windows"→"windows", ""/"linux"→"linux", "darwin"→"darwin" +- [ ] Write failing test: `checkContractPath` with `GOOS="windows"` checks the `windows` `required_path_entries` +- [ ] Thread the OS key through `checkContractPath` +- [ ] Write failing test: `checkContractEnvVars` with `GOOS="windows"` resolves `windows` defaults + honors `required_on: windows` +- [ ] Thread the OS key through `checkContractEnvVars` (`Default[key]`, `requiredOn` logic) +- [ ] Regression test: `GOOS="linux"` output unchanged (guards the byte-equivalence expectation) + +### Part 1 — auto-memory junction check + +- [ ] Add non-mutating `memlink.Status(cwd, target, project, vault)` + unit test (linked / real-dir / missing-source / repairable) +- [ ] Write failing test: `checkAutoMemoryLink` → SKIP when no vault source resolves +- [ ] Write failing test: → FAIL when source exists but link missing (no `--fix`) +- [ ] Write failing test: → FIX recreates link under `--fix`; PASS on re-run (idempotent) +- [ ] Implement `checkAutoMemoryLink(sys, rep, fix)` in `package doctor`, share the encode logic with the adapter +- [ ] Wire the check into `doctor.Run` (non-`--quick` sweep, alongside `checkVaultHooks`) + +## Closing + +- [ ] Every acceptance criterion covered by ≥1 test +- [ ] `features.json` written with executable verification commands +- [ ] `go vet` / `go build` clean; lint passes (heed gopls QF/staticcheck hints — CI golangci-lint v2.x) +- [ ] No unrelated changes in the diff +- [ ] `verification.md` filled in +- [ ] Part 3 (Hive venv) ticketed as a new issue +- [ ] PR opened referencing this spec folder (no auto-merge) diff --git a/specs/HARNESS-040-doctor-fix-drift-repair/verification.md b/specs/HARNESS-040-doctor-fix-drift-repair/verification.md new file mode 100644 index 00000000..f26dad65 --- /dev/null +++ b/specs/HARNESS-040-doctor-fix-drift-repair/verification.md @@ -0,0 +1,44 @@ +--- +tags: [spec, verification, templates] +created: "2026-06-25" +--- + +# Verification - HARNESS-040-doctor-fix-drift-repair + +## Evidence + +Branch `feat/doctor-fix-drift-repair`. Tests run on a Windows host (the production target for the broken-junction case), so the Windows path branches are exercised natively, not just via the `GOOS` seam. + +- [x] **Junction missing + source present → FAIL; `--fix` recreates → PASS; idempotent** → `TestCheckAutoMemoryLink/source_exists,_link_missing...` (FAIL) + `.../fix_links_it_then_a_re-run_is_idempotent` (FIX then PASS on re-run). Live: `dotf doctor` from the repo root now reports the section. +- [x] **No vault source → SKIP (both modes)** → `TestCheckAutoMemoryLink/no_vault_source_→_SKIP`. Live: running from `cli/` (a dir with no vault project) prints `[SKIP] … nothing to link`. +- [x] **Real non-empty dir → WARN, never destroyed by `--fix`** → `TestCheckAutoMemoryLink/real_non-empty_dir…` asserts `pathExists(ownFile)` after `--fix`. Live: `dotf doctor` from the repo root WARNs on this machine's real (diverged) auto-memory dir without touching its 4 files — the knowledge#120 case. +- [x] **OS-aware contract checks: `GOOS=windows`→windows dialect, `GOOS=linux`→unchanged** → `TestContractOS`, `TestCheckContractPath_Dialects` (linux + windows subtests), `TestCheckContractEnvVars_WindowsDialect`; existing `TestCheckContractEnvVars` (GOOS="") guards the linux regression. Live: PATH-entries section on this Windows machine went from 2 false `[WARN] …/.dotfiles/scripts not in PATH` to `(2 checks, all ok)` — the false session-start drift banner is gone. +- [x] **`cli go test ./...` passes for the touched packages; no unrelated changes** → doctor/mem/memlink all green; the 3 `TestEmbeddedTemplatesMatchVault` failures (initrepo/spec/vault) are pre-existing (reproduced with this branch stashed) and tracked by #461. + +## Test status + +- `go test ./internal/doctor/ ./internal/mem/ ./internal/memlink/` → **ok** (all three packages). +- `go build ./... && go vet ./...` → clean. +- Manual smoke: built `./cmd/dotf` and ran `dotf doctor` from both the repo root (WARN on the real diverged dir) and `cli/` (SKIP), plus confirmed the PATH-entries section is now all-ok on Windows. +- No regressions: the only failing tests in the module are the pre-existing template-drift checks (#461), confirmed by re-running them with this branch stashed. + +## Decisions made during implementation + +- **`memlink.Status` (read-only) added beside `Ensure`** so doctor can classify without the side effect of creating a link, mirroring `Ensure`'s exact decision order so the two never disagree. +- **OS encoding fixed at the root, in `memlink`**: `ClaudeProjectKey` now maps `/`, `\` AND `:` to `-` (Claude Code's real scheme), not just `/`. The retired shell-only `/` mapping was the latent cause of the wrong junction target on Windows. Consolidated as the single encoding shared by the session-start adapter and doctor (deleted the local `mem.encodeProjectPath`). +- **`isLink` now accepts `ModeIrregular`**: empirically, a `mklink /J` junction surfaces via `os.Lstat` as `ModeIrregular` (not `ModeSymlink`) on Go 1.26 — the existing `Ensure` no-op only worked by falling through to the `dirNotEmpty` branch. +- **`StateRealDir` is a WARN, never a destructive `--fix`**: when the auto-memory dir holds real (diverged) data, repair would mean data loss, which `memlink` refuses by contract. Honors the explicit "do NOT force-overwrite" guidance; the reconcile is manual. +- **Deferred, ticketed (per the fix-or-ticket rule):** #574 (Hive venv repair — cross-language) and #575 (`createLink` robustness for path components with bare cmd delimiters — surfaced by a comma in a `t.TempDir()` path; needs build-tagged Windows cmd-quoting). + +## Promotion candidates + +- [x] Lesson for the repo's `docs/lessons.md`? **yes** — "Windows junctions are `ModeIrregular`, not `ModeSymlink`, under Go 1.26; and Claude's project-key encoding maps `:`/`\` too, not just `/`." Both are non-obvious cross-OS gotchas likely to recur. +- [ ] ADR-worthy decision? no — consistent with ADR-021/ADR-025, no new architectural choice. +- [ ] New pattern for `00_meta/patterns/`? no — repo-specific, not cross-project. + +## Archive checklist + +- [ ] `proposal.md` frontmatter set to `status: archived` +- [ ] Folder moved: `specs/HARNESS-040-doctor-fix-drift-repair/` -> `specs/archive/HARNESS-040-doctor-fix-drift-repair/` +- [ ] Backlog entry ticked with PR link +- [ ] Promotions above executed (the `docs/lessons.md` entry)