diff --git a/booth/app.py b/booth/app.py index b29fc6b..6ae0623 100644 --- a/booth/app.py +++ b/booth/app.py @@ -1008,8 +1008,17 @@ def create_app( 178 of 221 rows today, on the ONE booth that carries a links.md. """ try: - return (data_dir / name).is_dir() - except OSError: + candidate = (data_dir / name).resolve() + # THE SAME CONTAINMENT `resolve_booth` ENFORCES. Without it the two + # disagree on a symlink: the marker would call a booth pointing + # outside the data root ALIVE while the page 404s it, so the row + # renders healthy and the link is dead — the worst of both, and + # invisible. 3-of-4 cold bug-hunt arms found the disagreement. + return candidate.parent == data_dir and candidate.is_dir() + except (OSError, ValueError): + # ValueError, not only OSError: an embedded NUL raises it rather + # than an OSError, and this predicate runs once per board row — one + # bad row must never cost the other 220. return False def _mark_redirect(name: str, form, anchor: str) -> RedirectResponse: diff --git a/booth/benches.py b/booth/benches.py index e998858..edbf5cf 100644 --- a/booth/benches.py +++ b/booth/benches.py @@ -27,6 +27,7 @@ import fcntl import json import os import stat +import tempfile from dataclasses import dataclass, replace from datetime import datetime, timezone from pathlib import Path @@ -299,9 +300,23 @@ def _write_all(root: Path, benches: dict[str, Bench]) -> None: raise ValueError( f"that registration would push the registry past {BENCHES_MAX_BYTES} " f"bytes, which its own reader refuses; nothing was written") - tmp = path.with_suffix(path.suffix + f".tmp.{os.getpid()}") + # AN UNPREDICTABLE SCRATCH NAME, IN THE SAME DIRECTORY. `.tmp.` is + # guessable, and a pre-planted symlink there redirects the write straight + # through the atomic replace — the replace is atomic, not safe. mkstemp + # creates with O_EXCL and 0600, so it cannot land on someone else's file. + # Same directory because os.replace is only atomic within a filesystem. + fd, tmpname = tempfile.mkstemp(dir=str(root), prefix=".benches-", suffix=".tmp") + tmp = Path(tmpname) try: - tmp.write_text(body) + with os.fdopen(fd, "w", encoding="utf-8") as fh: + fh.write(body) + fh.flush() + # FSYNC BEFORE THE REPLACE. os.replace orders the rename, not the + # DATA behind it: without this, a power loss can publish a name + # pointing at bytes that never reached the disk, which is a + # truncated registry wearing a successful write's clothes. + os.fsync(fh.fileno()) + os.chmod(tmp, 0o644) # mkstemp's 0600 is tighter than the rest os.replace(tmp, path) except BaseException: # A write that dies between create and replace would otherwise strand diff --git a/booth/templates/booth.html b/booth/templates/booth.html index f4f76f0..9713a51 100644 --- a/booth/templates/booth.html +++ b/booth/templates/booth.html @@ -66,7 +66,62 @@ {% else %}

{{ name }}

{% endif %} - {% if uploaded %}⬆ pickup {% endif %}{% if is_board %} + {% 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 %} · {{ lifetime(kept, hold, expires_in) }}{% else %}{% if marks_open %}{{ marks_open }} open · {% endif %}{{ items|length }} item{{ '' if items|length == 1 else 's' }} · {{ lifetime(kept, hold, expires_in) }}{% endif %} + {% if items %}⬇ zip{% endif %} + {{ provenance(manifest) }} + {# A durable multi-writer board gets no one-click wipe — same rule as the + kept lane on the index. Remove rows with the per-row ×, or release the + board from the index and wipe it from there. #} + {# Promote or release without going back to the index. `next` keeps you on + this page instead of bouncing you to /. #} + {% if kept %} +
+ + +
+ {% else %} +
+ + +
+ {% endif %} + {% if not board %} +
+ +
+ {% endif %} + + +{% if uploaded %} +
+ 📦 Pickup {{ name }} + + — download files below, or on nh3-dev grab ~/booth-data/{{ name }}/ +
+{% endif %} + +{# The marks panel: the session's questions, the operator's notes, and the way + back to the flagged items. Always rendered on a gallery booth — the add-note + field is a control, not a result, so it has to be there before the first + mark exists. #} +{# `marks or not board`: the standing link board renders as a board rather than + a gallery, and the add-note control would be noise on it — but the + suppression was unconditional, so a pick declared on a booth that happens to + carry a links.md had no form to answer it and nothing said so. #} +{% if marks or not board %} + {% include "_marks.html" %} +{% endif %} + +{# THE BENCH REGISTRY — BLOCK LEVEL, and that placement is load-bearing. + This
spent one commit nested inside the `` of the + booth header, because the insertion matched the FIRST `{% if board %}` in + the file rather than the block-level one. A
inside a is + invalid HTML: the parser closes the span implicitly and hoists the div + out, orphaning the rest of the sub-line. Three of four cold bug-hunt arms + found it and the seat confirmed it in the live document by byte offset. + Keep this block between the marks panel and the board form. #} +{% if is_board %} {# THE BENCH REGISTRY. A bench is a running thing — jackdaw's current bench, talk's current bench, the things that get promoted to Homepage when they are fully deployed. NOT a booth (a booth announces itself and is swept) and @@ -127,53 +182,6 @@
{% endif %} -{% if board %}{{ board|length }} link{{ '' if board|length == 1 else 's' }}{% if items %} · {{ items|length }} file{{ '' if items|length == 1 else 's' }}{% endif %} · {{ lifetime(kept, hold, expires_in) }}{% else %}{% if marks_open %}{{ marks_open }} open · {% endif %}{{ items|length }} item{{ '' if items|length == 1 else 's' }} · {{ lifetime(kept, hold, expires_in) }}{% endif %} - {% if items %}⬇ zip{% endif %} - {{ provenance(manifest) }} - {# A durable multi-writer board gets no one-click wipe — same rule as the - kept lane on the index. Remove rows with the per-row ×, or release the - board from the index and wipe it from there. #} - {# Promote or release without going back to the index. `next` keeps you on - this page instead of bouncing you to /. #} - {% if kept %} -
- - -
- {% else %} -
- - -
- {% endif %} - {% if not board %} -
- -
- {% endif %} -
- -{% if uploaded %} -
- 📦 Pickup {{ name }} - - — download files below, or on nh3-dev grab ~/booth-data/{{ name }}/ -
-{% endif %} - -{# The marks panel: the session's questions, the operator's notes, and the way - back to the flagged items. Always rendered on a gallery booth — the add-note - field is a control, not a result, so it has to be there before the first - mark exists. #} -{# `marks or not board`: the standing link board renders as a board rather than - a gallery, and the add-note control would be noise on it — but the - suppression was unconditional, so a pick declared on a booth that happens to - carry a links.md had no form to answer it and nothing said so. #} -{% if marks or not board %} - {% include "_marks.html" %} -{% endif %} - {% if board %} {# THE STANDING LINK BOARD. Every agent session on the fleet appends here, so this is the one booth where the useful granularity is the ROW, not the diff --git a/docs/contracts/u6_benches.contract.md b/docs/contracts/u6_benches.contract.md index 53e273f..6a18336 100644 --- a/docs/contracts/u6_benches.contract.md +++ b/docs/contracts/u6_benches.contract.md @@ -538,3 +538,44 @@ All four closed. INV-8's file list, the `registered`/`created` wording, and every line number in this document's prose — the panel found two already stale, which is the whole argument against putting them in prose at all. + +## Bug hunt — what the cold panel found + +`/heid-bug-hunt` panel `01M35CRRK2RTVWWF1BN09AFQG3`, four arms, diff-scoped +against `91fd8bc`. The most severe of the three rounds, and **three of its four +convergent findings were already closed by our own adversarial pass before the +reply landed** — which is the complementarity the skill claims, measured in both +directions on one diff. + +| finding | arms | state when the reply landed | +|---|---|---| +| **A single malformed board row blanks the ENTIRE 221-row board.** `%00` in a booth name decodes to an embedded NUL; `Path.is_dir()` raises **ValueError**, not `OSError`; `_board_rows`' blanket handler returns `[]`. Every row vanishes, the page still 200s, nothing says why. | 4/4 | **Already fixed** (control-character guard). | +| **`RecursionError` escapes `read_benches` and 500s the board page.** ~4 KB of nested brackets, well under the byte cap. **Three arms independently cited the precedent: this repo already paid for this exact class in `marks.py`** — the new module re-introduced the unguarded parse. | 4/4 | **Already fixed.** | +| **A FIFO still blocks the render path** while the code comment claims the hang lesson was applied. | 4/4 | **Already fixed** — and the comment that lied about it was the thing that made us look. | +| **IPv6 bracket loss.** Second independent sighting, same root. | 4/4 | **Already fixed** by the code-review round. | +| **The benches panel is nested inside ``.** A `
` in a ``: the parser closes the span implicitly and hoists the div out, orphaning the rest of the sub-line. Nothing 500s, which is why no test could see it. | 3/4 | **OPEN — fixed now.** Moved to block level; pinned by an offset assertion and verified with a real HTML parser (0 block-in-span violations). | +| **`_booth_exists` and `resolve_booth` disagree on a symlink.** The marker called a booth pointing outside the data root alive while the page 404s it — the row renders healthy and the link is dead. | 3/4 | **OPEN — fixed now.** Same containment, same rules. | +| **The board append opens its fd OUTSIDE the lock.** `flock LOCK printf … >> board` reads as locked and is not: the shell opens the append fd while parsing. A concurrent `unlink` replaces the inode via `os.replace`, the old fd keeps pointing at the unlinked one, and the append **succeeds, reports success, and vanishes.** | solo | **OPEN — fixed now.** Pre-existing, not this unit's, but it is silent data loss in the file this unit lives in. Proved by holding the lock and asserting nothing is written. | +| **A pre-planted symlink at the predictable `.benches.json.tmp.`** defeats the atomic write. The replace is atomic, not safe. | solo | **OPEN — fixed now.** `mkstemp` (O_EXCL, same directory), plus an `fsync` before the replace, because `os.replace` orders the rename and not the data behind it. | +| A successful registration can cross the read cap and poison the registry; an empty board hides the panel. | solo | **Already fixed** by the contract round. | + +**Declined, with the reasoning recorded.** Kimi: the `python3 -c` guard under +`set -e` means that on a host where `booth.links` is not importable, `booth +link` now refuses **every** URL, not just booth ones — the refusal mechanism +refuses everything, while the sibling `announce` call degrades gracefully. +**True, and kept as-is deliberately.** A guard that fails open is not a guard, +and the state it describes (the package unreachable from the script that +computes its path from its own location) is a broken install in which `booth +new`, `booth add` and `booth ask` are equally broken. Loud failure with a +message naming what is missing beats silent non-enforcement. Recorded rather +than silently dismissed, because the asymmetry with `announce` is real. + +**What the round says about the method.** The two lenses were complementary in +both directions on one diff: the cold panel found three live defects the +in-session pass missed (all three invisible to a test — a layout nesting, a +symlink disagreement, a lock-ordering race), and the in-session pass had already +closed three of the panel's four convergent findings. Neither substitutes for +the other. The sharpest single line in the reply is the one noting this repo had +already paid for the `RecursionError` class in `marks.py` — **a new module +re-introduced a bug the codebase had a test for**, which no amount of +reading the new module in isolation would surface. diff --git a/scripts/booth b/scripts/booth index 1e273f7..0d63068 100755 --- a/scripts/booth +++ b/scripts/booth @@ -405,9 +405,20 @@ case "$cmd" in # shared lock this line could land inside that window and be rewritten # away by the prune. touch -- "$board/.links.lock" - flock "$board/.links.lock" \ - printf -- '- [%s](%s) · %s · %s\n' \ - "${desc:-$link_url}" "$link_url" "$who" "$when" >> "$board/links.md" + # THE REDIRECTION OPENS INSIDE THE LOCK, which is why this is `sh -c` and + # not a bare printf. `flock LOCK printf ... >> board` reads as locked and is + # not: the SHELL opens the append fd while parsing, before flock acquires + # anything. If a concurrent `unlink` rewrites the board in that window, the + # rewrite lands on a NEW inode via os.replace and this fd still points at + # the old, unlinked one — so the append succeeds, reports success, and the + # row is gone. Found by a cold bug-hunt arm; pre-existing, not U6's, but it + # is a silent data loss in the file this unit spends its time in. + BK_DESC="${desc:-$link_url}" BK_URL="$link_url" BK_WHO="$who" BK_WHEN="$when" \ + BK_BOARD="$board/links.md" \ + flock "$board/.links.lock" sh -c ' + printf -- "- [%s](%s) · %s · %s\n" \ + "$BK_DESC" "$BK_URL" "$BK_WHO" "$BK_WHEN" >> "$BK_BOARD" + ' echo "$URL/b/$LINKS_BOARD/" ;; links) diff --git a/tests/test_benches.py b/tests/test_benches.py index 5d91ff5..0ba27a2 100644 --- a/tests/test_benches.py +++ b/tests/test_benches.py @@ -840,3 +840,50 @@ def test_name_and_owner_ARE_clipped_at_the_read(tmp_path): benches, err = read_benches(tmp_path) assert err is None assert len(benches[0].name) == NAME_MAX and len(benches[0].owner) == OWNER_MAX + + +def test_the_benches_panel_is_not_nested_inside_a_span(tmp_path): + """Cold bug-hunt panel, 3-of-4, seat-confirmed by byte offset in the live + document: the panel `
` had landed INSIDE the booth header's + ``, because the insertion matched the first + `{% if board %}` in the template rather than the block-level one. + + A `
` inside a `` is invalid HTML — the parser closes the span + implicitly and hoists the div out, orphaning the rest of the sub-line. It + renders "fine" in the sense that nothing 500s, which is exactly why no + other test in this file could see it. + + Checked the way the seat checked it: by offset. Defeating change: moving + the panel back above the sub-span's close.""" + _board(tmp_path, ROW_REF) + upsert_bench(tmp_path, "https://talk.test/", "talk", "o") + body = _client(tmp_path).get("/b/links/").text + sub_open = body.index('') + sub_close = body.index("", body.index("· ", sub_open)) + panel = body.index('
') + assert not (sub_open < panel < sub_close), ( + f"the benches div (offset {panel}) sits inside the sub span " + f"({sub_open}..{sub_close})") + + +def test_a_symlinked_booth_is_dead_to_the_marker_as_it_is_to_the_page(tmp_path): + """Cold bug-hunt panel, 3-of-4: `_booth_exists` used a bare `is_dir()` + while `resolve_booth` resolves and requires the parent to BE the data root. + They disagreed on a symlink — the marker called a booth pointing outside + the root alive while the page 404s it, so the row rendered healthy and the + link was dead. The worst of both, and invisible. + + Defeating change: dropping the containment check from `_booth_exists`.""" + outside = tmp_path.parent / f"outside-{tmp_path.name}" + outside.mkdir() + try: + (tmp_path / "escapee").symlink_to(outside, target_is_directory=True) + except OSError: + pytest.skip("no symlink support here") + _board(tmp_path, "- [x](http://h:8090/b/escapee/) · a · 2026-09-01 00:00\n") + c = _client(tmp_path) + body = c.get("/b/links/") + assert body.status_code == 200 + # the page's own verdict on that name, which the marker must agree with + assert c.get("/b/escapee/").status_code == 404 + assert _dead_rows(body.text), "the marker called a booth alive that the page 404s" diff --git a/tests/test_cli.py b/tests/test_cli.py index 9c569ea..d3cfb51 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -625,3 +625,37 @@ def test_a_credential_never_reaches_the_board(booth): assert r.returncode != OK assert "credentials" in r.stderr assert not (data / "links").exists(), "a credentialed URL created the board" + + +def test_the_append_happens_INSIDE_the_lock(booth): + """Cold bug-hunt panel, hulda solo. `flock LOCK printf ... >> board` reads + as locked and is not: the SHELL opens the append fd while parsing, before + flock acquires. A concurrent `unlink` rewriting the board in that window + replaces the inode, the old fd keeps pointing at the unlinked one, and the + append succeeds, reports success, and vanishes. + + Proved by holding the lock: if the open were outside it, `booth link` would + write and exit while blocked. Defeating change: reverting to the bare + `flock LOCK printf ... >>` form, under which this test writes the row. + """ + import fcntl + data, _ = booth + board = data / "links" + board.mkdir(parents=True) + (board / "links.md").write_text("") + lock = board / ".links.lock" + lock.touch() + with lock.open("r+") as lf: + fcntl.flock(lf, fcntl.LOCK_EX) + try: + # subprocess.run directly: `run()` pins timeout=30 itself. + with pytest.raises(subprocess.TimeoutExpired): + subprocess.run( + [str(SCRIPT), "link", "https://ok.test/x", "blocked"], + capture_output=True, text=True, timeout=5, + env={**os.environ, "BOOTH_DATA_DIR": str(data), + "BOOTH_URL": "http://booth.invalid"}) + finally: + fcntl.flock(lf, fcntl.LOCK_UN) + assert (board / "links.md").read_text() == "", \ + "the row was appended while another writer held the lock"