From 695bd02a3f950bcaf56403e9900f612268962c14 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 09:23:45 +0200 Subject: [PATCH 01/15] test(testenv): add sandbox env-redirect helper for test isolation --- internal/testenv/testenv.go | 183 ++++++++++++++++++++++++ internal/testenv/testenv_test.go | 237 +++++++++++++++++++++++++++++++ 2 files changed, 420 insertions(+) create mode 100644 internal/testenv/testenv.go create mode 100644 internal/testenv/testenv_test.go diff --git a/internal/testenv/testenv.go b/internal/testenv/testenv.go new file mode 100644 index 0000000..b475320 --- /dev/null +++ b/internal/testenv/testenv.go @@ -0,0 +1,183 @@ +// Package testenv redirects every environment variable grant uses to locate +// user state at a throwaway temporary directory, so running the test suite can +// never read or clobber the developer's real ~/.grant or ~/.idsec. +// +// It is a normal (non-test) package so that TestMain functions in several +// packages can share it, but it is imported only from _test.go files and so +// never links into the shipped binary. It deliberately does NOT import +// "testing": that import belongs to test binaries, and keeping it out means +// no importer inherits the testing flag set. +// +// # What the assertions actually prove +// +// AssertSandboxed verifies that the *configured destinations* — the paths +// config.ConfigDir, config.ConfigPath, cache.CacheDir and +// profiles.GetProfilesFolder resolve to — all sit under the sandbox root. That +// is all it proves. It does NOT prove that no code wrote outside the sandbox: +// it cannot see a future direct os.UserHomeDir call, a hardcoded path, or a +// dependency that writes via some other variable, and it cannot detect reads at +// all. A filesystem snapshot-diff gate was considered and rejected — a +// concurrently running real `grant` false-positives with certainty, size+mtime +// misses same-size rewrites, and reads stay invisible either way. +package testenv + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/aaearon/grant-cli/internal/cache" + "github.com/aaearon/grant-cli/internal/config" + "github.com/cyberark/idsec-sdk-golang/pkg/profiles" +) + +// redirectedVars lists every environment variable Run overrides, in the order +// it sets them. Each one is a way for some resolver to reach the real home: +// +// - HOME — POSIX os.UserHomeDir, and the SDK profile loader, +// which reads os.Getenv("HOME") directly on every platform. +// - USERPROFILE — Go's Windows os.UserHomeDir reads USERPROFILE, then +// HOMEDRIVE+HOMEPATH. It never consults HOME, so +// redirecting HOME alone leaves the Windows CI leg +// pointed at the real profile directory. +// - XDG_CONFIG_HOME — consulted by third-party config helpers. +// - IDSEC_PROFILES_FOLDER — takes precedence over HOME in the SDK loader. +// - GRANT_CONFIG — overrides config.ConfigPath. +var redirectedVars = []string{ + "HOME", + "USERPROFILE", + "XDG_CONFIG_HOME", + "IDSEC_PROFILES_FOLDER", + "GRANT_CONFIG", +} + +// sandboxRoot is the active sandbox root, or "" when Run is not executing. +var sandboxRoot string + +// Root returns the active sandbox root directory, or "" outside of Run. +func Root() string { return sandboxRoot } + +// Run creates a temporary sandbox root, redirects every variable in +// redirectedVars beneath it, invokes run, then restores the previous +// environment and removes the sandbox. It returns run's exit code so callers +// can write: +// +// func TestMain(m *testing.M) { os.Exit(testenv.Run(m.Run)) } +// +// Setting the variables here — before m.Run — is what makes this compatible +// with the t.Parallel() call sites throughout internal/: os.Setenv has no +// parallel restriction, whereas t.Setenv panics in a parallel test. +func Run(run func() int) int { + root, err := os.MkdirTemp("", "grant-testenv-") + if err != nil { + fmt.Fprintf(os.Stderr, "testenv: failed to create sandbox root: %v\n", err) + return 1 + } + + home := filepath.Join(root, "home") + if err := os.MkdirAll(home, 0o700); err != nil { + fmt.Fprintf(os.Stderr, "testenv: failed to create sandbox home: %v\n", err) + _ = os.RemoveAll(root) + return 1 + } + + values := map[string]string{ + "HOME": home, + "USERPROFILE": home, + "XDG_CONFIG_HOME": filepath.Join(home, ".config"), + "IDSEC_PROFILES_FOLDER": filepath.Join(home, ".idsec", "profiles"), + "GRANT_CONFIG": filepath.Join(home, ".grant", "config.yaml"), + } + + restore := make(map[string]*string, len(redirectedVars)) + for _, k := range redirectedVars { + if v, ok := os.LookupEnv(k); ok { + prev := v + restore[k] = &prev + } else { + restore[k] = nil + } + if err := os.Setenv(k, values[k]); err != nil { + fmt.Fprintf(os.Stderr, "testenv: failed to set %s: %v\n", k, err) + restoreEnv(restore) + _ = os.RemoveAll(root) + return 1 + } + } + + sandboxRoot = root + code := run() + sandboxRoot = "" + + restoreEnv(restore) + _ = os.RemoveAll(root) + return code +} + +// restoreEnv puts back the captured values; a nil entry means the variable was +// originally unset and must be unset again rather than set to "". +func restoreEnv(restore map[string]*string) { + for k, v := range restore { + if v == nil { + _ = os.Unsetenv(k) + continue + } + _ = os.Setenv(k, *v) + } +} + +// TB is the subset of *testing.T that AssertSandboxed needs. Accepting an +// interface is what lets this file stay free of the "testing" import. +type TB interface { + Helper() + Errorf(format string, args ...any) +} + +// AssertSandboxed checks that every path grant resolves for user state lands +// under the active sandbox root. See the package comment for exactly what this +// does and does not prove. +func AssertSandboxed(t TB) { + t.Helper() + + root := Root() + if root == "" { + t.Errorf("testenv.AssertSandboxed called outside testenv.Run; no sandbox is active") + return + } + + configDir, err := config.ConfigDir() + if err != nil { + t.Errorf("config.ConfigDir() failed inside sandbox: %v", err) + } else { + assertUnder(t, "config.ConfigDir()", configDir, root) + } + + configPath, err := config.ConfigPath() + if err != nil { + t.Errorf("config.ConfigPath() failed inside sandbox: %v", err) + } else { + assertUnder(t, "config.ConfigPath()", configPath, root) + } + + cacheDir, err := cache.CacheDir() + if err != nil { + t.Errorf("cache.CacheDir() failed inside sandbox: %v", err) + } else { + assertUnder(t, "cache.CacheDir()", cacheDir, root) + } + + assertUnder(t, "profiles.GetProfilesFolder()", profiles.GetProfilesFolder(), root) +} + +// assertUnder reports a failure unless got is root itself or below it. +func assertUnder(t TB, what, got, root string) { + t.Helper() + + cleanGot := filepath.Clean(got) + cleanRoot := filepath.Clean(root) + if cleanGot == cleanRoot || strings.HasPrefix(cleanGot, cleanRoot+string(filepath.Separator)) { + return + } + t.Errorf("%s = %q, which is outside the test sandbox %q; the suite would touch real user state", what, got, root) +} diff --git a/internal/testenv/testenv_test.go b/internal/testenv/testenv_test.go new file mode 100644 index 0000000..45337d6 --- /dev/null +++ b/internal/testenv/testenv_test.go @@ -0,0 +1,237 @@ +package testenv + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// recordingTB implements TB and records the failures reported to it, so the +// assertion helpers can be tested without failing the enclosing test. +type recordingTB struct { + helperCalls int + errs []string +} + +func (r *recordingTB) Helper() { r.helperCalls++ } + +func (r *recordingTB) Errorf(format string, args ...any) { + r.errs = append(r.errs, format) + _ = args +} + +// Not parallel: mutates process-wide environment variables. +func TestRun_RedirectsAllHomeEnvVars(t *testing.T) { + var ( + gotRoot string + seen = map[string]string{} + ) + + code := Run(func() int { + gotRoot = Root() + for _, k := range redirectedVars { + seen[k] = os.Getenv(k) + } + return 7 + }) + + if code != 7 { + t.Errorf("Run returned %d, want the code from run()", code) + } + if gotRoot == "" { + t.Fatal("Root() was empty inside run()") + } + + for _, k := range redirectedVars { + v := seen[k] + if v == "" { + t.Errorf("%s was empty inside run(); every redirected var must be set", k) + continue + } + if !strings.HasPrefix(v, gotRoot) { + t.Errorf("%s = %q, want a path under the sandbox root %q", k, v, gotRoot) + } + } +} + +// Not parallel: mutates process-wide environment variables. +func TestRun_RestoresPreviousEnvironment(t *testing.T) { + const sentinel = "/sentinel-home-value" + t.Setenv("HOME", sentinel) + // GRANT_CONFIG is deliberately left unset so we can prove an unset var + // stays unset rather than being restored as an empty string. + if err := os.Unsetenv("GRANT_CONFIG"); err != nil { + t.Fatalf("Unsetenv: %v", err) + } + t.Cleanup(func() { _ = os.Unsetenv("GRANT_CONFIG") }) + + Run(func() int { return 0 }) + + if got := os.Getenv("HOME"); got != sentinel { + t.Errorf("HOME = %q after Run, want the pre-existing value %q", got, sentinel) + } + if v, ok := os.LookupEnv("GRANT_CONFIG"); ok { + t.Errorf("GRANT_CONFIG is set to %q after Run; an originally-unset var must stay unset", v) + } +} + +// Not parallel: mutates process-wide environment variables. +func TestRun_RemovesSandboxRootAfterwards(t *testing.T) { + var root string + Run(func() int { + root = Root() + if _, err := os.Stat(root); err != nil { + t.Errorf("sandbox root %q does not exist during run(): %v", root, err) + } + return 0 + }) + + if _, err := os.Stat(root); !os.IsNotExist(err) { + t.Errorf("sandbox root %q still exists after Run (stat err = %v)", root, err) + } + if Root() != "" { + t.Errorf("Root() = %q after Run, want empty", Root()) + } +} + +// Not parallel: mutates process-wide environment variables. +func TestRun_HomeVarsPointAtARealDirectory(t *testing.T) { + Run(func() int { + home, err := os.UserHomeDir() + if err != nil { + t.Errorf("os.UserHomeDir() inside sandbox: %v", err) + return 1 + } + if !strings.HasPrefix(home, Root()) { + t.Errorf("os.UserHomeDir() = %q, want a path under sandbox root %q", home, Root()) + } + info, err := os.Stat(home) + if err != nil { + t.Errorf("sandbox home %q not created: %v", home, err) + return 1 + } + if !info.IsDir() { + t.Errorf("sandbox home %q is not a directory", home) + } + return 0 + }) +} + +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_PassesInsideSandbox(t *testing.T) { + Run(func() int { + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) != 0 { + t.Errorf("AssertSandboxed reported %d failures inside the sandbox: %v", len(rec.errs), rec.errs) + } + if rec.helperCalls == 0 { + t.Error("AssertSandboxed did not call Helper()") + } + return 0 + }) +} + +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsOutsideSandbox(t *testing.T) { + if runtime.GOOS == "windows" { + // The resolvers read USERPROFILE (os.UserHomeDir) and HOME (the SDK + // profile loader) from different variables on Windows; forcing a + // deterministic "outside" state needs both, and t.Setenv of HOME on + // Windows has no effect on os.UserHomeDir. Covered on POSIX instead. + t.Skip("environment-driven resolvers diverge on Windows; covered on POSIX") + } + + outside := t.TempDir() + t.Setenv("HOME", outside) + t.Setenv("GRANT_CONFIG", filepath.Join(outside, "config.yaml")) + t.Setenv("IDSEC_PROFILES_FOLDER", filepath.Join(outside, "profiles")) + + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) == 0 { + t.Error("AssertSandboxed reported no failures while running outside any sandbox") + } +} + +// TestAssertSandboxed_FailsWhenAResolverEscapes is the case that actually +// matters: a sandbox IS active, but one resolver still points at real user +// state. Without this, the outside-the-sandbox test above would be satisfied +// solely by the Root()=="" short-circuit. +// +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsWhenAResolverEscapes(t *testing.T) { + escapee := t.TempDir() // deliberately NOT under the sandbox root + + Run(func() int { + if err := os.Setenv("IDSEC_PROFILES_FOLDER", escapee); err != nil { + t.Errorf("Setenv: %v", err) + return 1 + } + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) != 1 { + t.Errorf("AssertSandboxed reported %d failures, want exactly 1 (the escaped profiles folder): %v", + len(rec.errs), rec.errs) + } + return 0 + }) +} + +// Not parallel: reads no globals, but kept serial with the rest of the file. +func TestAssertUnder(t *testing.T) { + root := filepath.Join(string(filepath.Separator), "sandbox", "root") + + tests := []struct { + name string + got string + wantFail bool + }{ + {name: "root itself", got: root}, + {name: "direct child", got: filepath.Join(root, "home")}, + {name: "deep descendant", got: filepath.Join(root, "home", ".grant", "cache")}, + {name: "unclean but inside", got: filepath.Join(root, "home", "..", "home", "x")}, + { + // The classic prefix-matching bug: "/sandbox/rootless" shares a + // string prefix with "/sandbox/root" but is not under it. + name: "sibling sharing a string prefix", + got: root + "less", + wantFail: true, + }, + {name: "unrelated absolute path", got: filepath.Join(string(filepath.Separator), "home", "tim", ".grant"), wantFail: true}, + {name: "parent of root", got: filepath.Dir(root), wantFail: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rec := &recordingTB{} + assertUnder(rec, "resolver()", tt.got, root) + if gotFail := len(rec.errs) > 0; gotFail != tt.wantFail { + t.Errorf("assertUnder(%q, %q) failed = %v, want %v", tt.got, root, gotFail, tt.wantFail) + } + }) + } +} + +// Not parallel: mutates process-wide environment variables. +func TestRun_IsReentrant(t *testing.T) { + // Nested/sequential calls must each get their own root and leave the + // process environment as they found it. + var first, second string + Run(func() int { + first = Root() + return 0 + }) + Run(func() int { + second = Root() + return 0 + }) + if first == "" || second == "" { + t.Fatal("Root() empty in one of the runs") + } + if first == second { + t.Errorf("both runs used the same sandbox root %q; each run must be isolated", first) + } +} From c294bc3ea3ddff83cb26710e49591788b3673e00 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 09:25:09 +0200 Subject: [PATCH 02/15] test: isolate cmd, config and cache test suites from the real home directory --- cmd/main_test.go | 90 ++++++++++++++++++++++++++++++++++++ internal/cache/main_test.go | 27 +++++++++++ internal/config/main_test.go | 27 +++++++++++ 3 files changed, 144 insertions(+) create mode 100644 cmd/main_test.go create mode 100644 internal/cache/main_test.go create mode 100644 internal/config/main_test.go diff --git a/cmd/main_test.go b/cmd/main_test.go new file mode 100644 index 0000000..a2d8aa5 --- /dev/null +++ b/cmd/main_test.go @@ -0,0 +1,90 @@ +//go:build !integration + +// The build tag is load-bearing: cmd/integration_test.go declares its own +// TestMain under `//go:build integration`, and two TestMain symbols in one +// package will not compile. Both harnesses call testenv.Run. +package cmd + +import ( + "errors" + "os" + "strings" + "testing" + + "github.com/aaearon/grant-cli/internal/cache" + "github.com/aaearon/grant-cli/internal/testenv" + sdkauth "github.com/cyberark/idsec-sdk-golang/pkg/auth" + sdkmodels "github.com/cyberark/idsec-sdk-golang/pkg/models" +) + +// errTestBootstrapDisabled is returned by the stubbed bootstrapImpl. It is a +// named sentinel rather than an anonymous error so tests can assert on it with +// errors.Is: a bare `wantErr: true` would otherwise be satisfied by an +// accidental bootstrap attempt, hiding the fact that a unit test reached for +// real credentials. +var errTestBootstrapDisabled = errors.New("bootstrapImpl is disabled in unit tests; inject via New...WithDeps instead") + +// TestMain does two things, in this order: +// +// 1. Redirects HOME/USERPROFILE/XDG_CONFIG_HOME/IDSEC_PROFILES_FOLDER/ +// GRANT_CONFIG at a throwaway directory. Before this existed the suite +// wrote the developer's real ~/.grant/cache/session_timestamps.json on +// every run, because cache.CacheDir resolves through os.UserHomeDir and +// GRANT_CONFIG does not affect it. +// 2. Replaces bootstrapImpl, so no unit test can load the real SDK profile +// or unlock the real keyring. +// +// recordSessionTimestamp is deliberately NOT stubbed: leaving the real writer +// live is what proves the HOME redirect actually works. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(func() int { + bootstrapImpl = func() (sdkauth.IdsecAuth, *sdkmodels.IdsecProfile, error) { + return nil, nil, errTestBootstrapDisabled + } + resetBootstrapCache() + return m.Run() + })) +} + +// TestSandboxIsolation pins the redirect. If TestMain ever stops wrapping +// m.Run, this fails instead of the suite silently writing to the real home. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} + +// TestCacheDirResolvesInsideSandbox is the specific regression for the 25 +// writes to the developer's real ~/.grant/cache/session_timestamps.json. The +// chain is recordSessionTimestamp -> cache.CacheDir -> config.ConfigDir -> +// os.UserHomeDir, which GRANT_CONFIG does not influence at all. +// +// Not parallel: reads process-wide environment state. +func TestCacheDirResolvesInsideSandbox(t *testing.T) { + dir, err := cache.CacheDir() + if err != nil { + t.Fatalf("cache.CacheDir(): %v", err) + } + root := testenv.Root() + if root == "" { + t.Fatal("no testenv sandbox is active; TestMain is not wrapping m.Run") + } + if !strings.HasPrefix(dir, root) { + t.Errorf("cache.CacheDir() = %q, want a path under the sandbox root %q", dir, root) + } +} + +// TestBootstrapSCAServiceIsDisabledInUnitTests asserts the sentinel +// specifically. Asserting merely "some error" would let a cached bootstrap +// failure satisfy unrelated wantErr cases in other tables. +// +// Not parallel: mutates the package-global bootstrap memoization state. +func TestBootstrapSCAServiceIsDisabledInUnitTests(t *testing.T) { + resetBootstrapCache() + t.Cleanup(resetBootstrapCache) + + _, _, _, err := bootstrapSCAService() + if !errors.Is(err, errTestBootstrapDisabled) { + t.Fatalf("bootstrapSCAService() error = %v, want errTestBootstrapDisabled", err) + } +} diff --git a/internal/cache/main_test.go b/internal/cache/main_test.go new file mode 100644 index 0000000..24b08bc --- /dev/null +++ b/internal/cache/main_test.go @@ -0,0 +1,27 @@ +// This file is in the external test package (cache_test) on purpose: the +// testenv helper imports internal/cache, so an in-package test file importing +// testenv would be an import cycle. TestMain may live in either test package +// and still governs the whole test binary. +package cache_test + +import ( + "os" + "testing" + + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain redirects HOME and friends at a throwaway directory so tests in this +// package can never touch the developer's real ~/.grant cache. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(m.Run)) +} + +// TestSandboxIsolation pins the redirect: if TestMain ever stops wrapping +// m.Run, or a resolver starts reading a variable testenv does not override, +// this fails instead of silently writing to the developer's home directory. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} diff --git a/internal/config/main_test.go b/internal/config/main_test.go new file mode 100644 index 0000000..0ced660 --- /dev/null +++ b/internal/config/main_test.go @@ -0,0 +1,27 @@ +// This file is in the external test package (config_test) on purpose: the +// testenv helper imports internal/config, so an in-package test file importing +// testenv would be an import cycle. TestMain may live in either test package +// and still governs the whole test binary. +package config_test + +import ( + "os" + "testing" + + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain redirects HOME and friends at a throwaway directory so tests in this +// package can never touch the developer's real ~/.grant or ~/.idsec. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(m.Run)) +} + +// TestSandboxIsolation pins the redirect: if TestMain ever stops wrapping +// m.Run, or a resolver starts reading a variable testenv does not override, +// this fails instead of silently writing to the developer's home directory. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} From 7547940a093014dad881dd89be04a7b9a265058b Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 09:26:54 +0200 Subject: [PATCH 03/15] fix(favorites): fail fast without a terminal before authenticating --- cmd/favorites.go | 13 ++++++ cmd/favorites_test.go | 100 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 112 insertions(+), 1 deletion(-) diff --git a/cmd/favorites.go b/cmd/favorites.go index 8da7205..c5cbbbc 100644 --- a/cmd/favorites.go +++ b/cmd/favorites.go @@ -145,6 +145,19 @@ func runFavoritesAddProduction(cmd *cobra.Command, args []string) error { } } + // Fail fast without a terminal. This must stay AFTER the duplicate-name + // check above (moving it earlier makes a duplicate report "not + // interactive" instead of "already exists") and BEFORE the bootstrap + // below: everything past this point authenticates and calls the SCA + // eligibility API, which a scripted `grant favorites add myfav` used to do + // before ui.ErrNotInteractive finally surfaced from PromptName. + // + // The message is favorites-specific on purpose; earlyNonInteractiveCheck + // in request_picker.go would wrongly tell the user to supply a request ID. + if !ui.IsInteractive() { + return fmt.Errorf("%w; pass --target and --role, or --type groups with --group", ui.ErrNotInteractive) + } + // Bootstrap auth and SCA service _, scaService, _, err := bootstrapSCAService() if err != nil { diff --git a/cmd/favorites_test.go b/cmd/favorites_test.go index 6028f84..bb49e1a 100644 --- a/cmd/favorites_test.go +++ b/cmd/favorites_test.go @@ -316,7 +316,10 @@ func TestFavoritesAddCommand(t *testing.T) { _ = config.Save(cfg, path) }, args: []string{"dev"}, - wantErr: true, // Should fail with duplicate error + wantErr: true, + // A bare wantErr passes on ANY error — an auth failure or the + // non-interactive guard would both satisfy it. Name the message. + wantContain: []string{`favorite "dev" already exists`}, }, { name: "success with target and role flags", @@ -1252,3 +1255,98 @@ func TestSurveyNamePrompter_NonTTY(t *testing.T) { t.Errorf("error should suggest providing name as argument, got: %v", err) } } + +// setTTY forces ui.IsInteractive() to the given value for the duration of the +// test. +// +// Not parallel: mutates the package-global ui.IsTerminalFunc. +func setTTY(t *testing.T, interactive bool) { + t.Helper() + original := ui.IsTerminalFunc + t.Cleanup(func() { ui.IsTerminalFunc = original }) + ui.IsTerminalFunc = func(fd uintptr) bool { return interactive } +} + +// TestFavoritesAdd_NonInteractiveGuard pins the BUG that `grant favorites add +// myfav` in a script used to authenticate and call the SCA eligibility API +// before ui.ErrNotInteractive finally surfaced from PromptName. The guard must +// fire before bootstrapSCAService, and must sit AFTER the duplicate-name check +// so a duplicate still reports "already exists". +// +// Not parallel: mutates package-global TTY and bootstrap state. +func TestFavoritesAdd_NonInteractiveGuard(t *testing.T) { + tests := []struct { + name string + // interactive drives ui.IsInteractive(). + interactive bool + setupConfig func(string) + args []string + wantNotInteract bool // error must wrap ui.ErrNotInteractive + wantContain []string // substrings required in the error text + }{ + { + name: "name given but no flags and no terminal", + setupConfig: func(path string) { _ = config.Save(config.DefaultConfig(), path) }, + args: []string{"myfav"}, + // The hint must name the favorites flags, not a request ID. + wantNotInteract: true, + wantContain: []string{"--target", "--role"}, + }, + { + name: "groups type without --group and no terminal", + setupConfig: func(path string) { _ = config.Save(config.DefaultConfig(), path) }, + args: []string{"myfav", "--type", "groups"}, + wantNotInteract: true, + wantContain: []string{"--group"}, + }, + { + name: "duplicate name still reports the duplicate, not the missing terminal", + setupConfig: func(path string) { + cfg := config.DefaultConfig() + _ = config.AddFavorite(cfg, "dev", config.Favorite{ + Provider: "azure", Target: "subscription-123", Role: "Contributor", + }) + _ = config.Save(cfg, path) + }, + args: []string{"dev"}, + wantContain: []string{`favorite "dev" already exists`}, + }, + { + name: "with a terminal the guard does not fire and bootstrap is reached", + interactive: true, + setupConfig: func(path string) { _ = config.Save(config.DefaultConfig(), path) }, + args: []string{"myfav"}, + // errTestBootstrapDisabled proves execution got past the guard. + wantContain: []string{"bootstrapImpl is disabled in unit tests"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + setTTY(t, tt.interactive) + resetBootstrapCache() + t.Cleanup(resetBootstrapCache) + + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + tt.setupConfig(configPath) + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommand()) + + _, err := executeCommand(rootCmd, append([]string{"favorites", "add"}, tt.args...)...) + if err == nil { + t.Fatal("expected an error, got nil") + } + if gotNotInteract := errors.Is(err, ui.ErrNotInteractive); gotNotInteract != tt.wantNotInteract { + t.Errorf("errors.Is(err, ui.ErrNotInteractive) = %v, want %v (err = %v)", + gotNotInteract, tt.wantNotInteract, err) + } + for _, want := range tt.wantContain { + if !strings.Contains(err.Error(), want) { + t.Errorf("error missing %q, got: %v", want, err) + } + } + }) + } +} From 1e3da63e7965f134dbbb661cedcae8d240bd780d Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 09:29:02 +0200 Subject: [PATCH 04/15] test(integration): isolate the harness and assert exact exit codes and error text --- cmd/bootstrap_stub_test.go | 32 ++++ cmd/integration_test.go | 346 ++++++++++++++++++++----------------- cmd/main_test.go | 14 +- 3 files changed, 225 insertions(+), 167 deletions(-) create mode 100644 cmd/bootstrap_stub_test.go diff --git a/cmd/bootstrap_stub_test.go b/cmd/bootstrap_stub_test.go new file mode 100644 index 0000000..c75f40b --- /dev/null +++ b/cmd/bootstrap_stub_test.go @@ -0,0 +1,32 @@ +package cmd + +import ( + "errors" + + sdkauth "github.com/cyberark/idsec-sdk-golang/pkg/auth" + sdkmodels "github.com/cyberark/idsec-sdk-golang/pkg/models" +) + +// This file carries NO build tag on purpose. Both TestMain implementations — +// the default one in main_test.go and the integration one in +// integration_test.go — install the same stub, so the in-process cmd tests +// behave identically under `go test ./cmd` and `go test -tags=integration +// ./cmd`. The integration tests themselves exercise a separate child process +// and are unaffected. + +// errTestBootstrapDisabled is returned by the stubbed bootstrapImpl. It is a +// named sentinel rather than an anonymous error so tests can assert on it with +// errors.Is: a bare `wantErr: true` would otherwise be satisfied by an +// accidental bootstrap attempt, hiding the fact that a unit test reached for +// real credentials. +var errTestBootstrapDisabled = errors.New("bootstrapImpl is disabled in unit tests; inject via New...WithDeps instead") + +// installBootstrapStub replaces the real profile-load + authenticate path so +// no in-process test can load the developer's SDK profile or unlock the real +// keyring. Call it from TestMain, before m.Run. +func installBootstrapStub() { + bootstrapImpl = func() (sdkauth.IdsecAuth, *sdkmodels.IdsecProfile, error) { + return nil, nil, errTestBootstrapDisabled + } + resetBootstrapCache() +} diff --git a/cmd/integration_test.go b/cmd/integration_test.go index ad96343..9fef25a 100644 --- a/cmd/integration_test.go +++ b/cmd/integration_test.go @@ -3,32 +3,131 @@ package cmd import ( + "errors" + "fmt" "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" + + "github.com/aaearon/grant-cli/internal/testenv" ) -// TestMain builds the binary before running integration tests +// testBinary is the compiled grant binary under test. It lives in a private +// temp directory with a unique name rather than the old shared ../grant-test, +// so two concurrent runs (or a stale artifact from a killed run) cannot +// interfere with each other or leave debris in the repo. +var testBinary string + +// goEnvPassthrough carries the Go tool's own directories into the sandboxed +// build. They are resolved BEFORE testenv.Run redirects HOME, because GOCACHE +// and GOMODCACHE default to locations under the user's home: without this the +// build inside the sandbox would start from an empty module cache and need the +// network. +var goEnvPassthrough []string + func TestMain(m *testing.M) { - // Build the binary - cmd := exec.Command("go", "build", "-o", "../grant-test", "../.") - if err := cmd.Run(); err != nil { - panic("Failed to build binary for integration tests: " + err.Error()) + goEnvPassthrough = resolveGoEnv("GOCACHE", "GOMODCACHE", "GOPATH") + + os.Exit(testenv.Run(func() int { + dir, err := os.MkdirTemp("", "grant-integration-bin-") + if err != nil { + fmt.Fprintf(os.Stderr, "integration: failed to create binary dir: %v\n", err) + return 1 + } + defer func() { _ = os.RemoveAll(dir) }() + + name := "grant-integration-test" + if runtime.GOOS == "windows" { + name += ".exe" + } + testBinary = filepath.Join(dir, name) + + build := exec.Command("go", "build", "-o", testBinary, "..") + build.Env = append(os.Environ(), goEnvPassthrough...) + if out, err := build.CombinedOutput(); err != nil { + fmt.Fprintf(os.Stderr, "integration: failed to build binary: %v\n%s\n", err, out) + return 1 + } + + // The in-process cmd unit tests compile into this binary too; give + // them the same disabled bootstrap they get in the default build. + installBootstrapStub() + + return m.Run() + })) +} + +// resolveGoEnv returns KEY=VALUE strings for the named `go env` keys. +func resolveGoEnv(keys ...string) []string { + out, err := exec.Command("go", append([]string{"env"}, keys...)...).Output() + if err != nil { + return nil + } + lines := strings.Split(strings.ReplaceAll(string(out), "\r\n", "\n"), "\n") + env := make([]string, 0, len(keys)) + for i, k := range keys { + if i >= len(lines) || lines[i] == "" { + continue + } + env = append(env, k+"="+lines[i]) } + return env +} + +// result is the outcome of one child invocation. +type result struct { + output string + exitCode int +} + +// contains reports whether the combined output contains want. +func (r result) contains(want string) bool { return strings.Contains(r.output, want) } - // Run tests - code := m.Run() +// runGrant executes the built binary with the process's (already sandboxed) +// environment plus extra, with stdin closed so no interactive prompt can +// block. It returns the combined output and the real exit code. +func runGrant(t *testing.T, extraEnv []string, args ...string) result { + t.Helper() - // Clean up - os.Remove("../grant-test") + cmd := exec.Command(testBinary, args...) + cmd.Env = append(os.Environ(), extraEnv...) + cmd.Stdin = nil // closed: never a TTY, and never blocks on a prompt - os.Exit(code) + out, err := cmd.CombinedOutput() + + code := 0 + if err != nil { + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + t.Fatalf("running %v: %v\noutput:\n%s", args, err, out) + } + code = exitErr.ExitCode() + } + + r := result{output: string(out), exitCode: code} + + // A panic satisfies almost any keyword-based assertion, which is exactly + // how the previous version of these tests could pass on a crash. + if r.contains("panic:") || r.contains("goroutine 1 [running]:") { + t.Fatalf("binary panicked on %v (exit %d):\n%s", args, code, out) + } + return r } -func getBinaryPath() string { - return filepath.Join("..", "grant-test") +// isolatedEnv points config and credentials at a fresh per-test directory on +// top of the process-wide testenv sandbox. +func isolatedEnv(t *testing.T) []string { + t.Helper() + dir := t.TempDir() + return []string{ + "HOME=" + dir, + "USERPROFILE=" + dir, + "GRANT_CONFIG=" + filepath.Join(dir, "config.yaml"), + "IDSEC_PROFILES_FOLDER=" + filepath.Join(dir, "profiles"), + } } func TestIntegration_Help(t *testing.T) { @@ -66,16 +165,14 @@ func TestIntegration_Help(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd := exec.Command(getBinaryPath(), tt.args...) - output, err := cmd.CombinedOutput() - if err != nil && !strings.Contains(string(output), "Usage:") { - t.Fatalf("Command failed: %v\nOutput: %s", err, output) + got := runGrant(t, isolatedEnv(t), tt.args...) + // Help is a success path: exit 0, exactly. + if got.exitCode != 0 { + t.Fatalf("exit code = %d, want 0\noutput:\n%s", got.exitCode, got.output) } - - outputStr := string(output) for _, want := range tt.wantText { - if !strings.Contains(outputStr, want) { - t.Errorf("Expected output to contain %q, got:\n%s", want, outputStr) + if !got.contains(want) { + t.Errorf("output missing %q, got:\n%s", want, got.output) } } }) @@ -83,184 +180,125 @@ func TestIntegration_Help(t *testing.T) { } func TestIntegration_Version(t *testing.T) { - cmd := exec.Command(getBinaryPath(), "version") - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("Version command failed: %v\nOutput: %s", err, output) + got := runGrant(t, isolatedEnv(t), "version") + if got.exitCode != 0 { + t.Fatalf("exit code = %d, want 0\noutput:\n%s", got.exitCode, got.output) } - outputStr := string(output) - requiredFields := []string{"grant version", "commit:", "built:"} - - for _, field := range requiredFields { - if !strings.Contains(outputStr, field) { - t.Errorf("Expected output to contain %q, got:\n%s", field, outputStr) + for _, field := range []string{"grant version", "commit:", "built:"} { + if !got.contains(field) { + t.Errorf("output missing %q, got:\n%s", field, got.output) } } - - // Should contain at least one of the default values (dev build) - if !strings.Contains(outputStr, "dev") && !strings.Contains(outputStr, "unknown") { - t.Errorf("Expected version output to show dev build info, got:\n%s", outputStr) + // The integration binary is built without -ldflags, so the version stays + // at its compiled-in default. + if !got.contains("dev") && !got.contains("unknown") { + t.Errorf("expected a dev build banner, got:\n%s", got.output) } } func TestIntegration_ElevateWithoutLogin(t *testing.T) { - // Set a temporary config path to avoid interfering with real config - tempDir := t.TempDir() - - cmd := exec.Command(getBinaryPath(), "--provider", "azure") - cmd.Env = append(os.Environ(), "GRANT_CONFIG="+filepath.Join(tempDir, "config.yaml")) - cmd.Env = append(cmd.Env, "HOME="+tempDir) // Isolate from real credentials + got := runGrant(t, isolatedEnv(t), "--provider", "azure") - output, err := cmd.CombinedOutput() - - // Command should fail when not authenticated - if err == nil { - t.Errorf("Expected elevation to fail without authentication, but it succeeded.\nOutput: %s", output) + if got.exitCode != 1 { + t.Fatalf("exit code = %d, want 1\noutput:\n%s", got.exitCode, got.output) } - - outputStr := string(output) - - // Should contain an error message about authentication or configuration - errorKeywords := []string{"error", "Error", "failed", "Failed", "not found", "authenticate"} - foundError := false - for _, keyword := range errorKeywords { - if strings.Contains(outputStr, keyword) { - foundError = true - break + // Exact text, not a keyword soup. With an empty sandbox profile directory + // the SDK authenticator refuses before any network call, and the non- + // verbose hint is part of the contract. + for _, want := range []string{ + "authentication failed: either a profile or a specific auth profile must be supplied", + "Hint: re-run with --verbose for more details", + } { + if !got.contains(want) { + t.Errorf("output missing %q, got:\n%s", want, got.output) } } - - if !foundError { - t.Errorf("Expected error output when not authenticated, got:\n%s", outputStr) - } } func TestIntegration_StatusWithoutLogin(t *testing.T) { - // Set a temporary config path to avoid interfering with real config - tempDir := t.TempDir() - - cmd := exec.Command(getBinaryPath(), "status") - cmd.Env = append(os.Environ(), "GRANT_CONFIG="+filepath.Join(tempDir, "config.yaml")) - cmd.Env = append(cmd.Env, "HOME="+tempDir) // Isolate from real credentials + got := runGrant(t, isolatedEnv(t), "status") - output, err := cmd.CombinedOutput() - - // Status command should run but show not authenticated - outputStr := string(output) - - // Should indicate not authenticated state - if !strings.Contains(outputStr, "Not authenticated") && !strings.Contains(outputStr, "not authenticated") { - // If it doesn't explicitly say "not authenticated", it should at least not show a username - if strings.Contains(outputStr, "Username:") && err == nil { - t.Errorf("Expected status to show 'not authenticated', but it showed a username.\nOutput: %s", outputStr) - } + if got.exitCode != 1 { + t.Fatalf("exit code = %d, want 1\noutput:\n%s", got.exitCode, got.output) + } + if want := "authentication failed: either a profile or a specific auth profile must be supplied"; !got.contains(want) { + t.Errorf("output missing %q, got:\n%s", want, got.output) + } + // It must never claim an identity it does not have. + if got.contains("Username:") { + t.Errorf("status printed a username without authentication:\n%s", got.output) } } func TestIntegration_FavoritesList(t *testing.T) { - // Set a temporary config path to avoid interfering with real config - tempDir := t.TempDir() - - cmd := exec.Command(getBinaryPath(), "favorites", "list") - cmd.Env = append(os.Environ(), "GRANT_CONFIG="+filepath.Join(tempDir, "config.yaml")) + got := runGrant(t, isolatedEnv(t), "favorites", "list") - output, err := cmd.CombinedOutput() - - // Command should succeed (empty list is valid) - if err != nil && !strings.Contains(string(output), "No favorites") { - t.Fatalf("Favorites list command failed unexpectedly: %v\nOutput: %s", err, output) + if got.exitCode != 0 { + t.Fatalf("exit code = %d, want 0 (an empty favorites list is a success)\noutput:\n%s", + got.exitCode, got.output) } - - outputStr := string(output) - - // Should either show "No favorites" or be empty - if !strings.Contains(outputStr, "No favorites") && strings.TrimSpace(outputStr) != "" { - // If it's not empty and doesn't say "No favorites", it should at least be valid output - t.Logf("Favorites list output: %s", outputStr) + if !got.contains("No favorites") { + t.Errorf("output missing %q, got:\n%s", "No favorites", got.output) } } func TestIntegration_FavoritesAddWithFlags(t *testing.T) { - tempDir := t.TempDir() - configPath := filepath.Join(tempDir, "config.yaml") - env := append(os.Environ(), "GRANT_CONFIG="+configPath) - - // Add a favorite via flags - cmd := exec.Command(getBinaryPath(), "favorites", "add", "test-fav", "--target", "sub-123", "--role", "Contributor") - cmd.Env = env - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("favorites add with flags failed: %v\nOutput: %s", err, output) - } + env := isolatedEnv(t) - outputStr := string(output) - if !strings.Contains(outputStr, "Added favorite") { - t.Errorf("expected output to contain 'Added favorite', got:\n%s", outputStr) + added := runGrant(t, env, "favorites", "add", "test-fav", "--target", "sub-123", "--role", "Contributor") + if added.exitCode != 0 { + t.Fatalf("favorites add exit code = %d, want 0\noutput:\n%s", added.exitCode, added.output) } - - // Verify via favorites list - cmd = exec.Command(getBinaryPath(), "favorites", "list") - cmd.Env = env - output, err = cmd.CombinedOutput() - if err != nil { - t.Fatalf("favorites list failed: %v\nOutput: %s", err, output) + if !added.contains("Added favorite") { + t.Errorf("output missing %q, got:\n%s", "Added favorite", added.output) } - outputStr = string(output) + listed := runGrant(t, env, "favorites", "list") + if listed.exitCode != 0 { + t.Fatalf("favorites list exit code = %d, want 0\noutput:\n%s", listed.exitCode, listed.output) + } for _, want := range []string{"test-fav", "azure/sub-123/Contributor"} { - if !strings.Contains(outputStr, want) { - t.Errorf("favorites list missing %q, got:\n%s", want, outputStr) + if !listed.contains(want) { + t.Errorf("favorites list missing %q, got:\n%s", want, listed.output) } } } -func TestIntegration_FavoritesAddInteractiveRequiresAuth(t *testing.T) { - tempDir := t.TempDir() - - cmd := exec.Command(getBinaryPath(), "favorites", "add", "test-fav") - cmd.Env = append(os.Environ(), "GRANT_CONFIG="+filepath.Join(tempDir, "config.yaml")) - cmd.Env = append(cmd.Env, "HOME="+tempDir) // Isolate from real credentials - - output, err := cmd.CombinedOutput() +// TestIntegration_FavoritesAddWithoutTTYFailsBeforeAuth is the end-to-end +// counterpart of TestFavoritesAdd_NonInteractiveGuard: with stdin closed the +// command must refuse immediately with the non-interactive hint, and must NOT +// reach the profile load / authentication it used to attempt first. +func TestIntegration_FavoritesAddWithoutTTYFailsBeforeAuth(t *testing.T) { + got := runGrant(t, isolatedEnv(t), "favorites", "add", "test-fav") - // Should fail — interactive mode needs auth - if err == nil { - t.Errorf("Expected favorites add interactive to fail without auth, but it succeeded.\nOutput: %s", output) + if got.exitCode != 1 { + t.Fatalf("exit code = %d, want 1\noutput:\n%s", got.exitCode, got.output) } - - outputStr := string(output) - - // Should contain an auth-related error (profile not found, auth failed, etc.) - errorKeywords := []string{"error", "Error", "failed", "Failed", "not found", "profile"} - foundError := false - for _, keyword := range errorKeywords { - if strings.Contains(outputStr, keyword) { - foundError = true - break + for _, want := range []string{"interactive selection requires a terminal", "--target", "--role"} { + if !got.contains(want) { + t.Errorf("output missing %q, got:\n%s", want, got.output) } } - - if !foundError { - t.Errorf("Expected auth-related error for interactive favorites add, got:\n%s", outputStr) + if got.contains("failed to load profile") || got.contains("authentication failed") { + t.Errorf("favorites add reached authentication before the non-interactive guard:\n%s", got.output) } } func TestIntegration_InvalidCommand(t *testing.T) { - cmd := exec.Command(getBinaryPath(), "nonexistent-command") - output, err := cmd.CombinedOutput() + got := runGrant(t, isolatedEnv(t), "nonexistent-command") - // Should fail for invalid command - if err == nil { - t.Errorf("Expected invalid command to fail, but it succeeded.\nOutput: %s", output) + if got.exitCode != 1 { + t.Fatalf("exit code = %d, want 1\noutput:\n%s", got.exitCode, got.output) } - - outputStr := string(output) - - // Should show an error or help message - if !strings.Contains(outputStr, "unknown command") && - !strings.Contains(outputStr, "Error:") && - !strings.Contains(outputStr, "Usage:") { - t.Errorf("Expected error for invalid command, got:\n%s", outputStr) + if want := `unknown command "nonexistent-command" for "grant"`; !got.contains(want) { + t.Errorf("output missing %q, got:\n%s", want, got.output) } } + +// TestIntegration_SandboxIsolation asserts the harness itself is sandboxed, so +// a regression in TestMain surfaces here rather than as writes to the +// developer's real home directory. +func TestIntegration_SandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} diff --git a/cmd/main_test.go b/cmd/main_test.go index a2d8aa5..7f0192c 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -13,17 +13,8 @@ import ( "github.com/aaearon/grant-cli/internal/cache" "github.com/aaearon/grant-cli/internal/testenv" - sdkauth "github.com/cyberark/idsec-sdk-golang/pkg/auth" - sdkmodels "github.com/cyberark/idsec-sdk-golang/pkg/models" ) -// errTestBootstrapDisabled is returned by the stubbed bootstrapImpl. It is a -// named sentinel rather than an anonymous error so tests can assert on it with -// errors.Is: a bare `wantErr: true` would otherwise be satisfied by an -// accidental bootstrap attempt, hiding the fact that a unit test reached for -// real credentials. -var errTestBootstrapDisabled = errors.New("bootstrapImpl is disabled in unit tests; inject via New...WithDeps instead") - // TestMain does two things, in this order: // // 1. Redirects HOME/USERPROFILE/XDG_CONFIG_HOME/IDSEC_PROFILES_FOLDER/ @@ -38,10 +29,7 @@ var errTestBootstrapDisabled = errors.New("bootstrapImpl is disabled in unit tes // live is what proves the HOME redirect actually works. func TestMain(m *testing.M) { os.Exit(testenv.Run(func() int { - bootstrapImpl = func() (sdkauth.IdsecAuth, *sdkmodels.IdsecProfile, error) { - return nil, nil, errTestBootstrapDisabled - } - resetBootstrapCache() + installBootstrapStub() return m.Run() })) } From 3cf90d7f98fbc3cbdb2abf0324d65e82b53a267a Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 09:31:33 +0200 Subject: [PATCH 05/15] ci: lint tagged files, run integration and shuffled test passes on both legs --- .github/workflows/ci.yml | 14 ++++++++++++++ .golangci.yml | 7 +++++++ cmd/test_helpers.go | 14 ++++++++++++++ internal/testenv/testenv_test.go | 5 +++++ 4 files changed, 40 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e2f1963..9b04902 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,6 +42,20 @@ jobs: if: runner.os == 'Windows' run: go test -race ./... -v + # Integration tests drive the compiled binary as a child process. They + # need no network and take ~2s, and they are the only place the real + # argument parsing, exit codes and error text are exercised end to end. + # Unguarded so both platforms are covered; the harness builds its own + # binary into a temp dir. + - name: Integration tests + run: go test -tags=integration ./cmd -count=1 -v + + # The suite mutates package globals (bootstrapImpl, log, + # ui.IsTerminalFunc). Randomising order is what surfaces a test that only + # passes because another test ran first. + - name: Test with shuffled order + run: go test -shuffle=on -count=1 ./... + # Runs on BOTH legs so the platforms stay comparable. This is the only # test that replaces a real, running executable, which is where Windows # and POSIX genuinely differ. It needs no network: the fixture binaries diff --git a/.golangci.yml b/.golangci.yml index 0dd4ee9..edde26b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -5,6 +5,13 @@ run: timeout: 3m tests: true + # Without these, cmd/integration_test.go and internal/selfupdate/e2e_test.go + # are invisible to every linter. Note the effect on cmd/: with `integration` + # set, cmd/main_test.go (//go:build !integration) is excluded instead, so the + # two tagged files are linted at the cost of that one. + build-tags: + - integration + - selfupdate_e2e linters: disable-all: true diff --git a/cmd/test_helpers.go b/cmd/test_helpers.go index 4f47512..2da550b 100644 --- a/cmd/test_helpers.go +++ b/cmd/test_helpers.go @@ -25,6 +25,8 @@ func newNoOpCommand() *cobra.Command { // When SilenceErrors is true, error text is appended to the output buffer // to match production behavior (where Execute() prints the error). func executeCommand(cmd *cobra.Command, args ...string) (string, error) { + defer restoreOutputFormat(outputFormat) + buf := new(bytes.Buffer) cmd.SetOut(buf) cmd.SetErr(buf) @@ -37,12 +39,22 @@ func executeCommand(cmd *cobra.Command, args ...string) (string, error) { return buf.String(), err } +// restoreOutputFormat puts the package-global outputFormat back. Cobra binds +// --output to that global with StringVarP, so executing any command that +// carries the flag leaves the global set for every later test. A command built +// without the root flag set (NewEnvCommandWithDeps, for instance) then inherits +// "json" from whichever test ran before it — an order dependence that only +// shows up under `go test -shuffle=on`. +func restoreOutputFormat(saved string) { outputFormat = saved } + // executeCommandStreams executes a command keeping stdout and stderr apart and // writing no error text into either. Use it when a test needs to assert on the // exact stdout payload (e.g. valid JSON) *and* on a returned error, which // executeCommand cannot express because it merges the streams and appends the // error text. func executeCommandStreams(cmd *cobra.Command, args ...string) (stdout, stderr string, err error) { + defer restoreOutputFormat(outputFormat) + var outBuf, errBuf bytes.Buffer cmd.SetOut(&outBuf) cmd.SetErr(&errBuf) @@ -55,6 +67,8 @@ func executeCommandStreams(cmd *cobra.Command, args ...string) (stdout, stderr s // executeWithHint simulates Execute() logic without os.Exit, returning the error output. // Used for testing the verbose hint behavior. func executeWithHint(cmd *cobra.Command, args []string) string { + defer restoreOutputFormat(outputFormat) + passedArgValidation = false cmd.SetArgs(args) err := cmd.Execute() diff --git a/internal/testenv/testenv_test.go b/internal/testenv/testenv_test.go index 45337d6..81d1562 100644 --- a/internal/testenv/testenv_test.go +++ b/internal/testenv/testenv_test.go @@ -166,6 +166,11 @@ func TestAssertSandboxed_FailsWhenAResolverEscapes(t *testing.T) { escapee := t.TempDir() // deliberately NOT under the sandbox root Run(func() int { + // os.Setenv, not t.Setenv: t.Setenv's cleanup fires when the test + // ends, which is *after* Run has already restored the environment — + // it would put this bogus value back and leak it into later tests. + // Run's own restore covers us here. + //nolint:usetesting // see comment above if err := os.Setenv("IDSEC_PROFILES_FOLDER", escapee); err != nil { t.Errorf("Setenv: %v", err) return 1 From 608b9f88c8aceea3f6f8c60459e392604c59bed3 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 09:32:31 +0200 Subject: [PATCH 06/15] docs: correct the DI seams and document test isolation --- CHANGELOG.md | 4 +++ CLAUDE.md | 69 +++++++++++++++++++++++----------------------------- 2 files changed, 35 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6d6ee1..31b56da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Fixed + +- `grant favorites add` now fails immediately without a terminal instead of authenticating first + ## [0.9.0] - 2026-08-14 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 3487905..19aea1d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,7 +85,7 @@ Custom `SCAAccessService` follows SDK conventions: - `httptest.NewServer` for service mocks - `httpClient` interface for DI - Test files co-located as `_test.go` -- Tests that swap a package-level var (e.g. `ui.IsTerminalFunc`, `recordSessionTimestamp`, `getAuth`) MUST NOT call `t.Parallel()` — `-race` flags concurrent access to the global. Mark them with a `// Not parallel: mutates the package-global X.` comment. This is why the `cmd` package tests are all serial. +- Tests that swap a package-level var (e.g. `ui.IsTerminalFunc`, `recordSessionTimestamp`, `bootstrapImpl`) MUST NOT call `t.Parallel()` — `-race` flags concurrent access to the global. Mark them with a `// Not parallel: mutates the package-global X.` comment. This is why the `cmd` package tests are all serial. ## CLI - `spf13/cobra` for CLI framework @@ -217,6 +217,8 @@ make clean # Clean build artifacts - Windows runners have no GNU make, so that leg runs the equivalent Go commands directly (`go build -trimpath -o grant.exe .`, `go test -race ./... -v`); Linux keeps `make build` / `make test-race`. Keep the two legs in sync when Makefile targets change - `go test -race` works on windows/amd64 because the runner image ships gcc (the race detector needs cgo) - The `Self-update end-to-end` step runs the `selfupdate_e2e`-tagged tests on **both** legs (no `if:` guard) — it is the only test that replaces a real running executable, and comparing the two platforms is the whole point. It builds its fixtures locally, so it needs no network. Keep it unguarded; guarding it to Linux would defeat its purpose +- `Integration tests` (`go test -tags=integration ./cmd`) and `Test with shuffled order` (`go test -shuffle=on -count=1 ./...`) run **unguarded on both legs**. Integration needs no network and takes ~2s; shuffling is what catches order dependence in a suite that mutates package globals +- `.golangci.yml` sets `run.build-tags: [integration, selfupdate_e2e]` so both tagged files are linted. Side effect: with `integration` set, `cmd/main_test.go` (`//go:build !integration`) is excluded from linting - Lint (`golangci-lint-action`) runs on Linux only — a second pass on Windows adds minutes and finds nothing new - Tests must be OS-portable. Never assert POSIX permission bits without a `runtime.GOOS == "windows"` skip: Go synthesizes `0666`/`0777` for Windows files and `os.Chmod` there only toggles the read-only attribute. Current skips: `internal/config/config_test.go` (`TestLoadConfig_PermissionError`, `TestConfigDir_Error` — chmod 0000 and `HOME`) and `internal/cache/cache_test.go` (`TestSet_FilePermissions`) - Prefer a portable construction over a skip where one exists. To force a write failure, point at a path whose parent component is an existing regular file (`MkdirAll` fails with ENOTDIR on POSIX and ERROR_DIRECTORY on Windows) rather than a hardcoded `/dev/null/...` path, which is an ordinary writable location on Windows @@ -274,35 +276,35 @@ func init() { ### Dependency Injection -Commands use interfaces for testability: +Commands declare their collaborators as interfaces in `cmd/interfaces.go` (`authLoader`, `eligibilityLister`, `groupsEligibilityLister`, `elevateService`, `accessRequestService`, `selfUpdater`, …). There are two ways to substitute them. -```go -// interfaces.go -type authProvider interface { - Authenticate(profile *models.IdsecProfile) (*models.IdsecToken, error) -} +**Preferred — `New*WithDeps` constructors.** Every command has one: `NewRootCommandWithDeps`, `NewEnvCommandWithDeps`, `NewStatusCommandWithDeps`, `NewRevokeCommandWithDeps`, `NewListCommandWithDeps`, `NewRequestCommandWithDeps`, `NewLogoutCommandWithDeps`, `NewUpdateCommandWithDeps`, plus `runFavoritesAddWithDeps`. The production factory is a thin wrapper that bootstraps the real services and calls the same function. Tests build the command with mocks and never touch a global. -type scaService interface { - ListEligibility(ctx context.Context, csp models.CSP) (*models.EligibilityResponse, error) - Elevate(ctx context.Context, req *models.ElevateRequest) (*models.ElevateResponse, error) -} +**Package-var seams**, for the few things a constructor cannot reach: -// Command runtime resolution -var ( - getAuth = func() (authProvider, error) { /* ... */ } - getSCAService = func() (scaService, error) { /* ... */ } -) +| Var | File | Purpose | +|---|---|---| +| `bootstrapImpl` | `cmd/root.go` | Profile load + authenticate. Memoized by `bootstrapISPAuth` via `sync.Once`; clear it with `resetBootstrapCache()` | +| `recordSessionTimestamp` | `cmd/session_tracking.go` | Elevation timestamp writer | +| `resolveRequestIDFn` | `cmd/request_picker.go` | Interactive request picker | +| `submitPromptFn`, `confirmSubmitFn`, `resolveSubmitTargetFn`, `submitWorkspaceSelectorFn`, `resolveRoleFn` | `cmd/request_submit.go` | `request submit` prompt and resolution steps | +| `log` | `cmd/verbose.go` | Verbose logger; tests swap in `spyLogger` | +| `ui.IsTerminalFunc` | `internal/ui/tty.go` | TTY detection | -// Test injection via package vars -func TestMyCommand(t *testing.T) { - originalGetAuth := getAuth - defer func() { getAuth = originalGetAuth }() +There is no `getAuth`/`getSCAService`; those never existed. Every test that swaps one of these globals restores it via `t.Cleanup`, and no such test may call `t.Parallel()`. - getAuth = func() (authProvider, error) { - return &mockAuth{}, nil - } -} -``` +### Test Isolation + +`internal/testenv` is a normal package imported only from `_test.go` files, so it never links into the binary. `testenv.Run(m.Run)` redirects `HOME`, `USERPROFILE`, `XDG_CONFIG_HOME`, `IDSEC_PROFILES_FOLDER` and `GRANT_CONFIG` under one temp root before `m.Run`, then restores them. + +- `USERPROFILE` is not optional: Go's Windows `os.UserHomeDir` reads `USERPROFILE`, then `HOMEDRIVE`+`HOMEPATH`, and never `HOME`. `HOME` alone leaves the Windows CI leg pointed at the real profile. The SDK profile loader, conversely, reads `HOME` on every platform. +- `GRANT_CONFIG` does **not** cover the cache: `cache.CacheDir()` → `config.ConfigDir()` → `os.UserHomeDir()`. Before this existed the suite wrote the developer's real `~/.grant/cache/session_timestamps.json` on every run. +- `testenv` must not import `testing`; `AssertSandboxed` therefore takes a `TB` interface (`Helper`/`Errorf`) that `*testing.T` satisfies. +- `AssertSandboxed` checks the *configured destinations* — `config.ConfigDir`, `config.ConfigPath`, `cache.CacheDir`, `profiles.GetProfilesFolder`. It does not prove nothing was written outside the sandbox, and cannot see reads. A snapshot-diff gate was considered and rejected: a concurrently running real `grant` false-positives with certainty, and size+mtime misses same-size rewrites. +- `TestMain` lives in `cmd/main_test.go` (`//go:build !integration`, because `cmd/integration_test.go` declares its own), `internal/config/main_test.go` and `internal/cache/main_test.go`. The last two are in the **external** test package (`config_test`/`cache_test`) because `testenv` imports those packages — an in-package test file importing it would be an import cycle. +- `os.Setenv` in `TestMain` runs before `m.Run`, so it does not collide with the `t.Parallel()` sites in `internal/` — unlike `t.Setenv`, which the stdlib forbids in parallel tests. +- `cmd/bootstrap_stub_test.go` (untagged, so both build configurations get it) points `bootstrapImpl` at `errTestBootstrapDisabled`. Assert it with `errors.Is`, never a bare `wantErr: true` — otherwise a stray bootstrap attempt silently satisfies an unrelated case. `recordSessionTimestamp` is deliberately left live: it is what proves the redirect works. +- `executeCommand`/`executeCommandStreams`/`executeWithHint` restore the package-global `outputFormat`, which Cobra binds to `--output`. Without that a command built outside the root (e.g. `NewEnvCommandWithDeps`) inherits `json` from a previous test — an order dependence `go test -shuffle=on` exposes. ### Testing Patterns @@ -344,19 +346,10 @@ func (m *mockAuthProvider) Authenticate(p *models.IdsecProfile) (*models.IdsecTo ``` #### Integration Tests -```go -//go:build integration - -// integration_test.go - tests compiled binary -func TestMain(m *testing.M) { - // Build binary before tests - cmd := exec.Command("go", "build", "-o", "../grant-test", "../.") - cmd.Run() - code := m.Run() - os.Remove("../grant-test") - os.Exit(code) -} -``` + +`cmd/integration_test.go` (`//go:build integration`) drives the compiled binary as a child process. Its `TestMain` runs inside `testenv.Run` and builds into a unique temp directory — never the shared `../grant-test`, which two concurrent runs would fight over. `GOCACHE`/`GOMODCACHE`/`GOPATH` are resolved **before** the `HOME` redirect and passed to the build, otherwise it starts from an empty module cache and needs the network. + +Assertions are exact exit codes plus exact error text. Keyword soup (`error|Error|failed|not found`) is banned here: a panic satisfies it. `runGrant` fails the test outright if the child output contains a panic, and closes stdin so no prompt can block. ### Error Handling From 0d6395057cf3b3992cf82b62c6406d9c7326af8b Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 09:53:44 +0200 Subject: [PATCH 07/15] docs: add the mutation ledger as the remediation definition of done --- docs/mutation-ledger.md | 279 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 279 insertions(+) create mode 100644 docs/mutation-ledger.md diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md new file mode 100644 index 0000000..fe92bc9 --- /dev/null +++ b/docs/mutation-ledger.md @@ -0,0 +1,279 @@ +# Mutation ledger + +This is the definition of done for the test-remediation effort. Every row is one +mutation that a five-part adversarial mutation audit applied to production code +and that the test suite failed to notice — plus the rows the verifiers **refuted**, +which are recorded here precisely so nobody re-opens a settled question. + +**How a row is closed.** + +1. Apply the mutation in the **Mutation** cell verbatim at the **File:line** shown. +2. Run the test named in **Planned test** and watch it **fail**. +3. Revert the mutation. +4. Run the same test and watch it **pass**. +5. Always `-count=1`. A cached success is not evidence. + +Then flip **Status** to `done`. **A PR is not complete until every one of its rows +is `done`.** `wont-fix` and `refuted` rows are closed by review, not by a test; +mark them `done` when the PR that owns them has landed the comment / rationale. + +**Line numbers were re-verified against the current tree** (`main`, post-`f14e2b9`). +Several drifted from the source reports; corrections are noted in the Mutation cell +with `(was ...)`. **File:line is always the production site, never the test site.** + +**Verdicts** are the verifiers' conclusions, not the original reports': +`CONFIRMED` (survivor reproduced), `OVERSTATED` (survivor real, stated consequence +weaker than claimed), `REFUTED` (the claim is false — the mutant dies, or the +premise does not hold). + +--- + +## Ledger + +| ID | Area | File:line | Mutation (exact, applyable) | Verdict | Disposition | Planned test | PR | Status | +|---|---|---|---|---|---|---|---|---| +| REQ-01 | cmd/request finalize | `cmd/request_finalize.go:100` | `svc.FinalizeRequest(ctx, requestID, decision, reason)` → `svc.FinalizeRequest(ctx, requestID, "APPROVED", reason)` | CONFIRMED | test | `TestRequestReject_SendsRejectedDecision` | PR4 | todo | +| REQ-02 | cmd/request cancel | `cmd/request_cancel.go:60` | `svc.CancelRequest(ctx, requestID, reason)` → `svc.CancelRequest(ctx, "WRONG-ID", reason)` | CONFIRMED | test | `TestRequestCancel_PassesRequestID` | PR4 | todo | +| REQ-03 | cmd/request get | `cmd/request_get.go:53` | `svc.GetRequest(ctx, requestID)` → `svc.GetRequest(ctx, "WRONG-ID")` | CONFIRMED | test | `TestRequestGet_PassesRequestID` | PR4 | todo | +| REQ-04 | cmd/request list | `cmd/request_list.go:93-98` | Swap the asc/desc branches: `order := "asc"` → `order := "desc"` and `order = "desc"` → `order = "asc"` | CONFIRMED | test | `TestRequestList_SortDirection` | PR4 | todo | +| REQ-05 | cmd/request list | `cmd/request_list.go:82` | `if role != "CREATOR" && role != "APPROVER" {` → `if false {` | CONFIRMED | test | `TestRequestList_RejectsInvalidRole` | PR4 | todo | +| REQ-06 | cmd/request list | `cmd/request_list.go:78` | `params.FreeText = v` → `params.FreeText = ""` | CONFIRMED | test | `TestRequestList_PassesFreeText` | PR4 | todo | +| REQ-07 | cmd/request list | `cmd/request_list.go:58` | Delete `filters = append(filters, fmt.Sprintf("(requestState eq %s)", upper))` | CONFIRMED | test | `TestRequestList_PassesStateFilter` | PR4 | todo | +| REQ-08 | cmd/request submit | `cmd/request_submit.go:306` | `TargetCategory: "CLOUD_CONSOLE"` → `TargetCategory: "WRONG"` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | todo | +| REQ-09 | cmd/request submit | `cmd/request_submit.go:507` | `"workspaceId": ws.WorkspaceID,` → `"workspaceId": "",` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | todo | +| REQ-10 | cmd/request submit | `cmd/request_submit.go:511-512` | Swap the two values: `"timeFrom": f.timeTo,` / `"timeTo": f.timeFrom,` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | todo | +| REQ-11 | cmd/request submit | `cmd/request_submit.go:274` | Delete the `if err := validateSubmitFields(fields); err != nil { return err }` call site | CONFIRMED | test | `TestRunRequestSubmit_InvokesValidation` | PR4 | todo | +| REQ-12 | cmd/request submit | `cmd/request_submit.go:631` | `return errors.New("--date is required")` → `return errors.New("WRONG ERROR MESSAGE")` | CONFIRMED | test | `TestValidateSubmitFields_ErrorMessages` | PR4 | todo | +| REQ-13 | cmd/request get | `cmd/request_picker.go:15` (reached from `cmd/request_get.go`) | In the `get` path, disable the guard: `if requestID == "" && !ui.IsInteractive() {` → `if false {`, keeping `_ = requestID` so it compiles | CONFIRMED | test | `TestRequestGet_NonInteractiveRequiresID` | PR4 | todo | +| REQ-14 | cmd/request approve | `cmd/request_finalize.go:16` | Disable the early non-interactive guard for `approve` (`if false {`), preserving `_ = requestID` — a literal deletion does not compile (`declared and not used: requestID`) | CONFIRMED | test | `TestRequestApprove_NonInteractiveRequiresID` | PR4 | todo | +| REQ-15 | cmd/request reject | `cmd/request_finalize.go:16` | Same as REQ-14 for the `reject` path, with `_ = requestID` | CONFIRMED | test | `TestRequestReject_NonInteractiveRequiresID` | PR4 | todo | +| REQ-16 | cmd/request submit | `cmd/request_submit.go:377` | `if ws.CSP == models.CSPGCP {` → `if false {` inside `rejectGCPWorkspace`. The fixture sets both `WorkspaceType: WorkspaceTypeProject` and `CSP: CSPGCP`, so the workspace-type switch masks loss of the CSP arm | CONFIRMED | test | `TestRejectGCPWorkspace_CSPTagOnly` | PR4 | todo | +| REQ-17 | cmd/request output (text) | `cmd/request.go:79-80` | Swap the table values: `r.DetailString("workspaceName")` / `r.DetailString("roleName")` | CONFIRMED | test | `TestRequestList_TextFieldMapping` | PR5 | todo | +| REQ-18 | cmd/request output (text) | `cmd/request.go:125` | `fmt.Fprintf(w, "Created By: %s\n", r.CreatedBy)` → source from `r.UpdatedBy` | CONFIRMED | test | `TestRequestGet_TextFieldMapping` | PR5 | todo | +| REQ-19 | cmd/request output (JSON) | `cmd/request.go:190-191` | Swap `TimeFrom: r.DetailString("timeFrom")` and `TimeTo: r.DetailString("timeTo")` | CONFIRMED | test | `TestRequestGetJSON_FieldMapping` (`assertJSONEqual`) | PR5 | todo | +| REQ-20 | cmd/login | `cmd/login.go:52` | `if profile == nil {` → `if false {` (auto-configure branch). Feature *is* implemented; `login_test.go` skips it with the factually wrong reason "Auto-configure not yet implemented" — delete the skip | CONFIRMED | test | `TestRunLogin_AutoConfiguresMissingProfile` | PR4 | todo | +| REQ-21 | cmd/login | `cmd/login.go:74` | `auth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, true)` → `..., true, false)` (swap `force`/`refreshAuth`) | CONFIRMED | test | `TestRunLogin_AuthenticateFlags` | PR4 | todo | +| REQ-22 | cmd/request submit | `cmd/request_submit.go:251` | `if !ui.IsInteractive() {` → `if false {` inside the `roleID == ""` branch | CONFIRMED | test | `TestRunRequestSubmit_NonInteractiveRequiresRoleID` | PR4 | todo | +| REQ-23 | cmd integration suite | `cmd/integration_test.go` (harness, not a production site) | No mutation. Claim was "integration tests are absent from CI **and** every assertion accepts a panic". First half confirmed (`.github/workflows/ci.yml` runs `make test-race` / `go test -race`, never `-tags=integration`); second half is too broad — only line 153 accepts any panic; lines 71/125/235 require specific output | OVERSTATED | test | `TestMain` isolation + exact exit-code/error-text assertions; add `-tags=integration` to both CI legs | PR1 | todo | +| OUT-01 | cmd/list flags | `cmd/list.go:73` | Delete `cmd.MarkFlagsMutuallyExclusive("groups", "provider")`. `TestListCommand_MutualExclusivity` passes today on the *unrelated* runtime error `no eligible targets or groups found` | CONFIRMED | test | `TestListCommand_MutualExclusivity` (assert Cobra's `[groups provider] were all set`) | PR5 | todo | +| OUT-02 | cmd/favorites (interactive) | `cmd/favorites.go:245` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `selectFavoriteInteractive` | CONFIRMED | test | `TestFavoritesAddInteractive_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | +| OUT-03 | cmd/favorites (group add) | `cmd/favorites.go:375` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `addGroupFavorite` | CONFIRMED | test | `TestAddGroupFavorite_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | +| OUT-04 | cmd/list | `cmd/list.go:135` | `if provider == "" {` → `if true {` (groups fetched and emitted even when `--provider` is set) | CONFIRMED | test | `TestListCommand_ProviderSuppressesGroups` | PR5 | todo | +| OUT-05 | cmd/status JSON | `cmd/status.go:212` | `Provider: strings.ToLower(string(s.CSP))` → `strings.ToUpper(string(s.CSP))` | CONFIRMED | test | `TestStatusJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| OUT-06 | cmd/status JSON | `cmd/status.go:213` | `WorkspaceID: s.WorkspaceID` → `WorkspaceID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | +| OUT-07 | cmd/status JSON | `cmd/status.go:214` | `Duration: s.SessionDuration` → `Duration: 0` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | +| OUT-08 | cmd/status JSON | `cmd/status.go:215` | `RoleID: s.RoleID` → `RoleID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | +| OUT-09 | cmd/status JSON | `cmd/status.go:217-219` | Delete the `if name, ok := data.nameMap[s.WorkspaceID]; ok { so.WorkspaceName = name }` block | CONFIRMED | test | `TestStatusJSON_ResolvesWorkspaceName` | PR5 | todo | +| OUT-10 | cmd/status JSON | `cmd/status.go:221` | `so.Type = "group"` → `so.Type = "cloud"` | CONFIRMED | test | `TestStatusJSON_GroupSessionType` | PR5 | todo | +| OUT-11 | cmd/list JSON | `cmd/list.go:164` | `WorkspaceID: t.WorkspaceID` → `WorkspaceID: t.OrganizationID` | CONFIRMED | test | `TestListJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| OUT-12 | cmd/list JSON | `cmd/list.go:165` | `WorkspaceType: strings.ToLower(string(t.WorkspaceType))` → `WorkspaceType: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | +| OUT-13 | cmd/list JSON | `cmd/list.go:167` | `RoleID: t.RoleInfo.ID` → `RoleID: ""`. `roleId` is the field an LLM/automation feeds straight back into `grant request submit --role-id`. Note the verifier's correction: `--target` resolves on the emitted **name**, not `workspaceId` | CONFIRMED | test | `TestListJSON_RoundTripsToRequestSubmit` | PR5 | todo | +| OUT-14 | cmd/list JSON | `cmd/list.go:175` | `GroupID: g.GroupID` → `GroupID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | +| OUT-15 | cmd/list JSON | `cmd/list.go:176` | `DirectoryID: g.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | +| OUT-16 | cmd/favorites JSON | `cmd/favorites.go:449` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| OUT-17 | cmd/favorites JSON | `cmd/favorites.go:451` | `Role: entry.Role` → `Role: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | +| OUT-18 | cmd/favorites JSON | `cmd/favorites.go:453` | `DirectoryID: entry.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | +| OUT-19 | cmd/favorites | `cmd/favorites.go:320` | `fav.Provider = cfg.DefaultProvider` → `fav.Provider = "azure"`. Every command test uses the azure default, so a non-default `DefaultProvider` (aws/gcp) is unpinned. Secondary, same defect class: `internal/config/favorites.go:21-22` independently defaults empty → `"azure"` | CONFIRMED | test | `TestFavoritesAdd_HonorsNonDefaultProvider` | PR5 | todo | +| OUT-20 | cmd/favorites | `cmd/favorites.go:186-192` | Delete the `--type groups` / `--target`+`--role` pairing validation from `parseFavoritesAddFlags`. Dead-covered: `runFavoritesAddProduction` re-validates, so this is redundancy loss for DI callers, not a current user-facing hole | CONFIRMED | test | `TestParseFavoritesAddFlags_Validation` | PR5 | todo | +| OUT-21 | cmd/status | `cmd/status.go:110-114` | Make the directory-name merge unconditional: drop the `if _, exists := data.nameMap[k]; !exists` guard. Precedence is genuinely unasserted, but in production both lookups read the same cached Azure eligibility response, so a divergence needs colliding IDs or malformed data | OVERSTATED | test | `TestStatus_DirectoryNameMergePrecedence` | PR5 | todo | +| OUT-22 | cmd/status | `cmd/status.go:129` | Delete `_ = cache.CleanupSessions(tracker, activeIDs)` | CONFIRMED | test | `TestStatus_CleansUpStaleSessionTimestamps` | PR5 | todo | +| OUT-23 | cmd/status (test quality) | `cmd/status.go:185-192` (`computeRemainingTime`) | No production defect. `TestStatusCommand_RemainingTime/text_output_shows_remaining_time` asserts `remaining: 4` as a substring, which `remaining: 4h 30m` satisfies — only the JSON sibling killed a sixfold arithmetic error. Signal-poor assertion, not an uncovered defect | CONFIRMED | test | Tighten the text subtest to an exact `remaining: 45m` | PR5 | todo | +| OUT-24 | cmd/favorites | `cmd/favorites.go:419-420` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | todo | +| OUT-25 | cmd/list flags | `cmd/list.go:71` | Delete `cmd.Flags().Bool("refresh", ...)` registration. **Verifier correction:** `grant list --refresh` is **not** already a no-op — `list.go:91-92` reads it and passes it into `buildCachedLister`, and CLAUDE.md is correct. The real finding is missing flag-registration/wiring coverage | OVERSTATED | test | `TestListCommand_RefreshBypassesCache` | PR5 | todo | +| OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | todo | +| OUT-27 | cmd/favorites | `cmd/favorites.go:235-237` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | refuted | n/a — already killed | — | todo | +| OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | +| OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | +| SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | todo | +| SCA-02 | internal/sca | `internal/sca/service.go:208` | `s.httpClient.Post(ctx, "/api/access/elevate", req)` → `..., nil)` | CONFIRMED | test | `TestElevate_SendsExactBody` (add `gotBody` to `mockHTTPClient`) | PR8 | todo | +| SCA-03 | internal/sca | `internal/sca/service.go:236` | `s.httpClient.Post(ctx, "/api/access/sessions/revoke", req)` → `..., nil)` | CONFIRMED | test | `TestRevokeSessions_SendsExactBody` | PR8 | todo | +| SCA-04 | internal/sca | `internal/sca/service.go:390` | `s.httpClient.Post(ctx, "/api/access/elevate/groups", req)` → `..., nil)` | CONFIRMED | test | `TestElevateGroups_SendsExactBody` | PR8 | todo | +| SCA-05 | internal/sca | `internal/sca/service.go:208` | Route `"/api/access/elevate"` → `"/WRONG"` | CONFIRMED | test | `TestElevate_Route` (add `gotRoute`) | PR8 | todo | +| SCA-06 | internal/sca | `internal/sca/service.go:258` | Route `"/api/access/sessions"` → `"/WRONG"` | CONFIRMED | test | `TestListSessions_Route` | PR8 | todo | +| SCA-07 | internal/sca | `internal/sca/service.go:236` | Route `"/api/access/sessions/revoke"` → `"/WRONG"` | CONFIRMED | test | `TestRevokeSessions_Route` | PR8 | todo | +| SCA-08 | internal/sca | `internal/sca/service.go:285` | `route := fmt.Sprintf("/api/access/%s/eligibility/groups", csp)` → `fmt.Sprintf("/WRONG/%s", csp)` | CONFIRMED | test | `TestListGroupsEligibility_Route` | PR8 | todo | +| SCA-09 | internal/sca | `internal/sca/service.go:390` | Route `"/api/access/elevate/groups"` → `"/WRONG"` | CONFIRMED | test | `TestElevateGroups_Route` | PR8 | todo | +| SCA-10 | internal/sca | `internal/sca/service.go:323` and `:356` | `"pageSize": -1` → `"pageSize": 10` at **both** on-demand call sites. The wire contract is genuinely untested; the *consequence* ("-1 means all, so 10 truncates the role picker") is **unevidenced** — neither the repo, the pinned SDK, nor official docs document this endpoint's `-1` semantics. Assert the sent value; do not assert a truncation story | OVERSTATED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | todo | +| SCA-11 | internal/sca | `internal/sca/service.go:326` and `:360` | `"target_category": "cloud_console"` → `"WRONG"` at **both** on-demand call sites | CONFIRMED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | todo | +| SCA-12 | internal/sca | `internal/sca/service.go:258-262` | `ListSessions`'s `buildParams` closure returns `nil` instead of `map[string]string{"csp": string(*csp)}`. `TestListSessions_WithCSPFilter` is tautological — its canned response is already Azure and it never inspects params. `grant status --provider azure` does no local filtering, so all providers' sessions would display | CONFIRMED | test | Replace `TestListSessions_WithCSPFilter` with `TestListSessions_SendsCSPQueryParam` (add `gotParams`) | PR8 | todo | +| SCA-13 | internal/sca | `internal/sca/service.go:144` | `s.httpClient.Get(ctx, route, p)` → `s.httpClient.Get(context.Background(), route, p)` in `paginate` | CONFIRMED | test | `TestPaginate_PropagatesContextCancellation` | PR8 | todo | +| SCA-14 | internal/sca | `internal/sca/service.go:184` (and the sibling decoders at `:267`, `:291`) | In the `ListEligibility` decode closure, swallow the error: `if err := json.NewDecoder(r).Decode(&page); err != nil { return nil, nil, 0, nil }` | CONFIRMED | test | `TestListEligibility_PropagatesDecodeError` | PR8 | todo | +| SCA-15 | internal/sca | `internal/sca/service.go:74` | `client.SetHeader("X-API-Version", "2.0")` → delete the call, and separately → `"1.0"`. The **only** guard lives inside `TestNewSCAAccessServiceDisablesTransientRetry`, so a retry-motivated rename silently deletes the header assertion. Verifier could not execute it (loopback prohibited in that sandbox); coverage topology confirmed by grep | OVERSTATED | test | Extract `TestNewSCAAccessService_SetsAPIVersionHeader` as its own named test | PR8 | todo | +| SCA-16 | internal/sca models | `internal/sca/models/elevate.go:28` | `RoleID string \`json:"roleId"\`` → `json:"roleIdXX"` on the **request** model | CONFIRMED | test | `TestElevateRequest_JSONTags` | PR8 | todo | +| SCA-17 | internal/sca models | `internal/sca/models/credentials.go:17` (`ParseAWSCredentials`) | Swap `SecretAccessKey` and `SessionToken` in the parser output. **Mutant dies repo-wide**: `env_test.go:77` and `root_elevate_test.go:426`. Nuance: swapping the *struct JSON tags* instead is also caught, by `TestAWSCredentials_JSONUnmarshal`. Recorded so this is not re-filed as a survivor | REFUTED | refuted | n/a — already killed | — | todo | +| SCA-18 | internal/sca | `internal/sca/service.go:75-ish` (`sdkclient.DisableTransientRetry` call) | Claim: deleting the call yields `inbound requests = 4, want 1`. **Not reproducible** in the verifier's sandbox (loopback prohibited); a literal deletion is a compile kill first (`"internal/sdkclient" imported and not used`). Same for the workflows twin. The guard tests exist and are correctly aimed; only their runtime assertion was unverifiable | OVERSTATED | refuted | n/a — `internal/sca/retry_policy_test.go` / `internal/workflows/retry_policy_test.go` already guard this | — | todo | +| WF-01 | internal/workflows | `internal/workflows/service.go:102` | Delete `if err := checkResponse(resp, "request forms"); err != nil { return nil, err }` | CONFIRMED | test | `TestWorkflows_Non200` (table over all six call sites) | PR8 | todo | +| WF-02 | internal/workflows | `internal/workflows/service.go:161` | Delete the `checkResponse(resp, "list requests")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | +| WF-03 | internal/workflows | `internal/workflows/service.go:196` | Delete the `checkResponse(resp, "get request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | +| WF-04 | internal/workflows | `internal/workflows/service.go:221` | Delete the `checkResponse(resp, "submit request")` guard. Verified consequence: a 500 carrying `{}` decodes to an empty request, so `grant request submit` prints a blank `Request ID:` / `State:` and exits 0 | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | +| WF-05 | internal/workflows | `internal/workflows/service.go:245` | Delete the `checkResponse(resp, "cancel request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | +| WF-06 | internal/workflows | `internal/workflows/service.go:272` | Delete the `checkResponse(resp, "finalize request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | +| WF-07 | internal/workflows | `internal/workflows/logging_client.go:58` | Delete the `if redacted.Get("Authorization") != ""` redaction block. Real token-in-logs risk; the SCA twin is covered by `TestLoggingClient_DebugLogsHeaders` | CONFIRMED | test | new `internal/workflows/logging_client_test.go`, mirroring the sca one **including Authorization redaction** | PR8 | todo | +| WF-08 | internal/workflows | `internal/workflows/logging_client.go:24-26` | Route `Get` through `c.inner.Post(ctx, route, params)` | CONFIRMED | test | `TestLoggingClient_GetUsesGet` (workflows) | PR8 | todo | +| WF-09 | internal/workflows | `internal/workflows/logging_client.go:24-33` | Swallow the inner error and return a synthetic 200 response with `err = nil` | CONFIRMED | test | `TestLoggingClient_PropagatesInnerError` (workflows) | PR8 | todo | +| WF-10 | internal/workflows models | `internal/workflows/models/submit.go:6` | `RequestDetails map[string]interface{} \`json:"requestDetails"\`` → `json:"requestDetailsXX"` | CONFIRMED | test | `TestSubmitAccessRequest_JSONTags` | PR8 | todo | +| WF-11 | internal/workflows models | `internal/workflows/models/finalize.go:5` | `Result string \`json:"result"\`` → `json:"resultXX"` | CONFIRMED | test | `TestFinalizeAccessRequest_JSONTags` | PR8 | todo | +| WF-12 | internal/workflows models | `internal/workflows/models/cancel.go:5` | `CancelReason *string \`json:"cancelReason"\`` → `json:"cancelReasonXX"` | CONFIRMED | test | `TestCancelAccessRequest_JSONTags` | PR8 | todo | +| WF-13 | internal/workflows | `internal/workflows/service.go:263` | Delete `FinalizationReason: reason,` from the `FinalizeAccessRequest` literal | CONFIRMED | test | `TestFinalizeRequest_SendsFinalizationReason` | PR8 | todo | +| WF-14 | internal/workflows | `internal/workflows/service.go:260` | `route := fmt.Sprintf("/api/workflows/requests/%s/finalize", requestID)` → `"/api/workflows/requests/finalize"`. The cancel twin *is* covered (`service_test.go:274`), which is the contrast that proves the gap | CONFIRMED | test | `TestFinalizeRequest_ExactRoute` | PR8 | todo | +| WF-15 | internal/workflows | `internal/workflows/service.go:141` | Delete `qp["limit"] = strconv.Itoa(limit)` | CONFIRMED | test | `TestListRequests_SendsLimit` | PR8 | todo | +| WF-16 | internal/workflows | `internal/workflows/service.go:124` | `const defaultPageSize = 50` → `= 1` | CONFIRMED | test | `TestListRequests_DefaultPageSize` | PR8 | todo | +| WF-17 | internal/workflows | `internal/workflows/service.go:156` | `s.httpClient.Get(ctx, "/api/workflows/requests", qp)` → `Get(context.Background(), ...)` in the pagination loop | CONFIRMED | test | `TestListRequests_PropagatesContextCancellation` | PR8 | todo | +| WF-18 | internal/workflows | `internal/workflows/service.go:201` | In `GetRequest`, swallow the decode error: `if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { return &result, nil }` | CONFIRMED | test | `TestGetRequest_PropagatesDecodeError` | PR8 | todo | +| WF-19 | internal/workflows | `internal/workflows/service_config.go:9` | `ServiceName: "access-requests"` → `"WRONG"`. There is no workflows `service_config_test.go` at all | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | todo | +| WF-20 | internal/workflows | `internal/workflows/service.go:45` | `base.Authenticator("isp")` → `base.Authenticator("WRONG")` | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | todo | +| ELV-01 | cmd/selection | `cmd/selection.go:78` | `return &items[i], nil` → `return &items[0], nil`. `TestFindItemByDisplay` only checks non-nil/error, so selecting one display value silently elevates the first sorted target and prints a success line naming the wrong one | CONFIRMED | test | `TestFindItemByDisplay_ReturnsMatchingItem` | PR4 | todo | +| ELV-02 | cmd/root (unified elevate builder) | `cmd/root.go:786-793` | In `elevateCloud`, swap `WorkspaceID: selectedTarget.WorkspaceID` and `RoleID: selectedTarget.RoleInfo.ID` | CONFIRMED | test | `TestElevateCloud_RequestPayload` (`mockElevateService` history) | PR4 | todo | +| ELV-03 | cmd/root (unified elevate builder) | `cmd/root.go:786-788` | In `elevateCloud`, blank both `CSP:` and `OrganizationID:` | CONFIRMED | test | `TestElevateCloud_RequestPayload` | PR4 | todo | +| ELV-04 | cmd/root (env/direct elevate builder) | `cmd/root.go:497-506` | In `resolveAndElevate`, swap `WorkspaceID` and `RoleID` in the `ElevateRequest` literal. (Plan calls these "both builders": `:497` and `:786`) | CONFIRMED | test | `TestResolveAndElevate_RequestPayload` | PR4 | todo | +| ELV-05 | cmd/env favorite path | `cmd/root.go:429` | `if flags.favorite != "" {` → `if false {` in `resolveAndElevate`. No env test exercises `--favorite`, yet the flag is registered (`cmd/env.go:39`) and advertised in help (`cmd/env.go:29`) | CONFIRMED | test | `TestEnv_FavoriteMode` | PR4 | todo | +| ELV-06 | cmd/env favorite path | `cmd/root.go:438-440` | Delete the group-favorite rejection (`if fav.ResolvedType() == config.FavoriteTypeGroups { return ... }`) | CONFIRMED | test | `TestEnv_RejectsGroupFavorite` | PR4 | todo | +| ELV-07 | cmd/env favorite path | `cmd/root.go:443-445` | Delete the provider-mismatch check `if flags.provider != "" && !strings.EqualFold(flags.provider, fav.Provider)` | CONFIRMED | test | `TestEnv_FavoriteProviderMismatch` | PR4 | todo | +| ELV-08 | cmd/env direct path | `cmd/root.go:456-458` | Delete the paired `--target`/`--role` validation in `resolveAndElevate` | CONFIRMED | test | `TestEnv_RequiresBothTargetAndRole` | PR4 | todo | +| ELV-09 | cmd/root favorite path | `cmd/root.go:577` | `if fav.ResolvedType() == config.FavoriteTypeGroups {` → `if false {` in `resolveFavoriteFlags`. This is the row that refutes "all root equivalents are covered" — root group-favorite **detection** is also unpinned | CONFIRMED | test | `TestResolveFavoriteFlags_DetectsGroupFavorite` | PR4 | todo | +| ELV-10 | cmd/root group elevate | `cmd/root.go:837-841` | `if result.ErrorInfo != nil {` → `if false {` in `elevateGroup`. Execution then builds a result and returns nil: a policy denial prints as success and exits 0 | CONFIRMED | test | `TestElevateGroup_SurfacesErrorInfo` | PR4 | todo | +| ELV-11 | cmd/env elevate | `cmd/root.go:525-530` | `if result.ErrorInfo != nil {` → `if false {` in `resolveAndElevate` (the env path). Same false-success consequence | CONFIRMED | test | `TestEnv_SurfacesErrorInfo` | PR4 | todo | +| ELV-12 | cmd/env elevate | `cmd/root.go:520-522` | Delete `if len(elevateResp.Response.Results) == 0 { return nil, errors.New("elevation failed: no results returned") }` (env path). Mutant panics on `Results[0]` only if a test supplies an empty slice — none does | CONFIRMED | test | `TestEnv_EmptyResultsGuard` | PR4 | todo | +| ELV-13 | cmd/root cloud elevate | `cmd/root.go:802-805` | Delete the identical empty-`Results` guard in `elevateCloud` | CONFIRMED | test | `TestElevateCloud_EmptyResultsGuard` | PR4 | todo | +| ELV-14 | cmd/root group elevate | `cmd/root.go:832-835` | Delete the identical empty-`Results` guard in `elevateGroup` | CONFIRMED | test | `TestElevateGroup_EmptyResultsGuard` | PR4 | todo | +| ELV-15 | cmd/root JSON | `cmd/root.go:936-937` | In `writeElevationJSON`, swap `Target: cloudRes.target.WorkspaceName` and `Role: cloudRes.target.RoleInfo.Name` | CONFIRMED | test | `TestElevationJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| ELV-16 | cmd/root JSON | `cmd/root.go:923-924` | In `writeElevationJSON`, swap `GroupID: groupRes.group.GroupID` and `DirectoryID: groupRes.group.DirectoryID` | CONFIRMED | test | `TestGroupElevationJSON_Contract` | PR5 | todo | +| ELV-17 | cmd/env JSON | `cmd/env.go:145-147` | Swap `SecretAccessKey: awsCreds.SecretAccessKey` and `SessionToken: awsCreds.SessionToken`. Asymmetry is the point: the identical swap in the **text** export path is killed (`env_test.go:77`) | CONFIRMED | test | `TestEnvJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| ELV-18 | cmd/root JSON | `cmd/root.go:934` | `Provider: strings.ToLower(string(cloudRes.target.CSP))` → drop the `strings.ToLower` | CONFIRMED | test | `TestElevationJSON_Contract` | PR5 | todo | +| ELV-19 | cmd/root | `cmd/root.go:341-344` | Delete `if len(all) == 0 { return nil, errors.New("no eligible targets found, check your SCA policies") }` in `fetchEligibility`'s multi-CSP branch. Callers replace the intended aggregate error with their own message | CONFIRMED | test | `TestFetchEligibility_AllCSPsFail` | PR4 | todo | +| ELV-20 | cmd/env elevate | `cmd/root.go:510-514` | Remove the fresh-context setup and elevate with the original `ctx`: delete `elevCtx, elevCancel := context.WithTimeout(...)` / `defer elevCancel()` and call `elevateService.Elevate(ctx, req)`. **Note:** a raw `elevCtx → ctx` token substitution does *not* compile (`declared and not used: elevCtx`) — use the semantic form above. Root's three interactive dispatch paths *are* covered by `TestRootElevate_SlowPromptTimeout`; env is not | CONFIRMED | test | `TestEnv_SlowPromptTimeout` | PR4 | todo | +| ELV-21 | cmd/env auth | `cmd/root.go:419` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in `resolveAndElevate` (env path) | CONFIRMED | test | `TestEnv_AuthCacheFlag` | PR4 | todo | +| ELV-22 | cmd/root auth | `cmd/root.go:620` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in the root elevate path | CONFIRMED | test | `TestRootElevate_AuthCacheFlag` | PR4 | todo | +| ELV-23 | cmd/env selector | `cmd/root.go:480` | `selector.SelectTarget(allTargets)` → `selector.SelectTarget(nil)`. `mockTargetSelector` returns its canned target without inspecting the slice | CONFIRMED | test | `TestEnv_SelectorReceivesAllTargets` | PR4 | todo | +| ELV-24 | cmd/root Execute | `cmd/root.go:291` | `if !verbose && passedArgValidation {` → `if verbose && passedArgValidation {`. `TestVerboseHintSuppressedForArgErrors` calls Cobra's `root.Execute()` and then *reconstructs* the hint logic; it never invokes the package-level `Execute()` | CONFIRMED | test | `TestExecute_VerboseHintCondition` | PR4 | todo | +| ELV-25 | cmd/root dispatch | `cmd/root.go:631-636` | Swap the dispatch order: test `if flags.groups` before `if flags.group != ""`. `--group` and `--groups` are **not** mutually exclusive (`root.go:136-142` pairs neither), so their precedence is unspecified and unpinned | CONFIRMED | test | `TestRootElevate_GroupAndGroupsPrecedence` | PR4 | todo | +| ELV-26 | cmd test quality | `cmd/root_elevate_test.go:248` | No production site. The `multi-CSP concurrent fetch - parallel execution` case duplicates the line-174 success setup, adds sleeps, and asserts **no** elapsed time. Real concurrency is covered by `TestFetchEligibility_ConcurrentExecution` | CONFIRMED | test | Delete the duplicate case or give it a real elapsed-time assertion | PR4 | todo | +| ELV-27 | cmd test quality | `cmd/root_test.go` (`TestFetchEligibility_ConcurrentExecution`) | Claim: the `<350ms` bound for two concurrent 200ms sleeps is flaky. **Not demonstrated** — 50/50 runs passed. The wall-clock sensitivity remains a plausible overloaded-CI risk, so widen the bound; do not claim an observed flake | OVERSTATED | test | Widen the bound in `TestFetchEligibility_ConcurrentExecution` | PR4 | todo | +| SFU-01 | internal/selfupdate | `internal/selfupdate/selfupdate.go:345` | `case path.IsAbs(cleaned):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath` — one guard-specific `wantErrContains` per arm, with a valid `grant` entry beside each malicious one so the "no binary" fallback cannot be the reason for the error | PR2 | todo | +| SFU-02 | internal/selfupdate | `internal/selfupdate/selfupdate.go:347` | `case strings.HasPrefix(normalized, "//"):` → `case false:`. **Production change (PR2):** move this arm *before* `path.IsAbs` — `path.Clean` collapses `//host/share/x` → `/host/share/x`, so `IsAbs` always wins and the UNC arm is unreachable. Rejection is unchanged; only the message differs | CONFIRMED | test + prod-fix | `TestCheckArchivePath/unc_path` | PR2 | todo | +| SFU-03 | internal/selfupdate | `internal/selfupdate/selfupdate.go:349` | `case hasDriveLetter(normalized):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/drive_absolute` | PR2 | todo | +| SFU-04 | internal/selfupdate | `internal/selfupdate/selfupdate.go:343` | `case name == "":` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/empty_name` | PR2 | todo | +| SFU-05 | internal/selfupdate | `internal/selfupdate/selfupdate.go:339` | `normalized := strings.ReplaceAll(name, "\\", "/")` → `normalized := name` (backslash traversal and backslash UNC then slip through) | CONFIRMED | test | `TestCheckArchivePath/backslash_traversal`, `.../backslash_unc` | PR2 | todo | +| SFU-06 | internal/selfupdate | `internal/selfupdate/selfupdate.go:359` (`hasDriveLetter`) | Narrow the check to uppercase drive letters only, so lowercase `c:\...` passes | CONFIRMED | test | `TestCheckArchivePath/lowercase_drive`, `.../forward_slash_drive` | PR2 | todo | +| SFU-07 | internal/selfupdate | `internal/selfupdate/selfupdate.go:392` | `if hdr.Typeflag != tar.TypeReg \|\| !isBinaryEntry(hdr.Name) {` → drop the `hdr.Typeflag != tar.TypeReg` operand. Probe (`Typeflag: tar.TypeSymlink, Name: "grant", Size: 0, Linkname: "/etc/passwd"`): baseline `bytes=0 err=archive does not contain a grant binary`; mutated `bytes=0 err=` — i.e. a **zero-byte self-destruct**. No symlink case exists anywhere in the package | CONFIRMED | test + prod-fix | `TestExtractBinary_RejectsNonRegularEntries` (symlink / hardlink / directory named `grant`, plus a zip directory entry). **Production:** reject a zero-length extracted binary *and* add a second non-empty check at the apply boundary (`internal/selfupdate/apply.go:50`). CHANGELOG `### Security` | PR2 | todo | +| SFU-08 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389-391` | Delete the tar declared-size guard (`if hdr.Size > maxDownloadBytes`) | CONFIRMED | test | `TestExtractFromTarGz_RejectsOversizeDecoy` — oversized **decoy** beside a valid binary | PR2 | todo | +| SFU-09 | internal/selfupdate | `internal/selfupdate/selfupdate.go:430-432` | Delete the zip declared-size guard (`if maxDownloadBytes >= 0 && f.UncompressedSize64 > uint64(maxDownloadBytes)`) | CONFIRMED | test | `TestExtractFromZip_RejectsOversizeDecoy` | PR2 | todo | +| SFU-10 | internal/selfupdate | `internal/selfupdate/apply.go:74` | Delete the `if err := syncStagedFile(target); err != nil { ... }` call. Per the consistency review this is **not** a production gap — `applyWithOptions` already returns a wrapped sync error before commit and `syncStagedFile` (`:110`) already returns `f.Sync()` errors. Scope is a seam plus tests; **no CHANGELOG entry** | CONFIRMED | test | `TestApplyWithOptions_SyncsBeforeCommit` via a `syncStagedFileFn` seam (call-order + abort-before-commit) | PR3 | todo | +| SFU-11 | internal/selfupdate | `internal/selfupdate/apply.go:110-115` | In `syncStagedFile`, ignore the `f.Sync()` error: `_ = f.Sync(); return nil` | CONFIRMED | test | `TestApplyWithOptions_AbortsOnSyncError` | PR3 | todo | +| SFU-12 | internal/selfupdate | `internal/selfupdate/apply.go:154-156` | In `InterruptedUpdate`, delete the target-exists guard (`if _, err := os.Stat(targetPath); err == nil \|\| !errors.Is(err, os.ErrNotExist) { return "", false }`). The untested case is target **present** and `.old` present — the documented Windows steady state | CONFIRMED | test | `TestInterruptedUpdate_TargetPresentWithOldBackup` | PR3 | todo | +| SFU-13 | internal/selfupdate | `internal/selfupdate/selfupdate.go:197-199` | Delete the non-200 check `if resp.StatusCode != http.StatusOK { ... }` in `fetchLatestRelease` | CONFIRMED | test | `newFixtureServerWith(t, opts)` → `TestFetchLatestRelease_Non200` | PR3 | todo | +| SFU-14 | internal/selfupdate | `internal/selfupdate/selfupdate.go:202-204` | In `fetchLatestRelease`, swallow the `json.Unmarshal` error on an empty body: `_ = json.Unmarshal(body, &rel)` | CONFIRMED | test | `TestFetchLatestRelease_EmptyBody` | PR3 | todo | +| SFU-15 | internal/selfupdate | `internal/selfupdate/selfupdate.go:205-207` | Delete `if rel.TagName == "" { return nil, errors.New("GitHub release response has no tag_name") }` | CONFIRMED | test | `TestFetchLatestRelease_EmptyTagName`; also non-200 on the **asset** and **checksums** downloads (`:229`) | PR3 | todo | +| SFU-16 | internal/selfupdate | `internal/selfupdate/version.go` (`comparePreRelease`, numeric-vs-numeric branch) | Invert the numeric-vs-numeric comparison so `rc.10` sorts before `rc.2` | CONFIRMED | test | `TestCompareVersions_NumericPrereleaseOrdering` (`rc.10` vs `rc.2`) | PR3 | todo | +| SFU-17 | internal/selfupdate | `internal/selfupdate/version.go:194` | `if !isAllDigits(part) {` → `if false {` in the core `MAJOR.MINOR.PATCH` loop, so `"1.+5.3"` is accepted | CONFIRMED | test | `TestParseVersion/invalid_core_segment` (`"1.+5.3"`) | PR3 | todo | +| SFU-18 | internal/selfupdate | `internal/selfupdate/selfupdate.go:284-287` | `if len(fields) != 2 { return fmt.Errorf("malformed line in %s: %q", ...) }` → `continue` | CONFIRMED | test | `TestVerifyChecksum_MalformedLine` | PR3 | todo | +| SFU-19 | internal/selfupdate | `internal/selfupdate/selfupdate.go:316-317` | In `extractBinary`, replace the `default:` unsupported-format error with `return extractFromTarGz(archive)` | CONFIRMED | test | `TestExtractBinary_UnsupportedFormat` | PR3 | todo | +| SFU-20 | internal/selfupdate | `internal/selfupdate/selfupdate.go:402` | Delete `if int64(len(data)) != hdr.Size { ... }` (tar truncation cross-check). **Unreachable by construction**: a successful capped read returns exactly `hdr.Size`, and earlier exhaustion returns `io.ErrUnexpectedEOF`. Hand-patched proof: header declares 40 with body `"bin"` → `bytes=40 err=`; header declares 2000 → `bytes=0 err=... unexpected EOF` | CONFIRMED | wont-fix | none — keep as defense-in-depth, comment it as unreachable, and claim no coverage. Rename `TestExtractBinaryRejectsTruncatedEntry` → `...TruncatedArchive` | PR2 | todo | +| SFU-21 | internal/selfupdate | `internal/selfupdate/selfupdate.go:437` | Delete `if uint64(len(data)) != f.UncompressedSize64 { ... }` (zip truncation cross-check). Same unreachability argument as SFU-20 | CONFIRMED | wont-fix | none — defense-in-depth, no coverage claimed | PR2 | todo | +| SFU-22 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389` vs `:430` | tar/zip size-check asymmetry. The original "zip decompression bomb" framing is **overstated**: the structural asymmetry is real (`maxDownloadBytes=10`, 5000-byte decoy → `TAR bytes=0 err=` vs `ZIP bytes=3 err=`), but `zip.NewReader` parses only the central directory and never opens skipped entries. The tar guard is load-bearing; the zip placement is a consistency point, not a vulnerability | OVERSTATED | wont-fix | Pin the asymmetry as **intentional** with a comment and a test asserting a skipped zip entry is never inflated | PR2 | todo | +| CACHE-01 | internal/cache | `internal/cache/cached_eligibility.go:84` | When `c.refresh` is true, skip the write: guard `Set(c.store, key, *resp)` with `if !c.refresh`. `--refresh` must bypass the **read** but still **write** | CONFIRMED | test | `TestCachedEligibility_RefreshStillWrites` | PR6 | todo | +| CACHE-02 | internal/cache | `internal/cache/cached_eligibility.go:117` | Same mutation on the groups-eligibility write | CONFIRMED | test | `TestCachedGroupsEligibility_RefreshStillWrites` | PR6 | todo | +| CACHE-03 | internal/cache | `internal/cache/cached_roles.go:53` | Same mutation on the on-demand-roles write | CONFIRMED | test | `TestCachedRoles_RefreshStillWrites` | PR6 | todo | +| CACHE-04 | internal/cache | `internal/cache/cache.go:39-41` | `if err := json.Unmarshal(data, &e); err != nil { return false }` → ignore the error and fall through. `TestGet_CorruptJSON` passes today via the zero-`CachedAt` TTL branch, not the unmarshal guard | CONFIRMED | test | Fix `TestGet_CorruptJSON` to use a **fresh** `cached_at` with a type-mismatched payload, so only the unmarshal guard can produce the miss | PR6 | todo | +| CACHE-05 | internal/cache | `internal/cache/cached_eligibility.go:131` | `"groups_eligibility_" + ...` → `"eligibility_" + ...` (key collision with the cloud-eligibility prefix) | CONFIRMED | test | `TestCacheKeys_DistinctPrefixes` | PR6 | todo | +| CACHE-06 | internal/cache | `internal/cache/cached_eligibility.go:127` and `:131` | Drop `strings.ToLower(string(csp))` from both key builders | CONFIRMED | test | `TestCacheKeys_LowercaseCSP` | PR6 | todo | +| CACHE-07 | internal/cache | `internal/cache/session_tracker.go:14` | `const maxSessionAge = 24 * time.Hour` → `25 * time.Hour`. No test pins either value; code says 24h and CLAUDE.md says 25h | CONFIRMED | test + prod-fix | `TestSessionTimestamps_RetentionBoundary`. **Production:** rename to `sessionTimestampRetention` (keep 24h) with a comment stating it is local retention for remaining-time display — not a session limit or access-control boundary. Also fix the factually wrong "removed on cleanup" comment: `CleanupSessions` filters on active IDs and never reads it. Drop the 25h claim from CLAUDE.md | PR6 | todo | +| CFG-01 | internal/config | `internal/config/config.go:54-58` | In `Load`, return the default config for **any** read error: `if err != nil { return DefaultConfig(), nil }`. Killed on Linux by `config_test.go:197`, but `config_test.go:184-186` **skips on Windows**, so there is zero coverage on the windows-latest leg | CONFIRMED | test | Portable replacement: `Load()` — errors as EISDIR on POSIX / ERROR_ACCESS_DENIED on Windows, and `errors.Is(err, os.ErrNotExist)` is false on both. (Windows half reasoned, not measured — verify on the CI leg) | PR6 | todo | +| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-5m`, `garbage`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go:541`. CHANGELOG `### Changed` | PR6 | todo | +| CFG-03 | internal/config | `internal/config/config.go:20` | `const DefaultCacheTTL = 4 * time.Hour` → `400 * time.Hour`. The existing assertion is the tautology `want: DefaultCacheTTL` | CONFIRMED | test | `TestParseCacheTTL_DefaultIsFourHours` (assert the literal `4 * time.Hour`) | PR6 | todo | +| CFG-04 | internal/config | `internal/config/config.go:60` | `cfg := DefaultConfig()` → `cfg := &Config{}` (defaults no longer survive a partial YAML file) | CONFIRMED | test | `TestLoad_PartialYAMLKeepsDefaults` | PR6 | todo | +| CFG-05 | internal/config | `internal/config/config.go:65-67` | Delete `if cfg.Favorites == nil { cfg.Favorites = make(map[string]Favorite) }` | CONFIRMED | test | `TestLoad_FavoritesNeverNil` | PR6 | todo | +| CFG-06 | internal/config | `internal/config/config.go:61-63` | Swallow the YAML error: `_ = yaml.Unmarshal(data, cfg)` | CONFIRMED | test | `TestLoad_InvalidYAMLErrors` | PR6 | todo | +| CFG-07 | internal/config | `internal/config/config.go:103` | `filepath.Join(home, ".grant")` → `".grantx"` | CONFIRMED | test | `TestConfigDir_EndsInDotGrant` | PR6 | todo | +| CFG-08 | internal/config | `internal/config/config.go:84` | `os.WriteFile(path, data, 0o600)` → `0o644` | CONFIRMED | test | `TestSave_FileMode` with the `runtime.GOOS == "windows"` skip | PR6 | todo | +| CFG-09 | internal/config | `internal/config/config.go:75` | `if err := os.MkdirAll(dir, 0o700); err != nil { ... }` → ignore the error | CONFIRMED | test | `TestSave_MkdirAllFailure` — force it portably by pointing at a path whose parent component is an existing **regular file** (ENOTDIR / ERROR_DIRECTORY), never a hardcoded `/dev/null/...` | PR6 | todo | +| UI-01 | internal/ui | `internal/ui/tty.go:18` | `return IsTerminalFunc(os.Stdin.Fd())` → `IsTerminalFunc(os.Stdout.Fd())`. This swap is what makes `grant revoke < /dev/null` hang in a terminal. All twelve prompt-level guards are well covered (8 spot-checked, all killed with `errors.Is` + flag hints); `IsInteractive()` itself is not, because every stub ignores `fd` | CONFIRMED | test | `TestIsInteractive_ChecksStdinFd` — a stub that records the fd and asserts `os.Stdin.Fd()` | PR7 | todo | +| UI-02 | internal/ui | `internal/ui/group_selector.go:60` | Delete the `sort.Slice(sorted, ...)` call in the group selector | CONFIRMED | test + prod-fix | Extract `sortGroupsForDisplay` so ordering is testable without a TTY; `TestSortGroupsForDisplay_CollisionOrdering` | PR7 | todo | +| UI-03 | internal/ui | `internal/ui/session_selector.go:57` | `if remaining <= 0 {` → `if remaining < 0 {`. The fixture only supplies `-5m` (`session_selector_test.go:154`), so exactly-zero is unpinned | CONFIRMED | test | `TestFormatSessionOption_ExactlyZeroRemaining` | PR7 | todo | +| UI-04 | internal/ui | `internal/ui/request_selector.go:18` | Delete the `time.Parse(time.RFC3339Nano, ts)` branch in the timestamp formatter | CONFIRMED | test | `TestFormatRequestOption_RFC3339Nano` | PR7 | todo | +| UI-05 | internal/ui | `internal/ui/role_selector.go:36` | Make the role sort case-**sensitive** (drop the `strings.ToLower` normalization in the `sort.SliceStable` less-func). Note: the raw mutation orphans the `strings` import — remove it too | CONFIRMED | test | `TestSortRolesForDisplay_MixedCase` | PR7 | todo | +| UI-06 | internal/ui | `internal/ui/selector.go:49` | Delete the `if len(targets) == 0` guard in `SelectTarget`. (`SelectRole`/`SelectRequest` equivalents are **killed**; these three are not) | CONFIRMED | test | `TestSelectTarget_EmptyList` | PR7 | todo | +| UI-07 | internal/ui | `internal/ui/session_selector.go:112` | Delete the `if len(sessions) == 0` guard in `SelectSessions` | CONFIRMED | test | `TestSelectSessions_EmptyList` | PR7 | todo | +| UI-08 | internal/ui | `internal/ui/group_selector.go:54` | Delete the `if len(groups) == 0` guard in `SelectGroup`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | `TestSelectGroup_EmptyList` | PR7 | todo | +| UI-09 | internal/ui | `internal/ui/role_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRole` | CONFIRMED | wont-fix | none — defensive-only and unreachable through `survey`, which can only return a string it was given | PR7 | todo | +| UI-10 | internal/ui | `internal/ui/request_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRequest` | CONFIRMED | wont-fix | none — same rationale as UI-09 | PR7 | todo | + +--- + +## Summary + +### By PR + +| PR | Rows | Of which CONFIRMED | +|---|---|---| +| PR1 — Test isolation + integration harness | 2 | 0 (both OVERSTATED) | +| PR2 — Archive extraction and path security | 12 | 11 | +| PR3 — Remaining self-update correctness | 10 | 10 | +| PR4 — Argument capture | 42 | 41 | +| PR5 — Output contracts | 32 | 30 | +| PR6 — Cache and config semantics | 16 | 16 | +| PR7 — UI behavior | 10 | 10 | +| PR8 — SCA / workflows / models wire contracts | 36 | 34 | +| *(no PR — settled, recorded only)* | 5 | 0 | +| **Total** | **165** | **152** | + +### By verdict + +| Verdict | Rows | +|---|---| +| CONFIRMED | 152 | +| OVERSTATED | 9 | +| REFUTED | 4 | + +### By disposition + +| Disposition | Rows | +|---|---| +| `test` | 149 | +| `test + prod-fix` | 5 | +| `prod-fix` | 1 | +| `wont-fix` | 5 | +| `refuted` | 5 | +| **Total** | **165** | + +The seven production changes, matching the plan's table: + +| Row | Change | PR | CHANGELOG | +|---|---|---|---| +| SFU-07 | Reject zero-length extracted binary + apply-boundary check | PR2 | `### Security` | +| SFU-02 | UNC check before `path.IsAbs` (message only) | PR2 | no | +| OUT-26 | `favorites add` early non-interactive guard, favorites-specific message | PR1 | `### Fixed` | +| CFG-02 | `ParseCacheTTL` errors on any explicitly-invalid value | PR6 | `### Changed` | +| CACHE-07 | `maxSessionAge` → `sessionTimestampRetention` | PR6 | no (internal) | +| UI-02 | `sortGroupsForDisplay` extraction | PR7 | no (refactor) | +| SFU-10 | `syncStagedFileFn` seam | PR3 | no (test seam; **not** a production gap) | + +### Total CONFIRMED + +**152 CONFIRMED**, plus **9 OVERSTATED** (real survivors whose stated consequence +was weaker than originally claimed) and **4 REFUTED**. **165 rows total.** +5 rows are `wont-fix` (SFU-20, SFU-21, SFU-22, UI-09, UI-10), so **160 rows require +work**, of which 6 carry a production change. + +### Reconciliation against "145" + +The ledger does **not** reconcile cleanly to 145, and the rows were not padded or +truncated to make it. The gap is almost entirely *granularity* — every row traces +to a named, independently reproduced finding in a verification report. + +| Batch | Verifier's stated count | Rows here | Reconciles? | +|---|---|---|---| +| 1 — cmd request/auth | 22 confirmed | 23 (22 CONFIRMED + 1 OVERSTATED) | **Yes.** The 22 CONFIRMED rows are mutation-level and match exactly. REQ-23 (integration suite absent from CI) is reported *outside* the 22. | +| 2 — cmd status/favorites/list | 25 real of 29 claimed | 29 (23 CONFIRMED, 3 OVERSTATED, 3 REFUTED) | **Approximately.** All 29 claimed items are listed so the three refutations stay recorded. 23 + 3 OVERSTATED = 26 actionable, one above the verifier's "25" — its own prose is imprecise about whether OUT-21/OUT-25/OUT-26 count as "real". | +| 3 — internal/sca + workflows | 31 + 4 extra = 35 | 38 (34 CONFIRMED, 3 OVERSTATED, 1 REFUTED) | **Yes.** 34 CONFIRMED + SCA-10 (a real survivor, only its truncation consequence overstated) = exactly 35 survivors. The other 3 rows are SCA-15 (X-API-Version coverage topology, which PR8 acts on) and SCA-17/SCA-18, recorded so they are not re-filed. | +| 4 — cmd root/elevate/env | 22 confirmed | 27 (26 CONFIRMED, 1 OVERSTATED) | **No — +5.** The report's headline groups its own sub-lettered mutations inconsistently: H2.1–2.3, H3.1–3.4, M4.1–2, M5.1–3, M6.1–6.4 and M9.1–3 are enumerated individually in the body, each with its own `go test ./cmd/ -count=1 → ok` transcript, but collapsed in the total. Listing them individually gives 25 production mutations plus 2 test-quality rows (ELV-26, ELV-27). | +| 5 — cache/config/ui/selfupdate | 44 reproduced, 41 actionable | 48 (all CONFIRMED, 4 of them `wont-fix`) | **Partly — +4.** Same granularity problem: M7 and M9 are two mutations each; L1, L3, L4 and L5 are three each; L9 is three survivors. Enumerating every reproduced mutation gives 48; removing the 4 unreachable/defensive `wont-fix` rows (SFU-20, SFU-21, UI-09, UI-10) gives **44 actionable**, matching "44 reproduced" but not "41 actionable" — the report never itemises which 3 it dropped. | + +**Net: 165 rows against a headline of 145.** Roughly 9 of the excess is finer +enumeration in batches 4 and 5 (mutations the reports reproduced individually but +totalled in groups); the rest is the 13 OVERSTATED/REFUTED rows the headline count +deliberately excluded but which belong here as settled questions. Nothing was +invented and nothing was dropped to hit a number. + +There are no `NEEDS-REVIEW` rows: every row's source report is unambiguous about +the mutation applied and the observed result. From 92000af2d124d5e8f47ff8e4dd13606a55dac65a Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 11:12:34 +0200 Subject: [PATCH 08/15] test: sandbox the two SDK env vars that escaped the testenv redirect IDSEC_KEYRING_FOLDER and IDSEC_FILE_LOG_PATH are absolute-path overrides that bypass the HOME fallback, so a pre-existing value in the developer's or CI environment sends SDK keyring and log writes outside the sandbox while AssertSandboxed still reports zero failures. Redirect both, force the SDK file keyring with IDSEC_BASIC_KEYRING=1 (the OS keyring is a daemon, not a path, and cannot be redirected at all), and extend AssertSandboxed to cover them. The redirect list is now asserted against an explicit literal and every entry gets a hostile pre-existing value before Run: ranging over redirectedVars itself meant a drop-one mutation survived on four of the original five entries. Run also restores the environment and removes the sandbox from a defer so a panic cannot leak either, and saves/restores sandboxRoot so a nested Run hands the outer root back. --- internal/testenv/testenv.go | 110 ++++++++++++++-- internal/testenv/testenv_test.go | 218 ++++++++++++++++++++++++++++++- 2 files changed, 311 insertions(+), 17 deletions(-) diff --git a/internal/testenv/testenv.go b/internal/testenv/testenv.go index b475320..c66866c 100644 --- a/internal/testenv/testenv.go +++ b/internal/testenv/testenv.go @@ -1,6 +1,21 @@ -// Package testenv redirects every environment variable grant uses to locate -// user state at a throwaway temporary directory, so running the test suite can -// never read or clobber the developer's real ~/.grant or ~/.idsec. +// Package testenv redirects the environment variables grant and its SDK use to +// locate user state at a throwaway temporary directory, so running the test +// suite can never read or clobber the developer's real ~/.grant or ~/.idsec. +// +// # What it does not cover +// +// The redirect is a list of known variables (see redirectedVars), not a +// containment boundary. Anything that reaches user state by another route +// escapes it: +// +// - A direct os.UserHomeDir call is covered only because HOME/USERPROFILE are +// redirected; a hardcoded path or a newly-added SDK variable is not covered +// at all, and nothing here discovers one automatically. +// - The OS keyring is a daemon, not a path, so no environment redirect can +// sandbox it. Run sets IDSEC_BASIC_KEYRING=1 to force the SDK's file +// backend into the sandboxed IDSEC_KEYRING_FOLDER instead. If a future SDK +// stops honoring that variable, keyring access leaves the sandbox again +// and nothing here will notice. // // It is a normal (non-test) package so that TestMain functions in several // packages can share it, but it is imported only from _test.go files and so @@ -11,8 +26,10 @@ // # What the assertions actually prove // // AssertSandboxed verifies that the *configured destinations* — the paths -// config.ConfigDir, config.ConfigPath, cache.CacheDir and -// profiles.GetProfilesFolder resolve to — all sit under the sandbox root. That +// config.ConfigDir, config.ConfigPath, cache.CacheDir, +// profiles.GetProfilesFolder, the SDK keyring folder and the SDK file-log path +// resolve to — all sit under the sandbox root, and that IDSEC_BASIC_KEYRING is +// set so the file keyring is the one in use. That // is all it proves. It does NOT prove that no code wrote outside the sandbox: // it cannot see a future direct os.UserHomeDir call, a hardcoded path, or a // dependency that writes via some other variable, and it cannot detect reads at @@ -41,15 +58,41 @@ import ( // HOMEDRIVE+HOMEPATH. It never consults HOME, so // redirecting HOME alone leaves the Windows CI leg // pointed at the real profile directory. -// - XDG_CONFIG_HOME — consulted by third-party config helpers. +// - XDG_CONFIG_HOME — speculative/defensive. Nothing in grant or in the +// pinned SDK reads it today (`rg XDG_` finds only this +// comment and testenv's own tests). It is redirected +// because it is the conventional escape hatch a config +// helper would reach for, and a redirect costs nothing. // - IDSEC_PROFILES_FOLDER — takes precedence over HOME in the SDK loader. // - GRANT_CONFIG — overrides config.ConfigPath. +// - IDSEC_KEYRING_FOLDER — overrides the SDK file-keyring folder outright +// (pkg/common/keyring/idsec_basic_keyring.go), bypassing +// its HOME fallback. A pre-existing value in the +// developer's or CI environment would otherwise put +// keyring writes outside the sandbox. +// - IDSEC_FILE_LOG_PATH — overrides the SDK's file-log destination +// (pkg/config, consumed in pkg/common/idsec_logger.go), +// whose parent directory the SDK MkdirAll's. +// - IDSEC_BASIC_KEYRING — not a path: any non-empty value forces the SDK's +// file keyring. Without it, a plain Linux box with +// DBUS_SESSION_BUS_ADDRESS set selects the real +// libsecret store, which no path redirect can sandbox. var redirectedVars = []string{ "HOME", "USERPROFILE", "XDG_CONFIG_HOME", "IDSEC_PROFILES_FOLDER", "GRANT_CONFIG", + "IDSEC_KEYRING_FOLDER", + "IDSEC_FILE_LOG_PATH", + "IDSEC_BASIC_KEYRING", +} + +// nonPathVars are the entries of redirectedVars whose value is a mode switch +// rather than a filesystem path, so "must resolve under the sandbox root" does +// not apply to them. +var nonPathVars = map[string]bool{ + "IDSEC_BASIC_KEYRING": true, } // sandboxRoot is the active sandbox root, or "" when Run is not executing. @@ -88,9 +131,20 @@ func Run(run func() int) int { "XDG_CONFIG_HOME": filepath.Join(home, ".config"), "IDSEC_PROFILES_FOLDER": filepath.Join(home, ".idsec", "profiles"), "GRANT_CONFIG": filepath.Join(home, ".grant", "config.yaml"), + "IDSEC_KEYRING_FOLDER": filepath.Join(home, ".idsec", "cache", "keyring"), + "IDSEC_FILE_LOG_PATH": filepath.Join(home, ".idsec", "logs", "idsec.log"), + "IDSEC_BASIC_KEYRING": "1", } + // Deferred so a panic inside run — a -race detection, a stray panic in a + // test — still restores the environment and removes the sandbox instead of + // leaking the directory and leaving the process redirected. restore := make(map[string]*string, len(redirectedVars)) + defer func() { + restoreEnv(restore) + _ = os.RemoveAll(root) + }() + for _, k := range redirectedVars { if v, ok := os.LookupEnv(k); ok { prev := v @@ -100,19 +154,17 @@ func Run(run func() int) int { } if err := os.Setenv(k, values[k]); err != nil { fmt.Fprintf(os.Stderr, "testenv: failed to set %s: %v\n", k, err) - restoreEnv(restore) - _ = os.RemoveAll(root) return 1 } } + // Save and restore rather than clearing: a nested Run must hand the outer + // run's root back, not "". + prevRoot := sandboxRoot sandboxRoot = root - code := run() - sandboxRoot = "" + defer func() { sandboxRoot = prevRoot }() - restoreEnv(restore) - _ = os.RemoveAll(root) - return code + return run() } // restoreEnv puts back the captured values; a nil entry means the variable was @@ -168,6 +220,38 @@ func AssertSandboxed(t TB) { } assertUnder(t, "profiles.GetProfilesFolder()", profiles.GetProfilesFolder(), root) + assertUnder(t, "SDK keyring folder", keyringFolder(), root) + assertUnder(t, "SDK file log path", fileLogPath(), root) + + if os.Getenv("IDSEC_BASIC_KEYRING") == "" { + t.Errorf("IDSEC_BASIC_KEYRING is empty; the SDK may select the real OS keyring, which no path redirect can sandbox") + } +} + +// keyringFolder mirrors the SDK's keyring folder resolution +// (pkg/common/keyring/idsec_basic_keyring.go NewIdsecBasicKeyring): the +// IDSEC_KEYRING_FOLDER override, else DefaultBasicKeyringFolder under HOME. +// It is reimplemented rather than called because the SDK constructor creates +// the directory as a side effect, which an assertion must not do. +func keyringFolder() string { + if folder := os.Getenv("IDSEC_KEYRING_FOLDER"); folder != "" { + return folder + } + return filepath.Join(os.Getenv("HOME"), ".idsec", "cache", "keyring") +} + +// fileLogPath mirrors the SDK's file-log resolution (pkg/common/idsec_logger.go +// resolveFileLogWriter): the IDSEC_FILE_LOG_PATH override, else a default under +// os.UserHomeDir. The SDK MkdirAll's this path's parent. +func fileLogPath() string { + if p := strings.TrimSpace(os.Getenv("IDSEC_FILE_LOG_PATH")); p != "" { + return p + } + home, err := os.UserHomeDir() + if err != nil { + return "" + } + return filepath.Join(home, ".idsec", "logs", "idsec.log") } // assertUnder reports a failure unless got is root itself or below it. diff --git a/internal/testenv/testenv_test.go b/internal/testenv/testenv_test.go index 81d1562..506058b 100644 --- a/internal/testenv/testenv_test.go +++ b/internal/testenv/testenv_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "runtime" + "slices" "strings" "testing" ) @@ -22,6 +23,33 @@ func (r *recordingTB) Errorf(format string, args ...any) { _ = args } +// wantRedirectedVars is an explicit literal, deliberately NOT derived from +// redirectedVars. The previous version of this test ranged over redirectedVars +// itself, so deleting an entry merely checked one fewer thing; a drop-one +// mutation on four of the five entries then passed the whole suite. +var wantRedirectedVars = []string{ + "HOME", + "USERPROFILE", + "XDG_CONFIG_HOME", + "IDSEC_PROFILES_FOLDER", + "GRANT_CONFIG", + "IDSEC_KEYRING_FOLDER", + "IDSEC_FILE_LOG_PATH", + "IDSEC_BASIC_KEYRING", +} + +// TestRedirectedVarsIsExactlyTheExpectedSet pins the membership of +// redirectedVars. Adding a variable to the production list without adding it +// here is a deliberate speed bump: the new entry needs a hostile-value case in +// TestRun_OverridesPreExistingHostileValues too. +// +// Not parallel: kept serial with the rest of the file. +func TestRedirectedVarsIsExactlyTheExpectedSet(t *testing.T) { + if !slices.Equal(redirectedVars, wantRedirectedVars) { + t.Errorf("redirectedVars = %q, want exactly %q", redirectedVars, wantRedirectedVars) + } +} + // Not parallel: mutates process-wide environment variables. func TestRun_RedirectsAllHomeEnvVars(t *testing.T) { var ( @@ -31,7 +59,7 @@ func TestRun_RedirectsAllHomeEnvVars(t *testing.T) { code := Run(func() int { gotRoot = Root() - for _, k := range redirectedVars { + for _, k := range wantRedirectedVars { seen[k] = os.Getenv(k) } return 7 @@ -44,18 +72,133 @@ func TestRun_RedirectsAllHomeEnvVars(t *testing.T) { t.Fatal("Root() was empty inside run()") } - for _, k := range redirectedVars { + for _, k := range wantRedirectedVars { v := seen[k] if v == "" { t.Errorf("%s was empty inside run(); every redirected var must be set", k) continue } + if nonPathVars[k] { + continue // a mode switch, not a path: nothing to locate under the root + } if !strings.HasPrefix(v, gotRoot) { t.Errorf("%s = %q, want a path under the sandbox root %q", k, v, gotRoot) } } } +// TestRun_OverridesPreExistingHostileValues is the case that gives each entry +// in redirectedVars its own failing signal. Every var is pre-set to a path +// outside any sandbox before Run — exactly the state a developer or CI runner +// with the variable already exported is in — and each must come back +// overridden. With HOME redirected the other vars' *fallbacks* already land +// in-sandbox, so a pre-existing value is the only thing that distinguishes a +// redirected var from an unredirected one. +// +// Not parallel: mutates process-wide environment variables. +func TestRun_OverridesPreExistingHostileValues(t *testing.T) { + hostile := t.TempDir() // outside any sandbox root, by construction + + for _, k := range wantRedirectedVars { + t.Setenv(k, filepath.Join(hostile, strings.ToLower(k))) + } + + seen := map[string]string{} + var gotRoot string + Run(func() int { + gotRoot = Root() + for _, k := range wantRedirectedVars { + seen[k] = os.Getenv(k) + } + return 0 + }) + + if gotRoot == "" { + t.Fatal("Root() was empty inside run()") + } + + for _, k := range wantRedirectedVars { + got := seen[k] + if strings.HasPrefix(got, hostile) { + t.Errorf("%s = %q inside the sandbox; Run did not override the pre-existing value", k, got) + continue + } + if nonPathVars[k] { + if got == "" { + t.Errorf("%s was empty inside the sandbox, want a non-empty override", k) + } + continue + } + if !strings.HasPrefix(got, gotRoot) { + t.Errorf("%s = %q, want a path under the sandbox root %q", k, got, gotRoot) + } + } +} + +// TestAssertSandboxed_FailsOnAHostileKeyringFolder is the concrete escape the +// package comment used to deny: IDSEC_KEYRING_FOLDER overrides the file-keyring +// folder outright, bypassing the HOME fallback, so before it joined +// redirectedVars the SDK created a keyring folder outside the sandbox while +// AssertSandboxed reported zero failures. +// +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsOnAHostileKeyringFolder(t *testing.T) { + escapee := t.TempDir() + + Run(func() int { + //nolint:usetesting // t.Setenv's cleanup would fire after Run restores. + if err := os.Setenv("IDSEC_KEYRING_FOLDER", escapee); err != nil { + t.Errorf("Setenv: %v", err) + return 1 + } + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) != 1 { + t.Errorf("AssertSandboxed reported %d failures, want exactly 1 (the escaped keyring folder): %v", + len(rec.errs), rec.errs) + } + return 0 + }) +} + +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsOnAHostileFileLogPath(t *testing.T) { + escapee := filepath.Join(t.TempDir(), "idsec.log") + + Run(func() int { + //nolint:usetesting // t.Setenv's cleanup would fire after Run restores. + if err := os.Setenv("IDSEC_FILE_LOG_PATH", escapee); err != nil { + t.Errorf("Setenv: %v", err) + return 1 + } + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) != 1 { + t.Errorf("AssertSandboxed reported %d failures, want exactly 1 (the escaped file log path): %v", + len(rec.errs), rec.errs) + } + return 0 + }) +} + +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsWhenBasicKeyringIsNotForced(t *testing.T) { + Run(func() int { + //nolint:usetesting // t.Setenv's cleanup would fire after Run restores. + if err := os.Setenv("IDSEC_BASIC_KEYRING", ""); err != nil { + t.Errorf("Setenv: %v", err) + return 1 + } + rec := &recordingTB{} + AssertSandboxed(rec) + if len(rec.errs) != 1 { + t.Errorf("AssertSandboxed reported %d failures, want exactly 1 (basic keyring not forced): %v", + len(rec.errs), rec.errs) + } + return 0 + }) +} + // Not parallel: mutates process-wide environment variables. func TestRun_RestoresPreviousEnvironment(t *testing.T) { const sentinel = "/sentinel-home-value" @@ -222,8 +365,8 @@ func TestAssertUnder(t *testing.T) { // Not parallel: mutates process-wide environment variables. func TestRun_IsReentrant(t *testing.T) { - // Nested/sequential calls must each get their own root and leave the - // process environment as they found it. + // Sequential calls must each get their own root. Nesting is covered + // separately by TestRun_NestsWithoutClobberingTheOuterRoot. var first, second string Run(func() int { first = Root() @@ -240,3 +383,70 @@ func TestRun_IsReentrant(t *testing.T) { t.Errorf("both runs used the same sandbox root %q; each run must be isolated", first) } } + +// TestRun_NestsWithoutClobberingTheOuterRoot pins the save/restore of +// sandboxRoot. Before it, an inner Run cleared the global on the way out and +// the outer run continued with Root() == "", which makes AssertSandboxed report +// "no sandbox is active" even though one is. +// +// Not parallel: mutates process-wide environment variables. +func TestRun_NestsWithoutClobberingTheOuterRoot(t *testing.T) { + var outer, inner, afterInner string + + Run(func() int { + outer = Root() + Run(func() int { + inner = Root() + return 0 + }) + afterInner = Root() + return 0 + }) + + if outer == "" || inner == "" { + t.Fatalf("Root() empty: outer=%q inner=%q", outer, inner) + } + if outer == inner { + t.Errorf("nested Run reused the outer root %q; each run must get its own", outer) + } + if afterInner != outer { + t.Errorf("Root() = %q after the nested Run returned, want the outer root %q", afterInner, outer) + } + if Root() != "" { + t.Errorf("Root() = %q after both runs, want empty", Root()) + } +} + +// TestRun_RestoresEnvironmentAfterAPanic pins the deferred cleanup. Without it +// a panic inside run — a -race detection, or any stray panic — skipped both the +// environment restore and the sandbox removal, so the process kept running with +// a redirected HOME pointing at a directory that still existed. +// +// Not parallel: mutates process-wide environment variables. +func TestRun_RestoresEnvironmentAfterAPanic(t *testing.T) { + const sentinel = "/sentinel-home-value" + t.Setenv("HOME", sentinel) + + var root string + func() { + defer func() { + if recover() == nil { + t.Error("expected the panic to propagate out of Run") + } + }() + Run(func() int { + root = Root() + panic("boom") + }) + }() + + if got := os.Getenv("HOME"); got != sentinel { + t.Errorf("HOME = %q after a panicking Run, want the pre-existing value %q", got, sentinel) + } + if Root() != "" { + t.Errorf("Root() = %q after a panicking Run, want empty", Root()) + } + if _, err := os.Stat(root); !os.IsNotExist(err) { + t.Errorf("sandbox root %q survived a panicking Run (stat err = %v)", root, err) + } +} From 453efdb916ae3177222c2191ff93a3bb0257bcab Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 11:12:40 +0200 Subject: [PATCH 09/15] test: extend the sandbox to the packages that build real SDK services internal/sca, internal/workflows and internal/sdkclient drive the real service constructors in their retry-policy tests but had no TestMain, so a new test there could reach real user state with no failing signal. They can hold an in-package TestMain because testenv imports internal/sca/models, not internal/sca. --- internal/sca/main_test.go | 28 ++++++++++++++++++++++++++++ internal/sdkclient/main_test.go | 24 ++++++++++++++++++++++++ internal/workflows/main_test.go | 25 +++++++++++++++++++++++++ 3 files changed, 77 insertions(+) create mode 100644 internal/sca/main_test.go create mode 100644 internal/sdkclient/main_test.go create mode 100644 internal/workflows/main_test.go diff --git a/internal/sca/main_test.go b/internal/sca/main_test.go new file mode 100644 index 0000000..4b61414 --- /dev/null +++ b/internal/sca/main_test.go @@ -0,0 +1,28 @@ +package sca + +import ( + "os" + "testing" + + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain redirects HOME and friends at a throwaway directory. This package +// drives the *real* service constructors (see retry_policy_test.go), which +// build an SDK client and can therefore reach the SDK's profile and keyring +// resolution; without the sandbox a new test here could read or write the +// developer's real ~/.idsec with no failing signal. +// +// An in-package (not _test) TestMain is safe here: testenv imports +// internal/sca/models, not internal/sca, so there is no import cycle. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(m.Run)) +} + +// TestSandboxIsolation pins the redirect: if TestMain ever stops wrapping +// m.Run, this fails instead of the suite silently touching real user state. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} diff --git a/internal/sdkclient/main_test.go b/internal/sdkclient/main_test.go new file mode 100644 index 0000000..00c655c --- /dev/null +++ b/internal/sdkclient/main_test.go @@ -0,0 +1,24 @@ +package sdkclient + +import ( + "os" + "testing" + + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain redirects HOME and friends at a throwaway directory. This package +// constructs real SDK clients, which can reach the SDK's profile and keyring +// resolution; without the sandbox a new test here could read or write the +// developer's real ~/.idsec with no failing signal. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(m.Run)) +} + +// TestSandboxIsolation pins the redirect: if TestMain ever stops wrapping +// m.Run, this fails instead of the suite silently touching real user state. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} diff --git a/internal/workflows/main_test.go b/internal/workflows/main_test.go new file mode 100644 index 0000000..bb1913f --- /dev/null +++ b/internal/workflows/main_test.go @@ -0,0 +1,25 @@ +package workflows + +import ( + "os" + "testing" + + "github.com/aaearon/grant-cli/internal/testenv" +) + +// TestMain redirects HOME and friends at a throwaway directory. This package +// drives the *real* AccessRequestService constructor (see retry_policy_test.go), +// which builds an SDK client and can therefore reach the SDK's profile and +// keyring resolution; without the sandbox a new test here could read or write +// the developer's real ~/.idsec with no failing signal. +func TestMain(m *testing.M) { + os.Exit(testenv.Run(m.Run)) +} + +// TestSandboxIsolation pins the redirect: if TestMain ever stops wrapping +// m.Run, this fails instead of the suite silently touching real user state. +// +// Not parallel: reads process-wide environment state. +func TestSandboxIsolation(t *testing.T) { + testenv.AssertSandboxed(t) +} From 5c58cb7ffb60012be6a92a737b378ca526a8c3d3 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 11:12:47 +0200 Subject: [PATCH 10/15] test: tighten the version assertion and restore verbose alongside outputFormat The integration version check accepted `contains("dev") || contains("unknown")`, and a non-ldflags build always prints "commit: unknown", so the second arm made it true for any version string. Assert "grant version dev" instead. The cmd test helpers restored only outputFormat, but newRootCommand binds verbose to a package global through the same mechanism. Restore both. isolatedEnv also passes the SDK keyring/log overrides down to the child process, which inherits the parent environment. --- cmd/integration_test.go | 15 +++++++++++++-- cmd/test_helpers.go | 28 ++++++++++++++++++---------- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/cmd/integration_test.go b/cmd/integration_test.go index 9fef25a..5604c3d 100644 --- a/cmd/integration_test.go +++ b/cmd/integration_test.go @@ -127,6 +127,14 @@ func isolatedEnv(t *testing.T) []string { "USERPROFILE=" + dir, "GRANT_CONFIG=" + filepath.Join(dir, "config.yaml"), "IDSEC_PROFILES_FOLDER=" + filepath.Join(dir, "profiles"), + // The SDK reads these two absolute-path overrides directly, bypassing + // the HOME fallback, and forces its file keyring on any non-empty + // IDSEC_BASIC_KEYRING. Without them a pre-existing value in the + // developer's or CI environment sends the subprocess's keyring and log + // writes outside the sandbox. + "IDSEC_KEYRING_FOLDER=" + filepath.Join(dir, "keyring"), + "IDSEC_FILE_LOG_PATH=" + filepath.Join(dir, "logs", "idsec.log"), + "IDSEC_BASIC_KEYRING=1", } } @@ -191,8 +199,11 @@ func TestIntegration_Version(t *testing.T) { } } // The integration binary is built without -ldflags, so the version stays - // at its compiled-in default. - if !got.contains("dev") && !got.contains("unknown") { + // at its compiled-in default. Assert the version line specifically: the + // binary always prints "commit: unknown" for a non-ldflags build, so an + // `|| contains("unknown")` arm would make this assertion true regardless + // of the version string. + if !got.contains("grant version dev") { t.Errorf("expected a dev build banner, got:\n%s", got.output) } } diff --git a/cmd/test_helpers.go b/cmd/test_helpers.go index 2da550b..45d4f47 100644 --- a/cmd/test_helpers.go +++ b/cmd/test_helpers.go @@ -25,7 +25,7 @@ func newNoOpCommand() *cobra.Command { // When SilenceErrors is true, error text is appended to the output buffer // to match production behavior (where Execute() prints the error). func executeCommand(cmd *cobra.Command, args ...string) (string, error) { - defer restoreOutputFormat(outputFormat) + defer restoreCommandGlobals(outputFormat, verbose) buf := new(bytes.Buffer) cmd.SetOut(buf) @@ -39,13 +39,21 @@ func executeCommand(cmd *cobra.Command, args ...string) (string, error) { return buf.String(), err } -// restoreOutputFormat puts the package-global outputFormat back. Cobra binds -// --output to that global with StringVarP, so executing any command that -// carries the flag leaves the global set for every later test. A command built -// without the root flag set (NewEnvCommandWithDeps, for instance) then inherits -// "json" from whichever test ran before it — an order dependence that only -// shows up under `go test -shuffle=on`. -func restoreOutputFormat(saved string) { outputFormat = saved } +// restoreCommandGlobals puts back the two package-globals that newRootCommand +// binds to persistent flags: outputFormat (--output, StringVarP) and verbose +// (--verbose, BoolVarP). Executing any command carrying those flags leaves both +// globals set for every later test, and a command built without the root flag +// set (NewEnvCommandWithDeps, for instance) then inherits the previous test's +// values — an order dependence that only shows up under `go test -shuffle=on`. +// +// outputFormat is the load-bearing half: making this a no-op fails on 5 of 8 +// fixed shuffle seeds. verbose is latent today, because pflag rewrites it to +// the registered default whenever newRootCommand runs, but it is the same leak +// through the same mechanism and is restored for the same reason. +func restoreCommandGlobals(savedOutput string, savedVerbose bool) { + outputFormat = savedOutput + verbose = savedVerbose +} // executeCommandStreams executes a command keeping stdout and stderr apart and // writing no error text into either. Use it when a test needs to assert on the @@ -53,7 +61,7 @@ func restoreOutputFormat(saved string) { outputFormat = saved } // executeCommand cannot express because it merges the streams and appends the // error text. func executeCommandStreams(cmd *cobra.Command, args ...string) (stdout, stderr string, err error) { - defer restoreOutputFormat(outputFormat) + defer restoreCommandGlobals(outputFormat, verbose) var outBuf, errBuf bytes.Buffer cmd.SetOut(&outBuf) @@ -67,7 +75,7 @@ func executeCommandStreams(cmd *cobra.Command, args ...string) (stdout, stderr s // executeWithHint simulates Execute() logic without os.Exit, returning the error output. // Used for testing the verbose hint behavior. func executeWithHint(cmd *cobra.Command, args []string) string { - defer restoreOutputFormat(outputFormat) + defer restoreCommandGlobals(outputFormat, verbose) passedArgValidation = false cmd.SetArgs(args) From 063c0c586c2684d6a304f19fd439c6d58aebc6ab Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 11:12:54 +0200 Subject: [PATCH 11/15] docs: close PR1's ledger rows and record the test-isolation contract REQ-23 and OUT-26 are the ledger's own PR1 rows; leaving them todo broke the rule the ledger introduced. Both are flipped to done with the mutation evidence that closes them. CLAUDE.md's Test Isolation section now states what testenv covers, what it cannot cover (the OS keyring is a daemon, not a path), and why XDG_CONFIG_HOME is defensive rather than load-bearing. --- CLAUDE.md | 13 +++++++++---- docs/mutation-ledger.md | 43 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 19aea1d..730b9e7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -295,16 +295,21 @@ There is no `getAuth`/`getSCAService`; those never existed. Every test that swap ### Test Isolation -`internal/testenv` is a normal package imported only from `_test.go` files, so it never links into the binary. `testenv.Run(m.Run)` redirects `HOME`, `USERPROFILE`, `XDG_CONFIG_HOME`, `IDSEC_PROFILES_FOLDER` and `GRANT_CONFIG` under one temp root before `m.Run`, then restores them. +`internal/testenv` is a normal package imported only from `_test.go` files, so it never links into the binary. `testenv.Run(m.Run)` redirects eight variables under one temp root before `m.Run`, then restores them: `HOME`, `USERPROFILE`, `XDG_CONFIG_HOME`, `IDSEC_PROFILES_FOLDER`, `GRANT_CONFIG`, `IDSEC_KEYRING_FOLDER`, `IDSEC_FILE_LOG_PATH` and `IDSEC_BASIC_KEYRING`. - `USERPROFILE` is not optional: Go's Windows `os.UserHomeDir` reads `USERPROFILE`, then `HOMEDRIVE`+`HOMEPATH`, and never `HOME`. `HOME` alone leaves the Windows CI leg pointed at the real profile. The SDK profile loader, conversely, reads `HOME` on every platform. - `GRANT_CONFIG` does **not** cover the cache: `cache.CacheDir()` → `config.ConfigDir()` → `os.UserHomeDir()`. Before this existed the suite wrote the developer's real `~/.grant/cache/session_timestamps.json` on every run. +- `IDSEC_KEYRING_FOLDER` and `IDSEC_FILE_LOG_PATH` are **absolute-path overrides that bypass the `HOME` fallback entirely** (`pkg/common/keyring/idsec_basic_keyring.go`, `pkg/common/idsec_logger.go`, which `MkdirAll`s the log's parent). A value already exported in the developer's or CI environment sends those writes outside the sandbox no matter what `HOME` says. +- `IDSEC_BASIC_KEYRING=1` is set because the OS keyring is a **daemon, not a path**, so no redirect can sandbox it: on a non-WSL Linux box with `DBUS_SESSION_BUS_ADDRESS` set, `GetKeyring` picks the real libsecret store. Forcing the file backend puts keyring state into the sandboxed folder instead. If a future SDK stops honoring the variable, this containment is gone and nothing here detects it. +- `XDG_CONFIG_HOME` is **speculative/defensive** — nothing in grant or the pinned SDK reads it (`rg XDG_` finds only testenv's own comment and tests). It is redirected because it is the conventional escape hatch and costs nothing. - `testenv` must not import `testing`; `AssertSandboxed` therefore takes a `TB` interface (`Helper`/`Errorf`) that `*testing.T` satisfies. -- `AssertSandboxed` checks the *configured destinations* — `config.ConfigDir`, `config.ConfigPath`, `cache.CacheDir`, `profiles.GetProfilesFolder`. It does not prove nothing was written outside the sandbox, and cannot see reads. A snapshot-diff gate was considered and rejected: a concurrently running real `grant` false-positives with certainty, and size+mtime misses same-size rewrites. -- `TestMain` lives in `cmd/main_test.go` (`//go:build !integration`, because `cmd/integration_test.go` declares its own), `internal/config/main_test.go` and `internal/cache/main_test.go`. The last two are in the **external** test package (`config_test`/`cache_test`) because `testenv` imports those packages — an in-package test file importing it would be an import cycle. +- `AssertSandboxed` checks the *configured destinations* — `config.ConfigDir`, `config.ConfigPath`, `cache.CacheDir`, `profiles.GetProfilesFolder`, the SDK keyring folder and the SDK file-log path — plus that `IDSEC_BASIC_KEYRING` is non-empty. The last two resolvers are **reimplemented** in testenv rather than called, because the SDK constructor creates the directory as a side effect and an assertion must not write. It does not prove nothing was written outside the sandbox, and cannot see reads. A snapshot-diff gate was considered and rejected: a concurrently running real `grant` false-positives with certainty, and size+mtime misses same-size rewrites. +- The redirect list is validated against an **explicit literal** in `testenv_test.go`, and every entry additionally gets a hostile pre-existing value before `Run` in `TestRun_OverridesPreExistingHostileValues`. Ranging over `redirectedVars` itself is the trap this replaced: dropping an entry merely checked one fewer thing, and four of the original five survived a drop-one mutation because with `HOME` redirected their *fallbacks* already landed in-sandbox. A var's whole value is defending against a pre-existing value, so that is what the test must supply. +- `Run` restores the environment and removes the sandbox from a **`defer`**, so a panic inside `m.Run` (a `-race` detection, a stray panic) cannot leak the directory or leave the process redirected. `sandboxRoot` is saved and restored rather than cleared, so a nested `Run` hands the outer root back. +- `TestMain` lives in `cmd/main_test.go` (`//go:build !integration`, because `cmd/integration_test.go` declares its own), `internal/config/main_test.go`, `internal/cache/main_test.go`, `internal/sca/main_test.go`, `internal/workflows/main_test.go` and `internal/sdkclient/main_test.go`. The `config`/`cache` ones are in the **external** test package (`config_test`/`cache_test`) because `testenv` imports those packages — an in-package test file importing it would be an import cycle. `sca`/`workflows`/`sdkclient` can be in-package: `testenv` imports `internal/sca/models`, not `internal/sca`. Those three earn a `TestMain` because their retry-policy tests drive the **real** service constructors. - `os.Setenv` in `TestMain` runs before `m.Run`, so it does not collide with the `t.Parallel()` sites in `internal/` — unlike `t.Setenv`, which the stdlib forbids in parallel tests. - `cmd/bootstrap_stub_test.go` (untagged, so both build configurations get it) points `bootstrapImpl` at `errTestBootstrapDisabled`. Assert it with `errors.Is`, never a bare `wantErr: true` — otherwise a stray bootstrap attempt silently satisfies an unrelated case. `recordSessionTimestamp` is deliberately left live: it is what proves the redirect works. -- `executeCommand`/`executeCommandStreams`/`executeWithHint` restore the package-global `outputFormat`, which Cobra binds to `--output`. Without that a command built outside the root (e.g. `NewEnvCommandWithDeps`) inherits `json` from a previous test — an order dependence `go test -shuffle=on` exposes. +- `executeCommand`/`executeCommandStreams`/`executeWithHint` call `restoreCommandGlobals` to put back **both** globals Cobra binds to persistent flags: `outputFormat` (`--output`) and `verbose` (`--verbose`). Without that a command built outside the root (e.g. `NewEnvCommandWithDeps`) inherits the previous test's values — an order dependence `go test -shuffle=on` exposes. `outputFormat` is the load-bearing half (a no-op restore fails 5 of 8 fixed shuffle seeds); `verbose` is latent only because pflag rewrites it at registration. ### Testing Patterns diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index fe92bc9..a33b180 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -54,7 +54,7 @@ premise does not hold). | REQ-20 | cmd/login | `cmd/login.go:52` | `if profile == nil {` → `if false {` (auto-configure branch). Feature *is* implemented; `login_test.go` skips it with the factually wrong reason "Auto-configure not yet implemented" — delete the skip | CONFIRMED | test | `TestRunLogin_AutoConfiguresMissingProfile` | PR4 | todo | | REQ-21 | cmd/login | `cmd/login.go:74` | `auth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, true)` → `..., true, false)` (swap `force`/`refreshAuth`) | CONFIRMED | test | `TestRunLogin_AuthenticateFlags` | PR4 | todo | | REQ-22 | cmd/request submit | `cmd/request_submit.go:251` | `if !ui.IsInteractive() {` → `if false {` inside the `roleID == ""` branch | CONFIRMED | test | `TestRunRequestSubmit_NonInteractiveRequiresRoleID` | PR4 | todo | -| REQ-23 | cmd integration suite | `cmd/integration_test.go` (harness, not a production site) | No mutation. Claim was "integration tests are absent from CI **and** every assertion accepts a panic". First half confirmed (`.github/workflows/ci.yml` runs `make test-race` / `go test -race`, never `-tags=integration`); second half is too broad — only line 153 accepts any panic; lines 71/125/235 require specific output | OVERSTATED | test | `TestMain` isolation + exact exit-code/error-text assertions; add `-tags=integration` to both CI legs | PR1 | todo | +| REQ-23 | cmd integration suite | `cmd/integration_test.go` (harness, not a production site) | No mutation. Claim was "integration tests are absent from CI **and** every assertion accepts a panic". First half confirmed (`.github/workflows/ci.yml` runs `make test-race` / `go test -race`, never `-tags=integration`); second half is too broad — only line 153 accepts any panic; lines 71/125/235 require specific output | OVERSTATED | test | `TestMain` isolation + exact exit-code/error-text assertions; add `-tags=integration` to both CI legs | PR1 | done | | OUT-01 | cmd/list flags | `cmd/list.go:73` | Delete `cmd.MarkFlagsMutuallyExclusive("groups", "provider")`. `TestListCommand_MutualExclusivity` passes today on the *unrelated* runtime error `no eligible targets or groups found` | CONFIRMED | test | `TestListCommand_MutualExclusivity` (assert Cobra's `[groups provider] were all set`) | PR5 | todo | | OUT-02 | cmd/favorites (interactive) | `cmd/favorites.go:245` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `selectFavoriteInteractive` | CONFIRMED | test | `TestFavoritesAddInteractive_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | | OUT-03 | cmd/favorites (group add) | `cmd/favorites.go:375` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `addGroupFavorite` | CONFIRMED | test | `TestAddGroupFavorite_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | @@ -80,7 +80,7 @@ premise does not hold). | OUT-23 | cmd/status (test quality) | `cmd/status.go:185-192` (`computeRemainingTime`) | No production defect. `TestStatusCommand_RemainingTime/text_output_shows_remaining_time` asserts `remaining: 4` as a substring, which `remaining: 4h 30m` satisfies — only the JSON sibling killed a sixfold arithmetic error. Signal-poor assertion, not an uncovered defect | CONFIRMED | test | Tighten the text subtest to an exact `remaining: 45m` | PR5 | todo | | OUT-24 | cmd/favorites | `cmd/favorites.go:419-420` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | todo | | OUT-25 | cmd/list flags | `cmd/list.go:71` | Delete `cmd.Flags().Bool("refresh", ...)` registration. **Verifier correction:** `grant list --refresh` is **not** already a no-op — `list.go:91-92` reads it and passes it into `buildCachedLister`, and CLAUDE.md is correct. The real finding is missing flag-registration/wiring coverage | OVERSTATED | test | `TestListCommand_RefreshBypassesCache` | PR5 | todo | -| OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | todo | +| OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | done | | OUT-27 | cmd/favorites | `cmd/favorites.go:235-237` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | refuted | n/a — already killed | — | todo | | OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | | OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | @@ -277,3 +277,42 @@ invented and nothing was dropped to hit a number. There are no `NEEDS-REVIEW` rows: every row's source report is unambiguous about the mutation applied and the observed result. + +--- + +## PR1 closure evidence + +Both PR1 rows are `done`. Neither has a production mutation of its own (both are +harness rows), so each is closed against the mutation that its new assertion is +supposed to kill. + +**REQ-23** — mutation: `cmd/version.go:31`, `v = "dev"` → `v = "bogus"`. + +| assertion | result | +|---|---| +| old, `contains("dev") \|\| contains("unknown")` | `ok` — **survived**; `commit: unknown` satisfies the second arm regardless of the version string | +| new, `contains("grant version dev")` | `FAIL: expected a dev build banner, got: grant version bogus` | + +Reverted; `go test -tags=integration ./cmd -count=1 -run TestIntegration_Version` → `ok`. +The other half of the row (integration tests absent from CI) is closed by the +`Integration tests` step running `go test -tags=integration ./cmd` on both CI legs. + +**OUT-26** — mutation: `cmd/main_test.go` `TestMain`, unwrap `testenv.Run` so it +calls `installBootstrapStub(); os.Exit(m.Run())` directly. + +``` +--- FAIL: TestSandboxIsolation (0.00s) + main_test.go:40: testenv.AssertSandboxed called outside testenv.Run; no sandbox is active +--- FAIL: TestCacheDirResolvesInsideSandbox (0.00s) + main_test.go:56: no testenv sandbox is active; TestMain is not wrapping m.Run +``` + +Reverted; both tests `ok`. The row's second half (the `favorites add` +non-interactive guard) was mutation-verified when it landed. + +**Redirect-list drop-one.** Every entry of `testenv.redirectedVars` was deleted +in turn and `go test ./cmd ./internal/config ./internal/cache ./internal/testenv +-count=1` run. Before the explicit-literal and hostile-value tests, four of five +survived (`USERPROFILE`, `XDG_CONFIG_HOME`, `IDSEC_PROFILES_FOLDER`, +`GRANT_CONFIG` — with `HOME` redirected their fallbacks already land in-sandbox). +After, all eight fail. From d90deefa9972a13792cd3c21dde31e741e7ff8a8 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 12:20:11 +0200 Subject: [PATCH 12/15] ci: run on every pull request, not only those targeting main --- .github/workflows/ci.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9b04902..da23328 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,8 +3,10 @@ name: CI on: push: branches: [main] + # No branch filter: a pull_request trigger scoped to [main] silently skips CI + # on stacked PRs (feature branch -> feature branch), which is exactly when a + # green signal matters most. Run on every PR regardless of its base. pull_request: - branches: [main] permissions: contents: read From 06a5d8e33dbf21c3fb50d9db50af1c549ff56de4 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:23:01 +0200 Subject: [PATCH 13/15] test(testenv): make every sandbox assertion killable, unset SDK behavior vars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of AssertSandboxed's six resolver checks — config.ConfigDir, config.ConfigPath and cache.CacheDir — could each be deleted outright with the whole suite still green. The existing failure tests only asserted len(errs) > 0, which any other assertion in the function satisfies. Add two cases that pin the failure COUNT and which resolver reported it: GRANT_CONFIG outside the sandbox isolates ConfigPath (exactly 1), and HOME+USERPROFILE outside isolates ConfigDir and CacheDir (exactly 2, since CacheDir delegates to ConfigDir -> os.UserHomeDir). Setting both home vars keeps the case real on the Windows leg instead of skipping it. recordingTB now stores the formatted message, because the resolver name is an argument rather than part of the format string. Also unset IDSEC_PROFILE and DEPLOY_ENV for the duration of Run. Both are read by non-test SDK code — the profile loader picks the default profile name from the first, isp.FromISPAuth resolves the tenant environment from the second — and neither has a sane sandbox value, so absent is the only safe state. Restoration preserves the set-vs-unset distinction exactly. Finally, pass GOENV through to the sandboxed integration build: the child `go build` resolves its env file via os.UserConfigDir, which the redirect points at an empty sandbox on Linux while Windows reads an unredirected %AppData%, so the legs disagreed and any `go env -w` setting was dropped. --- cmd/integration_test.go | 9 ++- internal/testenv/testenv.go | 46 +++++++++-- internal/testenv/testenv_test.go | 135 ++++++++++++++++++++++++++++++- 3 files changed, 182 insertions(+), 8 deletions(-) diff --git a/cmd/integration_test.go b/cmd/integration_test.go index 5604c3d..11a1ea6 100644 --- a/cmd/integration_test.go +++ b/cmd/integration_test.go @@ -26,10 +26,17 @@ var testBinary string // and GOMODCACHE default to locations under the user's home: without this the // build inside the sandbox would start from an empty module cache and need the // network. +// +// GOENV is in the list for the same reason and one more. The child `go build` +// locates its env file through os.UserConfigDir: $XDG_CONFIG_HOME/go/env on +// Linux — which the redirect points at an empty sandbox — but %AppData%\go\env +// on Windows, which is not redirected at all. Without pinning GOENV the two CI +// legs resolve different files, and any `go env -w GOPROXY=…` / `GOFLAGS` / +// `GOPRIVATE` the developer or runner configured is silently dropped on Linux. var goEnvPassthrough []string func TestMain(m *testing.M) { - goEnvPassthrough = resolveGoEnv("GOCACHE", "GOMODCACHE", "GOPATH") + goEnvPassthrough = resolveGoEnv("GOCACHE", "GOMODCACHE", "GOPATH", "GOENV") os.Exit(testenv.Run(func() int { dir, err := os.MkdirTemp("", "grant-integration-bin-") diff --git a/internal/testenv/testenv.go b/internal/testenv/testenv.go index c66866c..8507e55 100644 --- a/internal/testenv/testenv.go +++ b/internal/testenv/testenv.go @@ -4,7 +4,8 @@ // // # What it does not cover // -// The redirect is a list of known variables (see redirectedVars), not a +// The redirect is a list of known variables (see redirectedVars, plus +// unsetVars for the ones that are removed rather than pointed elsewhere), not a // containment boundary. Anything that reaches user state by another route // escapes it: // @@ -88,6 +89,27 @@ var redirectedVars = []string{ "IDSEC_BASIC_KEYRING", } +// unsetVars lists environment variables Run REMOVES for the duration of the +// run instead of redirecting. They select behavior rather than a location, so +// there is no sandbox value to point them at — the only safe state is absent, +// which is what CI has and what a developer's shell may not: +// +// - IDSEC_PROFILE — the SDK profile loader +// (pkg/profiles/idsec_profile_loader.go) reads it to choose the DEFAULT +// PROFILE NAME. A developer with it exported runs the suite against a +// different profile than CI does, from the same source tree. +// - DEPLOY_ENV — feeds tenant-environment resolution inside +// isp.FromISPAuth (pkg/common/isp/idsec_isp_service_client.go), which the +// internal/sca and internal/workflows retry-policy tests now drive for +// real, so an exported value changes what those constructors resolve. +// +// Restoration is exact: a variable that was set comes back with its original +// value, one that was unset stays unset. +var unsetVars = []string{ + "IDSEC_PROFILE", + "DEPLOY_ENV", +} + // nonPathVars are the entries of redirectedVars whose value is a mode switch // rather than a filesystem path, so "must resolve under the sandbox root" does // not apply to them. @@ -139,25 +161,39 @@ func Run(run func() int) int { // Deferred so a panic inside run — a -race detection, a stray panic in a // test — still restores the environment and removes the sandbox instead of // leaking the directory and leaving the process redirected. - restore := make(map[string]*string, len(redirectedVars)) + restore := make(map[string]*string, len(redirectedVars)+len(unsetVars)) defer func() { restoreEnv(restore) _ = os.RemoveAll(root) }() - for _, k := range redirectedVars { + // capture records k's current state so restoreEnv can put it back exactly, + // preserving the set-with-value / unset distinction. + capture := func(k string) { if v, ok := os.LookupEnv(k); ok { prev := v restore[k] = &prev - } else { - restore[k] = nil + return } + restore[k] = nil + } + + for _, k := range redirectedVars { + capture(k) if err := os.Setenv(k, values[k]); err != nil { fmt.Fprintf(os.Stderr, "testenv: failed to set %s: %v\n", k, err) return 1 } } + for _, k := range unsetVars { + capture(k) + if err := os.Unsetenv(k); err != nil { + fmt.Fprintf(os.Stderr, "testenv: failed to unset %s: %v\n", k, err) + return 1 + } + } + // Save and restore rather than clearing: a nested Run must hand the outer // run's root back, not "". prevRoot := sandboxRoot diff --git a/internal/testenv/testenv_test.go b/internal/testenv/testenv_test.go index 506058b..76df230 100644 --- a/internal/testenv/testenv_test.go +++ b/internal/testenv/testenv_test.go @@ -1,6 +1,7 @@ package testenv import ( + "fmt" "os" "path/filepath" "runtime" @@ -19,8 +20,31 @@ type recordingTB struct { func (r *recordingTB) Helper() { r.helperCalls++ } func (r *recordingTB) Errorf(format string, args ...any) { - r.errs = append(r.errs, format) - _ = args + // Formatted, not the raw format string: the resolver name is an *argument* + // of assertUnder's message, so only the formatted text says WHICH assertion + // failed — and naming the failing assertion is what makes each block in + // AssertSandboxed individually killable. + r.errs = append(r.errs, fmt.Sprintf(format, args...)) +} + +// assertFailedResolvers requires exactly len(want) recorded failures, one +// matching each want substring. The COUNT is the load-bearing half: a bare +// "at least one failure" is satisfied by any *other* assertion inside +// AssertSandboxed, so the block under test could be deleted outright and the +// test would still pass. +func assertFailedResolvers(t *testing.T, rec *recordingTB, want ...string) { + t.Helper() + + if len(rec.errs) != len(want) { + t.Errorf("AssertSandboxed reported %d failures, want exactly %d (%v): %v", + len(rec.errs), len(want), want, rec.errs) + return + } + for _, w := range want { + if !slices.ContainsFunc(rec.errs, func(e string) bool { return strings.Contains(e, w) }) { + t.Errorf("no reported failure mentions %q; got %v", w, rec.errs) + } + } } // wantRedirectedVars is an explicit literal, deliberately NOT derived from @@ -50,6 +74,61 @@ func TestRedirectedVarsIsExactlyTheExpectedSet(t *testing.T) { } } +// wantUnsetVars pins the unset list for the same reason wantRedirectedVars +// pins the redirect list: an explicit literal, deliberately not derived from +// the production slice. +var wantUnsetVars = []string{ + "IDSEC_PROFILE", + "DEPLOY_ENV", +} + +// Not parallel: kept serial with the rest of the file. +func TestUnsetVarsIsExactlyTheExpectedSet(t *testing.T) { + if !slices.Equal(unsetVars, wantUnsetVars) { + t.Errorf("unsetVars = %q, want exactly %q", unsetVars, wantUnsetVars) + } +} + +// TestRun_UnsetsSDKBehaviorVarsAndRestoresThem covers both halves of the +// contract: inside Run the variables must be absent (not empty — the SDK's own +// checks distinguish the two), and afterwards each must return to its exact +// prior state, including "was never set". +// +// Not parallel: mutates process-wide environment variables. +func TestRun_UnsetsSDKBehaviorVarsAndRestoresThem(t *testing.T) { + // One var pre-set, one deliberately unset, so a restore that writes "" for + // an originally-absent var is caught. + const preset = "developer-profile" + t.Setenv("IDSEC_PROFILE", preset) + + if err := os.Unsetenv("DEPLOY_ENV"); err != nil { + t.Fatalf("Unsetenv: %v", err) + } + t.Cleanup(func() { _ = os.Unsetenv("DEPLOY_ENV") }) + + inside := map[string]bool{} // var -> was present inside Run + Run(func() int { + for _, k := range wantUnsetVars { + _, ok := os.LookupEnv(k) + inside[k] = ok + } + return 0 + }) + + for _, k := range wantUnsetVars { + if inside[k] { + t.Errorf("%s was still set inside Run; it must be unset, not redirected", k) + } + } + + if got, ok := os.LookupEnv("IDSEC_PROFILE"); !ok || got != preset { + t.Errorf("IDSEC_PROFILE = %q (set=%v) after Run, want the pre-existing %q", got, ok, preset) + } + if v, ok := os.LookupEnv("DEPLOY_ENV"); ok { + t.Errorf("DEPLOY_ENV is set to %q after Run; an originally-unset var must stay unset", v) + } +} + // Not parallel: mutates process-wide environment variables. func TestRun_RedirectsAllHomeEnvVars(t *testing.T) { var ( @@ -181,6 +260,58 @@ func TestAssertSandboxed_FailsOnAHostileFileLogPath(t *testing.T) { }) } +// TestAssertSandboxed_FailsOnAHostileConfigPath is what makes the +// config.ConfigPath() block in AssertSandboxed load-bearing. GRANT_CONFIG +// overrides config.ConfigPath outright, bypassing ConfigDir, so it is the only +// input that fails that block and nothing else: deleting the block makes this +// test see zero failures. +// +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsOnAHostileConfigPath(t *testing.T) { + escapee := filepath.Join(t.TempDir(), "config.yaml") + + Run(func() int { + //nolint:usetesting // t.Setenv's cleanup would fire after Run restores. + if err := os.Setenv("GRANT_CONFIG", escapee); err != nil { + t.Errorf("Setenv: %v", err) + return 1 + } + rec := &recordingTB{} + AssertSandboxed(rec) + assertFailedResolvers(t, rec, "config.ConfigPath()") + return 0 + }) +} + +// TestAssertSandboxed_FailsOnAHostileHome is what makes the config.ConfigDir() +// and cache.CacheDir() blocks load-bearing. Both resolve through +// os.UserHomeDir (cache.CacheDir delegates to config.ConfigDir), and every +// other resolver has an explicit env override that still points in-sandbox, so +// an escaped home fails exactly those two. Deleting either block drops the +// count to one and this test fails. +// +// Not parallel: mutates process-wide environment variables. +func TestAssertSandboxed_FailsOnAHostileHome(t *testing.T) { + escapee := t.TempDir() + + Run(func() int { + // Both variables: POSIX os.UserHomeDir reads HOME, the Windows one + // reads USERPROFILE. Setting both makes the case real on either leg + // instead of skipping half the matrix. + for _, k := range []string{"HOME", "USERPROFILE"} { + //nolint:usetesting // t.Setenv's cleanup would fire after Run restores. + if err := os.Setenv(k, escapee); err != nil { + t.Errorf("Setenv %s: %v", k, err) + return 1 + } + } + rec := &recordingTB{} + AssertSandboxed(rec) + assertFailedResolvers(t, rec, "config.ConfigDir()", "cache.CacheDir()") + return 0 + }) +} + // Not parallel: mutates process-wide environment variables. func TestAssertSandboxed_FailsWhenBasicKeyringIsNotForced(t *testing.T) { Run(func() int { From 2c6def9eabd8d4a4743f793e63ad70225f4fab5a Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:23:09 +0200 Subject: [PATCH 14/15] docs(mutation-ledger): re-point cmd/favorites.go rows at this branch's lines This branch inserts 13 lines at cmd/favorites.go:148, so every ledger row below that point named the wrong statement while the header still claimed the numbers were verified. All nine cmd/favorites.go rows shifted by +13 (OUT-02/03/16/17/18/19/20/24/27), each confirmed against the actual line content rather than blanket-added. The header now says the numbers are relative to this branch. --- docs/mutation-ledger.md | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index a33b180..fd3e343 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -17,9 +17,12 @@ Then flip **Status** to `done`. **A PR is not complete until every one of its ro is `done`.** `wont-fix` and `refuted` rows are closed by review, not by a test; mark them `done` when the PR that owns them has landed the comment / rationale. -**Line numbers were re-verified against the current tree** (`main`, post-`f14e2b9`). -Several drifted from the source reports; corrections are noted in the Mutation cell -with `(was ...)`. **File:line is always the production site, never the test site.** +**Line numbers are relative to this branch's tree**, not to `main`. They were first +re-verified against `main` post-`f14e2b9`; the nine `cmd/favorites.go` rows were then +shifted by +13 for the non-interactive guard this branch inserts at +`cmd/favorites.go:148`. Several also drifted from the source reports; those corrections +are noted in the Mutation cell with `(was ...)`. **File:line is always the production +site, never the test site.** **Verdicts** are the verifiers' conclusions, not the original reports': `CONFIRMED` (survivor reproduced), `OVERSTATED` (survivor real, stated consequence @@ -56,8 +59,8 @@ premise does not hold). | REQ-22 | cmd/request submit | `cmd/request_submit.go:251` | `if !ui.IsInteractive() {` → `if false {` inside the `roleID == ""` branch | CONFIRMED | test | `TestRunRequestSubmit_NonInteractiveRequiresRoleID` | PR4 | todo | | REQ-23 | cmd integration suite | `cmd/integration_test.go` (harness, not a production site) | No mutation. Claim was "integration tests are absent from CI **and** every assertion accepts a panic". First half confirmed (`.github/workflows/ci.yml` runs `make test-race` / `go test -race`, never `-tags=integration`); second half is too broad — only line 153 accepts any panic; lines 71/125/235 require specific output | OVERSTATED | test | `TestMain` isolation + exact exit-code/error-text assertions; add `-tags=integration` to both CI legs | PR1 | done | | OUT-01 | cmd/list flags | `cmd/list.go:73` | Delete `cmd.MarkFlagsMutuallyExclusive("groups", "provider")`. `TestListCommand_MutualExclusivity` passes today on the *unrelated* runtime error `no eligible targets or groups found` | CONFIRMED | test | `TestListCommand_MutualExclusivity` (assert Cobra's `[groups provider] were all set`) | PR5 | todo | -| OUT-02 | cmd/favorites (interactive) | `cmd/favorites.go:245` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `selectFavoriteInteractive` | CONFIRMED | test | `TestFavoritesAddInteractive_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | -| OUT-03 | cmd/favorites (group add) | `cmd/favorites.go:375` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `addGroupFavorite` | CONFIRMED | test | `TestAddGroupFavorite_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | +| OUT-02 | cmd/favorites (interactive) | `cmd/favorites.go:258` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `selectFavoriteInteractive` | CONFIRMED | test | `TestFavoritesAddInteractive_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | +| OUT-03 | cmd/favorites (group add) | `cmd/favorites.go:388` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `addGroupFavorite` | CONFIRMED | test | `TestAddGroupFavorite_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | | OUT-04 | cmd/list | `cmd/list.go:135` | `if provider == "" {` → `if true {` (groups fetched and emitted even when `--provider` is set) | CONFIRMED | test | `TestListCommand_ProviderSuppressesGroups` | PR5 | todo | | OUT-05 | cmd/status JSON | `cmd/status.go:212` | `Provider: strings.ToLower(string(s.CSP))` → `strings.ToUpper(string(s.CSP))` | CONFIRMED | test | `TestStatusJSON_Contract` (`assertJSONEqual`) | PR5 | todo | | OUT-06 | cmd/status JSON | `cmd/status.go:213` | `WorkspaceID: s.WorkspaceID` → `WorkspaceID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | @@ -70,18 +73,18 @@ premise does not hold). | OUT-13 | cmd/list JSON | `cmd/list.go:167` | `RoleID: t.RoleInfo.ID` → `RoleID: ""`. `roleId` is the field an LLM/automation feeds straight back into `grant request submit --role-id`. Note the verifier's correction: `--target` resolves on the emitted **name**, not `workspaceId` | CONFIRMED | test | `TestListJSON_RoundTripsToRequestSubmit` | PR5 | todo | | OUT-14 | cmd/list JSON | `cmd/list.go:175` | `GroupID: g.GroupID` → `GroupID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | | OUT-15 | cmd/list JSON | `cmd/list.go:176` | `DirectoryID: g.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | -| OUT-16 | cmd/favorites JSON | `cmd/favorites.go:449` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | todo | -| OUT-17 | cmd/favorites JSON | `cmd/favorites.go:451` | `Role: entry.Role` → `Role: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | -| OUT-18 | cmd/favorites JSON | `cmd/favorites.go:453` | `DirectoryID: entry.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | -| OUT-19 | cmd/favorites | `cmd/favorites.go:320` | `fav.Provider = cfg.DefaultProvider` → `fav.Provider = "azure"`. Every command test uses the azure default, so a non-default `DefaultProvider` (aws/gcp) is unpinned. Secondary, same defect class: `internal/config/favorites.go:21-22` independently defaults empty → `"azure"` | CONFIRMED | test | `TestFavoritesAdd_HonorsNonDefaultProvider` | PR5 | todo | -| OUT-20 | cmd/favorites | `cmd/favorites.go:186-192` | Delete the `--type groups` / `--target`+`--role` pairing validation from `parseFavoritesAddFlags`. Dead-covered: `runFavoritesAddProduction` re-validates, so this is redundancy loss for DI callers, not a current user-facing hole | CONFIRMED | test | `TestParseFavoritesAddFlags_Validation` | PR5 | todo | +| OUT-16 | cmd/favorites JSON | `cmd/favorites.go:462` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | todo | +| OUT-17 | cmd/favorites JSON | `cmd/favorites.go:464` | `Role: entry.Role` → `Role: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | +| OUT-18 | cmd/favorites JSON | `cmd/favorites.go:466` | `DirectoryID: entry.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | +| OUT-19 | cmd/favorites | `cmd/favorites.go:333` | `fav.Provider = cfg.DefaultProvider` → `fav.Provider = "azure"`. Every command test uses the azure default, so a non-default `DefaultProvider` (aws/gcp) is unpinned. Secondary, same defect class: `internal/config/favorites.go:21-22` independently defaults empty → `"azure"` | CONFIRMED | test | `TestFavoritesAdd_HonorsNonDefaultProvider` | PR5 | todo | +| OUT-20 | cmd/favorites | `cmd/favorites.go:199-205` | Delete the `--type groups` / `--target`+`--role` pairing validation from `parseFavoritesAddFlags`. Dead-covered: `runFavoritesAddProduction` re-validates, so this is redundancy loss for DI callers, not a current user-facing hole | CONFIRMED | test | `TestParseFavoritesAddFlags_Validation` | PR5 | todo | | OUT-21 | cmd/status | `cmd/status.go:110-114` | Make the directory-name merge unconditional: drop the `if _, exists := data.nameMap[k]; !exists` guard. Precedence is genuinely unasserted, but in production both lookups read the same cached Azure eligibility response, so a divergence needs colliding IDs or malformed data | OVERSTATED | test | `TestStatus_DirectoryNameMergePrecedence` | PR5 | todo | | OUT-22 | cmd/status | `cmd/status.go:129` | Delete `_ = cache.CleanupSessions(tracker, activeIDs)` | CONFIRMED | test | `TestStatus_CleansUpStaleSessionTimestamps` | PR5 | todo | | OUT-23 | cmd/status (test quality) | `cmd/status.go:185-192` (`computeRemainingTime`) | No production defect. `TestStatusCommand_RemainingTime/text_output_shows_remaining_time` asserts `remaining: 4` as a substring, which `remaining: 4h 30m` satisfies — only the JSON sibling killed a sixfold arithmetic error. Signal-poor assertion, not an uncovered defect | CONFIRMED | test | Tighten the text subtest to an exact `remaining: 45m` | PR5 | todo | -| OUT-24 | cmd/favorites | `cmd/favorites.go:419-420` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | todo | +| OUT-24 | cmd/favorites | `cmd/favorites.go:432-433` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | todo | | OUT-25 | cmd/list flags | `cmd/list.go:71` | Delete `cmd.Flags().Bool("refresh", ...)` registration. **Verifier correction:** `grant list --refresh` is **not** already a no-op — `list.go:91-92` reads it and passes it into `buildCachedLister`, and CLAUDE.md is correct. The real finding is missing flag-registration/wiring coverage | OVERSTATED | test | `TestListCommand_RefreshBypassesCache` | PR5 | todo | | OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | done | -| OUT-27 | cmd/favorites | `cmd/favorites.go:235-237` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | refuted | n/a — already killed | — | todo | +| OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | refuted | n/a — already killed | — | todo | | OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | | OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | | SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | todo | From ab22afebdd69bbe56f26ed1e073fff923344010c Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:23:09 +0200 Subject: [PATCH 15/15] ci: shuffle the integration build; fix favorites non-interactive hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 23 untagged cmd/*_test.go files compile into the integration binary too, so their order dependence was never shuffled there — -shuffle=on was applied only to the untagged build. Add it to the integration step, still unguarded so both legs run it. `grant favorites add` also requires a NAME argument, which the new non-interactive error omitted; it now says so. --- .github/workflows/ci.yml | 6 +++++- CHANGELOG.md | 1 + CLAUDE.md | 6 ++++-- cmd/favorites.go | 2 +- cmd/favorites_test.go | 8 +++++--- 5 files changed, 16 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index da23328..5d5aebb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,8 +49,12 @@ jobs: # argument parsing, exit codes and error text are exercised end to end. # Unguarded so both platforms are covered; the harness builds its own # binary into a temp dir. + # + # -shuffle=on because the 23 untagged cmd/*_test.go files compile into + # this binary too: without it their order dependence is only ever + # shuffled in the untagged build, never here. - name: Integration tests - run: go test -tags=integration ./cmd -count=1 -v + run: go test -tags=integration ./cmd -count=1 -shuffle=on -v # The suite mutates package globals (bootstrapImpl, log, # ui.IsTerminalFunc). Randomising order is what surfaces a test that only diff --git a/CHANGELOG.md b/CHANGELOG.md index 31b56da..f5229a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ All notable changes to this project will be documented in this file. ### Fixed - `grant favorites add` now fails immediately without a terminal instead of authenticating first +- `grant favorites add`'s non-interactive error now mentions the required favorite name, not only the flags ## [0.9.0] - 2026-08-14 diff --git a/CLAUDE.md b/CLAUDE.md index 730b9e7..a0da585 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -217,7 +217,7 @@ make clean # Clean build artifacts - Windows runners have no GNU make, so that leg runs the equivalent Go commands directly (`go build -trimpath -o grant.exe .`, `go test -race ./... -v`); Linux keeps `make build` / `make test-race`. Keep the two legs in sync when Makefile targets change - `go test -race` works on windows/amd64 because the runner image ships gcc (the race detector needs cgo) - The `Self-update end-to-end` step runs the `selfupdate_e2e`-tagged tests on **both** legs (no `if:` guard) — it is the only test that replaces a real running executable, and comparing the two platforms is the whole point. It builds its fixtures locally, so it needs no network. Keep it unguarded; guarding it to Linux would defeat its purpose -- `Integration tests` (`go test -tags=integration ./cmd`) and `Test with shuffled order` (`go test -shuffle=on -count=1 ./...`) run **unguarded on both legs**. Integration needs no network and takes ~2s; shuffling is what catches order dependence in a suite that mutates package globals +- `Integration tests` (`go test -tags=integration ./cmd -shuffle=on`) and `Test with shuffled order` (`go test -shuffle=on -count=1 ./...`) run **unguarded on both legs**. Integration needs no network and takes ~2s; shuffling is what catches order dependence in a suite that mutates package globals. The integration step is shuffled too because the untagged `cmd/*_test.go` files compile into that binary as well — without it their order dependence is only ever shuffled in the untagged build - `.golangci.yml` sets `run.build-tags: [integration, selfupdate_e2e]` so both tagged files are linted. Side effect: with `integration` set, `cmd/main_test.go` (`//go:build !integration`) is excluded from linting - Lint (`golangci-lint-action`) runs on Linux only — a second pass on Windows adds minutes and finds nothing new - Tests must be OS-portable. Never assert POSIX permission bits without a `runtime.GOOS == "windows"` skip: Go synthesizes `0666`/`0777` for Windows files and `os.Chmod` there only toggles the read-only attribute. Current skips: `internal/config/config_test.go` (`TestLoadConfig_PermissionError`, `TestConfigDir_Error` — chmod 0000 and `HOME`) and `internal/cache/cache_test.go` (`TestSet_FilePermissions`) @@ -302,8 +302,10 @@ There is no `getAuth`/`getSCAService`; those never existed. Every test that swap - `IDSEC_KEYRING_FOLDER` and `IDSEC_FILE_LOG_PATH` are **absolute-path overrides that bypass the `HOME` fallback entirely** (`pkg/common/keyring/idsec_basic_keyring.go`, `pkg/common/idsec_logger.go`, which `MkdirAll`s the log's parent). A value already exported in the developer's or CI environment sends those writes outside the sandbox no matter what `HOME` says. - `IDSEC_BASIC_KEYRING=1` is set because the OS keyring is a **daemon, not a path**, so no redirect can sandbox it: on a non-WSL Linux box with `DBUS_SESSION_BUS_ADDRESS` set, `GetKeyring` picks the real libsecret store. Forcing the file backend puts keyring state into the sandboxed folder instead. If a future SDK stops honoring the variable, this containment is gone and nothing here detects it. - `XDG_CONFIG_HOME` is **speculative/defensive** — nothing in grant or the pinned SDK reads it (`rg XDG_` finds only testenv's own comment and tests). It is redirected because it is the conventional escape hatch and costs nothing. +- `IDSEC_PROFILE` and `DEPLOY_ENV` are **unset**, not redirected (`unsetVars`) — they select behavior, not a location, so the only safe state is absent. `IDSEC_PROFILE` picks the SDK's default profile *name*; `DEPLOY_ENV` feeds tenant-env resolution inside `isp.FromISPAuth`, which the sca/workflows retry-policy tests drive for real. `Run` captures set-vs-unset per variable and restores that exact state. - `testenv` must not import `testing`; `AssertSandboxed` therefore takes a `TB` interface (`Helper`/`Errorf`) that `*testing.T` satisfies. - `AssertSandboxed` checks the *configured destinations* — `config.ConfigDir`, `config.ConfigPath`, `cache.CacheDir`, `profiles.GetProfilesFolder`, the SDK keyring folder and the SDK file-log path — plus that `IDSEC_BASIC_KEYRING` is non-empty. The last two resolvers are **reimplemented** in testenv rather than called, because the SDK constructor creates the directory as a side effect and an assertion must not write. It does not prove nothing was written outside the sandbox, and cannot see reads. A snapshot-diff gate was considered and rejected: a concurrently running real `grant` false-positives with certainty, and size+mtime misses same-size rewrites. +- Each `AssertSandboxed` block must be individually killable, which means asserting the failure **count and which resolver failed**, not `len(errs) > 0`: any other assertion satisfies a bare "at least one", so the block under test could be deleted outright. `GRANT_CONFIG` pointed outside the sandbox isolates `config.ConfigPath` (1 failure); `HOME`+`USERPROFILE` pointed outside isolates `config.ConfigDir` **and** `cache.CacheDir` (2 failures, because `CacheDir` delegates to `ConfigDir` → `os.UserHomeDir`). `recordingTB` therefore stores the *formatted* message — the resolver name is an argument, not part of the format string. - The redirect list is validated against an **explicit literal** in `testenv_test.go`, and every entry additionally gets a hostile pre-existing value before `Run` in `TestRun_OverridesPreExistingHostileValues`. Ranging over `redirectedVars` itself is the trap this replaced: dropping an entry merely checked one fewer thing, and four of the original five survived a drop-one mutation because with `HOME` redirected their *fallbacks* already landed in-sandbox. A var's whole value is defending against a pre-existing value, so that is what the test must supply. - `Run` restores the environment and removes the sandbox from a **`defer`**, so a panic inside `m.Run` (a `-race` detection, a stray panic) cannot leak the directory or leave the process redirected. `sandboxRoot` is saved and restored rather than cleared, so a nested `Run` hands the outer root back. - `TestMain` lives in `cmd/main_test.go` (`//go:build !integration`, because `cmd/integration_test.go` declares its own), `internal/config/main_test.go`, `internal/cache/main_test.go`, `internal/sca/main_test.go`, `internal/workflows/main_test.go` and `internal/sdkclient/main_test.go`. The `config`/`cache` ones are in the **external** test package (`config_test`/`cache_test`) because `testenv` imports those packages — an in-package test file importing it would be an import cycle. `sca`/`workflows`/`sdkclient` can be in-package: `testenv` imports `internal/sca/models`, not `internal/sca`. Those three earn a `TestMain` because their retry-policy tests drive the **real** service constructors. @@ -352,7 +354,7 @@ func (m *mockAuthProvider) Authenticate(p *models.IdsecProfile) (*models.IdsecTo #### Integration Tests -`cmd/integration_test.go` (`//go:build integration`) drives the compiled binary as a child process. Its `TestMain` runs inside `testenv.Run` and builds into a unique temp directory — never the shared `../grant-test`, which two concurrent runs would fight over. `GOCACHE`/`GOMODCACHE`/`GOPATH` are resolved **before** the `HOME` redirect and passed to the build, otherwise it starts from an empty module cache and needs the network. +`cmd/integration_test.go` (`//go:build integration`) drives the compiled binary as a child process. Its `TestMain` runs inside `testenv.Run` and builds into a unique temp directory — never the shared `../grant-test`, which two concurrent runs would fight over. `GOCACHE`/`GOMODCACHE`/`GOPATH`/`GOENV` are resolved **before** the `HOME` redirect and passed to the build, otherwise it starts from an empty module cache and needs the network. `GOENV` is in the list because the child `go build` finds its env file via `os.UserConfigDir` — `$XDG_CONFIG_HOME/go/env` on Linux (redirected to an empty sandbox) but `%AppData%\go\env` on Windows (not redirected at all), so without it the two CI legs read different files and any `go env -w GOPROXY=…`/`GOFLAGS`/`GOPRIVATE` is silently dropped on Linux. Assertions are exact exit codes plus exact error text. Keyword soup (`error|Error|failed|not found`) is banned here: a panic satisfies it. `runGrant` fails the test outright if the child output contains a panic, and closes stdin so no prompt can block. diff --git a/cmd/favorites.go b/cmd/favorites.go index c5cbbbc..f53f68c 100644 --- a/cmd/favorites.go +++ b/cmd/favorites.go @@ -155,7 +155,7 @@ func runFavoritesAddProduction(cmd *cobra.Command, args []string) error { // The message is favorites-specific on purpose; earlyNonInteractiveCheck // in request_picker.go would wrongly tell the user to supply a request ID. if !ui.IsInteractive() { - return fmt.Errorf("%w; pass --target and --role, or --type groups with --group", ui.ErrNotInteractive) + return fmt.Errorf("%w; pass a favorite name plus --target and --role, or a name plus --type groups with --group", ui.ErrNotInteractive) } // Bootstrap auth and SCA service diff --git a/cmd/favorites_test.go b/cmd/favorites_test.go index bb49e1a..5efac96 100644 --- a/cmd/favorites_test.go +++ b/cmd/favorites_test.go @@ -1288,16 +1288,18 @@ func TestFavoritesAdd_NonInteractiveGuard(t *testing.T) { name: "name given but no flags and no terminal", setupConfig: func(path string) { _ = config.Save(config.DefaultConfig(), path) }, args: []string{"myfav"}, - // The hint must name the favorites flags, not a request ID. + // The hint must name the favorites flags, not a request ID — and + // the NAME argument, which is required too and which a + // flags-only hint silently omits. wantNotInteract: true, - wantContain: []string{"--target", "--role"}, + wantContain: []string{"name", "--target", "--role"}, }, { name: "groups type without --group and no terminal", setupConfig: func(path string) { _ = config.Save(config.DefaultConfig(), path) }, args: []string{"myfav", "--type", "groups"}, wantNotInteract: true, - wantContain: []string{"--group"}, + wantContain: []string{"name", "--group"}, }, { name: "duplicate name still reports the duplicate, not the missing terminal",