From 097822207641ecf169b3a4c7a6fc627d5a6350d4 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Wed, 20 May 2026 06:25:34 -0400 Subject: [PATCH 1/4] feat(state): port slck to cli-common state components MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopt cli-common/statedir for the config-dir resolver (native per OS: $XDG_CONFIG_HOME→Linux, ~/Library/Application Support→macOS, %APPDATA%→Windows; relative $XDG_CONFIG_HOME now errors per the §1.1 intentional tightening). Make Save atomic via os.CreateTemp + chmod + rename (perms 0700/0600 already correct). Bump cli-common pin b753d5c → e67b2fc (HEAD; statedir/statedirtest available). Add a macOS/Windows config-relocation gate (copy-leave-old; fail loud on divergence; mutation-free runtime read-fallback via the new LoadForRuntime wrapper used by non-init callers). The gate runs ahead of EnsureMigrated in `slck init` so divergent old/new aborts before any token migration / config write papers over the conflict. Old-only malformed YAML fails loud BEFORE CopyNeeded so a corrupt legacy file never propagates into the new dir. LoadForRuntime soft-degrade contract (MON-5371 lesson): only suppress ErrRelocationConflict when a canonical config was actually read. A malformed canonical under conflict returns (nil, ErrRelocationConflict) so non-init commands hard-fail instead of silently swapping CredentialRef back to DefaultCredentialRef. `slck config clear --all` now removes BOTH the new canonical config.yml AND the pre-MON-5372 hand-rolled path. Runtime old-only fallback would otherwise let a stale old config silently resurrect post-clear. Path-identity dedup (Linux: old == new). Switch internal/testutil to delegate state-dir isolation to cli-common/statedirtest (full 7-var set) — closes a Windows real-dir leak (AppData/USERPROFILE/LocalAppData/XDG_CACHE_HOME/XDG_DATA_HOME were missing). Add testutil.ConfigDir(t) for the new canonical surface and testutil.LegacyCredentialsPath(t) for the keychain migrator's hand-rolled probe path (the two diverge on macOS/Windows; test helpers must not conflate them). §3.2 acceptance matrix coverage: 8 cases against the config surface (old-only / new-only / equal-both / divergent-both / malformed-old / malformed-new / neither / no-real-dir-writes) plus runtime soft-conflict canonical-returned and malformed-canonical-hard-fail. Closes #162 [MON-5372] --- go.mod | 2 +- go.sum | 2 + internal/cmd/config/clear.go | 35 ++- internal/cmd/config/config_test.go | 34 +++ internal/cmd/config/show.go | 2 +- internal/cmd/initcmd/init.go | 14 ++ internal/cmd/initcmd/init_test.go | 10 +- internal/config/config.go | 154 +++++++++--- internal/config/config_test.go | 95 ++++++++ internal/config/relocate.go | 256 ++++++++++++++++++++ internal/config/relocate_test.go | 373 +++++++++++++++++++++++++++++ internal/keychain/keychain.go | 4 +- internal/keychain/keychain_test.go | 16 +- internal/testutil/testutil.go | 70 +++++- 14 files changed, 997 insertions(+), 70 deletions(-) create mode 100644 internal/config/config_test.go create mode 100644 internal/config/relocate.go create mode 100644 internal/config/relocate_test.go diff --git a/go.mod b/go.mod index ff1fa98..cb634c9 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/open-cli-collective/slack-chat-api go 1.24.0 require ( - github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 + github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d github.com/spf13/cobra v1.8.0 github.com/stretchr/testify v1.11.1 golang.org/x/term v0.27.0 diff --git a/go.sum b/go.sum index 8589f67..b90901c 100644 --- a/go.sum +++ b/go.sum @@ -25,6 +25,8 @@ github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e h1:fD57ERR4JtEqsWb github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14 h1:78EW5uCbAzbAO32+oY4HDaFOqS2sPYnc4AT+G5UjdL0= github.com/open-cli-collective/cli-common v0.0.0-20260516182733-b753d5c62d14/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds= +github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d h1:fiyfxc/Wvpuen/u6Mk3bCUG0DLyOylF0K+eol4y9C64= +github.com/open-cli-collective/cli-common v0.0.0-20260519134256-e67b2fc81f9d/go.mod h1:5i4MkFToMVPLBW29O01lsHS9d1m9pC0BxSOYjFDz7ds= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= diff --git a/internal/cmd/config/clear.go b/internal/cmd/config/clear.go index bed81ba..1d66ab1 100644 --- a/internal/cmd/config/clear.go +++ b/internal/cmd/config/clear.go @@ -54,18 +54,29 @@ func runClear(opts *clearOptions) error { } if opts.all { - // §1.7: --all returns the active profile to a pre-init state — - // keyring bundle (above) + config file + caches + empty dirs. - // slck has NO cache directory today (no command writes one); if - // one is ever added it must also be removed here. - p := appconfig.Path() - switch err := os.Remove(p); { - case err == nil: - output.Printf("Removed %s\n", p) - case os.IsNotExist(err): - // idempotent - default: - return err + // §1.7: --all returns the active profile to a pre-init state. We + // must remove BOTH the new canonical config and the pre-MON-5372 + // hand-rolled path — runtime old-only fallback would otherwise let + // a stale old config silently resurrect post-clear. Path-identity + // dedupe (Linux: old == new). + paths := []string{} + newPath, perr := appconfig.Path() + if perr != nil { + return perr + } + paths = append(paths, newPath) + if oldPath, oerr := appconfig.OldConfigPath(); oerr == nil && oldPath != newPath { + paths = append(paths, oldPath) + } + for _, p := range paths { + switch err := os.Remove(p); { + case err == nil: + output.Printf("Removed %s\n", p) + case os.IsNotExist(err): + // idempotent + default: + return err + } } } return nil diff --git a/internal/cmd/config/config_test.go b/internal/cmd/config/config_test.go index 8e0a25c..69709fb 100644 --- a/internal/cmd/config/config_test.go +++ b/internal/cmd/config/config_test.go @@ -2,6 +2,8 @@ package config import ( "bytes" + "os" + "path/filepath" "strings" "testing" @@ -10,6 +12,7 @@ import ( "github.com/open-cli-collective/cli-common/credstore" + appconfig "github.com/open-cli-collective/slack-chat-api/internal/config" "github.com/open-cli-collective/slack-chat-api/internal/keychain" "github.com/open-cli-collective/slack-chat-api/internal/output" "github.com/open-cli-collective/slack-chat-api/internal/testutil" @@ -121,3 +124,34 @@ func TestRunClear_Idempotent(t *testing.T) { _, err = captureOutput(t, func() error { return runClear(&clearOptions{}) }) require.NoError(t, err) } + +// TestRunClearAll_RemovesBothNewAndOldConfigPaths pins the MON-5372 contract: +// --all must scrub BOTH the new canonical config.yml AND the pre-MON-5372 +// hand-rolled path. Otherwise a stale old file would silently resurrect the +// config via runtime old-only fallback. Linux: paths dedupe to one. +func TestRunClearAll_RemovesBothNewAndOldConfigPaths(t *testing.T) { + testutil.Setup(t) + + // Plant a config.yml at the NEW canonical path. + newPath, err := appconfig.Path() + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Dir(newPath), 0o700)) + require.NoError(t, os.WriteFile(newPath, []byte("credential_ref: slack-chat-api/test\n"), 0o600)) + + // Plant another config.yml at the OLD hand-rolled path (distinct on + // macOS/Windows; identical to new on Linux — which the dedupe handles). + oldPath, err := appconfig.OldConfigPath() + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Dir(oldPath), 0o700)) + require.NoError(t, os.WriteFile(oldPath, []byte("credential_ref: slack-chat-api/stale\n"), 0o600)) + + _, err = captureOutput(t, func() error { return runClear(&clearOptions{all: true}) }) + require.NoError(t, err) + + if _, e := os.Stat(newPath); !os.IsNotExist(e) { + t.Errorf("new path must be removed by --all: stat err=%v", e) + } + if _, e := os.Stat(oldPath); !os.IsNotExist(e) { + t.Errorf("old path must be removed by --all: stat err=%v", e) + } +} diff --git a/internal/cmd/config/show.go b/internal/cmd/config/show.go index 6e8430a..a2ca877 100644 --- a/internal/cmd/config/show.go +++ b/internal/cmd/config/show.go @@ -39,7 +39,7 @@ type showStatus struct { } func runShow(_ *showOptions) error { - cfg, err := appconfig.Load() + cfg, err := appconfig.LoadForRuntime() if err != nil { return err } diff --git a/internal/cmd/initcmd/init.go b/internal/cmd/initcmd/init.go index f954382..e235053 100644 --- a/internal/cmd/initcmd/init.go +++ b/internal/cmd/initcmd/init.go @@ -77,6 +77,20 @@ func runInit(opts *initOptions) error { output.Println("Slack CLI Setup") output.Println() + // Step -1 (must precede the §1.8 keyring migration): the MON-5372 + // config-dir relocation gate. If the old hand-rolled dir and the new + // statedir-resolved dir both exist with divergent settings, abort + // BEFORE any token migration / config write — otherwise we'd soft-read + // one side, migrate, then SaveConfig, papering over the conflict. On + // only-old we copy old→new (atomic, idempotent). + if reloc, err := appconfig.DetectConfigRelocation(); err != nil { + return fmt.Errorf("detecting config relocation: %w", err) + } else if reloc.CopyNeeded { + if err := appconfig.ApplyConfigRelocation(reloc); err != nil { + return fmt.Errorf("relocating config from %s to %s: %w", reloc.OldPath, reloc.NewPath, err) + } + } + // Opening the store runs the one-time legacy migration (§1.8). With // --overwrite a legacy/keyring conflict is resolved by forcing legacy. open := keychain.Open diff --git a/internal/cmd/initcmd/init_test.go b/internal/cmd/initcmd/init_test.go index 2ad200c..55c12cc 100644 --- a/internal/cmd/initcmd/init_test.go +++ b/internal/cmd/initcmd/init_test.go @@ -6,7 +6,6 @@ import ( "net/http" "net/http/httptest" "os" - "path/filepath" "strings" "testing" @@ -21,13 +20,12 @@ import ( ) // writeLegacyCreds writes the legacy plaintext credentials file at the path -// the one-time migration scans ($XDG_CONFIG_HOME/slack-chat-api/credentials, -// per testutil.Setup's isolated XDG). +// the keychain migrator's legacyCredentialsPath() scans (the hand-rolled +// XDG/$HOME/.config path — distinct from the new canonical config dir on +// macOS/Windows post-MON-5372). func writeLegacyCreds(t *testing.T, kv map[string]string) string { t.Helper() - dir := filepath.Join(os.Getenv("XDG_CONFIG_HOME"), "slack-chat-api") - require.NoError(t, os.MkdirAll(dir, 0o700)) - path := filepath.Join(dir, "credentials") + path := testutil.LegacyCredentialsPath(t) var b strings.Builder for k, v := range kv { b.WriteString(k + "=" + v + "\n") diff --git a/internal/config/config.go b/internal/config/config.go index 0c4ff36..65fbbec 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,16 +1,19 @@ // Package config holds slck's non-secret on-disk configuration per the Open -// CLI Collective Secret-Handling Standard §1.2 / §2.4. No access secret is -// ever written here — secrets live only in the OS keyring via cli-common's +// CLI Collective Secret-Handling Standard §1.2 / §2.4 and the +// working-with-state.md state-component standard. No access secret is ever +// written here — secrets live only in the OS keyring via cli-common's // credstore. This file owns config.yml: the authoritative credential_ref // (§1.3), the non-secret workspace identifier, and the optional §1.4 // file-backend opt-in. package config import ( + "errors" "fmt" "os" "path/filepath" + "github.com/open-cli-collective/cli-common/statedir" "gopkg.in/yaml.v3" ) @@ -20,10 +23,25 @@ const ( // callers — never assumed structurally (§1.3). DefaultCredentialRef = "slack-chat-api/default" - appDirName = "slack-chat-api" + // AppDirName is the credential-scope name for this CLI (working-with- + // state.md §3). Single-binary → tool/repo name. Exposed so the + // relocation helpers can compose paths off it. + AppDirName = "slack-chat-api" configFileName = "config.yml" + + // DirPerm / FilePerm match working-with-state.md §3 (0700 dirs, 0600 + // files). slck was already at these perms; statedir's ConfigDirEnsured + // enforces 0700 on new creations. + DirPerm = 0o700 + FilePerm = 0o600 ) +// configScope is the cli-common state-scope for slck's config dir. Resolution +// is os.UserConfigDir + AppDirName: Linux $XDG_CONFIG_HOME (or ~/.config), +// macOS ~/Library/Application Support, Windows %APPDATA%. A relative +// $XDG_CONFIG_HOME on Linux now yields an error (§1.1 intentional tightening). +var configScope = statedir.Scope{Name: AppDirName} + // Config is slck's config.yml. Everything here is safe for an org to commit // to a private/MDM-controlled store (§1.2); none of it is an access secret. type Config struct { @@ -46,38 +64,88 @@ type KeyringConfig struct { Backend string `yaml:"backend,omitempty"` } -// Dir is the cross-platform config directory: $XDG_CONFIG_HOME/slack-chat-api -// else ~/.config/slack-chat-api. Identical on Linux, macOS, and Windows — -// this matches the released layout (the legacy code has no %APPDATA% branch), -// so config.yml sits beside the legacy credentials file it supersedes. -func Dir() string { - if xdg := os.Getenv("XDG_CONFIG_HOME"); xdg != "" { - return filepath.Join(xdg, appDirName) - } - home, _ := os.UserHomeDir() - return filepath.Join(home, ".config", appDirName) +// Dir resolves the config directory WITHOUT creating it. Delegated to +// cli-common/statedir; native per OS. A relative $XDG_CONFIG_HOME on Linux +// is rejected (§1.1). +func Dir() (string, error) { + return configScope.ConfigDir() +} + +// dirEnsured resolves and creates the config dir at 0700. Used by Save. +func dirEnsured() (string, error) { + return configScope.ConfigDirEnsured() } -// Path is the config.yml location. -func Path() string { return filepath.Join(Dir(), configFileName) } +// Path is the config.yml location (resolved but not created). +func Path() (string, error) { + dir, err := Dir() + if err != nil { + return "", err + } + return filepath.Join(dir, configFileName), nil +} -// Load reads config.yml. An absent file is not an error: defaults are applied -// (CredentialRef = DefaultCredentialRef) and a usable Config is returned. +// Load reads config.yml. The strict variant — used by `slck init`'s +// relocation gate. Returns ErrRelocationConflict (with a wrapped detail +// message) when both the old hand-rolled and new statedir-resolved dirs +// contain materially-different config.yml files; on conflict, the canonical +// new-dir config is still returned alongside the error when canonical was +// readable. If LoadConfig couldn't populate cfg (e.g. malformed canonical +// YAML), returns (nil, ErrRelocationConflict) so LoadForRuntime hard-fails +// instead of warning-and-defaulting (MON-5371 lesson). An absent file is not +// an error: defaults are applied and a usable Config is returned. func Load() (*Config, error) { + relErr := error(nil) + reloc, derr := DetectConfigRelocation() + if derr != nil && errors.Is(derr, ErrRelocationConflict) { + relErr = derr + } else if derr != nil { + return nil, derr + } + c := &Config{} - data, err := os.ReadFile(Path()) - if err != nil { - if os.IsNotExist(err) { - c.applyDefaults() - return c, nil + read := false + // Attempt new-dir read unconditionally so soft-degrading callers get the + // user's actual settings alongside relErr. + if reloc.NewPath != "" { + newYML := filepath.Join(reloc.NewPath, configFileName) + if data, err := os.ReadFile(newYML); err == nil { //nolint:gosec // path from validated dir + if uerr := yaml.Unmarshal(data, c); uerr != nil { + if relErr == nil { + return nil, fmt.Errorf("parse config %s: %w", newYML, uerr) + } + } else { + read = true + } + } else if !os.IsNotExist(err) { + if relErr == nil { + return nil, fmt.Errorf("read config %s: %w", newYML, err) + } + } + } + if !read && reloc.Kind == relocOldOnly && reloc.OldPath != "" { + oldYML := filepath.Join(reloc.OldPath, configFileName) + if data, err := os.ReadFile(oldYML); err == nil { //nolint:gosec // hand-rolled legacy dir + if uerr := yaml.Unmarshal(data, c); uerr != nil { + if relErr == nil { + return nil, fmt.Errorf("parse config %s: %w", oldYML, uerr) + } + } else { + read = true + } + } else if !os.IsNotExist(err) { + if relErr == nil { + return nil, fmt.Errorf("read config %s: %w", oldYML, err) + } } - return nil, fmt.Errorf("read config %s: %w", Path(), err) } - if err := yaml.Unmarshal(data, c); err != nil { - return nil, fmt.Errorf("parse config %s: %w", Path(), err) + // Malformed canonical under conflict: soft-degrade is unsafe (would swap + // CredentialRef back to default and mask the corruption). Return nil. + if !read && relErr != nil { + return nil, relErr } c.applyDefaults() - return c, nil + return c, relErr } func (c *Config) applyDefaults() { @@ -86,18 +154,42 @@ func (c *Config) applyDefaults() { } } -// Save writes config.yml at 0600 under a 0700 directory. Non-secret, but -// there is no reason for it to be world-readable. +// Save writes config.yml atomically (temp+chmod+rename) at 0600 under a 0700 +// directory. Non-secret, but there is no reason for it to be world-readable. +// Unique temp name from os.CreateTemp so same-process concurrent saves and +// crash-leftover orphans never collide; orphan-tmp from a host crash is +// harmless (never read as config). func (c *Config) Save() error { - if err := os.MkdirAll(Dir(), 0o700); err != nil { + dir, err := dirEnsured() + if err != nil { return fmt.Errorf("create config dir: %w", err) } data, err := yaml.Marshal(c) if err != nil { return fmt.Errorf("marshal config: %w", err) } - if err := os.WriteFile(Path(), data, 0o600); err != nil { - return fmt.Errorf("write config %s: %w", Path(), err) + final := filepath.Join(dir, configFileName) + tmp, err := os.CreateTemp(dir, "config-*.yml.tmp") + if err != nil { + return fmt.Errorf("create temp config file: %w", err) + } + tmpPath := tmp.Name() + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpPath) + return fmt.Errorf("write temp config file: %w", err) + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("close temp config file: %w", err) + } + if err := os.Chmod(tmpPath, FilePerm); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("set config file mode: %w", err) + } + if err := os.Rename(tmpPath, final); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("finalize config %s: %w", final, err) } return nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..4c7a9a3 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,95 @@ +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/open-cli-collective/cli-common/statedirtest" +) + +func hermeticConfig(t *testing.T) string { + t.Helper() + return statedirtest.Hermetic(t) +} + +func TestDir_ResolvedUnderUserConfigDir(t *testing.T) { + hermeticConfig(t) + base, err := os.UserConfigDir() + if err != nil { + t.Fatalf("UserConfigDir: %v", err) + } + want := filepath.Join(base, AppDirName) + got, err := Dir() + if err != nil { + t.Fatalf("Dir: %v", err) + } + if got != want { + t.Errorf("Dir = %q, want %q", got, want) + } +} + +func TestPath_PointsAtConfigYAML(t *testing.T) { + hermeticConfig(t) + dir, _ := Dir() + p, err := Path() + if err != nil { + t.Fatalf("Path: %v", err) + } + if want := filepath.Join(dir, configFileName); p != want { + t.Errorf("Path = %q, want %q", p, want) + } +} + +func TestLoad_AbsentReturnsDefaults(t *testing.T) { + hermeticConfig(t) + c, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if c.CredentialRef != DefaultCredentialRef { + t.Errorf("CredentialRef = %q, want %q", c.CredentialRef, DefaultCredentialRef) + } +} + +func TestSave_AtomicNoStaleTmp(t *testing.T) { + hermeticConfig(t) + cfg := &Config{CredentialRef: "slack-chat-api/test", Workspace: "T_X"} + if err := cfg.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + final, _ := Path() + info, err := os.Stat(final) + if err != nil { + t.Fatalf("stat final: %v", err) + } + if info.Mode().Perm() != FilePerm { + t.Errorf("file mode = %v, want %v", info.Mode().Perm(), FilePerm) + } + dir, _ := Dir() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("readdir: %v", err) + } + for _, e := range entries { + if strings.HasSuffix(e.Name(), ".tmp") { + t.Errorf("stale temp file left in %s: %s", dir, e.Name()) + } + } +} + +func TestSave_RoundTrip(t *testing.T) { + hermeticConfig(t) + in := &Config{CredentialRef: "slack-chat-api/rt", Workspace: "T_Y", Keyring: KeyringConfig{Backend: "file"}} + if err := in.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + out, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if out.CredentialRef != in.CredentialRef || out.Workspace != in.Workspace || out.Keyring.Backend != in.Keyring.Backend { + t.Errorf("round-trip mismatch: in=%+v out=%+v", in, out) + } +} diff --git a/internal/config/relocate.go b/internal/config/relocate.go new file mode 100644 index 0000000..eb2de79 --- /dev/null +++ b/internal/config/relocate.go @@ -0,0 +1,256 @@ +package config + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + "reflect" + "sync" + + "gopkg.in/yaml.v3" +) + +// ErrRelocationConflict is returned by Load (and surfaced through +// LoadForRuntime) when both the old hand-rolled config dir and the new +// statedir-resolved config dir contain a config.yml with materially-different +// user settings. Mutation-free: nothing is copied, nothing is overwritten. +// The user reconciles by running `slck init` (which fails the same way at its +// pre-write gate) or by manually deleting one side. +var ErrRelocationConflict = errors.New("config: shared old/new config diverge") + +// relocKind is the four-way classification used by the relocation detector. +// Linux always collapses to relocNone because old and new paths are identical +// (os.UserConfigDir on Linux ≡ $XDG_CONFIG_HOME else $HOME/.config). +type relocKind int + +const ( + relocNone relocKind = iota // old absent OR old==new (Linux short-circuit) + relocOldOnly // only the old hand-rolled config.yml exists + relocBothEqual // both exist with materially-equal Configs + relocBothDivergent // both exist with materially-different Configs +) + +// SharedRelocation is the result of DetectConfigRelocation. Paths are filled +// even on relocNone so callers can log/diagnose; CopyNeeded is true iff a +// gated ApplyConfigRelocation would actually do work. +type SharedRelocation struct { + Kind relocKind + OldPath string // old hand-rolled config dir; "" on no-HOME edge + NewPath string // statedir-resolved config dir + CopyNeeded bool // relocOldOnly only +} + +// oldHandRolledConfigDir reproduces the prior pre-MON-5372 resolver: +// $XDG_CONFIG_HOME if set, else $HOME/.config; then "/slack-chat-api". Same +// shape on Linux/macOS/Windows (the deliberate "no %APPDATA% branch"). A +// missing HOME is an error (matches the original behavior). +func oldHandRolledConfigDir() (string, error) { + configHome := os.Getenv("XDG_CONFIG_HOME") + if configHome == "" { + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + configHome = filepath.Join(home, ".config") + } + return filepath.Join(configHome, AppDirName), nil +} + +// OldConfigPath returns the pre-MON-5372 hand-rolled config.yml location. +// Exported so `slck config clear --all` can scrub it alongside the new path +// (runtime old-only fallback would otherwise let a stale old file +// silently resurrect the config post-clear). +func OldConfigPath() (string, error) { + dir, err := oldHandRolledConfigDir() + if err != nil { + return "", err + } + return filepath.Join(dir, configFileName), nil +} + +// DetectConfigRelocation classifies the old/new pair without touching disk +// beyond stats and reads. Never copies, never writes. On Linux (old==new) it +// short-circuits to relocNone. +func DetectConfigRelocation() (SharedRelocation, error) { + newDir, err := Dir() + if err != nil { + return SharedRelocation{}, err + } + return detectRelocation(newDir) +} + +// detectRelocation is the testable core: the new-dir path is injected so +// macOS/Windows divergence can be exercised on Linux CI. +func detectRelocation(newDir string) (SharedRelocation, error) { + oldDir, err := oldHandRolledConfigDir() + if err != nil { + // Old path unresolvable (no HOME): treat as relocNone with new-only. + // Load still works against new; the gate is harmless. + return SharedRelocation{Kind: relocNone, NewPath: newDir}, nil + } + if oldDir == newDir { + // Linux: identical paths — nothing to relocate. + return SharedRelocation{Kind: relocNone, OldPath: oldDir, NewPath: newDir}, nil + } + + oldYML := filepath.Join(oldDir, configFileName) + newYML := filepath.Join(newDir, configFileName) + oldPresent := fileExists(oldYML) + newPresent := fileExists(newYML) + switch { + case !oldPresent && !newPresent: + return SharedRelocation{Kind: relocNone, OldPath: oldDir, NewPath: newDir}, nil + case oldPresent && !newPresent: + // Validate old parses cleanly BEFORE signaling CopyNeeded — otherwise + // init's ApplyConfigRelocation would propagate a malformed legacy + // file into the new dir and Load would die parsing it post-copy. + // §3.2 malformed-old: fail loud, mutate nothing. (MON-5371 lesson.) + if _, oerr := loadConfigFromFile(oldYML); oerr != nil { + return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, + fmt.Errorf("%w: old %s is malformed: %w", ErrRelocationConflict, oldYML, oerr) + } + return SharedRelocation{Kind: relocOldOnly, OldPath: oldDir, NewPath: newDir, CopyNeeded: true}, nil + case !oldPresent && newPresent: + return SharedRelocation{Kind: relocNone, OldPath: oldDir, NewPath: newDir}, nil + } + + // Both present — load and compare comparable subset. + oldCfg, oerr := loadConfigFromFile(oldYML) + newCfg, nerr := loadConfigFromFile(newYML) + if oerr != nil { + return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, + fmt.Errorf("%w: old %s unreadable: %w", ErrRelocationConflict, oldYML, oerr) + } + if nerr != nil { + return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, + fmt.Errorf("%w: new %s unreadable: %w", ErrRelocationConflict, newYML, nerr) + } + if configsMaterialEqual(oldCfg, newCfg) { + return SharedRelocation{Kind: relocBothEqual, OldPath: oldDir, NewPath: newDir}, nil + } + return SharedRelocation{Kind: relocBothDivergent, OldPath: oldDir, NewPath: newDir}, + fmt.Errorf("%w: old %s and new %s have different settings; reconcile (or delete one) before running slck init", + ErrRelocationConflict, oldYML, newYML) +} + +// loadConfigFromFile parses a single config.yml. applyDefaults is NOT applied +// here directly — configsMaterialEqual applies it to both sides before +// comparing, so omitted-vs-explicit DefaultCredentialRef classify as equal. +func loadConfigFromFile(path string) (Config, error) { + data, err := os.ReadFile(path) //nolint:gosec // path composed from validated config dir + if err != nil { + return Config{}, err + } + var c Config + if uerr := yaml.Unmarshal(data, &c); uerr != nil { + return Config{}, uerr + } + return c, nil +} + +// configsMaterialEqual compares the user-meaningful subset of two Configs. +// applyDefaults is called on both sides first so an omitted `credential_ref` +// (which is semantically DefaultCredentialRef per applyDefaults) compares +// equal to an explicit DefaultCredentialRef. reflect.DeepEqual on the whole +// Keyring sub-struct so future KeyringConfig fields are auto-covered. +func configsMaterialEqual(a, b Config) bool { + a.applyDefaults() + b.applyDefaults() + if a.CredentialRef != b.CredentialRef { + return false + } + if a.Workspace != b.Workspace { + return false + } + if !reflect.DeepEqual(a.Keyring, b.Keyring) { + return false + } + return true +} + +// ApplyConfigRelocation copies the single config.yml file from old → new +// atomically; idempotent (skips if new already exists). The old dir is NOT +// modified — leave-old gives the user a recovery point and matches the +// MON-5370/5371 family invariant. Called only from `slck init`'s gate. +func ApplyConfigRelocation(r SharedRelocation) error { + if !r.CopyNeeded { + return nil + } + if r.OldPath == "" || r.NewPath == "" { + return fmt.Errorf("config: ApplyConfigRelocation called with empty path") + } + if err := os.MkdirAll(r.NewPath, DirPerm); err != nil { + return fmt.Errorf("create new config dir: %w", err) + } + src := filepath.Join(r.OldPath, configFileName) + dst := filepath.Join(r.NewPath, configFileName) + if fileExists(dst) { + return nil // idempotent + } + return copyFileAtomic(src, dst) +} + +func copyFileAtomic(src, dst string) error { + in, err := os.Open(src) //nolint:gosec // path from old config dir + if err != nil { + return err + } + defer func() { _ = in.Close() }() + + dir := filepath.Dir(dst) + tmp, err := os.CreateTemp(dir, filepath.Base(dst)+"-*.tmp") + if err != nil { + return err + } + tmpPath := tmp.Name() + if _, err := io.Copy(tmp, in); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpPath) + return err + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpPath) + return err + } + if err := os.Chmod(tmpPath, FilePerm); err != nil { + _ = os.Remove(tmpPath) + return err + } + if err := os.Rename(tmpPath, dst); err != nil { + _ = os.Remove(tmpPath) + return err + } + return nil +} + +func fileExists(path string) bool { + _, err := os.Stat(path) + return err == nil +} + +// LoadForRuntime is the soft-conflict variant of Load for non-init callers. +// On ErrRelocationConflict it prints a one-shot stderr warning and returns +// the canonical (new-dir) config so the command can keep working — BUT only +// when a canonical config was actually read. If Load couldn't populate cfg +// (e.g. malformed YAML on the canonical side), the runtime must hard-fail +// instead of warning-and-defaulting; otherwise it would silently swap +// CredentialRef back to DefaultCredentialRef and mask the corrupt file +// (MON-5371 contract). +func LoadForRuntime() (*Config, error) { + cfg, err := Load() + if err != nil && errors.Is(err, ErrRelocationConflict) && cfg != nil { + warnReloConflictOnce(err) + return cfg, nil + } + return cfg, err +} + +var reloConflictOnce sync.Once + +func warnReloConflictOnce(err error) { + reloConflictOnce.Do(func() { + fmt.Fprintf(os.Stderr, "warning: %v; using the new config. Run `slck init` to reconcile.\n", err) + }) +} diff --git a/internal/config/relocate_test.go b/internal/config/relocate_test.go new file mode 100644 index 0000000..7332490 --- /dev/null +++ b/internal/config/relocate_test.go @@ -0,0 +1,373 @@ +package config + +import ( + "errors" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/open-cli-collective/cli-common/statedirtest" +) + +// reloctest gives two distinct directories (old, new) under a per-test +// hermetic root. detectAt below pins XDG_CONFIG_HOME at the old dir's +// parent so oldHandRolledConfigDir resolves to oldDir on Linux too. +func reloctest(t *testing.T) (oldDir, newDir string) { + t.Helper() + root := statedirtest.Hermetic(t) + oldDir = filepath.Join(root, "old", AppDirName) + newDir = filepath.Join(root, "new", AppDirName) + return oldDir, newDir +} + +func detectAt(t *testing.T, oldDir, newDir string) (SharedRelocation, error) { + t.Helper() + t.Setenv("XDG_CONFIG_HOME", filepath.Dir(oldDir)) + return detectRelocation(newDir) +} + +func TestRelocate_OldOnly_CopiedAtInit(t *testing.T) { + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(oldDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, configFileName), + []byte("credential_ref: slack-chat-api/old\nworkspace: T_OLD\n"), 0o600); err != nil { + t.Fatal(err) + } + + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocOldOnly || !r.CopyNeeded { + t.Fatalf("kind=%v copy=%v, want relocOldOnly w/ copy", r.Kind, r.CopyNeeded) + } + if err := ApplyConfigRelocation(r); err != nil { + t.Fatalf("apply: %v", err) + } + if _, err := os.Stat(filepath.Join(newDir, configFileName)); err != nil { + t.Errorf("config.yml not copied to new: %v", err) + } + // Old preserved (recovery point). + if _, err := os.Stat(filepath.Join(oldDir, configFileName)); err != nil { + t.Errorf("old config.yml must remain (copy-leave-old): %v", err) + } +} + +func TestRelocate_NewOnly_LeftUntouched(t *testing.T) { + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(newDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(newDir, configFileName), + []byte("credential_ref: slack-chat-api/only-new\n"), 0o600); err != nil { + t.Fatal(err) + } + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocNone || r.CopyNeeded { + t.Fatalf("kind=%v copy=%v, want relocNone", r.Kind, r.CopyNeeded) + } + if err := ApplyConfigRelocation(r); err != nil { + t.Fatalf("apply: %v", err) + } + data, _ := os.ReadFile(filepath.Join(newDir, configFileName)) + if !strings.Contains(string(data), "slack-chat-api/only-new") { + t.Errorf("new must be untouched, got %q", string(data)) + } +} + +func TestRelocate_Equal_NoOp(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, d := range []string{oldDir, newDir} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(d, configFileName), + []byte("credential_ref: slack-chat-api/same\nworkspace: T1\n"), 0o600); err != nil { + t.Fatal(err) + } + } + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocBothEqual { + t.Errorf("kind=%v, want relocBothEqual", r.Kind) + } + if r.CopyNeeded { + t.Errorf("equal must not request a copy") + } +} + +func TestRelocate_Equal_DefaultOmittedVsExplicit_IsEqual(t *testing.T) { + // Old omits credential_ref (semantically DefaultCredentialRef via + // applyDefaults); new spells it out. Must classify as equal — not a + // real user-divergence. + oldDir, newDir := reloctest(t) + for _, d := range []string{oldDir, newDir} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(oldDir, configFileName), + []byte("workspace: T1\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(newDir, configFileName), + []byte("credential_ref: "+DefaultCredentialRef+"\nworkspace: T1\n"), 0o600); err != nil { + t.Fatal(err) + } + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocBothEqual { + t.Errorf("kind=%v, want relocBothEqual (omitted == explicit default)", r.Kind) + } +} + +func TestRelocate_Divergent_FailLoudNamesBothPaths_MutatesNothing(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, p := range []struct{ dir, content string }{ + {oldDir, "credential_ref: slack-chat-api/old\n"}, + {newDir, "credential_ref: slack-chat-api/new\n"}, + } { + if err := os.MkdirAll(p.dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(p.dir, configFileName), []byte(p.content), 0o600); err != nil { + t.Fatal(err) + } + } + oldBefore, _ := os.ReadFile(filepath.Join(oldDir, configFileName)) + newBefore, _ := os.ReadFile(filepath.Join(newDir, configFileName)) + + r, err := detectAt(t, oldDir, newDir) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("want ErrRelocationConflict, got %v", err) + } + if r.Kind != relocBothDivergent { + t.Errorf("kind=%v, want relocBothDivergent", r.Kind) + } + if !strings.Contains(err.Error(), oldDir) || !strings.Contains(err.Error(), newDir) { + t.Errorf("error must name both paths: %v", err) + } + oldAfter, _ := os.ReadFile(filepath.Join(oldDir, configFileName)) + newAfter, _ := os.ReadFile(filepath.Join(newDir, configFileName)) + if string(oldBefore) != string(oldAfter) || string(newBefore) != string(newAfter) { + t.Errorf("detect must mutate nothing") + } +} + +func TestRelocate_OldOnlyMalformed_FailLoud_MutatesNothing(t *testing.T) { + oldDir, newDir := reloctest(t) + if err := os.MkdirAll(oldDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, configFileName), + []byte("not-valid-yaml: : :\n"), 0o600); err != nil { + t.Fatal(err) + } + r, err := detectAt(t, oldDir, newDir) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("malformed old-only must fail loud, got %v", err) + } + if r.CopyNeeded { + t.Errorf("CopyNeeded must be false on malformed old-only") + } + if _, e := os.Stat(newDir); !os.IsNotExist(e) { + t.Errorf("new dir must not exist after malformed-old detect: stat err=%v", e) + } +} + +func TestRelocate_MalformedNew_FailLoud(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, d := range []string{oldDir, newDir} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(oldDir, configFileName), + []byte("credential_ref: slack-chat-api/old\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(newDir, configFileName), + []byte("not-valid-yaml: : :\n"), 0o600); err != nil { + t.Fatal(err) + } + _, err := detectAt(t, oldDir, newDir) + if !errors.Is(err, ErrRelocationConflict) { + t.Fatalf("malformed new must fail loud, got %v", err) + } +} + +func TestRelocate_Neither_PathResolvedNotCreated(t *testing.T) { + oldDir, newDir := reloctest(t) + r, err := detectAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocNone { + t.Errorf("kind=%v, want relocNone", r.Kind) + } + if _, err := os.Stat(oldDir); !os.IsNotExist(err) { + t.Errorf("detect must not create old dir; stat err=%v", err) + } + if _, err := os.Stat(newDir); !os.IsNotExist(err) { + t.Errorf("detect must not create new dir; stat err=%v", err) + } +} + +func TestRelocate_LinuxOldEqualsNew_ShortCircuits(t *testing.T) { + root := statedirtest.Hermetic(t) + same := filepath.Join(root, "same") + if err := os.MkdirAll(same, 0o700); err != nil { + t.Fatal(err) + } + t.Setenv("XDG_CONFIG_HOME", same) + r, err := detectRelocation(filepath.Join(same, AppDirName)) + if err != nil { + t.Fatalf("detect: %v", err) + } + if r.Kind != relocNone { + t.Errorf("kind=%v, want relocNone on path-identity short-circuit", r.Kind) + } +} + +func TestApplyConfigRelocation_IdempotentSkipsExistingNew(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, d := range []string{oldDir, newDir} { + if err := os.MkdirAll(d, 0o700); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(newDir, configFileName), + []byte("credential_ref: slack-chat-api/new\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(oldDir, configFileName), + []byte("credential_ref: slack-chat-api/old\n"), 0o600); err != nil { + t.Fatal(err) + } + r := SharedRelocation{Kind: relocOldOnly, OldPath: oldDir, NewPath: newDir, CopyNeeded: true} + if err := ApplyConfigRelocation(r); err != nil { + t.Fatalf("apply: %v", err) + } + got, _ := os.ReadFile(filepath.Join(newDir, configFileName)) + if !strings.Contains(string(got), "slack-chat-api/new") { + t.Errorf("existing new must not be overwritten, got %q", string(got)) + } +} + +// loadForRuntimeAt mirrors Load + LoadForRuntime against injected dirs so +// the soft-degrade vs hard-fail branches are exercisable on Linux. +func loadForRuntimeAt(t *testing.T, oldDir, newDir string) (*Config, error) { + t.Helper() + t.Setenv("XDG_CONFIG_HOME", filepath.Dir(oldDir)) + reloConflictOnce = sync.Once{} + + r, derr := detectRelocation(newDir) + relErr := error(nil) + if derr != nil && errors.Is(derr, ErrRelocationConflict) { + relErr = derr + } else if derr != nil { + return nil, derr + } + cfg := &Config{} + read := false + if r.NewPath != "" { + newYML := filepath.Join(r.NewPath, configFileName) + if data, err := os.ReadFile(newYML); err == nil { + c, lerr := loadConfigFromFile(newYML) + if lerr != nil { + if relErr == nil { + return nil, lerr + } + } else { + cfg = &c + read = true + } + _ = data + } + } + if !read && r.Kind == relocOldOnly && r.OldPath != "" { + oldYML := filepath.Join(r.OldPath, configFileName) + c, lerr := loadConfigFromFile(oldYML) + if lerr != nil { + if relErr == nil { + return nil, lerr + } + } else { + cfg = &c + read = true + } + } + if !read && relErr != nil { + return nil, relErr + } + cfg.applyDefaults() + if relErr != nil { + warnReloConflictOnce(relErr) + return cfg, nil + } + return cfg, nil +} + +func TestLoadForRuntime_SoftConflict_ReturnsCanonical(t *testing.T) { + oldDir, newDir := reloctest(t) + for _, p := range []struct{ dir, content string }{ + {oldDir, "credential_ref: slack-chat-api/old\n"}, + {newDir, "credential_ref: slack-chat-api/new\nworkspace: T_NEW\n"}, + } { + if err := os.MkdirAll(p.dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(p.dir, configFileName), []byte(p.content), 0o600); err != nil { + t.Fatal(err) + } + } + cfg, err := loadForRuntimeAt(t, oldDir, newDir) + if err != nil { + t.Fatalf("soft-conflict must return nil err, got %v", err) + } + if cfg.CredentialRef != "slack-chat-api/new" { + t.Errorf("soft-conflict must return new-dir cfg, got CredentialRef=%q", cfg.CredentialRef) + } + if cfg.Workspace != "T_NEW" { + t.Errorf("soft-conflict must return non-default fields too, got Workspace=%q", cfg.Workspace) + } +} + +func TestLoadForRuntime_MalformedCanonicalUnderConflict_HardFails(t *testing.T) { + // Both old and new present; new is malformed. Runtime must hard-fail + // (not warn-and-default) so a corrupt canonical doesn't silently swap + // CredentialRef back to the default and mask the corruption. + oldDir, newDir := reloctest(t) + for _, p := range []struct{ dir, content string }{ + {oldDir, "credential_ref: slack-chat-api/old\n"}, + {newDir, "not-valid-yaml: : :\n"}, + } { + if err := os.MkdirAll(p.dir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(p.dir, configFileName), []byte(p.content), 0o600); err != nil { + t.Fatal(err) + } + } + cfg, err := loadForRuntimeAt(t, oldDir, newDir) + if err == nil { + t.Fatalf("malformed canonical under conflict must hard-fail, got cfg=%+v err=nil", cfg) + } + if !errors.Is(err, ErrRelocationConflict) { + t.Errorf("error must wrap ErrRelocationConflict, got %v", err) + } + if cfg != nil { + t.Errorf("cfg must be nil on hard-fail, got %+v", cfg) + } +} diff --git a/internal/keychain/keychain.go b/internal/keychain/keychain.go index ab232e7..b224890 100644 --- a/internal/keychain/keychain.go +++ b/internal/keychain/keychain.go @@ -63,7 +63,7 @@ func OpenForMigrationOverwrite() (*Store, error) { return open(true, true) } func OpenNoMigrate() (*Store, error) { return open(false, false) } func open(overwrite, runMigration bool) (*Store, error) { - cfg, err := config.Load() + cfg, err := config.LoadForRuntime() if err != nil { return nil, err } @@ -79,7 +79,7 @@ func open(overwrite, runMigration bool) (*Store, error) { // service/profile). set-credential is pure ingress; migration still runs on // the next init / first API call via the default Open path. func OpenRef(ref string) (*Store, error) { - cfg, err := config.Load() + cfg, err := config.LoadForRuntime() if err != nil { return nil, err } diff --git a/internal/keychain/keychain_test.go b/internal/keychain/keychain_test.go index 5e1dd30..5fe3917 100644 --- a/internal/keychain/keychain_test.go +++ b/internal/keychain/keychain_test.go @@ -3,7 +3,6 @@ package keychain import ( "errors" "os" - "path/filepath" "strings" "testing" @@ -192,11 +191,10 @@ func TestRefAuthoritative(t *testing.T) { func writeLegacyFile(t *testing.T, kv map[string]string) string { t.Helper() - dir := filepath.Join(os.Getenv("XDG_CONFIG_HOME"), "slack-chat-api") - if err := os.MkdirAll(dir, 0o700); err != nil { - t.Fatal(err) - } - p := filepath.Join(dir, "credentials") + // Legacy plaintext credentials file at the path the keychain migrator's + // legacyCredentialsPath() scans (the hand-rolled XDG/$HOME/.config path, + // distinct from the new canonical config dir on macOS/Windows). + p := testutil.LegacyCredentialsPath(t) var b strings.Builder for k, v := range kv { b.WriteString(k + "=" + v + "\n") @@ -226,7 +224,11 @@ func TestMigratePlaintextFileRenamesAndCleansUp(t *testing.T) { if _, err := os.Stat(legacy); !os.IsNotExist(err) { t.Fatalf("legacy plaintext file not removed: %v", err) } - if _, err := os.Stat(appconfig.Path()); err != nil { + cfgPath, perr := appconfig.Path() + if perr != nil { + t.Fatal(perr) + } + if _, err := os.Stat(cfgPath); err != nil { t.Fatalf("config.yml not written: %v", err) } } diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go index 085f1f0..c6a9a88 100644 --- a/internal/testutil/testutil.go +++ b/internal/testutil/testutil.go @@ -1,25 +1,37 @@ // Package testutil provides a hermetic credential environment for tests -// (§1.12 test obligation). It forces credstore's encrypted-file backend -// inside a per-test temp HOME with a fixed passphrase, so no test ever -// touches the real OS keyring, shells out to `security`, or depends on -// machine state. This is the test pattern B2/B3 reuse. +// (§1.12 test obligation). It delegates state-dir isolation to the shared +// cli-common/statedirtest helper (the full 7-var env set per §3.1 — closes +// the Windows real-dir leak the old HOME/XDG-only setup had), then layers +// the slck-specific keyring backend selection on top: credstore's +// encrypted-file backend with a known passphrase, plus the legacy +// `security` scan disabler so no test ever shells out to the real OS +// keychain. This is the test pattern the B2/B3 migration tests reuse. package testutil import ( + "os" "path/filepath" "testing" + "github.com/open-cli-collective/cli-common/statedirtest" + + appconfig "github.com/open-cli-collective/slack-chat-api/internal/config" "github.com/open-cli-collective/slack-chat-api/internal/output" ) -// Setup isolates HOME/XDG to a temp dir and forces the file backend with a -// known passphrase via the §1.4 named env vars. Returns the temp dir so a -// test can plant legacy artifacts (e.g. a legacy credentials file) under it. +// Setup isolates the full §3.1 7-var env set under t.TempDir() (via +// statedirtest.Hermetic) and forces credstore's file backend with a known +// passphrase via the §1.4 named env vars. The darwin `security` legacy +// probe is neutralized so the suite is hermetic regardless of the +// destination backend (§2.4). Returns the temp root so a test can plant +// legacy artifacts — but tests should resolve their paths through +// ConfigDir(t) / LegacyCredentialsPath(t) below, not by hand-building +// subdirs, because os.UserConfigDir is platform-native (macOS ~/Library/ +// Application Support, Windows %APPDATA%) and not derived from any single +// env var. func Setup(t *testing.T) string { t.Helper() - tmp := t.TempDir() - t.Setenv("HOME", tmp) - t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, "xdgconfig")) + tmp := statedirtest.Hermetic(t) // Force credstore's encrypted-file backend (never the real Keychain / // Secret Service), with the passphrase supplied non-interactively. t.Setenv("SLACK_CHAT_API_KEYRING_BACKEND", "file") @@ -34,3 +46,41 @@ func Setup(t *testing.T) string { t.Cleanup(output.ResetMigration) return tmp } + +// ConfigDir resolves the post-statedirtest hermetic CANONICAL config dir +// (statedir-resolved per OS) and creates it. Tests that plant or inspect +// `config.yml` should use this rather than hand-building subdirs of Setup's +// tmp root. +func ConfigDir(t *testing.T) string { + t.Helper() + dir, err := appconfig.Dir() + if err != nil { + t.Fatalf("testutil.ConfigDir: %v", err) + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("testutil.ConfigDir mkdir: %v", err) + } + return dir +} + +// LegacyCredentialsPath returns the pre-MON-5372 hand-rolled `credentials` +// file path the keychain migrator's `legacyCredentialsPath()` scans: +// $XDG_CONFIG_HOME/slack-chat-api/credentials else $HOME/.config/... +// Distinct from ConfigDir on macOS/Windows where the canonical resolver +// returns a different OS-native path. Tests that seed/inspect the legacy +// credentials file (the secret-bearing k=v fixture, NOT config.yml) must +// use this helper. Returns the absolute path; the parent dir is created so +// tests can `os.WriteFile` immediately. +func LegacyCredentialsPath(t *testing.T) string { + t.Helper() + configHome := os.Getenv("XDG_CONFIG_HOME") + if configHome == "" { + home, _ := os.UserHomeDir() + configHome = filepath.Join(home, ".config") + } + dir := filepath.Join(configHome, "slack-chat-api") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("testutil.LegacyCredentialsPath mkdir: %v", err) + } + return filepath.Join(dir, "credentials") +} From f62764215d267864b3aade440d52e699364baaf4 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Wed, 20 May 2026 06:31:23 -0400 Subject: [PATCH 2/4] =?UTF-8?q?fix(state):=20real=20seam=20+=20init-level?= =?UTF-8?q?=20=C2=A73.2=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex r1 caught two majors: - Test helper loadForRuntimeAt was a parallel implementation of Load + LoadForRuntime — production could regress and tests would pass. Extract unexported loadFromNewDir / loadForRuntimeFromNewDir as the testable seams; production Load / LoadForRuntime now route through the same code paths the tests exercise. - Init relocation gate was only unit-tested (Detect + Apply directly), not through runInit. A regression that moved the gate after EnsureMigrated would still pass. Add init-level tests: TestRunInit_RelocationGate_OldOnlyCopied (asserts old→new copy through runInit) and TestRunInit_RelocationGate_DivergentAborts BeforeMutation (skip on Linux where old == new; asserts abort-before-mutation on macOS/Windows). [MON-5372] --- internal/cmd/initcmd/init_test.go | 82 +++++++++++++++++++++++++++++++ internal/config/config.go | 22 +++++++-- internal/config/relocate.go | 12 ++++- internal/config/relocate_test.go | 53 ++------------------ 4 files changed, 115 insertions(+), 54 deletions(-) diff --git a/internal/cmd/initcmd/init_test.go b/internal/cmd/initcmd/init_test.go index 55c12cc..855ba75 100644 --- a/internal/cmd/initcmd/init_test.go +++ b/internal/cmd/initcmd/init_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "os" + "path/filepath" "strings" "testing" @@ -15,6 +16,7 @@ import ( "github.com/open-cli-collective/cli-common/credstore" "github.com/open-cli-collective/slack-chat-api/internal/client" + appconfig "github.com/open-cli-collective/slack-chat-api/internal/config" "github.com/open-cli-collective/slack-chat-api/internal/keychain" "github.com/open-cli-collective/slack-chat-api/internal/testutil" ) @@ -183,6 +185,86 @@ func TestRunInit_OverwriteResolvesMigrationConflict(t *testing.T) { assert.True(t, os.IsNotExist(statErr), "legacy file must be removed after forced migrate") } +// TestRunInit_RelocationGate_OldOnlyCopied pins the MON-5372 init-level +// contract: when only the old hand-rolled config.yml exists, runInit must +// copy it to the new canonical dir BEFORE any token migration / save runs. +// Tests the actual gate ordering through runInit (not just the unit-level +// Detect+Apply pair), so a regression that removed or reordered the gate +// would surface here. +func TestRunInit_RelocationGate_OldOnlyCopied(t *testing.T) { + testutil.Setup(t) + // On Linux old == new path; the gate is a no-op and the test simply + // confirms init still works. On macOS/Windows these diverge and the + // gate actually performs the copy. + configHome := os.Getenv("XDG_CONFIG_HOME") + if configHome == "" { + // statedirtest sets it; defensive. + t.Skip("hermetic env did not set XDG_CONFIG_HOME") + } + oldDir := filepath.Join(configHome, "slack-chat-api") + require.NoError(t, os.MkdirAll(oldDir, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(oldDir, "config.yml"), + []byte("credential_ref: slack-chat-api/from-old\nworkspace: T_OLD_PRE_INIT\n"), 0o600)) + + // Run init with no tokens — wizard short-circuits at "no tokens provided" + // AFTER the relocation gate has run (and AFTER EnsureMigrated). The gate + // must have copied old→new by then if they differ. + err := runInit(&initOptions{stdin: strings.NewReader("\nn\n"), noVerify: true}) + require.NoError(t, err) + + // New canonical path must exist with the old content (or, on Linux, + // remain at the same path — same assertion either way). + newDir, err := appconfig.Dir() + require.NoError(t, err) + data, err := os.ReadFile(filepath.Join(newDir, "config.yml")) + require.NoError(t, err, "new canonical config.yml must exist after init") + assert.Contains(t, string(data), "slack-chat-api/from-old", + "new canonical must carry the old config content") +} + +// TestRunInit_RelocationGate_DivergentAbortsBeforeMutation pins the MON-5372 +// fail-loud contract through runInit: divergent old/new config aborts init +// BEFORE any token migration or config write papers over the conflict. Skips +// on Linux where old == new path (the gate short-circuits to relocNone). +func TestRunInit_RelocationGate_DivergentAbortsBeforeMutation(t *testing.T) { + testutil.Setup(t) + configHome := os.Getenv("XDG_CONFIG_HOME") + if configHome == "" { + t.Skip("hermetic env did not set XDG_CONFIG_HOME") + } + // On Linux the resolved new dir IS the XDG-rooted dir — old == new, no + // way to construct divergence at the public seam. The unit-level test + // already covers this branch on every OS via the injectable newDir. + newDir, err := appconfig.Dir() + require.NoError(t, err) + oldDir := filepath.Join(configHome, "slack-chat-api") + if newDir == oldDir { + t.Skip("Linux: old path equals new path; divergence covered at unit level") + } + + require.NoError(t, os.MkdirAll(oldDir, 0o700)) + require.NoError(t, os.MkdirAll(newDir, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(oldDir, "config.yml"), + []byte("credential_ref: slack-chat-api/old\n"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(newDir, "config.yml"), + []byte("credential_ref: slack-chat-api/new\n"), 0o600)) + + // Snapshot pre-init bytes to assert mutate-nothing. + oldBefore, _ := os.ReadFile(filepath.Join(oldDir, "config.yml")) + newBefore, _ := os.ReadFile(filepath.Join(newDir, "config.yml")) + + err = runInit(&initOptions{botEnv: "BOT_TOK", noVerify: true}) + require.Error(t, err) + assert.Contains(t, err.Error(), "detecting config relocation", + "init must abort at the relocation gate") + + // Nothing mutated. + oldAfter, _ := os.ReadFile(filepath.Join(oldDir, "config.yml")) + newAfter, _ := os.ReadFile(filepath.Join(newDir, "config.yml")) + assert.Equal(t, string(oldBefore), string(oldAfter), "old config must not be mutated by aborted init") + assert.Equal(t, string(newBefore), string(newAfter), "new config must not be mutated by aborted init") +} + func TestRunInit_VerificationFailed(t *testing.T) { testutil.Setup(t) s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/config/config.go b/internal/config/config.go index 65fbbec..ad64624 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -90,13 +90,25 @@ func Path() (string, error) { // message) when both the old hand-rolled and new statedir-resolved dirs // contain materially-different config.yml files; on conflict, the canonical // new-dir config is still returned alongside the error when canonical was -// readable. If LoadConfig couldn't populate cfg (e.g. malformed canonical -// YAML), returns (nil, ErrRelocationConflict) so LoadForRuntime hard-fails -// instead of warning-and-defaulting (MON-5371 lesson). An absent file is not -// an error: defaults are applied and a usable Config is returned. +// readable. If Load couldn't populate cfg (e.g. malformed canonical YAML), +// returns (nil, ErrRelocationConflict) so LoadForRuntime hard-fails instead +// of warning-and-defaulting (MON-5371 lesson). An absent file is not an +// error: defaults are applied and a usable Config is returned. func Load() (*Config, error) { + newDir, err := Dir() + if err != nil { + return nil, err + } + return loadFromNewDir(newDir) +} + +// loadFromNewDir is the testable seam Load and the tests both call. Taking +// an injected newDir is the only way to exercise the divergent/malformed +// branches on Linux where os.UserConfigDir == $XDG_CONFIG_HOME (and so +// would collapse old == new at the public Load entry point). +func loadFromNewDir(newDir string) (*Config, error) { relErr := error(nil) - reloc, derr := DetectConfigRelocation() + reloc, derr := detectRelocation(newDir) if derr != nil && errors.Is(derr, ErrRelocationConflict) { relErr = derr } else if derr != nil { diff --git a/internal/config/relocate.go b/internal/config/relocate.go index eb2de79..3db5166 100644 --- a/internal/config/relocate.go +++ b/internal/config/relocate.go @@ -239,7 +239,17 @@ func fileExists(path string) bool { // CredentialRef back to DefaultCredentialRef and mask the corrupt file // (MON-5371 contract). func LoadForRuntime() (*Config, error) { - cfg, err := Load() + newDir, err := Dir() + if err != nil { + return nil, err + } + return loadForRuntimeFromNewDir(newDir) +} + +// loadForRuntimeFromNewDir is the testable seam LoadForRuntime and the +// tests both call. +func loadForRuntimeFromNewDir(newDir string) (*Config, error) { + cfg, err := loadFromNewDir(newDir) if err != nil && errors.Is(err, ErrRelocationConflict) && cfg != nil { warnReloConflictOnce(err) return cfg, nil diff --git a/internal/config/relocate_test.go b/internal/config/relocate_test.go index 7332490..c7025fa 100644 --- a/internal/config/relocate_test.go +++ b/internal/config/relocate_test.go @@ -265,58 +265,15 @@ func TestApplyConfigRelocation_IdempotentSkipsExistingNew(t *testing.T) { } } -// loadForRuntimeAt mirrors Load + LoadForRuntime against injected dirs so -// the soft-degrade vs hard-fail branches are exercisable on Linux. +// loadForRuntimeAt calls the production LoadForRuntime seam against the +// injected (old, new) pair. Pinning XDG_CONFIG_HOME at oldDir's parent +// makes oldHandRolledConfigDir resolve to oldDir on Linux, so the +// divergent/malformed branches are exercised on the same OS CI runs. func loadForRuntimeAt(t *testing.T, oldDir, newDir string) (*Config, error) { t.Helper() t.Setenv("XDG_CONFIG_HOME", filepath.Dir(oldDir)) reloConflictOnce = sync.Once{} - - r, derr := detectRelocation(newDir) - relErr := error(nil) - if derr != nil && errors.Is(derr, ErrRelocationConflict) { - relErr = derr - } else if derr != nil { - return nil, derr - } - cfg := &Config{} - read := false - if r.NewPath != "" { - newYML := filepath.Join(r.NewPath, configFileName) - if data, err := os.ReadFile(newYML); err == nil { - c, lerr := loadConfigFromFile(newYML) - if lerr != nil { - if relErr == nil { - return nil, lerr - } - } else { - cfg = &c - read = true - } - _ = data - } - } - if !read && r.Kind == relocOldOnly && r.OldPath != "" { - oldYML := filepath.Join(r.OldPath, configFileName) - c, lerr := loadConfigFromFile(oldYML) - if lerr != nil { - if relErr == nil { - return nil, lerr - } - } else { - cfg = &c - read = true - } - } - if !read && relErr != nil { - return nil, relErr - } - cfg.applyDefaults() - if relErr != nil { - warnReloConflictOnce(relErr) - return cfg, nil - } - return cfg, nil + return loadForRuntimeFromNewDir(newDir) } func TestLoadForRuntime_SoftConflict_ReturnsCanonical(t *testing.T) { From a31675a178c2759c8f41414dd6617b27735bbcca Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Wed, 20 May 2026 06:33:09 -0400 Subject: [PATCH 3/4] test(state): divergent-aborts test seeds legacy credentials too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex r2 catch: the divergent-abort test snapshot-asserted only config.yml bytes — if the gate were moved after keychain.Open, the test would still pass unless a legacy source existed for the migrator to mutate. Seed writeLegacyCreds before runInit and assert the legacy file still exists post-abort: that proves the gate ran BEFORE keychain.Open / migration, not just before SaveConfig. [MON-5372] --- internal/cmd/initcmd/init_test.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/internal/cmd/initcmd/init_test.go b/internal/cmd/initcmd/init_test.go index 855ba75..495db3d 100644 --- a/internal/cmd/initcmd/init_test.go +++ b/internal/cmd/initcmd/init_test.go @@ -249,6 +249,12 @@ func TestRunInit_RelocationGate_DivergentAbortsBeforeMutation(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(newDir, "config.yml"), []byte("credential_ref: slack-chat-api/new\n"), 0o600)) + // Seed a legacy credentials file too. If the relocation gate were + // accidentally moved AFTER keychain.Open, the migrator would consume + // this file before the abort and the post-abort stat would fail. With + // the gate ordered correctly, the legacy file stays untouched. + legacy := writeLegacyCreds(t, map[string]string{"api_token": "xoxb-MUST-NOT-BE-MIGRATED"}) + // Snapshot pre-init bytes to assert mutate-nothing. oldBefore, _ := os.ReadFile(filepath.Join(oldDir, "config.yml")) newBefore, _ := os.ReadFile(filepath.Join(newDir, "config.yml")) @@ -258,11 +264,18 @@ func TestRunInit_RelocationGate_DivergentAbortsBeforeMutation(t *testing.T) { assert.Contains(t, err.Error(), "detecting config relocation", "init must abort at the relocation gate") - // Nothing mutated. + // Nothing mutated by the abort. oldAfter, _ := os.ReadFile(filepath.Join(oldDir, "config.yml")) newAfter, _ := os.ReadFile(filepath.Join(newDir, "config.yml")) assert.Equal(t, string(oldBefore), string(oldAfter), "old config must not be mutated by aborted init") assert.Equal(t, string(newBefore), string(newAfter), "new config must not be mutated by aborted init") + + // Critical proof the gate ran BEFORE keychain.Open / migration: the + // legacy credentials file must still exist. If migration had run, it + // would have consumed and removed this file. + if _, statErr := os.Stat(legacy); statErr != nil { + t.Errorf("legacy credentials file must still exist (gate must abort before migration); stat err=%v", statErr) + } } func TestRunInit_VerificationFailed(t *testing.T) { From 424dfb3f3f11abc5ef39c36a665a56cae5ee1bd1 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Wed, 20 May 2026 06:42:36 -0400 Subject: [PATCH 4/4] =?UTF-8?q?fix(state):=20daemon=20r1=20=E2=80=94=20dua?= =?UTF-8?q?l-return=20doc=20+=20safety=20nits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address 4 of 7 pr-review-daemon findings (3 buggered off in-thread): - Load() now carries a prominent doc warning of the dual-return contract (non-nil cfg + non-nil ErrRelocationConflict simultaneously). Route init's only remaining direct Load() callsite through LoadForRuntime too — post-gate behavior is identical but keeps the read pattern uniform and harder to misuse. - configsMaterialEqual now uses reflect.DeepEqual on the whole default-applied Config struct, so any future top-level Config field is auto-covered as a divergence-trigger. - fileExists distinguishes os.IsNotExist from other errors; an oddly- permissioned dir no longer silently collapses the gate to a no-op. - Load wraps parse-error diagnostics into relErr under conflict (`%w; canonical also malformed: %v`) instead of swallowing them. [MON-5372] --- internal/cmd/initcmd/init.go | 7 ++++++- internal/config/config.go | 32 +++++++++++++++++++++++++------- internal/config/relocate.go | 33 +++++++++++++++++---------------- 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/internal/cmd/initcmd/init.go b/internal/cmd/initcmd/init.go index e235053..1d84b77 100644 --- a/internal/cmd/initcmd/init.go +++ b/internal/cmd/initcmd/init.go @@ -172,7 +172,12 @@ func runInit(opts *initOptions) error { } // Persist non-secret config (credential_ref + workspace, §1.2/§2.4). - cfg, err := appconfig.Load() + // Use LoadForRuntime even here so the dual-return-on-conflict contract + // is never exposed to a `if err != nil` callsite: by this point the + // init relocation gate above has reconciled or aborted, so soft-degrade + // is moot — but routing through LoadForRuntime keeps the read pattern + // uniform and the contract harder to misuse. + cfg, err := appconfig.LoadForRuntime() if err != nil { return err } diff --git a/internal/config/config.go b/internal/config/config.go index ad64624..a0ba567 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -88,12 +88,22 @@ func Path() (string, error) { // Load reads config.yml. The strict variant — used by `slck init`'s // relocation gate. Returns ErrRelocationConflict (with a wrapped detail // message) when both the old hand-rolled and new statedir-resolved dirs -// contain materially-different config.yml files; on conflict, the canonical -// new-dir config is still returned alongside the error when canonical was -// readable. If Load couldn't populate cfg (e.g. malformed canonical YAML), -// returns (nil, ErrRelocationConflict) so LoadForRuntime hard-fails instead -// of warning-and-defaulting (MON-5371 lesson). An absent file is not an -// error: defaults are applied and a usable Config is returned. +// contain materially-different config.yml files. +// +// DUAL-RETURN CONTRACT: on a relocation conflict, Load can return BOTH a +// non-nil *Config (the canonical new-dir config, if readable) AND a non-nil +// error wrapping ErrRelocationConflict — so the caller can choose to +// soft-degrade. The standard `if err != nil { return err }` idiom would +// silently DISCARD a valid config in the conflict case; callers must check +// `errors.Is(err, ErrRelocationConflict)` explicitly, or use the +// LoadForRuntime wrapper which encodes the right discipline. Runtime call +// sites have all been migrated to LoadForRuntime; init uses Load directly +// only AFTER its own DetectConfigRelocation gate succeeded (so no conflict +// can be observed at that callsite). If Load couldn't populate cfg (e.g. +// malformed canonical YAML), returns (nil, ErrRelocationConflict) so +// LoadForRuntime hard-fails instead of warning-and-defaulting (MON-5371 +// lesson). An absent file is not an error: defaults are applied and a +// usable Config is returned. func Load() (*Config, error) { newDir, err := Dir() if err != nil { @@ -118,7 +128,11 @@ func loadFromNewDir(newDir string) (*Config, error) { c := &Config{} read := false // Attempt new-dir read unconditionally so soft-degrading callers get the - // user's actual settings alongside relErr. + // user's actual settings alongside relErr. Under conflict we don't + // re-surface a bare parse error (LoadForRuntime needs the wrapped + // ErrRelocationConflict so it can soft-degrade), but we DO append the + // parse diagnostic to relErr so the user sees both the conflict path + // and the corruption detail in one message. if reloc.NewPath != "" { newYML := filepath.Join(reloc.NewPath, configFileName) if data, err := os.ReadFile(newYML); err == nil { //nolint:gosec // path from validated dir @@ -126,6 +140,7 @@ func loadFromNewDir(newDir string) (*Config, error) { if relErr == nil { return nil, fmt.Errorf("parse config %s: %w", newYML, uerr) } + relErr = fmt.Errorf("%w; canonical also malformed: %v", relErr, uerr) } else { read = true } @@ -133,6 +148,7 @@ func loadFromNewDir(newDir string) (*Config, error) { if relErr == nil { return nil, fmt.Errorf("read config %s: %w", newYML, err) } + relErr = fmt.Errorf("%w; canonical also unreadable: %v", relErr, err) } } if !read && reloc.Kind == relocOldOnly && reloc.OldPath != "" { @@ -142,6 +158,7 @@ func loadFromNewDir(newDir string) (*Config, error) { if relErr == nil { return nil, fmt.Errorf("parse config %s: %w", oldYML, uerr) } + relErr = fmt.Errorf("%w; old also malformed: %v", relErr, uerr) } else { read = true } @@ -149,6 +166,7 @@ func loadFromNewDir(newDir string) (*Config, error) { if relErr == nil { return nil, fmt.Errorf("read config %s: %w", oldYML, err) } + relErr = fmt.Errorf("%w; old also unreadable: %v", relErr, err) } } // Malformed canonical under conflict: soft-degrade is unsafe (would swap diff --git a/internal/config/relocate.go b/internal/config/relocate.go index 3db5166..94d4ddf 100644 --- a/internal/config/relocate.go +++ b/internal/config/relocate.go @@ -150,24 +150,16 @@ func loadConfigFromFile(path string) (Config, error) { return c, nil } -// configsMaterialEqual compares the user-meaningful subset of two Configs. -// applyDefaults is called on both sides first so an omitted `credential_ref` -// (which is semantically DefaultCredentialRef per applyDefaults) compares -// equal to an explicit DefaultCredentialRef. reflect.DeepEqual on the whole -// Keyring sub-struct so future KeyringConfig fields are auto-covered. +// configsMaterialEqual compares two Configs after applying defaults on both +// sides (so an omitted `credential_ref` — semantically DefaultCredentialRef +// — compares equal to an explicit DefaultCredentialRef). reflect.DeepEqual +// on the whole default-applied struct so any future Config field is +// automatically covered as a divergence-trigger; we don't need to remember +// to update this comparator when Config grows. func configsMaterialEqual(a, b Config) bool { a.applyDefaults() b.applyDefaults() - if a.CredentialRef != b.CredentialRef { - return false - } - if a.Workspace != b.Workspace { - return false - } - if !reflect.DeepEqual(a.Keyring, b.Keyring) { - return false - } - return true + return reflect.DeepEqual(a, b) } // ApplyConfigRelocation copies the single config.yml file from old → new @@ -225,9 +217,18 @@ func copyFileAtomic(src, dst string) error { return nil } +// fileExists distinguishes "not present" from other stat errors. A +// permission-denied (or any other non-IsNotExist) error must NOT silently +// degrade to "absent" — that would let an oddly-permissioned old dir +// collapse an old-only relocation to a no-op. Treat unknown errors as +// "present" so the relocation flow's subsequent open/read surfaces the +// real error instead of skipping the gate. func fileExists(path string) bool { _, err := os.Stat(path) - return err == nil + if err == nil { + return true + } + return !os.IsNotExist(err) } // LoadForRuntime is the soft-conflict variant of Load for non-init callers.