compass(tests): the abandoned-staging pin checks what is inside, not only that it stands

[jgong5]

Closes #216

What changed

One assertion, in test_an_abandoned_staging_directory_does_not_block_a_publish
(tests/compass/test_artifact_store.py). After the publish, the test used to check only
abandoned.is_dir(). It now also checks that the abandoned directory holds exactly the
half-built file it was given, with its bytes unchanged:

assert {item.name: item.read_bytes() for item in abandoned.iterdir()} == {
    half_built.name: b"half"
}

The half-built file is now bound to a local (half_built) so the assertion can name it,
and the docstring gains a four-line paragraph saying why is_dir() alone is not enough. The
is_dir() assertion stays, so the original defect still fails with its own message.
No production code changed.

Named result: re-measured, not cited

Re-measured at the current tip. The brief's numbers came from 065f34f06. Since then the
file gained a test (there are now 40, not 39) and the assertion moved from line 503 to 571
(577 at head).

The corruption is a line-count-preserving mutation of ArtifactStore._write
(atom/compass/artifacts/store.py, 493 lines before and after). Line 471 empties every
sibling staging directory of the destination but leaves each directory in place:

        destination.parent.mkdir(parents=True, exist_ok=True); [stale.unlink() for stale in destination.parent.glob(f".{destination.name}.*/*")]

Node 18, xiaobizh_n18_cpu. Each tree is a git archive copy staged with docker exec -i,
and atom.__file__ resolves under that tree's root on every run.
Command: pytest -q tests/compass/test_artifact_store.py.

tree	corruption	result
tip e1da5404e	cleared, not removed	40 passed: blind
head e55679e99	cleared, not removed	1 failed, 39 passed: tests/compass/test_artifact_store.py::test_an_abandoned_staging_directory_does_not_block_a_publish, at :578, assert {} == {'prices.dp0o...son': b'half'}
tip e1da5404e	none (null control)	40 passed
head e55679e99	none (null control)	40 passed
tip e1da5404e	pre-fix staging code reinstated	1 failed, 39 passed, same node id, at :571 assert abandoned.is_dir()
head e55679e99	pre-fix staging code reinstated	1 failed, 39 passed, same node id, at :577 assert abandoned.is_dir()

The "pre-fix staging code" mutation takes the three lines of fc4c47560's _write (a
fixed .<entry>.publishing name, shutil.rmtree, mkdir) and puts them in place of the
three mkdtemp lines, 472–474. So the pin still bites on the defect it was written for,
and it bites through the original assertion, not the new one.

The whole table was also measured once at fa5180b62, the tip when this was claimed.
#164 landed during the work, and the branch was rebased onto it. The results were
identical there (40 / 40 / 1F+39 / 40 / 1F+39 / 1F+39).

Gate 1: CPU tier, as a delta against a measured control

Each tree used its own scripts/compass/gate_cpu.sh, with .compass-commit and
.compass-changed stamps written from the same rev-parse as the archive. Every run was
bounded by timeout -k 10 1500, was unpiped, and ran one at a time.

	commit stamp	passed	skipped	xfailed	failed	GATE_CPU_RC
control	e1da5404e	5044	149	3	0	0
head	e55679e99	5044	149	3	0	0

• Delta: 0 / 0 / 0. No added node ids: the change is an assertion inside an existing
  test. The collected ids of test_artifact_store.py are identical on both sides (40).
• The same pair at fa5180b62 / a03c67ba9 (before the rebase) gave 5004 / 149 / 3,
  GATE_CPU_RC=0, on both sides.
• The gate printed gpu: not required on both sides.
• AST lines:
  • production: 0;
  • tests: +10 / −1 physical lines in one function. That is one assignment added,
    one statement rewritten to use it, one assert added, and a four-line docstring paragraph (plus its blank line).
• ruff check and black --check on the file: rc 0 and rc 0.
• git merge-tree --write-tree e1da5404e e55679e99 → 58a48d516ba266a99b5994b1dbc01e3764903b87, clean.

Gate 2

No new test function. The brief sizes the fix as roughly one assertion, and it lands
inside the pin it repairs. It is CPU-only and runs in the CPU tier above.

What surprised me

• The notes defect named in the brief is already fixed and already pinned. At this
  tip, publish refuses non-text notes before any staging exists, and _write's
  except also catches TypeError/ValueError.
  • I reverted both, a line-preserving two-line mutation (if False: at 205, and
    except OSError at 484). The TypeError and the leaked .<entry>.<random> staging
    directory came back, confirmed with a probe.
  • test_artifact_store.py stayed at 40 passed.
  • test_artifact_invalidation.py failed 2:
    test_notes_that_are_not_text_are_refused_and_leave_nothing_behind and
    test_a_publish_that_fails_on_the_way_to_the_rename_leaves_nothing_behind.
  • So nothing is unpinned there. The pin just lives in the other file. Nothing was
    changed for it.
• The tip moved during the work, so everything was re-measured after the rebase rather
  than carried across.

Pin inventory: what each other pin in this file does not notice

