fix(editor): serialise save and seal — the threading fix could seal an unvalidated register

A second adversarial review, aimed at the previous round's fixes, found
that ThreadingHTTPServer (added there so a long 1Password pull could not
block Save or Cancel) turned the register file into shared mutable state.

setup.sh validates the register and encrypts it in two separate passes.
A /save arriving between them was invisible to the validator: /seal still
returned ok, the ceremony still showed three shares, and the owner still
walked away with a printed, share-split Executor File containing a
register that never held their assets. Measured: 238 bytes against a
2646-byte control.

- One RLock now spans the whole of /save and the whole of /seal, plus
  /load and the dedup scan that read the same file. The browser disables
  Save for the duration too.
- Each save writes its own temp name and unlinks it on failure. Two
  concurrent saves used to share target + ".tmp" and race os.replace, so
  the winner renamed the LOSER's bytes into place and returned 200 to
  the wrong request.
- The suite proves the invariant by timing, and the test was checked for
  teeth: with the lock neutered it fails, sealing a 208-byte invalid
  register.
- setup.sh gains a timeout; one 1Password pull at a time.

Also from the same review:
- The honesty gate could not be cleared. Judging "untouched action" field
  by field left an owner who rewrote the note but kept an action that was
  simply right warned forever — and notify-only is right for most of a
  real vault, so a 563-item import would have made the warning permanent
  noise, which is how a gate teaches people to click through it.
  Engagement is now per card: the first edit to any field clears it.
- \b was the wrong boundary for vault names: "_" is a word character, so
  Work_Vault and Teams matched nothing and defaulted to TICKED — pulling
  a colleague's accounts into a personal estate register. Bounded on
  letters now, with an optional trailing s. Homework and Framework stay
  ticked.
- Declining the gate said nothing, so the button looked broken; the
  zero-match screen claimed items did not exist when they were merely
  hidden; the wizard's "transfer" wording named a beneficiary the wizard
  never asks for.

Suite 166 -> 169 pass / 0 fail (167/0 under expect), shellcheck clean.
Spec §12.2 records the round.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: TheWayWithin

CLAUDE.md

@@ -88,6 +88,17 @@ All specs live in `ideation/` (moved from repo root, 17 Jul 2026):
   matching. Every one lived in the seam between the importer and
   something that already existed, not in the import logic where the
   tests were densest.
+  A **second** review, aimed at those fixes, found that the
+  `ThreadingHTTPServer` change had made the register file shared mutable
+  state (spec §12.2): a **Save landing inside a Seal made setup.sh
+  validate one register and encrypt another** — `/seal` returned ok, the
+  ceremony ran, and the owner got printed shares to a 238-byte file that
+  never held their assets. Now one `RLock` spans save/seal/load, each
+  save writes its own temp file, and the suite proves the invariant by
+  timing (verified to FAIL with the lock removed). Also: the honesty
+  gate could never be cleared once bulk-imported (cry-wolf), and `\b`
+  matched vault names wrongly so `Work_Vault` and `Teams` defaulted to
+  ticked. Suite **169 pass / 0 fail** (167/0 under expect).
 - **Pickup sequence (agreed 29 Jul 2026; P1 and P2 now done):**
   1. ~~EF-ISS-8 **P1** — pull + mapper + stub-`op` tests.~~ DONE 29 Jul.
   2. ~~**P2** — editor integration.~~ DONE 10 Aug.

ideation/1password-import-spec.md

@@ -452,3 +452,54 @@ between the importer and something that already existed** — the seal
 path, review mode, the notes field, the executor's reading experience.
 None of them were in the import logic itself, which is where the tests
 were densest.
