Fix 4 of 7 edit-protection residuals from PR #11's review

docs/07-annotation-model.md

@@ -610,6 +610,19 @@ holds at the document layer too), and that append to the op-log.
   pitfall): **WebVTT chapters**, **YouTube chapter block**, W3C Web Annotation, and `.annot`.
   The first three are ~20 lines each and prove the AST/renderer split immediately.
 
+### 6.6 Edit-protection residuals (issue #12)
+
+PR #11's three adversarial review rounds surfaced seven residuals, tracked as `thorwhalen/paces#12`. Four are fixed (`paces/model.py`, `paces/edits.py`):
+
+- `Lock.by` must be non-empty; `Lock.at` must match the ISO-8601 UTC second-resolution format `apply_edits` itself writes. Both previously accepted any string.
+- `validate_document` now flags any raw `float` reaching the wire outside a schema-typed float field (`confidence`) — closing the `Lock.was`/`attrs` hole described in §6.5, since both are typed `Any` and the schema alone can't catch a float landing in either.
+- `apply_edits` refuses renaming a step's `id` onto an id another step in the same list already uses, instead of accepting it and leaving `validate_document` to flag the resulting duplicate after the fact.
+- `merge_regenerated` no longer resurrects the pre-rename id: a step's own `/id` lock records the rename (`was` = old id), and a fresh projection that still emits that old id — because analysis has no knowledge of the rename — is matched to the renamed committed step instead of being merged in as an unrelated new one.
+
+Three are declined; see §7 for the one-line reasons: optional `SourceSpan.id` (the identity-swap fix — needs a schema change, out of scope here), the `Lock.path` camelCase/snake_case mismatch (documented instead, below), and the selection-boost scoring gap (the issue itself already defers it).
+
+**`Lock.path` spelling.** `Lock.path` segments are recorded in **snake_case** (`/spans/0/some_field`), matching Python attribute names — not the wire's camelCase. A consumer resolving a `Lock.path` against the *wire* JSON (camelCase keys) must convert each field segment with the same `alias_generator=to_camel` the model uses (`pydantic.alias_generators.to_camel`) before indexing into it. List-item segments (an id or an index) need no conversion. This is a documented convention, not a bug: changing what gets *recorded* would be an on-disk format change for every already-written lock, which issue #12 was scoped to avoid.
+
 ---
 
 ## 7. What I deliberately left out, and why
@@ -627,6 +640,8 @@ holds at the document layer too), and that append to the op-log.
 | **A `kind`/`type` discriminated union on `Step`** | the POC needed exactly one step type. `Beat`-like non-content steps (a title card, a rest) can be `Step(tags=["rest"], spans=[])`. Add the union at the third real variant, not the first. |
 | **`TimeInterval` on the document's spans** | `{"start":{"v":231300,"r":1000},"end":…}` is correct and unreadable. Decimal strings on the wire, `RationalTime` in memory, one converter in the loader. Inside the lacing store, rational all the way. |
 | **A DSL / Markdown authoring layer** | `an/ir/sync.py` proves it can be done and its bidirectional round-trip is the riskiest code in that package. JSON + typed patches first; add a Markdown face only after the JSON contract has survived two renderers. |
