Add shared pycc_scratch scratch-directory abstraction + repo lint gate by rotnov · Pull Request #786 · rotnov/pycc

rotnov · 2026-08-25T07:39:34Z

Outstanding gate: the D-068 pinned local reviewer (ievo:deep-reviewer) has not yet run against this diff — this dispatched session lacks Agent-tool access to invoke it. A session with that access must run it and address findings before merge.

Summary

Part 1 of #779 (the pycc test/production temp-directory disk-fill incident: 70,000+ leaked directories, 70+ GiB). Implements the full plan published at #781 (comment), planned against origin/main tip 7b3c4301 after 4 rounds of adversarial review.

New workspace crate crates/pycc_scratch with ScratchDir: an RAII handle that derefs to Path and removes its directory tree on Drop, including during panic unwinding. Named pycc_{category}_{pid}_{nanos}_{seq} (full epoch nanoseconds + a per-process atomic counter) — collision-safe within a process and, in practice, across process restarts. No external dependency (tempfile/rand both considered and rejected — see D-200).
scripts/check_scratch_dir_usage.py (self-tested by scripts/test_check_scratch_dir_usage.py), wired into the governance CI job: rejects any tracked .rs file with more raw temp_dir().join(...) occurrences than a checked-in per-file count allowlist records for it (384 occurrences across 36 files at this commit, matching the plan's corrected baseline). A file not in the allowlist is held to the new rule immediately; a listed file's count may only stay the same or shrink.
docs/decisions/D-200-...md records the crate/lint design and rejected alternatives; a short note added to D-085 clarifying pycc_scratch is not pycc_testkit.
docs/TESTING.md gained a "Scratch directories" subsection; docs/DELIVERY_PLAN.md's v0.3 section now links all 5 parts of Fix pycc test temporary-directory lifecycle to prevent disk fill from repeated/interrupted test runs #779. docs/ROADMAP.md needed no change (no v0.3 acceptance-criterion status changes).
docs/sessions/2026-08-25-02-issue-781-scratch-dir-abstraction.md is this session's handoff entry.

This PR changes zero existing temp_dir().join(...) call sites and does not touch src/main.rs's behavior at all — migrating the ~384 existing test sites (Part 2, #782) and fixing src/main.rs's two production leaks (Part 3, #783) are separate, already-filed follow-up issues, both depending only on this PR's crate landing.

Gates run locally (all green)

cargo doc --workspace --no-deps — succeeds.
cargo build --workspace — succeeds.
cargo test --workspace — 0 failures (1281+ tests; the error[...] lines visible in raw test output are expected diagnostic-rejection assertions inside diagnostics tests, not real failures).
cargo llvm-cov --workspace --fail-under-lines 100 --fail-under-regions 100 — 100.00% lines / 100.00% regions, 0 missed, across all 49 measured files, including the new crates/pycc_scratch/src/lib.rs (154/154 regions, 86/86 lines). The parallel-creation regression test was manually verified to fail against a reverted PID-only naming scheme before being finalized, per the plan's own instruction.
python3 -B scripts/check_scratch_dir_usage.py and python3 -B scripts/test_check_scratch_dir_usage.py — pass.
python3 -B -m unittest discover -s scripts -p 'test_*.py' (governance job's first step) — 947 tests, 0 failures.
ruby scripts/check_ci_permissions.rb — passes.
ruby scripts/check_roadmap_evidence.rb / test_check_roadmap_evidence.rb — pass under a UTF-8 locale.
python3 scripts/generate_decisions_index.py docs/decisions docs/decisions/README.md --check — up to date.

Test plan

cargo test --workspace — 0 failures.
cargo llvm-cov --workspace --fail-under-lines 100 --fail-under-regions 100 — 100%/100%.
scripts/check_scratch_dir_usage.py correctly rejects a raw std::env::temp_dir().join("pycc_*") pattern and allows the ScratchDir helper (see scripts/test_check_scratch_dir_usage.py's 8 mutation-test cases).
Manual regression-test validity check: parallel-creation test fails against a PID-only naming scheme (temporarily reverted, re-run, restored).
D-068 pinned reviewer (ievo:deep-reviewer) — outstanding, see note at top.

Fixes #781

chatgpt-codex-connector · 2026-08-25T07:39:38Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

rotnov · 2026-08-25T07:50:11Z

D-068 review follow-up: both warnings addressed

The pinned ievo:deep-reviewer pass on this PR's committed diff returned no blockers, 2 warnings, 2 notes. Both warnings are fixed in f5a1425; the two notes were informational/no-action-required and left as-is.

Warning 1 (doc drift) — docs/ARCHITECTURE.md's "Workspace crates" table didn't list pycc_scratch even though Cargo.toml already has it as a real workspace member. Added a row in the same terse style as the existing pycc_artifact_layout entry, referencing issue #779/#781 and D-200.

Warning 2 (governance gate robustness) — two real gaps in scripts/check_scratch_dir_usage.py:

Allowlist ratchet had no enforcement. find_violations only ever compared the current tree against whatever ALLOWLIST says in the same commit, so a PR could pad a file's allowed count upward in the same diff that adds new occurrences and still pass clean; a deleted/renamed file's ALLOWLIST entry also never got re-validated once tracked_rust_files() stopped returning it. Added two tests to scripts/test_check_scratch_dir_usage.py:
- test_allowlist_values_never_exceed_the_real_tracked_trees_current_count — compares every checked-in ALLOWLIST value directly against the real tree's current occurrence count for that file (independent of find_violations/validate, which alone can't catch a same-commit pad-up).
- test_every_allowlisted_file_still_exists_in_the_tracked_tree — asserts every ALLOWLIST key is present in tracked_rust_files()'s output, catching stale entries left by a deletion/rename.
Import-alias evasion. The detection regex required the literal temp_dir().join( shape, so use std::env::temp_dir as get_scratch_root; followed by get_scratch_root().join(...) evaded it entirely — this is ordinary idiomatic Rust, not just the already-documented let-binding-split evasion. Added a narrower ALIAS_PATTERN matching use ... temp_dir as <name> (covering both std::env::temp_dir as and the shorter env::temp_dir as form for a caller with env already in scope), folded its matches into occurrence_count, extended the module docstring's "Known scope limitation" paragraph to name this case honestly (it flags the import line, not the aliased call site, since call-site tracking is the data-flow question this script deliberately doesn't attempt), and added test_import_alias_evasion_is_detected + test_import_alias_evasion_via_unqualified_env_path_is_detected proving both forms are caught.

Verification:

python3 -B -m unittest scripts.test_check_scratch_dir_usage -v — 12/12 pass (up from 9, 3 new tests added).
python3 -B -m unittest discover -s scripts -p 'test_*.py' — 951/951 pass.
python3 -B scripts/check_scratch_dir_usage.py — passes against the real tree.
ruby scripts/check_ci_permissions.rb — passes.
cargo check --workspace — clean (no Rust source touched by this follow-up; only docs + two Python scripts changed).

A fresh ievo:deep-reviewer pass wasn't re-run for this follow-up — these are warning-level, narrowly-scoped fixes (one doc-table row, two new unit tests, one additional regex + docstring clarification), not a design change, so a repeat pass isn't strictly required per AGENTS.md's review-focus guidance. Flagging here in case a maintainer wants a fresh look before merge regardless.

…sequence D-199's Consequences section claimed "a narrowing fact never survives a reassignment of the narrowed name" without qualification -- true only for the constructs D-199 itself covered (straight-line bodies and if/else joins), never true in general once while/for loops and try/except handlers entered scope, per the two counterexamples this round's soundness fix addresses. D-199 is left unedited per this repository's decision-log convention (never hand-edit an accepted entry); this new entry narrows its claim and records the kill-prescan design, alternatives considered, and consequences. Regenerated docs/decisions/README.md via scripts/generate_decisions_index.py. Verified D-201 is the next free decision number: origin/main tops out at D-198, and D-199/D-200 are already claimed by this PR (#780) and PR #786 respectively. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Part 1 of #779 (issue #781): a real disk-fill incident (70,000+ leaked temp directories, 70+ GiB) traced to ~384 ad hoc std::env::temp_dir().join(...) call sites across 36 test files plus two unconditional production leaks in src/main.rs, none of them cleaned up reliably on panic/early return, and some colliding on name. New crates/pycc_scratch crate exposes ScratchDir: an RAII handle that derefs to Path and removes its directory tree on Drop, including during panic unwinding, named pycc_{category}_{pid}_{nanos}_{seq} (full epoch nanoseconds + a per-process atomic counter) for collision-safe creation under concurrent use -- fixing the exact PID-only collision defect the prior ad hoc TempTestDir pattern had. No external dependency added. scripts/check_scratch_dir_usage.py (wired into the governance CI job) rejects any tracked .rs file with more raw temp_dir().join(...) occurrences than a checked-in per-file count allowlist records for it, so new code cannot add to the leak while the pre-existing backlog is migrated by Parts 2/3 (#782/#783). Adds D-200 recording the crate/lint design, updates docs/TESTING.md and docs/DELIVERY_PLAN.md, and does not touch any of the ~384 existing call sites or change src/main.rs's behavior -- that is Parts 2-4's scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Warning 1: docs/ARCHITECTURE.md's workspace crate table omitted pycc_scratch even though it is a real Cargo.toml workspace member -- add its row alongside the existing pycc_artifact_layout style entry. Warning 2: close two robustness gaps in scripts/check_scratch_dir_usage.py flagged by the pinned reviewer: - The allowlist ratchet (a listed file's count may only go down, never up) had no test enforcing it, and a deleted/renamed file's stale ALLOWLIST entry was never re-validated. Add test_allowlist_values_never_exceed_the_real_tracked_trees_current_count and test_every_allowlisted_file_still_exists_in_the_tracked_tree to scripts/test_check_scratch_dir_usage.py. - The detection regex required the literal `temp_dir().join(` shape, so `use std::env::temp_dir as get_scratch_root;` followed by `get_scratch_root().join(...)` evaded it entirely -- ordinary, idiomatic Rust, not just the already-documented let-binding-split case. Add a narrower ALIAS_PATTERN that flags the `use ... as` import line itself, fold it into occurrence_count, extend the module docstring's "Known scope limitation" paragraph, and add test_import_alias_evasion_is_detected (+ the short `env::temp_dir as` path variant) proving it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…782) Retire the crate-local tempfile_dir()/TempTestDir wrapper in favor of pycc_scratch::ScratchDir across crates/pycc_codegen/src/tests.rs (273 call sites) and bigint_rc.rs (2 call sites), including the file's one raw std::env::temp_dir().join(...) occurrence. Deletes tests_support.rs and its #[path]/pub(crate) re-export wiring, adds pycc_scratch as a dev-dependency to root Cargo.toml and crates/pycc_codegen/Cargo.toml, and shrinks check_scratch_dir_usage.py's ALLOWLIST to match. Part of #782 (Batch A per the published implementation plan); the remaining batches (src/main.rs + project_config.rs, and the tests/*.rs integration files) are separate follow-up PRs, so #782 stays open. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

crates/pycc_codegen/Cargo.toml already declares pycc_scratch as its own dev-dependency (path = "../pycc_scratch"), which is sufficient for the crate's tests. The duplicate entry in root Cargo.toml's [dev-dependencies] was unused, and it tripped the D-091 bench-manifest fingerprint check in frontend-perf-measure: that check hard-aborts on any change to root Cargo.toml's [dev-dependencies]-onward tail by design, since anything there could affect the pinned benchmark's measurement rather than the compiled artifact it measures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

The rebase onto origin/main required renumbering this PR's D-200 decision to D-201 because main independently claimed D-200 for an unrelated decision (raising the llms.txt aggregate budget). This ARCHITECTURE.md crate-table reference was missed in the rebase conflict resolution commit and still pointed at the old number. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

main merged issue #150's fix (a `tests/` integration test using the raw `std::env::temp_dir().join(...)` pattern) after this branch's original ALLOWLIST snapshot commit but before this PR's own rebase onto main. The snapshot is defined as "every file containing the pattern at the commit where the gate takes effect" -- that commit is this rebased merge, not the pre-rebase branch tip -- so recording this file fulfills the snapshot rather than breaching its one-time property. Migrating the file onto ScratchDir is out of scope here: it would require re-adding pycc_scratch as a root [dev-dependencies] entry, which f79bb2b already tried and reverted because it trips D-091's bench-manifest fingerprint gate in frontend-perf-measure -- the same blocker tracked against #782 Batch B (PR #793). It stays tracked under #782's Part 2 migration scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

These two tests (a_walrus_in_an_if_test_predeclares_its_storage_slot_and_runs and a_walrus_with_an_optional_int_value_and_a_repeated_target_name_predeclare_correctly) were added by #774 before this branch's own base commit, but the rebase's tests.rs conflict resolution missed converting their tempfile_dir(...) calls to pycc_scratch::ScratchDir::new(...) along with the rest of the file's Batch A migration (71f497a), leaving two dangling references to the now-removed tempfile_dir helper and a cargo build failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…D-201 The scratch-dir gate's ALLOWLIST addition for this file was justified only in the commit message; move that reasoning into the decision record itself so a reviewer reading D-201 sees why the "one-time snapshot" allowlist gained an entry during this PR's rebase.

…ften ALLOWLIST ratchet claim, refresh session handoff The D-068 deep-reviewer pass against the rebased diff (merge-base af2384f) found one blocker and three actionable warnings, all doc drift introduced by the earlier rebase conflict resolution: - D-201 stated Part 1 "registers pycc_scratch in the workspace [members] list only" and "changes zero existing call sites", but the rebase's conflict resolution in crates/pycc_codegen (retiring tests_support.rs's TempTestDir/tempfile_dir directly onto ScratchDir, since it was already slated for deletion on both sides of the conflict) added pycc_scratch as a pycc_codegen dev-dependency and rewrote every call site in tests.rs and bigint_rc.rs. Corrected both claims and narrowed Part 2's (#782's) remaining scope description accordingly. - docs/TESTING.md's "Scratch directories" section presumed tests_support.rs still existed; added a note describing its retirement. - The ALLOWLIST "never up" ratchet was documented (in both D-201 and check_scratch_dir_usage.py's own docstring) as a mechanically enforced property, but the gate only compares the current tree against the checked-in snapshot value for the same commit -- it has no visibility into a prior commit's ALLOWLIST entry, so a PR that pads both the real count and the ALLOWLIST value together would still pass. Softened the language in both places to describe this accurately as a review convention backstopped by the D-068 pass, not a mechanical guarantee. - The unmerged session handoff file (part of this same diff, not a foreign prior session's entry) was stale in four places: it predated the rebase, described the D-068 pass as not yet run, and still assigned tests_support.rs's retirement to Part 2 as future work. Updated in place per D-066/D-130's "never preserve an already-completed step as current work" rule. All three notes from the same review pass (the ScratchDir::new category splice, Drop's discarded remove_dir_all Result, and the reviewer's own environment limitation) were confirmed non-issues and need no code change. Verified: check_scratch_dir_usage.py and its own test suite both still pass, and docs/decisions/README.md remains up to date under --check.

