fix(marks): a write over a damaged mark file was wiping the booth's judgment
Three defects and a missing test, all surfaced by the cross-frontier contract panel dispatched before implementation and triaged after it (heid, four arms, artifact-only, thread 01M33VSNFER4N1554G0Y0VC9C8). v0.2.0 was already tagged and announced to fifteen handles when they landed, which is the argument for running the gate at all. DATA LOSS. `marks_for` is deliberately lenient — an unparseable `.marks.json` reads as "no marks" so a review page still loads. The write path inherited that leniency through the same reader, so one flag click appended a single entry to an empty list and atomically replaced the file: every mark in the booth gone, silently, from a click. Reproduced first, then fixed. The fix is an asymmetry, not a retreat from leniency. Reads stay lenient; writes go strict through `_read_raw_strict`, which distinguishes bytes-present-but- unreadable from absent and valid-but-empty, and raises `MarksCorrupt`. The damaged bytes are left on disk. Routes answer 409 rather than 500 — the service is fine and the request was well-formed, the state on disk is not — and the body says what to do, because the alternative the operator reaches for otherwise is deleting the file, which is the thing being protected. The CLI says it in one line instead of a traceback. A PICK COULD NOT TARGET AN ITEM. `Mark.target` carried one, `marks_for_target` retrieved by it, and the panel already rendered "on <item>" — but `declare_pick` had no parameter for it, so no session could produce one. A question about one artifact is the whole point of the 2026-09-09 inline-placement ruling; the door was simply missing. THE IMPORTER STRANDED AN ANSWER. A stem already present as a mark was skipped wholesale. If a session had re-declared that stem through marks while the operator's choice sat in the legacy sidecar, that choice was lost permanently — reads are forbidden from looking at sidecars. The declaration is still skipped (idempotence holds) but a legacy answer is now adopted when the existing mark is an unanswered pick, and an answer made through marks is never overwritten. INV-3 NAMED A SURFACE NOTHING TESTED. All four arms converged on it: the rule protects gallery tile, zoom view and doc view; the falsifiable check covered one. The doc view was implemented and untested, so shipping it unmarked would have passed. Three tests now, one per surface. The contract carries the full triage, including two findings accepted and NOT closed: INV-2's and INV-5's checks comply in letter — openness can be re-derived without spelling the grepped pattern, and importlib inside a function defeats the AST walk. Both describe a future careless change, and the honest statement is that these checks raise the cost of drifting rather than making it impossible. Recorded rather than papered over. Also pins the three prose ambiguities the panel found, normatively and once each: what counts as open, the three distinct broken-declaration cases, and INV-6, which had named a helper that does not exist and forbidden the calls that helper must make. 253 tests.
This commit is contained in:
@@ -713,3 +713,171 @@ def test_a_real_write_then_a_no_op_leaves_the_file_alone(tmp_path):
|
||||
|
||||
set_flag(booth, "a.png", True) # idempotent: already flagged
|
||||
assert path.stat().st_mtime == before, "an idempotent flag rewrote the file"
|
||||
|
||||
|
||||
# ---- findings from the cross-frontier contract panel, 2026-09-22 -------------
|
||||
#
|
||||
# Heid panel (thread 01M33VSNFER4N1554G0Y0VC9C8). Four arms, artifact-only.
|
||||
|
||||
|
||||
def test_a_write_over_a_corrupt_marks_file_refuses_instead_of_replacing(tmp_path):
|
||||
"""DATA LOSS, shipped in v0.2.0. Found by Kimi (flag 2), converged with Hulda.
|
||||
|
||||
`marks_for` is deliberately lenient — an unparseable file reads as "no marks"
|
||||
so a review page still loads. The write path inherited that leniency through
|
||||
the same reader, so the next flag toggle appended one entry to an empty list
|
||||
and atomically replaced the file: every judgment in that booth gone, from one
|
||||
click, silently.
|
||||
|
||||
The read stays lenient and the WRITE goes strict. That asymmetry is the fix —
|
||||
a page that renders without an annotation is recoverable, a file that
|
||||
overwrote the operator's judgment is not, and this repo's standing rule is
|
||||
that nothing deletes his data.
|
||||
"""
|
||||
from booth.marks import MarksCorrupt, set_flag, write_note
|
||||
|
||||
booth = tmp_path / "b"
|
||||
booth.mkdir()
|
||||
write_note(booth, "a.png", "judgment one")
|
||||
write_note(booth, "b.png", "judgment two")
|
||||
raw = (booth / MARKS_FILE).read_text()
|
||||
(booth / MARKS_FILE).write_text(raw[: len(raw) // 2]) # truncated mid-write
|
||||
|
||||
with pytest.raises(MarksCorrupt):
|
||||
set_flag(booth, "c.png", True)
|
||||
|
||||
# The damaged bytes are still on disk — untouched, recoverable by hand.
|
||||
assert (booth / MARKS_FILE).read_text() == raw[: len(raw) // 2]
|
||||
# And the read path is still lenient, so the page renders rather than 500s.
|
||||
assert marks_for(booth) == []
|
||||
|
||||
|
||||
def test_an_absent_or_empty_marks_file_is_not_corrupt(tmp_path):
|
||||
"""The strict write path must not mistake "nothing yet" for "damaged"."""
|
||||
from booth.marks import set_flag
|
||||
|
||||
booth = tmp_path / "b"
|
||||
booth.mkdir()
|
||||
assert set_flag(booth, "a.png", True) is not None # no file at all
|
||||
(booth / MARKS_FILE).write_text("")
|
||||
assert set_flag(booth, "b.png", True) is not None # zero bytes
|
||||
(booth / MARKS_FILE).write_text('{"version": 1, "marks": []}')
|
||||
assert set_flag(booth, "c.png", True) is not None # valid but empty
|
||||
|
||||
|
||||
def test_a_pick_can_target_one_item(tmp_path):
|
||||
"""Found by Hulda (flag 1), converged with Regin.
|
||||
|
||||
`Mark.target` carries an item rel, `marks_for_target` retrieves by it, and
|
||||
the panel template already renders "on <item>" for a pick — but
|
||||
`declare_pick` had no target parameter, so a session could not actually
|
||||
produce one. A question about ONE artifact is the 2026-09-09 ruling's whole
|
||||
point; the record supported it and the door was missing.
|
||||
"""
|
||||
from booth.marks import marks_for_target
|
||||
|
||||
booth = tmp_path / "b"
|
||||
booth.mkdir()
|
||||
declare_pick(booth, "which-crop", _single(), target="v3/DSC03389.jpg")
|
||||
m = marks_for(booth)[0]
|
||||
assert m.target == "v3/DSC03389.jpg"
|
||||
assert [x.id for x in marks_for_target(marks_for(booth), "v3/DSC03389.jpg")] == ["which-crop"]
|
||||
# and it still answers normally
|
||||
answer_pick(booth, "which-crop", "A — baseline")
|
||||
assert marks_for(booth)[0].answer["complete"] is True
|
||||
|
||||
|
||||
def test_a_pick_target_cannot_escape_the_booth(tmp_path):
|
||||
booth = tmp_path / "b"
|
||||
booth.mkdir()
|
||||
for bad in ("../outside.png", "/etc/passwd"):
|
||||
with pytest.raises(AskError):
|
||||
declare_pick(booth, "p", _single(), target=bad)
|
||||
|
||||
|
||||
def test_redeclaring_a_pick_may_move_its_target(tmp_path):
|
||||
booth = tmp_path / "b"
|
||||
booth.mkdir()
|
||||
declare_pick(booth, "p", _single(), target="a.png")
|
||||
declare_pick(booth, "p", _single(), target="b.png")
|
||||
assert marks_for(booth)[0].target == "b.png"
|
||||
|
||||
|
||||
def test_import_adopts_a_legacy_answer_for_an_already_declared_pick(tmp_path):
|
||||
"""Found by Gróa (flag 10).
|
||||
|
||||
The idempotence rule skipped any stem already present as a mark. If a
|
||||
session had re-declared that stem through marks (so the mark exists, still
|
||||
unanswered) while the operator's answer sat in the legacy sidecar, the import
|
||||
skipped and that answer was stranded on disk forever — with the read path
|
||||
forbidden from looking at sidecars. Adopting the answer preserves both rules:
|
||||
idempotent, and never clobbers a NEWER judgment.
|
||||
"""
|
||||
from booth.asks import ANSWER_SUFFIX, build_answer, normalize_ask
|
||||
from booth.marks import import_legacy_asks
|
||||
|
||||
booth = tmp_path / "b"
|
||||
_sidecar(booth, "winner", _single())
|
||||
doc = build_answer(normalize_ask(_single(), "winner"), "B — async", notes="from the sidecar")
|
||||
(booth / f"winner{ANSWER_SUFFIX}").write_text(json.dumps(doc))
|
||||
declare_pick(booth, "winner", _single()) # re-declared, unanswered
|
||||
assert marks_for(booth)[0].answer is None
|
||||
|
||||
import_legacy_asks(booth)
|
||||
got = marks_for(booth)[0]
|
||||
assert got.answer is not None, "the legacy answer was stranded"
|
||||
assert got.answer["choice"] == "B — async"
|
||||
assert open_marks(marks_for(booth)) == []
|
||||
|
||||
|
||||
def test_import_never_overwrites_an_answer_made_through_marks(tmp_path):
|
||||
"""The other half of the same rule: a judgment recorded SINCE the sidecar
|
||||
outranks it, and adoption must not reach back over it."""
|
||||
from booth.asks import ANSWER_SUFFIX, build_answer, normalize_ask
|
||||
from booth.marks import import_legacy_asks
|
||||
|
||||
booth = tmp_path / "b"
|
||||
_sidecar(booth, "winner", _single())
|
||||
old = build_answer(normalize_ask(_single(), "winner"), "A — baseline")
|
||||
(booth / f"winner{ANSWER_SUFFIX}").write_text(json.dumps(old))
|
||||
declare_pick(booth, "winner", _single())
|
||||
answer_pick(booth, "winner", "B — async") # the operator changed his mind
|
||||
|
||||
import_legacy_asks(booth)
|
||||
assert marks_for(booth)[0].answer["choice"] == "B — async"
|
||||
|
||||
|
||||
def test_the_doc_view_carries_the_marks(client):
|
||||
"""INV-3's third surface — flagged 4/4 by the panel as named in the rule but
|
||||
covered by no test, so shipping it unmarked would have passed."""
|
||||
from booth.marks import write_note
|
||||
|
||||
c, data = client
|
||||
b = data / "b"
|
||||
b.mkdir()
|
||||
(b / "notes.md").write_text("# report\n\nprose here\n")
|
||||
write_note(b, "notes.md", "this section is wrong")
|
||||
|
||||
html = c.get("/b/b/view?f=notes.md").text
|
||||
assert "prose here" in html
|
||||
assert "this section is wrong" in html
|
||||
|
||||
|
||||
def test_a_corrupt_marks_file_gives_the_browser_a_409_not_a_500(client):
|
||||
"""The request was fine and the service is fine — the state on disk is not,
|
||||
and the refusal is deliberate. A 500 would read as "the Booth is broken" and
|
||||
send the operator looking for something to restart."""
|
||||
c, data = client
|
||||
b = data / "b"
|
||||
b.mkdir()
|
||||
from booth.marks import write_note
|
||||
write_note(b, "a.png", "keep me")
|
||||
(b / MARKS_FILE).write_text("{truncated")
|
||||
|
||||
r = c.post("/b/b/flag", data={"target": "a.png", "on": "1"}, follow_redirects=False)
|
||||
assert r.status_code == 409
|
||||
body = r.json()
|
||||
assert "cannot be read" in body["error"] and body["fix"]
|
||||
# the page still renders, so the operator can see the booth at all
|
||||
assert c.get("/b/b/").status_code == 200
|
||||
assert c.get("/b/b/marks.json").status_code == 200
|
||||
|
||||
Reference in New Issue
Block a user