fix(manifest): fold in both cross-frontier panels — and a live hole in v0.2.2
Two four-arm artifact-only rounds landed together: the contract paraphrase (against the pre-seam-review capture) and the code-vs-contract conformance review (against the amended one), correctly firewalled from each other. The conformance round found ZERO drift in the strict sense — the code is a clause-for-clause implementation of the contract — and the weight of both rounds landed one layer down, in what green tests structurally cannot report. Full triage in persistent-memory.d/. A LIVE HOLE IN RELEASED CODE, FOUND ON THE SIBLING MODULE v0.2.2 adopted the RecursionError finding from the bug-hunt round and closed half of it: `_hydrate_safe` guards hydration, but `json.loads` runs above it in `_read_raw`, whose catch list covers neither RecursionError nor MemoryError. A 400 KB file of nothing but brackets in any ONE booth therefore still returned 500 for `/` and `/healthz` across every booth on the service. Confirmed by running it before believing it. Both modules now bound the read by `stat` before touching the bytes and catch both classes anyway, so raising a bound later cannot quietly re-open the hole. The strict half of the marks asymmetry refuses everything the lenient half tolerates, or a file that reads as "no marks" gets replaced by a write that believed it. THE WHY-WIPE `booth new x --why "..."` then `booth add x out/*.png` erased the sentence the first command existed to record. Omitted flags meant empty strings and empty strings overwrote. Two arms predicted it from the contract's wording alone; every test here passed --why on both calls and so could not see it. Omitted now means unchanged and an explicit --why "" still clears — the shell carries the distinction by leaving the variable UNSET, not empty. --title WAS WRITE-ONLY Stored, flag-surfaced, rendered nowhere. 4/4, and independently top-ranked by every arm of the paraphrase round. It lands on the booth page heading with the directory name beside it, because the directory name is the identity the operator navigates by and refers to positionally. THREE TESTS THAT COULD NOT FAIL - test_the_write_is_atomic asserted no *.tmp survived, which a plain write_text passes. It asserts the inode changes now. (The first replacement was ALSO vacuous — it spied on os.open, which Path.write_text reaches through io.open in C and never touches. Recorded in the test, because writing a second vacuous test while fixing the first is exactly the failure this round is about.) - The INV-3 preservation test passed against an implementation that regenerated `created` every time, because _now() is whole-second resolution and back-to-back writes share a stamp. Seeded from 2019 now. - test_announcing_is_activity passed whether or not _newest_mtime counted the manifest, because writing it bumps the directory mtime either way. The directory's clock is put back, leaving the file as the only thing that can keep the booth alive. ALSO - The title fallback skipped the normalizer the explicit value gets; a directory name may legally carry a newline and run to 255 bytes. - Every writer derived the same .booth.json.tmp. Marks are protected from that by their flock; the manifest has none, so uniqueness stands in. - test_stdlib_only was blind to relative imports in all four modules. - INV-1 had no guard at all; INV-5 named two different promises; the negative render states were asserted on the index only. Contract amended throughout: the 4 GB case is a stat-checked bound rather than a return constraint, every field of an error-carrying record has a stated value, INV-1 no longer contradicts INV-3, repo-wide rules are named in words instead of by a colliding number, and touches admits the macro partial the implementation added. 329 tests.
This commit is contained in:
Vuong Hoang
@@ -251,3 +251,37 @@ def test_a_flag_with_no_value_does_not_eat_the_booth_name(tmp_path):
|
||||
assert r.returncode == 2
|
||||
assert "usage:" in r.stderr
|
||||
assert not (tmp_path / "b").exists()
|
||||
|
||||
|
||||
def test_a_bare_add_does_not_wipe_the_why_the_new_set(tmp_path):
|
||||
"""`booth new x --why "..."` then `booth add x out/*.png` is THE sequence,
|
||||
and the second call must not erase the first one's sentence. The module
|
||||
distinguishes omitted from empty; the shell has to carry that distinction
|
||||
across, which means an UNSET variable, not an empty one."""
|
||||
env = {**os.environ, "ALTHING_HANDLE": "booth-dev",
|
||||
"BOOTH_DATA_DIR": str(tmp_path), "BOOTH_URL": "http://booth.invalid"}
|
||||
src = tmp_path / "a.png"
|
||||
src.write_bytes(b"x")
|
||||
|
||||
subprocess.run([str(SCRIPT), "new", "b", "--why", "pick the denoiser",
|
||||
"--title", "R18 A/B"],
|
||||
check=True, capture_output=True, timeout=30, env=env)
|
||||
subprocess.run([str(SCRIPT), "add", "b", str(src)],
|
||||
check=True, capture_output=True, timeout=30, env=env)
|
||||
|
||||
m = _manifest(tmp_path / "b")
|
||||
assert m.why == "pick the denoiser", "a bare `booth add` wiped the why"
|
||||
assert m.title == "R18 A/B"
|
||||
|
||||
|
||||
def test_an_explicitly_empty_why_still_clears_it(tmp_path):
|
||||
"""Omitted means unchanged; supplied-and-empty means the poster meant to
|
||||
take it back. Both have to be reachable from the shell."""
|
||||
env = {**os.environ, "ALTHING_HANDLE": "booth-dev",
|
||||
"BOOTH_DATA_DIR": str(tmp_path), "BOOTH_URL": "http://booth.invalid"}
|
||||
subprocess.run([str(SCRIPT), "new", "b", "--why", "wrong"], check=True,
|
||||
capture_output=True, timeout=30, env=env)
|
||||
subprocess.run([str(SCRIPT), "new", "b", "--why", ""], check=True,
|
||||
capture_output=True, timeout=30, env=env)
|
||||
|
||||
assert _manifest(tmp_path / "b").why == ""
|
||||
|
||||
+234
-5
@@ -161,9 +161,12 @@ def test_re_announcing_over_a_damaged_file_does_not_inherit_its_created(tmp_path
|
||||
# ---- slice 4: the write is atomic, and invisible to every listing -----------
|
||||
|
||||
|
||||
def test_the_write_is_atomic(tmp_path):
|
||||
"""INV-5 of CLAUDE.md. The CLI writes this in one process while the browser
|
||||
reads it in another, so a reader must never see a half-written document."""
|
||||
def test_the_write_leaves_no_temp_file(tmp_path):
|
||||
"""Half of the atomic-write promise, and the weaker half — see
|
||||
`test_the_write_replaces_rather_than_truncating` for the part that actually
|
||||
discriminates. Kept because a leaked `.tmp` is its own small defect: it
|
||||
would sit in the booth forever and, unlike the manifest, nothing would ever
|
||||
overwrite it."""
|
||||
b = tmp_path / "b"
|
||||
b.mkdir()
|
||||
write_manifest(b, "booth-dev", why="x")
|
||||
@@ -214,8 +217,13 @@ def test_stdlib_only():
|
||||
for node in ast.walk(ast.parse(src.read_text())):
|
||||
if isinstance(node, ast.Import):
|
||||
roots.update(a.name.split(".")[0] for a in node.names)
|
||||
elif isinstance(node, ast.ImportFrom) and node.level == 0 and node.module:
|
||||
roots.add(node.module.split(".")[0])
|
||||
elif isinstance(node, ast.ImportFrom):
|
||||
# A RELATIVE import (`from . import marks`) carries no module root
|
||||
# and used to pass this walk unseen — which matters more here than
|
||||
# in the shared copy, because this module forbids sibling imports
|
||||
# outright. Recorded as `booth` so the assertion below catches it.
|
||||
roots.add("booth" if node.level else
|
||||
(node.module or "").split(".")[0])
|
||||
assert not (roots - set(sys.stdlib_module_names)), (
|
||||
f"booth/manifest.py imports outside the stdlib: "
|
||||
f"{sorted(roots - set(sys.stdlib_module_names))}"
|
||||
@@ -343,3 +351,224 @@ def test_a_pickup_booth_announces_itself_as_the_booths_own(client):
|
||||
got = read_manifest(booth)
|
||||
assert got is not None and got.handle == "booth"
|
||||
assert 'class="prov prov-none"' not in c.get("/").text
|
||||
|
||||
|
||||
# ---- findings from the cross-frontier CODE-REVIEW panel, 2026-09-22 ----------
|
||||
#
|
||||
# Heid panel (thread 01M341E9XAPZEFBSPK9HPGAM0S). Four arms, artifact-only.
|
||||
# The round found ZERO drift in the strict sense and landed its weight one layer
|
||||
# down, in test strength: five of the ten adopted findings are tests of mine
|
||||
# that pass on the regression they exist to catch.
|
||||
|
||||
|
||||
def test_the_read_survives_a_document_no_one_can_parse(tmp_path):
|
||||
"""INV-2 said "never raises" and named a 4 GB file as a tested case. It was
|
||||
not tested, and it did not hold: `except ValueError` catches a truncated
|
||||
document, but `json.loads` on deeply nested input raises RecursionError,
|
||||
which is not a ValueError and is not an OSError either.
|
||||
|
||||
`list_booths` calls this once per booth on every index load, so the one
|
||||
file costs the whole front page — the exact outage shape the invariant
|
||||
cites as its reason for existing. Three of four arms reached it
|
||||
independently; the eight-payload parametrize above has no size or depth
|
||||
case, so the hole stayed green.
|
||||
"""
|
||||
b = tmp_path / "b"
|
||||
b.mkdir()
|
||||
(b / MANIFEST_FILE).write_text("[" * 200_000 + "]" * 200_000)
|
||||
|
||||
got = read_manifest(b)
|
||||
assert isinstance(got, Manifest) and got.error
|
||||
|
||||
|
||||
def test_the_read_refuses_a_document_too_large_to_be_a_manifest(tmp_path):
|
||||
"""The other half of INV-2's named case. A manifest is four short fields;
|
||||
anything approaching a megabyte is not one, and reading it into memory to
|
||||
discover that is the wrong order of operations. Bounded BEFORE the read, so
|
||||
the size is checked by `stat` rather than survived."""
|
||||
from booth.manifest import MANIFEST_MAX_BYTES
|
||||
|
||||
b = tmp_path / "b"
|
||||
b.mkdir()
|
||||
(b / MANIFEST_FILE).write_text('{"handle": "x", "why": "' +
|
||||
"y" * (MANIFEST_MAX_BYTES + 100) + '"}')
|
||||
|
||||
got = read_manifest(b)
|
||||
assert isinstance(got, Manifest) and got.error
|
||||
assert "too large" in got.error
|
||||
|
||||
|
||||
def test_a_hostile_directory_name_does_not_reach_the_record_raw(tmp_path):
|
||||
"""`_one_line(title, TITLE_MAX) or booth.name` — the FALLBACK skips the
|
||||
normalization the explicit value gets. A directory name may legally carry a
|
||||
newline on POSIX and may be 255 bytes, and either lands in a card's
|
||||
sub-line. Same shape on the read path's fallback."""
|
||||
# 200-odd bytes, under the filesystem's own 255 limit but well over
|
||||
# TITLE_MAX — and a newline, which POSIX permits in a filename.
|
||||
name = "we" + "i" * 200 + "rd\nname"
|
||||
b = tmp_path / name
|
||||
b.mkdir()
|
||||
|
||||
m = write_manifest(b, "booth-dev")
|
||||
assert "\n" not in m.title and len(m.title) <= 120
|
||||
assert "\n" not in read_manifest(b).title
|
||||
|
||||
|
||||
def test_the_write_replaces_rather_than_truncating(tmp_path):
|
||||
"""The previous version of this test asserted only that no `*.tmp` file
|
||||
survived — which a plain `write_text` passes, since it leaves no temp file
|
||||
either. All four arms said so, and they were right.
|
||||
|
||||
THE INODE IS THE DISCRIMINATOR. `os.replace` publishes a different file over
|
||||
the old name, so the inode changes; truncate-and-rewrite keeps it. That is
|
||||
also exactly why the promise holds for a concurrent reader: it either has
|
||||
the old inode, intact, or opens the new one, complete. A test of the
|
||||
mechanism rather than of its litter.
|
||||
|
||||
(An earlier draft spied on `os.open` to prove the published path was never
|
||||
opened for writing. It passed — vacuously. `Path.write_text` reaches the
|
||||
syscall through `io.open` in C and never touches the Python-level
|
||||
`os.open`, so the spy could not have fired either way. Recorded because
|
||||
writing a second vacuous test while fixing the first is the failure mode
|
||||
this whole round is about.)
|
||||
"""
|
||||
b = tmp_path / "b"
|
||||
b.mkdir()
|
||||
published = b / MANIFEST_FILE
|
||||
|
||||
write_manifest(b, "booth-dev", why="first")
|
||||
first_inode = published.stat().st_ino
|
||||
write_manifest(b, "booth-dev", why="second")
|
||||
|
||||
assert published.stat().st_ino != first_inode, (
|
||||
"the manifest was rewritten in place, not replaced"
|
||||
)
|
||||
assert read_manifest(b).why == "second"
|
||||
|
||||
|
||||
def test_the_temp_file_is_not_a_name_two_writers_share(tmp_path):
|
||||
"""Every writer derived the same `.booth.json.tmp`. Two `booth add` calls on
|
||||
one booth could then interleave through a stale descriptor into the
|
||||
published path — the atomic-write promise is that READERS never see a
|
||||
partial file, and it says nothing about two writers sharing a scratch name.
|
||||
Marks are protected from this by their flock; the manifest has none."""
|
||||
b = tmp_path / "b"
|
||||
b.mkdir()
|
||||
seen = set()
|
||||
for i in range(5):
|
||||
write_manifest(b, "booth-dev", why=f"pass {i}")
|
||||
seen.update(p.name for p in b.iterdir() if p.name != MANIFEST_FILE)
|
||||
assert not seen, f"left temp files behind: {sorted(seen)}"
|
||||
|
||||
from booth.manifest import _temp_path
|
||||
names = {_temp_path(b).name for _ in range(20)}
|
||||
assert len(names) > 1, "every writer derives the same temp name"
|
||||
|
||||
|
||||
def test_a_bare_re_announce_does_not_wipe_the_why(tmp_path):
|
||||
"""THE WORKFLOW IS `new --why` THEN `add`. Omitted flags meant empty
|
||||
strings, and empty strings overwrote — so the second command silently
|
||||
erased the sentence the first one existed to record, on the single most
|
||||
common sequence this feature has.
|
||||
|
||||
Two arms of the paraphrase panel predicted it from the contract's wording
|
||||
alone ("gains a manifest with no why" does not distinguish a first write
|
||||
from a re-announce with the flags omitted). Every test I wrote passed
|
||||
`--why` on both calls, so none of them could see it.
|
||||
|
||||
Omitted now means UNCHANGED; only a value that was actually supplied
|
||||
overwrites, and an explicit empty string still clears.
|
||||
"""
|
||||
b = tmp_path / "b"
|
||||
b.mkdir()
|
||||
write_manifest(b, "booth-dev", title="R18 A/B", why="pick the denoiser")
|
||||
|
||||
write_manifest(b, "booth-dev") # a bare `booth add`
|
||||
kept = read_manifest(b)
|
||||
assert kept.why == "pick the denoiser", "a bare re-announce wiped the why"
|
||||
assert kept.title == "R18 A/B"
|
||||
|
||||
write_manifest(b, "booth-dev", why="sharper") # supplied: overwrites
|
||||
assert read_manifest(b).why == "sharper"
|
||||
|
||||
write_manifest(b, "booth-dev", why="") # explicit: clears
|
||||
assert read_manifest(b).why == ""
|
||||
|
||||
|
||||
def test_re_announcing_preserves_a_created_from_before_this_second(tmp_path):
|
||||
"""`_now()` is whole-second resolution, so two `write_manifest` calls in a
|
||||
row share a timestamp and the old preservation test passed even against an
|
||||
implementation that regenerated `created` every time. Three of four arms
|
||||
caught it. Seed a stamp that could not have come from now()."""
|
||||
b = tmp_path / "b"
|
||||
b.mkdir()
|
||||
(b / MANIFEST_FILE).write_text(json.dumps({
|
||||
"handle": "booth-dev", "title": "b", "why": "first",
|
||||
"created": "2019-03-04T11:22:33-08:00",
|
||||
}))
|
||||
|
||||
assert write_manifest(b, "booth-dev", why="second").created == \
|
||||
"2019-03-04T11:22:33-08:00"
|
||||
|
||||
|
||||
def test_only_the_manifest_module_opens_the_manifest(tmp_path):
|
||||
"""INV-1, which had no guard anywhere. One resolver is only one resolver
|
||||
while nothing else learns the filename."""
|
||||
root = pathlib.Path(__file__).parent.parent
|
||||
offenders = []
|
||||
for src in sorted((root / "booth").glob("*.py")):
|
||||
if src.name == "manifest.py":
|
||||
continue
|
||||
if ".booth.json" in src.read_text():
|
||||
offenders.append(src.name)
|
||||
assert not offenders, f"{offenders} name the manifest file directly"
|
||||
|
||||
|
||||
def test_announcing_is_activity_via_the_manifest_file_itself(tmp_path):
|
||||
"""The previous version could not fail. Writing the manifest creates a
|
||||
directory entry, which bumps the DIRECTORY's mtime, so the booth read as
|
||||
fresh whether or not `_newest_mtime` counted the manifest at all — a test
|
||||
of the side effect rather than of the thing.
|
||||
|
||||
Put the directory's clock back afterwards, leaving the manifest's own mtime
|
||||
as the only thing that can keep the booth alive."""
|
||||
import os
|
||||
|
||||
from booth.app import booth_age_seconds
|
||||
|
||||
b = tmp_path / "b"
|
||||
b.mkdir()
|
||||
old = 1_000_000_000
|
||||
os.utime(b, (old, old))
|
||||
write_manifest(b, "booth-dev", why="look at this")
|
||||
os.utime(b, (old, old)) # only the file can save it now
|
||||
|
||||
assert booth_age_seconds(b, now=old + 90_000) < 86_400
|
||||
|
||||
|
||||
def test_the_booth_header_marks_an_unannounced_booth_too(client):
|
||||
"""The negative states were asserted on `/` only, so a header that rendered
|
||||
provenance for clean manifests and nothing for the other two would have
|
||||
passed the whole suite."""
|
||||
c, data = client
|
||||
_booth(data, "quiet")
|
||||
damaged = _booth(data, "damaged")
|
||||
(damaged / MANIFEST_FILE).write_text("{oops")
|
||||
|
||||
assert 'class="prov prov-none"' in c.get("/b/quiet/").text
|
||||
assert 'class="prov prov-broken"' in c.get("/b/damaged/").text
|
||||
|
||||
|
||||
def test_the_title_reaches_a_surface(client):
|
||||
"""`--title` promised a display name and nothing rendered it — 4/4 on the
|
||||
paraphrase panel, independently the top-ranked flag of that round. It lands
|
||||
on the booth page heading, where there is room for it; the INDEX card keeps
|
||||
the directory name, because that is the identity the operator navigates and
|
||||
refers to positionally."""
|
||||
c, data = client
|
||||
b = _booth(data, "r18-ab")
|
||||
write_manifest(b, "booth-dev", title="R18 A/B — denoiser bakeoff", why="w")
|
||||
|
||||
page = c.get("/b/r18-ab/").text
|
||||
assert "R18 A/B — denoiser bakeoff" in page
|
||||
assert "r18-ab" in page, "the directory name stopped being visible"
|
||||
|
||||
+55
-2
@@ -291,8 +291,17 @@ def test_stdlib_only(module):
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.Import):
|
||||
roots.update(a.name.split(".")[0] for a in node.names)
|
||||
elif isinstance(node, ast.ImportFrom) and node.level == 0 and node.module:
|
||||
roots.add(node.module.split(".")[0])
|
||||
elif isinstance(node, ast.ImportFrom):
|
||||
# `node.level > 0` is a RELATIVE import (`from . import marks`),
|
||||
# which has no `module` root to inspect and used to slip through
|
||||
# this walk entirely. It cannot reach outside the package, so it is
|
||||
# stdlib-safe by construction — but it is recorded rather than
|
||||
# ignored, because `manifest.py` additionally forbids importing a
|
||||
# sibling and its own test needs to see one.
|
||||
if node.level:
|
||||
roots.add("booth")
|
||||
elif node.module:
|
||||
roots.add(node.module.split(".")[0])
|
||||
outside = {r for r in roots if r != "booth" and r not in sys.stdlib_module_names}
|
||||
assert not outside, f"booth/{module}.py imports non-stdlib: {sorted(outside)}"
|
||||
|
||||
@@ -1194,3 +1203,47 @@ def test_an_unreadable_mark_is_visible_on_the_page(client):
|
||||
html = c.get("/b/b/").text
|
||||
assert "⚠ broken" in html, "an unreadable mark rendered as an empty note"
|
||||
assert "n1" in html
|
||||
|
||||
|
||||
def test_a_marks_file_no_one_can_parse_does_not_take_down_the_index(tmp_path):
|
||||
"""The v0.2.2 round adopted the RecursionError finding and closed only half
|
||||
of it. `_hydrate_safe` guards hydration; `json.loads` runs BEFORE that, in
|
||||
`_read_raw`, whose `except (OSError, ValueError, UnicodeDecodeError)` does
|
||||
not cover RecursionError or MemoryError.
|
||||
|
||||
So a 400 KB file of nothing but brackets, in any one booth, still returned
|
||||
500 for `/` and `/healthz` across every booth on the service. Found by the
|
||||
U5 code-review panel against the sibling module and confirmed by running it.
|
||||
The read is bounded now and both classes are caught.
|
||||
"""
|
||||
booth = tmp_path / "b"
|
||||
booth.mkdir()
|
||||
(booth / MARKS_FILE).write_text("[" * 200_000 + "]" * 200_000)
|
||||
|
||||
assert marks_for(booth) == []
|
||||
|
||||
|
||||
def test_a_marks_file_too_large_to_be_marks_is_refused_before_it_is_read(tmp_path):
|
||||
"""Bounded by `stat`, not survived. A booth holds one marks document, and
|
||||
the index reads every booth's on every page load."""
|
||||
from booth.marks import MARKS_MAX_BYTES
|
||||
|
||||
booth = tmp_path / "b"
|
||||
booth.mkdir()
|
||||
(booth / MARKS_FILE).write_text(" " * (MARKS_MAX_BYTES + 10))
|
||||
|
||||
assert marks_for(booth) == []
|
||||
|
||||
|
||||
def test_a_write_over_an_unparseable_marks_file_still_refuses(tmp_path):
|
||||
"""The strict half of the asymmetry has to see the same failures the lenient
|
||||
half does, or a file that reads as "no marks" gets replaced by a write that
|
||||
believed it. Same two exception classes, same bound."""
|
||||
from booth.marks import MarksCorrupt, set_flag
|
||||
|
||||
booth = tmp_path / "b"
|
||||
booth.mkdir()
|
||||
(booth / MARKS_FILE).write_text("[" * 200_000 + "]" * 200_000)
|
||||
|
||||
with pytest.raises(MarksCorrupt):
|
||||
set_flag(booth, "a.png", True)
|
||||
|
||||
Reference in New Issue
Block a user