test: isolate the suite from real user state, and wire integration tests into CI

.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
@@ -42,6 +44,24 @@ 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.
+      #
+      # -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 -shuffle=on -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

.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

CHANGELOG.md

@@ -4,6 +4,11 @@ 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
+- `grant favorites add`'s non-interactive error now mentions the required favorite name, not only the flags
+
 ## [0.9.0] - 2026-08-14
 
 ### Added

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 -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`)
 - 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,42 @@ 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 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.
+- `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.
+- `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` 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
 
@@ -344,19 +353,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`/`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.
 
 ### Error Handling
 

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()
+}

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 a favorite name plus --target and --role, or a name plus --type groups with --group", ui.ErrNotInteractive)
+	}
+
 	// Bootstrap auth and SCA service
 	_, scaService, _, err := bootstrapSCAService()
 	if err != nil {