Merging origin/main's PR #786 (issue #781, D-201 scratch-dir abstraction) into this branch removed crates/pycc_codegen/src/tests_support.rs and its tempfile_dir helper in favor of pycc_scratch::ScratchDir, but PR #794's own 16 except*/ExceptionGroup unit tests in crates/pycc_codegen/src/tests.rs still called the now-gone tempfile_dir, breaking the merged build with 16 E0425 errors. Migrated every call site to pycc_scratch::ScratchDir::new(...).expect(...), matching every other test in this file post-#786. Also fixes a scripts/check_scratch_dir_usage.py drift the merge exposed: tests/issue_542_except_star.rs (a tests/ integration-test file, which cannot depend on pycc_scratch per D-201's own documented root-crate constraint) has 41 raw temp_dir().join(...) call sites and was missing from ALLOWLIST entirely, and tests/issue_382_exceptions.rs's recorded count (57) was stale by one -- PR #794's own earlier history removed a test from that file (397d9b2) without updating the allowlist. Added the former with its real count and lowered the latter to match, per the script's own documented remediation for exactly this drift. cargo build --workspace, cargo test --workspace (1300+ tests, 0 failed), cargo clippy --workspace --all-targets -- -D warnings, cargo doc --workspace --no-deps, scripts/check_scratch_dir_usage.py, scripts/test_check_scratch_dir_usage.py, and scripts/generate_decisions_index.py --check are all green against this merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…conflict in the #542 handoff Records that all five external Codex-bot review threads on PR #794 were replied to and resolved via GraphQL, and documents a second concurrent-writer overlap: origin/main advanced with PR #786 (issue #781's pycc_scratch crate) while this session was re-verifying gates, causing a merge conflict this session started resolving locally before a second writer independently pushed its own resolution (53ea7d2) first. Per the project's "concurrent background actor" operating lesson, this session discarded its own unpushed merge commit and re-ran every gate (build, test, clippy, doc, 100.00%/100.00% coverage) against the authoritative pushed head instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…sequence D-199's Consequences section claimed "a narrowing fact never survives a reassignment of the narrowed name" without qualification -- true only for the constructs D-199 itself covered (straight-line bodies and if/else joins), never true in general once while/for loops and try/except handlers entered scope, per the two counterexamples this round's soundness fix addresses. D-199 is left unedited per this repository's decision-log convention (never hand-edit an accepted entry); this new entry narrows its claim and records the kill-prescan design, alternatives considered, and consequences. Regenerated docs/decisions/README.md via scripts/generate_decisions_index.py. Verified D-201 is the next free decision number: origin/main tops out at D-198, and D-199/D-200 are already claimed by this PR (#780) and PR #786 respectively. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…p a raw temp_dir call D-068 eighth review round (against e77c1b6..HEAD): `pycc_mir::stmt`'s `TryStar` arm still lowered its body/handler/orelse/finalbody positions with a raw `.map(lower_stmt)` loop, which never calls `apply_post_if_narrowing` and never runs the `Try` arm's `pre_handlers_narrowed`/`apply_kill_prescan`/ `restore_narrowing` sequence around the handler loop -- silently dropping guard-clause narrowing propagation and handler-reentry kill-prescan protection for the entire `except*` construct, even though the sibling type-checker-side fix (`check_try_star_stmt` routing through `check_stmt_sequence_shared`) already restored the equivalent behavior on the checker side. Fixed by routing every `TryStar` position through `lower_scoped_body` and mirroring the `Try` arm's handler-loop sequence exactly, with two new MIR-level regression tests (`a_try_star_except_handler_reached_after_a_try_body_kill_does_not_unwrap_the_read`, `an_early_return_guard_inside_a_try_star_body_narrows_the_read_after_it`). Also fixed five stale `check_stmt_shared` references in `crates/pycc_types/src/exception/except_star_tests.rs` comments left over from the same rename. Separately, `tests/issue_769_optional_narrowing.rs` called `std::env::temp_dir().join(...)` directly with manual cleanup -- `scripts/check_scratch_dir_usage.py` (added by #786's `pycc_scratch` crate landing, itself absorbed into this branch's rebase base) flags this as a new raw-temp-dir call site. Migrated to `pycc_scratch::ScratchDir::new(...)` (added as a root-crate dev-dependency), which also removes the now-redundant manual `remove_dir_all` since `ScratchDir`'s `Drop` handles cleanup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

