fix(u6): fold the cold bug-hunt panel — a div in a span, a symlink split, and an append outside its lock

/heid-bug-hunt panel 01M35CRRK2RTVWWF1BN09AFQG3, 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. Three
were not.

- The benches panel was nested inside the booth header's <span class="sub">.
  The insertion had matched the first `{% if board %}` in the template rather
  than the block-level one. A div inside a span is invalid HTML: the parser
  closes the span implicitly and hoists the div out, orphaning the rest of the
  sub-line. Nothing 500s, which is precisely why no test in this suite could
  see it. Moved to block level, pinned by an offset assertion, and verified
  with a real HTML parser.

- _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. Same containment now, and
  ValueError joins OSError in the guard -- one bad row must never cost the
  other 220.

- The board append opened its fd OUTSIDE the lock. `flock LOCK printf ... >>
  board` reads as locked and is not: the shell opens the append fd while
  parsing, before flock acquires. A concurrent unlink replaces the inode via
  os.replace, the old fd still points at the unlinked one, and the append
  succeeds, reports success, and vanishes. Pre-existing rather than 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.

- The atomic write used a predictable .tmp.<pid> name; a pre-planted symlink
  there redirects the write straight through the replace. mkstemp with O_EXCL
  in the same directory, and an fsync before the replace -- os.replace orders
  the rename, not the data behind it.

Declined and recorded: on a host where booth.links cannot be imported, `booth
link` now refuses every URL rather than only booth ones. True, and kept. A
guard that fails open is not a guard, and that state is a broken install in
which most of the CLI is equally broken.

The sharpest line in the reply is one three arms found independently: this repo
had ALREADY paid for the RecursionError class in marks.py, and the new module
re-introduced the unguarded parse. Reading the new module in isolation would
never have surfaced that.

