From c015a917ee52ff3c1df3bc7c6634c12179bdb8c8 Mon Sep 17 00:00:00 2001 From: Vuong Hoang Date: Tue, 22 Sep 2026 01:29:27 -0700 Subject: [PATCH] =?UTF-8?q?fix(manifest):=20fold=20in=20both=20cross-front?= =?UTF-8?q?ier=20panels=20=E2=80=94=20and=20a=20live=20hole=20in=20v0.2.2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- booth/manifest.py | 93 ++++++-- booth/marks.py | 38 ++- booth/templates/base.html | 4 + booth/templates/booth.html | 10 +- docs/contracts/u5_booth_manifest.contract.md | 190 +++++++++++---- scripts/booth | 40 +++- tests/test_cli.py | 34 +++ tests/test_manifest.py | 239 ++++++++++++++++++- tests/test_marks.py | 57 ++++- 9 files changed, 612 insertions(+), 93 deletions(-) diff --git a/booth/manifest.py b/booth/manifest.py index 3b83ef3..dd3349a 100644 --- a/booth/manifest.py +++ b/booth/manifest.py @@ -26,6 +26,7 @@ from __future__ import annotations import json import os +import secrets from dataclasses import dataclass from datetime import datetime from pathlib import Path @@ -38,6 +39,14 @@ MANIFEST_FILE = ".booth.json" HANDLE_MAX = 64 TITLE_MAX = 120 WHY_MAX = 200 +CREATED_MAX = 64 + +# A manifest is four short fields. Anything near this is not one, and reading it +# into memory to find that out is the wrong order of operations: `list_booths` +# calls the reader once per booth on every index load, so an unbounded read is +# the service-wide outage the lenient reader exists to prevent, arriving in a +# different costume. Checked by `stat`, before the bytes are touched. +MANIFEST_MAX_BYTES = 64 * 1024 # The handle a booth created by the service itself carries. A pickup booth and # the standing link board are made by the Booth, not by an agent, and saying so @@ -62,11 +71,27 @@ class Manifest: def _one_line(value, limit: int) -> str: + """One line, bounded. Collapses ALL runs of whitespace, not only newlines — + a tab or a forty-space indent in a `why` renders as badly inside a card's + sub-line as a newline does, and the field is one line by construction.""" if not isinstance(value, str): return "" return " ".join(value.split())[:limit] +def _temp_path(booth: Path) -> Path: + """A scratch name no other writer will pick. + + Every writer used to derive the same `.booth.json.tmp`, so two `booth add` + calls on one booth could interleave through a stale descriptor into the + published path. Marks are protected from that by their flock; the manifest + deliberately has none — it is written once at creation, not read-modify- + written per click — so uniqueness is what stands in for the lock. Still a + dotfile, so no listing, gallery or zip can see it mid-write. + """ + return booth / f"{MANIFEST_FILE}.{secrets.token_hex(4)}.tmp" + + def _now() -> str: return datetime.now().astimezone().isoformat(timespec="seconds") @@ -92,18 +117,33 @@ def read_manifest(booth: Path) -> Manifest | None: """ booth = Path(booth) path = booth / MANIFEST_FILE + # BOUNDED BEFORE THE READ. "Never raises" was not true of an unbounded one: + # a 4 GB file raises MemoryError and a deeply nested document raises + # RecursionError out of `json.loads`, and neither is an OSError or a + # ValueError. Both escape into `list_booths`, which calls this per booth on + # every index load — so one file returns 500 for the whole front page. Size + # first, by `stat`; then catch the two classes anyway, because a bound that + # is one day raised should not quietly re-open the hole. + try: + size = path.stat().st_size + except FileNotFoundError: + return None + except OSError as exc: + return _broken(booth, f"cannot be read: {exc}") + if size > MANIFEST_MAX_BYTES: + return _broken(booth, f"is too large to be a manifest ({size} bytes)") try: text = path.read_text(encoding="utf-8") except FileNotFoundError: return None - except (OSError, UnicodeDecodeError) as exc: + except (OSError, UnicodeDecodeError, MemoryError) as exc: return _broken(booth, f"cannot be read: {exc}") if not text.strip(): return _broken(booth, "is empty") try: raw = json.loads(text) - except ValueError as exc: - return _broken(booth, f"is not valid JSON: {exc}") + except (ValueError, RecursionError, MemoryError) as exc: + return _broken(booth, f"is not valid JSON: {type(exc).__name__}") if not isinstance(raw, dict): return _broken(booth, "is not a JSON object") @@ -112,9 +152,12 @@ def read_manifest(booth: Path) -> Manifest | None: return _broken(booth, "names no handle") return Manifest( handle=handle, - title=_one_line(raw.get("title"), TITLE_MAX) or booth.name, + # `or booth.name` goes THROUGH the normalizer too. A directory name may + # legally carry a newline on POSIX and may run to 255 bytes, and the + # fallback used to hand either straight into a card's sub-line. + title=_one_line(raw.get("title"), TITLE_MAX) or _one_line(booth.name, TITLE_MAX), why=_one_line(raw.get("why"), WHY_MAX), - created=_one_line(raw.get("created"), 64), + created=_one_line(raw.get("created"), CREATED_MAX), ) @@ -123,35 +166,49 @@ def _broken(booth: Path, reason: str) -> Manifest: error=f"{MANIFEST_FILE} {reason}") -def write_manifest(booth: Path, handle: str, *, title: str = "", - why: str = "") -> Manifest: +def write_manifest(booth: Path, handle: str, *, title: str | None = None, + why: str | None = None) -> Manifest: """Announce a booth, atomically (CLAUDE.md invariant 5). Temp file + `os.replace`, because the CLI writes this in one process while the browser reads it in another — a reader must never see a half-written - document. The temp file is itself a dotfile (`.booth.json.tmp`), so no - listing, gallery or zip can see it mid-write either. + document. The temp file is itself a dotfile, so no listing, gallery or zip + can see it mid-write either. + + OMITTED MEANS UNCHANGED; `""` MEANS CLEAR. `title` and `why` default to + None, not to the empty string, because the ordinary sequence is + `booth new x --why "..."` and then `booth add x out/*.png` — and while + omission meant empty, that second command silently erased the sentence the + first one existed to record. Two arms of the contract panel predicted it + from the wording alone; every test written for this module passed `--why` + on both calls and so could not see it. RE-ANNOUNCING PRESERVES `created` (INV-3). It is when the booth APPEARED, - and saying something more about it later is not a second appearance — - `booth add` on an existing booth is the common case, where the poster drops - the second batch and sharpens the why. A `created` that cannot be read back - is replaced rather than guessed at: a stamp that is silently wrong is worse - than one that is silently new. + and saying something more about it later is not a second appearance. A + `created` that cannot be read back is replaced rather than guessed at: a + stamp that is silently wrong is worse than one that is silently new. + + An empty `handle` becomes `SERVICE_HANDLE` rather than being refused — a + manifest with no handle does not read back at all, and an unreadable file is + the worse outcome. Unreachable from the CLI, whose fallback chain always + yields something; callers of this function directly should pass a real one. """ booth = Path(booth) booth.mkdir(parents=True, exist_ok=True) prior = read_manifest(booth) - created = prior.created if prior and not prior.error and prior.created else _now() + usable = prior if prior and not prior.error else None + created = usable.created if usable and usable.created else _now() record = Manifest( handle=_one_line(handle, HANDLE_MAX) or SERVICE_HANDLE, - title=_one_line(title, TITLE_MAX) or booth.name, - why=_one_line(why, WHY_MAX), + title=(_one_line(title, TITLE_MAX) if title is not None + else (usable.title if usable else "")) or _one_line(booth.name, TITLE_MAX), + why=(_one_line(why, WHY_MAX) if why is not None + else (usable.why if usable else "")), created=created, ) path = booth / MANIFEST_FILE - tmp = path.with_suffix(path.suffix + ".tmp") + tmp = _temp_path(booth) tmp.write_text( json.dumps( {"handle": record.handle, "title": record.title, diff --git a/booth/marks.py b/booth/marks.py index 17c8e34..4fd926d 100644 --- a/booth/marks.py +++ b/booth/marks.py @@ -73,6 +73,14 @@ class MarksCorrupt(RuntimeError): """ +# A booth's whole judgment lives in one document, so this is generous — a +# 270-item booth flagged throughout, with notes, is far under it. What it rules +# out is the case that is not marks at all: an unbounded read raises MemoryError +# and a deeply nested one raises RecursionError out of `json.loads`, neither of +# which is an OSError or a ValueError, and `list_booths` calls the reader once +# per booth on every index load. Bounded by `stat`, before the bytes are read. +MARKS_MAX_BYTES = 4 * 1024 * 1024 + MARKS_FILE = ".marks.json" MARKS_LOCK = ".marks.lock" SCHEMA_VERSION = 1 @@ -173,9 +181,12 @@ def _read_raw(booth: Path) -> list[dict]: for the same reason: a review surface that will not load is worse than one that has lost an annotation. """ + path = Path(booth) / MARKS_FILE try: - raw = json.loads((Path(booth) / MARKS_FILE).read_text(encoding="utf-8")) - except (OSError, ValueError, UnicodeDecodeError): + if path.stat().st_size > MARKS_MAX_BYTES: + return [] + raw = json.loads(path.read_text(encoding="utf-8")) + except (OSError, ValueError, UnicodeDecodeError, RecursionError, MemoryError): return [] if not isinstance(raw, dict): return [] @@ -198,18 +209,29 @@ def _read_raw_strict(booth: Path) -> list[dict]: case where writing would destroy something. """ path = Path(booth) / MARKS_FILE + try: + size = path.stat().st_size + except FileNotFoundError: + return [] + except OSError as exc: + raise MarksCorrupt(f"{path} cannot be read: {exc}") from exc + # The strict half has to refuse everything the lenient half tolerates, or a + # file that reads as "no marks" gets replaced by a write that believed it. + if size > MARKS_MAX_BYTES: + raise MarksCorrupt(f"{path} is too large to be a marks document ({size} bytes)") try: text = path.read_text(encoding="utf-8") except FileNotFoundError: return [] - except (OSError, UnicodeDecodeError) as exc: + except (OSError, UnicodeDecodeError, MemoryError) as exc: raise MarksCorrupt(f"{path} cannot be read: {exc}") from exc if not text.strip(): return [] try: raw = json.loads(text) - except ValueError as exc: - raise MarksCorrupt(f"{path} is not valid JSON: {exc}") from exc + except (ValueError, RecursionError, MemoryError) as exc: + raise MarksCorrupt( + f"{path} is not valid JSON: {type(exc).__name__}") from exc if not isinstance(raw, dict) or not isinstance(raw.get("marks"), list): raise MarksCorrupt(f"{path} is not a marks document") entries = [e for e in raw["marks"] if isinstance(e, dict) and isinstance(e.get("id"), str)] @@ -671,7 +693,8 @@ def import_legacy_asks(booth: Path) -> list[Mark]: continue try: decl = json.loads(p.read_text(encoding="utf-8")) - except (OSError, ValueError, UnicodeDecodeError) as exc: + except (OSError, ValueError, UnicodeDecodeError, + RecursionError, MemoryError) as exc: found.append((mtime, stem, None, f"unreadable ask: {exc}")) continue if not isinstance(decl, dict): @@ -693,7 +716,8 @@ def import_legacy_asks(booth: Path) -> list[Mark]: loaded = json.loads(ap.read_text(encoding="utf-8")) if isinstance(loaded, dict): answer = loaded - except (OSError, ValueError, UnicodeDecodeError): + except (OSError, ValueError, UnicodeDecodeError, + RecursionError, MemoryError): pass prior = by_id.get(stem) diff --git a/booth/templates/base.html b/booth/templates/base.html index d0f9b3c..8f81f1b 100644 --- a/booth/templates/base.html +++ b/booth/templates/base.html @@ -427,6 +427,10 @@ /* Its own row under the title, not another chip in the flex line — a `why` can run to WHY_MAX and would otherwise shove the zip link around. */ .boothhead .prov{flex:0 0 100%;margin-top:-.35rem} + /* The directory name beside a manifest title: quieter than the title, but + never absent — it is what the URL says and what "the third one" refers to. */ + .h1-slug{font-family:var(--font-mono);font-size:.62em;font-weight:400; + letter-spacing:.06em;color:var(--fg-3);margin-left:.5rem;white-space:nowrap} .wipe-lg{position:static} /* red-outline danger button — legible on the dark canvas, fills on hover */ .wipe-lg button{width:auto;height:auto;padding:.42rem .85rem;border-radius:var(--radius-md); diff --git a/booth/templates/booth.html b/booth/templates/booth.html index e97f3d0..b742e9d 100644 --- a/booth/templates/booth.html +++ b/booth/templates/booth.html @@ -56,7 +56,15 @@ {% block content %}
‹ all booths -