+
+### 12.2 Second review round: the threading fix bit back (2026-08-10)
+
+A second independent pass, briefed to attack §12.1's fixes rather than the
+original build, found that **the `ThreadingHTTPServer` change in §12.1(6)
+introduced the worst defect in the whole feature**, and proved it end to
+end. Recorded because the lesson is general: a fix aimed at responsiveness
+turned a serialised file into shared mutable state.
+
+1. **A Save landing inside a Seal seals a register nobody validated.**
+   `setup.sh` validates the file and encrypts it in two separate passes.
+   Once the server was threaded, a `/save` arriving between those passes
+   was invisible to the validator: `/seal` still returned `ok`, the
+   ceremony still displayed three shares, and the owner still walked away
+   with a printed, share-split Executor File — containing a register that
+   never held their assets. Measured at 238 bytes against a 2646-byte
+   control. One `RLock` now spans the whole of `/save` and the whole of
+   `/seal` (and `/load` and the dedup scan, which read the same file),
+   the browser disables Save for the duration, and the suite proves the
+   invariant by timing. **The test was checked for teeth**: with the lock
+   neutered it fails, sealing a 208-byte invalid register.
+2. **Two concurrent saves swapped bodies.** Both wrote the same
+   `target + ".tmp"` and raced `os.replace`; the winner renamed the
+   *loser's* bytes into place and returned 200 to the wrong request. Each
+   save now writes its own temp name, unlinks it on failure (a leftover
+   would be a plaintext register fragment), and takes the lock.
+3. **The honesty gate could not be cleared.** Judging "untouched action"
+   field by field meant an owner who rewrote the note but kept an action
+   that was simply right stayed warned forever — and `notify-only` is
+   right for most of a real vault, so on a 563-item import the warning
+   would have become permanent noise, which is how a gate teaches people
+   to click through it. Engagement is now tracked per card: the first
+   edit to any field removes the stamp.
+4. **`\b` was the wrong boundary for vault names.** `_` is a word
+   character, so `\bwork\b` missed `Work_Vault` and `\bteam\b` missed
+   `Teams` — and those default to *ticked*, which pulls a colleague's
+   accounts into a personal estate register. Now bounded on letters, with
+   an optional trailing `s`. "Homework" and "Framework" stay ticked.
+5. Smaller: the zero-match screen said "nothing matches" for items that
+   were merely hidden, and offered no way to reveal them; declining the
+   honesty gate said nothing at all, so the button looked broken;
+   `setup.sh` had no timeout, which after the threading change left a
+   permanently disabled Seal button; and the wizard's `transfer` wording
+   named a beneficiary the wizard never asks for, so the executor would
+   have been told to transfer an asset to nobody in particular.
+
+**Two rounds, two lessons.** The first: the defects were in the seams
+between the importer and what already existed. The second: the *fixes*
+were where the next defects lived, and the most dangerous one came from
+the most innocuous-looking change in the round. Both rounds were found by
+briefing a fresh reviewer from this spec rather than from the build.

tests/run-tests.sh

@@ -644,6 +644,16 @@ edcheck 'importGate("Save and finish anyway?")' "review-mode Save goes through i
 edcheck "dataset.importedAction"     "bulk drafts are stamped so the gate sees an untouched action"
 edcheck "impKeys"                    "the wizard is keyboard-reachable"
 edcheck "Nothing matches"            "a filter matching nothing does not end the wizard"
+edcheck "delete card.dataset.importedAction" "editing an imported entry clears the warning"
+# The register file is shared mutable state now the server is threaded. A
+# Save landing inside a Seal makes setup.sh validate one register and encrypt
+# another — proven possible with the lock removed.
+grep -q "file_lock = threading.RLock()" "$ROOT/web/edit-server.py" \
+  && ok "the register file is guarded by a lock" \
+  || fail "the save/seal lock is gone — a Save can be sealed unvalidated"
+grep -q 'tmp = "%s.tmp.%d" % (target, threading.get_ident())' "$ROOT/web/edit-server.py" \
+  && ok "each save writes its own temp file" \
+  || fail "concurrent saves share one temp name and can swap bodies"
 edcheck 'body: buildYAML()})' "sealing saves what is on screen first"
 # The wizard must not seed the imported marker as the owner's own words: it
 # would be sealed and read by the executor as a to-do for someone who has died.

tests/test-edit-server.py