+| **Optional `SourceSpan.id`** (issue #12) | the real cure for the one residual mis-identification case a structural match can't beat (identity-swapping content: a locked span whose start moves while a same-`(source, role)` sibling assumes its exact old start). Fits #4's evidence-layer timeline (store-side identity adoption wants ids anyway) — add it there, as a schema change, not as an unscoped addition here. |
+| **Refusing selection-boost scoring on fact presence vs. usability** (issue #12) | boundaries + N names + N matching chapters can pick `explicit` (unnamed spans + mismatch flag) where `chapters` would have named everything — honestly flagged today (the mismatch flag says so), never silently wrong. Revisiting the scoring function meaningfully needs a third real case to generalize from; the issue itself defers this to when a fourth segmenter lands. |
 
 ---
 

paces/edits.py

@@ -34,6 +34,16 @@
   edit does not survive; only its lock record does.
 - ``attrs`` bags merge committed-over-fresh per key: they are user/renderer
   data that analysis does not produce, so regeneration never wins there.
+- Renaming a list item's ``id`` onto an id already used by another item in
+  the same list is refused at edit time (previously accepted, and only
+  flagged later by :func:`~paces.model.validate_document`) — issue #12.
+- A step rename (its own ``/id`` lock) is remembered by :func:`merge_regenerated`:
+  a fresh projection that still emits the pre-rename id (because analysis
+  does not know about the rename) is matched to the renamed committed step
+  instead of being treated as a new, unrelated step — otherwise the old id
+  resurfaces alongside the renamed one. This does not follow renames inside
+  a merge across multiple regenerations; only the most recent ``/id`` lock is
+  consulted (issue #12).
 
 Not yet recorded anywhere: the fresh values a merge *rejects*
 (``Origin.value_digest`` and the op-log arrive with the evidence layer,
@@ -129,6 +139,36 @@ def _resolve_parent(root: Any, segments: list[str], *, path: str):
     raise ValueError(f"{path}: cannot set into {type(node).__name__}")
 
 
+def _all_steps(steps: list) -> list:
+    """Every step in the tree, depth-first — matching the scope of
+    :func:`~paces.model.validate_document`'s step-id uniqueness check, which
+    is document-wide, not per-sibling-list."""
+    out = []
+    for step in steps:
+        if isinstance(step, Mapping):
+            out.append(step)
+            out.extend(_all_steps(step.get("steps", [])))
+    return out
+
+
+def _containing_list(dump: dict, segments: list[str]) -> list | None:
+    """The list holding the item whose own field the leaf segment addresses
+    (e.g. for ``/steps/a/id``, the ``steps`` list a and its siblings live in),
+    or ``None`` when the leaf isn't a field on a list item."""
+    if len(segments) < 2:
+        return None
+    node: Any = dump
+    for i, segment in enumerate(segments[:-2]):
+        at = "/" + "/".join(segments[: i + 1])
+        if isinstance(node, list):
+            node = node[_index_of(node, segment, at=at)]
+        elif isinstance(node, Mapping):
+            node = node[_key_of(node, segment, at=at)]
+        else:
+            return None
+    return node if isinstance(node, list) else None
+
+
 def _canonical_segments(root: Any, segments: list[str], *, path: str) -> list[str]:
     """The stablest spelling of a path: snake_case fields; list items by id
     when one exists unambiguously in that list, by index otherwise."""
@@ -214,6 +254,33 @@ def apply_edits(
         raw_path = edit.get("path", "")
         segments = _canonical_segments(dump, _split_path(raw_path), path=raw_path)
         container, key = _resolve_parent(dump, segments, path=raw_path)
+        if key == "id" and isinstance(container, Mapping):
+            new_id = edit["value"]
+            all_steps = _all_steps(dump.get("steps", []))
+            is_step = any(s is container for s in all_steps)
+            # A step's id is unique document-wide (validate_document enforces
+            # this across the whole tree, not per sibling list) — check the
+            # same scope here. Everything else (sources, cues, questions)
+            # only needs uniqueness within its own list.
+            candidates = (
+                all_steps if is_step else (_containing_list(dump, segments) or [])
+            )
+            collision = next(
+                (
+                    candidate
+                    for candidate in candidates
+                    if isinstance(candidate, Mapping)
+                    and candidate is not container
+                    and candidate.get("id") == new_id
+                ),
+                None,
+            )
+            if collision is not None:
+                scope = "the document" if is_step else "the same list"
+                raise ValueError(
+                    f"edits[{i}]: cannot rename id to {new_id!r} — already used "
+                    f"by another item in {scope}"
+                )
         # The lock site must resolve BEFORE the mutation: an edit may change
         # the very value a segment addresses (renaming a step's id).
         site, relative = _lock_site(dump, segments)
@@ -352,14 +419,47 @@ def _merged_attrs(committed: Mapping, fresh: Mapping) -> dict:
     return {**copy.deepcopy(dict(fresh)), **copy.deepcopy(dict(committed))}
 
 
+def _renamed_from(committed: list[dict]) -> dict[str, dict]:
+    """old id -> committed step, for steps whose own ``/id`` lock records a
+    rename (``apply_edits`` writes exactly this). Analysis that re-runs
+    without knowledge of the rename still emits the old id — without this,
+    that fresh step would be treated as unrelated and the old id would
+    resurface alongside the renamed one (issue #12)."""
+    out: dict[str, dict] = {}
+    for step in committed:
+        for lock in step.get("locks", []):
+            if lock.get("path") == "/id" and lock.get("was") is not None:
+                out[str(lock["was"])] = step
+    return out
+
+
 def _merge_steps(committed: list[dict], fresh: list[dict]) -> list[dict]:
     committed_by_id: dict[str, dict] = {}
     for step in committed:  # first occurrence wins, matching apply_edits
         committed_by_id.setdefault(step["id"], step)
+    renamed_from = _renamed_from(committed)
     fresh_ids = {step["id"] for step in fresh}
+
+    # Direct id matches are resolved FIRST and claim their committed step
+    # before any rename substitution runs: if fresh already carries the
+    # renamed id, that's the real match, and a stray fresh entry still using
+    # the old id (analysis emitting both, or simply unrelated) must not also
+    # claim the same committed step — that would merge one committed step
+    # into two output entries with the same id (issue #12, blocking review).
+    matched_committed_ids: set[str] = {
+        committed_by_id[fresh_step["id"]]["id"]
+        for fresh_step in fresh
+        if fresh_step["id"] in committed_by_id
+    }
+
     merged: list[dict] = []
     for fresh_step in fresh:
         committed_step = committed_by_id.get(fresh_step["id"])
+        if committed_step is None:
+            candidate = renamed_from.get(fresh_step["id"])
+            if candidate is not None and candidate["id"] not in matched_committed_ids:
+                committed_step = candidate
+                matched_committed_ids.add(candidate["id"])
         if committed_step is None:
             merged.append(copy.deepcopy(fresh_step))
             continue
@@ -378,7 +478,10 @@ def _merge_steps(committed: list[dict], fresh: list[dict]) -> list[dict]:
     # Committed-only steps survive when they carry protection (locks anywhere
     # in their subtree, or a user origin); analysis leftovers are superseded.
     for position, committed_step in enumerate(committed):
-        if committed_step["id"] in fresh_ids:
+        if (
+            committed_step["id"] in fresh_ids
+            or committed_step["id"] in matched_committed_ids
+        ):
             continue
         subtree_protected = _is_protected(committed_step) or any(
             _is_protected(child) for child in _walk_dumps(committed_step)

paces/model.py

@@ -24,6 +24,8 @@
 
 from __future__ import annotations
 
+from collections.abc import Mapping
+from datetime import datetime
 from fractions import Fraction
 from typing import Annotated, Any, Literal
 
@@ -169,6 +171,12 @@ class Cue(_Base):
 # ── provenance & edit protection ────────────────────────────────────────────
 
 
+#: ``Lock.at`` is always written by :func:`paces.edits._now_iso`; this is that
+#: same format, enforced on the way in so a hand-authored ``at`` can't smuggle
+#: an unparseable timestamp onto the wire.
+_LOCK_AT_FORMAT = "%Y-%m-%dT%H:%M:%SZ"
+
+
 class Lock(_Base):
     """A human (or approved-AI) decision that regeneration MUST NOT overwrite."""
 
@@ -178,6 +186,25 @@ class Lock(_Base):
     was: Any | None = None  # the pre-edit value — makes the edit reversible
     reason: str | None = None
 
+    @field_validator("by")
+    @classmethod
+    def _by_must_be_attributed(cls, value: str) -> str:
+        if not value.strip():
+            raise ValueError("Lock.by must not be empty — who made this edit?")
+        return value
+
+    @field_validator("at")
+    @classmethod
+    def _at_must_be_iso_utc(cls, value: str) -> str:
+        try:
+            datetime.strptime(value, _LOCK_AT_FORMAT)
+        except ValueError as e:
+            raise ValueError(
+                f"Lock.at must be ISO-8601 UTC, second resolution "
+                f"('2026-08-30T00:00:00Z'); got {value!r}"
+            ) from e
+        return value
+
 
 class Origin(_Base):
     """Back-reference into the evidence layer: why this node says what it says,
@@ -346,16 +373,73 @@ def _resolve_steps(steps: list[Step], at: float) -> tuple[list[dict], float]:
     }
 
 
+#: Field names whose schema type is deliberately ``float`` (``docs/07 §6.5``
+#: accepts this as a pre-existing hole in the no-floats-on-the-wire rule) —
+#: everywhere else, a Python ``float`` reaching the wire (typically through an
+#: ``attrs`` bag or a ``Lock.was``, both typed ``Any``) is a leak. (A float
+#: under an ``attrs`` key that happens to be literally named ``confidence``
+#: slips through this same allowlist — a narrow, accepted gap: distinguishing
+#: it would mean knowing ``attrs`` is user data everywhere it appears.)
+_FLOAT_TYPED_KEYS = frozenset({"confidence"})
+
+#: The keys a ``Lock`` always dumps as (``model_dump(mode="python")``, no
+#: ``exclude_none``) — used to recognise a Lock structurally so ``was`` can be
+#: checked against what ``path`` says it actually holds, not the literal key
+#: name ``"was"`` (a ``Lock`` locking a ``confidence`` field legitimately
+#: carries a float in ``was``).
+_LOCK_DUMP_KEYS = frozenset({"path", "by", "at", "was", "reason"})
+
+
+def _scan_floats(node: Any, path: str, *, key: str | None = None) -> list[str]:
+    """Paths where a raw ``float`` reaches the wire outside a typed field."""
+    if isinstance(node, float):
+        if key in _FLOAT_TYPED_KEYS:
+            return []
+        return [
+            f"{path}: float value {node!r} on the no-floats wire (use a decimal string)"
+        ]
+    if isinstance(node, Mapping):
+        if set(node) == _LOCK_DUMP_KEYS and isinstance(node.get("path"), str):
+            was_key = node["path"].rsplit("/", 1)[-1]
+            return [
+                issue
+                for k, v in node.items()
+                for issue in _scan_floats(
+                    v, f"{path}/{k}", key=was_key if k == "was" else k
+                )
+            ]
+        return [
+            issue
+            for k, v in node.items()
+            for issue in _scan_floats(v, f"{path}/{k}", key=k)
+        ]
+    if isinstance(node, (list, tuple)):
+        return [
+            issue
+            for i, v in enumerate(node)
+            for issue in _scan_floats(
+                v,
+                f"{path}/{v['id']}"
+                if isinstance(v, Mapping) and "id" in v
+                else f"{path}/{i}",
+                key=key,
+            )
+        ]
+    return []
+
+
 def validate_document(doc: StepDocument) -> list[str]:
     """Semantic checks beyond the schema. Returns human-readable issues
     (empty list = clean); never raises.
 
     Checks: children durations account for the parent's (``repeat`` included),
     span sources exist, cue anchors point at real steps, step ids are unique
     (id-addressed edits and the regeneration merge both key on them — a
-    duplicate makes those silently ambiguous).
+    duplicate makes those silently ambiguous), and no raw ``float`` has
+    leaked onto the no-floats wire through an ``attrs`` bag or a ``Lock.was``
+    (both typed ``Any``, so the schema alone cannot catch this).
     """
-    issues: list[str] = []
+    issues: list[str] = _scan_floats(doc.model_dump(mode="python", by_alias=False), "")
     source_ids = {s.id for s in doc.sources}
     step_ids: set[str] = set()