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)