diff --git a/cli/internal/doctor/checks_tools.go b/cli/internal/doctor/checks_tools.go index 9fdc743..f4e6e24 100644 --- a/cli/internal/doctor/checks_tools.go +++ b/cli/internal/doctor/checks_tools.go @@ -107,6 +107,11 @@ var versionMatches = []versionedDir{ // version to the pin: a drift is a WARN, not a FAIL. func checkVersionMatch(sys *System, cfg *Config, rep *Report) { rep.Section("Version match (versions.conf)") + if cfg.VersionsPath != "" { + // Provenance (#697): show which versions.conf the pins came from, so a stale + // deployed copy producing nonsensical drift directions is self-diagnosing. + rep.Info("versions.conf: " + cfg.VersionsPath) + } if sys.GOOS == "windows" { // Windows installs these via winget, not ~/Applications/-, // so the versioned-dir check does not apply (parity with healthcheck.ps1 diff --git a/cli/internal/doctor/config.go b/cli/internal/doctor/config.go index 4bf975b..b055ee7 100644 --- a/cli/internal/doctor/config.go +++ b/cli/internal/doctor/config.go @@ -6,42 +6,47 @@ import ( "os" "path/filepath" "strings" + + envpkg "github.com/mlorentedev/dotfiles/cli/internal/env" ) // Config holds the resolved locations and parsed manifests a doctor run needs: -// the deployed dotfiles dir, the version pins from versions.conf, and the path -// to env-contract.json. It mirrors how healthcheck.sh sourced versions.conf and -// doctor.sh resolved the contract. +// the deployed dotfiles dir, the version pins from versions.conf, and the paths +// to env-contract.json and versions.conf (kept for the provenance line that makes +// a stale-copy read self-diagnosing). It mirrors how healthcheck.sh sourced +// versions.conf and doctor.sh resolved the contract. type Config struct { DotfilesDir string Versions map[string]string ContractPath string + VersionsPath string } // loadConfig resolves DOTFILES_DIR (defaulting to $HOME/.dotfiles), then locates -// versions.conf and env-contract.json under it, falling back to the git repo -// root walked up from startDir (so `dotf doctor` works from inside a checkout, -// not just against a deployed ~/.dotfiles). A missing versions.conf is tolerated -// (version checks skip); a missing contract is reported by the caller. +// versions.conf and env-contract.json **repo-first** — the same precedence +// `dotf env generate` uses (env.ResolveRepoFirst) — so doctor and env never read +// different copies of the same file and hand out contradictory stale/fresh +// verdicts on a machine whose deploy dir lags the repo (#697). The checkout is +// resolved like env does (DOTFILES_REPO_DIR when it points at a real dir, else +// the .git walk-up from startDir). A missing versions.conf is tolerated (version +// checks skip); a missing contract is reported by the caller. func loadConfig(sys *System, startDir string) (*Config, error) { dotfilesDir := sys.env("DOTFILES_DIR", filepath.Join(sys.home(), ".dotfiles")) - repoRoot, _ := findRepoRoot(startDir) + repoDir := sys.env("DOTFILES_REPO_DIR", "") + if !isDir(repoDir) { + repoDir, _ = findRepoRoot(startDir) + } cfg := &Config{ - DotfilesDir: dotfilesDir, - Versions: map[string]string{}, - ContractPath: firstExisting( - filepath.Join(dotfilesDir, "env-contract.json"), - joinIf(repoRoot, "env-contract.json"), - ), + DotfilesDir: dotfilesDir, + Versions: map[string]string{}, + ContractPath: envpkg.ResolveRepoFirst("env-contract.json", repoDir, dotfilesDir, startDir), + VersionsPath: envpkg.ResolveRepoFirst("versions.conf", repoDir, dotfilesDir, startDir), } - if vp := firstExisting( - filepath.Join(dotfilesDir, "versions.conf"), - joinIf(repoRoot, "versions.conf"), - ); vp != "" { - v, err := parseVersionsConf(vp) + if cfg.VersionsPath != "" { + v, err := parseVersionsConf(cfg.VersionsPath) if err != nil { return nil, err } @@ -99,23 +104,3 @@ func findRepoRoot(start string) (string, error) { } } -// firstExisting returns the first path that exists, or "". -func firstExisting(paths ...string) string { - for _, p := range paths { - if p == "" { - continue - } - if _, err := os.Stat(p); err == nil { - return p - } - } - return "" -} - -// joinIf joins dir/name, or returns "" when dir is empty. -func joinIf(dir, name string) string { - if dir == "" { - return "" - } - return filepath.Join(dir, name) -} diff --git a/cli/internal/doctor/config_test.go b/cli/internal/doctor/config_test.go new file mode 100644 index 0000000..3b88c5b --- /dev/null +++ b/cli/internal/doctor/config_test.go @@ -0,0 +1,50 @@ +package doctor + +import ( + "path/filepath" + "testing" + + envpkg "github.com/mlorentedev/dotfiles/cli/internal/env" +) + +// TestLoadConfigResolvesRepoFirst is the #697 anti-drift guard: when both the +// checkout copy and the deployed copy of env-contract.json / versions.conf exist, +// doctor must resolve the checkout copy (the fresher SSOT) — the same one +// `dotf env generate` reads — so the two never hand out contradictory +// stale/fresh verdicts. It also confirms doctor reads the repo's pins, not a +// stale deployed pin that would produce nonsensical version-drift directions. +func TestLoadConfigResolvesRepoFirst(t *testing.T) { + repo := t.TempDir() + deployed := t.TempDir() + + repoContract := filepath.Join(repo, "env-contract.json") + writeFile(t, repoContract, `{"env_vars":[]}`) + writeFile(t, filepath.Join(deployed, "env-contract.json"), `{"env_vars":[]}`) + + repoVersions := filepath.Join(repo, "versions.conf") + writeFile(t, repoVersions, "DOTF_VERSION=9.9.9\n") + writeFile(t, filepath.Join(deployed, "versions.conf"), "DOTF_VERSION=0.0.1\n") + + sys := newSys(map[string]string{"DOTFILES_DIR": deployed, "DOTFILES_REPO_DIR": repo}, nil, nil) + cfg, err := loadConfig(sys, repo) + if err != nil { + t.Fatalf("loadConfig: %v", err) + } + if cfg.ContractPath != repoContract { + t.Errorf("ContractPath = %q, want repo copy %q", cfg.ContractPath, repoContract) + } + if cfg.VersionsPath != repoVersions { + t.Errorf("VersionsPath = %q, want repo copy %q", cfg.VersionsPath, repoVersions) + } + if cfg.Versions["DOTF_VERSION"] != "9.9.9" { + t.Errorf("read a stale deployed pin: DOTF_VERSION=%q, want 9.9.9 from the repo", cfg.Versions["DOTF_VERSION"]) + } + + // Cross-check: env's own resolver (globals) agrees with doctor's (seam), so the + // two resolution paths can never diverge again (#697). + t.Setenv("DOTFILES_REPO_DIR", repo) + t.Setenv("DOTFILES_DIR", deployed) + if got := envpkg.ResolveContractPath(); got != repoContract { + t.Errorf("env.ResolveContractPath() = %q, want repo copy %q (must agree with doctor)", got, repoContract) + } +} diff --git a/cli/internal/doctor/doctor.go b/cli/internal/doctor/doctor.go index d6b908a..dda21cd 100644 --- a/cli/internal/doctor/doctor.go +++ b/cli/internal/doctor/doctor.go @@ -114,6 +114,10 @@ func loadContractSection(sys *System, cfg *Config, rep *Report) *Contract { rep.Fail(fmt.Sprintf("env-contract.json unreadable (%v) — contract checks skipped", err)) return nil } - rep.Pass("env-contract.json loaded from " + cfg.ContractPath) + rep.Pass("env-contract.json loaded") + // Provenance (always shown, even in non-verbose where Pass is suppressed) so a + // stale-deployed-copy read is self-diagnosing rather than a silent contradiction + // with `dotf env generate` (#697). + rep.Info("contract: " + cfg.ContractPath) return contract } diff --git a/cli/internal/env/env.go b/cli/internal/env/env.go index 584906d..99d11b8 100644 --- a/cli/internal/env/env.go +++ b/cli/internal/env/env.go @@ -110,34 +110,47 @@ func MachinePath(home string) string { return filepath.Join(cfg, "dotfiles", "machine.json") } -// ResolveContractPath locates env-contract.json: first under DOTFILES_DIR (the -// deployed copy), then by walking up from the current directory (so the CLI -// works from inside a checkout). Returns "" when none is found. -func ResolveContractPath() string { - // Prefer the repo's contract when DOTFILES_REPO_DIR points at a real checkout. - // On a dev machine the checkout is fresher than the deployed copy under - // ~/.dotfiles, so this prevents the stale-deployed-copy drift where a relocated - // repo keeps generating from an out-of-date ~/.dotfiles/env-contract.json. Read - // with os.Getenv (not ResolvePath): ResolvePath calls back into here, so going - // through the cascade would recurse. A non-existent path (stale value) just - // falls through to the deployed copy below. - if repo := os.Getenv("DOTFILES_REPO_DIR"); repo != "" { - if p := filepath.Join(repo, "env-contract.json"); fileExists(p) { +// ResolveRepoFirst locates a repo-tracked config file (env-contract.json, +// versions.conf) using the repo-first precedence shared by `dotf env` and +// `dotf doctor` (#697): the checkout copy — the version-controlled SSOT, fresher +// on a dev machine whose deploy dir lags — wins over the deployed copy under +// DOTFILES_DIR, with a final walk-up from startDir so it resolves from inside a +// checkout even with no env var set. Returns "" when none exists. Centralizing +// this precedence is what stops doctor and env from drifting into contradictory +// stale/fresh verdicts about the same file. Callers pass their own repoDir / +// dotfilesDir / startDir (doctor through its System seam, env from globals), so a +// non-existent tier is simply skipped. +func ResolveRepoFirst(name, repoDir, dotfilesDir, startDir string) string { + if repoDir != "" { + if p := filepath.Join(repoDir, name); fileExists(p) { return p } } - home := Home() - if p := filepath.Join(DotfilesDir(home), "env-contract.json"); fileExists(p) { - return p + if dotfilesDir != "" { + if p := filepath.Join(dotfilesDir, name); fileExists(p) { + return p + } } - if cwd, err := os.Getwd(); err == nil { - if p := walkUpFor(cwd, "env-contract.json"); p != "" { + if startDir != "" { + if p := walkUpFor(startDir, name); p != "" { return p } } return "" } +// ResolveContractPath locates env-contract.json repo-first (see ResolveRepoFirst): +// the checkout copy under DOTFILES_REPO_DIR wins over the deployed copy under +// DOTFILES_DIR, then a walk-up from the working directory. This prevents the +// stale-deployed-copy drift where a machine whose ~/.dotfiles lags the repo keeps +// generating from an out-of-date contract. DOTFILES_REPO_DIR is read with +// os.Getenv (not ResolvePath): ResolvePath calls back into here, so going through +// the cascade would recurse. Returns "" when none is found. +func ResolveContractPath() string { + cwd, _ := os.Getwd() + return ResolveRepoFirst("env-contract.json", os.Getenv("DOTFILES_REPO_DIR"), DotfilesDir(Home()), cwd) +} + // RepoDir resolves the dotfiles checkout root: DOTFILES_REPO_DIR when it points at // a real directory, else walking up from the working directory for a .git entry (a // file in a worktree, a directory in a normal clone — os.Stat matches both). Returns diff --git a/cli/internal/env/env_test.go b/cli/internal/env/env_test.go index d670f3b..7a51f3b 100644 --- a/cli/internal/env/env_test.go +++ b/cli/internal/env/env_test.go @@ -191,6 +191,54 @@ func TestResolveContractPathRepoMissingFallsThrough(t *testing.T) { } } +func TestResolveRepoFirstPrefersRepoOverDeployed(t *testing.T) { + // Both the checkout copy and the deployed copy exist; the checkout (the fresher + // SSOT) must win, so doctor and env never read a stale deployed copy (#697). + repo := t.TempDir() + deployed := t.TempDir() + repoFile := filepath.Join(repo, "env-contract.json") + if err := os.WriteFile(repoFile, []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(deployed, "env-contract.json"), []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + if got := ResolveRepoFirst("env-contract.json", repo, deployed, ""); got != repoFile { + t.Errorf("ResolveRepoFirst() = %q, want repo copy %q", got, repoFile) + } +} + +func TestResolveRepoFirstFallsBackDeployedThenWalkUpThenEmpty(t *testing.T) { + // repoDir has no copy -> deployed wins. + deployed := t.TempDir() + deployedFile := filepath.Join(deployed, "versions.conf") + if err := os.WriteFile(deployedFile, []byte("A=1\n"), 0o644); err != nil { + t.Fatal(err) + } + if got := ResolveRepoFirst("versions.conf", filepath.Join(deployed, "no-repo"), deployed, ""); got != deployedFile { + t.Errorf("deployed fallback = %q, want %q", got, deployedFile) + } + + // Neither repoDir nor dotfilesDir has it -> walk up from startDir. + root := t.TempDir() + rootFile := filepath.Join(root, "versions.conf") + if err := os.WriteFile(rootFile, []byte("A=1\n"), 0o644); err != nil { + t.Fatal(err) + } + sub := filepath.Join(root, "a", "b") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatal(err) + } + if got := ResolveRepoFirst("versions.conf", "", filepath.Join(root, "nope"), sub); got != rootFile { + t.Errorf("walk-up = %q, want %q", got, rootFile) + } + + // None of the tiers has it -> "". + if got := ResolveRepoFirst("absent.conf", "", t.TempDir(), t.TempDir()); got != "" { + t.Errorf("no match = %q, want empty", got) + } +} + func TestRepoDirPrefersDotfilesRepoDir(t *testing.T) { repo := t.TempDir() t.Setenv("DOTFILES_REPO_DIR", repo) diff --git a/specs/BUG-030-doctor-env-shared-resolver/proposal.md b/specs/BUG-030-doctor-env-shared-resolver/proposal.md new file mode 100644 index 0000000..8e70e12 --- /dev/null +++ b/specs/BUG-030-doctor-env-shared-resolver/proposal.md @@ -0,0 +1,102 @@ +--- +id: "BUG-030-doctor-env-shared-resolver" +type: spec +status: implementing +created: "2026-07-10" +tags: [spec, proposal, doctor, env, path-resolution, adr-025, fresh-machine] +template_version: "1.0" +--- + +# BUG-030-doctor-env-shared-resolver + +Give `dotf doctor` and `dotf env` one shared repo-first resolver for +`env-contract.json` / `versions.conf`, and print a provenance line, so they can +never hand out contradictory stale/fresh verdicts. + +## Why + +`dotf doctor` located `env-contract.json` / `versions.conf` **deployed-copy-first** +(`config.go`: `firstExisting(DOTFILES_DIR/…, repo/…)`), while `dotf env generate` +resolved **repo-first** (`env.ResolveContractPath`). On a machine whose deploy dir +lags the repo, the same binary in the same shell said both: + +- `[FAIL] paths.ps1 is stale — run dotf env generate` (doctor, reading the stale + deployed contract), and +- `ok: … up to date` (env generate --check, reading the fresh repo contract). + +Doctor's fix advice could not converge, and version-drift directions came out +nonsensical (`installed=0.28.0 pinned=0.23.0` off a **stale** deployed pin — the +"fix" would act on the wrong number). It also silently re-introduced the #518 +class: a generate run that resolved the stale deployed contract renders paths +WITHOUT the age keys #663 added. + +Root cause: two independent resolvers with **opposite precedence**. The two +drifting apart is exactly the failure — so the fix is one shared resolver, not +two that happen to agree today. + +Source: issue #697 (audit process-audit-2026-07-07 §4 P4, CONFIRMED; §3 marks +#663 AT-RISK on this vector). Sibling of BUG-029/#696 (same fragmented-resolution +theme). + +## What + +- **Shared resolver.** New `env.ResolveRepoFirst(name, repoDir, dotfilesDir, + startDir)`: checkout copy (the version-controlled SSOT, fresher when the deploy + dir lags) wins over the deployed copy under `DOTFILES_DIR`, then a walk-up from + `startDir`. `env.ResolveContractPath` is refactored to call it (behavior + unchanged); `doctor.loadConfig` calls it for **both** the contract and + `versions.conf`. Precedence now lives in one function both commands share, so + they cannot drift again. +- **Consistent repo discovery in doctor.** `loadConfig` resolves the checkout the + way env does — `DOTFILES_REPO_DIR` when it points at a real dir (now reliably + seeded by BUG-029), else the `.git` walk-up from `startDir` — instead of only + the walk-up. +- **Provenance.** `dotf doctor` prints an always-visible `[INFO] contract: ` + and `[INFO] versions.conf: ` (via `rep.Info`, which is not suppressed in + non-verbose mode as `Pass` is), so a stale-copy read is self-diagnosing rather + than a silent contradiction. +- **Anti-drift guard.** A doctor test asserts that with **both** the checkout and + deployed copies present, `loadConfig` resolves the checkout copy and reads the + repo pin (not the stale deployed one), and cross-checks that + `env.ResolveContractPath` agrees. Plus `env` unit tests for `ResolveRepoFirst` + precedence (repo → deployed → walk-up → ""). + +## Out of scope + +- **Unifying repo *discovery* itself.** `env.ResolveContractPath` reads + `DOTFILES_REPO_DIR` via `os.Getenv` (it must not call `ResolvePath`, which would + recurse into it); doctor reads it via its `System` seam. The *precedence* is now + shared; the discovery seam legitimately differs (globals vs injectable seam) and + matches in production. Not merged further. +- **The other fresh-machine bugs** (#691, #690, #689) — their own issues. +- **Re-deploying a stale contract.** This makes doctor *read* repo-first and say + so; it does not auto-refresh `~/.dotfiles` (that is `dotf update` / setup). + +## Risks / open questions + +- A machine that legitimately runs the deployed copy with NO checkout present is + unaffected: `repoDir` is empty, so resolution falls straight through to the + deployed copy (tier 2) exactly as before. +- Changing the contract "loaded" line from `Pass(… from )` to + `Pass(loaded)` + `Info(contract: )` keeps the `env-contract.json loaded` + substring the existing test asserts, and makes the path visible in non-verbose. + +## Acceptance criteria + +- [x] `env.ResolveRepoFirst` resolves repo → deployed → walk-up → "" in that order. +- [x] `doctor.loadConfig` resolves contract AND versions.conf via the shared + resolver, repo-first, honoring `DOTFILES_REPO_DIR`. +- [x] `dotf doctor` prints `contract:` and `versions.conf:` provenance lines + (visible in non-verbose). +- [x] Anti-drift guard: with both copies present doctor picks the repo copy and + reads the repo pin; env agrees. +- [x] `go build`/`vet`/`test ./...` clean; golangci-lint clean; provenance + observed end-to-end on this machine (resolves the repo copy). + +## References + +- GH issue: [#697](https://github.com/mlorentedev/dotfiles/issues/697) +- ADR-025 (cross-machine paths / the cascade) +- Sibling: BUG-029/#696 (seed machine.json; same fragmented-resolution theme) +- At-risk prior fix: #663 (AGE_KEY_PATH/SOPS_AGE_KEY_FILE), #518 (age-key + discovery regression this stale read re-introduced) diff --git a/specs/BUG-030-doctor-env-shared-resolver/tasks.md b/specs/BUG-030-doctor-env-shared-resolver/tasks.md new file mode 100644 index 0000000..e8c75c6 --- /dev/null +++ b/specs/BUG-030-doctor-env-shared-resolver/tasks.md @@ -0,0 +1,24 @@ +--- +id: "BUG-030-doctor-env-shared-resolver" +type: spec +status: implementing +created: "2026-07-10" +tags: [spec, tasks] +template_version: "1.0" +--- + +# Tasks — BUG-030-doctor-env-shared-resolver + +- [x] T1. `env.ResolveRepoFirst(name, repoDir, dotfilesDir, startDir)` + + refactor `ResolveContractPath` to call it. Unit tests (precedence + fallbacks). +- [x] T2. `doctor.loadConfig`: resolve contract AND versions.conf via + `ResolveRepoFirst`, repo-first; resolve `repoDir` from `DOTFILES_REPO_DIR` (real + dir) else `.git` walk-up. Store `VersionsPath`. Remove the dead deployed-first + helpers (`firstExisting`/`joinIf`). +- [x] T3. Provenance: `rep.Info("contract: …")` in `loadContractSection`, + `rep.Info("versions.conf: …")` in `checkVersionMatch` (visible in non-verbose). +- [x] T4. Anti-drift guard: doctor test picks the repo copy over the deployed one + and cross-checks `env.ResolveContractPath` agrees. +- [x] T5. Local verification: `go build`/`vet`/`test ./...`, golangci-lint clean, + `dotf doctor` provenance observed resolving the repo copy. +- [ ] T6. CI green (`lint`, `lint-powershell`, `test`, `integration`, `spec-gate`). diff --git a/specs/BUG-030-doctor-env-shared-resolver/verification.md b/specs/BUG-030-doctor-env-shared-resolver/verification.md new file mode 100644 index 0000000..eebb5f8 --- /dev/null +++ b/specs/BUG-030-doctor-env-shared-resolver/verification.md @@ -0,0 +1,41 @@ +--- +id: "BUG-030-doctor-env-shared-resolver" +type: spec +status: implementing +created: "2026-07-10" +tags: [spec, verification] +template_version: "1.0" +--- + +# Verification — BUG-030-doctor-env-shared-resolver + +## Reproduction (pre-fix, #697) + +On a machine whose `~/.dotfiles` lags the repo (deployed contract predates #663; +deployed `DOTF_VERSION` older than the repo pin): `dotf doctor` reads the stale +deployed copy → `[FAIL] paths.* stale`, while `dotf env generate --check` reads +the fresh repo copy → `ok: up to date`. Contradictory verdicts, one binary. + +## Automated evidence (this branch) + +| Check | Command | Result | +|---|---|---| +| ResolveRepoFirst precedence | `go test ./internal/env/...` | pass | +| loadConfig repo-first + reads repo pin | `go test ./internal/doctor/...` | pass | +| env ↔ doctor agree (cross-check) | `TestLoadConfigResolvesRepoFirst` | pass | +| Build / vet / full suite | `go build ./...`, `go vet ./...`, `go test ./...` | clean | +| Lint | `golangci-lint run ./internal/{env,doctor}/...` | clean | +| Provenance rendered | `dotf doctor` on this machine | `[INFO] contract: …\dotfiles\env-contract.json` + `[INFO] versions.conf: …` — the **repo** copy, not `~/.dotfiles` | + +## CI evidence (post-push, T6) + +- [ ] `test` (Go unit incl. the anti-drift guard) green on ubuntu-latest. +- [ ] `lint` (golangci v2), `lint-powershell`, `spec-gate`, `integration` green. + +## Guard rationale (incident -> guard) + +The anti-drift test encodes the exact #697 failure: with BOTH the checkout and +the deployed copy present (and differing), doctor must resolve the checkout copy +and read the repo pin — and `env.ResolveContractPath` must return the same path. +Any future change that reintroduces a deployed-first (or otherwise divergent) +order for either command turns the test red.