Rectify: Ambient State Contamination — Ground-Truth Surface Immunity - #4694
Merged
Trecek merged 19 commits intoAug 19, 2026
Merged
Conversation
run_cmd built its subprocess env from raw os.environ (or env=None on the falsy-step_name branch), inheriting the entire ambient parent environment including AUTOSKILLIT_PRIVATE_ENV_VARS -- the only run_cmd/test_check asymmetry in the launch surface. Route it through the same build_sanitized_env() the test_check tool already uses, re-exported through the autoskillit.execution gateway. Adds T15 (private vars stripped) and rewrites the now-stale test_run_cmd_without_step_name_passes_no_env, whose permissive `env is None or ...` assertion would have silently kept passing while describing a contract that no longer holds. Part of the ambient-state-contamination rectify plan, Step 10.
all_validated_recipe_names()/all_validated_recipe_paths() scan the live working-tree directory with no regard for git tracking status, so an untracked or .git/info/exclude-ignored stray .yaml file under a scanned recipe directory silently inflates every parametrized test that consumes them -- 19 test modules did, 14 at collection time. list_recipes() production behavior is deliberately unchanged; only the test suite's parametrization source moves to tracked ground truth. - tests/_tracked_recipes.py (new): tracked_recipe_paths()/names() via `git ls-files` over both the project (.autoskillit/recipes) and builtin (pkg_root()/recipes) roots, filtered to RECIPE_SCAN_DIRS and intersected with list_recipes() output so a git-tracked-but-invalid file is never yielded. Fails closed (raises) on git errors or a non-editable install -- never falls back to a directory scan, which would reintroduce the exact pathology being fixed. - Repoints all 19 consumers (14 module-level, 5 in-function) from all_validated_recipe_*() to the tracked helpers. - tests/recipe/test_tracked_recipe_parity.py (new, T17-T20): untracked-file guard, an AST scan proving no consumer regresses to the working-tree source, a tmp_path reproduction, and a clean-checkout coverage parity check against the production loader. Part of the ambient-state-contamination rectify plan, Steps 11-13.
…ep 1, T1-T7/T10/T11) Every existing environment defense in this repo diffs one hand-written constant against another hand-written constant, so a foreign-owned name (e.g. CLAUDE_CODE_EXECPATH, set by the wrapping Claude Code CLI) can enter production's read surface and stay invisible to every guard indefinitely -- issue #4691 is the second occurrence of this exact shape (#3249 was the first, AUTOSKILLIT_* in 2026-05-29). tests/_ambient_env_surface.py (new): a two-pass AST scanner over src/autoskillit/**/*.py that independently rediscovers ground truth -- every os.environ read (direct or through a resolvable constant), every env-var-named keyword/field declaration, every env-name collection and prefix-denylist, every unresolvable dynamic read, and every wholesale os.environ forwarding site (copy/comprehension/union/bare-reference) -- mirroring the ground-truth-scan shape tests/hooks/test_hook_registration_ coverage.py already uses for hook scripts, applied here via AST of src/ instead of an rglob. AMBIENT_ENV_DISPOSITIONS classifies all 280 names the scanner currently finds (scrub/preserve + owner + justification), provably a superset of the legacy _clear_private_env fixture's two source sets (AUTOSKILLIT_PRIVATE_ ENV_VARS | _HEADLESS_EXCLUSIVE_VARS). DYNAMIC_READ_EXEMPTIONS and FORWARDING_SITES declare the scanner's own honestly-reported blind spots (8 unresolvable dynamic reads, 20 wholesale-forwarding sites) so a new one can't be added silently. tests/contracts/test_ambient_env_surface.py: T1 pins the specific regression (CLAUDE_CODE_EXECPATH must be in the surface); T2/T3 are the forward/reverse pincer keeping the registry in exact sync with the scanner's live output; T4 is the adversarial proof the scanner catches an unregistered read it was never told about; T5/T6 prove it reports (rather than silently drops) unresolvable and wholesale-forwarding sites, T6 parametrized over every R7 shape; T7 is the forwarding-site pincer; T10 enforces substantive justifications; T11 pre-validates the (not yet introduced) ambient_env marker's argument names. Part of the ambient-state-contamination rectify plan, Step 1 + Step 4 (first file).
…env (Step 2-4, T8-T9) _clear_private_env iterated a closed, hand-written universe (AUTOSKILLIT_PRIVATE_ENV_VARS | _HEADLESS_EXCLUSIVE_VARS) that a foreign-owned name like CLAUDE_CODE_EXECPATH could never enter. _scrub_ambient_env instead iterates the AST-derived AMBIENT_ENV_DISPOSITIONS registry from Step 1, so any name production code newly reads is automatically covered the moment it's classified -- no manual fixture edits, and T2/T3 (Step 1) keep the registry honest against what the scanner actually finds. A pytest.mark.ambient_env(*names) marker (registered in pyproject.toml) is the declarative opt-out for a test that genuinely needs an ambient value -- conformance-probes.yml's CI executable pin depends on this mechanism (see D3 in the plan) without depending on the specific variable. pytest_report_header makes contamination visible rather than silently corrected: it prints the names (never values) of every scrub-disposition var that was actually present at session startup. tests/contracts/test_ambient_env_scrub_e2e.py (T8, T9) proves both ends via pytester.runpytest_subprocess() against the real root conftest.py (registered as a plugin, not shadow-copied, and not `import *` since every autouse fixture here is underscore-prefixed): an ambient CLAUDE_CODE_EXECPATH injected into a real child process is invisible by default, and visible when the inner test carries the marker. Part of the ambient-state-contamination rectify plan, Steps 2-4 (second/third files) + Step 3.
…e (Steps 5-7) - tests/arch/test_conftest_env_coverage.py: retargets the mechanism guard (fixture exists / imports the source-of-truth / for-loop references it) from _clear_private_env + two hand-written constants to _scrub_ambient_env + AMBIENT_ENV_DISPOSITIONS. Folds the old registry-non-emptiness check into the renamed group -- it's now strictly subsumed by T2/T3 in tests/contracts/test_ambient_env_surface.py, which already assert full bidirectional set-equality between the live AST surface and the registry. - tests/test_conftest.py: retargets the "fixture must not import from autoskillit.server" layering guard to _scrub_ambient_env. - tests/execution/test_env_boundary.py: test_no_unrecognized_claude_code_vars_ pass_through's known_vars was a hardcoded 5-entry list a 6th CLAUDE_CODE_* var could silently bypass. Replaced with the AST-derived surface filtered to CLAUDE_CODE_*, unioned with _PREFIX_BRANCH_EXEMPLARS (CLAUDE_CODE_IDE_THEME exercises IDE_ENV_PREFIX_DENYLIST's prefix-match branch but is a synthetic exemplar absent from src/ -- R5 routes real prefix-denylist collections to surface.prefixes, never surface.names, so the union is load-bearing). - Deletes the two now-redundant CLAUDE_CODE_EXECPATH monkeypatch.delenv workarounds in test_session_launch.py and test_interactive_cold_launch_ medium.py -- both centrally handled by _scrub_ambient_env now. test_claude_code_backend.py's two CLAUDE_CODE_EXECPATH sites are untouched: genuine unit-under-test inputs, not ambient-contamination workarounds. Part of the ambient-state-contamination rectify plan, Steps 5-7.
…arounds (Step 9)
72 discrete monkeypatch.delenv("CLAUDECODE"/"CLAUDE_CODE_EXECPATH", ...) calls
across 16 files were independent re-derivations of a defense that now has one
authority (_scrub_ambient_env). Left in place, they document "per-test
scrubbing is the house pattern" to the next contributor touching a CLI-gated
path -- which is exactly how #4691's leak went unnoticed for as long as it
did. Removes them from the 13 files that still had inline calls (2 were
already removed in the prior commit), plus tests/cli/_fleet_helpers.py's
_stub_guards() helper (fans out to ~31 call sites across three
test_fleet_campaign*.py/test_fleet_dispatch.py files that never appear in
this diff but depend on it).
Extends the same removal, in the same already-touched files, to three other
now-centrally-scrubbed vars found sitting in the identical guard-clearing
blocks: AUTOSKILLIT_SESSION_TYPE (test_fleet_dispatch.py, test_fleet_run.py,
_fleet_helpers.py) and AUTOSKILLIT_SKIP_STALE_CHECK/AUTOSKILLIT_SKIP_UPDATE_
CHECK/AUTOSKILLIT_FORCE_UPDATE_CHECK (test_update_checks_guards.py,
test_update_checks_prompt.py) -- CI stays, since it's preserve-disposition,
not scrub. Every remaining monkeypatch.setenv(...) in these files is left
untouched: zero pop-then-restore pairs exist in this codebase, so every
setenv is a genuine behavioral input, not a workaround.
tests/arch/test_no_adhoc_env_workarounds.py (new, T13): an AST guard so the
pattern cannot silently return. Matches <anything>.delenv(...) (any
receiver -- AST can't resolve monkeypatch typing) and os.environ.pop(...)
(receiver must literally be os.environ, which is exactly what excludes the
three known local-child-env-dict .pop() sites without needing to allowlist
them). Fails naming any site whose var is scrub-disposition and not declared
in _INTENTIONAL_ENV_INPUT_SITES.
That allowlist carries two distinct kinds of entry: a handful of genuine
behavioral test inputs (test_claude_code_backend.py's two sites), and 144
pre-existing sites for OTHER AUTOSKILLIT_PRIVATE_ENV_VARS members across ~60
files this plan's investigation never scoped or audited -- a whole-tree
rescan with the new guard's own matcher surfaces them, and deleting them
without the same per-file scrutiny this step gave the 72 known sites would
be exactly the kind of unaudited, unassigned refactoring AGENTS.md rules
out. Each is declared with a file::var-keyed justification rather than
silently passing, so the guard is honest about what it currently tolerates;
a future consolidation pass can retire them individually. A second test,
test_intentional_env_input_sites_are_not_stale, keeps the allowlist itself
from rotting.
Also fixes two dangling docstring references to the deleted _clear_private_env
fixture name (tests/hooks/test_quota_check.py:697, test_quota_post_check.py:519)
-- the last two references to it anywhere in the repo.
Part of the ambient-state-contamination rectify plan, Step 9 (T13).
…y found Step 6 replaced test_env_boundary.py's hardcoded 5-entry known_vars list with the AST-derived production surface -- exactly the immunity property this whole plan is about. Running it for real surfaced a genuine pre-existing gap the old hardcoded list could never have caught: 5 more real CLAUDE_CODE_* vars with no classification in any of the four buckets the test checks. - CLAUDE_CODE_DISABLE_BACKGROUND_TASKS / CLAUDE_CODE_DISABLE_CRON: added to IDE_ENV_DENYLIST, not _HEADLESS_EXCLUSIVE_VARS -- the latter is asserted disjoint from _CLAUDE_SKILL_SESSION_HARDENING's keys by test_session_env_contracts.py::test_claude_skill_hardening_stays_backend_local. _CLAUDE_SKILL_SESSION_HARDENING force-sets both, but only for skill sessions; build_interactive_cmd/build_resume_cmd never apply that hardening, so a host-inherited value would otherwise leak through those paths unfiltered. - CLAUDE_CODE_MCP_TOOL_IDLE_TIMEOUT: same IDE_ENV_DENYLIST bucket -- only overridden when the caller passes mcp_tool_timeout_sec > 0, so a host value would otherwise leak through when it's 0/unset. - CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS: intentional passthrough -- force_inactive_agent_teams (default False) is an opt-in neutralize-and-assert mechanism, not a base-env denylist; default byte-for-byte passthrough is the contract test_launch_force_inactive_default.py pins. - CLAUDE_CODE_MCP_TOOL_IDLE_TIMEOUT_ENV_VAR: scanner false positive -- the constant's own __all__-list identifier, never itself a real env var. Also catalogs the new tests/arch/test_no_adhoc_env_workarounds.py guard (Step 9) in resolve-review/SKILL.md's Architectural Constraint Catalog, per test_resolve_review_arch_constraint_awareness.py::test_catalog_reverse_coverage. Both were the only two failures out of ~38000 tests on the first full test_check run for this plan.
Remediates 3 MISSING findings from the /audit-impl pass against
impl-rectify-ambient-state-contamination-20260818-153855:
1. T14 regression guard for run_cmd's env-sealing fix (Step 10) was never
added. Add test_server_tools_route_subprocess_env_through_a_sealing_function
to tests/cli/test_subprocess_env_contracts.py: every env= kwarg reaching
_run_subprocess/_run_subprocess_captured under server/** must trace to
build_sanitized_env()/build_agent_env()/build_maintenance_env(), never a
raw os.environ-derived dict and never None. Added as a distinct guard
rather than widening test_run_subprocess_callers_use_safe_env_pattern's
shared _check_env_value predicate in place -- that predicate's "safe"
definition (env=None, env={**os.environ, ...}) is the correct, deliberate
contract for the general _run_subprocess case and predates this fix;
overloading it would either weaken T14's "never None" requirement or
silently tighten the general rule for future _run_subprocess(env=...)
call sites outside this plan's scope. Verified live: zero server/** call
sites pass env= to _run_subprocess/_run_subprocess_captured other than
run_cmd's, so the new guard has no other blast radius, and it fails
against the pre-Step-10 IfExp/ternary env construction (traced by hand
against the removed code), proving it has teeth.
2. production_env_read_surface()'s R1 rule silently dropped all-keyword-
argument env reads -- os.getenv(key=..., default=...),
os.environ.pop(key=...), os.environ.setdefault(key=...) fell through to
node.args[0], which is absent for an all-keyword call, and vanished with
no trace in either surface.names or surface.unresolved. Route all three
through the same _get_call_key_arg() keyword-resolution path already used
for .get() (os.environ's methods and os.getenv all name their first
param `key`). Added a parametrized adversarial case per shape to
tests/contracts/test_ambient_env_surface.py. Verified live via a
synthetic probe module exercising all three shapes: previously 0/3
resolved into surface.names, now 3/3.
3. tests/arch/test_conftest_env_coverage.py lost a guard instead of being
cleanly retargeted: test_clear_private_env_imports_headless_exclusive_vars_
constant was deleted with no replacement, and test_coverage_parity_
private_env_vars (which the plan said stays unaffected) was weakened from
asserting AUTOSKILLIT_PRIVATE_ENV_VARS/_HEADLESS_EXCLUSIVE_VARS are
non-empty to asserting only that the unrelated 280-entry
AMBIENT_ENV_DISPOSITIONS registry is non-empty -- which would pass
vacuously even if either legacy constant regressed to empty, since the
V4 subset check is one-directional. Restored the direct non-emptiness
assertions on both legacy constants alongside (not instead of) the
registry check. The deleted mechanism-check is NOT restored 1:1: the new
_scrub_ambient_env fixture correctly imports only AMBIENT_ENV_DISPOSITIONS
(one unified registry symbol, proven a superset of both legacy sets via
V4), not two separate legacy-constant imports, so a second per-constant
import-check no longer has anything real to assert -- 3 mechanism tests
for 1 registry symbol is the correct replacement for 4 mechanism tests
covering 2 legacy symbols, not a coverage loss. Documented this in the
file's module docstring so the count discrepancy reads as deliberate.
Verified: pre-commit clean on all 4 touched files; all three fixes confirmed
live via direct interpreter probes (not the forbidden pytest invocation)
before commit.
…all recursion
Resetting bindings={} on recursion broke transitive two-hop name
resolution (e.g. tmp = build_sanitized_env(...); outer = tmp), causing
a false-positive sealed-env violation. Forward the outer bindings
instead, matching the actual multi-hop lookup the docstring implies.
…nv_violations
ast.walk(func_node) descended into nested function/class bodies when
collecting name bindings, so a nested function reassigning the same
name (e.g. an inner `env = {}`) silently overwrote the outer, legitimately
sealed binding — producing a false-positive violation. Add
_collect_own_scope_bindings(), a scope-respecting walk that stops at
nested Function/AsyncFunctionDef/ClassDef boundaries.
Removed the 'previously ran 5 tests: one existence check...' inventory of the file's pre-refactor shape — zero value to a current reader. Kept the live reasoning about the legacy-constant collapse and why the legacy constants are still checked directly.
…ations 144 entries shared the exact same 3-line boilerplate justification with only the var name substituted. Replace with a single _GRANDFATHERED_JUSTIFICATION template applied over a tuple of site keys via dict comprehension — same runtime dict (verified equal to the original by exec+comparison), no duplicated text.
… silently skipping except SyntaxError: continue dropped unparseable tests/**/*.py files with zero trace, so a syntactically broken file would evade both guard tests (false-clean PASS). _all_env_call_sites() now returns (sites, unparseable) and a new test_all_test_files_parse_for_env_call_site_scan fails loudly if any file was skipped.
except SyntaxError: continue silently dropped whole-file parse failures with no record anywhere in ProductionEnvSurface -- R6's `unresolved` list only covers unresolvable expressions inside files that DID parse, a different failure mode. Add unparseable_files to the surface so a consumer can assert completeness (fail loudly, never silently under-report, per this PR's own stated contract-test principle).
…gaps - _ambient_env_marker_names now records unparseable files instead of silently skipping them, with a new completeness test (same swallowed- SyntaxError pattern as _all_env_call_sites/production_env_read_surface). - DYNAMIC_READ_EXEMPTIONS had no forward pincer against surface.unresolved (unlike sibling FORWARDING_SITES's test_every_forwarding_site_is_declared), so a new unresolvable dynamic read would silently evade CI. Add test_every_unresolved_read_is_declared, plus reverse pincers test_no_orphan_forwarding_sites and test_no_orphan_dynamic_read_exemptions, mirroring the existing test_no_orphan_dispositions pattern. - New test_production_env_surface_scan_parses_all_source_files asserts the scanner's own completeness.
The other 3 pytest hooks in this file (pytest_addoption, pytest_configure, pytest_terminal_summary) all type their config parameter as pytest.Config; pytest_report_header was the one outlier.
19 AMBIENT_ENV_DISPOSITIONS entries (coreutils env(1) command-line flags
like --argv0, -C, -v) shared the exact same 3-line R4-false-positive
justification verbatim. Replace with a single
_R4_COREUTILS_ENV_FLAG_JUSTIFICATION constant applied over a tuple of
flag names via dict-comprehension unpacking (**{...}) into the larger
literal. Verified byte-for-byte equal to the original dict, field by
field, for all 280 entries.
Trecek
enabled auto-merge
August 19, 2026 04:17
…ient-state-contamination-20260818-153855
…er develop merge Merging origin/develop (#4676, #4679 decomposed execution/backends and server/_lifespan.py into directory packages) shifted the file:line locations this registry hardcodes for known forwarding sites and unresolved dynamic reads, without changing the sites themselves. - server/_lifespan.py:747 -> server/_lifespan/_session_boots.py:455 (file split, same EVIDENCE_READER_ENV_FORWARD_VARS dict comprehension) - claude.py/codex.py _HEADLESS_EXCLUSIVE_VARS / _INTERACTIVE_ENV_EXCLUSIONS forwarding sites shifted within their files; codex.py's maintenance/ version-probe dict(os.environ) site moved into the new execution/backends/_codex_probes.py module Verified each new site against the live source before updating its justification; all four ambient-env-surface pincer tests pass locally after the merge.
Trecek
deleted the
impl-rectify-ambient-state-contamination-20260818-153855
branch
August 19, 2026 05:39
Trecek
added a commit
that referenced
this pull request
Aug 19, 2026
The develop-merge (2edda61, #4694) brought in the FORWARDING_SITES / DYNAMIC_READ_EXEMPTIONS ground-truth registry in tests/_ambient_env_surface.py. This branch's own diff shifted line numbers in six of the registered files (cli/session/_session_launch.py, execution/backends/_codex_probes.py, execution/backends/claude.py, execution/backends/codex.py, execution/evidence_reader.py, server/_lifespan/_session_boots.py), so the registry's line-number keys no longer matched the AST scanner's findings on the merged tree. Updated the 12 stale keys to their current line numbers; justification text and site set are unchanged.
Trecek
added a commit
that referenced
this pull request
Aug 19, 2026
The rebase onto upstream/develop (which now includes #4665's _command_classification.py decomposition and #4694's ambient-env ground-truth surface) shifted line numbers referenced by two line-number-keyed registries in tests/_ambient_env_surface.py, and made one explicit env-scrub call redundant: - FORWARDING_SITES: execution/backends/codex.py sites shifted 514->516, 648->651, 767->771, 839->844. - DYNAMIC_READ_EXEMPTIONS: server/_lifespan/_session_boots.py 455->468; server/tools/tools_evidence_reader.py 170->174. - tests/cli/test_cook_real_root_smoke.py:107's explicit monkeypatch.delenv("CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS") is now redundant with #4694's autouse _scrub_ambient_env fixture (the var has disposition="scrub" in AMBIENT_ENV_DISPOSITIONS); removed per tests/arch/test_no_adhoc_env_workarounds.py's guard. Verified: all shifted line numbers checked against actual on-disk positions; tests/contracts/test_ambient_env_surface.py (26), tests/arch/test_no_adhoc_env_workarounds.py (3), and tests/cli/test_cook_real_root_smoke.py (1 skipped, opt-in gated) all pass locally. pre-commit run --all-files clean. The remaining CI failure (test_sigterm_writes_scenario_json, rc=-15) is byte-identical to develop and untouched by this branch's history — passes locally in isolation; treated as pre-existing CI flakiness, not a regression from this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Trecek
added a commit
that referenced
this pull request
Aug 20, 2026
The rebase onto upstream/develop (which now includes #4665's _command_classification.py decomposition and #4694's ambient-env ground-truth surface) shifted line numbers referenced by two line-number-keyed registries in tests/_ambient_env_surface.py, and made one explicit env-scrub call redundant: - FORWARDING_SITES: execution/backends/codex.py sites shifted 514->516, 648->651, 767->771, 839->844. - DYNAMIC_READ_EXEMPTIONS: server/_lifespan/_session_boots.py 455->468; server/tools/tools_evidence_reader.py 170->174. - tests/cli/test_cook_real_root_smoke.py:107's explicit monkeypatch.delenv("CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS") is now redundant with #4694's autouse _scrub_ambient_env fixture (the var has disposition="scrub" in AMBIENT_ENV_DISPOSITIONS); removed per tests/arch/test_no_adhoc_env_workarounds.py's guard. Verified: all shifted line numbers checked against actual on-disk positions; tests/contracts/test_ambient_env_surface.py (26), tests/arch/test_no_adhoc_env_workarounds.py (3), and tests/cli/test_cook_real_root_smoke.py (1 skipped, opt-in gated) all pass locally. pre-commit run --all-files clean. The remaining CI failure (test_sigterm_writes_scenario_json, rc=-15) is byte-identical to develop and untouched by this branch's history — passes locally in isolation; treated as pre-existing CI flakiness, not a regression from this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
CLAUDE_CODE_EXECPATH, set by the wrapping Claude Code CLI, leaks intotask test-checkand causes 16 local-only failures. The variable itself is a symptom. The architectural weakness is that every environment defense in this repository enumerates a closed world of names AutoSkillit already owns, so the next foreign-owned variable leaks through untouched. This is the second occurrence of the identical shape: #3249 closed theAUTOSKILLIT_*instance in 2026-05-29 with a registry-driven autouse scrub; #4691 is theCLAUDE_*instance.Closes #4691
Implementation Plan
Plan file:
/home/talon/projects/generic_automation_mcp/.autoskillit/temp/rectify/rectify_ambient_state_contamination_2026-08-18_134915.md🤖 Generated with Claude Code via AutoSkillit