From 3419740e1f909fd51577f53ede88e3497ab930a1 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 09:59:13 +0200 Subject: [PATCH 01/13] test(cmd): capture arguments in the shared command mocks Merge argument capture into mockAccessRequestService and delete the capturing variant, and record request histories for mockElevateService and mockGroupsElevator. Move withInteractiveTTY into the shared helpers file, renamed to _test.go so test-only code stops linking into the binary. --- cmd/request_picker_test.go | 40 ++--- cmd/request_test.go | 6 +- cmd/{test_helpers.go => test_helpers_test.go} | 15 ++ cmd/test_mocks.go | 151 ++++++++++++++++-- 4 files changed, 170 insertions(+), 42 deletions(-) rename cmd/{test_helpers.go => test_helpers_test.go} (83%) diff --git a/cmd/request_picker_test.go b/cmd/request_picker_test.go index c04ebdb..8d461e4 100644 --- a/cmd/request_picker_test.go +++ b/cmd/request_picker_test.go @@ -7,32 +7,13 @@ import ( "testing" "github.com/aaearon/grant-cli/internal/ui" - "github.com/aaearon/grant-cli/internal/workflows" wfmodels "github.com/aaearon/grant-cli/internal/workflows/models" ) -// capturingMockAccessRequestService embeds mockAccessRequestService and captures list params. -type capturingMockAccessRequestService struct { - mockAccessRequestService - lastListParams workflows.ListRequestsParams -} - -func (m *capturingMockAccessRequestService) ListRequests(_ context.Context, params workflows.ListRequestsParams) ([]wfmodels.AccessRequest, int, error) { - m.lastListParams = params - return m.listItems, m.listTotalCount, m.listErr -} - -func withInteractiveTTY(t *testing.T, interactive bool) { - t.Helper() - orig := ui.IsTerminalFunc - t.Cleanup(func() { ui.IsTerminalFunc = orig }) - ui.IsTerminalFunc = func(_ uintptr) bool { return interactive } -} - func TestResolveRequestIDInteractive_NonInteractive(t *testing.T) { withInteractiveTTY(t, false) - svc := &capturingMockAccessRequestService{} + svc := &mockAccessRequestService{} _, err := resolveRequestIDInteractive(t.Context(), svc, pickerScope{emptyMsg: "access requests"}) if err == nil { t.Fatal("expected error") @@ -48,7 +29,7 @@ func TestResolveRequestIDInteractive_NonInteractive(t *testing.T) { func TestResolveRequestIDInteractive_EmptyList(t *testing.T) { withInteractiveTTY(t, true) - svc := &capturingMockAccessRequestService{} + svc := &mockAccessRequestService{} _, err := resolveRequestIDInteractive(t.Context(), svc, pickerScope{ filter: "(requestState eq PENDING)", requestRole: "APPROVER", @@ -60,23 +41,22 @@ func TestResolveRequestIDInteractive_EmptyList(t *testing.T) { if !strings.Contains(err.Error(), "pending requests assigned to you") { t.Errorf("expected emptyMsg in error, got %v", err) } - if svc.lastListParams.Filter != "(requestState eq PENDING)" { - t.Errorf("filter: got %q", svc.lastListParams.Filter) + got := svc.lastListParams() + if got.Filter != "(requestState eq PENDING)" { + t.Errorf("filter: got %q", got.Filter) } - if svc.lastListParams.RequestRole != "APPROVER" { - t.Errorf("requestRole: got %q", svc.lastListParams.RequestRole) + if got.RequestRole != "APPROVER" { + t.Errorf("requestRole: got %q", got.RequestRole) } - if svc.lastListParams.Sort != "createdAt desc" { - t.Errorf("sort: got %q", svc.lastListParams.Sort) + if got.Sort != "createdAt desc" { + t.Errorf("sort: got %q", got.Sort) } } func TestResolveRequestIDInteractive_ListError(t *testing.T) { withInteractiveTTY(t, true) - svc := &capturingMockAccessRequestService{ - mockAccessRequestService: mockAccessRequestService{listErr: errors.New("boom")}, - } + svc := &mockAccessRequestService{listErr: errors.New("boom")} _, err := resolveRequestIDInteractive(t.Context(), svc, pickerScope{emptyMsg: "x"}) if err == nil || !strings.Contains(err.Error(), "boom") { t.Fatalf("expected list error, got %v", err) diff --git a/cmd/request_test.go b/cmd/request_test.go index d9121ba..efc4540 100644 --- a/cmd/request_test.go +++ b/cmd/request_test.go @@ -767,7 +767,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 +1017,8 @@ 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) + if got := svc.lastSubmit(); got != nil { + t.Errorf("request must not be submitted, got %+v", got) } }) } diff --git a/cmd/test_helpers.go b/cmd/test_helpers_test.go similarity index 83% rename from cmd/test_helpers.go rename to cmd/test_helpers_test.go index 45d4f47..f9d9ce7 100644 --- a/cmd/test_helpers.go +++ b/cmd/test_helpers_test.go @@ -4,10 +4,25 @@ package cmd import ( "bytes" + "testing" + "github.com/aaearon/grant-cli/internal/ui" "github.com/spf13/cobra" ) +// withInteractiveTTY forces ui.IsInteractive() to the given answer for the +// duration of the test, restoring the package global via t.Cleanup. +// +// Use it in every test whose behavior depends on interactivity: `go test` +// happens to run with a non-TTY stdin, but that is an accident of the harness, +// not an assertion, and it silently reverses under a PTY. +func withInteractiveTTY(t *testing.T, interactive bool) { + t.Helper() + orig := ui.IsTerminalFunc + t.Cleanup(func() { ui.IsTerminalFunc = orig }) + ui.IsTerminalFunc = func(_ uintptr) bool { return interactive } +} + // newTestRootCommand creates a root command for testing (no elevation RunE) func newTestRootCommand() *cobra.Command { return newRootCommand(nil) diff --git a/cmd/test_mocks.go b/cmd/test_mocks.go index 75e0495..c3f96df 100644 --- a/cmd/test_mocks.go +++ b/cmd/test_mocks.go @@ -58,20 +58,47 @@ 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. Only the eligibility listers fan out across +// goroutines (cmd/helpers.go:44,59,71 and cmd/root.go:316,326,734,739); every +// Elevate call site is sequential — cmd/root.go:514 (resolveAndElevate), +// :797 (elevateCloud, reached only after the fan-out channels have been +// joined). `make test-race` is what keeps that assumption honest. 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. +func (m *mockElevateService) lastElevate() *models.ElevateRequest { + if len(m.elevateCalls) == 0 { + return nil + } + return &m.elevateCalls[len(m.elevateCalls)-1] +} + // mockTargetSelector implements the targetSelector interface for testing type mockTargetSelector struct { selectFunc func(targets []models.EligibleTarget) (*models.EligibleTarget, error) @@ -202,20 +229,37 @@ 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, cmd/root.go:827 (elevateGroup), 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. +func (m *mockGroupsElevator) lastElevateGroups() *models.GroupsElevateRequest { + if len(m.elevateCalls) == 0 { + return nil + } + return &m.elevateCalls[len(m.elevateCalls)-1] +} + // mockUnifiedSelector implements unifiedSelector for testing type mockUnifiedSelector struct { selectFunc func(items []selectionItem) (*selectionItem, error) @@ -247,7 +291,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 +330,93 @@ 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. +func (m *mockAccessRequestService) lastSubmit() *wfmodels.SubmitAccessRequest { + if len(m.submitCalls) == 0 { + return nil + } + return &m.submitCalls[len(m.submitCalls)-1] +} + +// lastCancel returns the most recent cancel call, or nil if never called. +func (m *mockAccessRequestService) lastCancel() *cancelCall { + if len(m.cancelCalls) == 0 { + return nil + } + return &m.cancelCalls[len(m.cancelCalls)-1] +} + +// lastFinalize returns the most recent finalize call, or nil if never called. +func (m *mockAccessRequestService) lastFinalize() *finalizeCall { + if len(m.finalizeCalls) == 0 { + return nil + } + return &m.finalizeCalls[len(m.finalizeCalls)-1] +} + // countingEligibilityLister wraps an eligibilityLister and counts calls per CSP. // Thread-safe for concurrent access from goroutines in fetchStatusData etc. type countingEligibilityLister struct { From 8dd4af139371f10b20f666255400d8fd60c20afa Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:05:52 +0200 Subject: [PATCH 02/13] test(cmd): pin request and login arguments against their mutations Cover the finalize decision, cancel/get request IDs, list params, submit payload, the validateSubmitFields call site, the non-TTY guards on get/approve/reject, the CSP-only GCP rejection, and login's auto-configure branch and authenticate flags. --- cmd/login_args_test.go | 106 ++++++++ cmd/login_test.go | 8 +- cmd/request_args_test.go | 552 +++++++++++++++++++++++++++++++++++++++ cmd/request_test.go | 17 +- 4 files changed, 675 insertions(+), 8 deletions(-) create mode 100644 cmd/login_args_test.go create mode 100644 cmd/request_args_test.go diff --git a/cmd/login_args_test.go b/cmd/login_args_test.go new file mode 100644 index 0000000..4443485 --- /dev/null +++ b/cmd/login_args_test.go @@ -0,0 +1,106 @@ +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) { + // runConfigure prompts through survey, which reads os.Stdin directly and + // has no injectable seam. On a real terminal that would block forever, so + // skip rather than hang; CI and every non-interactive run still cover it. + if isatty.IsTerminal(os.Stdin.Fd()) { + t.Skip("stdin is a terminal: the configure prompt would block on real input") + } + + // An empty profiles folder: LoadProfile("grant") finds nothing. + t.Setenv("IDSEC_PROFILES_FOLDER", t.TempDir()) + + auth := &mockAuthenticator{ + authenticateFunc: func(*models.IdsecProfile, *authmodels.IdsecAuthProfile, *authmodels.IdsecSecret, bool, bool) (*authmodels.IdsecToken, error) { + t.Error("authentication must not be attempted before the profile is configured") + return nil, errors.New("unreachable") + }, + } + + cmd := NewLoginCommandWithAuth(auth) + output, err := executeCommand(cmd) + + if !strings.Contains(output, "No configuration found") { + t.Errorf("expected the auto-configure announcement, got:\n%s", output) + } + if err == nil { + t.Error("expected configuration to fail without a terminal, got nil") + } +} + +// TestRunLogin_AuthenticateFlags kills REQ-21: swapping the force/refreshAuth +// arguments at cmd/login.go:74. `grant login` must force a fresh +// authentication (refreshAuth=true) without forcing a profile rewrite +// (force=false); the swap silently reuses a cached token. +func TestRunLogin_AuthenticateFlags(t *testing.T) { + t.Setenv("IDSEC_PROFILES_FOLDER", t.TempDir()) + + profile := &models.IdsecProfile{ + ProfileName: "grant", + AuthProfiles: map[string]*authmodels.IdsecAuthProfile{ + "isp": { + Username: "test.user@example.com", + AuthMethod: authmodels.Identity, + AuthMethodSettings: &authmodels.IdentityIdsecAuthMethodSettings{ + IdentityURL: "https://example.cyberark.cloud", + IdentityMFAInteractive: true, + }, + }, + }, + } + loader := &profiles.FileSystemProfilesLoader{} + if err := loader.SaveProfile(profile); err != nil { + t.Fatalf("failed to create test profile: %v", err) + } + + var gotForce, gotRefresh bool + var gotSecret *authmodels.IdsecSecret + var calls int + auth := &mockAuthenticator{ + authenticateFunc: func(_ *models.IdsecProfile, _ *authmodels.IdsecAuthProfile, secret *authmodels.IdsecSecret, force, refreshAuth bool) (*authmodels.IdsecToken, error) { + calls++ + gotForce, gotRefresh, gotSecret = force, refreshAuth, secret + return &authmodels.IdsecToken{Token: "jwt", Username: "test.user@example.com"}, nil + }, + } + + cmd := NewLoginCommandWithAuth(auth) + if _, err := executeCommand(cmd); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if calls != 1 { + t.Fatalf("expected exactly 1 Authenticate call, got %d", calls) + } + if gotForce { + t.Error("force = true, want false") + } + if !gotRefresh { + t.Error("refreshAuth = false, want true") + } + if gotSecret == nil || gotSecret.Secret != "" { + t.Errorf("secret = %+v, want an empty interactive secret", gotSecret) + } +} diff --git a/cmd/login_test.go b/cmd/login_test.go index c12da94..b6e63fe 100644 --- a/cmd/login_test.go +++ b/cmd/login_test.go @@ -259,7 +259,7 @@ func TestLoginCommandUsage(t *testing.T) { } } -func TestLoginCommandAutoConfigure(t *testing.T) { - // Placeholder test — auto-configure DI not yet wired - t.Skip("Auto-configure not yet implemented") -} +// Auto-configure is covered by TestRunLogin_AutoConfiguresMissingProfile in +// login_args_test.go. The former placeholder here skipped with the factually +// wrong reason "Auto-configure not yet implemented" — the feature is +// implemented (cmd/login.go:52). diff --git a/cmd/request_args_test.go b/cmd/request_args_test.go new file mode 100644 index 0000000..e19b311 --- /dev/null +++ b/cmd/request_args_test.go @@ -0,0 +1,552 @@ +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) + } +} + +// 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, + }, + } + + 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). +func TestRunRequestSubmit_SubmitPayload(t *testing.T) { + submitStubWorkspace(t, &submitWorkspace{ + WorkspaceName: "Prod-EastUS", + WorkspaceID: "ws-payload-1", + WorkspaceType: models.WorkspaceTypeSubscription, + CSP: models.CSPAzure, + 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": "Azure", + "roleId": "role-payload-3", + "roleName": "Contributor", + "workspaceId": "ws-payload-1", + "workspaceName": "Prod-EastUS", + "workspaceType": string(models.WorkspaceTypeSubscription), + "orgId": "org-payload-9", + "reason": "need access", + "priority": "Medium", + "requestDate": "2026-04-21", + "timezone": "UTC", + "timeFrom": "08:15", + "timeTo": "19:45", + } + 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) + } + if got := svc.lastSubmit(); got != nil { + t.Errorf("nothing may be submitted after a validation failure, got %+v", got) + } +} + +// 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) + } + if got := svc.lastSubmit(); got != nil { + t.Errorf("nothing may be submitted, got %+v", got) + } +} diff --git a/cmd/request_test.go b/cmd/request_test.go index efc4540..3b0d515 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) From abc9d45e9e6c0b722495ad4d0d56e94cafd52afe Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:09:23 +0200 Subject: [PATCH 03/13] test(cmd): pin elevation payloads, env favorite paths and guards Add argument and guard coverage for elevateCloud/elevateGroup/resolveAndElevate payloads, the four grant env --favorite paths, ErrorInfo handling, the three empty-results guards, total multi-CSP failure, env's fresh post-prompt context, the auth cache flag, selector input and --group/--groups precedence. Extract shouldShowVerboseHint so the hint condition is testable without restating it, drop a duplicated multi-CSP case and widen the concurrency bound. --- cmd/elevate_args_test.go | 708 +++++++++++++++++++++++++++++++++++++++ cmd/root.go | 12 +- cmd/root_elevate_test.go | 82 +---- cmd/test_helpers_test.go | 4 +- 4 files changed, 728 insertions(+), 78 deletions(-) create mode 100644 cmd/elevate_args_test.go diff --git a/cmd/elevate_args_test.go b/cmd/elevate_args_test.go new file mode 100644 index 0000000..f7f2b1d --- /dev/null +++ b/cmd/elevate_args_test.go @@ -0,0 +1,708 @@ +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 (cmd/root.go:786). +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 used by grant env (cmd/root.go:497). +// +// 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:429) 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 at +// cmd/root.go:438. +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 at +// cmd/root.go:443. +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 at cmd/root.go:456. +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:577), 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:837) 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 +// (cmd/root.go:525). +func TestEnv_SurfacesErrorInfo(t *testing.T) { + elevator := &mockElevateService{ + response: &models.ElevateResponse{Response: models.ElevateAccessResult{ + CSP: models.CSPAWS, + Results: []models.ElevateTargetResult{{ + WorkspaceID: "111122223333", + 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 +// cmd/root.go:342. 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 (cmd/root.go:510). 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 at cmd/root.go:480. 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...) + 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 (cmd/root.go:631) 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/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/test_helpers_test.go b/cmd/test_helpers_test.go index f9d9ce7..515176f 100644 --- a/cmd/test_helpers_test.go +++ b/cmd/test_helpers_test.go @@ -99,7 +99,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 From cf253743d3a7776575fe9428a740ace615e71b19 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:14:02 +0200 Subject: [PATCH 04/13] docs: close the PR4 rows in the mutation ledger All 42 argument-capture rows reverified fail-to-pass with -count=1; records the mutation forms used where the literal form does not compile. --- cmd/elevate_args_test.go | 12 +++- docs/mutation-ledger.md | 115 +++++++++++++++++++++++++-------------- 2 files changed, 84 insertions(+), 43 deletions(-) diff --git a/cmd/elevate_args_test.go b/cmd/elevate_args_test.go index f7f2b1d..504393b 100644 --- a/cmd/elevate_args_test.go +++ b/cmd/elevate_args_test.go @@ -360,11 +360,16 @@ func TestElevateGroup_SurfacesErrorInfo(t *testing.T) { // TestEnv_SurfacesErrorInfo kills ELV-11: the same check on the env path // (cmd/root.go:525). 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", + WorkspaceID: "111122223333", + AccessCredentials: &creds, ErrorInfo: &models.ErrorInfo{ Code: "POLICY_DENIED", Message: "elevation denied by policy", @@ -581,6 +586,11 @@ func TestEnv_SelectorReceivesAllTargets(t *testing.T) { 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 }, } diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index fd3e343..1c61fe7 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -35,28 +35,28 @@ premise does not hold). | ID | Area | File:line | Mutation (exact, applyable) | Verdict | Disposition | Planned test | PR | Status | |---|---|---|---|---|---|---|---|---| -| REQ-01 | cmd/request finalize | `cmd/request_finalize.go:100` | `svc.FinalizeRequest(ctx, requestID, decision, reason)` → `svc.FinalizeRequest(ctx, requestID, "APPROVED", reason)` | CONFIRMED | test | `TestRequestReject_SendsRejectedDecision` | PR4 | todo | -| REQ-02 | cmd/request cancel | `cmd/request_cancel.go:60` | `svc.CancelRequest(ctx, requestID, reason)` → `svc.CancelRequest(ctx, "WRONG-ID", reason)` | CONFIRMED | test | `TestRequestCancel_PassesRequestID` | PR4 | todo | -| REQ-03 | cmd/request get | `cmd/request_get.go:53` | `svc.GetRequest(ctx, requestID)` → `svc.GetRequest(ctx, "WRONG-ID")` | CONFIRMED | test | `TestRequestGet_PassesRequestID` | PR4 | todo | -| REQ-04 | cmd/request list | `cmd/request_list.go:93-98` | Swap the asc/desc branches: `order := "asc"` → `order := "desc"` and `order = "desc"` → `order = "asc"` | CONFIRMED | test | `TestRequestList_SortDirection` | PR4 | todo | -| REQ-05 | cmd/request list | `cmd/request_list.go:82` | `if role != "CREATOR" && role != "APPROVER" {` → `if false {` | CONFIRMED | test | `TestRequestList_RejectsInvalidRole` | PR4 | todo | -| REQ-06 | cmd/request list | `cmd/request_list.go:78` | `params.FreeText = v` → `params.FreeText = ""` | CONFIRMED | test | `TestRequestList_PassesFreeText` | PR4 | todo | -| REQ-07 | cmd/request list | `cmd/request_list.go:58` | Delete `filters = append(filters, fmt.Sprintf("(requestState eq %s)", upper))` | CONFIRMED | test | `TestRequestList_PassesStateFilter` | PR4 | todo | -| REQ-08 | cmd/request submit | `cmd/request_submit.go:306` | `TargetCategory: "CLOUD_CONSOLE"` → `TargetCategory: "WRONG"` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | todo | -| REQ-09 | cmd/request submit | `cmd/request_submit.go:507` | `"workspaceId": ws.WorkspaceID,` → `"workspaceId": "",` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | todo | -| REQ-10 | cmd/request submit | `cmd/request_submit.go:511-512` | Swap the two values: `"timeFrom": f.timeTo,` / `"timeTo": f.timeFrom,` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | todo | -| REQ-11 | cmd/request submit | `cmd/request_submit.go:274` | Delete the `if err := validateSubmitFields(fields); err != nil { return err }` call site | CONFIRMED | test | `TestRunRequestSubmit_InvokesValidation` | PR4 | todo | -| REQ-12 | cmd/request submit | `cmd/request_submit.go:631` | `return errors.New("--date is required")` → `return errors.New("WRONG ERROR MESSAGE")` | CONFIRMED | test | `TestValidateSubmitFields_ErrorMessages` | PR4 | todo | -| REQ-13 | cmd/request get | `cmd/request_picker.go:15` (reached from `cmd/request_get.go`) | In the `get` path, disable the guard: `if requestID == "" && !ui.IsInteractive() {` → `if false {`, keeping `_ = requestID` so it compiles | CONFIRMED | test | `TestRequestGet_NonInteractiveRequiresID` | PR4 | todo | -| REQ-14 | cmd/request approve | `cmd/request_finalize.go:16` | Disable the early non-interactive guard for `approve` (`if false {`), preserving `_ = requestID` — a literal deletion does not compile (`declared and not used: requestID`) | CONFIRMED | test | `TestRequestApprove_NonInteractiveRequiresID` | PR4 | todo | -| REQ-15 | cmd/request reject | `cmd/request_finalize.go:16` | Same as REQ-14 for the `reject` path, with `_ = requestID` | CONFIRMED | test | `TestRequestReject_NonInteractiveRequiresID` | PR4 | todo | -| REQ-16 | cmd/request submit | `cmd/request_submit.go:377` | `if ws.CSP == models.CSPGCP {` → `if false {` inside `rejectGCPWorkspace`. The fixture sets both `WorkspaceType: WorkspaceTypeProject` and `CSP: CSPGCP`, so the workspace-type switch masks loss of the CSP arm | CONFIRMED | test | `TestRejectGCPWorkspace_CSPTagOnly` | PR4 | todo | +| REQ-01 | cmd/request finalize | `cmd/request_finalize.go:100` | `svc.FinalizeRequest(ctx, requestID, decision, reason)` → `svc.FinalizeRequest(ctx, requestID, "APPROVED", reason)` | CONFIRMED | test | `TestRequestReject_SendsRejectedDecision` + `TestRequestApprove_SendsApprovedDecision` | PR4 | done | +| REQ-02 | cmd/request cancel | `cmd/request_cancel.go:60` | `svc.CancelRequest(ctx, requestID, reason)` → `svc.CancelRequest(ctx, "WRONG-ID", reason)` | CONFIRMED | test | `TestRequestCancel_PassesRequestID` | PR4 | done | +| REQ-03 | cmd/request get | `cmd/request_get.go:53` | `svc.GetRequest(ctx, requestID)` → `svc.GetRequest(ctx, "WRONG-ID")` | CONFIRMED | test | `TestRequestGet_PassesRequestID` | PR4 | done | +| REQ-04 | cmd/request list | `cmd/request_list.go:93-98` | Swap the asc/desc branches: `order := "asc"` → `order := "desc"` and `order = "desc"` → `order = "asc"` | CONFIRMED | test | `TestRequestList_ParamsFromFlags` | PR4 | done | +| REQ-05 | cmd/request list | `cmd/request_list.go:82` | `if role != "CREATOR" && role != "APPROVER" {` → `if false {` | CONFIRMED | test | `TestRequestList_RejectsInvalidRole` | PR4 | done | +| REQ-06 | cmd/request list | `cmd/request_list.go:78` | `params.FreeText = v` → `params.FreeText = ""` | CONFIRMED | test | `TestRequestList_ParamsFromFlags` | PR4 | done | +| REQ-07 | cmd/request list | `cmd/request_list.go:58` | Delete `filters = append(filters, fmt.Sprintf("(requestState eq %s)", upper))` | CONFIRMED | test | `TestRequestList_ParamsFromFlags` | PR4 | done | +| REQ-08 | cmd/request submit | `cmd/request_submit.go:306` | `TargetCategory: "CLOUD_CONSOLE"` → `TargetCategory: "WRONG"` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | done | +| REQ-09 | cmd/request submit | `cmd/request_submit.go:507` | `"workspaceId": ws.WorkspaceID,` → `"workspaceId": "",` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | done | +| REQ-10 | cmd/request submit | `cmd/request_submit.go:511-512` | Swap the two values: `"timeFrom": f.timeTo,` / `"timeTo": f.timeFrom,` | CONFIRMED | test | `TestRunRequestSubmit_SubmitPayload` | PR4 | done | +| REQ-11 | cmd/request submit | `cmd/request_submit.go:274` | Delete the `if err := validateSubmitFields(fields); err != nil { return err }` call site | CONFIRMED | test | `TestRunRequestSubmit_InvokesValidation` | PR4 | done | +| REQ-12 | cmd/request submit | `cmd/request_submit.go:631` | `return errors.New("--date is required")` → `return errors.New("WRONG ERROR MESSAGE")` | CONFIRMED | test | `TestValidateSubmitFields_ErrorMessages` | PR4 | done | +| REQ-13 | cmd/request get | `cmd/request_picker.go:15` (reached from `cmd/request_get.go`) | In the `get` path, disable the guard: `if requestID == "" && !ui.IsInteractive() {` → `if false {`, keeping `_ = requestID` so it compiles | CONFIRMED | test | `TestRequestNonInteractiveRequiresID/get` | PR4 | done | +| REQ-14 | cmd/request approve | `cmd/request_finalize.go:16` | Disable the early non-interactive guard for `approve` (`if false {`), preserving `_ = requestID` — a literal deletion does not compile (`declared and not used: requestID`) | CONFIRMED | test | `TestRequestNonInteractiveRequiresID/approve` | PR4 | done | +| REQ-15 | cmd/request reject | `cmd/request_finalize.go:16` | Same as REQ-14 for the `reject` path, with `_ = requestID` | CONFIRMED | test | `TestRequestNonInteractiveRequiresID/reject` | PR4 | done | +| REQ-16 | cmd/request submit | `cmd/request_submit.go:377` | `if ws.CSP == models.CSPGCP {` → `if false {` inside `rejectGCPWorkspace`. The fixture sets both `WorkspaceType: WorkspaceTypeProject` and `CSP: CSPGCP`, so the workspace-type switch masks loss of the CSP arm | CONFIRMED | test | `TestRejectGCPWorkspace_CSPTagOnly` | PR4 | done | | REQ-17 | cmd/request output (text) | `cmd/request.go:79-80` | Swap the table values: `r.DetailString("workspaceName")` / `r.DetailString("roleName")` | CONFIRMED | test | `TestRequestList_TextFieldMapping` | PR5 | todo | | REQ-18 | cmd/request output (text) | `cmd/request.go:125` | `fmt.Fprintf(w, "Created By: %s\n", r.CreatedBy)` → source from `r.UpdatedBy` | CONFIRMED | test | `TestRequestGet_TextFieldMapping` | PR5 | todo | | REQ-19 | cmd/request output (JSON) | `cmd/request.go:190-191` | Swap `TimeFrom: r.DetailString("timeFrom")` and `TimeTo: r.DetailString("timeTo")` | CONFIRMED | test | `TestRequestGetJSON_FieldMapping` (`assertJSONEqual`) | PR5 | todo | -| REQ-20 | cmd/login | `cmd/login.go:52` | `if profile == nil {` → `if false {` (auto-configure branch). Feature *is* implemented; `login_test.go` skips it with the factually wrong reason "Auto-configure not yet implemented" — delete the skip | CONFIRMED | test | `TestRunLogin_AutoConfiguresMissingProfile` | PR4 | todo | -| REQ-21 | cmd/login | `cmd/login.go:74` | `auth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, true)` → `..., true, false)` (swap `force`/`refreshAuth`) | CONFIRMED | test | `TestRunLogin_AuthenticateFlags` | PR4 | todo | -| REQ-22 | cmd/request submit | `cmd/request_submit.go:251` | `if !ui.IsInteractive() {` → `if false {` inside the `roleID == ""` branch | CONFIRMED | test | `TestRunRequestSubmit_NonInteractiveRequiresRoleID` | PR4 | todo | +| REQ-20 | cmd/login | `cmd/login.go:52` | `if profile == nil {` → `if false {` (auto-configure branch). Feature *is* implemented; `login_test.go` skips it with the factually wrong reason "Auto-configure not yet implemented" — delete the skip | CONFIRMED | test | `TestRunLogin_AutoConfiguresMissingProfile` | PR4 | done | +| REQ-21 | cmd/login | `cmd/login.go:74` | `auth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, true)` → `..., true, false)` (swap `force`/`refreshAuth`) | CONFIRMED | test | `TestRunLogin_AuthenticateFlags` | PR4 | done | +| REQ-22 | cmd/request submit | `cmd/request_submit.go:251` | `if !ui.IsInteractive() {` → `if false {` inside the `roleID == ""` branch | CONFIRMED | test | `TestRunRequestSubmit_NonInteractiveRequiresRoleID` | PR4 | done | | REQ-23 | cmd integration suite | `cmd/integration_test.go` (harness, not a production site) | No mutation. Claim was "integration tests are absent from CI **and** every assertion accepts a panic". First half confirmed (`.github/workflows/ci.yml` runs `make test-race` / `go test -race`, never `-tags=integration`); second half is too broad — only line 153 accepts any panic; lines 71/125/235 require specific output | OVERSTATED | test | `TestMain` isolation + exact exit-code/error-text assertions; add `-tags=integration` to both CI legs | PR1 | done | | OUT-01 | cmd/list flags | `cmd/list.go:73` | Delete `cmd.MarkFlagsMutuallyExclusive("groups", "provider")`. `TestListCommand_MutualExclusivity` passes today on the *unrelated* runtime error `no eligible targets or groups found` | CONFIRMED | test | `TestListCommand_MutualExclusivity` (assert Cobra's `[groups provider] were all set`) | PR5 | todo | | OUT-02 | cmd/favorites (interactive) | `cmd/favorites.go:258` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `selectFavoriteInteractive` | CONFIRMED | test | `TestFavoritesAddInteractive_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | @@ -125,33 +125,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-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 | todo | | ELV-16 | cmd/root JSON | `cmd/root.go:923-924` | In `writeElevationJSON`, swap `GroupID: groupRes.group.GroupID` and `DirectoryID: groupRes.group.DirectoryID` | CONFIRMED | test | `TestGroupElevationJSON_Contract` | PR5 | todo | | ELV-17 | cmd/env JSON | `cmd/env.go:145-147` | Swap `SecretAccessKey: awsCreds.SecretAccessKey` and `SessionToken: awsCreds.SessionToken`. Asymmetry is the point: the identical swap in the **text** export path is killed (`env_test.go:77`) | CONFIRMED | test | `TestEnvJSON_Contract` (`assertJSONEqual`) | PR5 | todo | | ELV-18 | cmd/root JSON | `cmd/root.go:934` | `Provider: strings.ToLower(string(cloudRes.target.CSP))` → drop the `strings.ToLower` | CONFIRMED | test | `TestElevationJSON_Contract` | PR5 | todo | -| ELV-19 | cmd/root | `cmd/root.go:341-344` | Delete `if len(all) == 0 { return nil, errors.New("no eligible targets found, check your SCA policies") }` in `fetchEligibility`'s multi-CSP branch. Callers replace the intended aggregate error with their own message | CONFIRMED | test | `TestFetchEligibility_AllCSPsFail` | PR4 | todo | -| ELV-20 | cmd/env elevate | `cmd/root.go:510-514` | Remove the fresh-context setup and elevate with the original `ctx`: delete `elevCtx, elevCancel := context.WithTimeout(...)` / `defer elevCancel()` and call `elevateService.Elevate(ctx, req)`. **Note:** a raw `elevCtx → ctx` token substitution does *not* compile (`declared and not used: elevCtx`) — use the semantic form above. Root's three interactive dispatch paths *are* covered by `TestRootElevate_SlowPromptTimeout`; env is not | CONFIRMED | test | `TestEnv_SlowPromptTimeout` | PR4 | todo | -| ELV-21 | cmd/env auth | `cmd/root.go:419` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in `resolveAndElevate` (env path) | CONFIRMED | test | `TestEnv_AuthCacheFlag` | PR4 | todo | -| ELV-22 | cmd/root auth | `cmd/root.go:620` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in the root elevate path | CONFIRMED | test | `TestRootElevate_AuthCacheFlag` | PR4 | todo | -| ELV-23 | cmd/env selector | `cmd/root.go:480` | `selector.SelectTarget(allTargets)` → `selector.SelectTarget(nil)`. `mockTargetSelector` returns its canned target without inspecting the slice | CONFIRMED | test | `TestEnv_SelectorReceivesAllTargets` | PR4 | todo | -| ELV-24 | cmd/root Execute | `cmd/root.go:291` | `if !verbose && passedArgValidation {` → `if verbose && passedArgValidation {`. `TestVerboseHintSuppressedForArgErrors` calls Cobra's `root.Execute()` and then *reconstructs* the hint logic; it never invokes the package-level `Execute()` | CONFIRMED | test | `TestExecute_VerboseHintCondition` | PR4 | todo | -| ELV-25 | cmd/root dispatch | `cmd/root.go:631-636` | Swap the dispatch order: test `if flags.groups` before `if flags.group != ""`. `--group` and `--groups` are **not** mutually exclusive (`root.go:136-142` pairs neither), so their precedence is unspecified and unpinned | CONFIRMED | test | `TestRootElevate_GroupAndGroupsPrecedence` | PR4 | todo | -| ELV-26 | cmd test quality | `cmd/root_elevate_test.go:248` | No production site. The `multi-CSP concurrent fetch - parallel execution` case duplicates the line-174 success setup, adds sleeps, and asserts **no** elapsed time. Real concurrency is covered by `TestFetchEligibility_ConcurrentExecution` | CONFIRMED | test | Delete the duplicate case or give it a real elapsed-time assertion | PR4 | todo | -| ELV-27 | cmd test quality | `cmd/root_test.go` (`TestFetchEligibility_ConcurrentExecution`) | Claim: the `<350ms` bound for two concurrent 200ms sleeps is flaky. **Not demonstrated** — 50/50 runs passed. The wall-clock sensitivity remains a plausible overloaded-CI risk, so widen the bound; do not claim an observed flake | OVERSTATED | test | Widen the bound in `TestFetchEligibility_ConcurrentExecution` | PR4 | todo | +| ELV-19 | cmd/root | `cmd/root.go:341-344` | Delete `if len(all) == 0 { return nil, errors.New("no eligible targets found, check your SCA policies") }` in `fetchEligibility`'s multi-CSP branch. Callers replace the intended aggregate error with their own message | CONFIRMED | test | `TestFetchEligibility_AllCSPsFail` | PR4 | done | +| ELV-20 | cmd/env elevate | `cmd/root.go:510-514` | Remove the fresh-context setup and elevate with the original `ctx`: delete `elevCtx, elevCancel := context.WithTimeout(...)` / `defer elevCancel()` and call `elevateService.Elevate(ctx, req)`. **Note:** a raw `elevCtx → ctx` token substitution does *not* compile (`declared and not used: elevCtx`) — use the semantic form above. Root's three interactive dispatch paths *are* covered by `TestRootElevate_SlowPromptTimeout`; env is not | CONFIRMED | test | `TestEnv_SlowPromptTimeout` | PR4 | done | +| ELV-21 | cmd/env auth | `cmd/root.go:419` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in `resolveAndElevate` (env path) | CONFIRMED | test | `TestAuthCacheFlag/env_path` | PR4 | done | +| ELV-22 | cmd/root auth | `cmd/root.go:620` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in the root elevate path | CONFIRMED | test | `TestAuthCacheFlag/root_path` | PR4 | done | +| ELV-23 | cmd/env selector | `cmd/root.go:480` | `selector.SelectTarget(allTargets)` → `selector.SelectTarget(nil)`. `mockTargetSelector` returns its canned target without inspecting the slice | CONFIRMED | test | `TestEnv_SelectorReceivesAllTargets` | PR4 | done | +| ELV-24 | cmd/root Execute | `cmd/root.go:291` | `if !verbose && passedArgValidation {` → `if verbose && passedArgValidation {`. `TestVerboseHintSuppressedForArgErrors` calls Cobra's `root.Execute()` and then *reconstructs* the hint logic; it never invokes the package-level `Execute()` | CONFIRMED | test | `TestExecute_VerboseHintCondition` over the extracted `shouldShowVerboseHint` | 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 | @@ -203,6 +203,37 @@ premise does not hold). --- +## 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. +- **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. + +--- + ## Summary ### By PR From 0b5e38b26ebc5d703c60f4e5b7bd9b05dc076601 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:14:17 +0200 Subject: [PATCH 05/13] docs: record the mock capture convention in CLAUDE.md --- CLAUDE.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index a0da585..afc2627 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,6 +85,10 @@ Custom `SCAAccessService` follows SDK conventions: - `httptest.NewServer` for service mocks - `httpClient` interface for DI - Test files co-located as `_test.go` +- **Mock capture convention** (`cmd/test_mocks.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: only the eligibility listers fan out across goroutines (`cmd/helpers.go:44,59,71`, `cmd/root.go:316,326,734,739`); `Elevate`, `ElevateGroups` and all five `accessRequestService` methods are called from strictly sequential paths. `make test-race` is what keeps that honest +- Shared test helpers live in `cmd/test_helpers_test.go` (`executeCommand`, `executeCommandStreams`, `executeWithHint`, `withInteractiveTTY`). It is a `_test.go` file on purpose — it was a production file, which linked test-only code into the binary +- 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 From 772058512772231983a14b4444a04daa689b3b4b Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:33:16 +0200 Subject: [PATCH 06/13] test(cmd): pin the verbose hint's call site in Execute() end to end Extracting shouldShowVerboseHint made the predicate testable but left the call site unpinned: executeWithHint calls the same predicate, so deleting the whole if-block from Execute() kept go test ./cmd/ green. TestIntegration_VerboseHint drives the compiled binary and asserts the hint is present on a runtime error and absent for an unknown subcommand, an unknown flag, and an already-verbose run. --- cmd/integration_test.go | 57 +++++++++++++++++++++++++++++++++++++++++ docs/mutation-ledger.md | 19 +++++++++++++- 2 files changed, 75 insertions(+), 1 deletion(-) 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/docs/mutation-ledger.md b/docs/mutation-ledger.md index 1c61fe7..7821a87 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -148,7 +148,7 @@ premise does not hold). | 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` | 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 | @@ -217,6 +217,23 @@ where the production site moved. The forms actually used: 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. From b4b29b916fb1925dbcdcdeb8067b811e4558466c Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:33:21 +0200 Subject: [PATCH 07/13] test(cmd): set interactivity explicitly in the submit missing-flags test TestRunRequestSubmit_MissingFlags_NonInteractive relied on go test's ambient non-TTY stdin, the one violation of the MUST added in PR4. It fails under a forced TTY; withInteractiveTTY(t, false) makes the precondition its name already claims an assertion. Also switches the GCP negative assertion to len(submitCalls) != 0, which distinguishes 'never called' from 'called with nil'. --- cmd/request_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cmd/request_test.go b/cmd/request_test.go index 3b0d515..5cc2164 100644 --- a/cmd/request_test.go +++ b/cmd/request_test.go @@ -626,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 }() @@ -1026,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 got := svc.lastSubmit(); got != nil { - t.Errorf("request must not be submitted, got %+v", got) + // 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) } }) } From 1a1a9bb0317e154301b56e4306e08102439e3fb7 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:33:27 +0200 Subject: [PATCH 08/13] test(cmd): keep survey's terminal escapes off the developer's stdout TestRunLogin_AutoConfiguresMissingProfile reaches runConfigure, and survey writes prompts straight to os.Stdout rather than the cobra buffer, so every go test ./cmd/ emitted control sequences. One of them, ESC[6n, makes the terminal reply on stdin and corrupt the shell prompt. withDiscardedStdout redirects os.Stdout for the test's duration, restoring it via t.Cleanup, with no production prompt seam. --- cmd/login_args_test.go | 5 +++++ cmd/test_helpers_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/cmd/login_args_test.go b/cmd/login_args_test.go index 4443485..dbaed9a 100644 --- a/cmd/login_args_test.go +++ b/cmd/login_args_test.go @@ -29,6 +29,11 @@ func TestRunLogin_AutoConfiguresMissingProfile(t *testing.T) { 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()) diff --git a/cmd/test_helpers_test.go b/cmd/test_helpers_test.go index 515176f..38d0c1c 100644 --- a/cmd/test_helpers_test.go +++ b/cmd/test_helpers_test.go @@ -4,6 +4,7 @@ package cmd import ( "bytes" + "os" "testing" "github.com/aaearon/grant-cli/internal/ui" @@ -23,6 +24,29 @@ func withInteractiveTTY(t *testing.T, interactive bool) { 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) From 0c030e097f52813faabdb6a0ec5bb2b9e664bbb7 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:33:27 +0200 Subject: [PATCH 09/13] docs: correct the test_helpers rationale and strengthen submit negatives The CLAUDE.md note claimed the old production cmd/test_helpers.go 'linked test-only code into the binary'. It did not: that file imported only bytes and cobra, and 'go list -deps . | rg testing' matches on neither the base nor HEAD. The move was preventive. The two remaining lastSubmit() != nil negatives become len(submitCalls) != 0, matching the elevateCalls form already used in the env tests and distinguishing 'never called' from 'called with nil'. --- CLAUDE.md | 2 +- cmd/request_args_test.go | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index afc2627..67fb763 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -87,7 +87,7 @@ Custom `SCAAccessService` follows SDK conventions: - Test files co-located as `_test.go` - **Mock capture convention** (`cmd/test_mocks.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: only the eligibility listers fan out across goroutines (`cmd/helpers.go:44,59,71`, `cmd/root.go:316,326,734,739`); `Elevate`, `ElevateGroups` and all five `accessRequestService` methods are called from strictly sequential paths. `make test-race` is what keeps that honest -- Shared test helpers live in `cmd/test_helpers_test.go` (`executeCommand`, `executeCommandStreams`, `executeWithHint`, `withInteractiveTTY`). It is a `_test.go` file on purpose — it was a production file, which linked test-only code into the binary +- Shared test helpers live in `cmd/test_helpers_test.go` (`executeCommand`, `executeCommandStreams`, `executeWithHint`, `withInteractiveTTY`). It is a `_test.go` file on purpose. It used to be a production file; that never linked `testing` into the binary (the old file imported only `bytes` and `cobra`), so the move was **preventive, not remedial** — adding `withInteractiveTTY` would have been the first helper to pull `testing` in - 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. diff --git a/cmd/request_args_test.go b/cmd/request_args_test.go index e19b311..f0ff186 100644 --- a/cmd/request_args_test.go +++ b/cmd/request_args_test.go @@ -393,8 +393,10 @@ func TestRunRequestSubmit_InvokesValidation(t *testing.T) { if !strings.Contains(err.Error(), "--priority must be High, Medium, or Low") { t.Errorf("error = %v, want the --priority validation message", err) } - if got := svc.lastSubmit(); got != nil { - t.Errorf("nothing may be submitted after a validation failure, got %+v", got) + // 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) } } @@ -546,7 +548,9 @@ func TestRunRequestSubmit_NonInteractiveRequiresRoleID(t *testing.T) { if !strings.Contains(err.Error(), "requires --role-id") { t.Errorf("error = %v, want it to demand --role-id", err) } - if got := svc.lastSubmit(); got != nil { - t.Errorf("nothing may be submitted, got %+v", got) + // 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) } } From c122c410dccc60715ad43d53ef5bb1e6dced0f75 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:28:55 +0200 Subject: [PATCH 10/13] test(cmd): move the shared mocks into a _test.go file cmd/test_mocks.go was 443 lines of test scaffolding compiled into the production cmd package. The same argument that moved test_helpers.go applies with more force here: this is the larger and faster-growing of the two files. go list -deps . | grep -c '^testing$' stays 0. --- CLAUDE.md | 6 +++--- cmd/{test_mocks.go => test_mocks_test.go} | 0 2 files changed, 3 insertions(+), 3 deletions(-) rename cmd/{test_mocks.go => test_mocks_test.go} (100%) diff --git a/CLAUDE.md b/CLAUDE.md index 67fb763..fa407fa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,9 +85,9 @@ 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.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 +- **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: only the eligibility listers fan out across goroutines (`cmd/helpers.go:44,59,71`, `cmd/root.go:316,326,734,739`); `Elevate`, `ElevateGroups` and all five `accessRequestService` methods are called from strictly sequential paths. `make test-race` is what keeps that honest -- Shared test helpers live in `cmd/test_helpers_test.go` (`executeCommand`, `executeCommandStreams`, `executeWithHint`, `withInteractiveTTY`). It is a `_test.go` file on purpose. It used to be a production file; that never linked `testing` into the binary (the old file imported only `bytes` and `cobra`), so the move was **preventive, not remedial** — adding `withInteractiveTTY` would have been the first helper to pull `testing` in +- **Test scaffolding in `cmd` lives in `_test.go` files** so `testing` never enters the production build: `cmd/test_helpers_test.go` (`executeCommand`, `executeCommandStreams`, `executeWithHint`, `withInteractiveTTY`) and `cmd/test_mocks_test.go` (every shared mock). Both used to be production files. Neither linked `testing` in at the time, so the moves were **preventive, not remedial** — but `withInteractiveTTY` would have been the first helper to pull it in, and the mock file is the larger and faster-growing of the two. `go list -deps . | grep -c '^testing$'` must stay `0` - Any test whose behavior depends on interactivity MUST set it explicitly with `withInteractiveTTY`: `go test` happens to run with a non-TTY stdin, but that is an accident of the harness, not an assertion - Tests that swap a package-level var (e.g. `ui.IsTerminalFunc`, `recordSessionTimestamp`, `bootstrapImpl`) MUST NOT call `t.Parallel()` — `-race` flags concurrent access to the global. Mark them with a `// Not parallel: mutates the package-global X.` comment. This is why the `cmd` package tests are all serial. @@ -343,7 +343,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/test_mocks.go b/cmd/test_mocks_test.go similarity index 100% rename from cmd/test_mocks.go rename to cmd/test_mocks_test.go From 6a7329b91e6d6ac383ee7227bd735f0196d38bea Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:29:13 +0200 Subject: [PATCH 11/13] docs(test): cite function names instead of rotted cmd/root.go line numbers Extracting shouldShowVerboseHint shifted everything below it by ten lines, so every cmd/root.go:NNN reference written in this PR pointed at unrelated code. The no-mutex safety argument is carried entirely by those references, so a reviewer following them landed nowhere useful. Replaced with function names, which do not rot. --- CLAUDE.md | 2 +- cmd/elevate_args_test.go | 36 ++++++++++++++++++++---------------- cmd/test_mocks_test.go | 17 +++++++++++------ 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index fa407fa..75c3635 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -86,7 +86,7 @@ Custom `SCAAccessService` follows SDK conventions: - `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: only the eligibility listers fan out across goroutines (`cmd/helpers.go:44,59,71`, `cmd/root.go:316,326,734,739`); `Elevate`, `ElevateGroups` and all five `accessRequestService` methods are called from strictly sequential paths. `make test-race` is what keeps that honest +- No mutex on those histories: the only mocks reached from more than one goroutine are the eligibility listers, via the fan-outs in `fetchEligibility`/`fetchGroupsEligibility` (`cmd/root.go`), `resolveAndElevateUnifiedPath` (`cmd/root.go`) and `fetchAllTargets`/`fetchAllGroups` (`cmd/helpers.go`), and those mocks are stateless readers. `Elevate`, `ElevateGroups` and all five `accessRequestService` methods are called from strictly sequential paths. `make test-race` is what keeps that honest. Cite the **function name**, not a line number — this reasoning has already been invalidated once by an unrelated insertion above it - **Test scaffolding in `cmd` lives in `_test.go` files** so `testing` never enters the production build: `cmd/test_helpers_test.go` (`executeCommand`, `executeCommandStreams`, `executeWithHint`, `withInteractiveTTY`) and `cmd/test_mocks_test.go` (every shared mock). Both used to be production files. Neither linked `testing` in at the time, so the moves were **preventive, not remedial** — but `withInteractiveTTY` would have been the first helper to pull it in, and the mock file is the larger and faster-growing of the two. `go list -deps . | grep -c '^testing$'` must stay `0` - Any test whose behavior depends on interactivity MUST set it explicitly with `withInteractiveTTY`: `go test` happens to run with a non-TTY stdin, but that is an accident of the harness, not an assertion - Tests that swap a package-level var (e.g. `ui.IsTerminalFunc`, `recordSessionTimestamp`, `bootstrapImpl`) MUST NOT call `t.Parallel()` — `-race` flags concurrent access to the global. Mark them with a `// Not parallel: mutates the package-global X.` comment. This is why the `cmd` package tests are all serial. diff --git a/cmd/elevate_args_test.go b/cmd/elevate_args_test.go index 504393b..240b8a8 100644 --- a/cmd/elevate_args_test.go +++ b/cmd/elevate_args_test.go @@ -115,7 +115,7 @@ func TestFindItemByDisplay_ReturnsMatchingItem(t *testing.T) { // TestElevateCloud_RequestPayload kills ELV-02 and ELV-03: swapping // WorkspaceID/RoleID, or blanking CSP/OrganizationID, in the elevateCloud -// request builder (cmd/root.go:786). +// request builder in elevateCloud (cmd/root.go). func TestElevateCloud_RequestPayload(t *testing.T) { target := &models.EligibleTarget{ CSP: models.CSPAWS, @@ -139,7 +139,7 @@ func TestElevateCloud_RequestPayload(t *testing.T) { } // TestResolveAndElevate_RequestPayload kills ELV-04: the same swap in the -// second, byte-identical builder used by grant env (cmd/root.go:497). +// 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. @@ -205,7 +205,7 @@ func envConfigWithFavorites(favs map[string]config.Favorite) *config.Config { } // TestEnv_FavoriteMode kills ELV-05: the `--favorite` branch in -// resolveAndElevate (cmd/root.go:429) is registered and advertised by grant env +// 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() @@ -224,8 +224,8 @@ func TestEnv_FavoriteMode(t *testing.T) { assertElevateRequest(t, elevator, models.CSPAWS, "o-env-1", "111122223333", "role-env-9") } -// TestEnv_RejectsGroupFavorite kills ELV-06: the group-favorite rejection at -// cmd/root.go:438. +// 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{ @@ -245,8 +245,8 @@ func TestEnv_RejectsGroupFavorite(t *testing.T) { } } -// TestEnv_FavoriteProviderMismatch kills ELV-07: the provider-mismatch check at -// cmd/root.go:443. +// 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{ @@ -267,7 +267,7 @@ func TestEnv_FavoriteProviderMismatch(t *testing.T) { } // TestEnv_RequiresBothTargetAndRole kills ELV-08: the paired --target/--role -// validation at cmd/root.go:456. +// validation in resolveAndElevate (cmd/root.go). func TestEnv_RequiresBothTargetAndRole(t *testing.T) { tests := []struct { name string @@ -298,7 +298,7 @@ func TestEnv_RequiresBothTargetAndRole(t *testing.T) { } // TestResolveFavoriteFlags_DetectsGroupFavorite kills ELV-09: the group -// detection in resolveFavoriteFlags (cmd/root.go:577), which is the root +// 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{ @@ -328,7 +328,7 @@ func TestResolveFavoriteFlags_DetectsGroupFavorite(t *testing.T) { } // TestElevateGroup_SurfacesErrorInfo kills ELV-10: dropping the ErrorInfo check -// in elevateGroup (cmd/root.go:837) makes a policy denial print as a success +// in elevateGroup (cmd/root.go) makes a policy denial print as a success // and exit 0. func TestElevateGroup_SurfacesErrorInfo(t *testing.T) { elevator := &mockGroupsElevator{ @@ -357,8 +357,8 @@ func TestElevateGroup_SurfacesErrorInfo(t *testing.T) { } } -// TestEnv_SurfacesErrorInfo kills ELV-11: the same check on the env path -// (cmd/root.go:525). +// 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 @@ -446,7 +446,8 @@ func TestElevate_EmptyResultsGuards(t *testing.T) { } // TestFetchEligibility_AllCSPsFail kills ELV-19: the aggregate guard at -// cmd/root.go:342. Without it a total multi-CSP failure returns (nil, nil) and +// 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{ @@ -468,7 +469,8 @@ func TestFetchEligibility_AllCSPsFail(t *testing.T) { } // TestEnv_SlowPromptTimeout kills ELV-20: elevating on the original ctx rather -// than a fresh one (cmd/root.go:510). A user who takes longer than apiTimeout +// 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) { @@ -565,7 +567,8 @@ func assertCachedAuth(t *testing.T, got []bool) { } // TestEnv_SelectorReceivesAllTargets kills ELV-23: passing nil (or anything -// else) to SelectTarget at cmd/root.go:480. mockTargetSelector returns its +// 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{ @@ -637,7 +640,8 @@ func TestExecute_VerboseHintCondition(t *testing.T) { } // TestRootElevate_GroupAndGroupsPrecedence kills ELV-25: --group and --groups -// are not mutually exclusive, so their dispatch order (cmd/root.go:631) is a +// 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) { diff --git a/cmd/test_mocks_test.go b/cmd/test_mocks_test.go index c3f96df..aacc9ca 100644 --- a/cmd/test_mocks_test.go +++ b/cmd/test_mocks_test.go @@ -64,11 +64,16 @@ func (m *mockEligibilityLister) ListEligibility(ctx context.Context, csp models. // method body *before* dispatching to elevateFunc, so a test that supplies a // callback still gets the history. // -// No mutex, deliberately. Only the eligibility listers fan out across -// goroutines (cmd/helpers.go:44,59,71 and cmd/root.go:316,326,734,739); every -// Elevate call site is sequential — cmd/root.go:514 (resolveAndElevate), -// :797 (elevateCloud, reached only after the fan-out channels have been -// joined). `make test-race` is what keeps that assumption honest. +// 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 @@ -231,7 +236,7 @@ func (m *mockGroupsEligibilityLister) ListGroupsEligibility(ctx context.Context, // mockGroupsElevator implements groupsElevator for testing. // Same capture convention and same no-mutex reasoning as mockElevateService: -// the sole call site, cmd/root.go:827 (elevateGroup), is sequential. +// 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 From a16d8b303ea65560ac351f89c6c89ea90dfd49a3 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:29:24 +0200 Subject: [PATCH 12/13] test(cmd): pin the AWS locationType mapping and the empty --reason collapse - TestRunRequestSubmit_SubmitPayload becomes a table over Azure and AWS. The AWS row is where locationType stops being a naive string(ws.CSP), so an Azure-only fixture left "AWS" -> "Aws" alive. - The RequestDetails assertion now compares key counts, so an extra key in the payload fails instead of going unnoticed. - Add a --reason "" case to the cancel and finalize arg tests, pinning the deliberate collapse of explicit-empty into unset. - lastX() accessors return a pointer to a copy, not into the history slice's backing array, which append can reallocate under a held pointer. --- cmd/request_args_test.go | 178 +++++++++++++++++++++++++++------------ cmd/test_mocks_test.go | 23 +++-- 2 files changed, 144 insertions(+), 57 deletions(-) diff --git a/cmd/request_args_test.go b/cmd/request_args_test.go index f0ff186..555723f 100644 --- a/cmd/request_args_test.go +++ b/cmd/request_args_test.go @@ -92,6 +92,37 @@ func TestRequestApprove_SendsApprovedDecision(t *testing.T) { } } +// 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) { @@ -108,6 +139,14 @@ func TestRequestCancel_PassesRequestID(t *testing.T) { 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 { @@ -299,64 +338,99 @@ func submitStubWorkspace(t *testing.T, ws *submitWorkspace) { // 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) { - submitStubWorkspace(t, &submitWorkspace{ - WorkspaceName: "Prod-EastUS", - WorkspaceID: "ws-payload-1", - WorkspaceType: models.WorkspaceTypeSubscription, - CSP: models.CSPAzure, - OrganizationID: "org-payload-9", - }) - - svc := &mockAccessRequestService{ - submitResult: &wfmodels.AccessRequest{ - RequestID: "req-new", - RequestState: wfmodels.RequestStatePending, + 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", }, } - cmd := NewRequestCommandWithDeps(svc) - root := newTestRootCommand() - root.AddCommand(cmd) + 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", + }) - 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) - } + 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) + } - 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": "Azure", - "roleId": "role-payload-3", - "roleName": "Contributor", - "workspaceId": "ws-payload-1", - "workspaceName": "Prod-EastUS", - "workspaceType": string(models.WorkspaceTypeSubscription), - "orgId": "org-payload-9", - "reason": "need access", - "priority": "Medium", - "requestDate": "2026-04-21", - "timezone": "UTC", - "timeFrom": "08:15", - "timeTo": "19:45", - } - for key, want := range wantDetails { - if got := sent.RequestDetails[key]; got != want { - t.Errorf("requestDetails[%q] = %v, want %v", key, got, want) - } + 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) + } + } + }) } } diff --git a/cmd/test_mocks_test.go b/cmd/test_mocks_test.go index aacc9ca..721b2e1 100644 --- a/cmd/test_mocks_test.go +++ b/cmd/test_mocks_test.go @@ -97,11 +97,16 @@ func (m *mockElevateService) Elevate(ctx context.Context, req *models.ElevateReq } // 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 } - return &m.elevateCalls[len(m.elevateCalls)-1] + c := m.elevateCalls[len(m.elevateCalls)-1] + return &c } // mockTargetSelector implements the targetSelector interface for testing @@ -258,11 +263,13 @@ func (m *mockGroupsElevator) ElevateGroups(ctx context.Context, req *models.Grou } // 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 } - return &m.elevateCalls[len(m.elevateCalls)-1] + c := m.elevateCalls[len(m.elevateCalls)-1] + return &c } // mockUnifiedSelector implements unifiedSelector for testing @@ -399,27 +406,33 @@ func (m *mockAccessRequestService) lastListParams() workflows.ListRequestsParams } // 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 } - return &m.submitCalls[len(m.submitCalls)-1] + 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 } - return &m.cancelCalls[len(m.cancelCalls)-1] + 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 } - return &m.finalizeCalls[len(m.finalizeCalls)-1] + c := m.finalizeCalls[len(m.finalizeCalls)-1] + return &c } // countingEligibilityLister wraps an eligibilityLister and counts calls per CSP. From b69451d2eb28335749020cee6bc731810c8d4567 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:29:34 +0200 Subject: [PATCH 13/13] docs: record the two coverage caveats in the mutation ledger COV-01: deleting the verbose-hint call site from Execute() is killed only on the integration leg; the default local go test ./cmd/ loop leaves it green. COV-02: TestRunLogin_AutoConfiguresMissingProfile skips under a PTY, so the auto-configure branch has no coverage there. survey reads os.Stdin with no injectable seam, so this is accepted rather than fixed; the skip comment now says so plainly. --- cmd/login_args_test.go | 15 ++++++++++++--- docs/mutation-ledger.md | 2 ++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/cmd/login_args_test.go b/cmd/login_args_test.go index dbaed9a..8c6f0b6 100644 --- a/cmd/login_args_test.go +++ b/cmd/login_args_test.go @@ -22,9 +22,18 @@ import ( // fails rather than blocking; the assertion is on the announcement plus the // fact that authentication was never attempted. func TestRunLogin_AutoConfiguresMissingProfile(t *testing.T) { - // runConfigure prompts through survey, which reads os.Stdin directly and - // has no injectable seam. On a real terminal that would block forever, so - // skip rather than hang; CI and every non-interactive run still cover it. + // 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") } diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index 7821a87..9d2eab9 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -200,6 +200,8 @@ 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 | ---