Skip to content

feat(check_dataset_allowlist): fail when a capped should_simulate deletes committed data - #253

Merged
Jammy2211 merged 2 commits into
mainfrom
feature/dataset-allowlist-small-datasets-guard
Aug 22, 2026
Merged

feat(check_dataset_allowlist): fail when a capped should_simulate deletes committed data#253
Jammy2211 merged 2 commits into
mainfrom
feature/dataset-allowlist-small-datasets-guard

Conversation

@Jammy2211

@Jammy2211 Jammy2211 commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Closes #252. Follow-up to PyAutoArray#470 / autolens_workspace_test#264, which fixed the single live instance by declaring full_datasets on the one offending script. This closes the general footgun.

Paired docstring-only PR: PyAutoArray#480.

What it adds

Leg 1 asserts nothing generated got committed. Leg 2 asserts nothing committed gets deleted. should_simulate() ends in shutil.rmtree, so a script reaching a committed dataset while PYAUTO_SMALL_DATASETS=1 is still in force destroys data the allowlist exists to protect. The library-side stamp guard can't cover this in general — it reads <dataset>/data.fits, so JSON-only datasets are invisible to it.

The predicate is containment, not prefix matching

This is the part worth reviewing. The obvious implementation — "does the resolved path sit under an !dataset/... allowlist prefix?" — is wrong, and I only caught it by running the guard across every workspace before trusting it:

[capped-deletion] FAIL — ... (6):
    scripts/overview/overview_4_charge_injection_data.py:131 -> dataset/overview/imaging_ci/uniform
    scripts/overview/overview_5_fitting.py:276              -> dataset/overview/dataset_1d
    ... 4 more

All six are safe. autocti_workspace commits five doc images directly in dataset/overview/; those scripts regenerate dataset/overview/imaging_ci/uniform and dataset/overview/dataset_1d, which hold nothing tracked. Deleting them destroys nothing.

So the predicate is "rmtree(path) would delete git-tracked files", which is the actual invariant. Six spurious release-blocking failures → zero.

Resolution is exact, and gaps are loud

This runs in pre_build, so a false positive blocks a release. Arguments are resolved by a restricted AST evaluator — literals, top-level names in source order (these scripts reassign dataset_name between sections), multi-arg Path(...), os.path.join, / chains, plain f-strings. Anything it can't evaluate is reported and skipped, never guessed, and the skipped count always prints so a partial sweep can't read as full coverage.

Multi-arg Path("dataset", "multi_galaxy", dataset_name) turned out to be the dominant idiom — handling only the single-arg form left 79 of ~253 autolens_workspace call sites (~31%) unresolved. With it, coverage is complete:

Workspace Result Unresolved
autolens_workspace OK (49 committed files) 0
autogalaxy_workspace OK (1) 0
autocti_workspace OK (5) 0
autolens_workspace_test OK (15) 0
autofit_workspace, *_test no tracked dataset files
autolens_workspace_developer SKIP (Group B, not in regime)

Regression proof

Reverting autolens_workspace_test to its pre-#264 state reproduces the failure exactly:

[capped-deletion] FAIL — should_simulate() would delete COMMITTED files without releasing PYAUTO_SMALL_DATASETS (1):
    scripts/point_source/visualization/visualization.py:39 -> dataset/point_source/simple   [ENV: real_plots]

Also

ENV_DECLARATION_TOKENS now documents real_output as a superset token (it releases all four managed vars). Treating the map as four single-var tokens under-counts the released set and reports such a script as an offender — that mis-read cost real time during the #470 sweep. _releasing_tokens() derives from the map rather than hardcoding the pair, so a future releasing token protects scripts without an edit here.

Tests

10 new tests covering the resolver (multi-arg Path, / chains, os.path.join, source-order reassignment, unresolvable-not-guessed, function-scope exclusion), token derivation, and the containment predicate end-to-end — including the autocti false-positive shape as an explicit regression. Full suite: 370 passed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01F11sMzmaVWfU6NCz1PKVVb

Jammy2211 and others added 2 commits August 22, 2026 19:33
…etes committed data