@@ -10,11 +10,14 @@
 Exit 0 = all good; prints a diagnostic and exits 1 on any failure.
 """
 
+import glob
 import json
 import os
+import shutil
 import subprocess
 import sys
 import tempfile
+import threading
 import time
 import urllib.request
 
@@ -245,6 +248,93 @@ def check_import_endpoint(d):
     print("ok: import endpoint pulls metadata only, dedups, and fails cleanly")
 
 
+def check_save_seal_serialised(d):
+    """setup.sh validates the register and encrypts it in two separate passes.
+    A /save landing between them would be sealed WITHOUT ever being validated
+    — the owner would walk away with printed shares to a file that does not
+    hold their assets, and nothing would say so. The server must therefore
+    hold one lock across the whole of /save and the whole of /seal.
+    Proven by timing rather than by decrypting: the save must not complete
+    until the seal has."""
+    target = os.path.join(d, "race.yaml")
+    with open(EXAMPLE) as f:
+        original = f.read()
+    open(target, "w").write(original)
+    proc, url = start(target, mode="create")
+
+    took = {}
+
+    def seal():
+        t0 = time.time()
+        try:
+            took["seal_body"] = post(url + "seal", json.dumps({"overwrite": True}))
+        finally:
+            took["seal"] = (t0, time.time())
+
+    th = threading.Thread(target=seal)
+    th.start()
+    time.sleep(0.5)                      # let setup.sh get past its validation pass
+    t0 = time.time()
+    post(url + "save", SAVE_BODY)        # a valid but completely different register
+    took["save"] = (t0, time.time())
+    th.join(120)
+    if th.is_alive():
+        fail("the seal never returned", proc)
+
+    if not json.loads(took["seal_body"]).get("ok"):
+        fail(f"the seal itself failed: {took['seal_body'][:200]}", proc)
+    if took["save"][1] < took["seal"][1]:
+        fail("a Save completed while a Seal was running — setup.sh can then "
+             "validate one register and encrypt another")
+    # And the register on disk is exactly one of the two, never a blend.
+    on_disk = open(target).read()
+    if on_disk != SAVE_BODY and on_disk != original:
+        fail("the register on disk is neither of the two bodies written")
+    if glob.glob(target + ".tmp*"):
+        fail("a plaintext .tmp fragment was left beside the register")
+
+    post(url + "cancel")
+    wait_exit(proc, 2)
+    print("ok: a Save cannot land inside a Seal, and leaves no plaintext fragment")
+
+
+def check_concurrent_saves(d):
+    """Two saves must not swap bodies: with one shared temp filename the
+    winner renames the loser's bytes into place and still returns 200."""
+    target = os.path.join(d, "twosaves.yaml")
+    with open(EXAMPLE) as f:
+        open(target, "w").write(f.read())
+    proc, url = start(target, mode="create")
+
+    bodies = [SAVE_BODY.replace("Rev Tester", "Writer One"),
+              SAVE_BODY.replace("Rev Tester", "Writer Two")]
+    errors = []
+
+    def save(b):
+        try:
+            post(url + "save", b)
+        except Exception as e:                      # noqa: BLE001 - reported below
+            errors.append(str(e))
+
+    threads = [threading.Thread(target=save, args=(b,)) for b in bodies]
+    for t in threads:
+        t.start()
+    for t in threads:
+        t.join(30)
+
+    if errors:
+        fail(f"a concurrent save errored instead of being serialised: {errors}", proc)
+    on_disk = open(target).read()
+    if on_disk not in bodies:
+        fail("two concurrent saves produced a register that is neither of them", proc)
+    if glob.glob(target + ".tmp*"):
+        fail("a plaintext .tmp fragment was left beside the register", proc)
+
+    post(url + "cancel")
+    wait_exit(proc, 2)
+    print("ok: concurrent saves are serialised and cannot blend registers")
+
+
 def main():
     if not os.path.isfile(SERVER):
         fail(f"server not found at {SERVER}")
@@ -330,6 +420,13 @@ def main():
         else:
             print("note: create+seal path skipped (age/ssss not installed)")
 
+        # ---- threading safety around the register file ----
+        check_concurrent_saves(d)
+        if shutil.which("age") and shutil.which("ssss-split"):
+            check_save_seal_serialised(d)
+        else:
+            print("note: save-during-seal race check skipped (age/ssss not installed)")
+
         # ---- 1Password import endpoint (EF-ISS-8 P2) ----
         check_import_endpoint(d)
 

web/edit-server.py

@@ -33,6 +33,7 @@
 EDITOR_HTML = os.path.join(HERE, "editor.html")
 IDLE_TIMEOUT = 3600  # give up after an hour untouched
 IMPORT_TIMEOUT = 180  # the owner may be staring at 1Password's approval dialog
+SEAL_TIMEOUT = 600    # generous: setup.sh runs the whole proof chain
 
 # scripts/import-1password.sh exit code -> (machine code, plain-English line).
 # The script's own stderr carries the detail and is passed through verbatim.
@@ -71,6 +72,15 @@ def main() -> int:
 
     REPO_ROOT = os.path.dirname(HERE)
     state = {"result": None}
