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..1d84b77 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 @@ -158,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/cmd/initcmd/init_test.go b/internal/cmd/initcmd/init_test.go index 2ad200c..495db3d 100644 --- a/internal/cmd/initcmd/init_test.go +++ b/internal/cmd/initcmd/init_test.go @@ -16,18 +16,18 @@ 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" ) // 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") @@ -185,6 +185,99 @@ 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)) + + // 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")) + + 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 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) { 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 0c4ff36..a0ba567 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,118 @@ 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() } -// Path is the config.yml location. -func Path() string { return filepath.Join(Dir(), configFileName) } +// 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 (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. +// +// 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) { - c := &Config{} - data, err := os.ReadFile(Path()) + newDir, err := Dir() if err != nil { - if os.IsNotExist(err) { - c.applyDefaults() - return c, 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 := detectRelocation(newDir) + if derr != nil && errors.Is(derr, ErrRelocationConflict) { + relErr = derr + } else if derr != nil { + return nil, derr + } + + c := &Config{} + read := false + // Attempt new-dir read unconditionally so soft-degrading callers get the + // 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 + if uerr := yaml.Unmarshal(data, c); uerr != nil { + 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 + } + } else if !os.IsNotExist(err) { + if relErr == nil { + return nil, fmt.Errorf("read config %s: %w", newYML, err) + } + relErr = fmt.Errorf("%w; canonical also unreadable: %v", relErr, 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) + 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) + } + relErr = fmt.Errorf("%w; old also malformed: %v", relErr, uerr) + } else { + read = true + } + } else if !os.IsNotExist(err) { + 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 + // 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 +184,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..94d4ddf --- /dev/null +++ b/internal/config/relocate.go @@ -0,0 +1,267 @@ +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 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() + return reflect.DeepEqual(a, b) +} + +// 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 +} + +// 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) + if err == nil { + return true + } + return !os.IsNotExist(err) +} + +// 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) { + 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 + } + 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..c7025fa --- /dev/null +++ b/internal/config/relocate_test.go @@ -0,0 +1,330 @@ +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 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{} + return loadForRuntimeFromNewDir(newDir) +} + +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") +}