From bd77aa40c990e4118f0a6f4d1bed5e9554c2ca6c Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:30:35 +0200 Subject: [PATCH 1/7] test(cmd): pin the machine-facing JSON output contracts Add assertJSONEqual and one whole-object contract test per machine-facing document: status, list, cloud and group elevation, env credentials and the favorites list. Inline expected JSON, deliberately brittle against added fields so a new field forces a compatibility review. Also add the list -> request submit round trip: the emitted target NAME feeds --target and the emitted roleId feeds --role-id, and both must resolve back to the same eligible target. Fixture values are all distinct and self-describing on purpose; a swap mutation is invisible when both sides hold the same string. --- cmd/output_contract_test.go | 594 ++++++++++++++++++++++++++++++++++++ cmd/test_helpers_test.go | 33 ++ 2 files changed, 627 insertions(+) create mode 100644 cmd/output_contract_test.go diff --git a/cmd/output_contract_test.go b/cmd/output_contract_test.go new file mode 100644 index 0000000..9f68b77 --- /dev/null +++ b/cmd/output_contract_test.go @@ -0,0 +1,594 @@ +package cmd + +// Output contract tests. +// +// Each machine-facing document (status, list, elevation, env credentials, +// favorites list, access requests) gets ONE whole-object test that compares the +// emitted JSON against an inline literal via assertJSONEqual. Everything else +// about those outputs stays covered by focused tests. +// +// FIXTURE VALUES ARE DELIBERATELY ALL DIFFERENT AND SELF-DESCRIBING +// ("ws-name", "ws-id", "role-name", "role-id", "grp-id", "dir-id", +// "AKIA-fixture", "secret-fixture", "token-fixture"). A field swap — target +// with role, secret key with session token, groupId with directoryId — is +// invisible when both sides hold "test" or "", so these tests only detect one +// if every value is unique. Do not "tidy" them into shared constants or +// realistic-looking duplicates. + +import ( + "context" + "encoding/json" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/aaearon/grant-cli/internal/cache" + "github.com/aaearon/grant-cli/internal/config" + scamodels "github.com/aaearon/grant-cli/internal/sca/models" + wfmodels "github.com/aaearon/grant-cli/internal/workflows/models" + authmodels "github.com/cyberark/idsec-sdk-golang/pkg/models/auth" +) + +// --- status ----------------------------------------------------------------- + +// pinnedRemainingSeconds is the value substituted for the wall-clock-derived +// remainingSeconds field before a whole-object comparison. +const pinnedRemainingSeconds = 2700 + +// pinRemainingSeconds range-checks every sessions[].remainingSeconds and +// rewrites it to pinnedRemainingSeconds. The field is computed from time.Now() +// and therefore cannot appear verbatim in a literal; pinning it keeps the rest +// of the document — including whether the field is present at all — under the +// whole-object comparison. +func pinRemainingSeconds(t *testing.T, raw []byte, minSecs, maxSecs int) []byte { + t.Helper() + + var doc map[string]interface{} + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("status output is not valid JSON: %v\nraw:\n%s", err, raw) + } + sessions, ok := doc["sessions"].([]interface{}) + if !ok { + t.Fatalf("status output has no sessions array:\n%s", raw) + } + for _, s := range sessions { + session, ok := s.(map[string]interface{}) + if !ok { + t.Fatalf("session entry is not an object:\n%s", raw) + } + v, ok := session["remainingSeconds"] + if !ok { + continue + } + secs, ok := v.(float64) + if !ok { + t.Fatalf("remainingSeconds is not a number: %#v", v) + } + if int(secs) < minSecs || int(secs) > maxSecs { + t.Errorf("remainingSeconds = %d, want between %d and %d", int(secs), minSecs, maxSecs) + } + session["remainingSeconds"] = pinnedRemainingSeconds + } + + pinned, err := json.Marshal(doc) + if err != nil { + t.Fatalf("re-marshal failed: %v", err) + } + return pinned +} + +// TestStatusJSON_Contract pins the whole `grant status --output json` document +// for a cloud session and a group session at once. Six independent mutations in +// writeStatusJSON survived before it existed: provider case, workspaceId, +// duration, roleId, the workspace-name lookup and the group/cloud type tag. +func TestStatusJSON_Contract(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + + sessions := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{ + { + // CSP is mixed case on purpose: strings.ToLower, strings.ToUpper + // and the raw value are three different strings only if it is. + SessionID: "sess-cloud", CSP: "Azure", + WorkspaceID: "ws-id", RoleID: "role-id", SessionDuration: 3600, + }, + { + SessionID: "sess-group", CSP: "Azure", + WorkspaceID: "dir-ws-id", SessionDuration: 1800, + Target: &scamodels.SessionTarget{ID: "grp-id", Type: scamodels.TargetTypeGroups}, + }, + }, + Total: 2, + }} + + // Names differ from the IDs that key them, so dropping the lookup is visible. + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{ + {WorkspaceID: "ws-id", WorkspaceName: "ws-name"}, + {WorkspaceID: "dir-ws-id", WorkspaceName: "dir-ws-name"}, + }, + Total: 2, + }} + groupsElig := &mockGroupsEligibilityLister{response: &scamodels.GroupsEligibilityResponse{ + Response: []scamodels.GroupsEligibleTarget{ + {GroupID: "grp-id", GroupName: "grp-name", DirectoryID: "dir-id"}, + }, + Total: 1, + }} + + tracker := cache.NewStore(t.TempDir(), 25*time.Hour) + if err := cache.RecordSession(tracker, "sess-cloud", time.Now().Add(-15*time.Minute)); err != nil { + t.Fatalf("RecordSession() error = %v", err) + } + + cmd := NewStatusCommandWithDeps(auth, sessions, elig, groupsElig, tracker) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "status", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + got := pinRemainingSeconds(t, []byte(stdout), 2600, pinnedRemainingSeconds) + + assertJSONEqual(t, got, `{ + "authenticated": true, + "username": "user-fixture@example.test", + "sessions": [ + { + "sessionId": "sess-cloud", + "provider": "azure", + "workspaceId": "ws-id", + "workspaceName": "ws-name", + "roleId": "role-id", + "duration": 3600, + "remainingSeconds": 2700, + "type": "cloud" + }, + { + "sessionId": "sess-group", + "provider": "azure", + "workspaceId": "dir-ws-id", + "workspaceName": "dir-ws-name", + "duration": 1800, + "type": "group", + "groupId": "grp-id", + "groupName": "grp-name" + } + ] +}`) +} + +// TestStatus_DirectoryNameMergePrecedence pins the precedence rule in runStatus: +// a workspace name that came from eligibility wins over the directory-name +// fallback for the same key. Making the merge unconditional reverses it. +func TestStatus_DirectoryNameMergePrecedence(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + + sessions := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{ + {SessionID: "sess-collide", CSP: scamodels.CSPAzure, WorkspaceID: "ws-collide", RoleID: "role-id", SessionDuration: 3600}, + }, + Total: 1, + }} + + // The first entry teaches fetchStatusData ws-collide -> elig-name. + // The second makes buildDirectoryNameMap produce ws-collide -> dir-fallback-name + // for the very same key, via the organizationId fallback pass. + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{ + {WorkspaceID: "ws-collide", WorkspaceName: "elig-name"}, + {WorkspaceID: "ws-other", WorkspaceName: "dir-fallback-name", OrganizationID: "ws-collide"}, + }, + Total: 2, + }} + + cmd := NewStatusCommandWithDeps(auth, sessions, elig, nil, nil) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "status", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + var out statusOutput + if err := json.Unmarshal([]byte(stdout), &out); err != nil { + t.Fatalf("invalid JSON: %v\n%s", err, stdout) + } + if len(out.Sessions) != 1 { + t.Fatalf("expected 1 session, got %d", len(out.Sessions)) + } + if out.Sessions[0].WorkspaceName != "elig-name" { + t.Errorf("workspaceName = %q, want elig-name (eligibility must win over the directory fallback)", out.Sessions[0].WorkspaceName) + } +} + +// TestStatus_CleansUpStaleSessionTimestamps pins the lazy cleanup call in +// runStatus: timestamps for sessions the API no longer reports are dropped. +func TestStatus_CleansUpStaleSessionTimestamps(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + + sessions := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{ + {SessionID: "sess-active", CSP: scamodels.CSPAzure, WorkspaceID: "ws-id", RoleID: "role-id", SessionDuration: 3600}, + }, + Total: 1, + }} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{}} + + tracker := cache.NewStore(t.TempDir(), 25*time.Hour) + now := time.Now() + if err := cache.RecordSession(tracker, "sess-active", now.Add(-10*time.Minute)); err != nil { + t.Fatalf("RecordSession() error = %v", err) + } + if err := cache.RecordSession(tracker, "sess-gone", now.Add(-10*time.Minute)); err != nil { + t.Fatalf("RecordSession() error = %v", err) + } + + cmd := NewStatusCommandWithDeps(auth, sessions, elig, nil, tracker) + if _, err := executeCommand(cmd); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + remaining := cache.SessionTimestamps(tracker) + if _, ok := remaining["sess-gone"]; ok { + t.Error("timestamp for an inactive session survived; status must call cache.CleanupSessions") + } + if _, ok := remaining["sess-active"]; !ok { + t.Error("timestamp for the active session was removed") + } +} + +// --- list ------------------------------------------------------------------- + +// listContractFixture is the eligibility used by the list contract and +// round-trip tests. workspaceId, organizationId, name, role name and role id +// are five distinct strings so sourcing a field from the wrong one is visible. +func listContractFixture() []scamodels.EligibleTarget { + return []scamodels.EligibleTarget{{ + CSP: scamodels.CSPAzure, + OrganizationID: "org-id", + WorkspaceID: "ws-id", + WorkspaceName: "ws-name", + WorkspaceType: scamodels.WorkspaceTypeSubscription, + RoleInfo: scamodels.RoleInfo{ID: "role-id", Name: "role-name"}, + }} +} + +func listContractGroups() []scamodels.GroupsEligibleTarget { + return []scamodels.GroupsEligibleTarget{{ + GroupName: "grp-name", + GroupID: "grp-id", + DirectoryID: "dir-id", + DirectoryName: "dir-name", + }} +} + +// TestListJSON_Contract pins the whole `grant list --output json` document. +// Sourcing workspaceId from organizationId, or blanking workspaceType, roleId, +// groupId or directoryId, all survived before it existed. +func TestListJSON_Contract(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + targets := listContractFixture() + elig := &mockEligibilityLister{ + listFunc: func(_ context.Context, csp scamodels.CSP) (*scamodels.EligibilityResponse, error) { + if csp == scamodels.CSPAzure { + return &scamodels.EligibilityResponse{Response: targets, Total: len(targets)}, nil + } + return &scamodels.EligibilityResponse{}, nil + }, + } + groupsElig := &mockGroupsEligibilityLister{response: &scamodels.GroupsEligibilityResponse{ + Response: listContractGroups(), Total: 1, + }} + + cmd := NewListCommandWithDeps(auth, elig, groupsElig) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "list", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "cloud": [ + { + "provider": "azure", + "target": "ws-name", + "workspaceId": "ws-id", + "workspaceType": "subscription", + "role": "role-name", + "roleId": "role-id" + } + ], + "groups": [ + { + "groupName": "grp-name", + "groupId": "grp-id", + "directoryId": "dir-id", + "directory": "dir-name" + } + ] +}`) +} + +// TestListJSON_RoundTripsToRequestSubmit is the reusability guarantee an LLM or +// script depends on: values emitted by `grant list -o json` must feed straight +// back into the flags that consume them. +// +// Note which field goes where — a verifier corrected this. `--target` resolves +// on the emitted NAME (`target`), not on `workspaceId`; `roleId` is the value +// `--role-id` takes verbatim. +func TestListJSON_RoundTripsToRequestSubmit(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + targets := listContractFixture() + elig := &mockEligibilityLister{ + listFunc: func(_ context.Context, csp scamodels.CSP) (*scamodels.EligibilityResponse, error) { + if csp == scamodels.CSPAzure { + return &scamodels.EligibilityResponse{Response: targets, Total: len(targets)}, nil + } + return &scamodels.EligibilityResponse{}, nil + }, + } + + listCmd := NewListCommandWithDeps(auth, elig, &mockGroupsEligibilityLister{listErr: errNotAuthenticated}) + listRoot := newTestRootCommand() + listRoot.AddCommand(listCmd) + + stdout, stderr, err := executeCommandStreams(listRoot, "list", "--output", "json") + if err != nil { + t.Fatalf("list failed: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + var listed listOutput + if err := json.Unmarshal([]byte(stdout), &listed); err != nil { + t.Fatalf("invalid JSON: %v\n%s", err, stdout) + } + if len(listed.Cloud) != 1 { + t.Fatalf("expected 1 cloud target, got %d", len(listed.Cloud)) + } + emitted := listed.Cloud[0] + + // 1. The root command's --target/--role path, through the production matcher. + resolved := findMatchingTarget(targets, emitted.Target, emitted.Role) + if resolved == nil { + t.Fatalf("findMatchingTarget(%q, %q) = nil; the emitted target/role do not resolve back", emitted.Target, emitted.Role) + } + if resolved.WorkspaceID != emitted.WorkspaceID { + t.Errorf("resolved workspaceId = %q, emitted %q", resolved.WorkspaceID, emitted.WorkspaceID) + } + if resolved.RoleInfo.ID != emitted.RoleID { + t.Errorf("resolved roleId = %q, emitted %q", resolved.RoleInfo.ID, emitted.RoleID) + } + + // 2. `grant request submit --target --role-id `. + // The workspace resolver is stubbed (it bootstraps live SCA auth), but it + // matches the emitted NAME over the production deduplicateWorkspaces + // output, exactly as resolveSubmitTarget does. + origResolve := resolveSubmitTargetFn + t.Cleanup(func() { resolveSubmitTargetFn = origResolve }) + resolveSubmitTargetFn = func(_ context.Context, _, targetName string, _ bool) (*submitWorkspace, error) { + workspaces := deduplicateWorkspaces(targets) + for i := range workspaces { + if strings.EqualFold(workspaces[i].WorkspaceName, targetName) { + return &workspaces[i], nil + } + } + t.Errorf("emitted target %q matched no eligible workspace", targetName) + return nil, errNotAuthenticated + } + + svc := &mockAccessRequestService{submitResult: &wfmodels.AccessRequest{ + RequestID: "req-id", RequestState: wfmodels.RequestStatePending, + }} + submitRoot := newTestRootCommand() + submitRoot.AddCommand(NewRequestCommandWithDeps(svc)) + + out, err := executeCommand(submitRoot, "request", "submit", + "--target", emitted.Target, "--role-id", emitted.RoleID, "--role", emitted.Role, + "--reason", "reason-fixture", "--date", "2026-04-21", + "--timezone", "UTC", "--from", "09:00", "--to", "17:00", "--yes") + if err != nil { + t.Fatalf("submit failed: %v\noutput: %s", err, out) + } + + submitted := svc.lastSubmit() + if submitted == nil { + t.Fatal("SubmitRequest was never called") + } + for _, tc := range []struct{ key, want string }{ + {"workspaceId", emitted.WorkspaceID}, + {"workspaceName", emitted.Target}, + {"roleId", emitted.RoleID}, + {"roleName", emitted.Role}, + } { + if got, _ := submitted.RequestDetails[tc.key].(string); got != tc.want { + t.Errorf("submitted %s = %q, want %q", tc.key, got, tc.want) + } + } +} + +// --- elevation -------------------------------------------------------------- + +// awsCredsFixture is the accessCredentials payload used by the elevation and +// env contract tests. The three values are distinct so swapping the secret key +// with the session token is detectable. +const awsCredsFixture = `{"aws_access_key":"AKIA-fixture","aws_secret_access_key":"secret-fixture","aws_session_token":"token-fixture"}` + +func awsElevationTarget() *scamodels.EligibleTarget { + return &scamodels.EligibleTarget{ + CSP: scamodels.CSPAWS, + OrganizationID: "org-id", + WorkspaceID: "ws-id", + WorkspaceName: "ws-name", + WorkspaceType: scamodels.WorkspaceTypeAccount, + RoleInfo: scamodels.RoleInfo{ID: "role-id", Name: "role-name"}, + } +} + +// TestElevationJSON_Contract pins the whole cloud-elevation document, including +// the AWS credential block. Swapping target with role, dropping the provider +// lowercasing, and swapping secretAccessKey with sessionToken all survived. +func TestElevationJSON_Contract(t *testing.T) { + creds := awsCredsFixture + target := awsElevationTarget() + + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{*target}, Total: 1, + }} + elev := &mockElevateService{response: &scamodels.ElevateResponse{Response: scamodels.ElevateAccessResult{ + CSP: scamodels.CSPAWS, OrganizationID: "org-id", + Results: []scamodels.ElevateTargetResult{{ + WorkspaceID: "ws-id", RoleID: "role-id", SessionID: "sess-id", + AccessCredentials: &creds, + }}, + }}} + sel := &mockUnifiedSelector{item: &selectionItem{kind: selectionCloud, cloud: target}} + + cmd := NewRootCommandWithDeps(nil, auth, elig, elev, sel, + &mockGroupsEligibilityLister{listErr: errNotAuthenticated}, nil, config.DefaultConfig()) + + stdout, stderr, err := executeCommandStreams(cmd, "--output", "json", "--provider", "aws") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "type": "cloud", + "provider": "aws", + "sessionId": "sess-id", + "target": "ws-name", + "role": "role-name", + "credentials": { + "accessKeyId": "AKIA-fixture", + "secretAccessKey": "secret-fixture", + "sessionToken": "token-fixture" + } +}`) +} + +// TestGroupElevationJSON_Contract pins the whole group-elevation document. +// Swapping groupId with directoryId survived before it existed. +func TestGroupElevationJSON_Contract(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{}} + groupsElig := &mockGroupsEligibilityLister{response: &scamodels.GroupsEligibilityResponse{ + Response: []scamodels.GroupsEligibleTarget{{ + GroupName: "grp-name", GroupID: "grp-id", DirectoryID: "dir-id", DirectoryName: "dir-name", + }}, + Total: 1, + }} + groupsElev := &mockGroupsElevator{response: &scamodels.GroupsElevateResponse{ + DirectoryID: "dir-id", CSP: scamodels.CSPAzure, + Results: []scamodels.GroupsElevateTargetResult{{GroupID: "grp-id", SessionID: "sess-id"}}, + }} + + cmd := NewRootCommandWithDeps(nil, auth, elig, nil, nil, groupsElig, groupsElev, config.DefaultConfig()) + + stdout, stderr, err := executeCommandStreams(cmd, "--output", "json", "--group", "grp-name") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "type": "group", + "sessionId": "sess-id", + "groupName": "grp-name", + "groupId": "grp-id", + "directoryId": "dir-id", + "directory": "dir-name" +}`) +} + +// TestEnvJSON_Contract pins `grant env --output json`. The identical swap in the +// text export path is already caught by TestEnvCommand_AWSSuccess; the JSON +// document carrying the same three secrets was unpinned. +func TestEnvJSON_Contract(t *testing.T) { + creds := awsCredsFixture + target := awsElevationTarget() + + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{*target}, Total: 1, + }} + elev := &mockElevateService{response: &scamodels.ElevateResponse{Response: scamodels.ElevateAccessResult{ + CSP: scamodels.CSPAWS, OrganizationID: "org-id", + Results: []scamodels.ElevateTargetResult{{ + WorkspaceID: "ws-id", RoleID: "role-id", SessionID: "sess-id", + AccessCredentials: &creds, + }}, + }}} + sel := &mockTargetSelector{target: target} + + cmd := NewEnvCommandWithDeps(nil, auth, elig, elev, sel, config.DefaultConfig()) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "env", + "--provider", "aws", "--target", "ws-name", "--role", "role-name", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "accessKeyId": "AKIA-fixture", + "secretAccessKey": "secret-fixture", + "sessionToken": "token-fixture" +}`) +} + +// --- favorites -------------------------------------------------------------- + +// TestFavoritesListJSON_Contract pins the whole `grant favorites list -o json` +// array. Blanking provider, role or directoryId all survived: the previous test +// switched on Name and asserted only type, target and group. +func TestFavoritesListJSON_Contract(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + + cfg := config.DefaultConfig() + if err := config.AddFavorite(cfg, "fav-cloud", config.Favorite{ + Provider: "aws", Target: "ws-name", Role: "role-name", + }); err != nil { + t.Fatalf("AddFavorite() error = %v", err) + } + if err := config.AddFavorite(cfg, "fav-group", config.Favorite{ + Type: config.FavoriteTypeGroups, Provider: "azure", Group: "grp-name", DirectoryID: "dir-id", + }); err != nil { + t.Fatalf("AddFavorite() error = %v", err) + } + if err := config.Save(cfg, configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + root := newTestRootCommand() + root.AddCommand(NewFavoritesCommand()) + + stdout, stderr, err := executeCommandStreams(root, "favorites", "list", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + // ListFavorites sorts by name, so the order is deterministic. + assertJSONEqual(t, []byte(stdout), `[ + { + "name": "fav-cloud", + "type": "cloud", + "provider": "aws", + "target": "ws-name", + "role": "role-name" + }, + { + "name": "fav-group", + "type": "groups", + "provider": "azure", + "group": "grp-name", + "directoryId": "dir-id" + } +]`) +} diff --git a/cmd/test_helpers_test.go b/cmd/test_helpers_test.go index 38d0c1c..58d78bc 100644 --- a/cmd/test_helpers_test.go +++ b/cmd/test_helpers_test.go @@ -4,13 +4,46 @@ package cmd import ( "bytes" + "encoding/json" "os" + "reflect" "testing" "github.com/aaearon/grant-cli/internal/ui" "github.com/spf13/cobra" ) +// assertJSONEqual compares a machine-facing JSON document against an inline +// literal, structurally: both sides are unmarshalled into interface{} so key +// order and indentation are irrelevant, and compared with reflect.DeepEqual. +// +// It is deliberately whole-object. A field added to an output struct breaks +// every contract test that covers it, which is the point — the machine-facing +// documents are a compatibility surface, and adding to one should require a +// conscious decision rather than passing silently. Use it once per output; +// keep focused assertions for conditional and optional fields. +func assertJSONEqual(t *testing.T, got []byte, wantJSON string) { + t.Helper() + + var gotDoc, wantDoc interface{} + if err := json.Unmarshal(got, &gotDoc); err != nil { + t.Fatalf("got is not valid JSON: %v\nraw:\n%s", err, got) + } + if err := json.Unmarshal([]byte(wantJSON), &wantDoc); err != nil { + t.Fatalf("want is not valid JSON: %v\nraw:\n%s", err, wantJSON) + } + + if reflect.DeepEqual(gotDoc, wantDoc) { + return + } + + // Re-marshal both through the same encoder so the printed diff differs + // only where the documents actually differ (MarshalIndent sorts keys). + gotPretty, _ := json.MarshalIndent(gotDoc, "", " ") + wantPretty, _ := json.MarshalIndent(wantDoc, "", " ") + t.Errorf("JSON contract mismatch\n--- got ---\n%s\n--- want ---\n%s", gotPretty, wantPretty) +} + // withInteractiveTTY forces ui.IsInteractive() to the given answer for the // duration of the test, restoring the package global via t.Cleanup. // From 13e59fccb99514646119322e0118156e7d82da12 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:30:41 +0200 Subject: [PATCH 2/7] test(cmd): pin favorites persistence, list flags and status merges - favorites: DirectoryID is persisted on both the unified-selector and the --type groups path, verified by resolving the saved favorite back through findMatchingGroup against two same-named groups in different directories - favorites: non-default default_provider, --provider precedence over the target CSP, parseFavoritesAddFlags validation, remove arity - list: assert Cobra's actual mutual-exclusion error (the old test passed on an unrelated runtime error), --provider suppresses groups, --refresh - status: directory-name merge precedence, stale session-timestamp cleanup, and an exact "remaining: 45m" instead of a prefix a sixfold error satisfied - request: text and JSON field mappings (target/role columns, created vs updated attribution, timeFrom/timeTo) --- cmd/favorites_persistence_test.go | 280 ++++++++++++++++++++++++++++++ cmd/list_test.go | 90 ++++++++++ cmd/request_output_test.go | 157 +++++++++++++++++ cmd/status_test.go | 12 +- 4 files changed, 535 insertions(+), 4 deletions(-) create mode 100644 cmd/favorites_persistence_test.go create mode 100644 cmd/request_output_test.go diff --git a/cmd/favorites_persistence_test.go b/cmd/favorites_persistence_test.go new file mode 100644 index 0000000..fea6696 --- /dev/null +++ b/cmd/favorites_persistence_test.go @@ -0,0 +1,280 @@ +package cmd + +// Favorites persistence and flag-validation coverage. +// +// Fixture values are deliberately distinct and self-describing; see the header +// of output_contract_test.go for why that is mandatory rather than cosmetic. + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/aaearon/grant-cli/internal/config" + "github.com/aaearon/grant-cli/internal/sca/models" +) + +// twoGroupsSameName is the fixture for the DirectoryID persistence tests: two +// groups with the SAME display name in DIFFERENT directories. The names must +// collide, otherwise findMatchingGroup resolves on the name alone and the +// directory ID is never load-bearing. +func twoGroupsSameName() []models.GroupsEligibleTarget { + return []models.GroupsEligibleTarget{ + {GroupName: "grp-name", GroupID: "grp-id-a", DirectoryID: "dir-id-a"}, + {GroupName: "grp-name", GroupID: "grp-id-b", DirectoryID: "dir-id-b"}, + } +} + +// assertFavoriteResolvesToGroup reloads the saved favorite and pushes it back +// through findMatchingGroup — the production lookup behind `grant --favorite`. +// The round trip is what makes dropping `fav.DirectoryID = ...` fail: without +// it the favorite still names the right group, but resolution silently picks +// the first same-named group, in the wrong directory. +func assertFavoriteResolvesToGroup(t *testing.T, configPath, favName string, groups []models.GroupsEligibleTarget, wantDirectoryID, wantGroupID string) { + t.Helper() + + reloaded, err := config.Load(configPath) + if err != nil { + t.Fatalf("reload config failed: %v", err) + } + fav, err := config.GetFavorite(reloaded, favName) + if err != nil { + t.Fatalf("favorite not found: %v", err) + } + if fav.DirectoryID != wantDirectoryID { + t.Errorf("persisted DirectoryID = %q, want %q", fav.DirectoryID, wantDirectoryID) + } + + match := findMatchingGroup(groups, fav.Group, fav.DirectoryID) + if match == nil { + t.Fatalf("findMatchingGroup(%q, %q) = nil", fav.Group, fav.DirectoryID) + } + if match.GroupID != wantGroupID { + t.Errorf("favorite resolved to group %q, want %q", match.GroupID, wantGroupID) + } +} + +// TestFavoritesAddInteractive_PersistsDirectoryID covers the unified-selector +// path (selectFavoriteInteractive). +func TestFavoritesAddInteractive_PersistsDirectoryID(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + if err := config.Save(config.DefaultConfig(), configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + groups := twoGroupsSameName() + eligLister := &mockEligibilityLister{response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{{ + WorkspaceID: "ws-id", WorkspaceName: "ws-name", + WorkspaceType: models.WorkspaceTypeSubscription, + RoleInfo: models.RoleInfo{ID: "role-id", Name: "role-name"}, + }}, + Total: 1, + }} + groupsElig := &mockGroupsEligibilityLister{response: &models.GroupsEligibilityResponse{ + Response: groups, Total: len(groups), + }} + // Select the SECOND group, so a dropped DirectoryID resolves to the first. + sel := &mockUnifiedSelector{item: &selectionItem{kind: selectionGroup, group: &groups[1]}} + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommandWithAllDeps(eligLister, sel, &mockNamePrompter{}, groupsElig)) + + if out, err := executeCommand(rootCmd, "favorites", "add", "fav-group"); err != nil { + t.Fatalf("add favorite failed: %v\noutput: %s", err, out) + } + + assertFavoriteResolvesToGroup(t, configPath, "fav-group", twoGroupsSameName(), "dir-id-b", "grp-id-b") +} + +// TestAddGroupFavorite_PersistsDirectoryID covers the `--type groups` selector +// path (addGroupFavorite), which copies the directory ID independently. +func TestAddGroupFavorite_PersistsDirectoryID(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + if err := config.Save(config.DefaultConfig(), configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + groups := twoGroupsSameName() + groupsElig := &mockGroupsEligibilityLister{response: &models.GroupsEligibilityResponse{ + Response: groups, Total: len(groups), + }} + sel := &mockUnifiedSelector{item: &selectionItem{kind: selectionGroup, group: &groups[1]}} + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommandWithAllDeps(nil, sel, &mockNamePrompter{}, groupsElig)) + + if out, err := executeCommand(rootCmd, "favorites", "add", "fav-group", "--type", "groups"); err != nil { + t.Fatalf("add group favorite failed: %v\noutput: %s", err, out) + } + + assertFavoriteResolvesToGroup(t, configPath, "fav-group", twoGroupsSameName(), "dir-id-b", "grp-id-b") +} + +// TestFavoritesAdd_HonorsNonDefaultProvider pins that a providerless +// `favorites add --target/--role` takes its provider from +// config.default_provider. Every other test uses DefaultConfig(), whose default +// already IS "azure", so a hardcoded "azure" was indistinguishable from +// reading the field. +func TestFavoritesAdd_HonorsNonDefaultProvider(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + + cfg := config.DefaultConfig() + cfg.DefaultProvider = "aws" + if err := config.Save(cfg, configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommand()) + + out, err := executeCommand(rootCmd, "favorites", "add", "fav-cloud", "--target", "ws-name", "--role", "role-name") + if err != nil { + t.Fatalf("add favorite failed: %v\noutput: %s", err, out) + } + + reloaded, err := config.Load(configPath) + if err != nil { + t.Fatalf("reload config failed: %v", err) + } + fav, err := config.GetFavorite(reloaded, "fav-cloud") + if err != nil { + t.Fatalf("favorite not found: %v", err) + } + if fav.Provider != "aws" { + t.Errorf("Provider = %q, want aws (from config.default_provider)", fav.Provider) + } +} + +// TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP pins the precedence +// in selectFavoriteInteractive: an explicit --provider is stored verbatim +// rather than derived from the selected target's CSP. The fixture's CSP is aws +// while the flag says azure — with both azure the two branches are identical. +func TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + if err := config.Save(config.DefaultConfig(), configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + selected := models.EligibleTarget{ + CSP: models.CSPAWS, + WorkspaceID: "ws-id", WorkspaceName: "ws-name", + WorkspaceType: models.WorkspaceTypeAccount, + RoleInfo: models.RoleInfo{ID: "role-id", Name: "role-name"}, + } + eligLister := &mockEligibilityLister{response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{selected}, Total: 1, + }} + sel := &mockUnifiedSelector{item: &selectionItem{kind: selectionCloud, cloud: &selected}} + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommandWithAllDeps(eligLister, sel, &mockNamePrompter{}, nil)) + + out, err := executeCommand(rootCmd, "favorites", "add", "fav-cloud", "--provider", "azure") + if err != nil { + t.Fatalf("add favorite failed: %v\noutput: %s", err, out) + } + + reloaded, err := config.Load(configPath) + if err != nil { + t.Fatalf("reload config failed: %v", err) + } + fav, err := config.GetFavorite(reloaded, "fav-cloud") + if err != nil { + t.Fatalf("favorite not found: %v", err) + } + if fav.Provider != "azure" { + t.Errorf("Provider = %q, want azure (the --provider flag, not the target CSP)", fav.Provider) + } +} + +// TestParseFavoritesAddFlags_Validation exercises parseFavoritesAddFlags +// directly. runFavoritesAddProduction repeats two of these checks, so the +// command-level tests keep passing when this copy is deleted — every DI caller +// of runFavoritesAddWithDeps would lose the validation silently. +func TestParseFavoritesAddFlags_Validation(t *testing.T) { + tests := []struct { + name string + args []string + // wantErrContains empty means the parse must succeed. + wantErrContains string + }{ + {name: "cloud target and role", args: []string{"--target", "ws-name", "--role", "role-name"}}, + {name: "groups with group", args: []string{"--type", "groups", "--group", "grp-name"}}, + {name: "groups with target", args: []string{"--type", "groups", "--target", "ws-name"}, wantErrContains: "--target and --role cannot be used with --type groups"}, + {name: "groups with role", args: []string{"--type", "groups", "--role", "role-name"}, wantErrContains: "--target and --role cannot be used with --type groups"}, + {name: "group without type groups", args: []string{"--group", "grp-name"}, wantErrContains: "--group requires --type groups"}, + {name: "target without role", args: []string{"--target", "ws-name"}, wantErrContains: "both --target and --role must be provided"}, + {name: "role without target", args: []string{"--role", "role-name"}, wantErrContains: "both --target and --role must be provided"}, + {name: "invalid type", args: []string{"--type", "bogus"}, wantErrContains: `invalid --type "bogus"`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := newFavoritesAddCommandWithRunner(nil) + if err := cmd.ParseFlags(tt.args); err != nil { + t.Fatalf("ParseFlags() error = %v", err) + } + + f, err := parseFavoritesAddFlags(cmd) + if tt.wantErrContains == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if f == nil { + t.Fatal("expected parsed flags, got nil") + } + return + } + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.wantErrContains) + } + if !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %v, want it to contain %q", err, tt.wantErrContains) + } + }) + } +} + +// TestFavoritesRemove_RejectsExtraArgs pins the arity check: without it, +// `grant favorites remove first second` silently removes only "first". +func TestFavoritesRemove_RejectsExtraArgs(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv("GRANT_CONFIG", configPath) + + cfg := config.DefaultConfig() + if err := config.AddFavorite(cfg, "fav-first", config.Favorite{Provider: "azure", Target: "ws-name", Role: "role-name"}); err != nil { + t.Fatalf("AddFavorite() error = %v", err) + } + if err := config.AddFavorite(cfg, "fav-second", config.Favorite{Provider: "aws", Target: "ws-name-2", Role: "role-name-2"}); err != nil { + t.Fatalf("AddFavorite() error = %v", err) + } + if err := config.Save(cfg, configPath); err != nil { + t.Fatalf("Save() error = %v", err) + } + + rootCmd := newTestRootCommand() + rootCmd.AddCommand(NewFavoritesCommand()) + + _, err := executeCommand(rootCmd, "favorites", "remove", "fav-first", "fav-second") + if err == nil { + t.Fatal("expected an error for two favorite names") + } + if !strings.Contains(err.Error(), "expected 1 favorite name, got 2") { + t.Errorf("error = %v, want the arity message", err) + } + + reloaded, err := config.Load(configPath) + if err != nil { + t.Fatalf("reload config failed: %v", err) + } + for _, name := range []string{"fav-first", "fav-second"} { + if _, err := config.GetFavorite(reloaded, name); err != nil { + t.Errorf("favorite %q was removed despite the rejected command", name) + } + } +} diff --git a/cmd/list_test.go b/cmd/list_test.go index 29e99d3..1316e36 100644 --- a/cmd/list_test.go +++ b/cmd/list_test.go @@ -268,6 +268,96 @@ func TestListCommand_MutualExclusivity(t *testing.T) { if err == nil { t.Fatal("expected error for --groups + --provider") } + // Assert Cobra's own mutual-exclusion text. Merely requiring "an error" + // passed even with MarkFlagsMutuallyExclusive deleted, because the command + // then ran and failed with "no eligible targets or groups found" instead. + if !strings.Contains(err.Error(), "[groups provider] were all set") { + t.Errorf("expected Cobra's mutual-exclusion error, got: %v", err) + } +} + +// TestListCommand_ProviderSuppressesGroups pins that --provider restricts the +// output to cloud targets: groups are Azure-only and a provider filter has no +// meaning for them, so they are not fetched at all. +func TestListCommand_ProviderSuppressesGroups(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + eligLister := &mockEligibilityLister{response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{{ + WorkspaceID: "ws-id", WorkspaceName: "ws-name", + WorkspaceType: models.WorkspaceTypeSubscription, + RoleInfo: models.RoleInfo{ID: "role-id", Name: "role-name"}, + }}, + Total: 1, + }} + groupsCalled := false + groupsElig := &mockGroupsEligibilityLister{ + listFunc: func(_ context.Context, _ models.CSP) (*models.GroupsEligibilityResponse, error) { + groupsCalled = true + return &models.GroupsEligibilityResponse{ + Response: []models.GroupsEligibleTarget{{GroupID: "grp-id", GroupName: "grp-name", DirectoryID: "dir-id"}}, + Total: 1, + }, nil + }, + } + + cmd := NewListCommandWithDeps(auth, eligLister, groupsElig) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "list", "--provider", "azure", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + var parsed listOutput + if err := json.Unmarshal([]byte(stdout), &parsed); err != nil { + t.Fatalf("invalid JSON: %v\n%s", err, stdout) + } + if len(parsed.Groups) != 0 { + t.Errorf("expected no groups with --provider, got %d", len(parsed.Groups)) + } + if groupsCalled { + t.Error("groups eligibility must not be fetched when --provider is set") + } +} + +// TestListCommand_RefreshFlagRegistered pins the --refresh flag on `grant list`. +// The flag is not a no-op: NewListCommand reads it and passes it into +// buildCachedLister. That wiring runs behind bootstrapSCAService, which unit +// tests cannot reach, so this covers registration and parsing only. +func TestListCommand_RefreshFlagRegistered(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + eligLister := &mockEligibilityLister{response: &models.EligibilityResponse{ + Response: []models.EligibleTarget{{ + WorkspaceID: "ws-id", WorkspaceName: "ws-name", + WorkspaceType: models.WorkspaceTypeSubscription, + RoleInfo: models.RoleInfo{ID: "role-id", Name: "role-name"}, + }}, + Total: 1, + }} + groupsElig := &mockGroupsEligibilityLister{listErr: errors.New("skip groups")} + + cmd := NewListCommandWithDeps(auth, eligLister, groupsElig) + root := newTestRootCommand() + root.AddCommand(cmd) + + if _, err := executeCommand(root, "list", "--refresh"); err != nil { + t.Fatalf("grant list --refresh must be accepted: %v", err) + } + + refresh, err := cmd.Flags().GetBool("refresh") + if err != nil { + t.Fatalf("--refresh is not registered: %v", err) + } + if !refresh { + t.Error("--refresh parsed as false") + } + + // And it defaults to off on a fresh command. + fresh := NewListCommandWithDeps(auth, eligLister, groupsElig) + if def, err := fresh.Flags().GetBool("refresh"); err != nil || def { + t.Errorf("--refresh default = %v (err %v), want false", def, err) + } } // TestListCommand_JSONOutputSingleProvider guards the regression where diff --git a/cmd/request_output_test.go b/cmd/request_output_test.go new file mode 100644 index 0000000..9f266e7 --- /dev/null +++ b/cmd/request_output_test.go @@ -0,0 +1,157 @@ +package cmd + +// Access-request output field mappings. +// +// Fixture values are deliberately distinct and self-describing; see the header +// of output_contract_test.go for why that is mandatory rather than cosmetic. + +import ( + "strings" + "testing" + + wfmodels "github.com/aaearon/grant-cli/internal/workflows/models" +) + +// requestFixture is one fully-populated access request. Every detail value is +// unique so a mapping swap (target with role, createdBy with updatedBy, +// timeFrom with timeTo) changes the rendered output. +func requestFixture() *wfmodels.AccessRequest { + return &wfmodels.AccessRequest{ + RequestID: "req-id", + TargetCategory: "CLOUD_CONSOLE", + RequestState: wfmodels.RequestStatePending, + RequestResult: wfmodels.RequestResultUnknown, + RequestLink: "https://example.test/req-id", + RequestDetails: map[string]interface{}{ + "locationType": "provider-fixture", + "workspaceName": "ws-name", + "roleName": "role-name", + "reason": "reason-fixture", + "priority": "priority-fixture", + "requestDate": "2026-04-21", + "timezone": "tz-fixture", + "timeFrom": "01:11", + "timeTo": "22:22", + }, + FinalizationReason: "finalization-fixture", + CreatedBy: "creator-fixture", + CreatedAt: "2026-04-20T10:00:00Z", + UpdatedBy: "updater-fixture", + UpdatedAt: "2026-04-21T11:00:00Z", + } +} + +// TestRequestList_TextFieldMapping pins the TARGET and ROLE columns of the +// `grant request list` table to their respective request details. Swapping the +// two Fprintf arguments produced an equally plausible-looking table. +func TestRequestList_TextFieldMapping(t *testing.T) { + svc := &mockAccessRequestService{ + listItems: []wfmodels.AccessRequest{*requestFixture()}, + listTotalCount: 1, + } + + root := newTestRootCommand() + root.AddCommand(NewRequestCommandWithDeps(svc)) + + out, err := executeCommand(root, "request", "list") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, out) + } + + var row string + for _, line := range strings.Split(out, "\n") { + if strings.HasPrefix(line, "req-id") { + row = line + break + } + } + if row == "" { + t.Fatalf("no data row for req-id in output:\n%s", out) + } + + // Columns: ID STATE RESULT TARGET ROLE PRIORITY CREATED BY CREATED AT. + // No fixture value contains a space, so field positions are unambiguous. + fields := strings.Fields(row) + if len(fields) < 6 { + t.Fatalf("row has %d columns, want at least 6: %q", len(fields), row) + } + if fields[3] != "ws-name" { + t.Errorf("TARGET column = %q, want ws-name", fields[3]) + } + if fields[4] != "role-name" { + t.Errorf("ROLE column = %q, want role-name", fields[4]) + } + if fields[5] != "priority-fixture" { + t.Errorf("PRIORITY column = %q, want priority-fixture", fields[5]) + } + if fields[6] != "creator-fixture" { + t.Errorf("CREATED BY column = %q, want creator-fixture", fields[6]) + } +} + +// TestRequestGet_TextFieldMapping pins the created/updated attribution in the +// `grant request get` detail view. Sourcing "Created By" from UpdatedBy +// survived: both fields were "user@test" in the existing fixtures. +func TestRequestGet_TextFieldMapping(t *testing.T) { + svc := &mockAccessRequestService{getResult: requestFixture()} + + root := newTestRootCommand() + root.AddCommand(NewRequestCommandWithDeps(svc)) + + out, err := executeCommand(root, "request", "get", "req-id") + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, out) + } + + for _, want := range []string{ + "Created By: creator-fixture", + "Updated By: updater-fixture", + "Created At: 2026-04-20T10:00:00Z", + "Updated At: 2026-04-21T11:00:00Z", + "Target: ws-name", + "Role: role-name", + "Time From: 01:11", + "Time To: 22:22", + } { + if !strings.Contains(out, want) { + t.Errorf("output missing %q, got:\n%s", want, out) + } + } +} + +// TestRequestGetJSON_FieldMapping is the whole-object contract for the access +// request document, shared by `request get`, `submit`, `cancel`, `approve` and +// `reject`. Swapping timeFrom with timeTo survived every existing test. +func TestRequestGetJSON_FieldMapping(t *testing.T) { + svc := &mockAccessRequestService{getResult: requestFixture()} + + root := newTestRootCommand() + root.AddCommand(NewRequestCommandWithDeps(svc)) + + stdout, stderr, err := executeCommandStreams(root, "request", "get", "req-id", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "requestId": "req-id", + "targetCategory": "CLOUD_CONSOLE", + "state": "PENDING", + "result": "UNKNOWN", + "priority": "priority-fixture", + "reason": "reason-fixture", + "provider": "provider-fixture", + "target": "ws-name", + "role": "role-name", + "requestDate": "2026-04-21", + "timezone": "tz-fixture", + "timeFrom": "01:11", + "timeTo": "22:22", + "finalizationReason": "finalization-fixture", + "requestLink": "https://example.test/req-id", + "createdBy": "creator-fixture", + "createdAt": "2026-04-20T10:00:00Z", + "updatedBy": "updater-fixture", + "updatedAt": "2026-04-21T11:00:00Z" +}`) +} diff --git a/cmd/status_test.go b/cmd/status_test.go index 4f29520..f3c8108 100644 --- a/cmd/status_test.go +++ b/cmd/status_test.go @@ -1007,9 +1007,13 @@ func TestStatusCommand_RemainingTime(t *testing.T) { }, } - // Create a tracker with a recorded session timestamp + // Create a tracker with a recorded session timestamp. + // 14m30s ago of a 1h session leaves 45m30s, which formats as "remaining: + // 45m" and stays there for the ~30s of slack before the truncated minute + // rolls over — so the text assertion below can be exact rather than a + // prefix. "remaining: 4" was satisfied by "44m", "45m" and "4h 30m" alike. tracker := cache.NewStore(t.TempDir(), 25*time.Hour) - elevatedAt := now.Add(-15 * time.Minute) // elevated 15 minutes ago + elevatedAt := now.Add(-14*time.Minute - 30*time.Second) if err := cache.RecordSession(tracker, "tracked-session", elevatedAt); err != nil { t.Fatalf("RecordSession() error = %v", err) } @@ -1024,8 +1028,8 @@ func TestStatusCommand_RemainingTime(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(output, "remaining: 4") { - t.Errorf("output should show remaining time, got:\n%s", output) + if !strings.Contains(output, "remaining: 45m") { + t.Errorf("output should show 'remaining: 45m', got:\n%s", output) } // Untracked session should show duration if !strings.Contains(output, "duration: 30m") { From 40167078a82dfe20f58dcc98173f860603296ac2 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:30:54 +0200 Subject: [PATCH 3/7] docs: close the PR5 rows in the mutation ledger All 32 PR5 rows plus OUT-27 reverified with -count=1: mutation applied, test fails, mutation reverted, test passes. Test-name and scope corrections noted in the rows where the plan's placeholder name or fixture did not survive contact (OUT-09/10 fold into the status contract test, OUT-23's fixture, and OUT-25, which covers flag registration rather than a cache bypass). Record the output-contract and distinct-fixture conventions in CLAUDE.md. --- CLAUDE.md | 2 ++ docs/mutation-ledger.md | 72 ++++++++++++++++++++--------------------- 2 files changed, 38 insertions(+), 36 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4941a45..ad668ec 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -90,6 +90,8 @@ Custom `SCAAccessService` follows SDK conventions: - **Mock capture convention** (`cmd/test_mocks_test.go`, precedent `mockSessionRevoker`): record the arguments in the method body *before* dispatching to any `xxxFunc` callback, keep a history slice plus a `lastX()` accessor (a history is what answers "called exactly once?"), defensively copy slices/maps and pointer-to-struct args, and guard against a nil request. An optional `*string` argument is flattened to `reason string` + `reasonSet bool` so a test can tell `nil` from `""`. There is exactly one mock per interface — an arg-blind sibling silently opts every future test out of capture - No mutex on those histories: the only mocks reached from more than one goroutine are the eligibility listers, via the fan-outs in `fetchEligibility`/`fetchGroupsEligibility` (`cmd/root.go`), `resolveAndElevateUnifiedPath` (`cmd/root.go`) and `fetchAllTargets`/`fetchAllGroups` (`cmd/helpers.go`), and those mocks are stateless readers. `Elevate`, `ElevateGroups` and all five `accessRequestService` methods are called from strictly sequential paths. `make test-race` is what keeps that honest. Cite the **function name**, not a line number — this reasoning has already been invalidated once by an unrelated insertion above it - **Test scaffolding in `cmd` lives in `_test.go` files** so `testing` never enters the production build: `cmd/test_helpers_test.go` (`executeCommand`, `executeCommandStreams`, `executeWithHint`, `withInteractiveTTY`) and `cmd/test_mocks_test.go` (every shared mock). Both used to be production files. Neither linked `testing` in at the time, so the moves were **preventive, not remedial** — but `withInteractiveTTY` would have been the first helper to pull it in, and the mock file is the larger and faster-growing of the two. `go list -deps . | grep -c '^testing$'` must stay `0` +- **Output contracts**: every machine-facing document (status, list, elevation, `env` credentials, favorites list, access request) has exactly ONE whole-object test that compares the emitted JSON against an inline literal with `assertJSONEqual` (`cmd/test_helpers_test.go`). It is deliberately brittle against added fields — these documents are a compatibility surface, and a new field should force a conscious review rather than pass silently. Inline literals, not golden files: the repo has no `testdata` machinery and the objects are small. Keep focused tests for conditional and optional fields instead of converting every behavioral test into a whole-object one +- **Fixture values must be distinct and self-describing** (`ws-name`, `ws-id`, `role-name`, `role-id`, `grp-id`, `dir-id`, `AKIA-fixture`, …). A swap mutation — target with role, secretAccessKey with sessionToken, groupId with directoryId — is undetectable when both sides hold `"test"`. Same reason two same-named groups in different directories are the fixture for the favorites `DirectoryID` tests: a unique name makes the directory ID non-load-bearing - Any test whose behavior depends on interactivity MUST set it explicitly with `withInteractiveTTY`: `go test` happens to run with a non-TTY stdin, but that is an accident of the harness, not an assertion - Tests that swap a package-level var (e.g. `ui.IsTerminalFunc`, `recordSessionTimestamp`, `bootstrapImpl`) MUST NOT call `t.Parallel()` — `-race` flags concurrent access to the global. Mark them with a `// Not parallel: mutates the package-global X.` comment. This is why the `cmd` package tests are all serial. diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index bbe8f8e..d7b3d7d 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -65,40 +65,40 @@ premise does not hold). | 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-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 | done | +| REQ-18 | cmd/request output (text) | `cmd/request.go:125` | `fmt.Fprintf(w, "Created By: %s\n", r.CreatedBy)` → source from `r.UpdatedBy` | CONFIRMED | test | `TestRequestGet_TextFieldMapping` | PR5 | done | +| REQ-19 | cmd/request output (JSON) | `cmd/request.go:190-191` | Swap `TimeFrom: r.DetailString("timeFrom")` and `TimeTo: r.DetailString("timeTo")` | CONFIRMED | test | `TestRequestGetJSON_FieldMapping` (`assertJSONEqual`) | PR5 | done | | REQ-20 | cmd/login | `cmd/login.go:52` | `if profile == nil {` → `if false {` (auto-configure branch). Feature *is* implemented; `login_test.go` skips it with the factually wrong reason "Auto-configure not yet implemented" — delete the skip | CONFIRMED | test | `TestRunLogin_AutoConfiguresMissingProfile` | PR4 | done | | REQ-21 | cmd/login | `cmd/login.go:74` | `auth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, true)` → `..., true, false)` (swap `force`/`refreshAuth`) | CONFIRMED | test | `TestRunLogin_AuthenticateFlags` | PR4 | done | | REQ-22 | cmd/request submit | `cmd/request_submit.go:251` | `if !ui.IsInteractive() {` → `if false {` inside the `roleID == ""` branch | CONFIRMED | test | `TestRunRequestSubmit_NonInteractiveRequiresRoleID` | PR4 | done | -| REQ-23 | cmd integration suite | `cmd/integration_test.go` (harness, not a production site) | No mutation. Claim was "integration tests are absent from CI **and** every assertion accepts a panic". First half confirmed (`.github/workflows/ci.yml` runs `make test-race` / `go test -race`, never `-tags=integration`); second half is too broad — only line 153 accepts any panic; lines 71/125/235 require specific output | OVERSTATED | test | `TestMain` isolation + exact exit-code/error-text assertions; add `-tags=integration` to both CI legs | PR1 | done | -| OUT-01 | cmd/list flags | `cmd/list.go:73` | Delete `cmd.MarkFlagsMutuallyExclusive("groups", "provider")`. `TestListCommand_MutualExclusivity` passes today on the *unrelated* runtime error `no eligible targets or groups found` | CONFIRMED | test | `TestListCommand_MutualExclusivity` (assert Cobra's `[groups provider] were all set`) | PR5 | todo | -| OUT-02 | cmd/favorites (interactive) | `cmd/favorites.go:258` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `selectFavoriteInteractive` | CONFIRMED | test | `TestFavoritesAddInteractive_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | -| OUT-03 | cmd/favorites (group add) | `cmd/favorites.go:388` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `addGroupFavorite` | CONFIRMED | test | `TestAddGroupFavorite_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | todo | -| OUT-04 | cmd/list | `cmd/list.go:135` | `if provider == "" {` → `if true {` (groups fetched and emitted even when `--provider` is set) | CONFIRMED | test | `TestListCommand_ProviderSuppressesGroups` | PR5 | todo | -| OUT-05 | cmd/status JSON | `cmd/status.go:212` | `Provider: strings.ToLower(string(s.CSP))` → `strings.ToUpper(string(s.CSP))` | CONFIRMED | test | `TestStatusJSON_Contract` (`assertJSONEqual`) | PR5 | todo | -| OUT-06 | cmd/status JSON | `cmd/status.go:213` | `WorkspaceID: s.WorkspaceID` → `WorkspaceID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | -| OUT-07 | cmd/status JSON | `cmd/status.go:214` | `Duration: s.SessionDuration` → `Duration: 0` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | -| OUT-08 | cmd/status JSON | `cmd/status.go:215` | `RoleID: s.RoleID` → `RoleID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | todo | -| OUT-09 | cmd/status JSON | `cmd/status.go:217-219` | Delete the `if name, ok := data.nameMap[s.WorkspaceID]; ok { so.WorkspaceName = name }` block | CONFIRMED | test | `TestStatusJSON_ResolvesWorkspaceName` | PR5 | todo | -| OUT-10 | cmd/status JSON | `cmd/status.go:221` | `so.Type = "group"` → `so.Type = "cloud"` | CONFIRMED | test | `TestStatusJSON_GroupSessionType` | PR5 | todo | -| OUT-11 | cmd/list JSON | `cmd/list.go:164` | `WorkspaceID: t.WorkspaceID` → `WorkspaceID: t.OrganizationID` | CONFIRMED | test | `TestListJSON_Contract` (`assertJSONEqual`) | PR5 | todo | -| OUT-12 | cmd/list JSON | `cmd/list.go:165` | `WorkspaceType: strings.ToLower(string(t.WorkspaceType))` → `WorkspaceType: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | -| OUT-13 | cmd/list JSON | `cmd/list.go:167` | `RoleID: t.RoleInfo.ID` → `RoleID: ""`. `roleId` is the field an LLM/automation feeds straight back into `grant request submit --role-id`. Note the verifier's correction: `--target` resolves on the emitted **name**, not `workspaceId` | CONFIRMED | test | `TestListJSON_RoundTripsToRequestSubmit` | PR5 | todo | -| OUT-14 | cmd/list JSON | `cmd/list.go:175` | `GroupID: g.GroupID` → `GroupID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | -| OUT-15 | cmd/list JSON | `cmd/list.go:176` | `DirectoryID: g.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | todo | -| OUT-16 | cmd/favorites JSON | `cmd/favorites.go:462` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | todo | -| OUT-17 | cmd/favorites JSON | `cmd/favorites.go:464` | `Role: entry.Role` → `Role: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | -| OUT-18 | cmd/favorites JSON | `cmd/favorites.go:466` | `DirectoryID: entry.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | todo | -| OUT-19 | cmd/favorites | `cmd/favorites.go:333` | `fav.Provider = cfg.DefaultProvider` → `fav.Provider = "azure"`. Every command test uses the azure default, so a non-default `DefaultProvider` (aws/gcp) is unpinned. Secondary, same defect class: `internal/config/favorites.go:21-22` independently defaults empty → `"azure"` | CONFIRMED | test | `TestFavoritesAdd_HonorsNonDefaultProvider` | PR5 | todo | -| OUT-20 | cmd/favorites | `cmd/favorites.go:199-205` | Delete the `--type groups` / `--target`+`--role` pairing validation from `parseFavoritesAddFlags`. Dead-covered: `runFavoritesAddProduction` re-validates, so this is redundancy loss for DI callers, not a current user-facing hole | CONFIRMED | test | `TestParseFavoritesAddFlags_Validation` | PR5 | todo | -| OUT-21 | cmd/status | `cmd/status.go:110-114` | Make the directory-name merge unconditional: drop the `if _, exists := data.nameMap[k]; !exists` guard. Precedence is genuinely unasserted, but in production both lookups read the same cached Azure eligibility response, so a divergence needs colliding IDs or malformed data | OVERSTATED | test | `TestStatus_DirectoryNameMergePrecedence` | PR5 | todo | -| OUT-22 | cmd/status | `cmd/status.go:129` | Delete `_ = cache.CleanupSessions(tracker, activeIDs)` | CONFIRMED | test | `TestStatus_CleansUpStaleSessionTimestamps` | PR5 | todo | -| OUT-23 | cmd/status (test quality) | `cmd/status.go:185-192` (`computeRemainingTime`) | No production defect. `TestStatusCommand_RemainingTime/text_output_shows_remaining_time` asserts `remaining: 4` as a substring, which `remaining: 4h 30m` satisfies — only the JSON sibling killed a sixfold arithmetic error. Signal-poor assertion, not an uncovered defect | CONFIRMED | test | Tighten the text subtest to an exact `remaining: 45m` | PR5 | todo | -| OUT-24 | cmd/favorites | `cmd/favorites.go:432-433` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | todo | -| OUT-25 | cmd/list flags | `cmd/list.go:71` | Delete `cmd.Flags().Bool("refresh", ...)` registration. **Verifier correction:** `grant list --refresh` is **not** already a no-op — `list.go:91-92` reads it and passes it into `buildCachedLister`, and CLAUDE.md is correct. The real finding is missing flag-registration/wiring coverage | OVERSTATED | test | `TestListCommand_RefreshBypassesCache` | PR5 | todo | -| OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | done | -| OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | refuted | n/a — already killed | — | todo | +| REQ-23 | cmd integration suite | `cmd/integration_test.go` (harness, not a production site) | No mutation. Claim was "integration tests are absent from CI **and** every assertion accepts a panic". First half confirmed (`.github/workflows/ci.yml` runs `make test-race` / `go test -race`, never `-tags=integration`); second half is too broad — only line 153 accepts any panic; lines 71/125/235 require specific output | OVERSTATED | test | `TestMain` isolation + exact exit-code/error-text assertions; add `-tags=integration` to both CI legs | PR1 | todo | +| OUT-01 | cmd/list flags | `cmd/list.go:73` | Delete `cmd.MarkFlagsMutuallyExclusive("groups", "provider")`. `TestListCommand_MutualExclusivity` passes today on the *unrelated* runtime error `no eligible targets or groups found` | CONFIRMED | test | `TestListCommand_MutualExclusivity` (assert Cobra's `[groups provider] were all set`) | PR5 | done | +| OUT-02 | cmd/favorites (interactive) | `cmd/favorites.go:258` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `selectFavoriteInteractive` | CONFIRMED | test | `TestFavoritesAddInteractive_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | done | +| OUT-03 | cmd/favorites (group add) | `cmd/favorites.go:388` | `fav.DirectoryID = selected.group.DirectoryID` → delete the line, in `addGroupFavorite` | CONFIRMED | test | `TestAddGroupFavorite_PersistsDirectoryID` + `findMatchingGroup` round-trip | PR5 | done | +| OUT-04 | cmd/list | `cmd/list.go:135` | `if provider == "" {` → `if true {` (groups fetched and emitted even when `--provider` is set) | CONFIRMED | test | `TestListCommand_ProviderSuppressesGroups` | PR5 | done | +| OUT-05 | cmd/status JSON | `cmd/status.go:212` | `Provider: strings.ToLower(string(s.CSP))` → `strings.ToUpper(string(s.CSP))` | CONFIRMED | test | `TestStatusJSON_Contract` (`assertJSONEqual`) | PR5 | done | +| OUT-06 | cmd/status JSON | `cmd/status.go:213` | `WorkspaceID: s.WorkspaceID` → `WorkspaceID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | done | +| OUT-07 | cmd/status JSON | `cmd/status.go:214` | `Duration: s.SessionDuration` → `Duration: 0` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | done | +| OUT-08 | cmd/status JSON | `cmd/status.go:215` | `RoleID: s.RoleID` → `RoleID: ""` | CONFIRMED | test | `TestStatusJSON_Contract` | PR5 | done | +| OUT-09 | cmd/status JSON | `cmd/status.go:217-219` | Delete the `if name, ok := data.nameMap[s.WorkspaceID]; ok { so.WorkspaceName = name }` block | CONFIRMED | test | `TestStatusJSON_Contract` (one whole-object test kills OUT-05..10 together; no separate `TestStatusJSON_ResolvesWorkspaceName`) | PR5 | done | +| OUT-10 | cmd/status JSON | `cmd/status.go:221` | `so.Type = "group"` → `so.Type = "cloud"` | CONFIRMED | test | `TestStatusJSON_Contract` (as above; no separate `TestStatusJSON_GroupSessionType`) | PR5 | done | +| OUT-11 | cmd/list JSON | `cmd/list.go:164` | `WorkspaceID: t.WorkspaceID` → `WorkspaceID: t.OrganizationID` | CONFIRMED | test | `TestListJSON_Contract` (`assertJSONEqual`) | PR5 | done | +| OUT-12 | cmd/list JSON | `cmd/list.go:165` | `WorkspaceType: strings.ToLower(string(t.WorkspaceType))` → `WorkspaceType: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | +| OUT-13 | cmd/list JSON | `cmd/list.go:167` | `RoleID: t.RoleInfo.ID` → `RoleID: ""`. `roleId` is the field an LLM/automation feeds straight back into `grant request submit --role-id`. Note the verifier's correction: `--target` resolves on the emitted **name**, not `workspaceId` | CONFIRMED | test | `TestListJSON_Contract` + `TestListJSON_RoundTripsToRequestSubmit`. Reverified three ways: `RoleID: ""` against both tests, and `Target: t.WorkspaceID` against the round-trip, which confirms `--target` resolves on the emitted name | PR5 | done | +| OUT-14 | cmd/list JSON | `cmd/list.go:175` | `GroupID: g.GroupID` → `GroupID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | +| OUT-15 | cmd/list JSON | `cmd/list.go:176` | `DirectoryID: g.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | +| OUT-16 | cmd/favorites JSON | `cmd/favorites.go:462` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | done | +| OUT-17 | cmd/favorites JSON | `cmd/favorites.go:464` | `Role: entry.Role` → `Role: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | done | +| OUT-18 | cmd/favorites JSON | `cmd/favorites.go:466` | `DirectoryID: entry.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | done | +| OUT-19 | cmd/favorites | `cmd/favorites.go:333` | `fav.Provider = cfg.DefaultProvider` → `fav.Provider = "azure"`. Every command test uses the azure default, so a non-default `DefaultProvider` (aws/gcp) is unpinned. Secondary, same defect class: `internal/config/favorites.go:21-22` independently defaults empty → `"azure"` | CONFIRMED | test | `TestFavoritesAdd_HonorsNonDefaultProvider` | PR5 | done | +| OUT-20 | cmd/favorites | `cmd/favorites.go:199-205` | Delete the `--type groups` / `--target`+`--role` pairing validation from `parseFavoritesAddFlags` (reverified by deleting the whole `favType`/`else` validation block, which also drops the `--group requires --type groups` arm). Dead-covered: `runFavoritesAddProduction` re-validates, so this is redundancy loss for DI callers, not a current user-facing hole | CONFIRMED | test | `TestParseFavoritesAddFlags_Validation` | PR5 | done | +| OUT-21 | cmd/status | `cmd/status.go:110-114` | Make the directory-name merge unconditional: drop the `if _, exists := data.nameMap[k]; !exists` guard. Precedence is genuinely unasserted, but in production both lookups read the same cached Azure eligibility response, so a divergence needs colliding IDs or malformed data | OVERSTATED | test | `TestStatus_DirectoryNameMergePrecedence` | PR5 | done | +| OUT-22 | cmd/status | `cmd/status.go:129` | Delete `_ = cache.CleanupSessions(tracker, activeIDs)` | CONFIRMED | test | `TestStatus_CleansUpStaleSessionTimestamps` | PR5 | done | +| OUT-23 | cmd/status (test quality) | `cmd/status.go:185-192` (`computeRemainingTime`) | No production defect. `TestStatusCommand_RemainingTime/text_output_shows_remaining_time` asserts `remaining: 4` as a substring, which `remaining: 4h 30m` satisfies — only the JSON sibling killed a sixfold arithmetic error. Signal-poor assertion, not an uncovered defect | CONFIRMED | test | Text subtest tightened to an exact `remaining: 45m`; the fixture now elevates 14m30s ago, since 15m of a 1h session actually renders `44m`. Reverified by making `computeRemainingTime` multiply by `time.Minute` | PR5 | done | +| OUT-24 | cmd/favorites | `cmd/favorites.go:432-433` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | done | +| OUT-25 | cmd/list flags | `cmd/list.go:71` | Delete `cmd.Flags().Bool("refresh", ...)` registration. **Verifier correction:** `grant list --refresh` is **not** already a no-op — `list.go:91-92` reads it and passes it into `buildCachedLister`, and CLAUDE.md is correct. The real finding is missing flag-registration/wiring coverage | OVERSTATED | test | `TestListCommand_RefreshFlagRegistered` (renamed: the cache wiring sits behind `bootstrapSCAService`, so a unit test covers registration and parsing only, not a cache bypass) | PR5 | done | +| OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | todo | +| OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | test | `TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP` — added anyway in PR5: the old kill was incidental (an unrelated error-path assertion). Direct reverification needs a target whose CSP differs from the flag | PR5 | done | | OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | | OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | | SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | done | @@ -164,10 +164,10 @@ premise does not hold). | 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-15 | cmd/root JSON | `cmd/root.go:936-937` | In `writeElevationJSON`, swap `Target: cloudRes.target.WorkspaceName` and `Role: cloudRes.target.RoleInfo.Name` | CONFIRMED | test | `TestElevationJSON_Contract` (`assertJSONEqual`) | PR5 | done | +| ELV-16 | cmd/root JSON | `cmd/root.go:923-924` | In `writeElevationJSON`, swap `GroupID: groupRes.group.GroupID` and `DirectoryID: groupRes.group.DirectoryID` | CONFIRMED | test | `TestGroupElevationJSON_Contract` | PR5 | done | +| ELV-17 | cmd/env JSON | `cmd/env.go:145-147` | Swap `SecretAccessKey: awsCreds.SecretAccessKey` and `SessionToken: awsCreds.SessionToken`. Asymmetry is the point: the identical swap in the **text** export path is killed (`env_test.go:77`) | CONFIRMED | test | `TestEnvJSON_Contract` (`assertJSONEqual`) | PR5 | done | +| ELV-18 | cmd/root JSON | `cmd/root.go:934` | `Provider: strings.ToLower(string(cloudRes.target.CSP))` → drop the `strings.ToLower` | CONFIRMED | test | `TestElevationJSON_Contract` | PR5 | done | | ELV-19 | cmd/root | `cmd/root.go:341-344` | Delete `if len(all) == 0 { return nil, errors.New("no eligible targets found, check your SCA policies") }` in `fetchEligibility`'s multi-CSP branch. Callers replace the intended aggregate error with their own message | CONFIRMED | test | `TestFetchEligibility_AllCSPsFail` | PR4 | done | | ELV-20 | cmd/env elevate | `cmd/root.go:510-514` | Remove the fresh-context setup and elevate with the original `ctx`: delete `elevCtx, elevCancel := context.WithTimeout(...)` / `defer elevCancel()` and call `elevateService.Elevate(ctx, req)`. **Note:** a raw `elevCtx → ctx` token substitution does *not* compile (`declared and not used: elevCtx`) — use the semantic form above. Root's three interactive dispatch paths *are* covered by `TestRootElevate_SlowPromptTimeout`; env is not | CONFIRMED | test | `TestEnv_SlowPromptTimeout` | PR4 | done | | ELV-21 | cmd/env auth | `cmd/root.go:419` | `authLoader.LoadAuthentication(profile, true)` → `(profile, false)` in `resolveAndElevate` (env path) | CONFIRMED | test | `TestAuthCacheFlag/env_path` | PR4 | done | @@ -337,7 +337,7 @@ extracting a shared helper is a refactor and belongs in its own change. | PR2 — Archive extraction and path security | 12 | 11 | | PR3 — Remaining self-update correctness | 10 | 10 | | PR4 — Argument capture | 42 | 41 | -| PR5 — Output contracts | 32 | 30 | +| PR5 — Output contracts | 33 | 30 | | PR6 — Cache and config semantics | 16 | 16 | | PR7 — UI behavior | 10 | 10 | | PR8 — SCA / workflows / models wire contracts | 39 | 37 | From aedac1d48fa53cec35d7cf7391b7b7c04aced32a Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sat, 15 Aug 2026 10:47:45 +0200 Subject: [PATCH 4/7] fix(test): make the list->submit round-trip pin production and stop a panic Three adversarial-review findings on PR5's output contracts. 1. TestListJSON_RoundTripsToRequestSubmit's second leg stubbed resolveSubmitTargetFn with a textual copy of the resolveSubmitTarget matcher, so it pinned nothing: changing --target to resolve on WorkspaceID instead of WorkspaceName survived the whole cmd suite. Extract matchWorkspaceByName (behaviour-preserving) and call it from both production and the test, so the mutation now fails. 2. TestRequestList_TextFieldMapping guarded len(fields) < 6 while indexing fields[6]; a column-blanking mutation panicked and aborted the entire cmd test binary. Raise the guard to < 7. 3. Narrow pinRemainingSeconds' lower bound from 2600 to 2695 so a whole-minute arithmetic error dies at the pin itself, and label the deliberately impossible favorites provider/CSP fixture as such. --- cmd/favorites_persistence_test.go | 8 ++++++-- cmd/output_contract_test.go | 20 ++++++++++---------- cmd/request_output_test.go | 6 ++++-- cmd/request_submit.go | 19 +++++++++++++++---- docs/mutation-ledger.md | 6 +++--- 5 files changed, 38 insertions(+), 21 deletions(-) diff --git a/cmd/favorites_persistence_test.go b/cmd/favorites_persistence_test.go index fea6696..7118123 100644 --- a/cmd/favorites_persistence_test.go +++ b/cmd/favorites_persistence_test.go @@ -151,8 +151,12 @@ func TestFavoritesAdd_HonorsNonDefaultProvider(t *testing.T) { // TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP pins the precedence // in selectFavoriteInteractive: an explicit --provider is stored verbatim -// rather than derived from the selected target's CSP. The fixture's CSP is aws -// while the flag says azure — with both azure the two branches are identical. +// rather than derived from the selected target's CSP. The fixture is +// deliberately impossible: --provider azure with an AWS target is a +// combination production filtering would never produce, and it exists only so +// the two branches yield different values. Do not read it as a realistic +// scenario, and do not "fix" it to azure — with both azure the branches are +// indistinguishable and the mutation survives. func TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP(t *testing.T) { configPath := filepath.Join(t.TempDir(), "config.yaml") t.Setenv("GRANT_CONFIG", configPath) diff --git a/cmd/output_contract_test.go b/cmd/output_contract_test.go index 9f68b77..e357d1e 100644 --- a/cmd/output_contract_test.go +++ b/cmd/output_contract_test.go @@ -19,7 +19,6 @@ import ( "context" "encoding/json" "path/filepath" - "strings" "testing" "time" @@ -131,7 +130,10 @@ func TestStatusJSON_Contract(t *testing.T) { t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) } - got := pinRemainingSeconds(t, []byte(stdout), 2600, pinnedRemainingSeconds) + // The fixture makes this deterministically 2699; the window is narrow on + // purpose so a whole-minute arithmetic error dies at the pin itself rather + // than relying on the sibling text assertion. + got := pinRemainingSeconds(t, []byte(stdout), 2695, pinnedRemainingSeconds) assertJSONEqual(t, got, `{ "authenticated": true, @@ -366,17 +368,15 @@ func TestListJSON_RoundTripsToRequestSubmit(t *testing.T) { } // 2. `grant request submit --target --role-id `. - // The workspace resolver is stubbed (it bootstraps live SCA auth), but it - // matches the emitted NAME over the production deduplicateWorkspaces - // output, exactly as resolveSubmitTarget does. + // Only the auth/eligibility fetch is stubbed out; the resolution itself + // runs the production deduplicateWorkspaces + matchWorkspaceByName pair + // that resolveSubmitTarget calls, so changing what --target matches on + // breaks this test. origResolve := resolveSubmitTargetFn t.Cleanup(func() { resolveSubmitTargetFn = origResolve }) resolveSubmitTargetFn = func(_ context.Context, _, targetName string, _ bool) (*submitWorkspace, error) { - workspaces := deduplicateWorkspaces(targets) - for i := range workspaces { - if strings.EqualFold(workspaces[i].WorkspaceName, targetName) { - return &workspaces[i], nil - } + if ws := matchWorkspaceByName(deduplicateWorkspaces(targets), targetName); ws != nil { + return ws, nil } t.Errorf("emitted target %q matched no eligible workspace", targetName) return nil, errNotAuthenticated diff --git a/cmd/request_output_test.go b/cmd/request_output_test.go index 9f266e7..cfe2395 100644 --- a/cmd/request_output_test.go +++ b/cmd/request_output_test.go @@ -72,8 +72,10 @@ func TestRequestList_TextFieldMapping(t *testing.T) { // Columns: ID STATE RESULT TARGET ROLE PRIORITY CREATED BY CREATED AT. // No fixture value contains a space, so field positions are unambiguous. fields := strings.Fields(row) - if len(fields) < 6 { - t.Fatalf("row has %d columns, want at least 6: %q", len(fields), row) + // Guard on 7, the highest index read below: a short row must Fatal here + // rather than panic, which would abort the whole cmd test binary. + if len(fields) < 7 { + t.Fatalf("row has %d columns, want at least 7: %q", len(fields), row) } if fields[3] != "ws-name" { t.Errorf("TARGET column = %q, want ws-name", fields[3]) diff --git a/cmd/request_submit.go b/cmd/request_submit.go index a1bc453..ad6230d 100644 --- a/cmd/request_submit.go +++ b/cmd/request_submit.go @@ -423,10 +423,8 @@ func resolveSubmitTarget(ctx context.Context, provider, targetName string, refre // Non-interactive: match by --target flag if targetName != "" { - for i := range workspaces { - if strings.EqualFold(workspaces[i].WorkspaceName, targetName) { - return &workspaces[i], nil - } + if ws := matchWorkspaceByName(workspaces, targetName); ws != nil { + return ws, nil } return nil, fmt.Errorf("no eligible workspace found matching target=%q", targetName) } @@ -440,6 +438,19 @@ func resolveSubmitTarget(ctx context.Context, provider, targetName string, refre return submitWorkspaceSelectorFn(workspaces) } +// matchWorkspaceByName resolves the --target flag against the deduplicated +// workspace list. The match is on WorkspaceName — the same value `grant list +// --output json` emits as `target` — which is what makes the emitted name +// directly reusable as `grant request submit --target`. +func matchWorkspaceByName(workspaces []submitWorkspace, targetName string) *submitWorkspace { + for i := range workspaces { + if strings.EqualFold(workspaces[i].WorkspaceName, targetName) { + return &workspaces[i] + } + } + return nil +} + func selectSubmitWorkspace(workspaces []submitWorkspace) (*submitWorkspace, error) { if !ui.IsInteractive() { return nil, errors.New("non-interactive mode requires --target") diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index d7b3d7d..e860c39 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -65,7 +65,7 @@ premise does not hold). | 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 | done | +| REQ-17 | cmd/request output (text) | `cmd/request.go:79-80` | Swap the table values: `r.DetailString("workspaceName")` / `r.DetailString("roleName")` | CONFIRMED | test | `TestRequestList_TextFieldMapping`. **Corrected after adversarial review:** the column-count guard read `len(fields) < 6` while the assertions index `fields[6]`, so a column-blanking mutation panicked and aborted the entire `cmd` test binary instead of reporting one failure. Guard raised to `< 7`; reverified with the priority+createdBy blanking mutation, which now Fatals cleanly with zero panics across the package | PR5 | done | | REQ-18 | cmd/request output (text) | `cmd/request.go:125` | `fmt.Fprintf(w, "Created By: %s\n", r.CreatedBy)` → source from `r.UpdatedBy` | CONFIRMED | test | `TestRequestGet_TextFieldMapping` | PR5 | done | | REQ-19 | cmd/request output (JSON) | `cmd/request.go:190-191` | Swap `TimeFrom: r.DetailString("timeFrom")` and `TimeTo: r.DetailString("timeTo")` | CONFIRMED | test | `TestRequestGetJSON_FieldMapping` (`assertJSONEqual`) | PR5 | done | | REQ-20 | cmd/login | `cmd/login.go:52` | `if profile == nil {` → `if false {` (auto-configure branch). Feature *is* implemented; `login_test.go` skips it with the factually wrong reason "Auto-configure not yet implemented" — delete the skip | CONFIRMED | test | `TestRunLogin_AutoConfiguresMissingProfile` | PR4 | done | @@ -84,7 +84,7 @@ premise does not hold). | OUT-10 | cmd/status JSON | `cmd/status.go:221` | `so.Type = "group"` → `so.Type = "cloud"` | CONFIRMED | test | `TestStatusJSON_Contract` (as above; no separate `TestStatusJSON_GroupSessionType`) | PR5 | done | | OUT-11 | cmd/list JSON | `cmd/list.go:164` | `WorkspaceID: t.WorkspaceID` → `WorkspaceID: t.OrganizationID` | CONFIRMED | test | `TestListJSON_Contract` (`assertJSONEqual`) | PR5 | done | | OUT-12 | cmd/list JSON | `cmd/list.go:165` | `WorkspaceType: strings.ToLower(string(t.WorkspaceType))` → `WorkspaceType: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | -| OUT-13 | cmd/list JSON | `cmd/list.go:167` | `RoleID: t.RoleInfo.ID` → `RoleID: ""`. `roleId` is the field an LLM/automation feeds straight back into `grant request submit --role-id`. Note the verifier's correction: `--target` resolves on the emitted **name**, not `workspaceId` | CONFIRMED | test | `TestListJSON_Contract` + `TestListJSON_RoundTripsToRequestSubmit`. Reverified three ways: `RoleID: ""` against both tests, and `Target: t.WorkspaceID` against the round-trip, which confirms `--target` resolves on the emitted name | PR5 | done | +| OUT-13 | cmd/list JSON | `cmd/list.go:167` | `RoleID: t.RoleInfo.ID` → `RoleID: ""`. `roleId` is the field an LLM/automation feeds straight back into `grant request submit --role-id`. Note the verifier's correction: `--target` resolves on the emitted **name**, not `workspaceId` | CONFIRMED | test | `TestListJSON_Contract` + `TestListJSON_RoundTripsToRequestSubmit`. **Corrected after adversarial review:** leg 2 of the round-trip originally stubbed `resolveSubmitTargetFn` with a *textual copy* of the `resolveSubmitTarget` matcher, which pinned nothing in production — mutating `WorkspaceName` → `WorkspaceID` at `cmd/request_submit.go:415` SURVIVED the whole `cmd` suite. The earlier "reverified three ways" claim was wrong: the `Target: t.WorkspaceID` kill came from the ROOT command's `--target` (`findMatchingTarget`, `cmd/root.go:968`), not from `request submit`. Fixed by extracting `matchWorkspaceByName` in `cmd/request_submit.go` (behavior-preserving refactor; no CHANGELOG) and calling it from both production and the test. The `WorkspaceName` → `WorkspaceID` mutation now FAILS the test; reverted, it passes. `RoleID: ""` still dies against both tests | PR5 | done | | OUT-14 | cmd/list JSON | `cmd/list.go:175` | `GroupID: g.GroupID` → `GroupID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | | OUT-15 | cmd/list JSON | `cmd/list.go:176` | `DirectoryID: g.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | | OUT-16 | cmd/favorites JSON | `cmd/favorites.go:462` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | done | @@ -98,7 +98,7 @@ premise does not hold). | OUT-24 | cmd/favorites | `cmd/favorites.go:432-433` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | done | | OUT-25 | cmd/list flags | `cmd/list.go:71` | Delete `cmd.Flags().Bool("refresh", ...)` registration. **Verifier correction:** `grant list --refresh` is **not** already a no-op — `list.go:91-92` reads it and passes it into `buildCachedLister`, and CLAUDE.md is correct. The real finding is missing flag-registration/wiring coverage | OVERSTATED | test | `TestListCommand_RefreshFlagRegistered` (renamed: the cache wiring sits behind `bootstrapSCAService`, so a unit test covers registration and parsing only, not a cache bypass) | PR5 | done | | OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | todo | -| OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | test | `TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP` — added anyway in PR5: the old kill was incidental (an unrelated error-path assertion). Direct reverification needs a target whose CSP differs from the flag | PR5 | done | +| OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | test | `TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP` — added anyway in PR5: the old kill was incidental (an unrelated error-path assertion). Direct reverification needs a target whose CSP differs from the flag, so the fixture pairs `--provider azure` with an AWS target — a combination production filtering would never emit. The test comment now labels it deliberately impossible rather than merely "distinguishable" | PR5 | done | | OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | | OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | | SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | done | From 0df460ab198179d3ae6614d431ed64acf984d49a Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:24:34 +0200 Subject: [PATCH 5/7] test(cmd): pin request list and revoke JSON contracts Both machine-facing documents were entirely unpinned: every test unmarshalled into the very output struct under test, so renaming requests, sessionId or outcome survived. outcome is the single classification field callers switch on. --- cmd/output_contract_test.go | 117 ++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/cmd/output_contract_test.go b/cmd/output_contract_test.go index e357d1e..416076a 100644 --- a/cmd/output_contract_test.go +++ b/cmd/output_contract_test.go @@ -592,3 +592,120 @@ func TestFavoritesListJSON_Contract(t *testing.T) { } ]`) } + +// --- access request list ------------------------------------------------------ + +// TestRequestListJSON_Contract pins the whole `grant request list -o json` +// document. The envelope was entirely unpinned: renaming the `requests` key, or +// adding a field to accessRequestListOutput, survived every existing test — +// they all unmarshalled into the very struct under test, so the tags could +// drift freely. totalCount deliberately differs from len(requests): it comes +// from the service's pagination total, not from the returned page. +func TestRequestListJSON_Contract(t *testing.T) { + svc := &mockAccessRequestService{ + listItems: []wfmodels.AccessRequest{*requestFixture()}, + listTotalCount: 7, + } + + root := newTestRootCommand() + root.AddCommand(NewRequestCommandWithDeps(svc)) + + stdout, stderr, err := executeCommandStreams(root, "request", "list", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "requests": [ + { + "requestId": "req-id", + "targetCategory": "CLOUD_CONSOLE", + "state": "PENDING", + "result": "UNKNOWN", + "priority": "priority-fixture", + "reason": "reason-fixture", + "provider": "provider-fixture", + "target": "ws-name", + "role": "role-name", + "requestDate": "2026-04-21", + "timezone": "tz-fixture", + "timeFrom": "01:11", + "timeTo": "22:22", + "finalizationReason": "finalization-fixture", + "requestLink": "https://example.test/req-id", + "createdBy": "creator-fixture", + "createdAt": "2026-04-20T10:00:00Z", + "updatedBy": "updater-fixture", + "updatedAt": "2026-04-21T11:00:00Z" + } + ], + "totalCount": 7 +}`) +} + +// --- revoke ------------------------------------------------------------------- + +// TestRevokeJSON_Contract pins the whole `grant revoke --output json` array. +// Every existing revoke test unmarshalled into []revocationOutput, so renaming +// sessionId or outcome survived — and outcome is the single classification +// field callers switch on, which makes it the weakest point in the surface. +// +// The fixture carries four requested sessions with four DIFFERENT outcomes plus +// an unattributed row, so the per-row mapping is pinned, not just the envelope: +// a row-to-outcome shuffle changes the document. +func TestRevokeJSON_Contract(t *testing.T) { + revoker := &mockSessionRevoker{response: revokeResponse( + "sess-revoked", scamodels.RevocationSuccessful, + "sess-inprogress", scamodels.RevocationInProgress, + "sess-notapplicable", scamodels.RevocationNotApplicable, + "sess-unrequested", scamodels.RevocationSuccessful, + )} + + cmd := NewRevokeCommandWithDeps(testAuthLoader(), &mockSessionLister{}, &mockEligibilityLister{}, + revoker, &mockSessionSelector{}, &mockConfirmPrompter{}) + root := newTestRootCommand() + root.AddCommand(cmd) + + // sess-missing is requested but never answered, so it must still appear. + stdout, _, err := executeCommandStreams(root, "revoke", + "sess-revoked", "sess-inprogress", "sess-notapplicable", "sess-missing", + "--yes", "--output", "json") + if err == nil { + t.Fatal("expected a non-zero exit: not_applicable and a missing row are both failures") + } + + // A confirmed revocation carries no reason, so `reason` must be ABSENT from + // the first entry — omitempty on that field is part of the contract. + assertJSONEqual(t, []byte(stdout), `[ + { + "sessionId": "sess-revoked", + "status": "SUCCESSFULLY_REVOKED", + "outcome": "revoked" + }, + { + "sessionId": "sess-inprogress", + "status": "REVOCATION_IN_PROGRESS", + "outcome": "in_progress", + "reason": "accepted by the service, not yet confirmed complete" + }, + { + "sessionId": "sess-notapplicable", + "status": "REVOCATION_NOT_APPLICABLE", + "outcome": "not_applicable", + "reason": "the service reported revocation is not applicable to this session" + }, + { + "sessionId": "sess-missing", + "status": "", + "outcome": "unknown", + "reason": "no result returned by the service for this session" + }, + { + "sessionId": "sess-unrequested", + "status": "SUCCESSFULLY_REVOKED", + "outcome": "unknown", + "reason": "result was not requested and satisfies no requested session", + "unexpected": true + } +]`) +} From a8eed157c7a2407a02f3d198292da1d8bfd45df9 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:24:40 +0200 Subject: [PATCH 6/7] test(cmd): pin the absent state of optional JSON fields Every fixture populated credentials, workspaceName, username and directory, so dropping omitempty from any of them was invisible. Assert the key is absent, not null or empty. --- cmd/output_contract_test.go | 127 ++++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) diff --git a/cmd/output_contract_test.go b/cmd/output_contract_test.go index 416076a..a9fcd15 100644 --- a/cmd/output_contract_test.go +++ b/cmd/output_contract_test.go @@ -709,3 +709,130 @@ func TestRevokeJSON_Contract(t *testing.T) { } ]`) } + +// --- absent optional fields ---------------------------------------------------- +// +// The tests above populate every optional field, which pins the PRESENT state. +// Dropping `omitempty` from a field is invisible to them: the key is emitted +// either way. These three pin the ABSENT state — the key must not appear at +// all, rather than appearing as null or "". A consumer doing a key-presence +// check sees a contract change where a struct-unmarshalling test sees none. + +// TestElevationJSON_AzureOmitsCredentials pins that a non-AWS elevation emits +// NO `credentials` key. Dropping omitempty turns it into "credentials": null. +func TestElevationJSON_AzureOmitsCredentials(t *testing.T) { + target := &scamodels.EligibleTarget{ + CSP: scamodels.CSPAzure, + OrganizationID: "org-id", + WorkspaceID: "ws-id", + WorkspaceName: "ws-name", + WorkspaceType: scamodels.WorkspaceTypeSubscription, + RoleInfo: scamodels.RoleInfo{ID: "role-id", Name: "role-name"}, + } + + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt", Username: "user-fixture@example.test"}} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{*target}, Total: 1, + }} + // No AccessCredentials: Azure elevations never carry them. + elev := &mockElevateService{response: &scamodels.ElevateResponse{Response: scamodels.ElevateAccessResult{ + CSP: scamodels.CSPAzure, OrganizationID: "org-id", + Results: []scamodels.ElevateTargetResult{{ + WorkspaceID: "ws-id", RoleID: "role-id", SessionID: "sess-id", + }}, + }}} + sel := &mockUnifiedSelector{item: &selectionItem{kind: selectionCloud, cloud: target}} + + cmd := NewRootCommandWithDeps(nil, auth, elig, elev, sel, + &mockGroupsEligibilityLister{listErr: errNotAuthenticated}, nil, config.DefaultConfig()) + + stdout, stderr, err := executeCommandStreams(cmd, "--output", "json", "--provider", "azure") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "type": "cloud", + "provider": "azure", + "sessionId": "sess-id", + "target": "ws-name", + "role": "role-name" +}`) +} + +// TestStatusJSON_OmitsAbsentOptionalFields pins the absent state of the status +// document: an unnamed token emits NO `username`, and a session whose workspace +// ID is missing from the name map emits NO `workspaceName`. Dropping omitempty +// from either turns it into an empty string. +func TestStatusJSON_OmitsAbsentOptionalFields(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + + sessions := &mockSessionLister{sessions: &scamodels.SessionsResponse{ + Response: []scamodels.SessionInfo{{ + SessionID: "sess-unnamed", CSP: scamodels.CSPAzure, + WorkspaceID: "ws-id-unknown", RoleID: "role-id", SessionDuration: 3600, + }}, + Total: 1, + }} + // Eligibility knows a different workspace, so the lookup misses. + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{ + Response: []scamodels.EligibleTarget{{WorkspaceID: "ws-id-other", WorkspaceName: "ws-name"}}, + Total: 1, + }} + + cmd := NewStatusCommandWithDeps(auth, sessions, elig, nil, nil) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "status", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "authenticated": true, + "sessions": [ + { + "sessionId": "sess-unnamed", + "provider": "azure", + "workspaceId": "ws-id-unknown", + "roleId": "role-id", + "duration": 3600, + "type": "cloud" + } + ] +}`) +} + +// TestListJSON_OmitsAbsentDirectory pins that a group with no directory name +// emits NO `directory` key. Dropping omitempty turns it into "directory": "". +func TestListJSON_OmitsAbsentDirectory(t *testing.T) { + auth := &mockAuthLoader{token: &authmodels.IdsecToken{Token: "jwt"}} + elig := &mockEligibilityLister{response: &scamodels.EligibilityResponse{}} + groupsElig := &mockGroupsEligibilityLister{response: &scamodels.GroupsEligibilityResponse{ + Response: []scamodels.GroupsEligibleTarget{{ + GroupName: "grp-name", GroupID: "grp-id", DirectoryID: "dir-id", + }}, + Total: 1, + }} + + cmd := NewListCommandWithDeps(auth, elig, groupsElig) + root := newTestRootCommand() + root.AddCommand(cmd) + + stdout, stderr, err := executeCommandStreams(root, "list", "--output", "json") + if err != nil { + t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + assertJSONEqual(t, []byte(stdout), `{ + "cloud": [], + "groups": [ + { + "groupName": "grp-name", + "groupId": "grp-id", + "directoryId": "dir-id" + } + ] +}`) +} From 1a6b5bef8a95636db106cd0336253259f9c9b0c8 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 09:27:20 +0200 Subject: [PATCH 7/7] docs: scope the output-contract claim and log the closed mutations Name the nine pinned documents instead of claiming blanket coverage, and add OUT-30..38 to the mutation ledger. --- CLAUDE.md | 2 +- docs/mutation-ledger.md | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ad668ec..006d7aa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -90,7 +90,7 @@ Custom `SCAAccessService` follows SDK conventions: - **Mock capture convention** (`cmd/test_mocks_test.go`, precedent `mockSessionRevoker`): record the arguments in the method body *before* dispatching to any `xxxFunc` callback, keep a history slice plus a `lastX()` accessor (a history is what answers "called exactly once?"), defensively copy slices/maps and pointer-to-struct args, and guard against a nil request. An optional `*string` argument is flattened to `reason string` + `reasonSet bool` so a test can tell `nil` from `""`. There is exactly one mock per interface — an arg-blind sibling silently opts every future test out of capture - No mutex on those histories: the only mocks reached from more than one goroutine are the eligibility listers, via the fan-outs in `fetchEligibility`/`fetchGroupsEligibility` (`cmd/root.go`), `resolveAndElevateUnifiedPath` (`cmd/root.go`) and `fetchAllTargets`/`fetchAllGroups` (`cmd/helpers.go`), and those mocks are stateless readers. `Elevate`, `ElevateGroups` and all five `accessRequestService` methods are called from strictly sequential paths. `make test-race` is what keeps that honest. Cite the **function name**, not a line number — this reasoning has already been invalidated once by an unrelated insertion above it - **Test scaffolding in `cmd` lives in `_test.go` files** so `testing` never enters the production build: `cmd/test_helpers_test.go` (`executeCommand`, `executeCommandStreams`, `executeWithHint`, `withInteractiveTTY`) and `cmd/test_mocks_test.go` (every shared mock). Both used to be production files. Neither linked `testing` in at the time, so the moves were **preventive, not remedial** — but `withInteractiveTTY` would have been the first helper to pull it in, and the mock file is the larger and faster-growing of the two. `go list -deps . | grep -c '^testing$'` must stay `0` -- **Output contracts**: every machine-facing document (status, list, elevation, `env` credentials, favorites list, access request) has exactly ONE whole-object test that compares the emitted JSON against an inline literal with `assertJSONEqual` (`cmd/test_helpers_test.go`). It is deliberately brittle against added fields — these documents are a compatibility surface, and a new field should force a conscious review rather than pass silently. Inline literals, not golden files: the repo has no `testdata` machinery and the objects are small. Keep focused tests for conditional and optional fields instead of converting every behavioral test into a whole-object one +- **Output contracts**: each machine-facing document has exactly ONE whole-object test comparing the emitted JSON against an inline literal with `assertJSONEqual` (`cmd/test_helpers_test.go`) — nine of them: cloud elevation, group elevation, `env` credentials, `list`, `status`, `revoke`, `favorites list`, the access-request object (shared by `request get`/`submit`/`cancel`/`approve`/`reject`) and the `request list` envelope. Optional fields additionally get a whole-object case exercising the ABSENT state, so dropping an `omitempty` fails. It is deliberately brittle against added fields — these documents are a compatibility surface, and a new field should force a conscious review rather than pass silently. Inline literals, not golden files: the repo has no `testdata` machinery and the objects are small. Keep focused tests for conditional and optional fields instead of converting every behavioral test into a whole-object one - **Fixture values must be distinct and self-describing** (`ws-name`, `ws-id`, `role-name`, `role-id`, `grp-id`, `dir-id`, `AKIA-fixture`, …). A swap mutation — target with role, secretAccessKey with sessionToken, groupId with directoryId — is undetectable when both sides hold `"test"`. Same reason two same-named groups in different directories are the fixture for the favorites `DirectoryID` tests: a unique name makes the directory ID non-load-bearing - Any test whose behavior depends on interactivity MUST set it explicitly with `withInteractiveTTY`: `go test` happens to run with a non-TTY stdin, but that is an accident of the harness, not an assertion - Tests that swap a package-level var (e.g. `ui.IsTerminalFunc`, `recordSessionTimestamp`, `bootstrapImpl`) MUST NOT call `t.Parallel()` — `-race` flags concurrent access to the global. Mark them with a `// Not parallel: mutates the package-global X.` comment. This is why the `cmd` package tests are all serial. diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index e860c39..6f44937 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -151,6 +151,15 @@ premise does not hold). | WF-24 | internal/workflows models | `internal/workflows/models/request.go:32`, `:63` (and the rest of `AccessRequest`, `Entity`, `ApproverAction`, `ListRequestsResponse`) | Rename any response tag, e.g. `RequestDetails ... \`json:"requestDetails,omitempty"\`` → `json:"ZrequestDetails,omitempty"`, or `ApproverAction.Result \`json:"result"\`` → `json:"Zresult"`. `internal/workflows/models/wire_tags_test.go` pinned only the three request bodies while the SCA twin pinned both directions; every response test decoded a body marshaled from the same struct, so a rename round-tripped and `grant request get` / `list` would render blanks. Not claimed by the PR — recorded because it was found while closing WF-23 | CONFIRMED | test | `TestAccessRequest_DecodesPopulatedResponse` and `TestListRequestsResponse_DecodesPopulatedPage` — 18 tags verified killed (`requestId`, `targetCategory`, `requestState`, `requestResult`, `requestDetails`, `requestApprovers`, `requester`, `createdBy/At`, `updatedBy/At`, `entityId`, `entityName`, `approver`, `result`, `items`, `count`, `totalCount`) | PR8 | done | | WF-25 | internal/workflows models | `internal/workflows/models/form.go` (`FormQuestion`, `Validator`) | Rename any form-metadata tag, e.g. `Validator.Regex \`json:"regex,omitempty"\`` → `json:"Zregex,omitempty"`. Deliberately **not** closed in PR8: this is validation metadata for the interactive `grant request submit` form, one step removed from the user-visible output that WF-24 covers, and pinning it well needs a populated-form fixture rather than another tag list. Recorded so the gap is stated rather than silent | CONFIRMED | test | Follow-up: a populated `RequestFormResponse` decode fixture pinning `requestForms`, `requestForm`, `questions`, `key`, `required`, `valueType`, `valueChoices`, `validators` and the `Validator` fields | — | todo | | ELV-01 | cmd/selection | `cmd/selection.go:78` | `return &items[i], nil` → `return &items[0], nil`. `TestFindItemByDisplay` only checks non-nil/error, so selecting one display value silently elevates the first sorted target and prints a success line naming the wrong one | CONFIRMED | test | `TestFindItemByDisplay_ReturnsMatchingItem` | superseded by the index-based selector fix: `TestResolveSelectionItem` + `TestUIUnifiedSelector_PTY_DuplicateGroupDisplay` | done | +| OUT-30 | cmd/request list JSON | `cmd/output_types.go:103` | `Requests []accessRequestOutput` tag `json:"requests"` → `json:"requestsX"`. Found by adversarial review of PR5 itself: the whole `request list` document was unpinned, because every test unmarshalled into the very struct under test and so was tag-symmetric | CONFIRMED | test | `TestRequestListJSON_Contract` (`assertJSONEqual`; kills OUT-30..31 together) | PR5 (#66) | done | +| OUT-31 | cmd/request list JSON | `cmd/output_types.go:102-105` | Add a spurious field to `accessRequestListOutput`: `Spurious string` tag `json:"spurious"`. An ADDED key is as much a contract change as a renamed one | CONFIRMED | test | `TestRequestListJSON_Contract` | PR5 (#66) | done | +| OUT-32 | cmd/revoke JSON | `cmd/output_types.go:60` | `SessionID` tag `json:"sessionId"` → `json:"sessionIdX"`. Every revoke test unmarshalled into `[]revocationOutput`, so the tags drifted freely | CONFIRMED | test | `TestRevokeJSON_Contract` (kills OUT-32..33 and OUT-38 together; four requested sessions with four different outcomes plus an unattributed row, so the per-row mapping is pinned, not just the envelope) | PR5 (#66) | done | +| OUT-33 | cmd/revoke JSON | `cmd/output_types.go:62` | `Outcome` tag `json:"outcome"` → `json:"outcomeX"`. The weakest point in the JSON surface: `outcome` is the single classification field callers switch on, and CLAUDE.md deliberately refuses to add derived booleans beside it | CONFIRMED | test | `TestRevokeJSON_Contract` | PR5 (#66) | done | +| OUT-34 | cmd/root elevation JSON | `cmd/output_types.go:10` | Drop `omitempty`: `json:"credentials,omitempty"` → `json:"credentials"`. Every fixture was an AWS elevation, so the absent state was never emitted; the key becomes `"credentials": null` for Azure/GCP. `cmd/root_elevate_test.go:2081` cannot catch it — it unmarshals, and `null` still yields `Credentials == nil` | CONFIRMED | test | `TestElevationJSON_AzureOmitsCredentials` — asserts the key is ABSENT, not null | PR5 (#66) | done | +| OUT-35 | cmd/status JSON | `cmd/output_types.go:35` | Drop `omitempty` from `json:"workspaceName,omitempty"`. Every fixture's workspace ID resolved through the name map | CONFIRMED | test | `TestStatusJSON_OmitsAbsentOptionalFields` (kills OUT-35..36 together) | PR5 (#66) | done | +| OUT-36 | cmd/status JSON | `cmd/output_types.go:47` | Drop `omitempty` from `json:"username,omitempty"`. Every fixture token carried a username | CONFIRMED | test | `TestStatusJSON_OmitsAbsentOptionalFields` | PR5 (#66) | done | +| OUT-37 | cmd/list JSON | `cmd/list.go:36` | Drop `omitempty` from `listGroupTarget` `json:"directory,omitempty"`. Same class: every fixture group carried a directory name | CONFIRMED | test | `TestListJSON_OmitsAbsentDirectory` | PR5 (#66) | done | +| OUT-38 | cmd/revoke JSON | `cmd/output_types.go:63` | Drop `omitempty` from `json:"reason,omitempty"`. A confirmed revocation carries no reason, so the key must be absent on the `revoked` row | CONFIRMED | test | `TestRevokeJSON_Contract` | PR5 (#66) | done | | 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 | @@ -337,7 +346,7 @@ extracting a shared helper is a refactor and belongs in its own change. | PR2 — Archive extraction and path security | 12 | 11 | | PR3 — Remaining self-update correctness | 10 | 10 | | PR4 — Argument capture | 42 | 41 | -| PR5 — Output contracts | 33 | 30 | +| PR5 — Output contracts | 42 | 39 | | PR6 — Cache and config semantics | 16 | 16 | | PR7 — UI behavior | 10 | 10 | | PR8 — SCA / workflows / models wire contracts | 39 | 37 |