"Measured" means a line-preserving mutation was run against test_artifact_store.py and
test_artifact_invalidation.py together. Everything else is read from the code, not
measured.

pin	does not notice	reachable?
test_the_six_artifacts_are_the_ones_the_key_table_declares	Field order in KEY_FIELDS against the table. Also a field whose words happen to appear elsewhere in the same cell, because it checks count plus a substring.	Order: only if the order feeds dirname. Not measured.
test_a_price_list_asked_for_by_path_is_refused_by_name	A path in model (only source_root and a path= field are tried).	Not measured.
test_two_keys_never_share_a_directory	Two keys that differ only in model or source_root. Only width varies.	Low: dirname carries a hash of the key.
test_the_bare_name_is_never_produced, test_four_ranks_do_not_resolve_to_one_name	Any axis other than dp/tp above width 1. pp and pcp are never above 1 anywhere in this file.	Only if the suffix stops being generic over AXES.
test_an_entry_round_trips_through_the_naming_function	notes, fingerprints and gates round-tripping. The first is checked by the notes-rewrite test, the other two in test_artifact_invalidation.py.	Covered elsewhere.
test_a_notes_only_rewrite_of_a_handed_off_entry_is_refused	Litter left beside the entry by the refused second publish. It checks the entry, not its parent directory. Today the refusal comes before staging, so nothing is created.	Only if the overwrite check moves after staging.
test_a_member_changed_after_hand_off_is_refused	Whether entry.json itself is read-only. Only one member's mode is checked.	Yes, measured. Line 482 → item.chmod(0o444 if item.name != ENTRY_FILE else 0o644) gives 118 passed across both artifact test files. Nothing notices a writable entry.json.
test_a_hand_edited_entry_is_refused_by_name	A hand edit that leaves entry.json well-formed, for example changed notes. read accepts it and reports a new digest. The two exception-name cases ("KeyError", "ValueError") are loose needles.	The first is the store's design: identity is the digest, so there is no refusal to expect. The loose needles were not measured.
test_a_truncated_entry_is_refused_by_name, test_a_missing_entry_names_the_key_that_missed, test_an_entry_moved_by_hand_is_found_out	Nothing found beyond their own message substrings.	—
test_an_empty_directory_in_the_way_is_not_silently_replaced	A non-empty but unreadable directory in the way. It goes through the same _refuse_overwrite branch, so it is probably fine.	Not measured.
test_an_abandoned_staging_directory_does_not_block_a_publish (this PR)	A staging directory for a different key being touched. The abandoned directory shares the destination's prefix.	Not measured.
test_the_store_is_a_directory_convention_with_no_index	An index that is named something other than *index* / *manifest*. The top-level set check ({"price_list"}) catches a sibling, but not a file inside price_list/.	Not measured.
provenance / source-root pins	test_a_source_root_is_located_without_importing_it checks only the first candidate that has not been imported yet. test_a_tree_that_is_neither_is_refused depends on the box having no stamp in any ancestor. It asserts that, so it fails loudly rather than silently.	Environmental. No gap found.

Proposed follow-up (not filed). Pin entry.json read-only after publish, next to the
member-mode check. It is the one measured green mutant. Today a writable entry.json
turns a hand edit from "had to chmod first" into a silent write, and read then answers
under the edited document.

What was left undone

• The follow-up above is not filed.
• The inventory rows marked "not measured" are read from the code only.
• Gate 4 (review) belongs to the coordinator.

🤖 Generated with Claude Code

[jgong5]

Review of PR #260 (issue #216), head e55679e99e7945162d41d1d4195e83ff096eac13

Verdict: APPROVE at e55679e99e7945162d41d1d4195e83ff096eac13. No blocking issues. No inline findings: nothing in the diff needed a line comment.

Principles 1 to 8 and AI_DEV_RULES.md were read first. Each ruling below names the principle it rests on.

1. The named result, reproduced independently (principle 8)

Setup. Node 18, xiaobizh_n18_cpu. Each tree was a git archive copy shipped in with docker exec -i, into a private path in the container's own filesystem. Nothing was written to the shared mount. atom.__file__ resolved under the staged root on every run:

• .../stage/tip/ATOM/atom/__init__.py
• .../stage/head/ATOM/atom/__init__.py

Trees. tip f5003b255 (the current tip; store.py and test_artifact_store.py are byte-identical to e1da5404e) and head e55679e99.

Mutations. Every mutation is line-count-preserving (493 lines) and applied to atom/compass/artifacts/store.py.

My cleared-but-not-removed mutation deliberately uses a different mechanism from the developer's unlink glob. At :471 it does rmtree and then mkdir on every sibling .<entry>.*:

        destination.parent.mkdir(parents=True, exist_ok=True); [(shutil.rmtree(s), s.mkdir()) for s in destination.parent.glob(f".{destination.name}.*")]

Commands. Each case ran pytest -q tests/compass/test_artifact_store.py, and again with tests/compass/test_artifact_invalidation.py added (the "both" column). Every run was bounded by timeout -k 10 300.

tree	mutation	store file	both files	failing node id / assertion
tip f5003b255	none (null)	40 passed	118 passed	-
head e55679e99	none (null)	40 passed	118 passed	-
tip	cleared, not removed	40 passed: blind	118 passed	-
head	cleared, not removed	1 failed, 39 passed	1 failed, 117 passed	tests/compass/test_artifact_store.py::test_an_abandoned_staging_directory_does_not_block_a_publish, :578 assert {} == {'prices.dp0o...son': b'half'}
tip	pre-fix staging (fc4c47560's three lines in place of :472-474)	1 failed, 39 passed	1 failed, 117 passed	same node id, :571 assert abandoned.is_dir()
head	pre-fix staging	1 failed, 39 passed	1 failed, 117 passed	same node id, :577 assert abandoned.is_dir()

This matches the dev record row for row, reproduced through a different corruption mechanism.

The pin still bites on its original defect. It does so through the original is_dir() line, which is why keeping that line is right: the pre-fix failure keeps its own message instead of becoming a FileNotFoundError out of iterdir().

The pristine store.py md5 (b1f635d5...) was restored and checked after each tree's battery.

2. Is the new assertion too tight? No. It pins exactly the named property (principles 3 and 8)

The assertion constrains exactly one thing: the contents of another publisher's staging directory, as name-to-bytes. It says nothing about the publisher's own staging, the destination, or the parent directory.

Measured. A store that writes a marker into its own staging and removes it before the rename stays green: 40 passed at head, and 118 passed across both files. The mutation was line-preserving:

• :474 ); (staging / ".marker").write_bytes(b"m")
• :483 (staging / ".marker").unlink(); os.rename(...)

Read, not measured. The other plausible future changes:

• A reaper that removes stale staging. It already fails the pre-existing is_dir(), so the new line adds no new constraint there. Allowing a reaper would be a deliberate change to the docstring's "a crashed one is not in anybody's way", not collateral damage.
• A store that writes a lock or marker into sibling staging. That is touching another publisher's staging, which is precisely the property the test names.

So the only thing the new line refuses that is_dir() did not is the neighbouring defect that #216 names. The staging layout is not over-specified.

3. Pin inventory in the PR body

Measured row: confirmed. Line :482 becomes item.chmod(0o444 if item.name != ENTRY_FILE else 0o644). That gives 40 passed on the store file and 118 passed across both artifact files, at tip and at head. Nothing notices a writable entry.json.

Follow-up ruling: file it (principle 8). It is a real green mutant on behaviour the store implements deliberately: _write chmods every staged item to 0o444, including entry.json. Three tests also presume the property without asserting it. They chmod(0o644) entry.json before hand-editing it:

• test_artifact_store.py:490 and :509
• test_artifact_invalidation.py:669

The fix is the same size as this PR: one assertion beside :422's member-mode check. It should be its own issue, not folded in here. It is outside #216's named result, and this PR should stay one-assertion-sized (principles 3 and 5).

notes TypeError claim: true. I reverted both guards with a line-preserving pair:

• :205 if False:
• :484 except OSError as clash:

With both reverted, test_artifact_store.py stays at 40 passed, blind. With test_artifact_invalidation.py added, the result is 2 failed, 116 passed:

• tests/compass/test_artifact_invalidation.py::test_notes_that_are_not_text_are_refused_and_leave_nothing_behind
• tests/compass/test_artifact_invalidation.py::test_a_publish_that_fails_on_the_way_to_the_rename_leaves_nothing_behind

So the brief's "pre-existing and unpinned" was stale. It is fixed and pinned, and the pin lives in the other file.

Rows marked "not measured". These are honestly labelled as read-from-code. None of them is a blocker for this PR.

4. No design-doc references

The added lines (docstring paragraph, half_built, the assertion) contain no design-doc numbers, no principle citations and no gate labels.

5. Gate 1: the tree that will land

The merge tree. git merge-tree --write-tree f5003b255 e55679e99 gives d5df206f64d6a6a0c50df3a033f1d7a0f67747bf. The merge is clean (rc 0). The tip fork/feature/atomcompass_new was re-read at f5003b255 when this review started.

How it was staged. The merged tree was gated once on node 18. It carried:

• its own scripts/compass;
• a .compass-commit stamp holding the tree hash;
• a .compass-changed stamp from git diff --name-only f5003b255 d5df206f6, which is tests/compass/test_artifact_store.py only.

The run was bounded by timeout -k 10 2400 and unpiped. atom.__file__ was /tmp/xiaobizh-pr260-review/stage/merged/ATOM/atom/__init__.py. The gate printed commit: d5df206f6 (stamp) and gpu: not required.

tree	passed	skipped	xfailed	failed	GATE_CPU_RC
merged d5df206f6	5056	149	3	0	0

This is the expected 5056 for tip f5003b255. The 12 above the developer's 5044 at e1da5404e come from #212, which is the only change between the two tips (inferred from the diff, not gated separately). This PR adds no node ids. The gate printed pytest: rc=0. Nothing failed, so there was nothing to check against the flaky TestTheRegionIsNotCopiedPerChunk class.

The staging was removed from node 18 afterwards.