diff --git a/CLAUDE.md b/CLAUDE.md index a0da585..4e1b95f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,6 +85,12 @@ 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` +- **Output contracts**: each machine-facing document has exactly ONE whole-object test comparing the emitted JSON against an inline literal with `assertJSONEqual` (`cmd/test_helpers_test.go`) — nine of them: cloud elevation, group elevation, `env` credentials, `list`, `status`, `revoke`, `favorites list`, the access-request object (shared by `request get`/`submit`/`cancel`/`approve`/`reject`) and the `request list` envelope. Optional fields additionally get a whole-object case exercising the ABSENT state, so dropping an `omitempty` fails. It is deliberately brittle against added fields — these documents are a compatibility surface, and a new field should force a conscious review rather than pass silently. Inline literals, not golden files: the repo has no `testdata` machinery and the objects are small. Keep focused tests for conditional and optional fields instead of converting every behavioral test into a whole-object one +- **Fixture values must be distinct and self-describing** (`ws-name`, `ws-id`, `role-name`, `role-id`, `grp-id`, `dir-id`, `AKIA-fixture`, …). A swap mutation — target with role, secretAccessKey with sessionToken, groupId with directoryId — is undetectable when both sides hold `"test"`. Same reason two same-named groups in different directories are the fixture for the favorites `DirectoryID` tests: a unique name makes the directory ID non-load-bearing +- 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 @@ -339,7 +345,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..240b8a8 --- /dev/null +++ b/cmd/elevate_args_test.go @@ -0,0 +1,722 @@ +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"}} +} + +// TestFindItemByDisplay_ReturnsMatchingItem kills ELV-01: returning +// &items[0] instead of &items[i] at cmd/selection.go:78. The existing test only +// asserts the result is non-nil, so it never notices that the wrong target +// would be elevated under a display line naming the right one. +func TestFindItemByDisplay_ReturnsMatchingItem(t *testing.T) { + first := &models.EligibleTarget{ + WorkspaceName: "Aardvark-Sub", + WorkspaceType: models.WorkspaceTypeSubscription, + RoleInfo: models.RoleInfo{ID: "role-first", Name: "Reader"}, + } + second := &models.EligibleTarget{ + WorkspaceName: "Zulu-Sub", + WorkspaceType: models.WorkspaceTypeSubscription, + RoleInfo: models.RoleInfo{ID: "role-second", Name: "Owner"}, + } + group := &models.GroupsEligibleTarget{DirectoryName: "Contoso", GroupName: "Engineering"} + + items := []selectionItem{ + {kind: selectionCloud, cloud: first}, + {kind: selectionCloud, cloud: second}, + {kind: selectionGroup, group: group}, + } + + got, err := findItemByDisplay(items, formatSelectionItem(items[1])) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got.cloud != second { + t.Errorf("got workspace %q (role %q), want Zulu-Sub / role-second", + got.cloud.WorkspaceName, got.cloud.RoleInfo.ID) + } + + gotGroup, err := findItemByDisplay(items, formatSelectionItem(items[2])) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gotGroup.kind != selectionGroup || gotGroup.group != group { + t.Errorf("got %+v, want the group item", gotGroup) + } +} + +// 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/favorites_persistence_test.go b/cmd/favorites_persistence_test.go new file mode 100644 index 0000000..7118123 --- /dev/null +++ b/cmd/favorites_persistence_test.go @@ -0,0 +1,284 @@ +package cmd + +// Favorites persistence and flag-validation coverage. +// +// Fixture values are deliberately distinct and self-describing; see the header +// of output_contract_test.go for why that is mandatory rather than cosmetic. + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/aaearon/grant-cli/internal/config" + "github.com/aaearon/grant-cli/internal/sca/models" +) + +// twoGroupsSameName is the fixture for the DirectoryID persistence tests: two +// groups with the SAME display name in DIFFERENT directories. The names must +// collide, otherwise findMatchingGroup resolves on the name alone and the +// directory ID is never load-bearing. +func twoGroupsSameName() []models.GroupsEligibleTarget { + return []models.GroupsEligibleTarget{ + {GroupName: "grp-name", GroupID: "grp-id-a", DirectoryID: "dir-id-a"}, + {GroupName: "grp-name", GroupID: "grp-id-b", DirectoryID: "dir-id-b"}, + } +} + +// assertFavoriteResolvesToGroup reloads the saved favorite and pushes it back +// through findMatchingGroup — the production lookup behind `grant --favorite`. +// The round trip is what makes dropping `fav.DirectoryID = ...` fail: without +// it the favorite still names the right group, but resolution silently picks +// the first same-named group, in the wrong directory. +func assertFavoriteResolvesToGroup(t *testing.T, configPath, favName string, groups []models.GroupsEligibleTarget, wantDirectoryID, wantGroupID string) { + t.Helper() + + reloaded, err := config.Load(configPath) + if err != nil { + t.Fatalf("reload config failed: %v", err) + } + fav, err := config.GetFavorite(reloaded, favName) + if err != nil { + t.Fatalf("favorite not found: %v", err) + } + if fav.DirectoryID != wantDirectoryID { + t.Errorf("persisted DirectoryID = %q, want %q", fav.DirectoryID, wantDirectoryID) + } + + match := findMatchingGroup(groups, fav.Group, fav.DirectoryID) + if match == nil { + t.Fatalf("findMatchingGroup(%q, %q) = nil", fav.Group, fav.DirectoryID) + } + if match.GroupID != wantGroupID { + t.Errorf("favorite resolved to group %q, want %q", match.GroupID, wantGroupID) + } +} + +// TestFavoritesAddInteractive_PersistsDirectoryID covers the unified-selector +// path (selectFavoriteInteractive). +func TestFavoritesAddInteractive_PersistsDirectoryID(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + if err := config.Save(config.DefaultConfig(), configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + groups := twoGroupsSameName() + eligLister := &mockEligibilityLister{response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{{ + WorkspaceID: "ws-id", WorkspaceName: "ws-name", + WorkspaceType: models.WorkspaceTypeSubscription, + RoleInfo: models.RoleInfo{ID: "role-id", Name: "role-name"}, + }}, + Total: 1, + }} + groupsElig := &mockGroupsEligibilityLister{response: &models.GroupsEligibilityResponse{ + Response: groups, Total: len(groups), + }} + // Select the SECOND group, so a dropped DirectoryID resolves to the first. + sel := &mockUnifiedSelector{item: &selectionItem{kind: selectionGroup, group: &groups[1]}} + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommandWithAllDeps(eligLister, sel, &mockNamePrompter{}, groupsElig)) + + if out, err := executeCommand(rootCmd, "favorites", "add", "fav-group"); err != nil { + t.Fatalf("add favorite failed: %v\noutput: %s", err, out) + } + + assertFavoriteResolvesToGroup(t, configPath, "fav-group", twoGroupsSameName(), "dir-id-b", "grp-id-b") +} + +// TestAddGroupFavorite_PersistsDirectoryID covers the `--type groups` selector +// path (addGroupFavorite), which copies the directory ID independently. +func TestAddGroupFavorite_PersistsDirectoryID(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + if err := config.Save(config.DefaultConfig(), configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + groups := twoGroupsSameName() + groupsElig := &mockGroupsEligibilityLister{response: &models.GroupsEligibilityResponse{ + Response: groups, Total: len(groups), + }} + sel := &mockUnifiedSelector{item: &selectionItem{kind: selectionGroup, group: &groups[1]}} + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommandWithAllDeps(nil, sel, &mockNamePrompter{}, groupsElig)) + + if out, err := executeCommand(rootCmd, "favorites", "add", "fav-group", "--type", "groups"); err != nil { + t.Fatalf("add group favorite failed: %v\noutput: %s", err, out) + } + + assertFavoriteResolvesToGroup(t, configPath, "fav-group", twoGroupsSameName(), "dir-id-b", "grp-id-b") +} + +// TestFavoritesAdd_HonorsNonDefaultProvider pins that a providerless +// `favorites add --target/--role` takes its provider from +// config.default_provider. Every other test uses DefaultConfig(), whose default +// already IS "azure", so a hardcoded "azure" was indistinguishable from +// reading the field. +func TestFavoritesAdd_HonorsNonDefaultProvider(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + + cfg := config.DefaultConfig() + cfg.DefaultProvider = "aws" + if err := config.Save(cfg, configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommand()) + + out, err := executeCommand(rootCmd, "favorites", "add", "fav-cloud", "--target", "ws-name", "--role", "role-name") + if err != nil { + t.Fatalf("add favorite failed: %v\noutput: %s", err, out) + } + + reloaded, err := config.Load(configPath) + if err != nil { + t.Fatalf("reload config failed: %v", err) + } + fav, err := config.GetFavorite(reloaded, "fav-cloud") + if err != nil { + t.Fatalf("favorite not found: %v", err) + } + if fav.Provider != "aws" { + t.Errorf("Provider = %q, want aws (from config.default_provider)", fav.Provider) + } +} + +// TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP pins the precedence +// in selectFavoriteInteractive: an explicit --provider is stored verbatim +// rather than derived from the selected target's CSP. The fixture is +// deliberately impossible: --provider azure with an AWS target is a +// combination production filtering would never produce, and it exists only so +// the two branches yield different values. Do not read it as a realistic +// scenario, and do not "fix" it to azure — with both azure the branches are +// indistinguishable and the mutation survives. +func TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + if err := config.Save(config.DefaultConfig(), configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + selected := models.EligibleTarget{ + CSP: models.CSPAWS, + WorkspaceID: "ws-id", WorkspaceName: "ws-name", + WorkspaceType: models.WorkspaceTypeAccount, + RoleInfo: models.RoleInfo{ID: "role-id", Name: "role-name"}, + } + eligLister := &mockEligibilityLister{response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{selected}, Total: 1, + }} + sel := &mockUnifiedSelector{item: &selectionItem{kind: selectionCloud, cloud: &selected}} + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommandWithAllDeps(eligLister, sel, &mockNamePrompter{}, nil)) + + out, err := executeCommand(rootCmd, "favorites", "add", "fav-cloud", "--provider", "azure") + if err != nil { + t.Fatalf("add favorite failed: %v\noutput: %s", err, out) + } + + reloaded, err := config.Load(configPath) + if err != nil { + t.Fatalf("reload config failed: %v", err) + } + fav, err := config.GetFavorite(reloaded, "fav-cloud") + if err != nil { + t.Fatalf("favorite not found: %v", err) + } + if fav.Provider != "azure" { + t.Errorf("Provider = %q, want azure (the --provider flag, not the target CSP)", fav.Provider) + } +} + +// TestParseFavoritesAddFlags_Validation exercises parseFavoritesAddFlags +// directly. runFavoritesAddProduction repeats two of these checks, so the +// command-level tests keep passing when this copy is deleted — every DI caller +// of runFavoritesAddWithDeps would lose the validation silently. +func TestParseFavoritesAddFlags_Validation(t *testing.T) { + tests := []struct { + name string + args []string + // wantErrContains empty means the parse must succeed. + wantErrContains string + }{ + {name: "cloud target and role", args: []string{"--target", "ws-name", "--role", "role-name"}}, + {name: "groups with group", args: []string{"--type", "groups", "--group", "grp-name"}}, + {name: "groups with target", args: []string{"--type", "groups", "--target", "ws-name"}, wantErrContains: "--target and --role cannot be used with --type groups"}, + {name: "groups with role", args: []string{"--type", "groups", "--role", "role-name"}, wantErrContains: "--target and --role cannot be used with --type groups"}, + {name: "group without type groups", args: []string{"--group", "grp-name"}, wantErrContains: "--group requires --type groups"}, + {name: "target without role", args: []string{"--target", "ws-name"}, wantErrContains: "both --target and --role must be provided"}, + {name: "role without target", args: []string{"--role", "role-name"}, wantErrContains: "both --target and --role must be provided"}, + {name: "invalid type", args: []string{"--type", "bogus"}, wantErrContains: `invalid --type "bogus"`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := newFavoritesAddCommandWithRunner(nil) + if err := cmd.ParseFlags(tt.args); err != nil { + t.Fatalf("ParseFlags() error = %v", err) + } + + f, err := parseFavoritesAddFlags(cmd) + if tt.wantErrContains == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if f == nil { + t.Fatal("expected parsed flags, got nil") + } + return + } + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.wantErrContains) + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %v, want it to contain %q", err, tt.wantErrContains) + } + }) + } +} + +// TestFavoritesRemove_RejectsExtraArgs pins the arity check: without it, +// `grant favorites remove first second` silently removes only "first". +func TestFavoritesRemove_RejectsExtraArgs(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + + cfg := config.DefaultConfig() + if err := config.AddFavorite(cfg, "fav-first", config.Favorite{Provider: "azure", Target: "ws-name", Role: "role-name"}); err != nil { + t.Fatalf("AddFavorite() error = %v", err) + } + if err := config.AddFavorite(cfg, "fav-second", config.Favorite{Provider: "aws", Target: "ws-name-2", Role: "role-name-2"}); err != nil { + t.Fatalf("AddFavorite() error = %v", err) + } + if err := config.Save(cfg, configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommand()) + + _, err := executeCommand(rootCmd, "favorites", "remove", "fav-first", "fav-second") + if err == nil { + t.Fatal("expected an error for two favorite names") + } + if !strings.Contains(err.Error(), "expected 1 favorite name, got 2") { + t.Errorf("error = %v, want the arity message", err) + } + + reloaded, err := config.Load(configPath) + if err != nil { + t.Fatalf("reload config failed: %v", err) + } + for _, name := range []string{"fav-first", "fav-second"} { + if _, err := config.GetFavorite(reloaded, name); err != nil { + t.Errorf("favorite %q was removed despite the rejected command", name) + } + } +} 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/list_test.go b/cmd/list_test.go index 29e99d3..1316e36 100644 --- a/cmd/list_test.go +++ b/cmd/list_test.go @@ -268,6 +268,96 @@ func TestListCommand_MutualExclusivity(t *testing.T) { if err == nil { t.Fatal("expected error for --groups + --provider") } + // Assert Cobra's own mutual-exclusion text. Merely requiring "an error" + // passed even with MarkFlagsMutuallyExclusive deleted, because the command + // then ran and failed with "no eligible targets or groups found" instead. + if !strings.Contains(err.Error(), "[groups provider] were all set") { + t.Errorf("expected Cobra's mutual-exclusion error, got: %v", err) + } +} + +// TestListCommand_ProviderSuppressesGroups pins that --provider restricts the +// output to cloud targets: groups are Azure-only and a provider filter has no +// meaning for them, so they are not fetched at all. +func TestListCommand_ProviderSuppressesGroups(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + eligLister := &mockEligibilityLister{response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{{ + WorkspaceID: "ws-id", WorkspaceName: "ws-name", + WorkspaceType: models.WorkspaceTypeSubscription, + RoleInfo: models.RoleInfo{ID: "role-id", Name: "role-name"}, + }}, + Total: 1, + }} + groupsCalled := false + groupsElig := &mockGroupsEligibilityLister{ + listFunc: func(_ context.Context, _ models.CSP) (*models.GroupsEligibilityResponse, error) { + groupsCalled = true + return &models.GroupsEligibilityResponse{ + Response: []models.GroupsEligibleTarget{{GroupID: "grp-id", GroupName: "grp-name", DirectoryID: "dir-id"}}, + Total: 1, + }, nil + }, + } + + cmd := NewListCommandWithDeps(auth, eligLister, groupsElig) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "list", "--provider", "azure", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + var parsed listOutput + if err := json.Unmarshal([]byte(stdout), &parsed); err != nil { + t.Fatalf("invalid JSON: %v\n%s", err, stdout) + } + if len(parsed.Groups) != 0 { + t.Errorf("expected no groups with --provider, got %d", len(parsed.Groups)) + } + if groupsCalled { + t.Error("groups eligibility must not be fetched when --provider is set") + } +} + +// TestListCommand_RefreshFlagRegistered pins the --refresh flag on `grant list`. +// The flag is not a no-op: NewListCommand reads it and passes it into +// buildCachedLister. That wiring runs behind bootstrapSCAService, which unit +// tests cannot reach, so this covers registration and parsing only. +func TestListCommand_RefreshFlagRegistered(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + eligLister := &mockEligibilityLister{response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{{ + WorkspaceID: "ws-id", WorkspaceName: "ws-name", + WorkspaceType: models.WorkspaceTypeSubscription, + RoleInfo: models.RoleInfo{ID: "role-id", Name: "role-name"}, + }}, + Total: 1, + }} + groupsElig := &mockGroupsEligibilityLister{listErr: errors.New("skip groups")} + + cmd := NewListCommandWithDeps(auth, eligLister, groupsElig) + root := newTestRootCommand() + root.AddCommand(cmd) + + if _, err := executeCommand(root, "list", "--refresh"); err != nil { + t.Fatalf("grant list --refresh must be accepted: %v", err) + } + + refresh, err := cmd.Flags().GetBool("refresh") + if err != nil { + t.Fatalf("--refresh is not registered: %v", err) + } + if !refresh { + t.Error("--refresh parsed as false") + } + + // And it defaults to off on a fresh command. + fresh := NewListCommandWithDeps(auth, eligLister, groupsElig) + if def, err := fresh.Flags().GetBool("refresh"); err != nil || def { + t.Errorf("--refresh default = %v (err %v), want false", def, err) + } } // TestListCommand_JSONOutputSingleProvider guards the regression where 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/output_contract_test.go b/cmd/output_contract_test.go new file mode 100644 index 0000000..a9fcd15 --- /dev/null +++ b/cmd/output_contract_test.go @@ -0,0 +1,838 @@ +package cmd + +// Output contract tests. +// +// Each machine-facing document (status, list, elevation, env credentials, +// favorites list, access requests) gets ONE whole-object test that compares the +// emitted JSON against an inline literal via assertJSONEqual. Everything else +// about those outputs stays covered by focused tests. +// +// FIXTURE VALUES ARE DELIBERATELY ALL DIFFERENT AND SELF-DESCRIBING +// ("ws-name", "ws-id", "role-name", "role-id", "grp-id", "dir-id", +// "AKIA-fixture", "secret-fixture", "token-fixture"). A field swap — target +// with role, secret key with session token, groupId with directoryId — is +// invisible when both sides hold "test" or "", so these tests only detect one +// if every value is unique. Do not "tidy" them into shared constants or +// realistic-looking duplicates. + +import ( + "context" + "encoding/json" + "path/filepath" + "testing" + "time" + + "github.com/aaearon/grant-cli/internal/cache" + "github.com/aaearon/grant-cli/internal/config" + scamodels "github.com/aaearon/grant-cli/internal/sca/models" + wfmodels "github.com/aaearon/grant-cli/internal/workflows/models" + authmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/auth" +) + +// --- status ----------------------------------------------------------------- + +// pinnedRemainingSeconds is the value substituted for the wall-clock-derived +// remainingSeconds field before a whole-object comparison. +const pinnedRemainingSeconds = 2700 + +// pinRemainingSeconds range-checks every sessions[].remainingSeconds and +// rewrites it to pinnedRemainingSeconds. The field is computed from time.Now() +// and therefore cannot appear verbatim in a literal; pinning it keeps the rest +// of the document — including whether the field is present at all — under the +// whole-object comparison. +func pinRemainingSeconds(t *testing.T, raw []byte, minSecs, maxSecs int) []byte { + t.Helper() + + var doc map[string]interface{} + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("status output is not valid JSON: %v\nraw:\n%s", err, raw) + } + sessions, ok := doc["sessions"].([]interface{}) + if !ok { + t.Fatalf("status output has no sessions array:\n%s", raw) + } + for _, s := range sessions { + session, ok := s.(map[string]interface{}) + if !ok { + t.Fatalf("session entry is not an object:\n%s", raw) + } + v, ok := session["remainingSeconds"] + if !ok { + continue + } + secs, ok := v.(float64) + if !ok { + t.Fatalf("remainingSeconds is not a number: %#v", v) + } + if int(secs) < minSecs || int(secs) > maxSecs { + t.Errorf("remainingSeconds = %d, want between %d and %d", int(secs), minSecs, maxSecs) + } + session["remainingSeconds"] = pinnedRemainingSeconds + } + + pinned, err := json.Marshal(doc) + if err != nil { + t.Fatalf("re-marshal failed: %v", err) + } + return pinned +} + +// TestStatusJSON_Contract pins the whole `grant status --output json` document +// for a cloud session and a group session at once. Six independent mutations in +// writeStatusJSON survived before it existed: provider case, workspaceId, +// duration, roleId, the workspace-name lookup and the group/cloud type tag. +func TestStatusJSON_Contract(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + + sessions := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{ + { + // CSP is mixed case on purpose: strings.ToLower, strings.ToUpper + // and the raw value are three different strings only if it is. + SessionID: "sess-cloud", CSP: "Azure", + WorkspaceID: "ws-id", RoleID: "role-id", SessionDuration: 3600, + }, + { + SessionID: "sess-group", CSP: "Azure", + WorkspaceID: "dir-ws-id", SessionDuration: 1800, + Target: &scamodels.SessionTarget{ID: "grp-id", Type: scamodels.TargetTypeGroups}, + }, + }, + Total: 2, + }} + + // Names differ from the IDs that key them, so dropping the lookup is visible. + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{ + {WorkspaceID: "ws-id", WorkspaceName: "ws-name"}, + {WorkspaceID: "dir-ws-id", WorkspaceName: "dir-ws-name"}, + }, + Total: 2, + }} + groupsElig := &mockGroupsEligibilityLister{response: &scamodels.GroupsEligibilityResponse{ + Response: []scamodels.GroupsEligibleTarget{ + {GroupID: "grp-id", GroupName: "grp-name", DirectoryID: "dir-id"}, + }, + Total: 1, + }} + + tracker := cache.NewStore(t.TempDir(), 25*time.Hour) + if err := cache.RecordSession(tracker, "sess-cloud", time.Now().Add(-15*time.Minute)); err != nil { + t.Fatalf("RecordSession() error = %v", err) + } + + cmd := NewStatusCommandWithDeps(auth, sessions, elig, groupsElig, tracker) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "status", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + // The fixture makes this deterministically 2699; the window is narrow on + // purpose so a whole-minute arithmetic error dies at the pin itself rather + // than relying on the sibling text assertion. + got := pinRemainingSeconds(t, []byte(stdout), 2695, pinnedRemainingSeconds) + + assertJSONEqual(t, got, `{ + "authenticated": true, + "username": "user-fixture@example.test", + "sessions": [ + { + "sessionId": "sess-cloud", + "provider": "azure", + "workspaceId": "ws-id", + "workspaceName": "ws-name", + "roleId": "role-id", + "duration": 3600, + "remainingSeconds": 2700, + "type": "cloud" + }, + { + "sessionId": "sess-group", + "provider": "azure", + "workspaceId": "dir-ws-id", + "workspaceName": "dir-ws-name", + "duration": 1800, + "type": "group", + "groupId": "grp-id", + "groupName": "grp-name" + } + ] +}`) +} + +// TestStatus_DirectoryNameMergePrecedence pins the precedence rule in runStatus: +// a workspace name that came from eligibility wins over the directory-name +// fallback for the same key. Making the merge unconditional reverses it. +func TestStatus_DirectoryNameMergePrecedence(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + + sessions := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{ + {SessionID: "sess-collide", CSP: scamodels.CSPAzure, WorkspaceID: "ws-collide", RoleID: "role-id", SessionDuration: 3600}, + }, + Total: 1, + }} + + // The first entry teaches fetchStatusData ws-collide -> elig-name. + // The second makes buildDirectoryNameMap produce ws-collide -> dir-fallback-name + // for the very same key, via the organizationId fallback pass. + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{ + {WorkspaceID: "ws-collide", WorkspaceName: "elig-name"}, + {WorkspaceID: "ws-other", WorkspaceName: "dir-fallback-name", OrganizationID: "ws-collide"}, + }, + Total: 2, + }} + + cmd := NewStatusCommandWithDeps(auth, sessions, elig, nil, nil) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "status", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + var out statusOutput + if err := json.Unmarshal([]byte(stdout), &out); err != nil { + t.Fatalf("invalid JSON: %v\n%s", err, stdout) + } + if len(out.Sessions) != 1 { + t.Fatalf("expected 1 session, got %d", len(out.Sessions)) + } + if out.Sessions[0].WorkspaceName != "elig-name" { + t.Errorf("workspaceName = %q, want elig-name (eligibility must win over the directory fallback)", out.Sessions[0].WorkspaceName) + } +} + +// TestStatus_CleansUpStaleSessionTimestamps pins the lazy cleanup call in +// runStatus: timestamps for sessions the API no longer reports are dropped. +func TestStatus_CleansUpStaleSessionTimestamps(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + + sessions := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{ + {SessionID: "sess-active", CSP: scamodels.CSPAzure, WorkspaceID: "ws-id", RoleID: "role-id", SessionDuration: 3600}, + }, + Total: 1, + }} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{}} + + tracker := cache.NewStore(t.TempDir(), 25*time.Hour) + now := time.Now() + if err := cache.RecordSession(tracker, "sess-active", now.Add(-10*time.Minute)); err != nil { + t.Fatalf("RecordSession() error = %v", err) + } + if err := cache.RecordSession(tracker, "sess-gone", now.Add(-10*time.Minute)); err != nil { + t.Fatalf("RecordSession() error = %v", err) + } + + cmd := NewStatusCommandWithDeps(auth, sessions, elig, nil, tracker) + if _, err := executeCommand(cmd); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + remaining := cache.SessionTimestamps(tracker) + if _, ok := remaining["sess-gone"]; ok { + t.Error("timestamp for an inactive session survived; status must call cache.CleanupSessions") + } + if _, ok := remaining["sess-active"]; !ok { + t.Error("timestamp for the active session was removed") + } +} + +// --- list ------------------------------------------------------------------- + +// listContractFixture is the eligibility used by the list contract and +// round-trip tests. workspaceId, organizationId, name, role name and role id +// are five distinct strings so sourcing a field from the wrong one is visible. +func listContractFixture() []scamodels.EligibleTarget { + return []scamodels.EligibleTarget{{ + CSP: scamodels.CSPAzure, + OrganizationID: "org-id", + WorkspaceID: "ws-id", + WorkspaceName: "ws-name", + WorkspaceType: scamodels.WorkspaceTypeSubscription, + RoleInfo: scamodels.RoleInfo{ID: "role-id", Name: "role-name"}, + }} +} + +func listContractGroups() []scamodels.GroupsEligibleTarget { + return []scamodels.GroupsEligibleTarget{{ + GroupName: "grp-name", + GroupID: "grp-id", + DirectoryID: "dir-id", + DirectoryName: "dir-name", + }} +} + +// TestListJSON_Contract pins the whole `grant list --output json` document. +// Sourcing workspaceId from organizationId, or blanking workspaceType, roleId, +// groupId or directoryId, all survived before it existed. +func TestListJSON_Contract(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + targets := listContractFixture() + elig := &mockEligibilityLister{ + listFunc: func(_ context.Context, csp scamodels.CSP) (*scamodels.EligibilityResponse, error) { + if csp == scamodels.CSPAzure { + return &scamodels.EligibilityResponse{Response: targets, Total: len(targets)}, nil + } + return &scamodels.EligibilityResponse{}, nil + }, + } + groupsElig := &mockGroupsEligibilityLister{response: &scamodels.GroupsEligibilityResponse{ + Response: listContractGroups(), Total: 1, + }} + + cmd := NewListCommandWithDeps(auth, elig, groupsElig) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "list", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "cloud": [ + { + "provider": "azure", + "target": "ws-name", + "workspaceId": "ws-id", + "workspaceType": "subscription", + "role": "role-name", + "roleId": "role-id" + } + ], + "groups": [ + { + "groupName": "grp-name", + "groupId": "grp-id", + "directoryId": "dir-id", + "directory": "dir-name" + } + ] +}`) +} + +// TestListJSON_RoundTripsToRequestSubmit is the reusability guarantee an LLM or +// script depends on: values emitted by `grant list -o json` must feed straight +// back into the flags that consume them. +// +// Note which field goes where — a verifier corrected this. `--target` resolves +// on the emitted NAME (`target`), not on `workspaceId`; `roleId` is the value +// `--role-id` takes verbatim. +func TestListJSON_RoundTripsToRequestSubmit(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + targets := listContractFixture() + elig := &mockEligibilityLister{ + listFunc: func(_ context.Context, csp scamodels.CSP) (*scamodels.EligibilityResponse, error) { + if csp == scamodels.CSPAzure { + return &scamodels.EligibilityResponse{Response: targets, Total: len(targets)}, nil + } + return &scamodels.EligibilityResponse{}, nil + }, + } + + listCmd := NewListCommandWithDeps(auth, elig, &mockGroupsEligibilityLister{listErr: errNotAuthenticated}) + listRoot := newTestRootCommand() + listRoot.AddCommand(listCmd) + + stdout, stderr, err := executeCommandStreams(listRoot, "list", "--output", "json") + if err != nil { + t.Fatalf("list failed: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + var listed listOutput + if err := json.Unmarshal([]byte(stdout), &listed); err != nil { + t.Fatalf("invalid JSON: %v\n%s", err, stdout) + } + if len(listed.Cloud) != 1 { + t.Fatalf("expected 1 cloud target, got %d", len(listed.Cloud)) + } + emitted := listed.Cloud[0] + + // 1. The root command's --target/--role path, through the production matcher. + resolved := findMatchingTarget(targets, emitted.Target, emitted.Role) + if resolved == nil { + t.Fatalf("findMatchingTarget(%q, %q) = nil; the emitted target/role do not resolve back", emitted.Target, emitted.Role) + } + if resolved.WorkspaceID != emitted.WorkspaceID { + t.Errorf("resolved workspaceId = %q, emitted %q", resolved.WorkspaceID, emitted.WorkspaceID) + } + if resolved.RoleInfo.ID != emitted.RoleID { + t.Errorf("resolved roleId = %q, emitted %q", resolved.RoleInfo.ID, emitted.RoleID) + } + + // 2. `grant request submit --target --role-id `. + // Only the auth/eligibility fetch is stubbed out; the resolution itself + // runs the production deduplicateWorkspaces + matchWorkspaceByName pair + // that resolveSubmitTarget calls, so changing what --target matches on + // breaks this test. + origResolve := resolveSubmitTargetFn + t.Cleanup(func() { resolveSubmitTargetFn = origResolve }) + resolveSubmitTargetFn = func(_ context.Context, _, targetName string, _ bool) (*submitWorkspace, error) { + if ws := matchWorkspaceByName(deduplicateWorkspaces(targets), targetName); ws != nil { + return ws, nil + } + t.Errorf("emitted target %q matched no eligible workspace", targetName) + return nil, errNotAuthenticated + } + + svc := &mockAccessRequestService{submitResult: &wfmodels.AccessRequest{ + RequestID: "req-id", RequestState: wfmodels.RequestStatePending, + }} + submitRoot := newTestRootCommand() + submitRoot.AddCommand(NewRequestCommandWithDeps(svc)) + + out, err := executeCommand(submitRoot, "request", "submit", + "--target", emitted.Target, "--role-id", emitted.RoleID, "--role", emitted.Role, + "--reason", "reason-fixture", "--date", "2026-04-21", + "--timezone", "UTC", "--from", "09:00", "--to", "17:00", "--yes") + if err != nil { + t.Fatalf("submit failed: %v\noutput: %s", err, out) + } + + submitted := svc.lastSubmit() + if submitted == nil { + t.Fatal("SubmitRequest was never called") + } + for _, tc := range []struct{ key, want string }{ + {"workspaceId", emitted.WorkspaceID}, + {"workspaceName", emitted.Target}, + {"roleId", emitted.RoleID}, + {"roleName", emitted.Role}, + } { + if got, _ := submitted.RequestDetails[tc.key].(string); got != tc.want { + t.Errorf("submitted %s = %q, want %q", tc.key, got, tc.want) + } + } +} + +// --- elevation -------------------------------------------------------------- + +// awsCredsFixture is the accessCredentials payload used by the elevation and +// env contract tests. The three values are distinct so swapping the secret key +// with the session token is detectable. +const awsCredsFixture = `{"aws_access_key":"AKIA-fixture","aws_secret_access_key":"secret-fixture","aws_session_token":"token-fixture"}` + +func awsElevationTarget() *scamodels.EligibleTarget { + return &scamodels.EligibleTarget{ + CSP: scamodels.CSPAWS, + OrganizationID: "org-id", + WorkspaceID: "ws-id", + WorkspaceName: "ws-name", + WorkspaceType: scamodels.WorkspaceTypeAccount, + RoleInfo: scamodels.RoleInfo{ID: "role-id", Name: "role-name"}, + } +} + +// TestElevationJSON_Contract pins the whole cloud-elevation document, including +// the AWS credential block. Swapping target with role, dropping the provider +// lowercasing, and swapping secretAccessKey with sessionToken all survived. +func TestElevationJSON_Contract(t *testing.T) { + creds := awsCredsFixture + target := awsElevationTarget() + + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{*target}, Total: 1, + }} + elev := &mockElevateService{response: &scamodels.ElevateResponse{Response: scamodels.ElevateAccessResult{ + CSP: scamodels.CSPAWS, OrganizationID: "org-id", + Results: []scamodels.ElevateTargetResult{{ + WorkspaceID: "ws-id", RoleID: "role-id", SessionID: "sess-id", + AccessCredentials: &creds, + }}, + }}} + sel := &mockUnifiedSelector{item: &selectionItem{kind: selectionCloud, cloud: target}} + + cmd := NewRootCommandWithDeps(nil, auth, elig, elev, sel, + &mockGroupsEligibilityLister{listErr: errNotAuthenticated}, nil, config.DefaultConfig()) + + stdout, stderr, err := executeCommandStreams(cmd, "--output", "json", "--provider", "aws") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "type": "cloud", + "provider": "aws", + "sessionId": "sess-id", + "target": "ws-name", + "role": "role-name", + "credentials": { + "accessKeyId": "AKIA-fixture", + "secretAccessKey": "secret-fixture", + "sessionToken": "token-fixture" + } +}`) +} + +// TestGroupElevationJSON_Contract pins the whole group-elevation document. +// Swapping groupId with directoryId survived before it existed. +func TestGroupElevationJSON_Contract(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{}} + groupsElig := &mockGroupsEligibilityLister{response: &scamodels.GroupsEligibilityResponse{ + Response: []scamodels.GroupsEligibleTarget{{ + GroupName: "grp-name", GroupID: "grp-id", DirectoryID: "dir-id", DirectoryName: "dir-name", + }}, + Total: 1, + }} + groupsElev := &mockGroupsElevator{response: &scamodels.GroupsElevateResponse{ + DirectoryID: "dir-id", CSP: scamodels.CSPAzure, + Results: []scamodels.GroupsElevateTargetResult{{GroupID: "grp-id", SessionID: "sess-id"}}, + }} + + cmd := NewRootCommandWithDeps(nil, auth, elig, nil, nil, groupsElig, groupsElev, config.DefaultConfig()) + + stdout, stderr, err := executeCommandStreams(cmd, "--output", "json", "--group", "grp-name") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "type": "group", + "sessionId": "sess-id", + "groupName": "grp-name", + "groupId": "grp-id", + "directoryId": "dir-id", + "directory": "dir-name" +}`) +} + +// TestEnvJSON_Contract pins `grant env --output json`. The identical swap in the +// text export path is already caught by TestEnvCommand_AWSSuccess; the JSON +// document carrying the same three secrets was unpinned. +func TestEnvJSON_Contract(t *testing.T) { + creds := awsCredsFixture + target := awsElevationTarget() + + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{*target}, Total: 1, + }} + elev := &mockElevateService{response: &scamodels.ElevateResponse{Response: scamodels.ElevateAccessResult{ + CSP: scamodels.CSPAWS, OrganizationID: "org-id", + Results: []scamodels.ElevateTargetResult{{ + WorkspaceID: "ws-id", RoleID: "role-id", SessionID: "sess-id", + AccessCredentials: &creds, + }}, + }}} + sel := &mockTargetSelector{target: target} + + cmd := NewEnvCommandWithDeps(nil, auth, elig, elev, sel, config.DefaultConfig()) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "env", + "--provider", "aws", "--target", "ws-name", "--role", "role-name", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "accessKeyId": "AKIA-fixture", + "secretAccessKey": "secret-fixture", + "sessionToken": "token-fixture" +}`) +} + +// --- favorites -------------------------------------------------------------- + +// TestFavoritesListJSON_Contract pins the whole `grant favorites list -o json` +// array. Blanking provider, role or directoryId all survived: the previous test +// switched on Name and asserted only type, target and group. +func TestFavoritesListJSON_Contract(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + + cfg := config.DefaultConfig() + if err := config.AddFavorite(cfg, "fav-cloud", config.Favorite{ + Provider: "aws", Target: "ws-name", Role: "role-name", + }); err != nil { + t.Fatalf("AddFavorite() error = %v", err) + } + if err := config.AddFavorite(cfg, "fav-group", config.Favorite{ + Type: config.FavoriteTypeGroups, Provider: "azure", Group: "grp-name", DirectoryID: "dir-id", + }); err != nil { + t.Fatalf("AddFavorite() error = %v", err) + } + if err := config.Save(cfg, configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + root := newTestRootCommand() + root.AddCommand(NewFavoritesCommand()) + + stdout, stderr, err := executeCommandStreams(root, "favorites", "list", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + // ListFavorites sorts by name, so the order is deterministic. + assertJSONEqual(t, []byte(stdout), `[ + { + "name": "fav-cloud", + "type": "cloud", + "provider": "aws", + "target": "ws-name", + "role": "role-name" + }, + { + "name": "fav-group", + "type": "groups", + "provider": "azure", + "group": "grp-name", + "directoryId": "dir-id" + } +]`) +} + +// --- access request list ------------------------------------------------------ + +// TestRequestListJSON_Contract pins the whole `grant request list -o json` +// document. The envelope was entirely unpinned: renaming the `requests` key, or +// adding a field to accessRequestListOutput, survived every existing test — +// they all unmarshalled into the very struct under test, so the tags could +// drift freely. totalCount deliberately differs from len(requests): it comes +// from the service's pagination total, not from the returned page. +func TestRequestListJSON_Contract(t *testing.T) { + svc := &mockAccessRequestService{ + listItems: []wfmodels.AccessRequest{*requestFixture()}, + listTotalCount: 7, + } + + root := newTestRootCommand() + root.AddCommand(NewRequestCommandWithDeps(svc)) + + stdout, stderr, err := executeCommandStreams(root, "request", "list", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "requests": [ + { + "requestId": "req-id", + "targetCategory": "CLOUD_CONSOLE", + "state": "PENDING", + "result": "UNKNOWN", + "priority": "priority-fixture", + "reason": "reason-fixture", + "provider": "provider-fixture", + "target": "ws-name", + "role": "role-name", + "requestDate": "2026-04-21", + "timezone": "tz-fixture", + "timeFrom": "01:11", + "timeTo": "22:22", + "finalizationReason": "finalization-fixture", + "requestLink": "https://example.test/req-id", + "createdBy": "creator-fixture", + "createdAt": "2026-04-20T10:00:00Z", + "updatedBy": "updater-fixture", + "updatedAt": "2026-04-21T11:00:00Z" + } + ], + "totalCount": 7 +}`) +} + +// --- revoke ------------------------------------------------------------------- + +// TestRevokeJSON_Contract pins the whole `grant revoke --output json` array. +// Every existing revoke test unmarshalled into []revocationOutput, so renaming +// sessionId or outcome survived — and outcome is the single classification +// field callers switch on, which makes it the weakest point in the surface. +// +// The fixture carries four requested sessions with four DIFFERENT outcomes plus +// an unattributed row, so the per-row mapping is pinned, not just the envelope: +// a row-to-outcome shuffle changes the document. +func TestRevokeJSON_Contract(t *testing.T) { + revoker := &mockSessionRevoker{response: revokeResponse( + "sess-revoked", scamodels.RevocationSuccessful, + "sess-inprogress", scamodels.RevocationInProgress, + "sess-notapplicable", scamodels.RevocationNotApplicable, + "sess-unrequested", scamodels.RevocationSuccessful, + )} + + cmd := NewRevokeCommandWithDeps(testAuthLoader(), &mockSessionLister{}, &mockEligibilityLister{}, + revoker, &mockSessionSelector{}, &mockConfirmPrompter{}) + root := newTestRootCommand() + root.AddCommand(cmd) + + // sess-missing is requested but never answered, so it must still appear. + stdout, _, err := executeCommandStreams(root, "revoke", + "sess-revoked", "sess-inprogress", "sess-notapplicable", "sess-missing", + "--yes", "--output", "json") + if err == nil { + t.Fatal("expected a non-zero exit: not_applicable and a missing row are both failures") + } + + // A confirmed revocation carries no reason, so `reason` must be ABSENT from + // the first entry — omitempty on that field is part of the contract. + assertJSONEqual(t, []byte(stdout), `[ + { + "sessionId": "sess-revoked", + "status": "SUCCESSFULLY_REVOKED", + "outcome": "revoked" + }, + { + "sessionId": "sess-inprogress", + "status": "REVOCATION_IN_PROGRESS", + "outcome": "in_progress", + "reason": "accepted by the service, not yet confirmed complete" + }, + { + "sessionId": "sess-notapplicable", + "status": "REVOCATION_NOT_APPLICABLE", + "outcome": "not_applicable", + "reason": "the service reported revocation is not applicable to this session" + }, + { + "sessionId": "sess-missing", + "status": "", + "outcome": "unknown", + "reason": "no result returned by the service for this session" + }, + { + "sessionId": "sess-unrequested", + "status": "SUCCESSFULLY_REVOKED", + "outcome": "unknown", + "reason": "result was not requested and satisfies no requested session", + "unexpected": true + } +]`) +} + +// --- absent optional fields ---------------------------------------------------- +// +// The tests above populate every optional field, which pins the PRESENT state. +// Dropping `omitempty` from a field is invisible to them: the key is emitted +// either way. These three pin the ABSENT state — the key must not appear at +// all, rather than appearing as null or "". A consumer doing a key-presence +// check sees a contract change where a struct-unmarshalling test sees none. + +// TestElevationJSON_AzureOmitsCredentials pins that a non-AWS elevation emits +// NO `credentials` key. Dropping omitempty turns it into "credentials": null. +func TestElevationJSON_AzureOmitsCredentials(t *testing.T) { + target := &scamodels.EligibleTarget{ + CSP: scamodels.CSPAzure, + OrganizationID: "org-id", + WorkspaceID: "ws-id", + WorkspaceName: "ws-name", + WorkspaceType: scamodels.WorkspaceTypeSubscription, + RoleInfo: scamodels.RoleInfo{ID: "role-id", Name: "role-name"}, + } + + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{*target}, Total: 1, + }} + // No AccessCredentials: Azure elevations never carry them. + elev := &mockElevateService{response: &scamodels.ElevateResponse{Response: scamodels.ElevateAccessResult{ + CSP: scamodels.CSPAzure, OrganizationID: "org-id", + Results: []scamodels.ElevateTargetResult{{ + WorkspaceID: "ws-id", RoleID: "role-id", SessionID: "sess-id", + }}, + }}} + sel := &mockUnifiedSelector{item: &selectionItem{kind: selectionCloud, cloud: target}} + + cmd := NewRootCommandWithDeps(nil, auth, elig, elev, sel, + &mockGroupsEligibilityLister{listErr: errNotAuthenticated}, nil, config.DefaultConfig()) + + stdout, stderr, err := executeCommandStreams(cmd, "--output", "json", "--provider", "azure") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "type": "cloud", + "provider": "azure", + "sessionId": "sess-id", + "target": "ws-name", + "role": "role-name" +}`) +} + +// TestStatusJSON_OmitsAbsentOptionalFields pins the absent state of the status +// document: an unnamed token emits NO `username`, and a session whose workspace +// ID is missing from the name map emits NO `workspaceName`. Dropping omitempty +// from either turns it into an empty string. +func TestStatusJSON_OmitsAbsentOptionalFields(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + + sessions := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{{ + SessionID: "sess-unnamed", CSP: scamodels.CSPAzure, + WorkspaceID: "ws-id-unknown", RoleID: "role-id", SessionDuration: 3600, + }}, + Total: 1, + }} + // Eligibility knows a different workspace, so the lookup misses. + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{{WorkspaceID: "ws-id-other", WorkspaceName: "ws-name"}}, + Total: 1, + }} + + cmd := NewStatusCommandWithDeps(auth, sessions, elig, nil, nil) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "status", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "authenticated": true, + "sessions": [ + { + "sessionId": "sess-unnamed", + "provider": "azure", + "workspaceId": "ws-id-unknown", + "roleId": "role-id", + "duration": 3600, + "type": "cloud" + } + ] +}`) +} + +// TestListJSON_OmitsAbsentDirectory pins that a group with no directory name +// emits NO `directory` key. Dropping omitempty turns it into "directory": "". +func TestListJSON_OmitsAbsentDirectory(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{}} + groupsElig := &mockGroupsEligibilityLister{response: &scamodels.GroupsEligibilityResponse{ + Response: []scamodels.GroupsEligibleTarget{{ + GroupName: "grp-name", GroupID: "grp-id", DirectoryID: "dir-id", + }}, + Total: 1, + }} + + cmd := NewListCommandWithDeps(auth, elig, groupsElig) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "list", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "cloud": [], + "groups": [ + { + "groupName": "grp-name", + "groupId": "grp-id", + "directoryId": "dir-id" + } + ] +}`) +} 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_output_test.go b/cmd/request_output_test.go new file mode 100644 index 0000000..cfe2395 --- /dev/null +++ b/cmd/request_output_test.go @@ -0,0 +1,159 @@ +package cmd + +// Access-request output field mappings. +// +// Fixture values are deliberately distinct and self-describing; see the header +// of output_contract_test.go for why that is mandatory rather than cosmetic. + +import ( + "strings" + "testing" + + wfmodels "github.com/aaearon/grant-cli/internal/workflows/models" +) + +// requestFixture is one fully-populated access request. Every detail value is +// unique so a mapping swap (target with role, createdBy with updatedBy, +// timeFrom with timeTo) changes the rendered output. +func requestFixture() *wfmodels.AccessRequest { + return &wfmodels.AccessRequest{ + RequestID: "req-id", + TargetCategory: "CLOUD_CONSOLE", + RequestState: wfmodels.RequestStatePending, + RequestResult: wfmodels.RequestResultUnknown, + RequestLink: "https://example.test/req-id", + RequestDetails: map[string]interface{}{ + "locationType": "provider-fixture", + "workspaceName": "ws-name", + "roleName": "role-name", + "reason": "reason-fixture", + "priority": "priority-fixture", + "requestDate": "2026-04-21", + "timezone": "tz-fixture", + "timeFrom": "01:11", + "timeTo": "22:22", + }, + FinalizationReason: "finalization-fixture", + CreatedBy: "creator-fixture", + CreatedAt: "2026-04-20T10:00:00Z", + UpdatedBy: "updater-fixture", + UpdatedAt: "2026-04-21T11:00:00Z", + } +} + +// TestRequestList_TextFieldMapping pins the TARGET and ROLE columns of the +// `grant request list` table to their respective request details. Swapping the +// two Fprintf arguments produced an equally plausible-looking table. +func TestRequestList_TextFieldMapping(t *testing.T) { + svc := &mockAccessRequestService{ + listItems: []wfmodels.AccessRequest{*requestFixture()}, + listTotalCount: 1, + } + + root := newTestRootCommand() + root.AddCommand(NewRequestCommandWithDeps(svc)) + + out, err := executeCommand(root, "request", "list") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, out) + } + + var row string + for _, line := range strings.Split(out, "\n") { + if strings.HasPrefix(line, "req-id") { + row = line + break + } + } + if row == "" { + t.Fatalf("no data row for req-id in output:\n%s", out) + } + + // Columns: ID STATE RESULT TARGET ROLE PRIORITY CREATED BY CREATED AT. + // No fixture value contains a space, so field positions are unambiguous. + fields := strings.Fields(row) + // Guard on 7, the highest index read below: a short row must Fatal here + // rather than panic, which would abort the whole cmd test binary. + if len(fields) < 7 { + t.Fatalf("row has %d columns, want at least 7: %q", len(fields), row) + } + if fields[3] != "ws-name" { + t.Errorf("TARGET column = %q, want ws-name", fields[3]) + } + if fields[4] != "role-name" { + t.Errorf("ROLE column = %q, want role-name", fields[4]) + } + if fields[5] != "priority-fixture" { + t.Errorf("PRIORITY column = %q, want priority-fixture", fields[5]) + } + if fields[6] != "creator-fixture" { + t.Errorf("CREATED BY column = %q, want creator-fixture", fields[6]) + } +} + +// TestRequestGet_TextFieldMapping pins the created/updated attribution in the +// `grant request get` detail view. Sourcing "Created By" from UpdatedBy +// survived: both fields were "user@test" in the existing fixtures. +func TestRequestGet_TextFieldMapping(t *testing.T) { + svc := &mockAccessRequestService{getResult: requestFixture()} + + root := newTestRootCommand() + root.AddCommand(NewRequestCommandWithDeps(svc)) + + out, err := executeCommand(root, "request", "get", "req-id") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, out) + } + + for _, want := range []string{ + "Created By: creator-fixture", + "Updated By: updater-fixture", + "Created At: 2026-04-20T10:00:00Z", + "Updated At: 2026-04-21T11:00:00Z", + "Target: ws-name", + "Role: role-name", + "Time From: 01:11", + "Time To: 22:22", + } { + if !strings.Contains(out, want) { + t.Errorf("output missing %q, got:\n%s", want, out) + } + } +} + +// TestRequestGetJSON_FieldMapping is the whole-object contract for the access +// request document, shared by `request get`, `submit`, `cancel`, `approve` and +// `reject`. Swapping timeFrom with timeTo survived every existing test. +func TestRequestGetJSON_FieldMapping(t *testing.T) { + svc := &mockAccessRequestService{getResult: requestFixture()} + + root := newTestRootCommand() + root.AddCommand(NewRequestCommandWithDeps(svc)) + + stdout, stderr, err := executeCommandStreams(root, "request", "get", "req-id", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "requestId": "req-id", + "targetCategory": "CLOUD_CONSOLE", + "state": "PENDING", + "result": "UNKNOWN", + "priority": "priority-fixture", + "reason": "reason-fixture", + "provider": "provider-fixture", + "target": "ws-name", + "role": "role-name", + "requestDate": "2026-04-21", + "timezone": "tz-fixture", + "timeFrom": "01:11", + "timeTo": "22:22", + "finalizationReason": "finalization-fixture", + "requestLink": "https://example.test/req-id", + "createdBy": "creator-fixture", + "createdAt": "2026-04-20T10:00:00Z", + "updatedBy": "updater-fixture", + "updatedAt": "2026-04-21T11:00:00Z" +}`) +} 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_submit.go b/cmd/request_submit.go index f01e95f..a93c3ca 100644 --- a/cmd/request_submit.go +++ b/cmd/request_submit.go @@ -411,10 +411,8 @@ func resolveSubmitTarget(ctx context.Context, provider, targetName string, refre // Non-interactive: match by --target flag if targetName != "" { - for i := range workspaces { - if strings.EqualFold(workspaces[i].WorkspaceName, targetName) { - return &workspaces[i], nil - } + if ws := matchWorkspaceByName(workspaces, targetName); ws != nil { + return ws, nil } return nil, fmt.Errorf("no eligible workspace found matching target=%q", targetName) } @@ -428,6 +426,19 @@ func resolveSubmitTarget(ctx context.Context, provider, targetName string, refre return submitWorkspaceSelectorFn(workspaces) } +// matchWorkspaceByName resolves the --target flag against the deduplicated +// workspace list. The match is on WorkspaceName — the same value `grant list +// --output json` emits as `target` — which is what makes the emitted name +// directly reusable as `grant request submit --target`. +func matchWorkspaceByName(workspaces []submitWorkspace, targetName string) *submitWorkspace { + for i := range workspaces { + if strings.EqualFold(workspaces[i].WorkspaceName, targetName) { + return &workspaces[i] + } + } + return nil +} + func selectSubmitWorkspace(workspaces []submitWorkspace) (*submitWorkspace, error) { if !ui.IsInteractive() { return nil, errors.New("non-interactive mode requires --target") 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 729903f..af40115 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -284,11 +284,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/status_test.go b/cmd/status_test.go index 4f29520..f3c8108 100644 --- a/cmd/status_test.go +++ b/cmd/status_test.go @@ -1007,9 +1007,13 @@ func TestStatusCommand_RemainingTime(t *testing.T) { }, } - // Create a tracker with a recorded session timestamp + // Create a tracker with a recorded session timestamp. + // 14m30s ago of a 1h session leaves 45m30s, which formats as "remaining: + // 45m" and stays there for the ~30s of slack before the truncated minute + // rolls over — so the text assertion below can be exact rather than a + // prefix. "remaining: 4" was satisfied by "44m", "45m" and "4h 30m" alike. tracker := cache.NewStore(t.TempDir(), 25*time.Hour) - elevatedAt := now.Add(-15 * time.Minute) // elevated 15 minutes ago + elevatedAt := now.Add(-14*time.Minute - 30*time.Second) if err := cache.RecordSession(tracker, "tracked-session", elevatedAt); err != nil { t.Fatalf("RecordSession() error = %v", err) } @@ -1024,8 +1028,8 @@ func TestStatusCommand_RemainingTime(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(output, "remaining: 4") { - t.Errorf("output should show remaining time, got:\n%s", output) + if !strings.Contains(output, "remaining: 45m") { + t.Errorf("output should show 'remaining: 45m', got:\n%s", output) } // Untracked session should show duration if !strings.Contains(output, "duration: 30m") { diff --git a/cmd/test_helpers.go b/cmd/test_helpers_test.go similarity index 50% rename from cmd/test_helpers.go rename to cmd/test_helpers_test.go index 45d4f47..58d78bc 100644 --- a/cmd/test_helpers.go +++ b/cmd/test_helpers_test.go @@ -4,10 +4,82 @@ package cmd import ( "bytes" + "encoding/json" + "os" + "reflect" + "testing" + "github.com/aaearon/grant-cli/internal/ui" "github.com/spf13/cobra" ) +// assertJSONEqual compares a machine-facing JSON document against an inline +// literal, structurally: both sides are unmarshalled into interface{} so key +// order and indentation are irrelevant, and compared with reflect.DeepEqual. +// +// It is deliberately whole-object. A field added to an output struct breaks +// every contract test that covers it, which is the point — the machine-facing +// documents are a compatibility surface, and adding to one should require a +// conscious decision rather than passing silently. Use it once per output; +// keep focused assertions for conditional and optional fields. +func assertJSONEqual(t *testing.T, got []byte, wantJSON string) { + t.Helper() + + var gotDoc, wantDoc interface{} + if err := json.Unmarshal(got, &gotDoc); err != nil { + t.Fatalf("got is not valid JSON: %v\nraw:\n%s", err, got) + } + if err := json.Unmarshal([]byte(wantJSON), &wantDoc); err != nil { + t.Fatalf("want is not valid JSON: %v\nraw:\n%s", err, wantJSON) + } + + if reflect.DeepEqual(gotDoc, wantDoc) { + return + } + + // Re-marshal both through the same encoder so the printed diff differs + // only where the documents actually differ (MarshalIndent sorts keys). + gotPretty, _ := json.MarshalIndent(gotDoc, "", " ") + wantPretty, _ := json.MarshalIndent(wantDoc, "", " ") + t.Errorf("JSON contract mismatch\n--- got ---\n%s\n--- want ---\n%s", gotPretty, wantPretty) +} + +// 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 +156,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 fd3e343..c2d3afb 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -35,58 +35,67 @@ 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-17 | cmd/request output (text) | `cmd/request.go:79-80` | Swap the table values: `r.DetailString("workspaceName")` / `r.DetailString("roleName")` | CONFIRMED | test | `TestRequestList_TextFieldMapping` | PR5 | todo | -| REQ-18 | cmd/request output (text) | `cmd/request.go:125` | `fmt.Fprintf(w, "Created By: %s\n", r.CreatedBy)` → source from `r.UpdatedBy` | CONFIRMED | test | `TestRequestGet_TextFieldMapping` | PR5 | todo | -| REQ-19 | cmd/request output (JSON) | `cmd/request.go:190-191` | Swap `TimeFrom: r.DetailString("timeFrom")` and `TimeTo: r.DetailString("timeTo")` | CONFIRMED | test | `TestRequestGetJSON_FieldMapping` (`assertJSONEqual`) | PR5 | todo | -| REQ-20 | cmd/login | `cmd/login.go:52` | `if profile == nil {` → `if false {` (auto-configure branch). Feature *is* implemented; `login_test.go` skips it with the factually wrong reason "Auto-configure not yet implemented" — delete the skip | CONFIRMED | test | `TestRunLogin_AutoConfiguresMissingProfile` | PR4 | todo | -| REQ-21 | cmd/login | `cmd/login.go:74` | `auth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, true)` → `..., true, false)` (swap `force`/`refreshAuth`) | CONFIRMED | test | `TestRunLogin_AuthenticateFlags` | PR4 | todo | -| REQ-22 | cmd/request submit | `cmd/request_submit.go:251` | `if !ui.IsInteractive() {` → `if false {` inside the `roleID == ""` branch | CONFIRMED | test | `TestRunRequestSubmit_NonInteractiveRequiresRoleID` | PR4 | todo | -| REQ-23 | cmd integration suite | `cmd/integration_test.go` (harness, not a production site) | No mutation. Claim was "integration tests are absent from CI **and** every assertion accepts a panic". First half confirmed (`.github/workflows/ci.yml` runs `make test-race` / `go test -race`, never `-tags=integration`); second half is too broad — only line 153 accepts any panic; lines 71/125/235 require specific output | OVERSTATED | test | `TestMain` isolation + exact exit-code/error-text assertions; add `-tags=integration` to both CI legs | PR1 | 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 | -| OUT-03 | cmd/favorites (group add) | `cmd/favorites.go:388` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `addGroupFavorite` | CONFIRMED | test | `TestAddGroupFavorite_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | -| OUT-04 | cmd/list | `cmd/list.go:135` | `if provider == "" {` → `if true {` (groups fetched and emitted even when `--provider` is set) | CONFIRMED | test | `TestListCommand_ProviderSuppressesGroups` | PR5 | todo | -| OUT-05 | cmd/status JSON | `cmd/status.go:212` | `Provider: strings.ToLower(string(s.CSP))` → `strings.ToUpper(string(s.CSP))` | CONFIRMED | test | `TestStatusJSON_Contract` (`assertJSONEqual`) | PR5 | todo | -| OUT-06 | cmd/status JSON | `cmd/status.go:213` | `WorkspaceID: s.WorkspaceID` → `WorkspaceID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | -| OUT-07 | cmd/status JSON | `cmd/status.go:214` | `Duration: s.SessionDuration` → `Duration: 0` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | -| OUT-08 | cmd/status JSON | `cmd/status.go:215` | `RoleID: s.RoleID` → `RoleID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | -| OUT-09 | cmd/status JSON | `cmd/status.go:217-219` | Delete the `if name, ok := data.nameMap[s.WorkspaceID]; ok { so.WorkspaceName = name }` block | CONFIRMED | test | `TestStatusJSON_ResolvesWorkspaceName` | PR5 | todo | -| OUT-10 | cmd/status JSON | `cmd/status.go:221` | `so.Type = "group"` → `so.Type = "cloud"` | CONFIRMED | test | `TestStatusJSON_GroupSessionType` | PR5 | todo | -| OUT-11 | cmd/list JSON | `cmd/list.go:164` | `WorkspaceID: t.WorkspaceID` → `WorkspaceID: t.OrganizationID` | CONFIRMED | test | `TestListJSON_Contract` (`assertJSONEqual`) | PR5 | todo | -| OUT-12 | cmd/list JSON | `cmd/list.go:165` | `WorkspaceType: strings.ToLower(string(t.WorkspaceType))` → `WorkspaceType: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | -| OUT-13 | cmd/list JSON | `cmd/list.go:167` | `RoleID: t.RoleInfo.ID` → `RoleID: ""`. `roleId` is the field an LLM/automation feeds straight back into `grant request submit --role-id`. Note the verifier's correction: `--target` resolves on the emitted **name**, not `workspaceId` | CONFIRMED | test | `TestListJSON_RoundTripsToRequestSubmit` | PR5 | todo | -| OUT-14 | cmd/list JSON | `cmd/list.go:175` | `GroupID: g.GroupID` → `GroupID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | -| OUT-15 | cmd/list JSON | `cmd/list.go:176` | `DirectoryID: g.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | -| OUT-16 | cmd/favorites JSON | `cmd/favorites.go:462` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | todo | -| OUT-17 | cmd/favorites JSON | `cmd/favorites.go:464` | `Role: entry.Role` → `Role: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | -| OUT-18 | cmd/favorites JSON | `cmd/favorites.go:466` | `DirectoryID: entry.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | -| OUT-19 | cmd/favorites | `cmd/favorites.go:333` | `fav.Provider = cfg.DefaultProvider` → `fav.Provider = "azure"`. Every command test uses the azure default, so a non-default `DefaultProvider` (aws/gcp) is unpinned. Secondary, same defect class: `internal/config/favorites.go:21-22` independently defaults empty → `"azure"` | CONFIRMED | test | `TestFavoritesAdd_HonorsNonDefaultProvider` | PR5 | todo | -| OUT-20 | cmd/favorites | `cmd/favorites.go:199-205` | Delete the `--type groups` / `--target`+`--role` pairing validation from `parseFavoritesAddFlags`. Dead-covered: `runFavoritesAddProduction` re-validates, so this is redundancy loss for DI callers, not a current user-facing hole | CONFIRMED | test | `TestParseFavoritesAddFlags_Validation` | PR5 | todo | -| OUT-21 | cmd/status | `cmd/status.go:110-114` | Make the directory-name merge unconditional: drop the `if _, exists := data.nameMap[k]; !exists` guard. Precedence is genuinely unasserted, but in production both lookups read the same cached Azure eligibility response, so a divergence needs colliding IDs or malformed data | OVERSTATED | test | `TestStatus_DirectoryNameMergePrecedence` | PR5 | todo | -| OUT-22 | cmd/status | `cmd/status.go:129` | Delete `_ = cache.CleanupSessions(tracker, activeIDs)` | CONFIRMED | test | `TestStatus_CleansUpStaleSessionTimestamps` | PR5 | todo | -| OUT-23 | cmd/status (test quality) | `cmd/status.go:185-192` (`computeRemainingTime`) | No production defect. `TestStatusCommand_RemainingTime/text_output_shows_remaining_time` asserts `remaining: 4` as a substring, which `remaining: 4h 30m` satisfies — only the JSON sibling killed a sixfold arithmetic error. Signal-poor assertion, not an uncovered defect | CONFIRMED | test | Tighten the text subtest to an exact `remaining: 45m` | PR5 | todo | -| OUT-24 | cmd/favorites | `cmd/favorites.go:432-433` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | todo | -| OUT-25 | cmd/list flags | `cmd/list.go:71` | Delete `cmd.Flags().Bool("refresh", ...)` registration. **Verifier correction:** `grant list --refresh` is **not** already a no-op — `list.go:91-92` reads it and passes it into `buildCachedLister`, and CLAUDE.md is correct. The real finding is missing flag-registration/wiring coverage | OVERSTATED | test | `TestListCommand_RefreshBypassesCache` | PR5 | todo | -| OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | done | -| OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | refuted | n/a — already killed | — | todo | +| 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`. **Corrected after adversarial review:** the column-count guard read `len(fields) < 6` while the assertions index `fields[6]`, so a column-blanking mutation panicked and aborted the entire `cmd` test binary instead of reporting one failure. Guard raised to `< 7`; reverified with the priority+createdBy blanking mutation, which now Fatals cleanly with zero panics across the package | PR5 | done | +| 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 | done | +| 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 | done | +| 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 | todo | +| OUT-01 | cmd/list flags | `cmd/list.go:73` | Delete `cmd.MarkFlagsMutuallyExclusive("groups", "provider")`. `TestListCommand_MutualExclusivity` passes today on the *unrelated* runtime error `no eligible targets or groups found` | CONFIRMED | test | `TestListCommand_MutualExclusivity` (assert Cobra's `[groups provider] were all set`) | PR5 | done | +| 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 | done | +| OUT-03 | cmd/favorites (group add) | `cmd/favorites.go:388` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `addGroupFavorite` | CONFIRMED | test | `TestAddGroupFavorite_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | done | +| OUT-04 | cmd/list | `cmd/list.go:135` | `if provider == "" {` → `if true {` (groups fetched and emitted even when `--provider` is set) | CONFIRMED | test | `TestListCommand_ProviderSuppressesGroups` | PR5 | done | +| OUT-05 | cmd/status JSON | `cmd/status.go:212` | `Provider: strings.ToLower(string(s.CSP))` → `strings.ToUpper(string(s.CSP))` | CONFIRMED | test | `TestStatusJSON_Contract` (`assertJSONEqual`) | PR5 | done | +| OUT-06 | cmd/status JSON | `cmd/status.go:213` | `WorkspaceID: s.WorkspaceID` → `WorkspaceID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | done | +| OUT-07 | cmd/status JSON | `cmd/status.go:214` | `Duration: s.SessionDuration` → `Duration: 0` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | done | +| OUT-08 | cmd/status JSON | `cmd/status.go:215` | `RoleID: s.RoleID` → `RoleID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | done | +| OUT-09 | cmd/status JSON | `cmd/status.go:217-219` | Delete the `if name, ok := data.nameMap[s.WorkspaceID]; ok { so.WorkspaceName = name }` block | CONFIRMED | test | `TestStatusJSON_Contract` (one whole-object test kills OUT-05..10 together; no separate `TestStatusJSON_ResolvesWorkspaceName`) | PR5 | done | +| OUT-10 | cmd/status JSON | `cmd/status.go:221` | `so.Type = "group"` → `so.Type = "cloud"` | CONFIRMED | test | `TestStatusJSON_Contract` (as above; no separate `TestStatusJSON_GroupSessionType`) | PR5 | done | +| OUT-11 | cmd/list JSON | `cmd/list.go:164` | `WorkspaceID: t.WorkspaceID` → `WorkspaceID: t.OrganizationID` | CONFIRMED | test | `TestListJSON_Contract` (`assertJSONEqual`) | PR5 | done | +| OUT-12 | cmd/list JSON | `cmd/list.go:165` | `WorkspaceType: strings.ToLower(string(t.WorkspaceType))` → `WorkspaceType: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | +| OUT-13 | cmd/list JSON | `cmd/list.go:167` | `RoleID: t.RoleInfo.ID` → `RoleID: ""`. `roleId` is the field an LLM/automation feeds straight back into `grant request submit --role-id`. Note the verifier's correction: `--target` resolves on the emitted **name**, not `workspaceId` | CONFIRMED | test | `TestListJSON_Contract` + `TestListJSON_RoundTripsToRequestSubmit`. **Corrected after adversarial review:** leg 2 of the round-trip originally stubbed `resolveSubmitTargetFn` with a *textual copy* of the `resolveSubmitTarget` matcher, which pinned nothing in production — mutating `WorkspaceName` → `WorkspaceID` at `cmd/request_submit.go:415` SURVIVED the whole `cmd` suite. The earlier "reverified three ways" claim was wrong: the `Target: t.WorkspaceID` kill came from the ROOT command's `--target` (`findMatchingTarget`, `cmd/root.go:968`), not from `request submit`. Fixed by extracting `matchWorkspaceByName` in `cmd/request_submit.go` (behavior-preserving refactor; no CHANGELOG) and calling it from both production and the test. The `WorkspaceName` → `WorkspaceID` mutation now FAILS the test; reverted, it passes. `RoleID: ""` still dies against both tests | PR5 | done | +| OUT-14 | cmd/list JSON | `cmd/list.go:175` | `GroupID: g.GroupID` → `GroupID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | +| OUT-15 | cmd/list JSON | `cmd/list.go:176` | `DirectoryID: g.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | +| OUT-16 | cmd/favorites JSON | `cmd/favorites.go:462` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | done | +| OUT-17 | cmd/favorites JSON | `cmd/favorites.go:464` | `Role: entry.Role` → `Role: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | done | +| OUT-18 | cmd/favorites JSON | `cmd/favorites.go:466` | `DirectoryID: entry.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | done | +| OUT-19 | cmd/favorites | `cmd/favorites.go:333` | `fav.Provider = cfg.DefaultProvider` → `fav.Provider = "azure"`. Every command test uses the azure default, so a non-default `DefaultProvider` (aws/gcp) is unpinned. Secondary, same defect class: `internal/config/favorites.go:21-22` independently defaults empty → `"azure"` | CONFIRMED | test | `TestFavoritesAdd_HonorsNonDefaultProvider` | PR5 | done | +| OUT-20 | cmd/favorites | `cmd/favorites.go:199-205` | Delete the `--type groups` / `--target`+`--role` pairing validation from `parseFavoritesAddFlags` (reverified by deleting the whole `favType`/`else` validation block, which also drops the `--group requires --type groups` arm). Dead-covered: `runFavoritesAddProduction` re-validates, so this is redundancy loss for DI callers, not a current user-facing hole | CONFIRMED | test | `TestParseFavoritesAddFlags_Validation` | PR5 | done | +| OUT-21 | cmd/status | `cmd/status.go:110-114` | Make the directory-name merge unconditional: drop the `if _, exists := data.nameMap[k]; !exists` guard. Precedence is genuinely unasserted, but in production both lookups read the same cached Azure eligibility response, so a divergence needs colliding IDs or malformed data | OVERSTATED | test | `TestStatus_DirectoryNameMergePrecedence` | PR5 | done | +| OUT-22 | cmd/status | `cmd/status.go:129` | Delete `_ = cache.CleanupSessions(tracker, activeIDs)` | CONFIRMED | test | `TestStatus_CleansUpStaleSessionTimestamps` | PR5 | done | +| OUT-23 | cmd/status (test quality) | `cmd/status.go:185-192` (`computeRemainingTime`) | No production defect. `TestStatusCommand_RemainingTime/text_output_shows_remaining_time` asserts `remaining: 4` as a substring, which `remaining: 4h 30m` satisfies — only the JSON sibling killed a sixfold arithmetic error. Signal-poor assertion, not an uncovered defect | CONFIRMED | test | Text subtest tightened to an exact `remaining: 45m`; the fixture now elevates 14m30s ago, since 15m of a 1h session actually renders `44m`. Reverified by making `computeRemainingTime` multiply by `time.Minute` | PR5 | done | +| OUT-24 | cmd/favorites | `cmd/favorites.go:432-433` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | done | +| OUT-25 | cmd/list flags | `cmd/list.go:71` | Delete `cmd.Flags().Bool("refresh", ...)` registration. **Verifier correction:** `grant list --refresh` is **not** already a no-op — `list.go:91-92` reads it and passes it into `buildCachedLister`, and CLAUDE.md is correct. The real finding is missing flag-registration/wiring coverage | OVERSTATED | test | `TestListCommand_RefreshFlagRegistered` (renamed: the cache wiring sits behind `bootstrapSCAService`, so a unit test covers registration and parsing only, not a cache bypass) | PR5 | done | +| OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | todo | +| OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | test | `TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP` — added anyway in PR5: the old kill was incidental (an unrelated error-path assertion). Direct reverification needs a target whose CSP differs from the flag, so the fixture pairs `--provider azure` with an AWS target — a combination production filtering would never emit. The test comment now labels it deliberately impossible rather than merely "distinguishable" | PR5 | done | | OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | | OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | +| OUT-30 | cmd/request list JSON | `cmd/output_types.go:103` | `Requests []accessRequestOutput` tag `json:"requests"` → `json:"requestsX"`. Found by adversarial review of PR5 itself: the whole `request list` document was unpinned, because every test unmarshalled into the very struct under test and so was tag-symmetric | CONFIRMED | test | `TestRequestListJSON_Contract` (`assertJSONEqual`; kills OUT-30..31 together) | PR5 (#66) | done | +| OUT-31 | cmd/request list JSON | `cmd/output_types.go:102-105` | Add a spurious field to `accessRequestListOutput`: `Spurious string` tag `json:"spurious"`. An ADDED key is as much a contract change as a renamed one | CONFIRMED | test | `TestRequestListJSON_Contract` | PR5 (#66) | done | +| OUT-32 | cmd/revoke JSON | `cmd/output_types.go:60` | `SessionID` tag `json:"sessionId"` → `json:"sessionIdX"`. Every revoke test unmarshalled into `[]revocationOutput`, so the tags drifted freely | CONFIRMED | test | `TestRevokeJSON_Contract` (kills OUT-32..33 and OUT-38 together; four requested sessions with four different outcomes plus an unattributed row, so the per-row mapping is pinned, not just the envelope) | PR5 (#66) | done | +| OUT-33 | cmd/revoke JSON | `cmd/output_types.go:62` | `Outcome` tag `json:"outcome"` → `json:"outcomeX"`. The weakest point in the JSON surface: `outcome` is the single classification field callers switch on, and CLAUDE.md deliberately refuses to add derived booleans beside it | CONFIRMED | test | `TestRevokeJSON_Contract` | PR5 (#66) | done | +| OUT-34 | cmd/root elevation JSON | `cmd/output_types.go:10` | Drop `omitempty`: `json:"credentials,omitempty"` → `json:"credentials"`. Every fixture was an AWS elevation, so the absent state was never emitted; the key becomes `"credentials": null` for Azure/GCP. `cmd/root_elevate_test.go:2081` cannot catch it — it unmarshals, and `null` still yields `Credentials == nil` | CONFIRMED | test | `TestElevationJSON_AzureOmitsCredentials` — asserts the key is ABSENT, not null | PR5 (#66) | done | +| OUT-35 | cmd/status JSON | `cmd/output_types.go:35` | Drop `omitempty` from `json:"workspaceName,omitempty"`. Every fixture's workspace ID resolved through the name map | CONFIRMED | test | `TestStatusJSON_OmitsAbsentOptionalFields` (kills OUT-35..36 together) | PR5 (#66) | done | +| OUT-36 | cmd/status JSON | `cmd/output_types.go:47` | Drop `omitempty` from `json:"username,omitempty"`. Every fixture token carried a username | CONFIRMED | test | `TestStatusJSON_OmitsAbsentOptionalFields` | PR5 (#66) | done | +| OUT-37 | cmd/list JSON | `cmd/list.go:36` | Drop `omitempty` from `listGroupTarget` `json:"directory,omitempty"`. Same class: every fixture group carried a directory name | CONFIRMED | test | `TestListJSON_OmitsAbsentDirectory` | PR5 (#66) | done | +| OUT-38 | cmd/revoke JSON | `cmd/output_types.go:63` | Drop `omitempty` from `json:"reason,omitempty"`. A confirmed revocation carries no reason, so the key must be absent on the `revoked` row | CONFIRMED | test | `TestRevokeJSON_Contract` | PR5 (#66) | done | | SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | todo | | SCA-02 | internal/sca | `internal/sca/service.go:208` | `s.httpClient.Post(ctx, "/api/access/elevate", req)` → `..., nil)` | CONFIRMED | test | `TestElevate_SendsExactBody` (add `gotBody` to `mockHTTPClient`) | PR8 | todo | | SCA-03 | internal/sca | `internal/sca/service.go:236` | `s.httpClient.Post(ctx, "/api/access/sessions/revoke", req)` → `..., nil)` | CONFIRMED | test | `TestRevokeSessions_SendsExactBody` | PR8 | todo | @@ -125,33 +134,33 @@ premise does not hold). | WF-18 | internal/workflows | `internal/workflows/service.go:201` | In `GetRequest`, swallow the decode error: `if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { return &result, nil }` | CONFIRMED | test | `TestGetRequest_PropagatesDecodeError` | PR8 | todo | | WF-19 | internal/workflows | `internal/workflows/service_config.go:9` | `ServiceName: "access-requests"` → `"WRONG"`. There is no workflows `service_config_test.go` at all | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | todo | | WF-20 | internal/workflows | `internal/workflows/service.go:45` | `base.Authenticator("isp")` → `base.Authenticator("WRONG")` | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | todo | -| ELV-01 | cmd/selection | `cmd/selection.go:78` | `return &items[i], nil` → `return &items[0], nil`. `TestFindItemByDisplay` only checks non-nil/error, so selecting one display value silently elevates the first sorted target and prints a success line naming the wrong one | CONFIRMED | test | `TestFindItemByDisplay_ReturnsMatchingItem` | PR4 | todo | -| ELV-02 | cmd/root (unified elevate builder) | `cmd/root.go:786-793` | In `elevateCloud`, swap `WorkspaceID: selectedTarget.WorkspaceID` and `RoleID: selectedTarget.RoleInfo.ID` | CONFIRMED | test | `TestElevateCloud_RequestPayload` (`mockElevateService` history) | PR4 | todo | -| ELV-03 | cmd/root (unified elevate builder) | `cmd/root.go:786-788` | In `elevateCloud`, blank both `CSP:` and `OrganizationID:` | CONFIRMED | test | `TestElevateCloud_RequestPayload` | PR4 | todo | -| ELV-04 | cmd/root (env/direct elevate builder) | `cmd/root.go:497-506` | In `resolveAndElevate`, swap `WorkspaceID` and `RoleID` in the `ElevateRequest` literal. (Plan calls these "both builders": `:497` and `:786`) | CONFIRMED | test | `TestResolveAndElevate_RequestPayload` | PR4 | todo | -| ELV-05 | cmd/env favorite path | `cmd/root.go:429` | `if flags.favorite != "" {` → `if false {` in `resolveAndElevate`. No env test exercises `--favorite`, yet the flag is registered (`cmd/env.go:39`) and advertised in help (`cmd/env.go:29`) | CONFIRMED | test | `TestEnv_FavoriteMode` | PR4 | todo | -| ELV-06 | cmd/env favorite path | `cmd/root.go:438-440` | Delete the group-favorite rejection (`if fav.ResolvedType() == config.FavoriteTypeGroups { return ... }`) | CONFIRMED | test | `TestEnv_RejectsGroupFavorite` | PR4 | todo | -| ELV-07 | cmd/env favorite path | `cmd/root.go:443-445` | Delete the provider-mismatch check `if flags.provider != "" && !strings.EqualFold(flags.provider, fav.Provider)` | CONFIRMED | test | `TestEnv_FavoriteProviderMismatch` | PR4 | todo | -| ELV-08 | cmd/env direct path | `cmd/root.go:456-458` | Delete the paired `--target`/`--role` validation in `resolveAndElevate` | CONFIRMED | test | `TestEnv_RequiresBothTargetAndRole` | PR4 | todo | -| ELV-09 | cmd/root favorite path | `cmd/root.go:577` | `if fav.ResolvedType() == config.FavoriteTypeGroups {` → `if false {` in `resolveFavoriteFlags`. This is the row that refutes "all root equivalents are covered" — root group-favorite **detection** is also unpinned | CONFIRMED | test | `TestResolveFavoriteFlags_DetectsGroupFavorite` | PR4 | todo | -| ELV-10 | cmd/root group elevate | `cmd/root.go:837-841` | `if result.ErrorInfo != nil {` → `if false {` in `elevateGroup`. Execution then builds a result and returns nil: a policy denial prints as success and exits 0 | CONFIRMED | test | `TestElevateGroup_SurfacesErrorInfo` | PR4 | todo | -| ELV-11 | cmd/env elevate | `cmd/root.go:525-530` | `if result.ErrorInfo != nil {` → `if false {` in `resolveAndElevate` (the env path). Same false-success consequence | CONFIRMED | test | `TestEnv_SurfacesErrorInfo` | PR4 | todo | -| ELV-12 | cmd/env elevate | `cmd/root.go:520-522` | Delete `if len(elevateResp.Response.Results) == 0 { return nil, errors.New("elevation failed: no results returned") }` (env path). Mutant panics on `Results[0]` only if a test supplies an empty slice — none does | CONFIRMED | test | `TestEnv_EmptyResultsGuard` | PR4 | todo | -| ELV-13 | cmd/root cloud elevate | `cmd/root.go:802-805` | Delete the identical empty-`Results` guard in `elevateCloud` | CONFIRMED | test | `TestElevateCloud_EmptyResultsGuard` | PR4 | todo | -| ELV-14 | cmd/root group elevate | `cmd/root.go:832-835` | Delete the identical empty-`Results` guard in `elevateGroup` | CONFIRMED | test | `TestElevateGroup_EmptyResultsGuard` | PR4 | todo | -| ELV-15 | cmd/root JSON | `cmd/root.go:936-937` | In `writeElevationJSON`, swap `Target: cloudRes.target.WorkspaceName` and `Role: cloudRes.target.RoleInfo.Name` | CONFIRMED | test | `TestElevationJSON_Contract` (`assertJSONEqual`) | PR5 | todo | -| ELV-16 | cmd/root JSON | `cmd/root.go:923-924` | In `writeElevationJSON`, swap `GroupID: groupRes.group.GroupID` and `DirectoryID: groupRes.group.DirectoryID` | CONFIRMED | test | `TestGroupElevationJSON_Contract` | PR5 | todo | -| ELV-17 | cmd/env JSON | `cmd/env.go:145-147` | Swap `SecretAccessKey: awsCreds.SecretAccessKey` and `SessionToken: awsCreds.SessionToken`. Asymmetry is the point: the identical swap in the **text** export path is killed (`env_test.go:77`) | CONFIRMED | test | `TestEnvJSON_Contract` (`assertJSONEqual`) | PR5 | todo | -| ELV-18 | cmd/root JSON | `cmd/root.go:934` | `Provider: strings.ToLower(string(cloudRes.target.CSP))` → drop the `strings.ToLower` | CONFIRMED | test | `TestElevationJSON_Contract` | PR5 | todo | -| ELV-19 | cmd/root | `cmd/root.go:341-344` | Delete `if len(all) == 0 { return nil, errors.New("no eligible targets found, check your SCA policies") }` in `fetchEligibility`'s multi-CSP branch. Callers replace the intended aggregate error with their own message | CONFIRMED | test | `TestFetchEligibility_AllCSPsFail` | PR4 | todo | -| ELV-20 | cmd/env elevate | `cmd/root.go:510-514` | Remove the fresh-context setup and elevate with the original `ctx`: delete `elevCtx, elevCancel := context.WithTimeout(...)` / `defer elevCancel()` and call `elevateService.Elevate(ctx, req)`. **Note:** a raw `elevCtx → ctx` token substitution does *not* compile (`declared and not used: elevCtx`) — use the semantic form above. Root's three interactive dispatch paths *are* covered by `TestRootElevate_SlowPromptTimeout`; env is not | CONFIRMED | test | `TestEnv_SlowPromptTimeout` | PR4 | todo | -| ELV-21 | cmd/env auth | `cmd/root.go:419` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in `resolveAndElevate` (env path) | CONFIRMED | test | `TestEnv_AuthCacheFlag` | PR4 | todo | -| ELV-22 | cmd/root auth | `cmd/root.go:620` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in the root elevate path | CONFIRMED | test | `TestRootElevate_AuthCacheFlag` | PR4 | todo | -| ELV-23 | cmd/env selector | `cmd/root.go:480` | `selector.SelectTarget(allTargets)` → `selector.SelectTarget(nil)`. `mockTargetSelector` returns its canned target without inspecting the slice | CONFIRMED | test | `TestEnv_SelectorReceivesAllTargets` | PR4 | todo | -| ELV-24 | cmd/root Execute | `cmd/root.go:291` | `if !verbose && passedArgValidation {` → `if verbose && passedArgValidation {`. `TestVerboseHintSuppressedForArgErrors` calls Cobra's `root.Execute()` and then *reconstructs* the hint logic; it never invokes the package-level `Execute()` | CONFIRMED | test | `TestExecute_VerboseHintCondition` | PR4 | todo | -| ELV-25 | cmd/root dispatch | `cmd/root.go:631-636` | Swap the dispatch order: test `if flags.groups` before `if flags.group != ""`. `--group` and `--groups` are **not** mutually exclusive (`root.go:136-142` pairs neither), so their precedence is unspecified and unpinned | CONFIRMED | test | `TestRootElevate_GroupAndGroupsPrecedence` | PR4 | todo | -| ELV-26 | cmd test quality | `cmd/root_elevate_test.go:248` | No production site. The `multi-CSP concurrent fetch - parallel execution` case duplicates the line-174 success setup, adds sleeps, and asserts **no** elapsed time. Real concurrency is covered by `TestFetchEligibility_ConcurrentExecution` | CONFIRMED | test | Delete the duplicate case or give it a real elapsed-time assertion | PR4 | todo | -| ELV-27 | cmd test quality | `cmd/root_test.go` (`TestFetchEligibility_ConcurrentExecution`) | Claim: the `<350ms` bound for two concurrent 200ms sleeps is flaky. **Not demonstrated** — 50/50 runs passed. The wall-clock sensitivity remains a plausible overloaded-CI risk, so widen the bound; do not claim an observed flake | OVERSTATED | test | Widen the bound in `TestFetchEligibility_ConcurrentExecution` | PR4 | todo | +| 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 | 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 | done | +| 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 | done | +| 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 | done | +| 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 | done | +| 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 | todo | | SFU-02 | internal/selfupdate | `internal/selfupdate/selfupdate.go:347` | `case strings.HasPrefix(normalized, "//"):` → `case false:`. **Production change (PR2):** move this arm *before* `path.IsAbs` — `path.Clean` collapses `//host/share/x` → `/host/share/x`, so `IsAbs` always wins and the UNC arm is unreachable. Rejection is unchanged; only the message differs | CONFIRMED | test + prod-fix | `TestCheckArchivePath/unc_path` | PR2 | todo | | SFU-03 | internal/selfupdate | `internal/selfupdate/selfupdate.go:349` | `case hasDriveLetter(normalized):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/drive_absolute` | PR2 | todo | @@ -200,6 +209,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. --- @@ -213,18 +272,18 @@ premise does not hold). | PR2 — Archive extraction and path security | 12 | 11 | | PR3 — Remaining self-update correctness | 10 | 10 | | PR4 — Argument capture | 42 | 41 | -| PR5 — Output contracts | 32 | 30 | +| PR5 — Output contracts | 42 | 39 | | PR6 — Cache and config semantics | 16 | 16 | | PR7 — UI behavior | 10 | 10 | | PR8 — SCA / workflows / models wire contracts | 36 | 34 | -| *(no PR — settled, recorded only)* | 5 | 0 | -| **Total** | **165** | **152** | +| *(no PR — settled, recorded only)* | 4 | 0 | +| **Total** | **174** | **161** | ### By verdict | Verdict | Rows | |---|---| -| CONFIRMED | 152 | +| CONFIRMED | 161 | | OVERSTATED | 9 | | REFUTED | 4 | @@ -232,12 +291,12 @@ premise does not hold). | Disposition | Rows | |---|---| -| `test` | 149 | +| `test` | 158 | | `test + prod-fix` | 5 | | `prod-fix` | 1 | | `wont-fix` | 5 | | `refuted` | 5 | -| **Total** | **165** | +| **Total** | **174** | The seven production changes, matching the plan's table: