fix: read strictly before a merge-on-write so a failed read cannot wipe the file

tapscribe/config_store.py

@@ -15,6 +15,7 @@
 import json
 import os
 import re
+import stat
 import tempfile
 from collections.abc import Callable
 from dataclasses import dataclass
@@ -322,6 +323,26 @@ def validate_config_text(content: str) -> str:
     return content
 
 
+def read_json_strict(path: str | os.PathLike[str]) -> Any:
+    """Parse `path` as JSON, or None when nothing readable is there: the file
+    is absent, is not a regular file, or is torn. Every other `OSError`
+    (EACCES, EIO, EMFILE) raises, so a read-modify-write can tell "nothing
+    here" from "I could not read it" (#446)."""
+    try:
+        st = os.stat(path)
+    except (FileNotFoundError, NotADirectoryError):
+        return None
+    if not stat.S_ISREG(st.st_mode):
+        # A directory is not a JSON file, and opening a FIFO blocks the caller.
+        return None
+    try:
+        with open(path, encoding="utf-8") as fh:
+            return json.load(fh)
+    except (FileNotFoundError, NotADirectoryError, ValueError):
+        # Removed between the stat and the open, or torn: absent, not unreadable.
+        return None
+
+
 def atomic_write_text(path: Path, content: str) -> None:
     """Write `content` to `path` via tempfile + os.replace so a crashed
     write never leaves a half-written file on disk. Caller is responsible

tapscribe/roster.py

@@ -36,6 +36,7 @@
 from pathlib import Path
 from typing import Any
 
+from .config_store import read_json_strict
 from .session_paths import FILENAME_ROSTER_JSON
 from .tap_mode import TAP_MODE_MULTI, TAP_MODE_SINGLE, is_mode
 from .text import atomic_write_text, parse_wav_speaker_slug
@@ -107,8 +108,8 @@ def _coerce_entry(value: Any) -> dict[str, Any] | None:
 def coerce_roster(raw: Any) -> dict[str, dict[str, Any]]:
     """Coerce a raw parsed roster mapping into `{full identity: entry}`, dropping
     non-str identities and non-dict entries (and per-field junk, via
-    `_coerce_entry`). Non-dict top level → `{}`. Shared by `read_roster` (the
-    uncached write-path reader) and the cached poll path
+    `_coerce_entry`). Non-dict top level → `{}`. Shared by `load_roster` (the
+    strict write-path reader) and the cached poll path
     (`sessions._read_roster_cached`) so both produce the identical shape."""
     if not isinstance(raw, dict):
         return {}
@@ -138,18 +139,22 @@ def slug_owners(roster: Mapping[str, Any]) -> dict[str, set[str]]:
     return owners
 
 
+def load_roster(session_dir: Path) -> dict[str, dict[str, Any]]:
+    """The session's roster, parsed and coerced, for a read-modify-write. An
+    absent or torn file reads as `{}`, because the next occurrence rebuilds it.
+    Any other `OSError` raises, so the write never replaces a roster it could
+    not read (#446)."""
+    return coerce_roster(read_json_strict(session_dir / FILENAME_ROSTER_JSON))
+
+
 def read_roster(session_dir: Path) -> dict[str, dict[str, Any]]:
-    """The session's roster as `{full identity: entry}`. Missing, torn, or
-    non-dict top level → `{}` so a single bad file never crashes the poll."""
-    path = session_dir / FILENAME_ROSTER_JSON
+    """Lenient display/poll reader: the session's roster as `{full identity:
+    entry}`, unreadable for ANY reason → `{}` so a bad file never crashes the
+    poll. Read-modify-write callers use `load_roster`."""
     try:
-        data = json.loads(path.read_text(encoding="utf-8"))
-    except (OSError, ValueError):
-        # Missing file (OSError) or torn/garbage JSON (ValueError): the roster
-        # is best-effort durable state, recovered on the next occurrence — a
-        # read failure must degrade to "no roster", never propagate.
+        return load_roster(session_dir)
+    except OSError:
         return {}
-    return coerce_roster(data)
 
 
 def record_occurrence(
@@ -171,7 +176,7 @@ def record_occurrence(
     `sanitise_name` HERE — the one seam where it becomes durable state."""
     if not identity:
         return
-    roster = read_roster(session_dir)
+    roster = load_roster(session_dir)
     entry = roster.get(identity) or {
         "name": "",
         "source": "live",

tapscribe/routes/people.py

@@ -41,7 +41,7 @@
 from ..session_paths import resolve_session_dir
 from ..sessions import (
     gather_sessions,
-    read_session_meta,
+    load_session_meta,
     repoint_voice_person,
     write_session_meta,
 )
@@ -166,7 +166,7 @@ async def api_session_voice_mapping(session: str, req: Request, recorder: Record
     else:
         person_id = ""
 
-    mapping = dict(read_session_meta(session).get("voices") or {})
+    mapping = dict(load_session_meta(session).get("voices") or {})
     if person_id:
         mapping[key] = {"person_id": person_id, "run_id": entry["run_id"]}
     else:

tapscribe/session_maintenance.py

@@ -32,7 +32,7 @@
 import tapscribe.voices as voices
 
 from . import config, tap_registry
-from .roster import read_roster
+from .roster import load_roster
 from .session_paths import (
     DIRNAME_STRIPPED,
     FILENAME_ROSTER_JSON,
@@ -43,7 +43,7 @@
     resolve_wav,
     stripped_dir,
 )
-from .sessions import read_session_meta, write_session_meta
+from .sessions import load_session_meta, write_session_meta
 from .tap_mode import TAP_MODE_MULTI
 
 # Re-exported from tap_registry (the canonical home, #405). These names must
@@ -124,9 +124,12 @@ def session_is_empty(session_dir: Path) -> bool:
         return False
     if (session_dir / FILENAME_TRANSCRIPT_JSON).exists():
         return False
-    if read_session_meta(session_dir.name).get("label"):
+    try:
+        meta = load_session_meta(session_dir.name)
+    except OSError:
+        # A meta this cannot read may hold a label, and prune deletes on True.
         return False
-    return True
+    return not meta.get("label")
 
 
 def _iter_candidate_session_dirs(current_session: str) -> Iterator[Path]:
@@ -323,6 +326,18 @@ def absorb_session(target: str, source: str) -> dict[str, Any]:
     if collisions:
         raise AbsorbCollision(f"filename collision(s) between sessions: {', '.join(collisions[:5])}")
 
+    # Every fold input is read strictly, and only here, before the first move:
+    # a file absorb cannot read stops it before anything moves, and no read
+    # after the moves can leave the merge half-applied (#446).
+    tgt_meta = load_session_meta(target)
+    src_meta = load_session_meta(source)
+    src_roster = load_roster(source_dir)
+    tgt_roster = load_roster(target_dir)
+    src_voices = voices.load_voices(source_dir)
+    tgt_voices = voices.load_voices(target_dir)
+    src_strip_meta = strip_meta.load_strip_meta(src_stripped_dir)
+    tgt_strip_meta = strip_meta.load_strip_meta(tgt_stripped_dir)
+
     moved_wavs: list[str] = []
     moved_stripped: list[str] = []
 
@@ -349,26 +364,20 @@ def absorb_session(target: str, source: str) -> dict[str, Any]:
         # clash; target wins anyway). Knobs/stripped_at keep the TARGET's
         # values when both sides have a meta — they describe the target's
         # own last run; a target without a meta adopts the source's wholesale.
-        src_strip_meta = strip_meta.read_strip_meta(src_stripped_dir)
         if src_strip_meta is not None:
-            tgt_strip_meta = strip_meta.read_strip_meta(tgt_stripped_dir)
             if tgt_strip_meta is not None:
                 tgt_strip_meta["files"] = {**src_strip_meta["files"], **tgt_strip_meta["files"]}
             strip_meta.write_strip_meta(tgt_stripped_dir, tgt_strip_meta or src_strip_meta)
 
     # Carry the source's Voices across, or the rmtree below destroys them while
     # the WAVs they describe live on in the target. An identity on both sides is
     # dropped from both: each session's Voice `A` is a different human (ADR-0021).
-    src_voices = voices.read_voices(source_dir)
-    tgt_voices = voices.read_voices(target_dir)
     voices_merged, collided = voices.fold_voices(tgt_voices, src_voices)
     if src_voices:
         voices.write_voices(target_dir, voices_merged)
 
     # Merge speaker aliases. Target wins on conflict; source fills in keys
     # the target doesn't already have. Target's label is preserved as-is.
-    tgt_meta = read_session_meta(target)
-    src_meta = read_session_meta(source)
     src_aliases = src_meta.get("aliases") or {}
     tgt_aliases = dict(tgt_meta.get("aliases") or {})
     aliases_added: list[str] = []
@@ -399,6 +408,7 @@ def _lives(key: str) -> bool:
                 "aliases": tgt_aliases,
                 "voices": tgt_voice_map,
             },
+            base=tgt_meta,
         )
 
     # Carry the source's Roster into the target's. The Roster is the ONLY
@@ -414,10 +424,8 @@ def _lives(key: str) -> bool:
     # the scalar fields — EXCEPT `wavs`, which unions: the source's WAVs now
     # physically live in the target, so the target's entry has to account for
     # them.
-    src_roster = read_roster(source_dir)
     roster_merged = 0
     if src_roster:
-        tgt_roster = read_roster(target_dir)
         for identity, src_entry in src_roster.items():
             tgt_entry = tgt_roster.get(identity)
             if tgt_entry is None:

tapscribe/sessions.py

@@ -35,6 +35,7 @@
 
 from . import config
 from .audio import wav_duration_s
+from .config_store import read_json_strict
 from .name_resolution import DEFAULT_KNOWN_NAMES_LIMIT, known_names_from, resolve_session_names
 from .people import PeopleRegistry
 from .roster import coerce_roster, read_roster
@@ -127,7 +128,7 @@ def _coerce_voices(value: Any) -> dict[str, dict[str, str]]:
 def _coerce_session_meta(raw: Any) -> dict[str, Any]:
     """Coerce a raw session-meta dict into the standard shape: string-field
     projection, alias coercion, language normalisation. Shared by
-    `read_session_meta` (the uncached write-path caller) and the cached
+    `load_session_meta` (the strict write-path caller) and the cached
     path in `_describe_session` so both produce the identical result."""
     if not isinstance(raw, dict):
         return {}
@@ -148,20 +149,31 @@ def _coerce_session_meta(raw: Any) -> dict[str, Any]:
 
 def _read_roster_cached(sd: Path) -> dict[str, dict[str, Any]]:
     """session-roster.json through the stat-sig cache, coerced via the shared
-    `roster.coerce_roster` so the cached poll path and the uncached `read_roster`
+    `roster.coerce_roster` so the cached poll path and the uncached `load_roster`
     write path produce the identical shape. {} on None/non-dict."""
     return coerce_roster(_read_session_json_cached(sd / FILENAME_ROSTER_JSON))
 
 
+def load_session_meta(session: str) -> dict[str, Any]:
+    """The per-session meta, read STRICTLY for merge-on-write callers: a failed
+    read RAISES rather than reading as `{}`, so a write never starts from empty
+    over a file it merely could not see (#446). Missing or torn → `{}`."""
+    return _coerce_session_meta(_read_json_strict(session_meta_path(session)))
+
+
 def read_session_meta(session: str) -> dict[str, Any]:
     """Return the per-session metadata dict: operator-editable display
     label, speaker aliases, and per-session batch prompt/hotwords
     overrides. Missing or unreadable → {} (caller can treat as no
-    overrides). Non-string fields are dropped silently."""
-    return _coerce_session_meta(_read_json_or_none(session_meta_path(session)))
+    overrides). Non-string fields are dropped silently. Merge-on-write
+    callers use `load_session_meta`, which raises on a failed read."""
+    try:
+        return load_session_meta(session)
+    except OSError:
+        return {}
 
 
-def write_session_meta(session: str, meta: dict[str, Any]) -> None:
+def write_session_meta(session: str, meta: dict[str, Any], *, base: dict[str, Any] | None = None) -> None:
     """Persist the per-session meta. Partial updates (e.g. only
     `{"prompt": "..."}`) preserve existing fields the caller didn't
     mention — otherwise editing one field would clear the others.
@@ -173,9 +185,15 @@ def write_session_meta(session: str, meta: dict[str, Any]) -> None:
 
     Atomic via `atomic_write_text` so a crashed write never leaves a
     torn JSON file (which `_read_json_or_none` would silently swallow,
-    losing the operator's label + aliases + overrides all at once)."""
+    losing the operator's label + aliases + overrides all at once). The base
+    read is strict (`load_session_meta`), so a failed read RAISES instead of
+    merging onto `{}` and overwriting the fields it could not see (#446).
+
+    `base` is a meta the caller already read with `load_session_meta`. Absorb
+    passes it, so no read runs after its WAV moves and a read fault cannot
+    leave the merge half-applied."""
     session_dir = create_session_dir(session)
-    existing = read_session_meta(session)
+    existing = load_session_meta(session) if base is None else base
     allowed = {"aliases", "languages", "voices", *_META_STRING_FIELDS}
     merged = {**existing, **{k: v for k, v in meta.items() if k in allowed}}
     sanitized = {k: merged[k] if isinstance(merged.get(k), str) else "" for k in _META_STRING_FIELDS}
@@ -229,7 +247,9 @@ def repoint_voice_person(old_person_id: str, new_person_id: str) -> list[str]:
     for sd in sorted(config.RECORDINGS_DIR.glob("*")):
         if not sd.is_dir():
             continue
-        raw = _read_json_or_none(sd / FILENAME_META_JSON)
+        # Strict: a meta this cannot read may name the old id, and the caller
+        # deletes that Person next, so an unreadable meta aborts the merge.
+        raw = _read_json_strict(sd / FILENAME_META_JSON)
         mapping = _coerce_voices(raw.get("voices") if isinstance(raw, dict) else None)
         if not any(entry["person_id"] == old_person_id for entry in mapping.values()):
             continue
@@ -330,8 +350,8 @@ def read_session_transcript(session: str) -> dict[str, Any] | None:
     session has no merged transcript. Backs `GET /api/sessions/{session}/
     transcript`. `session` is validated against path traversal by
     `resolve_session_dir` (the canonical CodeQL realpath sanitiser); the file
-    is read through `_read_json_or_none`, which re-checks containment so static
-    analysis sees the guard at the point of file access."""
+    is read through `_read_json_or_none`, whose strict core re-checks containment
+    so static analysis sees the guard at the point of file access."""
     session_dir = resolve_session_dir(session)
     data = _read_json_or_none(session_dir / FILENAME_TRANSCRIPT_JSON)
     return data if isinstance(data, dict) else None
@@ -341,8 +361,8 @@ def read_session_summary(session: str) -> dict[str, Any] | None:
     """The FULL persisted session-summary.json for `session`, or None when the
     session has never been summarized. Backs `GET /api/sessions/{session}/
     summary`. Same path-safety shape as `read_session_transcript`:
-    `resolve_session_dir` validates traversal, `_read_json_or_none` re-checks
-    containment at the point of file access."""
+    `resolve_session_dir` validates traversal, and the strict core of
+    `_read_json_or_none` re-checks containment at the point of file access."""
     session_dir = resolve_session_dir(session)
     data = _read_json_or_none(session_dir / FILENAME_SUMMARY_JSON)
     return data if isinstance(data, dict) else None
@@ -401,36 +421,33 @@ def read_wav_strip_meta(session: str, name: str) -> dict[str, Any] | None:
 # ---------------------------------------------------------------------------
 
 
-def _read_json_or_none(path: Path) -> Any:
-    """Parse `path` as JSON. Returns None when the file is missing,
-    unparseable, or sits outside RECORDINGS_DIR — `gather_sessions`
-    tolerates per-WAV transcripts going stale without breaking the
-    dashboard listing.
-
-    The containment check is defense-in-depth: every caller already
-    passes a path derived from a validated session, but this second
-    layer makes the safety property local and visible to static
-    analysis, so a future refactor that bypasses the route-level
-    validation can't silently leak the function as an arbitrary
-    file-reader."""
-    # Inline the realpath + startswith sanitiser (canonical CodeQL
-    # `py/path-injection` form) so taint analysis sees the check at the
-    # point of file access. Use the realpath string `real` directly in
-    # subsequent os.path.* and open() calls — CodeQL flows the sanitiser
-    # property through the `real` variable but not through a re-wrapped Path.
+def _read_json_strict(path: Path) -> Any:
+    """Parse `path` as JSON, separating "nothing here" from "I could not read
+    it". Missing, not a regular file, torn, or outside RECORDINGS_DIR → None.
+    Every other `OSError` (EACCES, EIO, EMFILE) raises (#446).
+
+    The containment check sits at the point of file access (the canonical
+    CodeQL `py/path-injection` form): the realpath string `real` flows straight
+    into `read_json_strict`, so taint analysis sees the guard there. Every
+    caller already passes a `session_paths`-resolved dir, so it is defence in
+    depth, and keeping it local stops a refactor that bypasses route-level
+    validation from turning this into an arbitrary file reader."""
     root = os.path.realpath(config.RECORDINGS_DIR)
     try:
         real = os.path.realpath(path)
-    except (OSError, ValueError):
+    except ValueError:
+        # An embedded NUL makes the path malformed, not unreadable.
         return None
     if real != root and not real.startswith(root + os.sep):
         return None
-    if not os.path.isfile(real):
-        return None
+    return read_json_strict(real)
+
+
+def _read_json_or_none(path: Path) -> Any:
+    """`_read_json_strict` for the poll and display reads: any `OSError` → None."""
     try:
-        with open(real, encoding="utf-8") as fh:
-            return json.load(fh)
-    except (OSError, ValueError):
+        return _read_json_strict(path)
+    except OSError:
         return None