test(secretspec): in-tree regression guard for the activation-reason fix (RIG-2828) - #6
Open
rigel-mintaka wants to merge 1 commit into
Open
Conversation
rigel-mintaka
force-pushed
the
infra/2828-devenv-secretspec-regression-guard
branch
from
September 4, 2026 18:09
69323e3 to
ba2d0f7
Compare
🔍 Suggested ReviewersBased on git blame analysis of the changed lines, the following contributors have significant experience with the modified code:
Please consider reviewing this PR as you have authored significant portions of the code being modified. Your expertise would be valuable! 🙏 This comment was automatically generated by git-blame-auto-reviewer Last updated: 2026-09-04T20:28:45.426Z |
rigel-mintaka
force-pushed
the
infra/2828-devenv-secretspec-regression-guard
branch
from
September 4, 2026 20:02
ba2d0f7 to
af9684e
Compare
…fix (RIG-2828)
Adds a feature-gated integration test that pins the RIG-2822 fix in `resolve_secretspec_into`: the `.with_default_reason("devenv shell activation")` chain before `.validate()?` so secretspec 0.19+'s `require_reason="agents"` policy stops blocking a coding agent's automatic devenv-shell entry.
## What
A new `test-secretspec` cargo feature (folded into `test-all`) gating one integration test, `agent_shell_entry_supplies_a_default_secretspec_reason`, in `devenv/src/devenv/mod.rs`. It builds a tempdir `secretspec.toml` with `require_reason` unset (defaults to `"agents"`) and a `dotenv` provider, enters an isolated process world, then calls the real `resolve_secretspec_into` call site and asserts the resolve succeeds with the declared secret present.
## Isolation and soundness
The test drives real process-global state (cwd + a set of env vars). Because the default libtest harness runs a crate's tests multi-threaded in one binary — and `cli::tests` also mutates env — a crate-wide `TEST_ENV_LOCK` (added in `lib.rs`) now serializes every test env/cwd mutation; both this guard and `cli::tests::EnvVarGuard` take it (replacing the former per-module `ENV_LOCK`). The `ProcessGlobalGuard` snapshots and restores cwd + every touched var on drop, is constructed before any mutation so a panic mid-setup still unwinds through its `Drop`, and surfaces a failed cwd-restore loudly without panicking in `Drop`.
The guard SETs `CLAUDECODE`/`HOME`/`XDG_{STATE,CONFIG,DATA}_HOME` and CLEARs `SECRETSPEC_{REASON,AGENT,PROFILE,PROVIDER,SCOPE}` so the fixture is the sole input to the resolve — an ambient `SECRETSPEC_REASON` would otherwise satisfy the gate with the fix removed (a vacuous pass), and an ambient profile/provider would fail the resolve for an unrelated reason.
A negative-control assertion runs first: with no reason supplied the `require_reason` gate MUST fire (`Err(ReasonRequired)`), or the test is vacuous. This fails loudly if a secretspec bump flips the default off `"agents"` or a `detect-coding-agent` bump stops recognizing `CLAUDECODE` — either of which would silently disarm the gate.
## Verification
Red/green observed against the fix line: with the `with_default_reason` chain removed the test fails with `Err(ReasonRequired)` — including with an ambient `SECRETSPEC_REASON`/`SECRETSPEC_PROFILE` set (the vacuous-pass hazard, closed by the guard's env clearing); restored, it passes. The full 144-test `devenv` lib suite is green under plain multi-threaded `cargo test --features devenv/test-secretspec` (the cross-module concurrent path the shared lock protects). `cargo fmt --check` clean; `cargo clippy --tests` exit 0 both with and without the feature (only pre-existing `too_many_arguments` warnings on untouched code). `Cargo.nix` regenerated via `crate2nix generate` so the checked-in generated file matches the new feature.
Refs RIG-2828
Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
infra/2828-devenv-secretspec-regression-guard
branch
from
September 4, 2026 20:28
af9684e to
b63484c
Compare
rigel-mintaka
marked this pull request as ready for review
September 4, 2026 20:28
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
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.
Adds a feature-gated integration test that pins the RIG-2822 fix in
resolve_secretspec_into: the.with_default_reason("devenv shell activation")chain before.validate()?so secretspec 0.19+'srequire_reason="agents"policy stops blocking a coding agent's automatic devenv-shell entry.What
A new
test-secretspeccargo feature (folded intotest-all) gating one integration test,agent_shell_entry_supplies_a_default_secretspec_reason, indevenv/src/devenv/mod.rs. It builds a tempdirsecretspec.tomlwithrequire_reasonunset (defaults to"agents") and adotenvprovider, enters an isolated process world, then calls the realresolve_secretspec_intocall site and asserts the resolve succeeds with the declared secret present.Isolation and soundness
The test drives real process-global state (cwd + a set of env vars). Because the default libtest harness runs a crate's tests multi-threaded in one binary — and
cli::testsalso mutates env — a crate-wideTEST_ENV_LOCK(added inlib.rs) now serializes every test env/cwd mutation; both this guard andcli::tests::EnvVarGuardtake it (replacing the former per-moduleENV_LOCK). TheProcessGlobalGuardsnapshots and restores cwd + every touched var on drop, is constructed before any mutation so a panic mid-setup still unwinds through itsDrop, and surfaces a failed cwd-restore loudly without panicking inDrop.The guard SETs
CLAUDECODE/HOME/XDG_{STATE,CONFIG,DATA}_HOMEand CLEARsSECRETSPEC_{REASON,AGENT,PROFILE,PROVIDER,SCOPE}so the fixture is the sole input to the resolve — an ambientSECRETSPEC_REASONwould otherwise satisfy the gate with the fix removed (a vacuous pass), and an ambient profile/provider would fail the resolve for an unrelated reason.A negative-control assertion runs first: with no reason supplied the
require_reasongate MUST fire (Err(ReasonRequired)), or the test is vacuous. This fails loudly if a secretspec bump flips the default off"agents"or adetect-coding-agentbump stops recognizingCLAUDECODE— either of which would silently disarm the gate.Verification
Red/green observed against the fix line: with the
with_default_reasonchain removed the test fails withErr(ReasonRequired)— including with an ambientSECRETSPEC_REASON/SECRETSPEC_PROFILEset (the vacuous-pass hazard, closed by the guard's env clearing); restored, it passes. The full 144-testdevenvlib suite is green under plain multi-threadedcargo test --features devenv/test-secretspec(the cross-module concurrent path the shared lock protects).cargo fmt --checkclean;cargo clippy --testsexit 0 both with and without the feature (only pre-existingtoo_many_argumentswarnings on untouched code).Cargo.nixregenerated viacrate2nix generateso the checked-in generated file matches the new feature.Refs RIG-2828
Co-authored-by: Matt Wilkinson matt@rigel.build