diff --git a/CLAUDE.md b/CLAUDE.md index dc18bc6..4941a45 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -87,6 +87,10 @@ Custom `SCAAccessService` follows SDK conventions: - `httptest.NewServer` for service mocks - `httpClient` interface for DI - Test files co-located as `_test.go` +- **Mock capture convention** (`cmd/test_mocks_test.go`, precedent `mockSessionRevoker`): record the arguments in the method body *before* dispatching to any `xxxFunc` callback, keep a history slice plus a `lastX()` accessor (a history is what answers "called exactly once?"), defensively copy slices/maps and pointer-to-struct args, and guard against a nil request. An optional `*string` argument is flattened to `reason string` + `reasonSet bool` so a test can tell `nil` from `""`. There is exactly one mock per interface — an arg-blind sibling silently opts every future test out of capture +- No mutex on those histories: the only mocks reached from more than one goroutine are the eligibility listers, via the fan-outs in `fetchEligibility`/`fetchGroupsEligibility` (`cmd/root.go`), `resolveAndElevateUnifiedPath` (`cmd/root.go`) and `fetchAllTargets`/`fetchAllGroups` (`cmd/helpers.go`), and those mocks are stateless readers. `Elevate`, `ElevateGroups` and all five `accessRequestService` methods are called from strictly sequential paths. `make test-race` is what keeps that honest. Cite the **function name**, not a line number — this reasoning has already been invalidated once by an unrelated insertion above it +- **Test scaffolding in `cmd` lives in `_test.go` files** so `testing` never enters the production build: `cmd/test_helpers_test.go` (`executeCommand`, `executeCommandStreams`, `executeWithHint`, `withInteractiveTTY`) and `cmd/test_mocks_test.go` (every shared mock). Both used to be production files. Neither linked `testing` in at the time, so the moves were **preventive, not remedial** — but `withInteractiveTTY` would have been the first helper to pull it in, and the mock file is the larger and faster-growing of the two. `go list -deps . | grep -c '^testing$'` must stay `0` +- Any test whose behavior depends on interactivity MUST set it explicitly with `withInteractiveTTY`: `go test` happens to run with a non-TTY stdin, but that is an accident of the harness, not an assertion - 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 @@ -349,7 +353,7 @@ func TestCommand(t *testing.T) { #### Mock Implementations ```go -// test_mocks.go - shared mocks across tests +// test_mocks_test.go - shared mocks across tests type mockAuthProvider struct { authenticateFn func(*models.IdsecProfile) (*models.IdsecToken, error) } diff --git a/cmd/elevate_args_test.go b/cmd/elevate_args_test.go new file mode 100644 index 0000000..40e25c6 --- /dev/null +++ b/cmd/elevate_args_test.go @@ -0,0 +1,686 @@ +package cmd + +// Argument-capture and guard tests for the elevation paths (root, env, +// selection). Each test names the mutation it kills. +// +// Fixture IDs are deliberately distinct from one another — a swap mutation +// only dies when the two values differ. Do not collapse them. + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/aaearon/grant-cli/internal/config" + "github.com/aaearon/grant-cli/internal/sca/models" + sdkmodels "github.com/cyberark/idsec-sdk-golang/pkg/models" + authmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/auth" +) + +// failingTargetSelector fails the test if interactive selection is reached. +func failingTargetSelector(t *testing.T) *mockTargetSelector { + t.Helper() + return &mockTargetSelector{ + selectFunc: func([]models.EligibleTarget) (*models.EligibleTarget, error) { + t.Error("interactive target selection must not be reached") + return nil, errors.New("selector must not be called") + }, + } +} + +// awsFixtureTarget is the single eligible target used by the env tests. +func awsFixtureTarget() models.EligibleTarget { + return models.EligibleTarget{ + OrganizationID: "o-env-1", + WorkspaceID: "111122223333", + WorkspaceName: "AWS Mgmt", + WorkspaceType: models.WorkspaceTypeAccount, + RoleInfo: models.RoleInfo{ID: "role-env-9", Name: "AdminAccess"}, + } +} + +const envCredsJSON = `{"aws_access_key":"ASIAENV","aws_secret_access_key":"env-secret","aws_session_token":"env-token"}` + +func awsFixtureElevator() *mockElevateService { + return &mockElevateService{ + elevateFunc: func(_ context.Context, _ *models.ElevateRequest) (*models.ElevateResponse, error) { + creds := envCredsJSON + return &models.ElevateResponse{Response: models.ElevateAccessResult{ + CSP: models.CSPAWS, + OrganizationID: "o-env-1", + Results: []models.ElevateTargetResult{{ + WorkspaceID: "111122223333", RoleID: "role-env-9", + SessionID: "sess-env-1", AccessCredentials: &creds, + }}, + }}, nil + }, + } +} + +func awsFixtureLister() *mockEligibilityLister { + return &mockEligibilityLister{ + response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{awsFixtureTarget()}, + Total: 1, + }, + } +} + +func authedLoader() *mockAuthLoader { + return &mockAuthLoader{token: &authmodels.IdsecToken{Token: "test-jwt"}} +} + +// ELV-01 (findItemByDisplay returning &items[0]) is closed by the index-based +// selector fix: findItemByDisplay no longer exists. resolveSelectionItem is +// pinned by TestResolveSelectionItem in cmd/selection_test.go, and the real +// SelectItem wiring by the pty test in cmd/selection_pty_linux_test.go. + +// TestElevateCloud_RequestPayload kills ELV-02 and ELV-03: swapping +// WorkspaceID/RoleID, or blanking CSP/OrganizationID, in the elevateCloud +// request builder in elevateCloud (cmd/root.go). +func TestElevateCloud_RequestPayload(t *testing.T) { + target := &models.EligibleTarget{ + CSP: models.CSPAWS, + OrganizationID: "o-cloud-77", + WorkspaceID: "ws-cloud-A", + WorkspaceName: "AWS Sandbox", + RoleInfo: models.RoleInfo{ID: "role-cloud-B", Name: "ReadOnly"}, + } + elevator := &mockElevateService{ + response: &models.ElevateResponse{Response: models.ElevateAccessResult{ + CSP: models.CSPAWS, + Results: []models.ElevateTargetResult{{SessionID: "sess-1"}}, + }}, + } + + if _, _, err := elevateCloud(t.Context(), target, elevator); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + assertElevateRequest(t, elevator, models.CSPAWS, "o-cloud-77", "ws-cloud-A", "role-cloud-B") +} + +// TestResolveAndElevate_RequestPayload kills ELV-04: the same swap in the +// second, byte-identical builder in resolveAndElevate, used by grant env. +// +// The duplication between the two builders is real; extracting a shared helper +// is a refactor and is filed as follow-up, not done here. +func TestResolveAndElevate_RequestPayload(t *testing.T) { + target := models.EligibleTarget{ + CSP: models.CSPAWS, + OrganizationID: "o-env-88", + WorkspaceID: "ws-env-A", + WorkspaceName: "AWS Mgmt", + WorkspaceType: models.WorkspaceTypeAccount, + RoleInfo: models.RoleInfo{ID: "role-env-B", Name: "AdminAccess"}, + } + lister := &mockEligibilityLister{ + response: &models.EligibilityResponse{Response: []models.EligibleTarget{target}, Total: 1}, + } + creds := envCredsJSON + elevator := &mockElevateService{ + response: &models.ElevateResponse{Response: models.ElevateAccessResult{ + CSP: models.CSPAWS, + Results: []models.ElevateTargetResult{{ + SessionID: "sess-env", AccessCredentials: &creds, + }}, + }}, + } + + flags := &elevateFlags{provider: "aws", target: "AWS Mgmt", role: "AdminAccess"} + if _, err := resolveAndElevate(flags, nil, authedLoader(), lister, elevator, + failingTargetSelector(t), config.DefaultConfig(), nil); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + assertElevateRequest(t, elevator, models.CSPAWS, "o-env-88", "ws-env-A", "role-env-B") +} + +func assertElevateRequest(t *testing.T, elevator *mockElevateService, csp models.CSP, orgID, workspaceID, roleID string) { + t.Helper() + if len(elevator.elevateCalls) != 1 { + t.Fatalf("expected exactly 1 Elevate call, got %d", len(elevator.elevateCalls)) + } + req := elevator.lastElevate() + if req.CSP != csp { + t.Errorf("CSP = %q, want %q", req.CSP, csp) + } + if req.OrganizationID != orgID { + t.Errorf("OrganizationID = %q, want %q", req.OrganizationID, orgID) + } + if len(req.Targets) != 1 { + t.Fatalf("expected 1 target in the request, got %d", len(req.Targets)) + } + if req.Targets[0].WorkspaceID != workspaceID { + t.Errorf("Targets[0].WorkspaceID = %q, want %q", req.Targets[0].WorkspaceID, workspaceID) + } + if req.Targets[0].RoleID != roleID { + t.Errorf("Targets[0].RoleID = %q, want %q", req.Targets[0].RoleID, roleID) + } +} + +// envConfigWithFavorites builds a config carrying the named favorites. +func envConfigWithFavorites(favs map[string]config.Favorite) *config.Config { + cfg := config.DefaultConfig() + cfg.Favorites = favs + return cfg +} + +// TestEnv_FavoriteMode kills ELV-05: the `--favorite` branch in +// resolveAndElevate (cmd/root.go) is registered and advertised by grant env +// but was exercised by no test at all. +func TestEnv_FavoriteMode(t *testing.T) { + elevator := awsFixtureElevator() + cfg := envConfigWithFavorites(map[string]config.Favorite{ + "aws-fav": {Provider: "aws", Target: "AWS Mgmt", Role: "AdminAccess"}, + }) + + cmd := NewEnvCommandWithDeps(nil, authedLoader(), awsFixtureLister(), elevator, failingTargetSelector(t), cfg) + output, err := executeCommand(cmd, "--favorite", "aws-fav") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, output) + } + if !strings.Contains(output, "export AWS_ACCESS_KEY_ID='ASIAENV'") { + t.Errorf("expected AWS exports from the favorite, got:\n%s", output) + } + assertElevateRequest(t, elevator, models.CSPAWS, "o-env-1", "111122223333", "role-env-9") +} + +// TestEnv_RejectsGroupFavorite kills ELV-06: the group-favorite rejection in +// resolveAndElevate's --favorite branch (cmd/root.go). +func TestEnv_RejectsGroupFavorite(t *testing.T) { + elevator := awsFixtureElevator() + cfg := envConfigWithFavorites(map[string]config.Favorite{ + "grp-fav": {Type: config.FavoriteTypeGroups, Provider: "azure", Group: "Cloud Admins"}, + }) + + cmd := NewEnvCommandWithDeps(nil, authedLoader(), awsFixtureLister(), elevator, failingTargetSelector(t), cfg) + _, err := executeCommand(cmd, "--favorite", "grp-fav") + if err == nil { + t.Fatal("expected a group favorite to be rejected") + } + if !strings.Contains(err.Error(), "is a group favorite") { + t.Errorf("error = %v, want the group-favorite rejection", err) + } + if len(elevator.elevateCalls) != 0 { + t.Errorf("no elevation may be issued, got %+v", elevator.elevateCalls) + } +} + +// TestEnv_FavoriteProviderMismatch kills ELV-07: the provider-mismatch check in +// resolveAndElevate's --favorite branch (cmd/root.go). +func TestEnv_FavoriteProviderMismatch(t *testing.T) { + elevator := awsFixtureElevator() + cfg := envConfigWithFavorites(map[string]config.Favorite{ + "aws-fav": {Provider: "aws", Target: "AWS Mgmt", Role: "AdminAccess"}, + }) + + cmd := NewEnvCommandWithDeps(nil, authedLoader(), awsFixtureLister(), elevator, failingTargetSelector(t), cfg) + _, err := executeCommand(cmd, "--favorite", "aws-fav", "--provider", "azure") + if err == nil { + t.Fatal("expected a provider mismatch to be rejected") + } + if !strings.Contains(err.Error(), "does not match favorite provider") { + t.Errorf("error = %v, want the provider-mismatch rejection", err) + } + if len(elevator.elevateCalls) != 0 { + t.Errorf("no elevation may be issued, got %+v", elevator.elevateCalls) + } +} + +// TestEnv_RequiresBothTargetAndRole kills ELV-08: the paired --target/--role +// validation in resolveAndElevate (cmd/root.go). +func TestEnv_RequiresBothTargetAndRole(t *testing.T) { + tests := []struct { + name string + args []string + }{ + {name: "target without role", args: []string{"--provider", "aws", "--target", "AWS Mgmt"}}, + {name: "role without target", args: []string{"--provider", "aws", "--role", "AdminAccess"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + elevator := awsFixtureElevator() + cmd := NewEnvCommandWithDeps(nil, authedLoader(), awsFixtureLister(), elevator, + failingTargetSelector(t), config.DefaultConfig()) + + _, err := executeCommand(cmd, tt.args...) + if err == nil { + t.Fatal("expected an error when only one of --target/--role is given") + } + if !strings.Contains(err.Error(), "both --target and --role must be provided") { + t.Errorf("error = %v, want the paired-flag validation", err) + } + if len(elevator.elevateCalls) != 0 { + t.Errorf("no elevation may be issued, got %+v", elevator.elevateCalls) + } + }) + } +} + +// TestResolveFavoriteFlags_DetectsGroupFavorite kills ELV-09: the group +// detection in resolveFavoriteFlags (cmd/root.go), which is the root +// command's equivalent of the env check above and is separately unpinned. +func TestResolveFavoriteFlags_DetectsGroupFavorite(t *testing.T) { + cfg := envConfigWithFavorites(map[string]config.Favorite{ + "grp-fav": { + Type: config.FavoriteTypeGroups, Provider: "azure", + Group: "Cloud Admins", DirectoryID: "dir-abc", + }, + }) + + flags := &elevateFlags{favorite: "grp-fav"} + rf, err := resolveFavoriteFlags(flags, cfg) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !rf.isGroupFavorite { + t.Error("isGroupFavorite = false, want true") + } + if flags.group != "Cloud Admins" { + t.Errorf("flags.group = %q, want Cloud Admins", flags.group) + } + if rf.favDirectoryID != "dir-abc" { + t.Errorf("favDirectoryID = %q, want dir-abc", rf.favDirectoryID) + } + if rf.targetName != "" || rf.roleName != "" { + t.Errorf("a group favorite must not populate target/role, got %q/%q", rf.targetName, rf.roleName) + } +} + +// TestElevateGroup_SurfacesErrorInfo kills ELV-10: dropping the ErrorInfo check +// in elevateGroup (cmd/root.go) makes a policy denial print as a success +// and exit 0. +func TestElevateGroup_SurfacesErrorInfo(t *testing.T) { + elevator := &mockGroupsElevator{ + response: &models.GroupsElevateResponse{ + DirectoryID: "dir-1", + CSP: models.CSPAzure, + Results: []models.GroupsElevateTargetResult{{ + GroupID: "grp-1", + ErrorInfo: &models.ErrorInfo{ + Code: "POLICY_DENIED", + Message: "elevation denied by policy", + Description: "no matching policy", + }, + }}, + }, + } + + _, _, err := elevateGroup(t.Context(), &models.GroupsEligibleTarget{ + DirectoryID: "dir-1", GroupID: "grp-1", GroupName: "Cloud Admins", + }, elevator) + if err == nil { + t.Fatal("a denied group elevation must not be reported as success") + } + if !strings.Contains(err.Error(), "POLICY_DENIED") || !strings.Contains(err.Error(), "elevation denied by policy") { + t.Errorf("error = %v, want the ErrorInfo code and message", err) + } +} + +// TestEnv_SurfacesErrorInfo kills ELV-11: the same check on the env path, in +// resolveAndElevate (cmd/root.go). +func TestEnv_SurfacesErrorInfo(t *testing.T) { + // Credentials are present alongside the error so that dropping the guard + // produces the real false-success — exports printed, exit 0 — rather than + // tripping the downstream "no credentials" fallback. + creds := envCredsJSON + elevator := &mockElevateService{ + response: &models.ElevateResponse{Response: models.ElevateAccessResult{ + CSP: models.CSPAWS, + Results: []models.ElevateTargetResult{{ + WorkspaceID: "111122223333", + AccessCredentials: &creds, + ErrorInfo: &models.ErrorInfo{ + Code: "POLICY_DENIED", + Message: "elevation denied by policy", + Description: "no matching policy", + }, + }}, + }}, + } + + cmd := NewEnvCommandWithDeps(nil, authedLoader(), awsFixtureLister(), elevator, + failingTargetSelector(t), config.DefaultConfig()) + output, _, err := executeCommandStreams(cmd, "--provider", "aws", "--target", "AWS Mgmt", "--role", "AdminAccess") + if err == nil { + t.Fatal("a denied elevation must not be reported as success") + } + if !strings.Contains(err.Error(), "POLICY_DENIED") { + t.Errorf("error = %v, want the ErrorInfo code", err) + } + if strings.Contains(output, "export AWS_") { + t.Errorf("no credentials may be printed on a denied elevation, got:\n%s", output) + } +} + +// TestElevate_EmptyResultsGuards kills ELV-12, ELV-13 and ELV-14: all three +// "no results returned" guards. Without them the next line indexes Results[0] +// and panics. +func TestElevate_EmptyResultsGuards(t *testing.T) { + t.Run("elevateCloud", func(t *testing.T) { + elevator := &mockElevateService{ + response: &models.ElevateResponse{Response: models.ElevateAccessResult{ + CSP: models.CSPAzure, Results: nil, + }}, + } + _, _, err := elevateCloud(t.Context(), &models.EligibleTarget{CSP: models.CSPAzure}, elevator) + if err == nil { + t.Fatal("expected an error for an empty results list") + } + if !strings.Contains(err.Error(), "no results returned") { + t.Errorf("error = %v, want 'no results returned'", err) + } + }) + + t.Run("elevateGroup", func(t *testing.T) { + elevator := &mockGroupsElevator{ + response: &models.GroupsElevateResponse{DirectoryID: "dir-1", Results: nil}, + } + _, _, err := elevateGroup(t.Context(), &models.GroupsEligibleTarget{ + DirectoryID: "dir-1", GroupID: "grp-1", + }, elevator) + if err == nil { + t.Fatal("expected an error for an empty results list") + } + if !strings.Contains(err.Error(), "no results returned") { + t.Errorf("error = %v, want 'no results returned'", err) + } + }) + + t.Run("env path", func(t *testing.T) { + elevator := &mockElevateService{ + response: &models.ElevateResponse{Response: models.ElevateAccessResult{ + CSP: models.CSPAWS, Results: nil, + }}, + } + cmd := NewEnvCommandWithDeps(nil, authedLoader(), awsFixtureLister(), elevator, + failingTargetSelector(t), config.DefaultConfig()) + _, err := executeCommand(cmd, "--provider", "aws", "--target", "AWS Mgmt", "--role", "AdminAccess") + if err == nil { + t.Fatal("expected an error for an empty results list") + } + if !strings.Contains(err.Error(), "no results returned") { + t.Errorf("error = %v, want 'no results returned'", err) + } + }) +} + +// TestFetchEligibility_AllCSPsFail kills ELV-19: the aggregate guard at +// the end of fetchEligibility's multi-CSP branch (cmd/root.go). Without it a +// total multi-CSP failure returns (nil, nil) and +// each caller substitutes its own, less accurate message. +func TestFetchEligibility_AllCSPsFail(t *testing.T) { + lister := &mockEligibilityLister{ + listFunc: func(_ context.Context, csp models.CSP) (*models.EligibilityResponse, error) { + return nil, errors.New(string(csp) + " is down") + }, + } + + targets, err := fetchEligibility(t.Context(), lister, "") + if err == nil { + t.Fatal("expected an error when every CSP fails") + } + if !strings.Contains(err.Error(), "no eligible targets found, check your SCA policies") { + t.Errorf("error = %v, want the aggregate SCA-policy message", err) + } + if targets != nil { + t.Errorf("expected no targets, got %+v", targets) + } +} + +// TestEnv_SlowPromptTimeout kills ELV-20: elevating on the original ctx rather +// than a fresh one, in resolveAndElevate (cmd/root.go). A user who takes +// longer than apiTimeout +// to pick a target would get a deadline error instead of an elevation. Root's +// three dispatch paths have this coverage; env did not. +func TestEnv_SlowPromptTimeout(t *testing.T) { + origTimeout := apiTimeout + apiTimeout = 50 * time.Millisecond + t.Cleanup(func() { apiTimeout = origTimeout }) + + slowSelector := &mockTargetSelector{ + selectFunc: func(targets []models.EligibleTarget) (*models.EligibleTarget, error) { + time.Sleep(100 * time.Millisecond) // 2x apiTimeout + return &targets[0], nil + }, + } + + contextAware := &mockElevateService{ + elevateFunc: func(ctx context.Context, _ *models.ElevateRequest) (*models.ElevateResponse, error) { + if ctx.Err() != nil { + return nil, ctx.Err() + } + creds := envCredsJSON + return &models.ElevateResponse{Response: models.ElevateAccessResult{ + CSP: models.CSPAWS, + Results: []models.ElevateTargetResult{{ + SessionID: "sess-slow", AccessCredentials: &creds, + }}, + }}, nil + }, + } + + cmd := NewEnvCommandWithDeps(nil, authedLoader(), awsFixtureLister(), contextAware, + slowSelector, config.DefaultConfig()) + output, err := executeCommand(cmd, "--provider", "aws") + if err != nil { + t.Fatalf("elevation should succeed after a slow prompt, got: %v", err) + } + if !strings.Contains(output, "export AWS_ACCESS_KEY_ID='ASIAENV'") { + t.Errorf("unexpected output:\n%s", output) + } +} + +// TestAuthCacheFlag kills ELV-21 and ELV-22: both elevation paths must load +// authentication with cacheAuthentication=true, otherwise every invocation +// re-authenticates instead of reusing the cached token. +func TestAuthCacheFlag(t *testing.T) { + t.Run("env path", func(t *testing.T) { + var got []bool + loader := &mockAuthLoader{ + loadFunc: func(_ *sdkmodels.IdsecProfile, cacheAuthentication bool) (*authmodels.IdsecToken, error) { + got = append(got, cacheAuthentication) + return &authmodels.IdsecToken{Token: "test-jwt"}, nil + }, + } + + cmd := NewEnvCommandWithDeps(nil, loader, awsFixtureLister(), awsFixtureElevator(), + failingTargetSelector(t), config.DefaultConfig()) + if _, err := executeCommand(cmd, "--provider", "aws", "--target", "AWS Mgmt", "--role", "AdminAccess"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + assertCachedAuth(t, got) + }) + + t.Run("root path", func(t *testing.T) { + var got []bool + loader := &mockAuthLoader{ + loadFunc: func(_ *sdkmodels.IdsecProfile, cacheAuthentication bool) (*authmodels.IdsecToken, error) { + got = append(got, cacheAuthentication) + return &authmodels.IdsecToken{Token: "test-jwt"}, nil + }, + } + elevator := &mockElevateService{ + response: &models.ElevateResponse{Response: models.ElevateAccessResult{ + CSP: models.CSPAWS, + Results: []models.ElevateTargetResult{{SessionID: "sess-root"}}, + }}, + } + + cmd := NewRootCommandWithDeps(nil, loader, awsFixtureLister(), elevator, nil, + &mockGroupsEligibilityLister{response: &models.GroupsEligibilityResponse{}}, nil, config.DefaultConfig()) + if _, err := executeCommand(cmd, "--provider", "aws", "--target", "AWS Mgmt", "--role", "AdminAccess"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + assertCachedAuth(t, got) + }) +} + +func assertCachedAuth(t *testing.T, got []bool) { + t.Helper() + if len(got) != 1 { + t.Fatalf("expected exactly 1 LoadAuthentication call, got %d", len(got)) + } + if !got[0] { + t.Error("cacheAuthentication = false, want true") + } +} + +// TestEnv_SelectorReceivesAllTargets kills ELV-23: passing nil (or anything +// else) to SelectTarget in resolveAndElevate (cmd/root.go). mockTargetSelector +// returns its +// canned target without looking at the slice, so nothing else notices. +func TestEnv_SelectorReceivesAllTargets(t *testing.T) { + second := models.EligibleTarget{ + OrganizationID: "o-env-1", + WorkspaceID: "444455556666", + WorkspaceName: "AWS Sandbox", + WorkspaceType: models.WorkspaceTypeAccount, + RoleInfo: models.RoleInfo{ID: "role-env-2", Name: "ReadOnly"}, + } + lister := &mockEligibilityLister{ + response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{awsFixtureTarget(), second}, + Total: 2, + }, + } + + var offered []models.EligibleTarget + selector := &mockTargetSelector{ + selectFunc: func(targets []models.EligibleTarget) (*models.EligibleTarget, error) { + offered = append([]models.EligibleTarget(nil), targets...) + if len(targets) == 0 { + // Report rather than panic, so a mutation that passes nil + // fails with a readable message. + return nil, errors.New("selector received no targets") + } + return &targets[0], nil + }, + } + + cmd := NewEnvCommandWithDeps(nil, authedLoader(), lister, awsFixtureElevator(), selector, config.DefaultConfig()) + if _, err := executeCommand(cmd, "--provider", "aws"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(offered) != 2 { + t.Fatalf("selector received %d targets, want the 2 fetched", len(offered)) + } + if offered[0].WorkspaceID != "111122223333" || offered[1].WorkspaceID != "444455556666" { + t.Errorf("selector received %q/%q, want the fetched workspace IDs", + offered[0].WorkspaceID, offered[1].WorkspaceID) + } +} + +// TestExecute_VerboseHintCondition kills ELV-24: inverting the hint condition +// at cmd/root.go. The pre-existing test reconstructs the logic in the test +// body, so it cannot see the production condition change; this asserts the +// extracted predicate that Execute() actually calls. +func TestExecute_VerboseHintCondition(t *testing.T) { + tests := []struct { + name string + verboseOn bool + argValidationPassed bool + want bool + }{ + {name: "runtime error, verbose off", argValidationPassed: true, want: true}, + {name: "runtime error, verbose on", verboseOn: true, argValidationPassed: true, want: false}, + {name: "arg error, verbose off", want: false}, + {name: "arg error, verbose on", verboseOn: true, want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := shouldShowVerboseHint(tt.verboseOn, tt.argValidationPassed); got != tt.want { + t.Errorf("shouldShowVerboseHint(%v, %v) = %v, want %v", + tt.verboseOn, tt.argValidationPassed, got, tt.want) + } + }) + } +} + +// TestRootElevate_GroupAndGroupsPrecedence kills ELV-25: --group and --groups +// are not mutually exclusive, so their dispatch order in +// resolveAndElevateUnified (cmd/root.go) is a +// real, user-visible policy. --group names a specific group and must win over +// the --groups interactive filter. +func TestRootElevate_GroupAndGroupsPrecedence(t *testing.T) { + groupsLister := &mockGroupsEligibilityLister{ + response: &models.GroupsEligibilityResponse{ + Response: []models.GroupsEligibleTarget{ + {DirectoryID: "dir-1", GroupID: "grp-admins", GroupName: "Cloud Admins"}, + {DirectoryID: "dir-1", GroupID: "grp-readers", GroupName: "Cloud Readers"}, + }, + Total: 2, + }, + } + elevator := &mockGroupsElevator{ + response: &models.GroupsElevateResponse{ + DirectoryID: "dir-1", + CSP: models.CSPAzure, + Results: []models.GroupsElevateTargetResult{{GroupID: "grp-admins", SessionID: "sess-grp"}}, + }, + } + selector := &mockUnifiedSelector{ + selectFunc: func([]selectionItem) (*selectionItem, error) { + t.Error("--group must not fall through to the --groups interactive selector") + return nil, errors.New("selector must not be called") + }, + } + + cmd := NewRootCommandWithDeps(nil, authedLoader(), &mockEligibilityLister{ + response: &models.EligibilityResponse{}, + }, nil, selector, groupsLister, elevator, config.DefaultConfig()) + + output, err := executeCommand(cmd, "--group", "Cloud Admins", "--groups") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, output) + } + + req := elevator.lastElevateGroups() + if req == nil { + t.Fatal("ElevateGroups was never called") + } + if len(req.Targets) != 1 || req.Targets[0].GroupID != "grp-admins" { + t.Errorf("elevated %+v, want the group named by --group", req.Targets) + } + // Also pins the DirectoryID on the group elevate request. + if req.DirectoryID != "dir-1" { + t.Errorf("DirectoryID = %q, want dir-1", req.DirectoryID) + } +} + +// TestElevateGroup_RequestPayload pins the group elevation request body, +// including the DirectoryID that a blanking mutation would drop. +func TestElevateGroup_RequestPayload(t *testing.T) { + elevator := &mockGroupsElevator{ + response: &models.GroupsElevateResponse{ + DirectoryID: "dir-payload", + Results: []models.GroupsElevateTargetResult{{GroupID: "grp-payload", SessionID: "sess-1"}}, + }, + } + + if _, _, err := elevateGroup(t.Context(), &models.GroupsEligibleTarget{ + DirectoryID: "dir-payload", GroupID: "grp-payload", GroupName: "Cloud Admins", + }, elevator); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + req := elevator.lastElevateGroups() + if req == nil { + t.Fatal("ElevateGroups was never called") + } + if req.DirectoryID != "dir-payload" { + t.Errorf("DirectoryID = %q, want dir-payload", req.DirectoryID) + } + if req.CSP != models.CSPAzure { + t.Errorf("CSP = %q, want AZURE", req.CSP) + } + if len(req.Targets) != 1 || req.Targets[0].GroupID != "grp-payload" { + t.Errorf("Targets = %+v, want the selected group", req.Targets) + } +} diff --git a/cmd/integration_test.go b/cmd/integration_test.go index 11a1ea6..69cba81 100644 --- a/cmd/integration_test.go +++ b/cmd/integration_test.go @@ -314,6 +314,63 @@ func TestIntegration_InvalidCommand(t *testing.T) { } } +// TestIntegration_VerboseHint pins the hint's CALL SITE in Execute(), not just +// the shouldShowVerboseHint predicate. +// +// The unit-level coverage cannot do this: cmd/test_helpers_test.go's +// executeWithHint calls the same predicate, so it is a reimplementation of +// Execute()'s error path that can never disagree with it — deleting the whole +// `if shouldShowVerboseHint(...) { Fprintln(...) }` block from Execute() leaves +// `go test ./cmd/` green. Only the real binary exercises the wiring. +func TestIntegration_VerboseHint(t *testing.T) { + const hint = "Hint: re-run with --verbose for more details" + + tests := []struct { + name string + args []string + wantHint bool + why string + }{ + { + name: "runtime error prints the hint", + args: []string{"--provider", "azure"}, + wantHint: true, + why: "PersistentPreRunE ran, so --verbose would have added detail", + }, + { + name: "argument validation error does not", + args: []string{"nonexistent-command"}, + wantHint: false, + why: "PersistentPreRunE never ran, so --verbose would add nothing", + }, + { + name: "unknown flag does not", + args: []string{"--no-such-flag"}, + wantHint: false, + why: "flag parsing fails before PersistentPreRunE", + }, + { + name: "already verbose does not", + args: []string{"--verbose", "--provider", "azure"}, + wantHint: false, + why: "the hint tells the user to do what they already did", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := runGrant(t, isolatedEnv(t), tt.args...) + if got.exitCode != 1 { + t.Fatalf("exit code = %d, want 1\noutput:\n%s", got.exitCode, got.output) + } + if hasHint := got.contains(hint); hasHint != tt.wantHint { + t.Errorf("hint present = %v, want %v (%s)\noutput:\n%s", + hasHint, tt.wantHint, tt.why, 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. diff --git a/cmd/login_args_test.go b/cmd/login_args_test.go new file mode 100644 index 0000000..8c6f0b6 --- /dev/null +++ b/cmd/login_args_test.go @@ -0,0 +1,120 @@ +package cmd + +import ( + "errors" + "os" + "strings" + "testing" + + "github.com/cyberark/idsec-sdk-golang/pkg/models" + authmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/auth" + "github.com/cyberark/idsec-sdk-golang/pkg/profiles" + "github.com/mattn/go-isatty" +) + +// TestRunLogin_AutoConfiguresMissingProfile kills REQ-20: disabling the +// `if profile == nil` branch at cmd/login.go:52 would authenticate against a +// nil profile instead of running the configure flow. The feature IS +// implemented — login_test.go previously skipped this with the factually wrong +// reason "Auto-configure not yet implemented". +// +// The configure flow prompts through survey on a non-terminal stdin, which +// fails rather than blocking; the assertion is on the announcement plus the +// fact that authentication was never attempted. +func TestRunLogin_AutoConfiguresMissingProfile(t *testing.T) { + // KNOWN COVERAGE GAP (ledger COV-02). Under a PTY this test does not run at + // all, so the auto-configure branch has ZERO coverage there and REQ-20 is + // unpinned for anyone running `go test` from a terminal that hands the + // process a real stdin. + // + // There is no seam to close it with: runConfigure prompts through survey, + // which reads os.Stdin directly and offers no injection point, so on a real + // terminal this would block on input forever rather than fail. Skipping is + // the lesser evil. `go test`, CI and every non-interactive run get a + // non-TTY stdin and do exercise the branch — which is why this is accepted + // rather than fixed. Closing it properly means giving runConfigure a stdin + // seam first. + if isatty.IsTerminal(os.Stdin.Fd()) { + t.Skip("stdin is a terminal: the configure prompt would block on real input") + } + + // survey renders the configure prompt to os.Stdout directly, not to the + // cobra buffer, so without this the escape sequences (including ESC[6n, + // which the terminal answers on stdin) land on the developer's console. + withDiscardedStdout(t) + + // An empty profiles folder: LoadProfile("grant") finds nothing. + t.Setenv("IDSEC_PROFILES_FOLDER", t.TempDir()) + + auth := &mockAuthenticator{ + authenticateFunc: func(*models.IdsecProfile, *authmodels.IdsecAuthProfile, *authmodels.IdsecSecret, bool, bool) (*authmodels.IdsecToken, error) { + t.Error("authentication must not be attempted before the profile is configured") + return nil, errors.New("unreachable") + }, + } + + cmd := NewLoginCommandWithAuth(auth) + output, err := executeCommand(cmd) + + if !strings.Contains(output, "No configuration found") { + t.Errorf("expected the auto-configure announcement, got:\n%s", output) + } + if err == nil { + t.Error("expected configuration to fail without a terminal, got nil") + } +} + +// TestRunLogin_AuthenticateFlags kills REQ-21: swapping the force/refreshAuth +// arguments at cmd/login.go:74. `grant login` must force a fresh +// authentication (refreshAuth=true) without forcing a profile rewrite +// (force=false); the swap silently reuses a cached token. +func TestRunLogin_AuthenticateFlags(t *testing.T) { + t.Setenv("IDSEC_PROFILES_FOLDER", t.TempDir()) + + profile := &models.IdsecProfile{ + ProfileName: "grant", + AuthProfiles: map[string]*authmodels.IdsecAuthProfile{ + "isp": { + Username: "test.user@example.com", + AuthMethod: authmodels.Identity, + AuthMethodSettings: &authmodels.IdentityIdsecAuthMethodSettings{ + IdentityURL: "https://example.cyberark.cloud", + IdentityMFAInteractive: true, + }, + }, + }, + } + loader := &profiles.FileSystemProfilesLoader{} + if err := loader.SaveProfile(profile); err != nil { + t.Fatalf("failed to create test profile: %v", err) + } + + var gotForce, gotRefresh bool + var gotSecret *authmodels.IdsecSecret + var calls int + auth := &mockAuthenticator{ + authenticateFunc: func(_ *models.IdsecProfile, _ *authmodels.IdsecAuthProfile, secret *authmodels.IdsecSecret, force, refreshAuth bool) (*authmodels.IdsecToken, error) { + calls++ + gotForce, gotRefresh, gotSecret = force, refreshAuth, secret + return &authmodels.IdsecToken{Token: "jwt", Username: "test.user@example.com"}, nil + }, + } + + cmd := NewLoginCommandWithAuth(auth) + if _, err := executeCommand(cmd); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if calls != 1 { + t.Fatalf("expected exactly 1 Authenticate call, got %d", calls) + } + if gotForce { + t.Error("force = true, want false") + } + if !gotRefresh { + t.Error("refreshAuth = false, want true") + } + if gotSecret == nil || gotSecret.Secret != "" { + t.Errorf("secret = %+v, want an empty interactive secret", gotSecret) + } +} diff --git a/cmd/login_test.go b/cmd/login_test.go index c12da94..b6e63fe 100644 --- a/cmd/login_test.go +++ b/cmd/login_test.go @@ -259,7 +259,7 @@ func TestLoginCommandUsage(t *testing.T) { } } -func TestLoginCommandAutoConfigure(t *testing.T) { - // Placeholder test — auto-configure DI not yet wired - t.Skip("Auto-configure not yet implemented") -} +// Auto-configure is covered by TestRunLogin_AutoConfiguresMissingProfile in +// login_args_test.go. The former placeholder here skipped with the factually +// wrong reason "Auto-configure not yet implemented" — the feature is +// implemented (cmd/login.go:52). diff --git a/cmd/request_args_test.go b/cmd/request_args_test.go new file mode 100644 index 0000000..555723f --- /dev/null +++ b/cmd/request_args_test.go @@ -0,0 +1,630 @@ +package cmd + +// Argument-capture tests for the `grant request` subcommands. +// +// Each test names the mutation it kills. The pre-existing tests in +// request_test.go assert on printed text that the command generates locally +// (e.g. "rejected" comes from decisionPastTense, not from the wire), so they +// survive mutations to what is actually sent to the API. +// +// Fixture values are deliberately distinguishable — a swap mutation only dies +// when the two values differ. Do not "tidy" them back to a shared constant. + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/aaearon/grant-cli/internal/sca/models" + "github.com/aaearon/grant-cli/internal/ui" + wfmodels "github.com/aaearon/grant-cli/internal/workflows/models" +) + +// TestRequestReject_SendsRejectedDecision kills REQ-01: hardcoding the decision +// argument at cmd/request_finalize.go:100 (e.g. always "APPROVED"). The +// existing tests only assert the printed verb, which decisionPastTense derives +// from the local literal and not from what was sent. +func TestRequestReject_SendsRejectedDecision(t *testing.T) { + svc := &mockAccessRequestService{ + finalizeResult: &wfmodels.AccessRequest{ + RequestID: "req-reject-1", + RequestResult: wfmodels.RequestResultRejected, + }, + } + + cmd := NewRequestCommandWithDeps(svc) + root := newTestRootCommand() + root.AddCommand(cmd) + + output, err := executeCommand(root, "request", "reject", "req-reject-1", "--reason", "insufficient justification") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, output) + } + + if len(svc.finalizeCalls) != 1 { + t.Fatalf("expected exactly 1 FinalizeRequest call, got %d", len(svc.finalizeCalls)) + } + got := svc.lastFinalize() + if got.decision != "REJECTED" { + t.Errorf("decision sent = %q, want REJECTED", got.decision) + } + if got.requestID != "req-reject-1" { + t.Errorf("requestId sent = %q, want req-reject-1", got.requestID) + } + if !got.reasonSet || got.reason != "insufficient justification" { + t.Errorf("reason sent = %q (set=%v), want %q", got.reason, got.reasonSet, "insufficient justification") + } +} + +// TestRequestApprove_SendsApprovedDecision is the companion to the reject case: +// together they make a swap of the two literals fail. +func TestRequestApprove_SendsApprovedDecision(t *testing.T) { + svc := &mockAccessRequestService{ + finalizeResult: &wfmodels.AccessRequest{ + RequestID: "req-approve-1", + RequestResult: wfmodels.RequestResultApproved, + }, + } + + cmd := NewRequestCommandWithDeps(svc) + root := newTestRootCommand() + root.AddCommand(cmd) + + output, err := executeCommand(root, "request", "approve", "req-approve-1") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, output) + } + + got := svc.lastFinalize() + if got == nil { + t.Fatal("FinalizeRequest was never called") + } + if got.decision != "APPROVED" { + t.Errorf("decision sent = %q, want APPROVED", got.decision) + } + if got.requestID != "req-approve-1" { + t.Errorf("requestId sent = %q, want req-approve-1", got.requestID) + } + // No --reason: the command must send nil, not an empty string. + if got.reasonSet { + t.Errorf("reason should be nil when --reason is absent, got %q", got.reason) + } +} + +// TestRequestFinalize_EmptyReasonCollapsesToUnset pins the deliberate +// behavior of runFinalize's `if v != "" { reason = &v }`: an explicitly empty +// --reason is indistinguishable from no --reason at all on the wire. The mock's +// reasonSet flag is what makes the two expressible, so the distinction is +// asserted rather than merely representable. +func TestRequestFinalize_EmptyReasonCollapsesToUnset(t *testing.T) { + svc := &mockAccessRequestService{ + finalizeResult: &wfmodels.AccessRequest{ + RequestID: "req-approve-2", + RequestResult: wfmodels.RequestResultApproved, + }, + } + + cmd := NewRequestCommandWithDeps(svc) + root := newTestRootCommand() + root.AddCommand(cmd) + + output, err := executeCommand(root, "request", "approve", "req-approve-2", "--reason", "") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, output) + } + + if len(svc.finalizeCalls) != 1 { + t.Fatalf("expected exactly 1 FinalizeRequest call, got %d", len(svc.finalizeCalls)) + } + got := svc.lastFinalize() + if got.reasonSet { + t.Errorf(`--reason "" must collapse to a nil reason, got %q`, got.reason) + } +} + +// TestRequestCancel_PassesRequestID kills REQ-02: sending anything other than +// the requested ID at cmd/request_cancel.go:60. +func TestRequestCancel_PassesRequestID(t *testing.T) { + tests := []struct { + name string + args []string + wantReason string + wantReasonSet bool + }{ + {name: "without reason", args: []string{"request", "cancel", "req-cancel-7"}}, + { + name: "with reason", + args: []string{"request", "cancel", "req-cancel-7", "--reason", "changed my mind"}, + wantReason: "changed my mind", + wantReasonSet: true, + }, + { + // runRequestCancel deliberately collapses an explicitly empty + // --reason into "unset" (`if v != "" { reason = &v }`), so the API + // sees nil rather than a pointer to "". Pinned so that a future + // change to that collapse cannot slip through unnoticed. + name: "explicitly empty reason collapses to unset", + args: []string{"request", "cancel", "req-cancel-7", "--reason", ""}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc := &mockAccessRequestService{ + cancelResult: &wfmodels.AccessRequest{ + RequestID: "req-cancel-7", + RequestResult: wfmodels.RequestResultCanceled, + }, + } + + cmd := NewRequestCommandWithDeps(svc) + root := newTestRootCommand() + root.AddCommand(cmd) + + output, err := executeCommand(root, tt.args...) + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, output) + } + + if len(svc.cancelCalls) != 1 { + t.Fatalf("expected exactly 1 CancelRequest call, got %d", len(svc.cancelCalls)) + } + got := svc.lastCancel() + if got.requestID != "req-cancel-7" { + t.Errorf("requestId sent = %q, want req-cancel-7", got.requestID) + } + if got.reasonSet != tt.wantReasonSet || got.reason != tt.wantReason { + t.Errorf("reason sent = %q (set=%v), want %q (set=%v)", + got.reason, got.reasonSet, tt.wantReason, tt.wantReasonSet) + } + }) + } +} + +// TestRequestGet_PassesRequestID kills REQ-03: sending anything other than the +// requested ID at cmd/request_get.go:53. +func TestRequestGet_PassesRequestID(t *testing.T) { + svc := &mockAccessRequestService{ + getResult: &wfmodels.AccessRequest{ + RequestID: "req-get-42", + RequestState: wfmodels.RequestStateFinished, + RequestResult: wfmodels.RequestResultApproved, + CreatedBy: "user@test", + CreatedAt: "t", + UpdatedBy: "SYSTEM", + UpdatedAt: "t", + }, + } + + cmd := NewRequestCommandWithDeps(svc) + root := newTestRootCommand() + root.AddCommand(cmd) + + output, err := executeCommand(root, "request", "get", "req-get-42") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, output) + } + + if len(svc.getCalls) != 1 { + t.Fatalf("expected exactly 1 GetRequest call, got %d", len(svc.getCalls)) + } + if svc.getCalls[0] != "req-get-42" { + t.Errorf("requestId sent = %q, want req-get-42", svc.getCalls[0]) + } +} + +// TestRequestList_ParamsFromFlags kills REQ-04, REQ-06 and REQ-07: the +// --state filter, --search free text and the asc/desc sort direction must all +// reach ListRequestsParams. +func TestRequestList_ParamsFromFlags(t *testing.T) { + tests := []struct { + name string + args []string + wantSort string + wantFilt string + wantFree string + wantRole string + }{ + { + name: "default sort is descending", + args: []string{"request", "list"}, + wantSort: "createdAt desc", + }, + { + name: "desc=false sorts ascending", + args: []string{"request", "list", "--desc=false"}, + wantSort: "createdAt asc", + }, + { + name: "sort field is honored", + args: []string{"request", "list", "--sort", "updatedAt", "--desc=false"}, + wantSort: "updatedAt asc", + }, + { + name: "state becomes a filter", + args: []string{"request", "list", "--state", "pending"}, + wantSort: "createdAt desc", + wantFilt: "((requestState eq PENDING))", + }, + { + name: "search becomes free text", + args: []string{"request", "list", "--search", "prod-eastus"}, + wantSort: "createdAt desc", + wantFree: "prod-eastus", + }, + { + name: "role is uppercased and passed", + args: []string{"request", "list", "--role", "approver"}, + wantSort: "createdAt desc", + wantRole: "APPROVER", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc := &mockAccessRequestService{} + + cmd := NewRequestCommandWithDeps(svc) + root := newTestRootCommand() + root.AddCommand(cmd) + + output, err := executeCommand(root, tt.args...) + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, output) + } + + if len(svc.listCalls) != 1 { + t.Fatalf("expected exactly 1 ListRequests call, got %d", len(svc.listCalls)) + } + got := svc.lastListParams() + if got.Sort != tt.wantSort { + t.Errorf("Sort = %q, want %q", got.Sort, tt.wantSort) + } + if got.Filter != tt.wantFilt { + t.Errorf("Filter = %q, want %q", got.Filter, tt.wantFilt) + } + if got.FreeText != tt.wantFree { + t.Errorf("FreeText = %q, want %q", got.FreeText, tt.wantFree) + } + if got.RequestRole != tt.wantRole { + t.Errorf("RequestRole = %q, want %q", got.RequestRole, tt.wantRole) + } + }) + } +} + +// TestRequestList_RejectsInvalidRole kills REQ-05: dropping the --role +// validation at cmd/request_list.go:82 would forward an arbitrary role string +// to the API instead of failing locally. +func TestRequestList_RejectsInvalidRole(t *testing.T) { + svc := &mockAccessRequestService{} + + cmd := NewRequestCommandWithDeps(svc) + root := newTestRootCommand() + root.AddCommand(cmd) + + _, err := executeCommand(root, "request", "list", "--role", "AUDITOR") + if err == nil { + t.Fatal("expected an error for an invalid --role") + } + if !strings.Contains(err.Error(), "--role must be CREATOR or APPROVER") { + t.Errorf("error = %v, want the --role validation message", err) + } + if len(svc.listCalls) != 0 { + t.Errorf("ListRequests must not be called, got params %+v", svc.listCalls) + } +} + +// submitStub points resolveSubmitTargetFn at a fixed workspace and fails the +// test if role resolution is reached. +func submitStubWorkspace(t *testing.T, ws *submitWorkspace) { + t.Helper() + origTarget := resolveSubmitTargetFn + origRole := resolveRoleFn + t.Cleanup(func() { + resolveSubmitTargetFn = origTarget + resolveRoleFn = origRole + }) + resolveSubmitTargetFn = func(_ context.Context, _, _ string, _ bool) (*submitWorkspace, error) { + return ws, nil + } + resolveRoleFn = func(_ context.Context, _ *submitWorkspace, _ bool) (string, string, error) { + t.Fatal("role resolution must not run when --role-id is supplied") + return "", "", nil + } +} + +// TestRunRequestSubmit_SubmitPayload kills REQ-08, REQ-09 and REQ-10: the +// hardcoded TargetCategory, the workspace ID in the request details, and the +// timeFrom/timeTo pair (distinct values, so a swap dies). +// +// Table over both CSPs that buildRequestDetails special-cases. The AWS row is +// the load-bearing one: "AWS" is exactly where locationType stops being a +// naive string(ws.CSP), so an Azure-only fixture leaves that mapping unpinned. +// +// The assertion is exhaustive, not a subset: comparing len(RequestDetails) +// against len(wantDetails) is what makes an *extra* key fail too. +func TestRunRequestSubmit_SubmitPayload(t *testing.T) { + tests := []struct { + name string + csp models.CSP + workspaceType models.WorkspaceType + wantLocationType string + }{ + { + name: "azure subscription", + csp: models.CSPAzure, + workspaceType: models.WorkspaceTypeSubscription, + wantLocationType: "Azure", + }, + { + name: "aws account", + csp: models.CSPAWS, + workspaceType: models.WorkspaceTypeAccount, + wantLocationType: "AWS", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + submitStubWorkspace(t, &submitWorkspace{ + WorkspaceName: "Prod-EastUS", + WorkspaceID: "ws-payload-1", + WorkspaceType: tt.workspaceType, + CSP: tt.csp, + OrganizationID: "org-payload-9", + }) + + svc := &mockAccessRequestService{ + submitResult: &wfmodels.AccessRequest{ + RequestID: "req-new", + RequestState: wfmodels.RequestStatePending, + }, + } + + cmd := NewRequestCommandWithDeps(svc) + root := newTestRootCommand() + root.AddCommand(cmd) + + output, err := executeCommand(root, "request", "submit", + "--target", "Prod-EastUS", "--role-id", "role-payload-3", "--role", "Contributor", + "--reason", "need access", "--date", "2026-04-21", + "--timezone", "UTC", + // Distinguishable on purpose: identical values would not kill a swap. + "--from", "08:15", "--to", "19:45", + "--yes") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, output) + } + + if len(svc.submitCalls) != 1 { + t.Fatalf("expected exactly 1 SubmitRequest call, got %d", len(svc.submitCalls)) + } + sent := svc.lastSubmit() + if sent.TargetCategory != "CLOUD_CONSOLE" { + t.Errorf("targetCategory = %q, want CLOUD_CONSOLE", sent.TargetCategory) + } + + wantDetails := map[string]interface{}{ + "locationType": tt.wantLocationType, + "roleId": "role-payload-3", + "roleName": "Contributor", + "workspaceId": "ws-payload-1", + "workspaceName": "Prod-EastUS", + "workspaceType": string(tt.workspaceType), + "orgId": "org-payload-9", + "reason": "need access", + "priority": "Medium", + "requestDate": "2026-04-21", + "timezone": "UTC", + "timeFrom": "08:15", + "timeTo": "19:45", + } + if len(sent.RequestDetails) != len(wantDetails) { + t.Errorf("requestDetails has %d keys, want %d: got %v", + len(sent.RequestDetails), len(wantDetails), sent.RequestDetails) + } + for key, want := range wantDetails { + if got := sent.RequestDetails[key]; got != want { + t.Errorf("requestDetails[%q] = %v, want %v", key, got, want) + } + } + }) + } +} + +// TestRunRequestSubmit_InvokesValidation kills REQ-11: deleting the +// validateSubmitFields call site at cmd/request_submit.go:274. The existing +// coverage calls validateSubmitFields directly, so it cannot see the call site +// disappear. --priority is validated nowhere else in the flow. +func TestRunRequestSubmit_InvokesValidation(t *testing.T) { + submitStubWorkspace(t, &submitWorkspace{ + WorkspaceName: "Prod-EastUS", + WorkspaceID: "ws-1", + WorkspaceType: models.WorkspaceTypeSubscription, + CSP: models.CSPAzure, + OrganizationID: "org-1", + }) + + svc := &mockAccessRequestService{ + submitResult: &wfmodels.AccessRequest{RequestID: "must-not-be-reached"}, + } + + cmd := NewRequestCommandWithDeps(svc) + root := newTestRootCommand() + root.AddCommand(cmd) + + _, err := executeCommand(root, "request", "submit", + "--target", "Prod-EastUS", "--role-id", "role-1", + "--reason", "need access", "--priority", "Urgent", + "--date", "2026-04-21", "--timezone", "UTC", + "--from", "09:00", "--to", "17:00", + "--yes") + if err == nil { + t.Fatal("expected a validation error for --priority Urgent") + } + if !strings.Contains(err.Error(), "--priority must be High, Medium, or Low") { + t.Errorf("error = %v, want the --priority validation message", err) + } + // len(submitCalls) rather than lastSubmit(): the latter cannot tell + // "never called" apart from "called with a nil request". + if len(svc.submitCalls) != 0 { + t.Errorf("nothing may be submitted after a validation failure, got %+v", svc.submitCalls) + } +} + +// TestValidateSubmitFields_ErrorMessages kills REQ-12: the existing table only +// checks that an error occurred, so any message can be swapped for any other. +func TestValidateSubmitFields_ErrorMessages(t *testing.T) { + valid := submitFields{ + reason: "need access", priority: "Medium", date: "2026-04-21", + timezone: "UTC", timeFrom: "09:00", timeTo: "17:00", + } + + tests := []struct { + name string + mutate func(*submitFields) + wantErr string + }{ + {"missing reason", func(f *submitFields) { f.reason = "" }, "--reason is required"}, + {"bad priority", func(f *submitFields) { f.priority = "Urgent" }, `--priority must be High, Medium, or Low (got "Urgent")`}, + {"missing date", func(f *submitFields) { f.date = "" }, "--date is required"}, + {"bad date", func(f *submitFields) { f.date = "21-04-2026" }, `--date must be in YYYY-MM-DD format (got "21-04-2026")`}, + {"missing timezone", func(f *submitFields) { f.timezone = "" }, "--timezone is required"}, + {"bad timezone", func(f *submitFields) { f.timezone = "Eastern" }, `--timezone must be a valid TZ identifier`}, + {"missing from", func(f *submitFields) { f.timeFrom = "" }, "--from is required"}, + {"bad from", func(f *submitFields) { f.timeFrom = "9am" }, `--from must be in HH:MM format (got "9am")`}, + {"missing to", func(f *submitFields) { f.timeTo = "" }, "--to is required"}, + {"bad to", func(f *submitFields) { f.timeTo = "5pm" }, `--to must be in HH:MM format (got "5pm")`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := valid + tt.mutate(&f) + err := validateSubmitFields(&f) + if err == nil { + t.Fatalf("expected an error for %s", tt.name) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("error = %q, want it to contain %q", err.Error(), tt.wantErr) + } + }) + } + + if err := validateSubmitFields(&valid); err != nil { + t.Errorf("the valid fixture must pass, got %v", err) + } +} + +// TestRequestNonInteractiveRequiresID kills REQ-13, REQ-14 and REQ-15: the +// early non-interactive guard on get/approve/reject. Only cancel had coverage. +// +// Each command is built with a nil service so that, if the guard is disabled, +// the flow reaches bootstrap and returns the bootstrap sentinel instead — a +// different error, which is exactly what makes the mutant die. +func TestRequestNonInteractiveRequiresID(t *testing.T) { + tests := []struct { + name string + cmd string + }{ + {name: "get", cmd: "get"}, + {name: "approve", cmd: "approve"}, + {name: "reject", cmd: "reject"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + withInteractiveTTY(t, false) + + cmd := NewRequestCommandWithDeps(nil) + root := newTestRootCommand() + root.AddCommand(cmd) + + _, err := executeCommand(root, "request", tt.cmd) + if err == nil { + t.Fatalf("expected an error for %q without a request ID", tt.cmd) + } + if !errors.Is(err, ui.ErrNotInteractive) { + t.Errorf("expected ErrNotInteractive (bootstrap not reached), got %v", err) + } + if !strings.Contains(err.Error(), "grant request list") { + t.Errorf("error should hint at 'grant request list', got %v", err) + } + }) + } +} + +// TestRejectGCPWorkspace_CSPTagOnly kills REQ-16. The existing fixture sets +// both a GCP CSP tag and a GCP workspace type, so the workspace-type switch +// masks the loss of the CSP arm. Here the workspace type is a non-GCP one, so +// only the CSP check can reject it. +func TestRejectGCPWorkspace_CSPTagOnly(t *testing.T) { + err := rejectGCPWorkspace(&submitWorkspace{ + WorkspaceName: "Mislabelled", + WorkspaceID: "ws-1", + WorkspaceType: models.WorkspaceTypeSubscription, // deliberately NOT a GCP type + CSP: models.CSPGCP, + }) + if err == nil { + t.Fatal("a GCP-tagged workspace must be rejected on the CSP tag alone") + } + if !errors.Is(err, errGCPRequestUnsupported) { + t.Errorf("error = %v, want errGCPRequestUnsupported", err) + } + + // The companion arm: a GCP workspace type with no CSP tag. + if err := rejectGCPWorkspace(&submitWorkspace{ + WorkspaceType: models.WorkspaceTypeFolder, + }); !errors.Is(err, errGCPRequestUnsupported) { + t.Errorf("GCP workspace type must be rejected, got %v", err) + } + + // And a plain Azure workspace must still pass. + if err := rejectGCPWorkspace(&submitWorkspace{ + WorkspaceType: models.WorkspaceTypeSubscription, + CSP: models.CSPAzure, + }); err != nil { + t.Errorf("an Azure workspace must not be rejected, got %v", err) + } +} + +// TestRunRequestSubmit_NonInteractiveRequiresRoleID kills REQ-22: dropping the +// interactivity guard at cmd/request_submit.go:251 would drop a non-interactive +// caller into the interactive role picker. +func TestRunRequestSubmit_NonInteractiveRequiresRoleID(t *testing.T) { + withInteractiveTTY(t, false) + submitStubWorkspace(t, &submitWorkspace{ + WorkspaceName: "Prod-EastUS", + WorkspaceID: "ws-1", + WorkspaceType: models.WorkspaceTypeSubscription, + CSP: models.CSPAzure, + OrganizationID: "org-1", + }) + + svc := &mockAccessRequestService{ + submitResult: &wfmodels.AccessRequest{RequestID: "must-not-be-reached"}, + } + + cmd := NewRequestCommandWithDeps(svc) + root := newTestRootCommand() + root.AddCommand(cmd) + + _, err := executeCommand(root, "request", "submit", + "--target", "Prod-EastUS", + "--reason", "need access", "--date", "2026-04-21", + "--timezone", "UTC", "--from", "09:00", "--to", "17:00", + "--yes") + if err == nil { + t.Fatal("expected an error when --role-id is absent in non-interactive mode") + } + if !strings.Contains(err.Error(), "requires --role-id") { + t.Errorf("error = %v, want it to demand --role-id", err) + } + // len(submitCalls) rather than lastSubmit(): the latter cannot tell + // "never called" apart from "called with a nil request". + if len(svc.submitCalls) != 0 { + t.Errorf("nothing may be submitted, got %+v", svc.submitCalls) + } +} diff --git a/cmd/request_picker_test.go b/cmd/request_picker_test.go index c04ebdb..8d461e4 100644 --- a/cmd/request_picker_test.go +++ b/cmd/request_picker_test.go @@ -7,32 +7,13 @@ import ( "testing" "github.com/aaearon/grant-cli/internal/ui" - "github.com/aaearon/grant-cli/internal/workflows" wfmodels "github.com/aaearon/grant-cli/internal/workflows/models" ) -// capturingMockAccessRequestService embeds mockAccessRequestService and captures list params. -type capturingMockAccessRequestService struct { - mockAccessRequestService - lastListParams workflows.ListRequestsParams -} - -func (m *capturingMockAccessRequestService) ListRequests(_ context.Context, params workflows.ListRequestsParams) ([]wfmodels.AccessRequest, int, error) { - m.lastListParams = params - return m.listItems, m.listTotalCount, m.listErr -} - -func withInteractiveTTY(t *testing.T, interactive bool) { - t.Helper() - orig := ui.IsTerminalFunc - t.Cleanup(func() { ui.IsTerminalFunc = orig }) - ui.IsTerminalFunc = func(_ uintptr) bool { return interactive } -} - func TestResolveRequestIDInteractive_NonInteractive(t *testing.T) { withInteractiveTTY(t, false) - svc := &capturingMockAccessRequestService{} + svc := &mockAccessRequestService{} _, err := resolveRequestIDInteractive(t.Context(), svc, pickerScope{emptyMsg: "access requests"}) if err == nil { t.Fatal("expected error") @@ -48,7 +29,7 @@ func TestResolveRequestIDInteractive_NonInteractive(t *testing.T) { func TestResolveRequestIDInteractive_EmptyList(t *testing.T) { withInteractiveTTY(t, true) - svc := &capturingMockAccessRequestService{} + svc := &mockAccessRequestService{} _, err := resolveRequestIDInteractive(t.Context(), svc, pickerScope{ filter: "(requestState eq PENDING)", requestRole: "APPROVER", @@ -60,23 +41,22 @@ func TestResolveRequestIDInteractive_EmptyList(t *testing.T) { if !strings.Contains(err.Error(), "pending requests assigned to you") { t.Errorf("expected emptyMsg in error, got %v", err) } - if svc.lastListParams.Filter != "(requestState eq PENDING)" { - t.Errorf("filter: got %q", svc.lastListParams.Filter) + got := svc.lastListParams() + if got.Filter != "(requestState eq PENDING)" { + t.Errorf("filter: got %q", got.Filter) } - if svc.lastListParams.RequestRole != "APPROVER" { - t.Errorf("requestRole: got %q", svc.lastListParams.RequestRole) + if got.RequestRole != "APPROVER" { + t.Errorf("requestRole: got %q", got.RequestRole) } - if svc.lastListParams.Sort != "createdAt desc" { - t.Errorf("sort: got %q", svc.lastListParams.Sort) + if got.Sort != "createdAt desc" { + t.Errorf("sort: got %q", got.Sort) } } func TestResolveRequestIDInteractive_ListError(t *testing.T) { withInteractiveTTY(t, true) - svc := &capturingMockAccessRequestService{ - mockAccessRequestService: mockAccessRequestService{listErr: errors.New("boom")}, - } + svc := &mockAccessRequestService{listErr: errors.New("boom")} _, err := resolveRequestIDInteractive(t.Context(), svc, pickerScope{emptyMsg: "x"}) if err == nil || !strings.Contains(err.Error(), "boom") { t.Fatalf("expected list error, got %v", err) diff --git a/cmd/request_test.go b/cmd/request_test.go index d9121ba..5cc2164 100644 --- a/cmd/request_test.go +++ b/cmd/request_test.go @@ -188,6 +188,7 @@ func TestRequestCancelCommand(t *testing.T) { name string svc *mockAccessRequestService args []string + nonTTY bool wantContain []string wantErr bool }{ @@ -220,15 +221,23 @@ func TestRequestCancelCommand(t *testing.T) { wantErr: true, }, { - name: "cancel no args", - svc: &mockAccessRequestService{}, - args: []string{"cancel"}, - wantErr: true, + // Not merely "errors": without an explicit non-TTY this case + // passed only because `go test` happens to run with a non-terminal + // stdin, and it asserted nothing about which error came back. + name: "cancel no args without a terminal", + svc: &mockAccessRequestService{}, + args: []string{"cancel"}, + nonTTY: true, + wantErr: true, + wantContain: []string{"interactive selection requires a terminal"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.nonTTY { + withInteractiveTTY(t, false) + } cmd := NewRequestCommandWithDeps(tt.svc) root := newTestRootCommand() root.AddCommand(cmd) @@ -617,6 +626,8 @@ func TestRunRequestSubmit_ServiceError(t *testing.T) { } func TestRunRequestSubmit_MissingFlags_NonInteractive(t *testing.T) { + withInteractiveTTY(t, false) + original := resolveSubmitTargetFn defer func() { resolveSubmitTargetFn = original }() @@ -767,7 +778,7 @@ func TestRunRequestSubmit_InteractiveRoleSelection(t *testing.T) { t.Fatalf("unexpected error: %v\noutput: %s", err, output) } - submitted := svc.submitRequest + submitted := svc.lastSubmit() if submitted == nil { t.Fatal("expected a submitted request") } @@ -1017,8 +1028,10 @@ func TestRunRequestSubmit_GCPWorkspaceWithRoleIDRejected(t *testing.T) { if !strings.Contains(err.Error(), "not supported for GCP") { t.Errorf("error %q does not say GCP is unsupported", err.Error()) } - if svc.submitRequest != nil { - t.Errorf("request must not be submitted, got %+v", svc.submitRequest) + // len(submitCalls) rather than lastSubmit(): the latter cannot + // tell "never called" apart from "called with a nil request". + if len(svc.submitCalls) != 0 { + t.Errorf("request must not be submitted, got %+v", svc.submitCalls) } }) } diff --git a/cmd/root.go b/cmd/root.go index 6fef39b..7d6657e 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -292,11 +292,21 @@ func executeWithKeyringOverride(cmd *cobra.Command) error { return cmd.Execute() } +// shouldShowVerboseHint reports whether the "re-run with --verbose" hint +// belongs on a failed run. The hint is pointless when --verbose is already on, +// and misleading for an arg/flag validation error — PersistentPreRunE never +// ran, so --verbose would not have produced any extra detail either. +// +// Extracted so it is testable without re-implementing the condition in a test. +func shouldShowVerboseHint(verboseOn, argValidationPassed bool) bool { + return !verboseOn && argValidationPassed +} + func Execute() { passedArgValidation = false if err := executeWithKeyringOverride(rootCmd); err != nil { fmt.Fprintln(rootCmd.ErrOrStderr(), err) - if !verbose && passedArgValidation { + if shouldShowVerboseHint(verbose, passedArgValidation) { fmt.Fprintln(rootCmd.ErrOrStderr(), "Hint: re-run with --verbose for more details") } os.Exit(1) diff --git a/cmd/root_elevate_test.go b/cmd/root_elevate_test.go index 164449e..55141b1 100644 --- a/cmd/root_elevate_test.go +++ b/cmd/root_elevate_test.go @@ -244,79 +244,6 @@ func TestRootElevate_InteractiveMode(t *testing.T) { }, wantErr: false, }, - { - name: "multi-CSP concurrent fetch - parallel execution", - setupMocks: func() (*mockAuthLoader, *mockEligibilityLister, *mockElevateService, *mockUnifiedSelector, *config.Config) { - authLoader := &mockAuthLoader{ - token: &authmodels.IdsecToken{ - Token: "test-jwt", - Username: "test@example.com", - ExpiresIn: commonmodels.IdsecRFC3339Time(time.Now().Add(1 * time.Hour)), - }, - } - - awsTarget := models.EligibleTarget{ - OrganizationID: "o-abc", - WorkspaceID: "111222333444", - WorkspaceName: "AWS Sandbox", - WorkspaceType: models.WorkspaceTypeAccount, - RoleInfo: models.RoleInfo{ID: "role-aws", Name: "ReadOnly"}, - } - azureTarget := models.EligibleTarget{ - OrganizationID: "org-xyz", - WorkspaceID: "sub-999", - WorkspaceName: "Prod-EastUS", - WorkspaceType: models.WorkspaceTypeSubscription, - RoleInfo: models.RoleInfo{ID: "role-az", Name: "Contributor"}, - } - - // Each CSP call sleeps 200ms; if sequential total >= 400ms - eligibilityLister := &mockEligibilityLister{ - listFunc: func(ctx context.Context, csp models.CSP) (*models.EligibilityResponse, error) { - time.Sleep(200 * time.Millisecond) - switch csp { - case models.CSPAzure: - return &models.EligibilityResponse{Response: []models.EligibleTarget{azureTarget}, Total: 1}, nil - case models.CSPAWS: - return &models.EligibilityResponse{Response: []models.EligibleTarget{awsTarget}, Total: 1}, nil - } - return &models.EligibilityResponse{}, nil - }, - } - - credsJSON := `{"aws_access_key":"ASIAXXX","aws_secret_access_key":"secret","aws_session_token":"token"}` - elevateService := &mockElevateService{ - response: &models.ElevateResponse{ - Response: models.ElevateAccessResult{ - CSP: models.CSPAWS, - OrganizationID: "o-abc", - Results: []models.ElevateTargetResult{ - { - WorkspaceID: "111222333444", - RoleID: "ReadOnly", - SessionID: "session-par", - AccessCredentials: &credsJSON, - }, - }, - }, - }, - } - - selector := &mockUnifiedSelector{ - item: &selectionItem{kind: selectionCloud, cloud: &awsTarget}, - } - - cfg := config.DefaultConfig() - - return authLoader, eligibilityLister, elevateService, selector, cfg - }, - args: []string{}, // no --provider triggers multi-CSP - wantContain: []string{ - "Elevated to ReadOnly on AWS Sandbox", - "Session ID: session-par", - }, - wantErr: false, - }, { name: "AWS elevation without credentials should not show Azure message", setupMocks: func() (*mockAuthLoader, *mockEligibilityLister, *mockElevateService, *mockUnifiedSelector, *config.Config) { @@ -1227,9 +1154,12 @@ func TestFetchEligibility_ConcurrentExecution(t *testing.T) { t.Fatalf("expected 2 targets, got %d", len(targets)) } - // With 2 CSPs sleeping 200ms each, parallel should finish well under 400ms - if elapsed >= 350*time.Millisecond { - t.Errorf("expected concurrent execution (<350ms), took %v", elapsed) + // Three CSPs sleep 200ms each: sequential would take ~600ms, concurrent + // ~200ms. The bound is deliberately loose — it only has to separate those + // two regimes, and a tight one would be the first thing to flake on an + // overloaded CI runner. No flake has been observed at the old 350ms bound. + if elapsed >= 500*time.Millisecond { + t.Errorf("expected concurrent execution (<500ms), took %v", elapsed) } // Verify CSP tags were set diff --git a/cmd/test_helpers.go b/cmd/test_helpers_test.go similarity index 65% rename from cmd/test_helpers.go rename to cmd/test_helpers_test.go index 45d4f47..38d0c1c 100644 --- a/cmd/test_helpers.go +++ b/cmd/test_helpers_test.go @@ -4,10 +4,49 @@ package cmd import ( "bytes" + "os" + "testing" + "github.com/aaearon/grant-cli/internal/ui" "github.com/spf13/cobra" ) +// withInteractiveTTY forces ui.IsInteractive() to the given answer for the +// duration of the test, restoring the package global via t.Cleanup. +// +// Use it in every test whose behavior depends on interactivity: `go test` +// happens to run with a non-TTY stdin, but that is an accident of the harness, +// not an assertion, and it silently reverses under a PTY. +func withInteractiveTTY(t *testing.T, interactive bool) { + t.Helper() + orig := ui.IsTerminalFunc + t.Cleanup(func() { ui.IsTerminalFunc = orig }) + ui.IsTerminalFunc = func(_ uintptr) bool { return interactive } +} + +// withDiscardedStdout points os.Stdout at a throwaway file for the duration of +// the test, restoring it via t.Cleanup. +// +// survey writes its prompts straight to os.Stdout rather than to the cobra +// output buffer, so any test that reaches a prompt sprays terminal control +// sequences over the developer's console. One of them, ESC[6n (Device Status +// Report), makes the terminal write its reply back on stdin, which corrupts the +// shell prompt after `go test`. Redirecting os.Stdout keeps that off the real +// terminal without adding a production seam to the prompting code. +func withDiscardedStdout(t *testing.T) { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "stdout-") + if err != nil { + t.Fatalf("creating stdout sink: %v", err) + } + orig := os.Stdout + t.Cleanup(func() { + os.Stdout = orig + _ = f.Close() + }) + os.Stdout = f +} + // newTestRootCommand creates a root command for testing (no elevation RunE) func newTestRootCommand() *cobra.Command { return newRootCommand(nil) @@ -84,7 +123,9 @@ func executeWithHint(cmd *cobra.Command, args []string) string { return "" } out := err.Error() + "\n" - if !verbose && passedArgValidation { + // Call the production predicate rather than restating it, so this helper + // cannot drift away from Execute(). + if shouldShowVerboseHint(verbose, passedArgValidation) { out += "Hint: re-run with --verbose for more details\n" } return out diff --git a/cmd/test_mocks.go b/cmd/test_mocks_test.go similarity index 61% rename from cmd/test_mocks.go rename to cmd/test_mocks_test.go index 75e0495..721b2e1 100644 --- a/cmd/test_mocks.go +++ b/cmd/test_mocks_test.go @@ -58,20 +58,57 @@ func (m *mockEligibilityLister) ListEligibility(ctx context.Context, csp models. return m.response, m.listErr } -// mockElevateService implements the elevateService interface for testing +// mockElevateService implements the elevateService interface for testing. +// +// Capture convention (see mockSessionRevoker): the request is recorded in the +// method body *before* dispatching to elevateFunc, so a test that supplies a +// callback still gets the history. +// +// No mutex, deliberately. The only mocks reached from more than one goroutine +// are the eligibility listers, via the fan-outs in fetchEligibility and +// fetchGroupsEligibility (cmd/root.go), resolveAndElevateUnifiedPath +// (cmd/root.go) and fetchAllTargets/fetchAllGroups (cmd/helpers.go). Every +// Elevate call site is sequential — resolveAndElevate and elevateCloud, the +// latter reached only after those fan-out channels have been joined. +// `make test-race` is what keeps that assumption honest. +// +// Function names, not line numbers: an unrelated insertion elsewhere in +// cmd/root.go already invalidated the line-number form of this comment once. type mockElevateService struct { elevateFunc func(ctx context.Context, req *models.ElevateRequest) (*models.ElevateResponse, error) response *models.ElevateResponse elevateErr error + + // elevateCalls records every request sent, defensively copied so a later + // mutation by production code cannot rewrite history. + elevateCalls []models.ElevateRequest } func (m *mockElevateService) Elevate(ctx context.Context, req *models.ElevateRequest) (*models.ElevateResponse, error) { + if req != nil { + captured := *req + captured.Targets = append([]models.ElevateTarget(nil), req.Targets...) + m.elevateCalls = append(m.elevateCalls, captured) + } if m.elevateFunc != nil { return m.elevateFunc(ctx, req) } return m.response, m.elevateErr } +// lastElevate returns the most recent request, or nil if Elevate never ran. +// +// Returns a pointer to a copy, not into the history's backing array: a test +// that holds the pointer across a later call would otherwise read a stale +// array once append reallocates. +func (m *mockElevateService) lastElevate() *models.ElevateRequest { + if len(m.elevateCalls) == 0 { + return nil + } + c := m.elevateCalls[len(m.elevateCalls)-1] + return &c +} + // mockTargetSelector implements the targetSelector interface for testing type mockTargetSelector struct { selectFunc func(targets []models.EligibleTarget) (*models.EligibleTarget, error) @@ -202,20 +239,39 @@ func (m *mockGroupsEligibilityLister) ListGroupsEligibility(ctx context.Context, return m.response, m.listErr } -// mockGroupsElevator implements groupsElevator for testing +// mockGroupsElevator implements groupsElevator for testing. +// Same capture convention and same no-mutex reasoning as mockElevateService: +// the sole call site, elevateGroup (cmd/root.go), is sequential. type mockGroupsElevator struct { elevateFunc func(ctx context.Context, req *models.GroupsElevateRequest) (*models.GroupsElevateResponse, error) response *models.GroupsElevateResponse elevateErr error + + elevateCalls []models.GroupsElevateRequest } func (m *mockGroupsElevator) ElevateGroups(ctx context.Context, req *models.GroupsElevateRequest) (*models.GroupsElevateResponse, error) { + if req != nil { + captured := *req + captured.Targets = append([]models.GroupsElevateTarget(nil), req.Targets...) + m.elevateCalls = append(m.elevateCalls, captured) + } if m.elevateFunc != nil { return m.elevateFunc(ctx, req) } return m.response, m.elevateErr } +// lastElevateGroups returns the most recent request, or nil if never called. +// Returns a pointer to a copy; see mockElevateService.lastElevate. +func (m *mockGroupsElevator) lastElevateGroups() *models.GroupsElevateRequest { + if len(m.elevateCalls) == 0 { + return nil + } + c := m.elevateCalls[len(m.elevateCalls)-1] + return &c +} + // mockUnifiedSelector implements unifiedSelector for testing type mockUnifiedSelector struct { selectFunc func(items []selectionItem) (*selectionItem, error) @@ -247,7 +303,37 @@ func (m *mockSelfUpdater) UpdateSelf(ctx context.Context, current string) (newVe return m.newVersion, m.updated, m.updateErr } -// mockAccessRequestService implements accessRequestService for testing +// cancelCall records one CancelRequest invocation. +// +// The optional *string reason is flattened into a value plus a set flag: +// runRequestCancel passes nil when --reason is empty, and a test must be able +// to tell nil from "". +type cancelCall struct { + requestID string + reason string + reasonSet bool +} + +// finalizeCall records one FinalizeRequest invocation, with the same *string +// flattening as cancelCall. +type finalizeCall struct { + requestID string + decision string + reason string + reasonSet bool +} + +// mockAccessRequestService implements accessRequestService for testing. +// +// Every method records its arguments before returning, so tests assert on what +// the command actually sent rather than on the canned return value. This is the +// only access-request mock: an arg-blind variant would silently opt future +// tests out of that. +// +// No mutex, deliberately — all five methods are invoked from strictly +// sequential command paths (cmd/request_{list,get,submit,cancel,finalize}.go +// and cmd/request_picker.go). Only the eligibility listers fan out across +// goroutines. `make test-race` guards the assumption. type mockAccessRequestService struct { listItems []wfmodels.AccessRequest listTotalCount int @@ -256,34 +342,99 @@ type mockAccessRequestService struct { getErr error submitResult *wfmodels.AccessRequest submitErr error - submitRequest *wfmodels.SubmitAccessRequest cancelResult *wfmodels.AccessRequest cancelErr error finalizeResult *wfmodels.AccessRequest finalizeErr error + + // Call histories. A history (rather than a single "last" field) is what + // lets a test assert "called exactly once". + listCalls []workflows.ListRequestsParams + getCalls []string + submitCalls []wfmodels.SubmitAccessRequest + cancelCalls []cancelCall + finalizeCalls []finalizeCall } -func (m *mockAccessRequestService) ListRequests(_ context.Context, _ workflows.ListRequestsParams) ([]wfmodels.AccessRequest, int, error) { +func (m *mockAccessRequestService) ListRequests(_ context.Context, params workflows.ListRequestsParams) ([]wfmodels.AccessRequest, int, error) { + m.listCalls = append(m.listCalls, params) return m.listItems, m.listTotalCount, m.listErr } -func (m *mockAccessRequestService) GetRequest(_ context.Context, _ string) (*wfmodels.AccessRequest, error) { +func (m *mockAccessRequestService) GetRequest(_ context.Context, requestID string) (*wfmodels.AccessRequest, error) { + m.getCalls = append(m.getCalls, requestID) return m.getResult, m.getErr } func (m *mockAccessRequestService) SubmitRequest(_ context.Context, req *wfmodels.SubmitAccessRequest) (*wfmodels.AccessRequest, error) { - m.submitRequest = req + if req != nil { + captured := *req + captured.RequestDetails = make(map[string]interface{}, len(req.RequestDetails)) + for k, v := range req.RequestDetails { + captured.RequestDetails[k] = v + } + m.submitCalls = append(m.submitCalls, captured) + } return m.submitResult, m.submitErr } -func (m *mockAccessRequestService) CancelRequest(_ context.Context, _ string, _ *string) (*wfmodels.AccessRequest, error) { +func (m *mockAccessRequestService) CancelRequest(_ context.Context, requestID string, reason *string) (*wfmodels.AccessRequest, error) { + call := cancelCall{requestID: requestID} + if reason != nil { + call.reason, call.reasonSet = *reason, true + } + m.cancelCalls = append(m.cancelCalls, call) return m.cancelResult, m.cancelErr } -func (m *mockAccessRequestService) FinalizeRequest(_ context.Context, _, _ string, _ *string) (*wfmodels.AccessRequest, error) { +func (m *mockAccessRequestService) FinalizeRequest(_ context.Context, requestID, decision string, reason *string) (*wfmodels.AccessRequest, error) { + call := finalizeCall{requestID: requestID, decision: decision} + if reason != nil { + call.reason, call.reasonSet = *reason, true + } + m.finalizeCalls = append(m.finalizeCalls, call) return m.finalizeResult, m.finalizeErr } +// lastListParams returns the most recent ListRequests params, or the zero value +// if ListRequests never ran. +func (m *mockAccessRequestService) lastListParams() workflows.ListRequestsParams { + if len(m.listCalls) == 0 { + return workflows.ListRequestsParams{} + } + return m.listCalls[len(m.listCalls)-1] +} + +// lastSubmit returns the most recent submitted request, or nil if never called. +// Returns a pointer to a copy; see mockElevateService.lastElevate. +func (m *mockAccessRequestService) lastSubmit() *wfmodels.SubmitAccessRequest { + if len(m.submitCalls) == 0 { + return nil + } + c := m.submitCalls[len(m.submitCalls)-1] + return &c +} + +// lastCancel returns the most recent cancel call, or nil if never called. +// Returns a pointer to a copy; see mockElevateService.lastElevate. +func (m *mockAccessRequestService) lastCancel() *cancelCall { + if len(m.cancelCalls) == 0 { + return nil + } + c := m.cancelCalls[len(m.cancelCalls)-1] + return &c +} + +// lastFinalize returns the most recent finalize call, or nil if never called. +// Returns a pointer to a copy; see mockElevateService.lastElevate. +func (m *mockAccessRequestService) lastFinalize() *finalizeCall { + if len(m.finalizeCalls) == 0 { + return nil + } + c := m.finalizeCalls[len(m.finalizeCalls)-1] + return &c +} + // countingEligibilityLister wraps an eligibilityLister and counts calls per CSP. // Thread-safe for concurrent access from goroutines in fetchStatusData etc. type countingEligibilityLister struct { diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index fb74612..bbe8f8e 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -49,28 +49,28 @@ premise does not hold). | 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-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` + `TestRequestApprove_SendsApprovedDecision` | PR4 | done | +| 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 | done | +| REQ-03 | cmd/request get | `cmd/request_get.go:53` | `svc.GetRequest(ctx, requestID)` → `svc.GetRequest(ctx, "WRONG-ID")` | CONFIRMED | test | `TestRequestGet_PassesRequestID` | PR4 | done | +| 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_ParamsFromFlags` | PR4 | done | +| REQ-05 | cmd/request list | `cmd/request_list.go:82` | `if role != "CREATOR" && role != "APPROVER" {` → `if false {` | CONFIRMED | test | `TestRequestList_RejectsInvalidRole` | PR4 | done | +| REQ-06 | cmd/request list | `cmd/request_list.go:78` | `params.FreeText = v` → `params.FreeText = ""` | CONFIRMED | test | `TestRequestList_ParamsFromFlags` | PR4 | done | +| REQ-07 | cmd/request list | `cmd/request_list.go:58` | Delete `filters = append(filters, fmt.Sprintf("(requestState eq %s)", upper))` | CONFIRMED | test | `TestRequestList_ParamsFromFlags` | PR4 | done | +| REQ-08 | cmd/request submit | `cmd/request_submit.go:306` | `TargetCategory: "CLOUD_CONSOLE"` → `TargetCategory: "WRONG"` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | done | +| REQ-09 | cmd/request submit | `cmd/request_submit.go:507` | `"workspaceId": ws.WorkspaceID,` → `"workspaceId": "",` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | done | +| 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 | done | +| 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 | done | +| 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 | done | +| 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 | `TestRequestNonInteractiveRequiresID/get` | PR4 | done | +| 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 | `TestRequestNonInteractiveRequiresID/approve` | PR4 | done | +| REQ-15 | cmd/request reject | `cmd/request_finalize.go:16` | Same as REQ-14 for the `reject` path, with `_ = requestID` | CONFIRMED | test | `TestRequestNonInteractiveRequiresID/reject` | PR4 | done | +| 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 | done | | 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-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 | done | +| 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 | done | +| REQ-22 | cmd/request submit | `cmd/request_submit.go:251` | `if !ui.IsInteractive() {` → `if false {` inside the `roleID == ""` branch | CONFIRMED | test | `TestRunRequestSubmit_NonInteractiveRequiresRoleID` | PR4 | done | | 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:258` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `selectFavoriteInteractive` | CONFIRMED | test | `TestFavoritesAddInteractive_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | @@ -150,33 +150,33 @@ premise does not hold). | SCA-24 | internal/sca models | `internal/sca/models/groups.go:41` | `CSP CSP \`json:"csp"\`` → `json:"Zcsp"` on `GroupsElevateResponse` | CONFIRMED | test | `TestGroupsElevateResponse_DecodesPopulatedResult` | PR8 | done | | WF-24 | internal/workflows models | `internal/workflows/models/request.go:32`, `:63` (and the rest of `AccessRequest`, `Entity`, `ApproverAction`, `ListRequestsResponse`) | Rename any response tag, e.g. `RequestDetails ... \`json:"requestDetails,omitempty"\`` → `json:"ZrequestDetails,omitempty"`, or `ApproverAction.Result \`json:"result"\`` → `json:"Zresult"`. `internal/workflows/models/wire_tags_test.go` pinned only the three request bodies while the SCA twin pinned both directions; every response test decoded a body marshaled from the same struct, so a rename round-tripped and `grant request get` / `list` would render blanks. Not claimed by the PR — recorded because it was found while closing WF-23 | CONFIRMED | test | `TestAccessRequest_DecodesPopulatedResponse` and `TestListRequestsResponse_DecodesPopulatedPage` — 18 tags verified killed (`requestId`, `targetCategory`, `requestState`, `requestResult`, `requestDetails`, `requestApprovers`, `requester`, `createdBy/At`, `updatedBy/At`, `entityId`, `entityName`, `approver`, `result`, `items`, `count`, `totalCount`) | PR8 | done | | WF-25 | internal/workflows models | `internal/workflows/models/form.go` (`FormQuestion`, `Validator`) | Rename any form-metadata tag, e.g. `Validator.Regex \`json:"regex,omitempty"\`` → `json:"Zregex,omitempty"`. Deliberately **not** closed in PR8: this is validation metadata for the interactive `grant request submit` form, one step removed from the user-visible output that WF-24 covers, and pinning it well needs a populated-form fixture rather than another tag list. Recorded so the gap is stated rather than silent | CONFIRMED | test | Follow-up: a populated `RequestFormResponse` decode fixture pinning `requestForms`, `requestForm`, `questions`, `key`, `required`, `valueType`, `valueChoices`, `validators` and the `Validator` fields | — | 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-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` | superseded by the index-based selector fix: `TestResolveSelectionItem` + `TestUIUnifiedSelector_PTY_DuplicateGroupDisplay` | done | +| 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 | done | +| ELV-03 | cmd/root (unified elevate builder) | `cmd/root.go:786-788` | In `elevateCloud`, blank both `CSP:` and `OrganizationID:` | CONFIRMED | test | `TestElevateCloud_RequestPayload` | PR4 | done | +| 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 | done | +| 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 | done | +| 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 | done | +| 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 | done | +| ELV-08 | cmd/env direct path | `cmd/root.go:456-458` | Delete the paired `--target`/`--role` validation in `resolveAndElevate` | CONFIRMED | test | `TestEnv_RequiresBothTargetAndRole` | PR4 | done | +| 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 | done | +| 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 | done | +| 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 | done | +| 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 | `TestElevate_EmptyResultsGuards/env_path` | PR4 | done | +| ELV-13 | cmd/root cloud elevate | `cmd/root.go:802-805` | Delete the identical empty-`Results` guard in `elevateCloud` | CONFIRMED | test | `TestElevate_EmptyResultsGuards/elevateCloud` | PR4 | done | +| ELV-14 | cmd/root group elevate | `cmd/root.go:832-835` | Delete the identical empty-`Results` guard in `elevateGroup` | CONFIRMED | test | `TestElevate_EmptyResultsGuards/elevateGroup` | PR4 | done | | 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 | +| 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 | done | +| 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 | done | +| ELV-21 | cmd/env auth | `cmd/root.go:419` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in `resolveAndElevate` (env path) | CONFIRMED | test | `TestAuthCacheFlag/env_path` | PR4 | done | +| ELV-22 | cmd/root auth | `cmd/root.go:620` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in the root elevate path | CONFIRMED | test | `TestAuthCacheFlag/root_path` | PR4 | done | +| 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 | done | +| 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` over the extracted `shouldShowVerboseHint` (predicate) **plus** `TestIntegration_VerboseHint` (call site) | PR4 | done | +| 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 | done | +| 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 | Deleted the duplicated `multi-CSP concurrent fetch - parallel execution` case | PR4 | done | +| 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 | Widened the bound to 500ms in `TestFetchEligibility_ConcurrentExecution` | PR4 | done | | 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 | done | | 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 | done | | SFU-03 | internal/selfupdate | `internal/selfupdate/selfupdate.go:349` | `case hasDriveLetter(normalized):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/drive_absolute` | PR2 | done | @@ -274,6 +274,56 @@ premise does not hold). | 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 | +| COV-01 | cmd/root Execute | `cmd/root.go` (the `if shouldShowVerboseHint(...) { Fprintln(...) }` block in `Execute()`) | Delete the whole block. **Killed only on the integration leg.** `go test -count=1 ./cmd/` stays green — the package-level `Execute()` calls `os.Exit`, so only the compiled binary exercises the wiring. It fails under `-tags=integration` via `TestIntegration_ElevateWithoutLogin` and `TestIntegration_VerboseHint`. CI runs that leg unguarded on both OSes, so the call site *is* covered; the default local `make test` loop is not. Do not "simplify" CI by guarding the integration step | CONFIRMED | test | `TestIntegration_VerboseHint` + `TestIntegration_ElevateWithoutLogin` (`-tags=integration` only) | PR4 | done | +| COV-02 | cmd/login | `cmd/login.go` auto-configure branch (`if profile == nil`) | No mutation — a **coverage caveat** on REQ-20. `TestRunLogin_AutoConfiguresMissingProfile` (`cmd/login_args_test.go`) calls `t.Skip` when `os.Stdin` is a terminal, so under a PTY the auto-configure branch has **zero** coverage and REQ-20 is unpinned. Accepted, not fixed: `runConfigure` prompts through `survey`, which reads `os.Stdin` directly with no injectable seam, so on a real terminal the test would block on input rather than fail. `go test`, CI and every non-interactive run have a non-TTY stdin and do exercise it. Removing the skip requires giving `runConfigure` a seam first | OVERSTATED | wont-fix | none — the skip comment states the caveat at the call site | PR4 | done | + +--- + +## Notes from closing PR4 + +Mutations applied verbatim except where the literal form does not compile or +where the production site moved. The forms actually used: + +- **REQ-13/14/15** — deletion of `earlyNonInteractiveCheck` orphans `requestID` + (`declared and not used`). Applied as `_ = requestID` + `if false { return nil }` + at each of the three call sites (`request_get.go:21`, `request_finalize.go:20` + and `:54`), each verified separately, plus once at the shared guard + (`request_picker.go:15`). +- **ELV-24** — the condition now lives in `shouldShowVerboseHint` (extracted so a + test does not have to restate it); the equivalent mutation is inverting + `!verboseOn` to `verboseOn` inside that helper. `executeWithHint` in the test + helpers now calls the same predicate instead of duplicating it. + + **Residual surviving mutant, since closed — read this row as two mutants, not + one.** Extracting the predicate pinned the *condition* but unpinned the *call + site*: because `executeWithHint` calls `shouldShowVerboseHint` rather than + restating it, the helper became a reimplementation of `Execute()`'s error path + that can never disagree with it. Deleting the entire + `if shouldShowVerboseHint(...) { Fprintln(...) }` block from `Execute()` left + `go test ./cmd/ -count=1` **green**. The unit suite cannot kill this class of + mutant at all — the package-level `Execute()` calls `os.Exit`, so only the + compiled binary exercises the wiring. Closed by `TestIntegration_VerboseHint` + (`cmd/integration_test.go`, `-tags=integration`), which asserts the hint IS + present on a runtime error and is NOT present for an unknown subcommand, an + unknown flag, or an already-`--verbose` run. Both mutants — deleting the block, + and forcing the condition to `true` — were reverified against it: each leaves + `go test ./cmd/` green and fails `go test -tags=integration ./cmd`. The + consequence for CI is that **the ELV-24 call site is only covered on the + integration leg**; dropping `-tags=integration` silently unpins it again. +- **ELV-20** — used the semantic form the ledger prescribes (delete the + `elevCtx`/`elevCancel` pair and pass `ctx`), since a token substitution does + not compile. +- **ELV-02/03/04** — applied as three separate mutations (WorkspaceID⇄RoleID swap + in each builder, and blanking `CSP`/`OrganizationID` in `elevateCloud`). +- **ELV-26/27** — no production site: the duplicated interactive case was + deleted and the concurrency bound widened to 500ms (three CSPs × 200ms: + sequential ≈600ms, concurrent ≈200ms, so the bound still separates the two + regimes). No flake was observed at the old bound; none is claimed. + +**Follow-up filed, deliberately not done here:** the elevate-request builders at +`cmd/root.go:497` and `:786` are byte-identical. Both are now pinned by tests +(`TestResolveAndElevate_RequestPayload`, `TestElevateCloud_RequestPayload`); +extracting a shared helper is a refactor and belongs in its own change. ---