This was referenced Aug 25, 2026

Part 2 of #779: Migrate existing test files onto pycc_scratch::ScratchDir #782

Open

Migrate crates/pycc_codegen test wrapper onto ScratchDir (Batch A of #782) #792

Merged

rotnov force-pushed the feat/issue-781-scratch-dir branch from d9d3582 to 866f58a Compare August 26, 2026 11:20

rotnov and others added 9 commits August 26, 2026 12:20

rotnov force-pushed the feat/issue-781-scratch-dir branch from 866f58a to f901432 Compare August 26, 2026 11:26

rotnov merged commit 436913e into main Aug 26, 2026
28 of 30 checks passed

Navigation Menu

Add shared pycc_scratch scratch-directory abstraction + repo lint gate - #786

Add shared pycc_scratch scratch-directory abstraction + repo lint gate#786
rotnov merged 9 commits into
mainfrom
feat/issue-781-scratch-dir

rotnov commented Aug 25, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 25, 2026

Uh oh!

rotnov commented Aug 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

rotnov commented Aug 25, 2026

Summary

Gates run locally (all green)

Test plan

Uh oh!

chatgpt-codex-connector Bot commented Aug 25, 2026

Uh oh!

rotnov commented Aug 25, 2026

D-068 review follow-up: both warnings addressed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant