test: isolate the suite from real user state, and wire integration tests into CI - #63
Merged
Conversation
IDSEC_KEYRING_FOLDER and IDSEC_FILE_LOG_PATH are absolute-path overrides that bypass the HOME fallback, so a pre-existing value in the developer's or CI environment sends SDK keyring and log writes outside the sandbox while AssertSandboxed still reports zero failures. Redirect both, force the SDK file keyring with IDSEC_BASIC_KEYRING=1 (the OS keyring is a daemon, not a path, and cannot be redirected at all), and extend AssertSandboxed to cover them. The redirect list is now asserted against an explicit literal and every entry gets a hostile pre-existing value before Run: ranging over redirectedVars itself meant a drop-one mutation survived on four of the original five entries. Run also restores the environment and removes the sandbox from a defer so a panic cannot leak either, and saves/restores sandboxRoot so a nested Run hands the outer root back.
internal/sca, internal/workflows and internal/sdkclient drive the real service constructors in their retry-policy tests but had no TestMain, so a new test there could reach real user state with no failing signal. They can hold an in-package TestMain because testenv imports internal/sca/models, not internal/sca.
…putFormat
The integration version check accepted `contains("dev") || contains("unknown")`,
and a non-ldflags build always prints "commit: unknown", so the second arm made
it true for any version string. Assert "grant version dev" instead.
The cmd test helpers restored only outputFormat, but newRootCommand binds
verbose to a package global through the same mechanism. Restore both.
isolatedEnv also passes the SDK keyring/log overrides down to the child
process, which inherits the parent environment.
REQ-23 and OUT-26 are the ledger's own PR1 rows; leaving them todo broke the rule the ledger introduced. Both are flipped to done with the mutation evidence that closes them. CLAUDE.md's Test Isolation section now states what testenv covers, what it cannot cover (the OS keyring is a daemon, not a path), and why XDG_CONFIG_HOME is defensive rather than load-bearing.
This was referenced Aug 15, 2026
…ior vars Three of AssertSandboxed's six resolver checks — config.ConfigDir, config.ConfigPath and cache.CacheDir — could each be deleted outright with the whole suite still green. The existing failure tests only asserted len(errs) > 0, which any other assertion in the function satisfies. Add two cases that pin the failure COUNT and which resolver reported it: GRANT_CONFIG outside the sandbox isolates ConfigPath (exactly 1), and HOME+USERPROFILE outside isolates ConfigDir and CacheDir (exactly 2, since CacheDir delegates to ConfigDir -> os.UserHomeDir). Setting both home vars keeps the case real on the Windows leg instead of skipping it. recordingTB now stores the formatted message, because the resolver name is an argument rather than part of the format string. Also unset IDSEC_PROFILE and DEPLOY_ENV for the duration of Run. Both are read by non-test SDK code — the profile loader picks the default profile name from the first, isp.FromISPAuth resolves the tenant environment from the second — and neither has a sane sandbox value, so absent is the only safe state. Restoration preserves the set-vs-unset distinction exactly. Finally, pass GOENV through to the sandboxed integration build: the child `go build` resolves its env file via os.UserConfigDir, which the redirect points at an empty sandbox on Linux while Windows reads an unredirected %AppData%, so the legs disagreed and any `go env -w` setting was dropped.
…s lines This branch inserts 13 lines at cmd/favorites.go:148, so every ledger row below that point named the wrong statement while the header still claimed the numbers were verified. All nine cmd/favorites.go rows shifted by +13 (OUT-02/03/16/17/18/19/20/24/27), each confirmed against the actual line content rather than blanket-added. The header now says the numbers are relative to this branch.
The 23 untagged cmd/*_test.go files compile into the integration binary too, so their order dependence was never shuffled there — -shuffle=on was applied only to the untagged build. Add it to the integration step, still unguarded so both legs run it. `grant favorites add` also requires a NAME argument, which the new non-interactive error omitted; it now says so.
aaearon
added a commit
that referenced
this pull request
Aug 16, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 1 of 8 in a test-suite remediation. Merge this first — the other seven are stacked on it.
Why
An adversarial mutation audit (434 mutations, 153 confirmed escapes) found the suite writes the developer's real
~/.grant/cache/session_timestamps.json. Measured with strace on26bd890:~/.grant/~/.idsecmaincache.CacheDir()resolves viaos.UserHomeDir()and ignoresGRANT_CONFIG, so redirecting that alone was insufficient.What
internal/testenv— redirectsHOME,USERPROFILE,XDG_CONFIG_HOME,IDSEC_PROFILES_FOLDER,GRANT_CONFIG,IDSEC_KEYRING_FOLDER,IDSEC_FILE_LOG_PATH,IDSEC_BASIC_KEYRING.AssertSandboxedverifies each resolver lands in the sandbox. Does not importtesting.TestMainincmd,internal/config,internal/cache,internal/sca,internal/workflows,internal/sdkclient.bootstrapImplstubbed to a named sentinel so no unit test can authenticate. Deliberately untagged — 23 of 25cmd/*_test.gofiles carry no build tag and compile into the integration binary, so//go:build !integrationwould leave them running real auth undermake test-integration.-tags=integrationand-shuffle=onon both legs;.golangci.ymlnow lints both tagged files.grant favorites addfails immediately without a terminal instead of authenticating and querying SCA first.Honest limits
AssertSandboxedvalidates configured destinations. It cannot prove no code wrote elsewhere, catch a future directos.UserHomeDir()call, or see reads.DBUS_SESSION_BUS_ADDRESSset, the SDK picks the real libsecret store.IDSEC_BASIC_KEYRING=1contains this; it does not sandbox it. If a future SDK stops honoring that var, containment is lost and nothing here detects it.-shuffle=onexposed a real order dependence (outputFormatleaking viaexecuteCommand), now fixed. Quantified: disabling the restore fails 5 of 8 fixed seeds — so a single random-seed CI run has ~1-in-3 odds of missing a comparable future regression.Adversarial review performed (Codex credits exhausted mid-run; review by a Claude agent in an isolated worktree). All findings fixed.
docs/mutation-ledger.mdtracks all 167 audit rows.