{{ name }}

+ {# The manifest's TITLE is the display name; the directory name stays visible + beside it because that is the identity the operator navigates by and refers + to positionally, and losing it would be losing the thing the URL says. + Index cards keep the directory name alone for the same reason. #} + {% if manifest and not manifest.error and manifest.title and manifest.title != name %} +

{{ manifest.title }} {{ name }}

+ {% else %} +

{{ name }}

+ {% endif %} {% if uploaded %}⬆ pickup {% endif %}{% if board %}{{ board|length }} link{{ '' if board|length == 1 else 's' }}{% if items %} · {{ items|length }} file{{ '' if items|length == 1 else 's' }}{% endif %}{% else %}{% if marks_open %}{{ marks_open }} open · {% endif %}{{ items|length }} item{{ '' if items|length == 1 else 's' }} · expires in {{ expires_in|dur }}{% endif %} {% if items %}⬇ zip{% endif %} {{ provenance(manifest) }} diff --git a/docs/contracts/u5_booth_manifest.contract.md b/docs/contracts/u5_booth_manifest.contract.md index 54e18cd..b31c0ba 100644 --- a/docs/contracts/u5_booth_manifest.contract.md +++ b/docs/contracts/u5_booth_manifest.contract.md @@ -4,7 +4,7 @@ module: "booth.manifest" purpose: "A booth that says what it IS and who posted it. Today the index card shows a name, an item count and a countdown -- nothing about provenance or purpose -- so an agent that wants the operator to look at something has no way to make the booth say so, and posts a URL to the link board instead. That is job 5 (`Announce`), the job nobody named, and its absence is the measured cause of 145 dead link rows (69% of the board pointing at booths that no longer exist). This unit gives job 5 a home: each booth carries `.booth.json` -- `{handle, title, why, created}`, written by the CLI from `$ALTHING_HANDLE` -- and the index card and the booth page header render it. Enforcing the link rule WITHOUT giving job 5 a home first just makes it homeless; this is the home." depends_on: - "booth.items (the dotfile skip in `booth_items` -- `.booth.json` is excluded from tiles, counts and zips by the EXISTING `p.name.startswith('.')` rule at items.py:182, exactly as `.marks.json` is. No new exclusion rule is added or needed. Verified, not assumed: `test_a_manifest_is_not_an_item` asserts it.)" - - "booth.marks (the `_write_raw` shape only -- temp file + os.replace, INV-5. Copied as a pattern, NOT imported: manifest.py must not depend on marks.py, because the CLI imports each module on its own.)" + - "booth.marks (the `_write_raw` shape only -- temp file + os.replace, per CLAUDE.md invariant 5. Copied as a pattern, NOT imported: manifest.py must not depend on marks.py, because the CLI imports each module on its own.)" language: "python" complexity: "low" estimated_loc: 150 @@ -16,9 +16,10 @@ used_by: - "scripts/booth (`new` and `add` gain `--why` / `--title`; `link` announces the standing board)" touches: - "booth/manifest.py (new -- the record, the write, the lenient read)" - - "booth/app.py (list_booths gains one key; booth_view gains one key; the /upload path writes a manifest and adds it to the `used` dedupe set at app.py:1091)" + - "booth/app.py (list_booths gains one key; booth_view gains one key; the /upload path writes a manifest. It also adds MANIFEST_FILE to the `used` dedupe set -- CONSISTENCY, not a fix: SR-1 established the collision is unreachable because `safe_upload_name` strips leading dots, which is equally true of the `UPLOAD_MARKER` entry that has sat in that set since before this unit.)" + - "booth/templates/_provenance.html (new -- the provenance macro, defined ONCE and called from both index lanes and the booth header. Not in the first draft of this inventory: the implementation added the partial rather than repeating the four-state conditional three times, which is SR-6 plus the blurtoggle lesson, and the inventory lagged the decision.)" - "booth/templates/index.html (the provenance line on both lanes' cards -- kept AND ephemeral, or the kept lane silently keeps the old defect)" - - "booth/templates/booth.html (the same line in the boothhead sub)" + - "booth/templates/booth.html (the provenance line in the boothhead, and the h1 renders `title` with the directory name beside it)" - "booth/templates/base.html (the .prov-* CSS)" - "scripts/booth (`new` / `add` flag parse; `link` board announcement; usage string; the header doc block)" - "tests/test_manifest.py (new)" @@ -27,14 +28,13 @@ assumptions: - "THE MANIFEST IS A DOTFILE, and that is the whole integration story. `booth_items` skips `name.startswith('.')` (items.py:182), `zip_booth` skips it (app.py:351), and the legacy ask scan skips it (marks.py:656). So `.booth.json` costs nothing in item counts, galleries, zips or migration, and needs no new exclusion anywhere. This is the same reason `.marks.json` needed none. Settled -- do not re-derive it." - "WRITING A MANIFEST IS ACTIVITY. `.booth.json` is a dotfile but NOT a `.lock` dotfile, so `_newest_mtime` counts it (app.py:192 excludes only `..lock`). Creating or re-announcing a booth resets its TTL, which is correct: both are somebody touching it. The lock exemption exists for machinery that a READ path creates; this is a deliberate write." - "THE READ IS LENIENT AND THE FAILURE IS VISIBLE. `list_booths` reads every booth on every index load, so a manifest that cannot be parsed must never raise -- that is the v0.2.2 lesson, learned when a poisoned `.marks.json` returned 500 for `/` and `/healthz` across all 25 booths. `read_manifest` returns None for absent and a `Manifest` carrying `error` for damaged, and the card distinguishes them (`unannounced` vs `unreadable`). Silently treating damaged as absent would hide the one case somebody has to fix." - - "THE WRITE IS ATOMIC (INV-5). Temp file + os.replace, because the CLI writes it in one process while the browser reads it in another. The pattern is copied from `marks._write_raw` rather than imported: `scripts/booth` imports each module directly under the system python3, and a cross-import between two stdlib-only modules is a second way for INV-1 to break." - - "`booth/manifest.py` IS STDLIB-ONLY and joins the INV-1 list. `scripts/booth` imports it through a `python3 -c` heredoc with no venv, exactly as it imports `marks`, `asks` and `links`. `test_stdlib_only` is parametrized and gains `manifest`; that test is the only thing standing between a casual third-party import and `booth new` breaking on every fleet host." + - "THE WRITE IS ATOMIC (CLAUDE.md invariant 5, NOT this unit's INV-5). Temp file + os.replace onto a name no other writer derives, because the CLI writes it in one process while the browser reads it in another -- and because two `booth add` calls on one booth would otherwise share a scratch name, which the atomic-write promise says nothing about: it promises readers never see a partial file, not that writers never race. The pattern is copied from `marks._write_raw` rather than imported: `scripts/booth` imports each module directly under the system python3, and a cross-import between two stdlib-only modules is a second way for INV-1 to break." + - "`booth/manifest.py` IS STDLIB-ONLY and joins the CLAUDE.md invariant 1 list. `scripts/booth` imports it through a `python3 -c` heredoc with no venv, exactly as it imports `marks`, `asks` and `links`. `test_stdlib_only` is parametrized and gains `manifest`; that test is the only thing standing between a casual third-party import and `booth new` breaking on every fleet host." - "A MISSING MANIFEST IS NORMAL, NOT AN ERROR. All 26 live booths have none, and `rsync -a ./out/ nh3-dev:booth-data/my-run/` -- the documented path for every host that is not nh3-dev -- never runs the CLI at all, so unannounced booths keep arriving after this lands. The card marks them quietly and nothing refuses to render, expire, zip or sweep." - "THE BOOTH ANNOUNCES ITS OWN BOOTHS rather than exempting them. A pickup booth and the standing link board are created BY the service, so they are written with `handle: booth` -- which is true, not manufactured. The alternative was a pile of exemptions from the unannounced marker; this way there is one rule (a booth with no manifest is unannounced) and no special cases. `handle` therefore names an agent handle OR the service, and the field's docstring says so." - - "NOTHING NEW IS ORDERED, so invariant 6 is discharged by having no list. The manifest is one flat record per booth. The index keeps its stated rule -- kept lane first, then ephemeral newest-first by `_newest_mtime` -- and U5 does NOT add a second ordering keyed on `created` (operator, 2026-09-22). A what-landed feed ordered by announcement time is a genuinely different surface: it needs its own stated rule, it competes with the existing order for what 'the third one' means, and it has nothing to sort the 26 manifest-less booths by. Parked for v1.1." + - "NOTHING NEW IS ORDERED, so CLAUDE.md invariant 6 (every ordered collection has a stated, deterministic rule) does not bind here -- there is no new collection for it to bind to. The manifest is one flat record per booth. The index keeps its stated rule -- kept lane first, then ephemeral newest-first by `_newest_mtime` -- and U5 does NOT add a second ordering keyed on `created` (operator, 2026-09-22). A what-landed feed ordered by announcement time is a genuinely different surface: it needs its own stated rule, it competes with the existing order for what 'the third one' means, and it has nothing to sort the 26 manifest-less booths by. Parked for v1.1." open_questions: - "Whether `why` should also reach the zip manifest or a `booth ls` column. Both are one-liners over the same record and neither is on the v1 path; deferred rather than designed." - - "Whether a booth should be able to RE-announce (a second `booth new` on an existing name currently rewrites the manifest and resets `created`). Specified below as: re-announcing updates `title`/`why` and PRESERVES the original `created`, because `created` is when the booth appeared and a second announcement is not a second appearance." --- # U5 — self-announcing booths @@ -60,15 +60,25 @@ just makes it homeless.** ```python @dataclass(frozen=True) class Manifest: - handle: str # $ALTHING_HANDLE, or "booth" for a booth the service made - title: str # display name; falls back to the directory name - why: str # ONE line: what the operator is looking at and why - created: str # ISO-8601 with offset, from the first announcement - error: str | None # set when the stored bytes could not be read + handle: str # an althing handle, or "booth" for one the service made + title: str # display name; falls back to the directory name + why: str # ONE line: what the operator is looking at and why + created: str # ISO-8601 with offset, from the FIRST announcement + error: str | None = None # a read-time verdict; never stored ``` -`.booth.json` on disk is the same four fields, no `error` — that one is a -read-time verdict, not stored state. +`.booth.json` on disk is the same four fields, no `error`. + +**Every field on an error-carrying record has a stated value**, because the +templates render the record and a careless fill would re-raise the outage in +the renderer: `handle` and `why` and `created` are `""`, `title` is the +normalized directory name, and `error` says which of the six refusals fired. +`created` being `""` is what makes `write_manifest` treat a damaged prior as +having no stamp to preserve (INV-3). + +Caps, all applied at the write and again at the read: `handle` 64, `title` 120, +`why` 200, `created` 64. Each is a **display budget**, not a storage limit — +they exist because these strings land in a card's sub-line. ## Signatures @@ -77,6 +87,9 @@ MANIFEST_FILE = ".booth.json" HANDLE_MAX, TITLE_MAX, WHY_MAX = 64, 120, 200 +MANIFEST_MAX_BYTES = 64 * 1024 + + def read_manifest(booth: Path) -> Manifest | None: """This booth's announcement, or None if it never made one. @@ -86,37 +99,77 @@ def read_manifest(booth: Path) -> Manifest | None: made expensive: a read that can raise, called in a loop over every booth, is a service-wide outage wearing a single-booth bug's clothes. - Absent -> None. Present but unparseable, or not an object, or missing - `handle` -> a Manifest carrying `error`, so the card can say `unreadable` - rather than quietly showing the same thing as a booth that never announced. + "NEVER RAISES" IS BOUNDED, NOT MERELY CAUGHT. An earlier draft of this + contract named a 4 GB file as a tested case and constrained only the RETURN + — which is letter-compliant and purpose-defeating: reading four gigabytes + per booth per index load recreates the same outage in slow motion. The size + is checked by `stat` BEFORE the bytes are touched, and the two exception + classes that are neither `OSError` nor `ValueError` — `MemoryError` from a + huge document, `RecursionError` from a deeply nested one — are caught as + well, so that raising the bound one day cannot quietly re-open the hole. + + Absent -> None. Present but too large, unreadable, unparseable, not an + object, or missing `handle` -> a Manifest carrying `error`, so the card can + say `unreadable` rather than quietly showing the same thing as a booth that + never announced. """ -def write_manifest(booth: Path, handle: str, *, title: str = "", - why: str = "") -> Manifest: - """Announce a booth. Atomic (INV-5): temp file + os.replace. +def write_manifest(booth: Path, handle: str, *, title: str | None = None, + why: str | None = None) -> Manifest: + """Announce a booth. Atomic per CLAUDE.md invariant 5: temp file + + os.replace, onto a temp name no other writer will pick. - Re-announcing an existing booth updates `title` and `why` and PRESERVES the - original `created` — `created` is when the booth appeared, and saying - something more about it later is not a second appearance. A `created` that - cannot be read back is replaced by now(). + OMITTED MEANS UNCHANGED; `""` MEANS CLEAR. `title` and `why` default to + None. The ordinary sequence is `booth new x --why "..."` then + `booth add x out/*.png`, and while omission meant `""` the second command + silently erased the sentence the first one existed to record. The shell + carries the distinction by leaving the environment variable UNSET rather + than empty. - Empty `title` stores the booth's directory name. Every field is stripped of - newlines and truncated: a `why` is one line by construction, not by - convention, because it renders inside a card's sub-line. + Re-announcing PRESERVES the original `created` — `created` is when the + booth appeared, and saying something more about it later is not a second + appearance. A prior record carrying `error`, or one whose `created` is + `""`, is treated as having no stamp to preserve and gets `now()`: a stamp + that is silently wrong is worse than one that is silently new. + + `title` falls back to the directory name, THROUGH the same normalizer the + explicit value gets — a directory name may legally carry a newline on POSIX + and may run to 255 bytes, and the fallback used to hand either straight + into a card's sub-line. + + Every stored string is collapsed to a single line — all runs of whitespace, + not only newlines, because a tab or a forty-space indent renders as badly + in a sub-line as a newline does — and truncated to its cap. + + An empty `handle` becomes `"booth"` rather than being refused: a manifest + naming no handle does not read back at all, and an unreadable file is the + worse outcome. Unreachable from the CLI, whose fallback chain always yields + something; a direct caller should pass a real one. """ ``` ## What renders -One line, on both surfaces, driven by the same record: +One line, on both surfaces, driven by the same record. The example booth below +is the directory `r18-ab`, announced by the handle `booth-dev`: -| state | index card / booth header | +| state | the provenance line, on an index card AND on the booth header | |---|---| | announced, with a why | `booth-dev · pick the winning denoiser` | | announced, no why | `booth-dev` | | no manifest | `unannounced` (muted) | -| damaged manifest | `unreadable` (muted) | +| damaged manifest | `unreadable` (muted, warning tint, `title=` carries the reason) | + +**`title` renders too, and on exactly one surface.** An earlier draft stored it, +surfaced a `--title` flag for it, and rendered it nowhere — a promise of a +display name with no display, caught 4-of-4 and ranked first independently by +every arm. It lands on the **booth page heading**, where there is room: +`

R18 A/B r18-ab

`. The **index card keeps +the directory name alone**, because that is the identity the operator navigates +by and refers to positionally, and CLAUDE.md invariant 6 is about exactly that +kind of reference surviving a re-render. When `title` equals the directory name +— the default — the heading is unchanged from today. **Both index lanes get it.** The kept lane renders first and is a separate block in `index.html`; patching only the ephemeral lane would leave the 15 kept booths @@ -143,10 +196,25 @@ booth new scratch # still legal — handle + created, no why provenance means the same thing on the board and on the card. **Nothing existing breaks.** A bare `booth new x` / `booth add x f.png` keeps -working and gains a manifest with no `why`; the flags are optional and -order-independent after the positional arguments. The alternative — a separate -`booth announce` verb — was rejected because a second step is the step that gets -forgotten, which is the 69% rot's own mechanism. +working; the flags are optional and may sit on either side of the file +arguments, because a glob is usually last and a flag usually after it and +nothing enforces that. The alternative — a separate `booth announce` verb — was +rejected because a second step is the step that gets forgotten, which is the +69% rot's own mechanism. + +**A bare re-announce does not wipe what the last one said.** On a booth that has +never announced, a bare `new`/`add` writes `{handle, created}` with no `why`. On +one that HAS, an omitted flag leaves the stored value alone and only a supplied +one overwrites — `--why ""` still clears, which is a different intention. This +distinction is load-bearing rather than polite: `booth new x --why "…"` followed +by `booth add x out/*.png` is the ordinary sequence, and the naive reading +erases the sentence on the second command. + +**The handle is the CLI's three-step chain**, not `$ALTHING_HANDLE` alone: +`${ALTHING_HANDLE:-${BOOTH_SOURCE:-$(hostname -s)}}`, identical to the one +`booth link` already uses for its rows, so provenance means the same thing on +the board and on the card. A session with no handle set still announces, as its +host. ## Scope — the blast-radius pass @@ -174,8 +242,14 @@ rather than assumed: `items.booth_items` (items.py:182), `app.zip_booth` (app.py:351), `marks.import_legacy_asks` (marks.py:656). **One site the first draft of this contract got WRONG, corrected by the seam -review** (SR-1, below): `app.py:1091`'s `used: set = {UPLOAD_MARKER}` — the -upload path's filename dedupe set — does **not** need to gain `MANIFEST_FILE`. +review** (SR-1, below): the upload path's `used: set = {UPLOAD_MARKER}` filename +dedupe set does **not** need to gain `MANIFEST_FILE`. The implementation adds it +anyway, as consistency with the equally-unreachable entry already there, and +says so in a comment rather than claiming it prevents anything. + +⚠ **Line numbers in this section are the PRE-CHANGE coordinates** the +blast-radius pass was run against, kept because that is what makes the pass +auditable. They have moved; `grep` the symbol, do not trust the number. ## Seam review — what the real sibling surfaces said @@ -253,19 +327,41 @@ Deliberately deferred or never. Divergence here is not drift. ## Invariants -**INV-1 — one resolver for the manifest.** `read_manifest(booth)` is the only -place `.booth.json` is opened. No route body, template or CLI verb parses it. -Falsifiable: no `MANIFEST_FILE` read outside `manifest.py`. +Numbered INV-1..5 and local to this unit. Where a repo-wide rule is meant it is +named in words — "CLAUDE.md invariant 5", "CLAUDE.md invariant 6" — never by a +bare number, because an earlier draft used `INV-5` for both the repo's +atomic-write rule and this unit's render rule and the collision was caught +3-of-4. -**INV-2 — the read cannot raise.** `read_manifest` returns for every input, -including a directory that is not a booth, a `.booth.json` that is a list, one -that is 4 GB, and one that is not UTF-8. Tested per case. +**INV-1 — one module knows the filename.** `booth/manifest.py` is the only +module that names `MANIFEST_FILE`. No route body, template or CLI verb opens or +parses `.booth.json`; `write_manifest` reads it back inside that module, which +is what INV-3 requires and is not an exception to this rule. Falsifiable and +tested: no other file under `booth/` contains the literal `.booth.json`. + +**INV-2 — the read cannot raise, AND cannot cost the caller unboundedly.** +`read_manifest` returns for every input: an absent directory, a `.booth.json` +that is a list, a string, `null`, empty, not UTF-8, wrong-typed, missing its +handle, nested deeply enough to overflow the parser's stack, and one larger +than `MANIFEST_MAX_BYTES` — which is refused by `stat` before a byte is read, +because a bound that only constrains the RETURN recreates the outage in slow +motion. Tested per case, the size and depth cases included. **INV-3 — `created` survives re-announcement.** A second `write_manifest` on the -same booth preserves the first `created`. +same booth preserves the first `created`. A prior record carrying `error`, or +one whose `created` is `""`, has no stamp to preserve and gets `now()`. Tested +against a stamp that could not have come from `now()` — `_now()` is whole-second +resolution, so back-to-back writes share a timestamp and a naive test passes +against an implementation that regenerates it every time. -**INV-4 — stdlib-only (INV-1 of `CLAUDE.md`).** `booth/manifest.py` imports -nothing outside the standard library and nothing from `booth.*`. +**INV-4 — stdlib-only, and sibling-free** (this is CLAUDE.md invariant 1 +extended by one clause). `booth/manifest.py` imports nothing outside the +standard library and nothing from `booth.*` — a cross-import between two +stdlib-only modules is a second way for the repo rule to break. Relative +imports count; the AST walk sees them. -**INV-5 — the unannounced state is visible and distinct from the unreadable -one.** Both render; they do not render the same thing. +**INV-5 — unannounced and unreadable render DIFFERENT TEXT.** Not merely +different styling: the words differ (`unannounced` / `unreadable`), so the +distinction survives a stylesheet change and a reader who cannot see colour. A +one-pixel difference would satisfy a looser wording and encode nothing, and the +point is that one of the two states is something somebody has to go and fix. diff --git a/scripts/booth b/scripts/booth index 164eaa4..9c0db38 100755 --- a/scripts/booth +++ b/scripts/booth @@ -103,15 +103,20 @@ LINKS_BOARD="${BOOTH_LINKS_BOARD:-links}" # appear, so `booth add b *.png --why "..."` and `booth add b --why "..." *.png` # both work — a glob is usually last and a flag usually after it, but nothing # enforces that and a session should not have to care. -WHY=""; TITLE=""; ARGS=() +# OMITTED IS NOT EMPTY. `booth new x --why "..."` then `booth add x out/*.png` +# is the ordinary sequence, and while an omitted flag meant "" the second +# command silently erased the sentence the first one existed to record. So the +# shell tracks WHETHER the flag was given, and only passes it on when it was — +# an explicit `--why ""` still clears, which is a different intention. +WHY=""; TITLE=""; WHY_SET=0; TITLE_SET=0; ARGS=() strip_announce_flags() { - ARGS=() + ARGS=(); WHY_SET=0; TITLE_SET=0 while [ $# -gt 0 ]; do case "$1" in - --why) [ $# -ge 2 ] || usage; WHY="$2"; shift 2 ;; - --title) [ $# -ge 2 ] || usage; TITLE="$2"; shift 2 ;; - --why=*) WHY="${1#--why=}"; shift ;; - --title=*) TITLE="${1#--title=}"; shift ;; + --why) [ $# -ge 2 ] || usage; WHY="$2"; WHY_SET=1; shift 2 ;; + --title) [ $# -ge 2 ] || usage; TITLE="$2"; TITLE_SET=1; shift 2 ;; + --why=*) WHY="${1#--why=}"; WHY_SET=1; shift ;; + --title=*) TITLE="${1#--title=}"; TITLE_SET=1; shift ;; *) ARGS+=("$1"); shift ;; esac done @@ -120,18 +125,27 @@ strip_announce_flags() { # Announce a booth. Goes through booth/manifest.py rather than printf-ing JSON # from the shell, because a why containing a quote, a backslash or a newline is # not an edge case — it is a sentence somebody wrote. +# announce [title] [why] — the trailing two are passed as +# environment variables that are UNSET when the flag was not given, because +# that is the only way the shell can say "leave it alone" rather than "". announce() { - BOOTH_SRC="$(cd "$(dirname -- "$(readlink -f -- "$0")")/.." && pwd)" \ - BOOTH_ANN_DIR="$1" BOOTH_ANN_HANDLE="$2" \ - BOOTH_ANN_TITLE="${3:-}" BOOTH_ANN_WHY="${4:-}" python3 -c ' + local -a envs + envs=( "BOOTH_SRC=$(cd "$(dirname -- "$(readlink -f -- "$0")")/.." && pwd)" + "BOOTH_ANN_DIR=$1" "BOOTH_ANN_HANDLE=$2" ) + [ "${TITLE_SET:-0}" = 1 ] && envs+=( "BOOTH_ANN_TITLE=${3:-}" ) + [ "${WHY_SET:-0}" = 1 ] && envs+=( "BOOTH_ANN_WHY=${4:-}" ) + env "${envs[@]}" python3 -c ' import os, pathlib, sys sys.path.insert(0, os.environ["BOOTH_SRC"]) try: from booth.manifest import write_manifest + kw = {} + # Absent means the flag was omitted; present-and-empty means it was given + # as "" and the poster meant to take the line back. + if "BOOTH_ANN_TITLE" in os.environ: kw["title"] = os.environ["BOOTH_ANN_TITLE"] + if "BOOTH_ANN_WHY" in os.environ: kw["why"] = os.environ["BOOTH_ANN_WHY"] write_manifest(pathlib.Path(os.environ["BOOTH_ANN_DIR"]), - os.environ["BOOTH_ANN_HANDLE"], - title=os.environ["BOOTH_ANN_TITLE"], - why=os.environ["BOOTH_ANN_WHY"]) + os.environ["BOOTH_ANN_HANDLE"], **kw) except Exception as exc: # A booth that could not announce itself is still a booth. Say so on stderr # and carry on: failing `booth add` over its metadata would lose the files @@ -247,7 +261,7 @@ case "$cmd" in # The board announces itself as the SERVICE's, not as any one agent's: # seventeen handles post to it, so no handle owns it. Idempotent — a second # link keeps the original creation stamp. - announce "$board" "booth" "$LINKS_BOARD" \ + TITLE_SET=1 WHY_SET=1 announce "$board" "booth" "$LINKS_BOARD" \ "the standing link board — every agent session posts here" # Provenance, because a bare URL is unreadable three days later: who posted # it, from where, and when. diff --git a/tests/test_cli.py b/tests/test_cli.py index 5c140d2..c9e0047 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -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 == "" diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 7a1f5d6..5dad562 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -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" diff --git a/tests/test_marks.py b/tests/test_marks.py index 6a9e7e2..96d2a36 100644 --- a/tests/test_marks.py +++ b/tests/test_marks.py @@ -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)