604 -> 607 tests.
This commit is contained in:
vh
2026-09-22 14:20:06 -07:00
parent e3853e2692
commit 8cb21193dc
7 changed files with 220 additions and 55 deletions
+11 -2
View File
@@ -1008,8 +1008,17 @@ def create_app(
178 of 221 rows today, on the ONE booth that carries a links.md. 178 of 221 rows today, on the ONE booth that carries a links.md.
""" """
try: try:
return (data_dir / name).is_dir() candidate = (data_dir / name).resolve()
except OSError: # 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 return False
def _mark_redirect(name: str, form, anchor: str) -> RedirectResponse: def _mark_redirect(name: str, form, anchor: str) -> RedirectResponse:
+17 -2
View File
@@ -27,6 +27,7 @@ import fcntl
import json import json
import os import os
import stat import stat
import tempfile
from dataclasses import dataclass, replace from dataclasses import dataclass, replace
from datetime import datetime, timezone from datetime import datetime, timezone
from pathlib import Path from pathlib import Path
@@ -299,9 +300,23 @@ def _write_all(root: Path, benches: dict[str, Bench]) -> None:
raise ValueError( raise ValueError(
f"that registration would push the registry past {BENCHES_MAX_BYTES} " f"that registration would push the registry past {BENCHES_MAX_BYTES} "
f"bytes, which its own reader refuses; nothing was written") 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.<pid>` 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: 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) os.replace(tmp, path)
except BaseException: except BaseException:
# A write that dies between create and replace would otherwise strand # A write that dies between create and replace would otherwise strand
+56 -48
View File
@@ -66,7 +66,62 @@
{% else %} {% else %}
<h1>{{ name }}</h1> <h1>{{ name }}</h1>
{% endif %} {% endif %}
<span class="sub">{% if uploaded %}<span class="badge">⬆ pickup</span> {% endif %}{% if is_board %} <span class="sub">{% if uploaded %}<span class="badge">⬆ pickup</span> {% 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 %}<span class="badge badge-mark">{{ marks_open }} open</span> · {% endif %}{{ items|length }} item{{ '' if items|length == 1 else 's' }} · {{ lifetime(kept, hold, expires_in) }}{% endif %}</span>
{% if items %}<a class="dl-link" href="/b/{{ name_url }}/?download=1" title="download this booth as a zip">⬇ zip</a>{% 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 %}
<form class="keep-lg" method="post" action="/b/{{ name_url }}/unkeep">
<input type="hidden" name="next" value="/b/{{ name_url }}/">
<button title="release — rejoins the TTL sweep">★ kept — release</button>
</form>
{% else %}
<form class="keep-lg" method="post" action="/b/{{ name_url }}/keep">
<input type="hidden" name="next" value="/b/{{ name_url }}/">
<button title="keep — exempt from the TTL sweep">☆ keep</button>
</form>
{% endif %}
{% if not board %}
<form class="wipe wipe-lg" method="post" action="/b/{{ name_url }}/delete"
onsubmit="return confirm('Wipe this booth now?')">
<button>Wipe now</button>
</form>
{% endif %}
</div>
{% if uploaded %}
<div class="pickup-note">
📦 Pickup <code>{{ name }}</code>
<button type="button" class="copy-btn" data-copy="{{ name }}" title="copy id to clipboard">⧉ copy</button>
— download files below, or on nh3-dev grab <code>~/booth-data/{{ name }}/</code>
</div>
{% 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 <div> spent one commit nested inside the `<span class="sub">` of the
booth header, because the insertion matched the FIRST `{% if board %}` in
the file rather than the block-level one. A <div> inside a <span> 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, {# 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 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 are fully deployed. NOT a booth (a booth announces itself and is swept) and
@@ -127,53 +182,6 @@
</div> </div>
{% endif %} {% 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 %}<span class="badge badge-mark">{{ marks_open }} open</span> · {% endif %}{{ items|length }} item{{ '' if items|length == 1 else 's' }} · {{ lifetime(kept, hold, expires_in) }}{% endif %}</span>
{% if items %}<a class="dl-link" href="/b/{{ name_url }}/?download=1" title="download this booth as a zip">⬇ zip</a>{% 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 %}
<form class="keep-lg" method="post" action="/b/{{ name_url }}/unkeep">
<input type="hidden" name="next" value="/b/{{ name_url }}/">
<button title="release — rejoins the TTL sweep">★ kept — release</button>
</form>
{% else %}
<form class="keep-lg" method="post" action="/b/{{ name_url }}/keep">
<input type="hidden" name="next" value="/b/{{ name_url }}/">
<button title="keep — exempt from the TTL sweep">☆ keep</button>
</form>
{% endif %}
{% if not board %}
<form class="wipe wipe-lg" method="post" action="/b/{{ name_url }}/delete"
onsubmit="return confirm('Wipe this booth now?')">
<button>Wipe now</button>
</form>
{% endif %}
</div>
{% if uploaded %}
<div class="pickup-note">
📦 Pickup <code>{{ name }}</code>
<button type="button" class="copy-btn" data-copy="{{ name }}" title="copy id to clipboard">⧉ copy</button>
— download files below, or on nh3-dev grab <code>~/booth-data/{{ name }}/</code>
</div>
{% 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 %} {% if board %}
{# THE STANDING LINK BOARD. Every agent session on the fleet appends here, so {# 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 this is the one booth where the useful granularity is the ROW, not the
+41
View File
@@ -538,3 +538,44 @@ All four closed.
INV-8's file list, the `registered`/`created` wording, and every line number in 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 this document's prose — the panel found two already stale, which is the whole
argument against putting them in prose at all. 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 `<span class="sub">`.** A `<div>` in a `<span>`: 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.<pid>`** 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.
+14 -3
View File
@@ -405,9 +405,20 @@ case "$cmd" in
# shared lock this line could land inside that window and be rewritten # shared lock this line could land inside that window and be rewritten
# away by the prune. # away by the prune.
touch -- "$board/.links.lock" touch -- "$board/.links.lock"
flock "$board/.links.lock" \ # THE REDIRECTION OPENS INSIDE THE LOCK, which is why this is `sh -c` and
printf -- '- [%s](%s) <sub>· %s · %s</sub>\n' \ # not a bare printf. `flock LOCK printf ... >> board` reads as locked and is
"${desc:-$link_url}" "$link_url" "$who" "$when" >> "$board/links.md" # 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) <sub>· %s · %s</sub>\n" \
"$BK_DESC" "$BK_URL" "$BK_WHO" "$BK_WHEN" >> "$BK_BOARD"
'
echo "$URL/b/$LINKS_BOARD/" echo "$URL/b/$LINKS_BOARD/"
;; ;;
links) links)
+47
View File
@@ -840,3 +840,50 @@ def test_name_and_owner_ARE_clipped_at_the_read(tmp_path):
benches, err = read_benches(tmp_path) benches, err = read_benches(tmp_path)
assert err is None assert err is None
assert len(benches[0].name) == NAME_MAX and len(benches[0].owner) == OWNER_MAX 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 `<div>` had landed INSIDE the booth header's
`<span class="sub">`, because the insertion matched the first
`{% if board %}` in the template rather than the block-level one.
A `<div>` inside a `<span>` 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('<span class="sub">')
sub_close = body.index("</span>", body.index("· ", sub_open))
panel = body.index('<div class="benches">')
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/) <sub>· a · 2026-09-01 00:00</sub>\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"
+34
View File
@@ -625,3 +625,37 @@ def test_a_credential_never_reaches_the_board(booth):
assert r.returncode != OK assert r.returncode != OK
assert "credentials" in r.stderr assert "credentials" in r.stderr
assert not (data / "links").exists(), "a credentialed URL created the board" 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"