Leg 1 asserts nothing *generated* got committed. This adds the mirror-image
check: nothing *committed* gets deleted. `should_simulate` ends in
`shutil.rmtree`, so a script reaching a committed dataset while
PYAUTO_SMALL_DATASETS=1 is still in force destroys data the allowlist exists to
protect (PyAutoArray#470). The library-side stamp guard cannot cover this in
general — it reads `<dataset>/data.fits`, so JSON-only datasets are invisible
to it.

The predicate is "rmtree(path) would delete git-tracked files", NOT "path sits
under an allowlist prefix". Those differ, and the prefix form over-reports:
autocti_workspace commits five doc images directly in `dataset/overview/` while
its overview scripts regenerate `dataset/overview/imaging_ci/uniform` and
`dataset/overview/dataset_1d`, which hold nothing tracked. Deleting those
destroys nothing; prefix matching called all six release-blocking failures.

Call-site arguments are resolved through a restricted AST evaluator (literals,
top-level names in source order, multi-arg `Path(...)`, `os.path.join`, `/`
chains, plain f-strings). This gate runs in pre_build where a false positive
blocks a release, so anything unresolvable is REPORTED AND SKIPPED, never
guessed, and the skipped count is always printed — a partial sweep must not
read as full coverage.

Releasing tokens are derived from `ENV_DECLARATION_TOKENS`, not hardcoded, so a
future token that releases PYAUTO_SMALL_DATASETS starts protecting scripts
without an edit here. Also documents `real_output` as a superset token in
env_config: treating the map as four single-var tokens under-counts the
released set and reports such a script as an offender.

Verified across every workspace: zero violations, zero unresolved call sites,
and reverting autolens_workspace_test to its pre-#264 state reproduces the
failure with the exact file, line and resolved path.

Closes #252

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01F11sMzmaVWfU6NCz1PKVVb
The tenant-firewall gate failed on all three legs: generic organ code must not
carry instance facts (satellite repo names) outside the declared config
surfaces. Six tokens across three files.

Refactored, not allowlisted, per the recorded doctrine ("derivable or arbitrary
-> refactor; genuine branded fact -> declare the surface"; a
comments/docstrings exemption was considered and rejected). None of these were
load-bearing: the illustrative workspace shapes are described structurally
(`dataset/<section>/<generated>/`) and the issue references live in the PR and
commit trail, which is not organ code. The allowlist does not grow.

Behaviour unchanged — comments, docstrings and test prose only. Suite still 370
passed, and reverting the originating workspace to its pre-fix state still
reproduces the failure with the exact file, line and resolved path.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01F11sMzmaVWfU6NCz1PKVVb
@Jammy2211
Jammy2211 merged commit 90f108f into main Aug 22, 2026
3 checks passed
@Jammy2211
Jammy2211 deleted the feature/dataset-allowlist-small-datasets-guard branch August 22, 2026 23:47
Jammy2211 added a commit that referenced this pull request Aug 23, 2026
…ntexts (#255)

`autohands check_dataset_allowlist`, newly registered as a CLI verb in #254,
crashed with ModuleNotFoundError when run from a workspace root. The two merges
combined to expose it: #253 added `from autohands.env_config import ...` to this
module, and #254 made it reachable from the dispatcher.

`bin/autohands` (`_python_in_autohands`) runs these tools as scripts with
`autohands/` ITSELF on PYTHONPATH, so siblings are top-level modules — the flat
`from env_config import ...` idiom the other guards here already use. As a
library import (pytest, or anything importing
`autohands.check_dataset_allowlist`) the package's PARENT is on the path and the
flat name does not resolve. Supporting only one form breaks the other, so
`_env_config()` tries flat first and falls back to package-qualified.

Also fixes a quieter instance of the same bug. `_releasing_tokens` wrapped its
import in `except Exception` and returned the hardcoded `{full_datasets,
real_output}` fallback, so under the CLI it swallowed the ImportError and never
consulted ENV_DECLARATION_TOKENS at all — a silent degradation that still
produced a green run, and would have stopped honouring any future releasing
token without failing. The fallback is now a genuine last resort.

Verified in both contexts: the CLI verb runs clean from a workspace root and
still reports the originating defect (exact file, line, resolved path) when that
workspace is reverted to its pre-fix state. Suite 375 passed; firewall gate OK.


Claude-Session: https://claude.ai/code/session_01F11sMzmaVWfU6NCz1PKVVb

Co-authored-by: Jammy2211 <[email protected]>
Co-authored-by: Claude Opus 5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(check_dataset_allowlist): fail when a capped should_simulate would delete committed data

1 participant