fix(gates): close class G1 — deterministic temp-file publish - #2147
Open
silversurfer562 wants to merge 2 commits into
Open
fix(gates): close class G1 — deterministic temp-file publish#2147silversurfer562 wants to merge 2 commits into
silversurfer562 wants to merge 2 commits into
Conversation
The durable shape for "replace this file atomically" is
tempfile.mkstemp in the target's own directory then Path.replace. The
unsafe shape derives the temp path from the target (with_suffix(".tmp"),
dir / f"{name}.tmp", parent / (name + ".tmp")) and renames it into
place. Two processes writing the same target then pick the SAME temp
path: one truncates the other's partial write and the rename publishes
whichever half won. Measured in the library review at the memory layer:
two OS processes x 20 remember() landed 22 of 40 records.
The memory layer was fixed in the tier-1 PR (#2128) but the class had
no gate, so it stayed live everywhere else. This closes it.
Gate (tests/unit/gates/test_deterministic_tmp_gate.py): an AST rule
scanning the shipped tree — not a pinned list of known sites, which
cannot stop the class appearing in a module nobody has written yet.
Calibrated at 10 hits across exactly the 9 modules the class register
predicted independently, with zero false positives on the shape
fixtures and on the reference implementation.
Sweep-fix, all 10 sites. Each keeps its existing error contract:
mkstemp raises only OSError, which every guarded site already catches.
Three needed more than a mechanical swap:
- gates/envelope.py deliberately does NOT run _validate_file_path (it
would resolve the macOS /var -> /private/var symlink and desync from
the caller's load path), so it uses mkstemp inline rather than the
shared atomic_write_text helper, which validates.
- telemetry/usage_tracker.py catches only OSError; atomic_write_text's
ValueError would have escaped and crashed a best-effort write.
- pipeline_learner/scaffold.py publishes two files under both-or-neither
rollback (RR-7); both temp names are now unique and the rollback still
covers both.
authoring/polish.py also moves rename() -> replace(): rename fails on
Windows when the destination exists, which is the common case for a
cache re-put.
Path-validation gate widened, and this is the part worth reading. The
scanner knew .write_text / .open(mode=w) / shutil / os ops. It did not
know mkstemp + os.fdopen. Six modules therefore dropped OFF the offender
list the moment they adopted the safe idiom — still writing files, just
invisible — and the shrink-only ratchet then demanded their allowlist
entries be removed, which would have made the gate assert something
untrue. Taught the scanner tempfile.{mkstemp,mkdtemp,NamedTemporaryFile,
TemporaryFile} and os.fdopen-for-write, with unit tests pinning both.
That widening surfaced two PRE-EXISTING writers that were never visible:
- authoring/fact_check/tutorial_static_check.py — NamedTemporaryFile
with no caller-supplied dir, so the path is entirely OS-chosen.
Allowlisted, nothing to validate.
- ops/session_summary_cache.py — writes <attune_home>/ops/
session_summaries/<id>.json. Allowlisted, but the <id> component is
interpolated UNVALIDATED, so a traversing id would write outside the
cache dir. Recorded in the allowlist comment and chipped as its own
task rather than waved through or silently widened into this PR.
Two tests were rewritten because the class-M ruling applies to them:
they mocked the very mechanism the fix changed, so they stopped testing
anything the moment it moved.
- curator test_put_swallows_write_failure patched Path.write_text; it
now chmods the cache dir unwritable — a real refusal that cannot go
stale (skipped on Windows and as root).
- pipeline_learner rollback test keyed its fake on the deterministic
temp NAME; it now keys on the destination, and asserts no temp file of
any name survives.
Receipts. The gate fails against pre-fix source and passes after — it is
decisive for the class. The two rewritten tests pass both before and
after, which is correct: they cover contracts this fix did not change,
and the rewrite only made them mechanism-independent. Stated plainly:
the gate proves the SHAPE across the tree, not that each of the 10 sites
is concurrency-safe under load; the measured survival receipt exists for
the memory layer only. 1532 passed / 2 skipped / 3 xfailed across the
gates, help, curator, telemetry and pipeline_learner suites, run
serially.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
All five Windows lanes errored during COLLECTION:
@pytest.mark.skipif(os.geteuid() == 0, reason="root ignores the write bit")
E AttributeError: module 'os' has no attribute 'geteuid'
skipif conditions are evaluated at collection time, so the separate
os.name == "nt" decorator never got a chance — the AttributeError took
down the whole tests/unit/curator/test_cache.py module before any skip
could apply.
Collapse both conditions into one module-level constant computed with
getattr(os, "geteuid", lambda: 1), which is safe when the attribute is
absent. Verified the Windows path resolves to True (skip) rather than
raising.
The repo's three pre-existing geteuid uses already short-circuit
correctly (os.name == "posix" and ..., or hasattr(os, "geteuid") and ...)
— this was the only unsafe one, introduced in the previous commit.
Co-Authored-By: Claude Opus 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.
Closes class G1 from the library review — the first of the four fixed-but-ungated classes to get its gate.
The class
The durable shape for "replace this file atomically" is
tempfile.mkstempin the target's own directory thenPath.replace. The unsafe shape derives the temp path from the target —with_suffix(".tmp"),dir / f"{name}.tmp",parent / (name + ".tmp")— and renames it into place. Two processes writing the same target then pick the same temp path: one truncates the other's partial write and the rename publishes whichever half won.Measured at the memory layer during the review: two OS processes × 20
remember()landed 22 of 40 records. The memory layer was fixed in #2128, but the class had no gate, so it stayed live everywhere else.The gate
tests/unit/gates/test_deterministic_tmp_gate.py— an AST rule scanning the shipped tree, not a pinned list of known sites (a list cannot stop the class appearing in a module nobody has written yet).Calibration: 10 hits across exactly the 9 modules the class register predicted independently, zero false positives on the shape fixtures or the reference implementation. Fails against pre-fix source, passes after.
The sweep-fix — all 10 sites
Each keeps its existing error contract:
mkstempraises onlyOSError, which every guarded site already catches. Three needed more than a mechanical swap:gates/envelope.py_validate_file_path(it would resolve the macOS/var→/private/varsymlink and desync from the caller's load path), so it usesmkstempinline rather than the sharedatomic_write_texthelper, which validatestelemetry/usage_tracker.pyOSError;atomic_write_text'sValueErrorwould have escaped and crashed a best-effort writepipeline_learner/scaffold.pyauthoring/polish.pyalso movesrename()→replace(): rename fails on Windows when the destination exists, which is the common case for a cache re-put.The part worth reading — the path-validation gate went blind
The scanner knew
.write_text/.open(mode=w)/shutil/osops. It did not knowmkstemp+os.fdopen. So six modules dropped off the offender list the moment they adopted the safe idiom — still writing files, just invisible — and the shrink-only ratchet then demanded their allowlist entries be removed, which would have made the gate assert something untrue.Taught the scanner
tempfile.{mkstemp,mkdtemp,NamedTemporaryFile,TemporaryFile}andos.fdopen-for-write, with unit tests pinning both directions (write mode detected, read mode ignored).That widening surfaced two pre-existing writers that were never visible:
authoring/fact_check/tutorial_static_check.py—NamedTemporaryFilewith no caller-supplieddir, so the path is entirely OS-chosen. Allowlisted; nothing to validate.ops/session_summary_cache.py— writes<attune_home>/ops/session_summaries/<id>.json. Allowlisted, but the<id>component is interpolated unvalidated, so a traversing id would write outside the cache dir. Recorded in the allowlist comment and chipped as its own task rather than waved through, or silently widened into this PR.Two tests rewritten under the class-M ruling
Both mocked the very mechanism this fix changed, so they stopped testing anything the moment it moved:
curator::test_put_swallows_write_failurepatchedPath.write_text; it nowchmods the cache dir unwritable — a real refusal that cannot go stale (skipped on Windows and as root).pipeline_learnerrollback test keyed its fake on the deterministic temp name; it now keys on the destination and asserts no temp file of any name survives.Receipts
test_no_deterministic_tmp_publishtest_put_swallows_write_failuretest_partial_write_failure_rolls_back_both_or_neitherThe two rewritten tests passing both ways is correct — they cover contracts this fix did not change; the rewrite only made them mechanism-independent.
Stated plainly: the gate proves the shape across the tree, not that each of the 10 sites is concurrency-safe under load. The measured survival receipt exists for the memory layer only.
1532 passed / 2 skipped / 3 xfailed across the gates, help, curator, telemetry and pipeline_learner suites, run serially (
-p no:xdist).🤖 Generated with Claude Code