From 7991dc1ae0f4b6618e659753bd705a6cacc7b751 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Sun, 16 Aug 2026 15:51:31 +0200 Subject: [PATCH] docs: repair merge damage in the mutation ledger Squash-merging seven stacked PRs (#63-#69) that each carried their own copy of the ledger appended both the pre-work and post-work version of many rows. 48 IDs appeared twice inside the ## Ledger table. Collapse them, recompute the summary, and rewrite the rows whose production site no longer exists. --- docs/mutation-ledger.md | 191 +++++++++++++++++----------------------- 1 file changed, 82 insertions(+), 109 deletions(-) diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index 6f44937..a249fdd 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -17,12 +17,15 @@ Then flip **Status** to `done`. **A PR is not complete until every one of its ro is `done`.** `wont-fix` and `refuted` rows are closed by review, not by a test; mark them `done` when the PR that owns them has landed the comment / rationale. -**Line numbers are relative to this branch's tree**, not to `main`. They were first -re-verified against `main` post-`f14e2b9`; the nine `cmd/favorites.go` rows were then -shifted by +13 for the non-interactive guard this branch inserts at -`cmd/favorites.go:148`. Several also drifted from the source reports; those corrections -are noted in the Mutation cell with `(was ...)`. **File:line is always the production -site, never the test site.** +**Line numbers were recorded against the pre-merge work branches and drift as `main` +moves.** They are a locator, not an assertion; when one disagrees with the tree, +the tree wins. The nine `cmd/favorites.go` rows (OUT-02/03/16/17/18/19/20/24/27) +have been re-verified against merged `main` and now carry their current lines — the +earlier +13 shift for the non-interactive guard at `cmd/favorites.go:158` no longer +applies. Rows whose production site was **removed** rather than moved say so in the +File:line cell (ELV-01, UI-02, UI-08). Several sites also drifted from the source +reports; those corrections are noted in the Mutation cell with `(was ...)`. +**File:line is always the production site, never the test site.** **Mutating a call whose only purpose is its side effect.** A bare deletion of `sdkclient.DisableTransientRetry(...)` orphans the `internal/sdkclient` import, so @@ -71,10 +74,10 @@ premise does not hold). | REQ-20 | cmd/login | `cmd/login.go:52` | `if profile == nil {` → `if false {` (auto-configure branch). Feature *is* implemented; `login_test.go` skips it with the factually wrong reason "Auto-configure not yet implemented" — delete the skip | CONFIRMED | test | `TestRunLogin_AutoConfiguresMissingProfile` | PR4 | done | | REQ-21 | cmd/login | `cmd/login.go:74` | `auth.Authenticate(profile, nil, &authmodels.IdsecSecret{Secret: ""}, false, true)` → `..., true, false)` (swap `force`/`refreshAuth`) | CONFIRMED | test | `TestRunLogin_AuthenticateFlags` | PR4 | done | | REQ-22 | cmd/request submit | `cmd/request_submit.go:251` | `if !ui.IsInteractive() {` → `if false {` inside the `roleID == ""` branch | CONFIRMED | test | `TestRunRequestSubmit_NonInteractiveRequiresRoleID` | PR4 | done | -| REQ-23 | cmd integration suite | `cmd/integration_test.go` (harness, not a production site) | No mutation. Claim was "integration tests are absent from CI **and** every assertion accepts a panic". First half confirmed (`.github/workflows/ci.yml` runs `make test-race` / `go test -race`, never `-tags=integration`); second half is too broad — only line 153 accepts any panic; lines 71/125/235 require specific output | OVERSTATED | test | `TestMain` isolation + exact exit-code/error-text assertions; add `-tags=integration` to both CI legs | PR1 | todo | +| 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 | 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-02 | cmd/favorites (interactive) | `cmd/favorites.go:261` | `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:391` | `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 | @@ -87,20 +90,20 @@ premise does not hold). | OUT-13 | cmd/list JSON | `cmd/list.go:167` | `RoleID: t.RoleInfo.ID` → `RoleID: ""`. `roleId` is the field an LLM/automation feeds straight back into `grant request submit --role-id`. Note the verifier's correction: `--target` resolves on the emitted **name**, not `workspaceId` | CONFIRMED | test | `TestListJSON_Contract` + `TestListJSON_RoundTripsToRequestSubmit`. **Corrected after adversarial review:** leg 2 of the round-trip originally stubbed `resolveSubmitTargetFn` with a *textual copy* of the `resolveSubmitTarget` matcher, which pinned nothing in production — mutating `WorkspaceName` → `WorkspaceID` at `cmd/request_submit.go:415` SURVIVED the whole `cmd` suite. The earlier "reverified three ways" claim was wrong: the `Target: t.WorkspaceID` kill came from the ROOT command's `--target` (`findMatchingTarget`, `cmd/root.go:968`), not from `request submit`. Fixed by extracting `matchWorkspaceByName` in `cmd/request_submit.go` (behavior-preserving refactor; no CHANGELOG) and calling it from both production and the test. The `WorkspaceName` → `WorkspaceID` mutation now FAILS the test; reverted, it passes. `RoleID: ""` still dies against both tests | PR5 | done | | OUT-14 | cmd/list JSON | `cmd/list.go:175` | `GroupID: g.GroupID` → `GroupID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | | OUT-15 | cmd/list JSON | `cmd/list.go:176` | `DirectoryID: g.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestListJSON_Contract` | PR5 | done | -| OUT-16 | cmd/favorites JSON | `cmd/favorites.go:462` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | done | -| OUT-17 | cmd/favorites JSON | `cmd/favorites.go:464` | `Role: entry.Role` → `Role: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | done | -| OUT-18 | cmd/favorites JSON | `cmd/favorites.go:466` | `DirectoryID: entry.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | done | -| OUT-19 | cmd/favorites | `cmd/favorites.go:333` | `fav.Provider = cfg.DefaultProvider` → `fav.Provider = "azure"`. Every command test uses the azure default, so a non-default `DefaultProvider` (aws/gcp) is unpinned. Secondary, same defect class: `internal/config/favorites.go:21-22` independently defaults empty → `"azure"` | CONFIRMED | test | `TestFavoritesAdd_HonorsNonDefaultProvider` | PR5 | done | +| OUT-16 | cmd/favorites JSON | `cmd/favorites.go:465` | `Provider: entry.Provider` → `Provider: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` (`assertJSONEqual`) | PR5 | done | +| OUT-17 | cmd/favorites JSON | `cmd/favorites.go:467` | `Role: entry.Role` → `Role: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | done | +| OUT-18 | cmd/favorites JSON | `cmd/favorites.go:469` | `DirectoryID: entry.DirectoryID` → `DirectoryID: ""` | CONFIRMED | test | `TestFavoritesListJSON_Contract` | PR5 | done | +| OUT-19 | cmd/favorites | `cmd/favorites.go:336` | `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-24 | cmd/favorites | `cmd/favorites.go:435-437` | Delete the `if len(args) > 1 { return fmt.Errorf("expected 1 favorite name, got %d", len(args)) }` arity check. Without it `favorites remove first second` silently removes `first` | CONFIRMED | test | `TestFavoritesRemove_RejectsExtraArgs` | PR5 | done | | OUT-25 | cmd/list flags | `cmd/list.go:71` | Delete `cmd.Flags().Bool("refresh", ...)` registration. **Verifier correction:** `grant list --refresh` is **not** already a no-op — `list.go:91-92` reads it and passes it into `buildCachedLister`, and CLAUDE.md is correct. The real finding is missing flag-registration/wiring coverage | OVERSTATED | test | `TestListCommand_RefreshFlagRegistered` (renamed: the cache wiring sits behind `bootstrapSCAService`, so a unit test covers registration and parsing only, not a cache bypass) | PR5 | done | -| OUT-26 | cmd test isolation | `cmd/favorites_test.go` → production `bootstrapImpl` (`cmd/root.go:158`) | No production mutation. A passing unit test (`TestFavoritesAddCommand/add_duplicate_favorite_name`) reaches the **real** `~/.idsec` profile and keyring; it accepts any error, so keyring access or an SDK auth attempt counts as success. The exact `Identity Security Platform Secret` prompt was **not** reproduced, even under a PTY; `ui.IsTerminalFunc` does not guard this because it runs after `bootstrapSCAService()` | OVERSTATED | prod-fix | `TestMain` env redirect + `AssertSandboxed`; `favorites add` early non-interactive guard with a favorites-specific message | PR1 | todo | -| OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | test | `TestFavoritesAddInteractive_ProviderFlagWinsOverTargetCSP` — added anyway in PR5: the old kill was incidental (an unrelated error-path assertion). Direct reverification needs a target whose CSP differs from the flag, so the fixture pairs `--provider azure` with an AWS target — a combination production filtering would never emit. The test comment now labels it deliberately impossible rather than merely "distinguishable" | PR5 | done | -| OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | -| OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | +| OUT-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:250-251` | `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 | — | done | +| 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 | — | done | | SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | done | | SCA-02 | internal/sca | `internal/sca/service.go:208` | `s.httpClient.Post(ctx, "/api/access/elevate", req)` → `..., nil)` | CONFIRMED | test | `TestElevate_SendsExactRouteAndBody` (added `gotRoute`/`gotBody` to `mockHTTPClient`) — one test, shared with SCA-05 | PR8 | done | | SCA-03 | internal/sca | `internal/sca/service.go:236` | `s.httpClient.Post(ctx, "/api/access/sessions/revoke", req)` → `..., nil)` | CONFIRMED | test | `TestRevokeSessions_SendsExactRouteAndBody` — one test, shared with SCA-07 | PR8 | done | @@ -150,7 +153,7 @@ premise does not hold). | SCA-24 | internal/sca models | `internal/sca/models/groups.go:41` | `CSP CSP \`json:"csp"\`` → `json:"Zcsp"` on `GroupsElevateResponse` | CONFIRMED | test | `TestGroupsElevateResponse_DecodesPopulatedResult` | PR8 | done | | WF-24 | internal/workflows models | `internal/workflows/models/request.go:32`, `:63` (and the rest of `AccessRequest`, `Entity`, `ApproverAction`, `ListRequestsResponse`) | Rename any response tag, e.g. `RequestDetails ... \`json:"requestDetails,omitempty"\`` → `json:"ZrequestDetails,omitempty"`, or `ApproverAction.Result \`json:"result"\`` → `json:"Zresult"`. `internal/workflows/models/wire_tags_test.go` pinned only the three request bodies while the SCA twin pinned both directions; every response test decoded a body marshaled from the same struct, so a rename round-tripped and `grant request get` / `list` would render blanks. Not claimed by the PR — recorded because it was found while closing WF-23 | CONFIRMED | test | `TestAccessRequest_DecodesPopulatedResponse` and `TestListRequestsResponse_DecodesPopulatedPage` — 18 tags verified killed (`requestId`, `targetCategory`, `requestState`, `requestResult`, `requestDetails`, `requestApprovers`, `requester`, `createdBy/At`, `updatedBy/At`, `entityId`, `entityName`, `approver`, `result`, `items`, `count`, `totalCount`) | PR8 | done | | WF-25 | internal/workflows models | `internal/workflows/models/form.go` (`FormQuestion`, `Validator`) | Rename any form-metadata tag, e.g. `Validator.Regex \`json:"regex,omitempty"\`` → `json:"Zregex,omitempty"`. Deliberately **not** closed in PR8: this is validation metadata for the interactive `grant request submit` form, one step removed from the user-visible output that WF-24 covers, and pinning it well needs a populated-form fixture rather than another tag list. Recorded so the gap is stated rather than silent | CONFIRMED | test | Follow-up: a populated `RequestFormResponse` decode fixture pinning `requestForms`, `requestForm`, `questions`, `key`, `required`, `valueType`, `valueChoices`, `validators` and the `Validator` fields | — | todo | -| ELV-01 | cmd/selection | `cmd/selection.go:78` | `return &items[i], nil` → `return &items[0], nil`. `TestFindItemByDisplay` only checks non-nil/error, so selecting one display value silently elevates the first sorted target and prints a success line naming the wrong one | CONFIRMED | test | `TestFindItemByDisplay_ReturnsMatchingItem` | superseded by the index-based selector fix: `TestResolveSelectionItem` + `TestUIUnifiedSelector_PTY_DuplicateGroupDisplay` | done | +| ELV-01 | cmd/selection | `cmd/selection.go:82-87` (`resolveSelectionItem`; the original site, `findItemByDisplay` at the old `cmd/selection.go:78`, **no longer exists**) | Original: `return &items[i], nil` → `return &items[0], nil` in `findItemByDisplay`. `TestFindItemByDisplay` only checked non-nil/error, so selecting one display value silently elevated the first sorted target and printed a success line naming the wrong one. **Superseded:** PR7 (#68) replaced display-text lookup with index resolution, so the equivalent mutation today is clamping the index in `resolveSelectionItem` (`return &sorted[0], nil` instead of `&sorted[idx]`) | CONFIRMED | test | `TestResolveSelectionItem` and `TestResolveSelectionItem_EmptySlice` (`cmd/selection_test.go`) pin the resolver's bounds and mapping; `TestUIUnifiedSelector_PTY_DuplicateGroupDisplay` (`cmd/selection_pty_linux_test.go`) pins the wiring over a real pty. `TestFindItemByDisplay_ReturnsMatchingItem` was never added and cannot be — the function is gone | PR7 (#68) | 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 | @@ -209,54 +212,6 @@ premise does not hold). | SFU-21 | internal/selfupdate | `internal/selfupdate/selfupdate.go:437` | Delete `if uint64(len(data)) != f.UncompressedSize64 { ... }` (zip truncation cross-check). Same unreachability argument as SFU-20 | CONFIRMED | wont-fix | none — defense-in-depth, no coverage claimed | PR2 | done | | SFU-22 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389` vs `:430` | tar/zip size-check asymmetry. The original "zip decompression bomb" framing is **overstated**: the structural asymmetry is real (`maxDownloadBytes=10`, 5000-byte decoy → `TAR bytes=0 err=` vs `ZIP bytes=3 err=`), but `zip.NewReader` parses only the central directory and never opens skipped entries. The tar guard is load-bearing; the zip placement is a consistency point, not a vulnerability | OVERSTATED | wont-fix | Pin the asymmetry as **intentional** with a comment and a test asserting a skipped zip entry is never inflated | PR2 | done | | SFU-23 | internal/selfupdate | `internal/selfupdate/selfupdate.go:441` | `extractFromZip` filtered only on `f.FileInfo().IsDir()`, with no non-regular guard at all. A zip entry carrying `fs.ModeSymlink` named `grant.exe` was accepted **unmutated, in production**: probe returned `bytes=11 body="/etc/passwd" err=`. Not exploitable — extraction is in-memory, the link is never followed, and the bytes are checksum-gated and attacker-chosen either way — so the impact is only that grant installs a file whose content is the link-target string. It was an undocumented tar/zip asymmetry | CONFIRMED | test + prod-fix | **Production:** `if f.Mode()&fs.ModeType != 0 \|\| !isBinaryEntry(f.Name)`, mirroring the tar `Typeflag` guard. **Test:** `TestExtractBinaryRejectsNonRegularEntries/zip_symlink-mode_entry_named_grant`. Reverting the guard fails it behaviourally (`expected rejection, got 11 bytes`) | PR2 | done | -| CACHE-01 | internal/cache | `internal/cache/cached_eligibility.go:84` | When `c.refresh` is true, skip the write: guard `Set(c.store, key, *resp)` with `if !c.refresh`. `--refresh` must bypass the **read** but still **write** | CONFIRMED | test | `TestCachedEligibility_RefreshStillWrites` | PR6 | todo | -| CACHE-02 | internal/cache | `internal/cache/cached_eligibility.go:117` | Same mutation on the groups-eligibility write | CONFIRMED | test | `TestCachedGroupsEligibility_RefreshStillWrites` | PR6 | todo | -| CACHE-03 | internal/cache | `internal/cache/cached_roles.go:53` | Same mutation on the on-demand-roles write | CONFIRMED | test | `TestCachedRoles_RefreshStillWrites` | PR6 | todo | -| CACHE-04 | internal/cache | `internal/cache/cache.go:39-41` | `if err := json.Unmarshal(data, &e); err != nil { return false }` → ignore the error and fall through. `TestGet_CorruptJSON` passes today via the zero-`CachedAt` TTL branch, not the unmarshal guard | CONFIRMED | test | Fix `TestGet_CorruptJSON` to use a **fresh** `cached_at` with a type-mismatched payload, so only the unmarshal guard can produce the miss | PR6 | todo | -| CACHE-05 | internal/cache | `internal/cache/cached_eligibility.go:131` | `"groups_eligibility_" + ...` → `"eligibility_" + ...` (key collision with the cloud-eligibility prefix) | CONFIRMED | test | `TestCacheKeys_DistinctPrefixes` | PR6 | todo | -| CACHE-06 | internal/cache | `internal/cache/cached_eligibility.go:127` and `:131` | Drop `strings.ToLower(string(csp))` from both key builders | CONFIRMED | test | `TestCacheKeys_LowercaseCSP` | PR6 | todo | -| CACHE-07 | internal/cache | `internal/cache/session_tracker.go:14` | `const maxSessionAge = 24 * time.Hour` → `25 * time.Hour`. No test pins either value; code says 24h and CLAUDE.md says 25h | CONFIRMED | test + prod-fix | `TestSessionTimestamps_RetentionBoundary`. **Production:** rename to `sessionTimestampRetention` (keep 24h) with a comment stating it is local retention for remaining-time display — not a session limit or access-control boundary. Also fix the factually wrong "removed on cleanup" comment: `CleanupSessions` filters on active IDs and never reads it. Drop the 25h claim from CLAUDE.md | PR6 | todo | -| CFG-01 | internal/config | `internal/config/config.go:54-58` | In `Load`, return the default config for **any** read error: `if err != nil { return DefaultConfig(), nil }`. Killed on Linux by `config_test.go:197`, but `config_test.go:184-186` **skips on Windows**, so there is zero coverage on the windows-latest leg | CONFIRMED | test | Portable replacement: `Load()` — errors as EISDIR on POSIX / ERROR_ACCESS_DENIED on Windows, and `errors.Is(err, os.ErrNotExist)` is false on both. (Windows half reasoned, not measured — verify on the CI leg) | PR6 | todo | -| CFG-02 | internal/config | `internal/config/config.go:110-113` | Add a `d <= 0` rejection to `ParseCacheTTL` — **it also survives**, i.e. the tests are blind in both directions. There is no negative/zero-TTL table row at all | CONFIRMED | test + prod-fix | `TestParseCacheTTL` rows for `0s`, `-5m`, `garbage`. **Production:** `ParseCacheTTL` returns `(time.Duration, error)`; empty → default, any explicitly-supplied invalid value (unparseable **or** non-positive) → error. Validate at config load. Ripple: `buildCachedLister` (`cmd/root.go:242`, seven call sites) + `cmd/request_submit.go:541`. CHANGELOG `### Changed` | PR6 | todo | -| CFG-03 | internal/config | `internal/config/config.go:20` | `const DefaultCacheTTL = 4 * time.Hour` → `400 * time.Hour`. The existing assertion is the tautology `want: DefaultCacheTTL` | CONFIRMED | test | `TestParseCacheTTL_DefaultIsFourHours` (assert the literal `4 * time.Hour`) | PR6 | todo | -| CFG-04 | internal/config | `internal/config/config.go:60` | `cfg := DefaultConfig()` → `cfg := &Config{}` (defaults no longer survive a partial YAML file) | CONFIRMED | test | `TestLoad_PartialYAMLKeepsDefaults` | PR6 | todo | -| CFG-05 | internal/config | `internal/config/config.go:65-67` | Delete `if cfg.Favorites == nil { cfg.Favorites = make(map[string]Favorite) }` | CONFIRMED | test | `TestLoad_FavoritesNeverNil` | PR6 | todo | -| CFG-06 | internal/config | `internal/config/config.go:61-63` | Swallow the YAML error: `_ = yaml.Unmarshal(data, cfg)` | CONFIRMED | test | `TestLoad_InvalidYAMLErrors` | PR6 | todo | -| CFG-07 | internal/config | `internal/config/config.go:103` | `filepath.Join(home, ".grant")` → `".grantx"` | CONFIRMED | test | `TestConfigDir_EndsInDotGrant` | PR6 | todo | -| CFG-08 | internal/config | `internal/config/config.go:84` | `os.WriteFile(path, data, 0o600)` → `0o644` | CONFIRMED | test | `TestSave_FileMode` with the `runtime.GOOS == "windows"` skip | PR6 | todo | -| CFG-09 | internal/config | `internal/config/config.go:75` | `if err := os.MkdirAll(dir, 0o700); err != nil { ... }` → ignore the error | CONFIRMED | test | `TestSave_MkdirAllFailure` — force it portably by pointing at a path whose parent component is an existing **regular file** (ENOTDIR / ERROR_DIRECTORY), never a hardcoded `/dev/null/...` | PR6 | todo | -| UI-01 | internal/ui | `internal/ui/tty.go:18` | `return IsTerminalFunc(os.Stdin.Fd())` → `IsTerminalFunc(os.Stdout.Fd())`. This swap is what makes `grant revoke < /dev/null` hang in a terminal. All twelve prompt-level guards are well covered (8 spot-checked, all killed with `errors.Is` + flag hints); `IsInteractive()` itself is not, because every stub ignores `fd` | CONFIRMED | test | `TestIsInteractive_ChecksStdinFd` — a stub that records the fd and asserts `os.Stdin.Fd()` | PR7 | done | -| UI-02 | internal/ui | `internal/ui/group_selector.go:55` | Delete the `sort.SliceStable(sorted, ...)` call in `sortGroupsForDisplay` (orphans nothing — `sort` stays live via `BuildGroupOptions`) | CONFIRMED | test + prod-fix | Extract `sortGroupsForDisplay` so **list ordering** is testable without a TTY; `TestSortGroupsForDisplay_Ordering`. This row guards the order options are rendered in, and nothing else: sorting a copy never prevented wrong-group selection, it only made a display collision resolve consistently to the *first* matching row. Which group a selection denotes is `resolveGroupSelection`'s job (see the `SelectGroup` index fix in the production-changes table below) | PR7 | done | -| UI-03 | internal/ui | `internal/ui/session_selector.go:57` | `if remaining <= 0 {` → `if remaining < 0 {`. The fixture only supplies `-5m` (`session_selector_test.go:154`), so exactly-zero is unpinned | CONFIRMED | test | `TestFormatSessionOption_ExactlyZeroRemaining` | PR7 | done | -| UI-04 | internal/ui | `internal/ui/request_selector.go:18` | Delete the `time.Parse(time.RFC3339Nano, ts)` branch in the timestamp formatter | CONFIRMED | test | `TestFormatRequestOption_RFC3339Nano` | PR7 | done | -| UI-05 | internal/ui | `internal/ui/role_selector.go:36` | Make the role sort case-**sensitive** (drop the `strings.ToLower` normalization in the `sort.SliceStable` less-func). Note: the raw mutation orphans the `strings` import — remove it too | CONFIRMED | test | `TestBuildRoleOptions_MixedCaseSort` — the sort lives in `BuildRoleOptions`; no `sortRolesForDisplay` helper exists or was needed | PR7 | done | -| UI-06 | internal/ui | `internal/ui/selector.go:107` | Delete the `if len(targets) == 0` guard in `SelectTarget` (`:49` is the identically-worded guard inside `BuildOptions`, and mutating *that* one is an **equivalent mutant** — `make([]string, 0)` + `sort.Strings` yields the same empty non-nil slice — so it is not an escape). (`SelectRole`/`SelectRequest` equivalents are **killed**; these three are not) | CONFIRMED | test | `TestSelectTarget_EmptyList`; guard **order** (non-interactive first) is pinned separately by `TestSelectTarget_NonTTYEmptyList` (mutation also orphans the `errors` import — removed so the package compiles; survey then fails with "please provide options to select from") | PR7 | done | -| UI-07 | internal/ui | `internal/ui/session_selector.go:112` | Delete the `if len(sessions) == 0` guard in `SelectSessions` | CONFIRMED | test | `TestSelectSessions_EmptyList`; guard **order** pinned by `TestSelectSessions_NonTTYEmptyList` (the `errors` import stays live via "no sessions selected"; survey fails with "please provide options to select from") | PR7 | done | -| UI-08 | internal/ui | `internal/ui/group_selector.go:79` | Delete the `if len(groups) == 0` guard in `SelectGroup`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | `TestSelectGroup_EmptyList`; guard **order** pinned by `TestSelectGroup_NonTTYEmptyList` | PR7 | done | -| UI-09 | internal/ui | `internal/ui/role_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRole` | CONFIRMED | wont-fix | none — defensive-only and unreachable through `survey`, which can only return a string it was given. Closed as **wont-fix in PR7**: the guard stays in place, deliberately uncovered; do not chase it | PR7 | wont-fix (closed) | -| UI-10 | internal/ui | `internal/ui/request_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRequest` | CONFIRMED | wont-fix | none — same rationale as UI-09. Closed as **wont-fix in PR7**: the guard stays in place, deliberately uncovered; do not chase it | PR7 | wont-fix (closed) | -| SFU-01 | internal/selfupdate | `internal/selfupdate/selfupdate.go:345` | `case path.IsAbs(cleaned):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath` — one guard-specific `wantErrContains` per arm, with a valid `grant` entry beside each malicious one so the "no binary" fallback cannot be the reason for the error | PR2 | todo | -| SFU-02 | internal/selfupdate | `internal/selfupdate/selfupdate.go:347` | `case strings.HasPrefix(normalized, "//"):` → `case false:`. **Production change (PR2):** move this arm *before* `path.IsAbs` — `path.Clean` collapses `//host/share/x` → `/host/share/x`, so `IsAbs` always wins and the UNC arm is unreachable. Rejection is unchanged; only the message differs | CONFIRMED | test + prod-fix | `TestCheckArchivePath/unc_path` | PR2 | todo | -| SFU-03 | internal/selfupdate | `internal/selfupdate/selfupdate.go:349` | `case hasDriveLetter(normalized):` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/drive_absolute` | PR2 | todo | -| SFU-04 | internal/selfupdate | `internal/selfupdate/selfupdate.go:343` | `case name == "":` → `case false:` | CONFIRMED | test | `TestCheckArchivePath/empty_name` | PR2 | todo | -| SFU-05 | internal/selfupdate | `internal/selfupdate/selfupdate.go:339` | `normalized := strings.ReplaceAll(name, "\\", "/")` → `normalized := name` (backslash traversal and backslash UNC then slip through) | CONFIRMED | test | `TestCheckArchivePath/backslash_traversal`, `.../backslash_unc` | PR2 | todo | -| SFU-06 | internal/selfupdate | `internal/selfupdate/selfupdate.go:359` (`hasDriveLetter`) | Narrow the check to uppercase drive letters only, so lowercase `c:\...` passes | CONFIRMED | test | `TestCheckArchivePath/lowercase_drive`, `.../forward_slash_drive` | PR2 | todo | -| SFU-07 | internal/selfupdate | `internal/selfupdate/selfupdate.go:392` | `if hdr.Typeflag != tar.TypeReg \|\| !isBinaryEntry(hdr.Name) {` → drop the `hdr.Typeflag != tar.TypeReg` operand. Probe (`Typeflag: tar.TypeSymlink, Name: "grant", Size: 0, Linkname: "/etc/passwd"`): baseline `bytes=0 err=archive does not contain a grant binary`; mutated `bytes=0 err=` — i.e. a **zero-byte self-destruct**. No symlink case exists anywhere in the package | CONFIRMED | test + prod-fix | `TestExtractBinary_RejectsNonRegularEntries` (symlink / hardlink / directory named `grant`, plus a zip directory entry). **Production:** reject a zero-length extracted binary *and* add a second non-empty check at the apply boundary (`internal/selfupdate/apply.go:50`). CHANGELOG `### Security` | PR2 | todo | -| SFU-08 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389-391` | Delete the tar declared-size guard (`if hdr.Size > maxDownloadBytes`) | CONFIRMED | test | `TestExtractFromTarGz_RejectsOversizeDecoy` — oversized **decoy** beside a valid binary | PR2 | todo | -| SFU-09 | internal/selfupdate | `internal/selfupdate/selfupdate.go:430-432` | Delete the zip declared-size guard (`if maxDownloadBytes >= 0 && f.UncompressedSize64 > uint64(maxDownloadBytes)`) | CONFIRMED | test | `TestExtractFromZip_RejectsOversizeDecoy` | PR2 | todo | -| SFU-10 | internal/selfupdate | `internal/selfupdate/apply.go:74` | Delete the `if err := syncStagedFile(target); err != nil { ... }` call. Per the consistency review this is **not** a production gap — `applyWithOptions` already returns a wrapped sync error before commit and `syncStagedFile` (`:110`) already returns `f.Sync()` errors. Scope is a seam plus tests; **no CHANGELOG entry** | CONFIRMED | test | `TestApplyWithOptions_SyncsBeforeCommit` via a `syncStagedFileFn` seam (call-order + abort-before-commit) | PR3 | todo | -| SFU-11 | internal/selfupdate | `internal/selfupdate/apply.go:110-115` | In `syncStagedFile`, ignore the `f.Sync()` error: `_ = f.Sync(); return nil` | CONFIRMED | test | `TestApplyWithOptions_AbortsOnSyncError` | PR3 | todo | -| SFU-12 | internal/selfupdate | `internal/selfupdate/apply.go:154-156` | In `InterruptedUpdate`, delete the target-exists guard (`if _, err := os.Stat(targetPath); err == nil \|\| !errors.Is(err, os.ErrNotExist) { return "", false }`). The untested case is target **present** and `.old` present — the documented Windows steady state | CONFIRMED | test | `TestInterruptedUpdate_TargetPresentWithOldBackup` | PR3 | todo | -| SFU-13 | internal/selfupdate | `internal/selfupdate/selfupdate.go:197-199` | Delete the non-200 check `if resp.StatusCode != http.StatusOK { ... }` in `fetchLatestRelease` | CONFIRMED | test | `newFixtureServerWith(t, opts)` → `TestFetchLatestRelease_Non200` | PR3 | todo | -| SFU-14 | internal/selfupdate | `internal/selfupdate/selfupdate.go:202-204` | In `fetchLatestRelease`, swallow the `json.Unmarshal` error on an empty body: `_ = json.Unmarshal(body, &rel)` | CONFIRMED | test | `TestFetchLatestRelease_EmptyBody` | PR3 | todo | -| SFU-15 | internal/selfupdate | `internal/selfupdate/selfupdate.go:205-207` | Delete `if rel.TagName == "" { return nil, errors.New("GitHub release response has no tag_name") }` | CONFIRMED | test | `TestFetchLatestRelease_EmptyTagName`; also non-200 on the **asset** and **checksums** downloads (`:229`) | PR3 | todo | -| SFU-16 | internal/selfupdate | `internal/selfupdate/version.go` (`comparePreRelease`, numeric-vs-numeric branch) | Invert the numeric-vs-numeric comparison so `rc.10` sorts before `rc.2` | CONFIRMED | test | `TestCompareVersions_NumericPrereleaseOrdering` (`rc.10` vs `rc.2`) | PR3 | todo | -| SFU-17 | internal/selfupdate | `internal/selfupdate/version.go:194` | `if !isAllDigits(part) {` → `if false {` in the core `MAJOR.MINOR.PATCH` loop, so `"1.+5.3"` is accepted | CONFIRMED | test | `TestParseVersion/invalid_core_segment` (`"1.+5.3"`) | PR3 | todo | -| SFU-18 | internal/selfupdate | `internal/selfupdate/selfupdate.go:284-287` | `if len(fields) != 2 { return fmt.Errorf("malformed line in %s: %q", ...) }` → `continue` | CONFIRMED | test | `TestVerifyChecksum_MalformedLine` | PR3 | todo | -| SFU-19 | internal/selfupdate | `internal/selfupdate/selfupdate.go:316-317` | In `extractBinary`, replace the `default:` unsupported-format error with `return extractFromTarGz(archive)` | CONFIRMED | test | `TestExtractBinary_UnsupportedFormat` | PR3 | todo | -| SFU-20 | internal/selfupdate | `internal/selfupdate/selfupdate.go:402` | Delete `if int64(len(data)) != hdr.Size { ... }` (tar truncation cross-check). **Unreachable by construction**: a successful capped read returns exactly `hdr.Size`, and earlier exhaustion returns `io.ErrUnexpectedEOF`. Hand-patched proof: header declares 40 with body `"bin"` → `bytes=40 err=`; header declares 2000 → `bytes=0 err=... unexpected EOF` | CONFIRMED | wont-fix | none — keep as defense-in-depth, comment it as unreachable, and claim no coverage. Rename `TestExtractBinaryRejectsTruncatedEntry` → `...TruncatedArchive` | PR2 | todo | -| SFU-21 | internal/selfupdate | `internal/selfupdate/selfupdate.go:437` | Delete `if uint64(len(data)) != f.UncompressedSize64 { ... }` (zip truncation cross-check). Same unreachability argument as SFU-20 | CONFIRMED | wont-fix | none — defense-in-depth, no coverage claimed | PR2 | todo | -| SFU-22 | internal/selfupdate | `internal/selfupdate/selfupdate.go:389` vs `:430` | tar/zip size-check asymmetry. The original "zip decompression bomb" framing is **overstated**: the structural asymmetry is real (`maxDownloadBytes=10`, 5000-byte decoy → `TAR bytes=0 err=` vs `ZIP bytes=3 err=`), but `zip.NewReader` parses only the central directory and never opens skipped entries. The tar guard is load-bearing; the zip placement is a consistency point, not a vulnerability | OVERSTATED | wont-fix | Pin the asymmetry as **intentional** with a comment and a test asserting a skipped zip entry is never inflated | PR2 | todo | | CACHE-01 | internal/cache | `internal/cache/cached_eligibility.go:84` | When `c.refresh` is true, skip the write: guard `Set(c.store, key, *resp)` with `if !c.refresh`. `--refresh` must bypass the **read** but still **write** | CONFIRMED | test | `TestCachedEligibility_RefreshStillWrites` | PR6 | done | | CACHE-02 | internal/cache | `internal/cache/cached_eligibility.go:117` | Same mutation on the groups-eligibility write | CONFIRMED | test | `TestCachedGroupsEligibility_RefreshStillWrites` | PR6 | done | | CACHE-03 | internal/cache | `internal/cache/cached_roles.go:53` | Same mutation on the on-demand-roles write | CONFIRMED | test | `TestCachedRoles_RefreshStillWrites` | PR6 | done | @@ -273,16 +228,16 @@ premise does not hold). | CFG-07 | internal/config | `internal/config/config.go:103` | `filepath.Join(home, ".grant")` → `".grantx"` | CONFIRMED | test | `TestConfigDir_EndsInDotGrant` | PR6 | done | | CFG-08 | internal/config | `internal/config/config.go:84` | `os.WriteFile(path, data, 0o600)` → `0o644` | CONFIRMED | test | `TestSave_FileAndDirModes` (file 0600 and dir 0700) with the `runtime.GOOS == "windows"` skip | PR6 | done | | CFG-09 | internal/config | `internal/config/config.go:75` | `if err := os.MkdirAll(dir, 0o700); err != nil { ... }` → ignore the error | CONFIRMED | test | `TestSave_MkdirAllFailure` — force it portably by pointing at a path whose parent component is an existing **regular file**, never a hardcoded `/dev/null/...`. `os.MkdirAll` (`os/path.go`) stats the parent itself and synthesizes `&PathError{Op: "mkdir", Err: syscall.ENOTDIR}` in platform-independent Go, so it is ENOTDIR on **both** platforms — no Windows error code is involved, which is also why the `Op` assertion is portable. A bare "did it error" check does **not** kill this: with the error swallowed the later `WriteFile` fails for the same reason, so the test asserts the `*fs.PathError` `Op` is `mkdir` | PR6 | done | -| UI-01 | internal/ui | `internal/ui/tty.go:18` | `return IsTerminalFunc(os.Stdin.Fd())` → `IsTerminalFunc(os.Stdout.Fd())`. This swap is what makes `grant revoke < /dev/null` hang in a terminal. All twelve prompt-level guards are well covered (8 spot-checked, all killed with `errors.Is` + flag hints); `IsInteractive()` itself is not, because every stub ignores `fd` | CONFIRMED | test | `TestIsInteractive_ChecksStdinFd` — a stub that records the fd and asserts `os.Stdin.Fd()` | PR7 | todo | -| UI-02 | internal/ui | `internal/ui/group_selector.go:60` | Delete the `sort.Slice(sorted, ...)` call in the group selector | CONFIRMED | test + prod-fix | Extract `sortGroupsForDisplay` so ordering is testable without a TTY; `TestSortGroupsForDisplay_CollisionOrdering` | PR7 | todo | -| UI-03 | internal/ui | `internal/ui/session_selector.go:57` | `if remaining <= 0 {` → `if remaining < 0 {`. The fixture only supplies `-5m` (`session_selector_test.go:154`), so exactly-zero is unpinned | CONFIRMED | test | `TestFormatSessionOption_ExactlyZeroRemaining` | PR7 | todo | -| UI-04 | internal/ui | `internal/ui/request_selector.go:18` | Delete the `time.Parse(time.RFC3339Nano, ts)` branch in the timestamp formatter | CONFIRMED | test | `TestFormatRequestOption_RFC3339Nano` | PR7 | todo | -| UI-05 | internal/ui | `internal/ui/role_selector.go:36` | Make the role sort case-**sensitive** (drop the `strings.ToLower` normalization in the `sort.SliceStable` less-func). Note: the raw mutation orphans the `strings` import — remove it too | CONFIRMED | test | `TestSortRolesForDisplay_MixedCase` | PR7 | todo | -| UI-06 | internal/ui | `internal/ui/selector.go:49` | Delete the `if len(targets) == 0` guard in `SelectTarget`. (`SelectRole`/`SelectRequest` equivalents are **killed**; these three are not) | CONFIRMED | test | `TestSelectTarget_EmptyList` | PR7 | todo | -| UI-07 | internal/ui | `internal/ui/session_selector.go:112` | Delete the `if len(sessions) == 0` guard in `SelectSessions` | CONFIRMED | test | `TestSelectSessions_EmptyList` | PR7 | todo | -| UI-08 | internal/ui | `internal/ui/group_selector.go:54` | Delete the `if len(groups) == 0` guard in `SelectGroup`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | `TestSelectGroup_EmptyList` | PR7 | todo | -| UI-09 | internal/ui | `internal/ui/role_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRole` | CONFIRMED | wont-fix | none — defensive-only and unreachable through `survey`, which can only return a string it was given | PR7 | todo | -| UI-10 | internal/ui | `internal/ui/request_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRequest` | CONFIRMED | wont-fix | none — same rationale as UI-09 | PR7 | todo | +| UI-01 | internal/ui | `internal/ui/tty.go:18` | `return IsTerminalFunc(os.Stdin.Fd())` → `IsTerminalFunc(os.Stdout.Fd())`. This swap is what makes `grant revoke < /dev/null` hang in a terminal. All twelve prompt-level guards are well covered (8 spot-checked, all killed with `errors.Is` + flag hints); `IsInteractive()` itself is not, because every stub ignores `fd` | CONFIRMED | test | `TestIsInteractive_ChecksStdinFd` — a stub that records the fd and asserts `os.Stdin.Fd()` | PR7 | done | +| UI-02 | internal/ui, cmd/selection | `internal/ui/selector.go:69` (`sortTargetsForDisplay`) and `cmd/selection.go:62` (`buildUnifiedOptions`). **The original site is gone:** the mutation was first recorded against `sort.SliceStable` in `sortGroupsForDisplay` (`internal/ui/group_selector.go`), and PR7 (#68) deleted `SelectGroup`, `sortGroupsForDisplay` and `resolveGroupSelection` outright as having zero production callers | Delete the `sort.SliceStable(sorted, ...)` call at either surviving site, or weaken it to `sort.Slice` (the stability mutant needs a ≥13-entry colliding fixture; at n=4 Go's pdqsort falls back to insertion sort and is incidentally stable) | CONFIRMED | test + prod-fix | `TestSortTargetsForDisplay_Ordering` and `TestSortTargetsForDisplay_StableAmongCollisions` (`internal/ui/selector_test.go`); `TestBuildUnifiedOptions` and `TestBuildUnifiedOptions_StableAmongCollisions` (`cmd/selection_test.go`). The one group-side sort that survives the deletion is `sort.Strings(options)` in `BuildGroupOptions` (`internal/ui/group_selector.go:29`, reached from `cmd/list.go:202`), pinned by `TestBuildGroupOptions` and `TestBuildGroupOptions_DuplicateDisplayStrings`. This row guards the order options are **rendered** in and nothing else; which item a selection denotes is `resolveSelectionItem`'s job (ELV-01) | PR7 (#68) | done | +| UI-03 | internal/ui | `internal/ui/session_selector.go:57` | `if remaining <= 0 {` → `if remaining < 0 {`. The fixture only supplies `-5m` (`session_selector_test.go:154`), so exactly-zero is unpinned | CONFIRMED | test | `TestFormatSessionOption_ExactlyZeroRemaining` | PR7 | done | +| UI-04 | internal/ui | `internal/ui/request_selector.go:18` | Delete the `time.Parse(time.RFC3339Nano, ts)` branch in the timestamp formatter | CONFIRMED | test | `TestFormatRequestOption_RFC3339Nano` | PR7 | done | +| UI-05 | internal/ui | `internal/ui/role_selector.go:36` | Make the role sort case-**sensitive** (drop the `strings.ToLower` normalization in the `sort.SliceStable` less-func). Note: the raw mutation orphans the `strings` import — remove it too | CONFIRMED | test | `TestBuildRoleOptions_MixedCaseSort` — the sort lives in `BuildRoleOptions`; no `sortRolesForDisplay` helper exists or was needed | PR7 | done | +| UI-06 | internal/ui | `internal/ui/selector.go:95` | Delete the `if len(targets) == 0` guard in `SelectTarget` (`internal/ui/selector.go:49` is the identically-worded guard inside `BuildOptions`, and mutating *that* one is an **equivalent mutant** — `make([]string, 0)` + `sort.Strings` yields the same empty non-nil slice — so it is not an escape). (`SelectRole`/`SelectRequest` equivalents are **killed**; these three are not) | CONFIRMED | test | `TestSelectTarget_EmptyList`; guard **order** (non-interactive first) is pinned separately by `TestSelectTarget_NonTTYEmptyList` (mutation also orphans the `errors` import — removed so the package compiles; survey then fails with "please provide options to select from") | PR7 | done | +| UI-07 | internal/ui | `internal/ui/session_selector.go:122` | Delete the `if len(sessions) == 0` guard in `SelectSessions` | CONFIRMED | test | `TestSelectSessions_EmptyList`; guard **order** pinned by `TestSelectSessions_NonTTYEmptyList` (the `errors` import stays live via "no sessions selected"; survey fails with "please provide options to select from") | PR7 | done | +| UI-08 | cmd/root (unified selector) | `cmd/root.go:1005` (`uiUnifiedSelector.SelectItem`). **The original site is gone:** this was recorded against the `if len(groups) == 0` guard in `ui.SelectGroup` (`internal/ui/group_selector.go`), which PR7 (#68) deleted along with `SelectGroup` itself | Delete the `if len(items) == 0 { return nil, errors.New("no eligible targets or groups available") }` guard in `uiUnifiedSelector.SelectItem`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | **Not pinned on `main`.** `TestSelectGroup_EmptyList` / `TestSelectGroup_NonTTYEmptyList` went with `SelectGroup` and have no successor: `TestUnifiedSelector_NonTTY` (`cmd/root_test.go:348`) covers only the non-interactive guard that precedes it, and nothing exercises `SelectItem` with an empty slice. Needs a `TestUnifiedSelector_EmptyList` plus a non-TTY sibling to pin guard **order**, mirroring UI-06/UI-07 | PR7 (#68) | todo | +| UI-09 | internal/ui | `internal/ui/role_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRole` | CONFIRMED | wont-fix | none — defensive-only and unreachable through `survey`, which can only return a string it was given. Closed as **wont-fix in PR7**: the guard stays in place, deliberately uncovered; do not chase it | PR7 | wont-fix (closed) | +| UI-10 | internal/ui | `internal/ui/request_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRequest` | CONFIRMED | wont-fix | none — same rationale as UI-09. Closed as **wont-fix in PR7**: the guard stays in place, deliberately uncovered; do not chase it | PR7 | wont-fix (closed) | | COV-01 | cmd/root Execute | `cmd/root.go` (the `if shouldShowVerboseHint(...) { Fprintln(...) }` block in `Execute()`) | Delete the whole block. **Killed only on the integration leg.** `go test -count=1 ./cmd/` stays green — the package-level `Execute()` calls `os.Exit`, so only the compiled binary exercises the wiring. It fails under `-tags=integration` via `TestIntegration_ElevateWithoutLogin` and `TestIntegration_VerboseHint`. CI runs that leg unguarded on both OSes, so the call site *is* covered; the default local `make test` loop is not. Do not "simplify" CI by guarding the integration step | CONFIRMED | test | `TestIntegration_VerboseHint` + `TestIntegration_ElevateWithoutLogin` (`-tags=integration` only) | PR4 | done | | COV-02 | cmd/login | `cmd/login.go` auto-configure branch (`if profile == nil`) | No mutation — a **coverage caveat** on REQ-20. `TestRunLogin_AutoConfiguresMissingProfile` (`cmd/login_args_test.go`) calls `t.Skip` when `os.Stdin` is a terminal, so under a PTY the auto-configure branch has **zero** coverage and REQ-20 is unpinned. Accepted, not fixed: `runConfigure` prompts through `survey`, which reads `os.Stdin` directly with no injectable seam, so on a real terminal the test would block on input rather than fail. `go test`, CI and every non-interactive run have a non-TTY stdin and do exercise it. Removing the skip requires giving `runConfigure` a seam first | OVERSTATED | wont-fix | none — the skip comment states the caveat at the call site | PR4 | done | @@ -342,41 +297,46 @@ extracting a shared helper is a refactor and belongs in its own change. | PR | Rows | Of which CONFIRMED | |---|---|---| -| PR1 — Test isolation + integration harness | 2 | 0 (both OVERSTATED) | -| PR2 — Archive extraction and path security | 12 | 11 | -| PR3 — Remaining self-update correctness | 10 | 10 | -| PR4 — Argument capture | 42 | 41 | -| 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 | -| *(no PR — settled, recorded only)* | 4 | 0 | -| **Total** | **167** | **155** | +| PR1 (#63) — Test isolation + integration harness | 2 | 0 (both OVERSTATED) | +| PR2 (#64) — Archive extraction and path security | 13 | 12 | +| PR3 (#64) — Remaining self-update correctness | 10 | 10 | +| PR4 (#65) — Argument capture | 43 | 41 | +| PR5 (#66) — Output contracts | 42 | 39 | +| PR6 (#67) — Cache and config semantics | 16 | 16 | +| PR7 (#68) — UI behavior | 11 | 11 | +| PR8 (#69) — SCA / workflows / models wire contracts | 47 | 45 | +| *(no PR — settled or deferred, recorded only)* | 4 | 1 | +| **Total** | **188** | **175** | ### By verdict | Verdict | Rows | |---|---| -| CONFIRMED | 155 | -| OVERSTATED | 8 | +| CONFIRMED | 175 | +| OVERSTATED | 9 | | REFUTED | 4 | +| **Total** | **188** | ### By disposition | Disposition | Rows | |---|---| -| `test` | 152 | -| `test + prod-fix` | 5 | +| `test` | 172 | +| `test + prod-fix` | 6 | | `prod-fix` | 1 | -| `wont-fix` | 5 | -| `refuted` | 4 | -| **Total** | **167** | +| `wont-fix` | 6 | +| `refuted` | 3 | +| **Total** | **188** | -The seven production changes from the plan's table, plus two added by the PR7 review: +The production changes: the seven rows whose disposition carries `prod-fix` +(OUT-26, SFU-02, SFU-07, SFU-23, CACHE-07, CFG-02, UI-02 — CFG-02 landed as three +separate changes), plus the unnumbered selector fixes and test-only adjustments the +PR7 review added: | Row | Change | PR | CHANGELOG | |---|---|---|---| | SFU-07 | Reject zero-length extracted binary + apply-boundary check | PR2 | `### Security` | +| SFU-23 | `extractFromZip` rejects non-regular entries (`f.Mode()&fs.ModeType != 0`), mirroring the tar `Typeflag` guard | PR2 | `### Security` | | SFU-02 | UNC check before `path.IsAbs` (message only) | PR2 | no | | OUT-26 | `favorites add` early non-interactive guard, favorites-specific message | PR1 | `### Fixed` | | CFG-02 | `ParseCacheTTL` errors on any explicitly-invalid value | PR6 | `### Changed` | @@ -400,31 +360,44 @@ they pinned the helpers and not the behaviour. ### Total CONFIRMED -**155 CONFIRMED**, plus **8 OVERSTATED** (real survivors whose stated consequence -was weaker than originally claimed) and **4 REFUTED**. **167 rows total.** -5 rows are `wont-fix` (SFU-20, SFU-21, SFU-22, UI-09, UI-10) and 4 are closed by -review (`refuted`), so **158 rows require work**, of which 6 carry a production -change. +**175 CONFIRMED**, plus **9 OVERSTATED** (real survivors whose stated consequence +was weaker than originally claimed) and **4 REFUTED**. **188 rows total.** +6 rows are `wont-fix` (SFU-20, SFU-21, SFU-22, UI-09, UI-10, COV-02) and 3 are +closed by review alone (disposition `refuted`: OUT-28, OUT-29, SCA-17 — OUT-27 is +also REFUTED but a test was added anyway), so **179 rows require work**, of which 7 +carry a production change (OUT-26, SFU-02, SFU-07, SFU-23, CACHE-07, CFG-02, UI-02). + +**Status on merged `main`:** 184 rows are `done`, 2 are `wont-fix (closed)` (UI-09, +UI-10) and 2 remain `todo` — WF-25 (form-metadata wire tags, deliberately deferred +out of PR8; `Validator.Regex` is still unasserted anywhere in `internal/workflows`) +and UI-08 (its guard moved to `uiUnifiedSelector.SelectItem` when PR7 deleted +`ui.SelectGroup`, and no test succeeded `TestSelectGroup_EmptyList`). ### Reconciliation against "145" The ledger does **not** reconcile cleanly to 145, and the rows were not padded or -truncated to make it. The gap is almost entirely *granularity* — every row traces -to a named, independently reproduced finding in a verification report. +truncated to make it. The gap is *granularity* plus the rows found after the five +audit batches — every row traces to a named, independently reproduced finding. | Batch | Verifier's stated count | Rows here | Reconciles? | |---|---|---|---| | 1 — cmd request/auth | 22 confirmed | 23 (22 CONFIRMED + 1 OVERSTATED) | **Yes.** The 22 CONFIRMED rows are mutation-level and match exactly. REQ-23 (integration suite absent from CI) is reported *outside* the 22. | | 2 — cmd status/favorites/list | 25 real of 29 claimed | 29 (23 CONFIRMED, 3 OVERSTATED, 3 REFUTED) | **Approximately.** All 29 claimed items are listed so the three refutations stay recorded. 23 + 3 OVERSTATED = 26 actionable, one above the verifier's "25" — its own prose is imprecise about whether OUT-21/OUT-25/OUT-26 count as "real". | -| 3 — internal/sca + workflows | 31 + 4 extra = 35 | 38 (35 CONFIRMED, 2 OVERSTATED, 1 REFUTED) | **Yes.** 34 of the CONFIRMED rows + SCA-10 (a real survivor, only its truncation consequence overstated) = exactly 35 survivors. The remaining rows are SCA-15 (X-API-Version coverage topology, which PR8 acts on), SCA-18 (reproduced by PR8, above the headline) and SCA-17, recorded so it is not re-filed. | +| 3 — internal/sca + workflows | 31 + 4 extra = 35 | 38 (35 CONFIRMED, 2 OVERSTATED, 1 REFUTED) — SCA-01..18 and WF-01..20 | **Yes.** 34 of the CONFIRMED rows + SCA-10 (a real survivor, only its truncation consequence overstated) = exactly 35 survivors. The remaining rows are SCA-15 (X-API-Version coverage topology, which PR8 acts on), SCA-18 (reproduced by PR8, above the headline) and SCA-17, recorded so it is not re-filed. | | 4 — cmd root/elevate/env | 22 confirmed | 27 (26 CONFIRMED, 1 OVERSTATED) | **No — +5.** The report's headline groups its own sub-lettered mutations inconsistently: H2.1–2.3, H3.1–3.4, M4.1–2, M5.1–3, M6.1–6.4 and M9.1–3 are enumerated individually in the body, each with its own `go test ./cmd/ -count=1 → ok` transcript, but collapsed in the total. Listing them individually gives 25 production mutations plus 2 test-quality rows (ELV-26, ELV-27). | -| 5 — cache/config/ui/selfupdate | 44 reproduced, 41 actionable | 48 (all CONFIRMED, 4 of them `wont-fix`) | **Partly — +4.** Same granularity problem: M7 and M9 are two mutations each; L1, L3, L4 and L5 are three each; L9 is three survivors. Enumerating every reproduced mutation gives 48; removing the 4 unreachable/defensive `wont-fix` rows (SFU-20, SFU-21, UI-09, UI-10) gives **44 actionable**, matching "44 reproduced" but not "41 actionable" — the report never itemises which 3 it dropped. | +| 5 — cache/config/ui/selfupdate | 44 reproduced, 41 actionable | 48 (47 CONFIRMED, 1 OVERSTATED) — SFU-01..22, CACHE-01..07, CFG-01..09, UI-01..10 | **Partly — +7.** Same granularity problem: M7 and M9 are two mutations each; L1, L3, L4 and L5 are three each; L9 is three survivors. Enumerating every reproduced mutation gives 48; removing the 4 unreachable/defensive `wont-fix` rows (SFU-20, SFU-21, UI-09, UI-10) gives **44 actionable**, matching "44 reproduced" but not "41 actionable" — the report never itemises which 3 it dropped. SFU-22 is also `wont-fix` for the size check itself but did land a test, so it stays inside the 44. | +| *(none — found after the audits)* | — | 23 (22 CONFIRMED, 1 OVERSTATED) — OUT-30..38, SCA-19..24, WF-21..25, SFU-23, COV-01, COV-02 | **n/a.** These are not batch rows at all: they came out of the adversarial re-reviews of PR5, PR7 and PR8 and of the PR4 integration leg, and are recorded here so the findings are not lost. | + +**Net: 188 rows against a headline of 145.** The five batch headlines sum to exactly +145 (22 + 25 + 35 + 22 + 41). The 43-row excess decomposes cleanly: + +- **+23** rows found *after* the five audits, by adversarial re-review (the last table row above). +- **+20** within the batches, of which **12** are the OVERSTATED/REFUTED rows the headline + counts deliberately excluded but which belong here as settled questions (1 in batch 1, + 6 in batch 2, 3 in batch 3, 1 in batch 4, 1 in batch 5) and **8** are finer enumeration + in batches 4 and 5 — mutations the reports reproduced individually but totalled in groups. -**Net: 167 rows against a headline of 145.** Roughly 9 of the excess is finer -enumeration in batches 4 and 5 (mutations the reports reproduced individually but -totalled in groups); most of the rest is the 12 OVERSTATED/REFUTED rows the headline -count deliberately excluded but which belong here as settled questions. Nothing was -invented and nothing was dropped to hit a number. +Nothing was invented and nothing was dropped to hit a number. There are no `NEEDS-REVIEW` rows: every row's source report is unambiguous about the mutation applied and the observed result.