+    # The server is threaded so a long 1Password pull cannot block Save or
+    # Cancel — but that makes the register file shared mutable state across
+    # threads. Everything that reads or writes it takes this lock. Without it
+    # a Save landing inside a Seal is catastrophic and silent: setup.sh
+    # validates the file and encrypts it in two separate passes, so it will
+    # happily seal a register that never passed validation, and the owner
+    # walks away with shares to a file that does not hold their assets.
+    file_lock = threading.RLock()
+    import_busy = threading.Lock()
 
     def run_validate():
         p = subprocess.run(
@@ -93,10 +103,15 @@ def run_seal(overwrite):
         env = dict(os.environ, EXECUTOR_FILE_EMIT="1")
         # setup.sh is a bash script (set -o pipefail): invoke it via its own
         # shebang, not `sh` — Ubuntu's /bin/sh is dash and rejects pipefail.
-        p = subprocess.run(
-            [os.path.join(REPO_ROOT, "scripts", "setup.sh"), target],
-            stdin=subprocess.DEVNULL, capture_output=True, text=True, env=env,
-        )
+        try:
+            p = subprocess.run(
+                [os.path.join(REPO_ROOT, "scripts", "setup.sh"), target],
+                stdin=subprocess.DEVNULL, capture_output=True, text=True, env=env,
+                timeout=SEAL_TIMEOUT,
+            )
+        except subprocess.TimeoutExpired:
+            return {"ok": False, "error": "sealing did not finish in time; "
+                                          "nothing was changed. Try again."}
         if p.returncode != 0:
             msg = (p.stderr or p.stdout or "seal failed").strip()
             return {"ok": False, "error": msg[-800:]}
@@ -148,7 +163,8 @@ def flush():
                 assets.append(dict(cur))
                 cur.clear()
 
-        with open(target, encoding="utf-8", errors="replace") as f:
+        # Under the lock: a Save could be replacing this file underneath us.
+        with file_lock, open(target, encoding="utf-8", errors="replace") as f:
             for line in f:
                 if re.match(r"^[A-Za-z_]+:", line):
                     flush()
@@ -255,30 +271,49 @@ def do_GET(self):
                     self._send(200, body, "text/html; charset=utf-8")
             elif self.path == "/load":
                 data = b""
-                if os.path.isfile(target):
-                    with open(target, "rb") as f:
-                        data = f.read()
+                with file_lock:
+                    if os.path.isfile(target):
+                        with open(target, "rb") as f:
+                            data = f.read()
                 self._send(200, data)
             elif self.path == "/validate":
                 self._json(run_validate())
             elif self.path.split("?", 1)[0] == "/import/1password":
                 q = urllib.parse.parse_qs(urllib.parse.urlsplit(self.path).query)
-                self._json(run_import(q.get("vaults", []) == ["1"],
-                                      [v for v in q.get("vault", []) if v.strip()]))
+                # One pull at a time. Two would mean two 1Password approval
+                # dialogs racing each other in front of a confused owner.
+                if not import_busy.acquire(blocking=False):
+                    self._json({"ok": False, "code": "busy",
+                                "message": "A pull is already running.",
+                                "detail": "Wait for it to finish, or approve it in 1Password."})
+                    return
+                try:
+                    self._json(run_import(q.get("vaults", []) == ["1"],
+                                          [v for v in q.get("vault", []) if v.strip()]))
+                finally:
+                    import_busy.release()
             else:
                 self._send(404, b"not found")
 
         def do_POST(self):
             length = int(self.headers.get("Content-Length", 0))
             body = self.rfile.read(length) if length else b""
             if self.path == "/save":
-                try:
-                    tmp = target + ".tmp"
-                    with open(tmp, "wb") as f:
-                        f.write(body)
-                    os.replace(tmp, target)
-                except OSError as e:
-                    self._send(500, str(e).encode()); return
+                # Its own temp name per request, and the lock, so two saves
+                # cannot swap bodies: without both, the request that gets the
+                # 200 is not necessarily the one whose bytes landed.
+                tmp = "%s.tmp.%d" % (target, threading.get_ident())
+                with file_lock:
+                    try:
+                        with open(tmp, "wb") as f:
+                            f.write(body)
+                        os.replace(tmp, target)
+                    except OSError as e:
+                        try:
+                            os.unlink(tmp)          # never leave plaintext behind
+                        except OSError:
+                            pass
+                        self._send(500, str(e).encode()); return
                 # In create mode the server stays up so the owner can seal;
                 # in review mode saving is the end and review.sh takes over.
                 if mode == "create":
@@ -290,7 +325,11 @@ def do_POST(self):
                     params = json.loads(body) if body else {}
                 except ValueError:
                     params = {}
-                self._json(run_seal(bool(params.get("overwrite"))))
+                # Held for the whole ceremony: setup.sh validates and encrypts
+                # in separate passes, and a Save landing between them would be
+                # sealed without ever being validated.
+                with file_lock:
+                    self._json(run_seal(bool(params.get("overwrite"))))
             elif self.path == "/done":
                 self._send(200, b"done"); self._stop("saved")
             elif self.path == "/cancel":